[spice-streaming-agent,3/3] Externalize plugins usage

Submitted by Christophe de Dinechin on Nov. 14, 2017, 2:49 p.m.

Details

Message ID 20171114144943.19816-4-christophe@dinechin.org
State New
Headers show
Series "Rework option handling" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Nov. 14, 2017, 2:49 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/spice-streaming-agent/plugin.hpp |  6 ++++++
 src/concrete-agent.cpp                   | 11 +++++++++++
 src/concrete-agent.hpp                   |  1 +
 src/spice-streaming-agent.cpp            | 23 +++++++++++------------
 4 files changed, 29 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
index 41ad11f..83980d7 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -56,6 +56,12 @@  struct Settings
     unsigned    quality         =      80; // Normalized in 0-100 (100=high)
     unsigned    avg_bitrate     = 3000000; // Target average bitrate in bps
     unsigned    max_bitrate     = 8000000; // Target maximum bitrate
+
+#define STANDARD_OPTIONS_USAGE                                          \
+    "framerate  = [1-240]       Number of frames per second\n"          \
+    "quality    = [1-100]       Normalized quality, 100 = high\n"       \
+    "avg_bitrate= [1-10000000]  Average bits per second for stream\n"   \
+    "max_bitrate= [1-10000000]  Maximum bits per second for stream\n"
 };
 
 /*!
diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 59f11b2..377c934 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -140,6 +140,17 @@  FrameCapture *ConcreteAgent::GetBestFrameCapture()
     return nullptr;
 }
 
+void ConcreteAgent::PluginsUsage()
+{
+    for (auto &plugin: plugins) {
+        printf("\n"
+               "settings for %s:\n"
+               "%s",
+               plugin->Name(),
+               plugin->Usage());
+    }
+}
+
 void ConcreteAgent::ApplyOptions(Plugin *plugin)
 {
     bool usage = false;
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index b3d4e06..eeb43f8 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -34,6 +34,7 @@  public:
     void LoadPlugins(const char *directory);
     void ApplyOptions(Plugin *plugin);
     FrameCapture *GetBestFrameCapture();
+    void PluginsUsage();
 
 private:
     void LoadPlugin(const char *plugin_filename);
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index d5984bc..71a36e1 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -269,18 +269,16 @@  static void register_interrupts(void)
 
 static void usage(const char *progname)
 {
-    printf("usage: %s <options>\n", progname);
-    printf("options are:\n");
-    printf("\t-p portname  -- virtio-serial port to use\n");
-    printf("\t-i accept commands from stdin\n");
-    printf("\t-l file -- log frames to file\n");
-    printf("\t--log-binary -- log binary frames (following -l)\n");
-    printf("\t-d -- enable debug logs\n");
-    printf("\t-c variable=value -- change settings\n");
-    printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
-    printf("\n");
-    printf("\t-h or --help     -- print this help message\n");
-
+    printf("usage: %s <options>\n"
+           "options are:\n"
+           "\t-p portname  -- virtio-serial port to use\n"
+           "\t-i accept commands from stdin\n"
+           "\t-l file -- log frames to file\n"
+           "\t--log-binary -- log binary frames (following -l)\n"
+           "\t-d -- enable debug logs\n"
+           "\t-c variable=value -- change settings (see below)\n"
+           "\t-h or --help     -- print this help message\n", progname);
+    agent.PluginsUsage();
     exit(1);
 }
 
@@ -474,6 +472,7 @@  int main(int argc, char* argv[])
             setlogmask(logmask);
             break;
 	case 'h':
+            agent.LoadPlugins(PLUGINSDIR);
 	    usage(argv[0]);
 	    break;
 	}

Comments

> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/spice-streaming-agent/plugin.hpp |  6 ++++++
>  src/concrete-agent.cpp                   | 11 +++++++++++
>  src/concrete-agent.hpp                   |  1 +
>  src/spice-streaming-agent.cpp            | 23 +++++++++++------------
>  4 files changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index 41ad11f..83980d7 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -56,6 +56,12 @@ struct Settings
>      unsigned    quality         =      80; // Normalized in 0-100 (100=high)
>      unsigned    avg_bitrate     = 3000000; // Target average bitrate in bps
>      unsigned    max_bitrate     = 8000000; // Target maximum bitrate
> +
> +#define STANDARD_OPTIONS_USAGE                                          \
> +    "framerate  = [1-240]       Number of frames per second\n"          \
> +    "quality    = [1-100]       Normalized quality, 100 = high\n"       \
> +    "avg_bitrate= [1-10000000]  Average bits per second for stream\n"   \
> +    "max_bitrate= [1-10000000]  Maximum bits per second for stream\n"
>  };
>  
>  /*!

Why putting this in the public header?
I don't see any point for the plugins to know that information and
if they starts using it the value can become an ABI.
Also these defines are global (not inside any namespace) so the
"STANDARD_OPTIONS_USAGE" would prevent the name reuse.

> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 59f11b2..377c934 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -140,6 +140,17 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>      return nullptr;
>  }
>  
> +void ConcreteAgent::PluginsUsage()
> +{
> +    for (auto &plugin: plugins) {
> +        printf("\n"
> +               "settings for %s:\n"
> +               "%s",
> +               plugin->Name(),
> +               plugin->Usage());
> +    }
> +}
> +
>  void ConcreteAgent::ApplyOptions(Plugin *plugin)
>  {
>      bool usage = false;
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index b3d4e06..eeb43f8 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -34,6 +34,7 @@ public:
>      void LoadPlugins(const char *directory);
>      void ApplyOptions(Plugin *plugin);
>      FrameCapture *GetBestFrameCapture();
> +    void PluginsUsage();
>  
>  private:
>      void LoadPlugin(const char *plugin_filename);
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index d5984bc..71a36e1 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -269,18 +269,16 @@ static void register_interrupts(void)
>  
>  static void usage(const char *progname)
>  {
> -    printf("usage: %s <options>\n", progname);
> -    printf("options are:\n");
> -    printf("\t-p portname  -- virtio-serial port to use\n");
> -    printf("\t-i accept commands from stdin\n");
> -    printf("\t-l file -- log frames to file\n");
> -    printf("\t--log-binary -- log binary frames (following -l)\n");
> -    printf("\t-d -- enable debug logs\n");
> -    printf("\t-c variable=value -- change settings\n");
> -    printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> -    printf("\n");
> -    printf("\t-h or --help     -- print this help message\n");
> -
> +    printf("usage: %s <options>\n"
> +           "options are:\n"
> +           "\t-p portname  -- virtio-serial port to use\n"
> +           "\t-i accept commands from stdin\n"
> +           "\t-l file -- log frames to file\n"
> +           "\t--log-binary -- log binary frames (following -l)\n"
> +           "\t-d -- enable debug logs\n"
> +           "\t-c variable=value -- change settings (see below)\n"
> +           "\t-h or --help     -- print this help message\n", progname);
> +    agent.PluginsUsage();
>      exit(1);
>  }
>  
> @@ -474,6 +472,7 @@ int main(int argc, char* argv[])
>              setlogmask(logmask);
>              break;
>  	case 'h':
> +            agent.LoadPlugins(PLUGINSDIR);
>  	    usage(argv[0]);
>  	    break;
>  	}

Frediano
> On 15 Nov 2017, at 12:15, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> include/spice-streaming-agent/plugin.hpp |  6 ++++++
>> src/concrete-agent.cpp                   | 11 +++++++++++
>> src/concrete-agent.hpp                   |  1 +
>> src/spice-streaming-agent.cpp            | 23 +++++++++++------------
>> 4 files changed, 29 insertions(+), 12 deletions(-)
>> 
>> diff --git a/include/spice-streaming-agent/plugin.hpp
>> b/include/spice-streaming-agent/plugin.hpp
>> index 41ad11f..83980d7 100644
>> --- a/include/spice-streaming-agent/plugin.hpp
>> +++ b/include/spice-streaming-agent/plugin.hpp
>> @@ -56,6 +56,12 @@ struct Settings
>>     unsigned    quality         =      80; // Normalized in 0-100 (100=high)
>>     unsigned    avg_bitrate     = 3000000; // Target average bitrate in bps
>>     unsigned    max_bitrate     = 8000000; // Target maximum bitrate
>> +
>> +#define STANDARD_OPTIONS_USAGE                                          \
>> +    "framerate  = [1-240]       Number of frames per second\n"          \
>> +    "quality    = [1-100]       Normalized quality, 100 = high\n"       \
>> +    "avg_bitrate= [1-10000000]  Average bits per second for stream\n"   \
>> +    "max_bitrate= [1-10000000]  Maximum bits per second for stream\n"
>> };
>> 
>> /*!
> 
> Why putting this in the public header?

The initial idea was to make sure plugins were actually showing the
same message for common options. Thinking more about output
formatting, I realized it was stupid to emit the same usage info
for every plugin, so I am in the process of changing that.

> I don't see any point for the plugins to know that information and
> if they starts using it the value can become an ABI.
> Also these defines are global (not inside any namespace) so the
> "STANDARD_OPTIONS_USAGE" would prevent the name reuse.

You are right, and I found a better way to output the hierarchy of
usage information (and also offer a better format).



> 
>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
>> index 59f11b2..377c934 100644
>> --- a/src/concrete-agent.cpp
>> +++ b/src/concrete-agent.cpp
>> @@ -140,6 +140,17 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>>     return nullptr;
>> }
>> 
>> +void ConcreteAgent::PluginsUsage()
>> +{
>> +    for (auto &plugin: plugins) {
>> +        printf("\n"
>> +               "settings for %s:\n"
>> +               "%s",
>> +               plugin->Name(),
>> +               plugin->Usage());
>> +    }
>> +}
>> +
>> void ConcreteAgent::ApplyOptions(Plugin *plugin)
>> {
>>     bool usage = false;
>> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
>> index b3d4e06..eeb43f8 100644
>> --- a/src/concrete-agent.hpp
>> +++ b/src/concrete-agent.hpp
>> @@ -34,6 +34,7 @@ public:
>>     void LoadPlugins(const char *directory);
>>     void ApplyOptions(Plugin *plugin);
>>     FrameCapture *GetBestFrameCapture();
>> +    void PluginsUsage();
>> 
>> private:
>>     void LoadPlugin(const char *plugin_filename);
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index d5984bc..71a36e1 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -269,18 +269,16 @@ static void register_interrupts(void)
>> 
>> static void usage(const char *progname)
>> {
>> -    printf("usage: %s <options>\n", progname);
>> -    printf("options are:\n");
>> -    printf("\t-p portname  -- virtio-serial port to use\n");
>> -    printf("\t-i accept commands from stdin\n");
>> -    printf("\t-l file -- log frames to file\n");
>> -    printf("\t--log-binary -- log binary frames (following -l)\n");
>> -    printf("\t-d -- enable debug logs\n");
>> -    printf("\t-c variable=value -- change settings\n");
>> -    printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
>> -    printf("\n");
>> -    printf("\t-h or --help     -- print this help message\n");
>> -
>> +    printf("usage: %s <options>\n"
>> +           "options are:\n"
>> +           "\t-p portname  -- virtio-serial port to use\n"
>> +           "\t-i accept commands from stdin\n"
>> +           "\t-l file -- log frames to file\n"
>> +           "\t--log-binary -- log binary frames (following -l)\n"
>> +           "\t-d -- enable debug logs\n"
>> +           "\t-c variable=value -- change settings (see below)\n"
>> +           "\t-h or --help     -- print this help message\n", progname);
>> +    agent.PluginsUsage();
>>     exit(1);
>> }
>> 
>> @@ -474,6 +472,7 @@ int main(int argc, char* argv[])
>>             setlogmask(logmask);
>>             break;
>> 	case 'h':
>> +            agent.LoadPlugins(PLUGINSDIR);
>> 	    usage(argv[0]);
>> 	    break;
>> 	}
> 
> Frediano