[Spice-devel] [PATCH spice-streaming-agent 3/3] Externalize plugins usage

[Christophe de Dinechin]

> On 15 Nov 2017, at 12:15, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin at redhat.com>
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171115/c0d787e0/attachment-0001.html>