[spice-streaming-agent,v2,2/2] Rework option handling

Submitted by Christophe de Dinechin on Nov. 16, 2017, 10:10 a.m.

Details

Message ID 20171116101020.72028-3-christophe@dinechin.org
State New
Headers show
Series "Rework option handling" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Nov. 16, 2017, 10:10 a.m.
From: Christophe de Dinechin <dinechin@redhat.com>

The overall objective is to pave the way for features that will likely
become important, notably the ability to dynamically adjust quality of
service to respond to network conditions or client load issues.

Consequently, this patch cleans up the option-related aspects of the
plugin ABI and API as follows:

1. It adds standard settings that are shared across plugins, e.g. framerate.
   This will enable future QoS adjustments to be applied by the agent
   in a consistent way, irrespective of the actual capture plugin.

2. It makes the interface for option handling dynamic, so that the agent
   can in the future update settings without having to do expensive
   operations such as searching for pre-existing options.

3. It exposes a consistent way to deal witih settings values, so that
   error messages and acceptable values are consistent across plugins.
   The current interfaces only deals with integer values, possibly with
   a range of acceptable values, and possibly with a K/M suffix, e.g.
   to accept max_bitrate=100k.

4. It delegates usage information to the plugins, so that each plugin
   can document its specific options in a consistent way.

5. It cleans up some confusing or surprising aspects of the interace,
   notably null-terminated option vectors and the ownership of
   dynamically loaded plugins.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/spice-streaming-agent/plugin.hpp |  94 +++++++++++++++++---
 m4/spice-compile-warnings.m4             |   2 +
 src/concrete-agent.cpp                   | 143 ++++++++++++++++++++++++++++---
 src/concrete-agent.hpp                   |  34 ++++----
 src/mjpeg-fallback.cpp                   |  89 +++++++++----------
 src/spice-streaming-agent.cpp            |  23 +++--
 6 files changed, 284 insertions(+), 101 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
index 607fabf..6202d2c 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -8,6 +8,10 @@ 
 #define SPICE_STREAMING_AGENT_PLUGIN_HPP
 
 #include <spice/enums.h>
+#include <stdexcept>
+#include <vector>
+#include <climits>
+#include <cstdio>
 
 /*!
  * \file
@@ -20,6 +24,7 @@ 
 namespace SpiceStreamingAgent {
 
 class FrameCapture;
+class Agent;
 
 /*!
  * Plugin version, only using few bits, schema is 0xMMmm
@@ -42,31 +47,75 @@  enum Ranks : unsigned {
 };
 
 /*!
- * Configuration option.
- * An array of these will be passed to the plugin.
- * Simply a couple name and value passed as string.
- * For instance "framerate" and "20".
+ * Settings that are common to all plugins
  */
-struct ConfigureOption
+struct Settings
 {
+    unsigned    framerate       =      30; // Frames per second (1-240)
+    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
+};
+
+/*!
+ * Command line option
+ */
+struct Option
+{
+    Option(const char *name, const char *value): name(name), value(value) {}
     const char *name;
     const char *value;
 };
 
 /*!
+ * Error applying an option
+ */
+class OptionError : public std::runtime_error
+{
+public:
+    OptionError(const std::string &what): std::runtime_error(what) {}
+};
+
+/*!
+ * Usage information
+ */
+struct UsageInfo
+{
+    const char *name;
+    const char *range;
+    const char *description;
+};
+
+/*!
  * Interface a plugin should implement and register to the Agent.
  *
  * A plugin module can register multiple Plugin interfaces to handle
  * multiple codecs. In this case each Plugin will report data for a
  * specific codec.
+ *
+ * A plugin will typically extend the Settings struct to add its own
+ * settings.
  */
 class Plugin
 {
 public:
+    Plugin(Agent *agent): agent(agent) {}
+    
     /*!
      * Allows to free the plugin when not needed
      */
-    virtual ~Plugin() {};
+    virtual ~Plugin() { }
+
+    /*!
+     * Return the name of the plugin, e.g. for command-line usage information
+     */
+    virtual const char *Name() = 0;
+
+    /*!
+     * Return usage information for the plug-in, e.g. command-line options
+     * Usage is returned as a NULL-terminated array of UsageInfo
+     */
+    virtual const UsageInfo *Usage() = 0;
 
     /*!
      * Request an object for getting frames.
@@ -89,6 +138,22 @@  public:
      * Get video codec used to encode last frame
      */
     virtual SpiceVideoCodecType VideoCodecType() const=0;
+
+    /*!
+     * Return the concrete Settings for this plugin
+     */
+    virtual Settings &  PluginSettings() = 0;
+
+    /*!
+     * Apply a given option to the plugin settings.
+     * \return true if the option is valid, false if it is not recognized
+     * If there is an error applying the settings, set \arg error.
+     * If this returns false, agent will process standard options itself.
+     */
+    virtual bool ApplyOption(const Option &o) throw(OptionError) = 0;
+
+protected:
+    Agent *agent;
 };
 
 /*!
@@ -113,20 +178,23 @@  public:
      * Check if a given plugin version is compatible with this agent
      * \return true is compatible
      */
-    virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const=0;
+    virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const = 0;
 
     /*!
      * Register a plugin in the system.
+     * The agent then has shared ownership of the plugin, meaning that
+     * the plugin will be deleted when the agent is deleted unless another
+     * shared_ptr instance still holds it at that time.
      */
-    virtual void Register(Plugin& plugin)=0;
+    virtual void Register(Plugin *plugin)=0;
 
     /*!
-     * Get options array.
-     * Array is terminated with {nullptr, nullptr}.
-     * Never nullptr.
-     * \todo passing options to entry point instead?
+     * Helper to get integer values from command-line options
      */
-    virtual const ConfigureOption* Options() const=0;
+    virtual int IntOption(const Option &opt,
+                          int min = INT_MIN, int max = INT_MAX,
+                          bool sizeSuffixOK = false) const
+        throw(OptionError) = 0;
 };
 
 typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
diff --git a/m4/spice-compile-warnings.m4 b/m4/spice-compile-warnings.m4
index 66d7179..9e8bb6e 100644
--- a/m4/spice-compile-warnings.m4
+++ b/m4/spice-compile-warnings.m4
@@ -86,6 +86,8 @@  AC_DEFUN([SPICE_COMPILE_WARNINGS],[
     dontwarn="$dontwarn -Wpointer-sign"
     dontwarn="$dontwarn -Wpointer-to-int-cast"
     dontwarn="$dontwarn -Wstrict-prototypes"
+    dontwarn="$dontwarn -Wsuggest-final-types"
+    dontwarn="$dontwarn -Wsuggest-final-methods"
 
     # We want to enable these, but need to sort out the
     # decl mess with  gtk/generated_*.c
diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 51b4504..5c93e42 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -9,6 +9,10 @@ 
 #include <syslog.h>
 #include <glob.h>
 #include <dlfcn.h>
+#include <mutex>
+#include <climits>
+#include <sstream>
+#include <string>
 
 #include "concrete-agent.hpp"
 #include "static-plugin.hpp"
@@ -28,7 +32,11 @@  static inline unsigned MinorVersion(unsigned version)
 
 ConcreteAgent::ConcreteAgent()
 {
-    options.push_back(ConcreteConfigureOption(nullptr, nullptr));
+}
+
+void ConcreteAgent::Register(Plugin *plugin)
+{
+    plugins.push_back(shared_ptr<Plugin>(plugin));
 }
 
 bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
@@ -41,22 +49,83 @@  bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
         MinorVersion(version) >= MinorVersion(pluginVersion);
 }
 
-void ConcreteAgent::Register(Plugin& plugin)
+int ConcreteAgent::IntOption(const Option &opt,
+                             int min, int max, bool sizeSuffixOK) const
+    throw(OptionError)
 {
-    plugins.push_back(shared_ptr<Plugin>(&plugin));
+    std::string name = opt.name;
+    std::string value = opt.value;
+    std::istringstream input(value);
+    std::ostringstream err;
+
+    int result = 0;
+    input >> result;
+
+    if (!input.fail() && !input.eof() && sizeSuffixOK) {
+        std::string suffix;
+        input >> suffix;
+        bool ok = false;
+        if (!input.fail() && suffix.length() == 1) {
+            switch (suffix[0]) {
+            case 'b': case 'B': ok = true;                      break;
+            case 'k': case 'K': ok = true; result *= 1000;      break;
+            case 'm': case 'M': ok = true; result *= 1000000;   break;
+            default:                                            break;
+            }
+        }
+        if (!ok) {
+            err << "Unknown number suffix " << suffix
+                << " for " << name << "\n";
+            throw OptionError(err.str());
+        }
+    }
+
+    if (input.fail()) {
+        err << "Value " << value << " for " << name
+            << " is not a number\n";
+        throw OptionError(err.str());
+    }
+    if (!input.eof()) {
+        err << "Value " << value << " for " << name
+            << " does not end like an integer\n";
+        throw OptionError(err.str());
+    }
+    if (result < min || result > max) {
+        err << "The value " << value << " for " << name
+            << " is out of range (must be in " << min << ".." << max << ")\n";
+        throw OptionError(err.str());
+    }
+
+    return result;
 }
 
-const ConfigureOption* ConcreteAgent::Options() const
+bool ConcreteAgent::StandardOption(Settings &settings, const Option &option)
+    throw (OptionError)
 {
-    static_assert(sizeof(ConcreteConfigureOption) == sizeof(ConfigureOption),
-                  "ConcreteConfigureOption should be binary compatible with ConfigureOption");
-    return static_cast<const ConfigureOption*>(&options[0]);
+    string name = option.name;
+    if (name == "framerate" || name == "fps") {
+        settings.framerate = IntOption(option, 1, 240);
+        return true;
+    }
+    if (name == "quality") {
+        settings.quality = IntOption(option, 0, 100);
+        return true;
+    }
+    if (name == "avg_bitrate" || name == "bitrate") {
+        settings.avg_bitrate = IntOption(option, 10000, 32000000, true);
+        return true;
+    }
+    if (name == "max_bitrate") {
+        settings.max_bitrate = IntOption(option, 10000, 32000000, true);
+        return true;
+    }
+
+    return false;               // Unrecognized option
 }
 
 void ConcreteAgent::AddOption(const char *name, const char *value)
 {
-    // insert before the last {nullptr, nullptr} value
-    options.insert(--options.end(), ConcreteConfigureOption(name, value));
+    options.push_back(Option(name, value));
 }
 
 void ConcreteAgent::LoadPlugins(const char *directory)
@@ -84,7 +153,8 @@  void ConcreteAgent::LoadPlugin(const char *plugin_filename)
 {
     void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW);
     if (!dl) {
-        syslog(LOG_ERR, "error loading plugin %s", plugin_filename);
+        syslog(LOG_ERR, "error loading plugin %s: %s",
+               plugin_filename, dlerror());
         return;
     }
 
@@ -106,7 +176,7 @@  FrameCapture *ConcreteAgent::GetBestFrameCapture()
     vector<pair<unsigned, shared_ptr<Plugin>>> sorted_plugins;
 
     // sort plugins base on ranking, reverse order
-    for (const auto& plugin: plugins) {
+    for (auto& plugin: plugins) {
         sorted_plugins.push_back(make_pair(plugin->Rank(), plugin));
     }
     sort(sorted_plugins.rbegin(), sorted_plugins.rend());
@@ -116,6 +186,7 @@  FrameCapture *ConcreteAgent::GetBestFrameCapture()
         if (plugin.first == DontUse) {
             break;
         }
+        ApplyOptions(plugin.second.get());
         FrameCapture *capture = plugin.second->CreateCapture();
         if (capture) {
             return capture;
@@ -123,3 +194,53 @@  FrameCapture *ConcreteAgent::GetBestFrameCapture()
     }
     return nullptr;
 }
+
+void ConcreteAgent::PluginsUsage()
+{
+    static const UsageInfo standardSettingsInfo[] =
+    {
+        { "framerate",  "[1-240]",      "Number of frames per second" },
+        { "quality",    "[1-100]",      "Normalized quality, 0=low, 100 = high" },
+        { "avg_bitrate","[1-10000000]", "Average bits per second for stream" },
+        { "max_bitrate","[1-10000000]", "Maximum bits per second for stream" }
+    };
+    printf("\nsettings shared by all plugins:\n");
+    for (const UsageInfo &info : standardSettingsInfo) {
+        printf("%16s = %-16s : %s\n",
+               info.name, info.range, info.description);
+    }
+    
+    for (auto &plugin: plugins) {
+        const UsageInfo *info = plugin->Usage();
+        if (info) {
+            printf("\nsettings for %s:\n", plugin->Name());
+            for (info = info; info->name; info++) {
+                printf("%16s = %-16s : %s\n",
+                       info->name, info->range, info->description);
+            }
+        }
+    }
+}
+
+void ConcreteAgent::ApplyOptions(Plugin *plugin)
+{
+    for (const auto &opt : options) {
+        try
+        {
+            bool known = plugin->ApplyOption(opt);
+            if (!known) {
+                Settings &settings = plugin->PluginSettings();
+                known = StandardOption(settings, opt);
+            }
+            if (!known) {
+                syslog(LOG_ERR, "Option %s not recognized by plugin %s",
+                       opt.name, plugin->Name());
+            }
+        }
+        catch (OptionError &err)
+        {
+            syslog(LOG_ERR, "Plugin %s did not accept setting %s=%s: %s",
+                   plugin->Name(), opt.name, opt.value, err.what());
+        }
+    }
+}
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 828368b..6852b51 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -12,33 +12,37 @@ 
 
 namespace SpiceStreamingAgent {
 
-struct ConcreteConfigureOption: ConfigureOption
-{
-    ConcreteConfigureOption(const char *name, const char *value)
-    {
-        this->name = name;
-        this->value = value;
-    }
-};
-
 class ConcreteAgent final : public Agent
 {
 public:
     ConcreteAgent();
+
+public:
+    // Implementation of the Agent class virtual methods
     unsigned Version() const override {
         return PluginVersion;
     }
-    void Register(Plugin& plugin) override;
-    const ConfigureOption* Options() const override;
-    void LoadPlugins(const char *directory);
-    // pointer must remain valid
+    void Register(Plugin *plugin) override;
+    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
+    int IntOption(const Option &opt,
+                  int min = INT_MIN, int max = INT_MAX,
+                  bool sizeSuffixOK = false) const
+        throw(OptionError) override;
+    
+public:
+    // Interface used by the main agent program
     void AddOption(const char *name, const char *value);
+    void LoadPlugins(const char *directory);
     FrameCapture *GetBestFrameCapture();
-    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
+    bool StandardOption(Settings &settings, const Option &option)
+        throw (OptionError);
+    void PluginsUsage();
+
 private:
     void LoadPlugin(const char *plugin_filename);
+    void ApplyOptions(Plugin *plugin);
     std::vector<std::shared_ptr<Plugin>> plugins;
-    std::vector<ConcreteConfigureOption> options;
+    std::vector<Option> options;
 };
 
 }
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index f41e68a..6bd8bf5 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -41,16 +41,11 @@  static inline uint64_t get_time()
 }
 
 namespace {
-struct MjpegSettings
-{
-    int fps;
-    int quality;
-};
 
 class MjpegFrameCapture final: public FrameCapture
 {
 public:
-    MjpegFrameCapture(const MjpegSettings &settings);
+    MjpegFrameCapture(Settings &settings);
     ~MjpegFrameCapture();
     FrameInfo CaptureFrame() override;
     void Reset() override;
@@ -58,8 +53,8 @@  public:
         return SPICE_VIDEO_CODEC_TYPE_MJPEG;
     }
 private:
-    MjpegSettings settings;
     Display *dpy;
+    Settings &settings;
 
     vector<uint8_t> frame;
 
@@ -72,19 +67,21 @@  private:
 class MjpegPlugin final: public Plugin
 {
 public:
+    MjpegPlugin(Agent *agent): Plugin(agent) {}
+    virtual const char *Name() override;
+    virtual const UsageInfo *Usage() override;
     FrameCapture *CreateCapture() override;
     unsigned Rank() override;
-    void ParseOptions(const ConfigureOption *options);
-    SpiceVideoCodecType VideoCodecType() const {
-        return SPICE_VIDEO_CODEC_TYPE_MJPEG;
-    }
+    SpiceVideoCodecType VideoCodecType() const override;
+    Settings &PluginSettings() override;
+    bool ApplyOption(const Option &o) throw(OptionError) override;
 private:
-    MjpegSettings settings = { 10, 80 };
+    Settings settings;
 };
 }
 
-MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& settings):
-    settings(settings)
+MjpegFrameCapture::MjpegFrameCapture(Settings &settings)
+    : settings(settings)
 {
     dpy = XOpenDisplay(NULL);
     if (!dpy)
@@ -111,7 +108,7 @@  FrameInfo MjpegFrameCapture::CaptureFrame()
     if (last_time == 0) {
         last_time = now;
     } else {
-        const uint64_t delta = 1000000000u / settings.fps;
+        const uint64_t delta = 1000000000u / settings.framerate;
         if (now >= last_time + delta) {
             last_time = now;
         } else {
@@ -148,13 +145,13 @@  FrameInfo MjpegFrameCapture::CaptureFrame()
 
     int format = ZPixmap;
     // TODO handle errors
-    XImage *image = XGetImage(dpy, win, win_info.x, win_info.y, 
+    XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
                               win_info.width, win_info.height, AllPlanes, format);
 
     // TODO handle errors
     // TODO multiple formats (only 32 bit)
-    write_JPEG_file(frame, settings.quality, (uint8_t*) image->data,
-                    image->width, image->height);
+    write_JPEG_file(frame, settings.quality,
+                    (uint8_t*) image->data, image->width, image->height);
 
     image->f.destroy_image(image);
 
@@ -166,6 +163,16 @@  FrameInfo MjpegFrameCapture::CaptureFrame()
     return info;
 }
 
+const char *MjpegPlugin::Name()
+{
+    return "MJPEG";
+}
+
+const UsageInfo *MjpegPlugin::Usage()
+{
+    return NULL;
+}
+
 FrameCapture *MjpegPlugin::CreateCapture()
 {
     return new MjpegFrameCapture(settings);
@@ -176,33 +183,20 @@  unsigned MjpegPlugin::Rank()
     return FallBackMin;
 }
 
-void MjpegPlugin::ParseOptions(const ConfigureOption *options)
+SpiceVideoCodecType MjpegPlugin::VideoCodecType() const
 {
-#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
-
-    for (; options->name; ++options) {
-        const char *name = options->name;
-        const char *value = options->value;
-
-        if (strcmp(name, "framerate") == 0) {
-            int val = atoi(value);
-            if (val > 0) {
-                settings.fps = val;
-            }
-            else {
-                arg_error("wrong framerate arg %s\n", value);
-            }
-        }
-        if (strcmp(name, "mjpeg.quality") == 0) {
-            int val = atoi(value);
-            if (val > 0) {
-                settings.quality = val;
-            }
-            else {
-                arg_error("wrong mjpeg.quality arg %s\n", value);
-            }
-        }
-    }
+    return SPICE_VIDEO_CODEC_TYPE_MJPEG;
+}
+
+Settings &MjpegPlugin::PluginSettings()
+{
+    return settings;
+}
+
+bool MjpegPlugin::ApplyOption(const Option &o) throw(OptionError)
+{
+    // This plugin only relies solely on base options
+    return false;
 }
 
 static bool
@@ -211,12 +205,7 @@  mjpeg_plugin_init(Agent* agent)
     if (agent->Version() != PluginVersion)
         return false;
 
-    std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
-
-    plugin->ParseOptions(agent->Options());
-
-    agent->Register(*plugin.release());
-
+    agent->Register(new MjpegPlugin(agent));
     return true;
 }
 
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>
> 
> The overall objective is to pave the way for features that will likely
> become important, notably the ability to dynamically adjust quality of
> service to respond to network conditions or client load issues.
> 
> Consequently, this patch cleans up the option-related aspects of the
> plugin ABI and API as follows:
> 
> 1. It adds standard settings that are shared across plugins, e.g. framerate.
>    This will enable future QoS adjustments to be applied by the agent
>    in a consistent way, irrespective of the actual capture plugin.
> 
> 2. It makes the interface for option handling dynamic, so that the agent
>    can in the future update settings without having to do expensive
>    operations such as searching for pre-existing options.
> 
> 3. It exposes a consistent way to deal witih settings values, so that

typo

>    error messages and acceptable values are consistent across plugins.
>    The current interfaces only deals with integer values, possibly with
>    a range of acceptable values, and possibly with a K/M suffix, e.g.
>    to accept max_bitrate=100k.
> 

Was thinking about this unit thing. Maybe we can use an enum making possible
to add additional units in the future?

> 4. It delegates usage information to the plugins, so that each plugin
>    can document its specific options in a consistent way.
> 
> 5. It cleans up some confusing or surprising aspects of the interace,
>    notably null-terminated option vectors and the ownership of
>    dynamically loaded plugins.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

Maybe you should split the patch instead of having multiple points
inside the comment? I would personally starts with some commit that
can be merged without waiting, like adding a Name() method.

> ---
>  include/spice-streaming-agent/plugin.hpp |  94 +++++++++++++++++---
>  m4/spice-compile-warnings.m4             |   2 +
>  src/concrete-agent.cpp                   | 143
>  ++++++++++++++++++++++++++++---
>  src/concrete-agent.hpp                   |  34 ++++----
>  src/mjpeg-fallback.cpp                   |  89 +++++++++----------
>  src/spice-streaming-agent.cpp            |  23 +++--
>  6 files changed, 284 insertions(+), 101 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index 607fabf..6202d2c 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -8,6 +8,10 @@
>  #define SPICE_STREAMING_AGENT_PLUGIN_HPP
>  
>  #include <spice/enums.h>
> +#include <stdexcept>
> +#include <vector>
> +#include <climits>
> +#include <cstdio>
>  
>  /*!
>   * \file
> @@ -20,6 +24,7 @@
>  namespace SpiceStreamingAgent {
>  
>  class FrameCapture;
> +class Agent;
>  
>  /*!
>   * Plugin version, only using few bits, schema is 0xMMmm
> @@ -42,31 +47,75 @@ enum Ranks : unsigned {
>  };
>  
>  /*!
> - * Configuration option.
> - * An array of these will be passed to the plugin.
> - * Simply a couple name and value passed as string.
> - * For instance "framerate" and "20".
> + * Settings that are common to all plugins
>   */
> -struct ConfigureOption
> +struct Settings
>  {
> +    unsigned    framerate       =      30; // Frames per second (1-240)
> +    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
> +};
> +
> +/*!
> + * Command line option
> + */
> +struct Option
> +{
> +    Option(const char *name, const char *value): name(name), value(value) {}
>      const char *name;
>      const char *value;
>  };
>  
>  /*!
> + * Error applying an option
> + */
> +class OptionError : public std::runtime_error
> +{
> +public:
> +    OptionError(const std::string &what): std::runtime_error(what) {}
> +};
> +
> +/*!
> + * Usage information
> + */
> +struct UsageInfo
> +{
> +    const char *name;
> +    const char *range;
> +    const char *description;
> +};
> +
> +/*!
>   * Interface a plugin should implement and register to the Agent.
>   *
>   * A plugin module can register multiple Plugin interfaces to handle
>   * multiple codecs. In this case each Plugin will report data for a
>   * specific codec.
> + *
> + * A plugin will typically extend the Settings struct to add its own
> + * settings.
>   */
>  class Plugin
>  {
>  public:
> +    Plugin(Agent *agent): agent(agent) {}
> +
>      /*!
>       * Allows to free the plugin when not needed
>       */
> -    virtual ~Plugin() {};
> +    virtual ~Plugin() { }
> +
> +    /*!
> +     * Return the name of the plugin, e.g. for command-line usage
> information
> +     */
> +    virtual const char *Name() = 0;
> +
> +    /*!
> +     * Return usage information for the plug-in, e.g. command-line options
> +     * Usage is returned as a NULL-terminated array of UsageInfo
> +     */
> +    virtual const UsageInfo *Usage() = 0;
>  
>      /*!
>       * Request an object for getting frames.
> @@ -89,6 +138,22 @@ public:
>       * Get video codec used to encode last frame
>       */
>      virtual SpiceVideoCodecType VideoCodecType() const=0;
> +
> +    /*!
> +     * Return the concrete Settings for this plugin
> +     */
> +    virtual Settings &  PluginSettings() = 0;
> +
> +    /*!
> +     * Apply a given option to the plugin settings.
> +     * \return true if the option is valid, false if it is not recognized
> +     * If there is an error applying the settings, set \arg error.
> +     * If this returns false, agent will process standard options itself.
> +     */
> +    virtual bool ApplyOption(const Option &o) throw(OptionError) = 0;
> +
> +protected:
> +    Agent *agent;
>  };
>  
>  /*!
> @@ -113,20 +178,23 @@ public:
>       * Check if a given plugin version is compatible with this agent
>       * \return true is compatible
>       */
> -    virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const=0;
> +    virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const =
> 0;
>  
>      /*!
>       * Register a plugin in the system.
> +     * The agent then has shared ownership of the plugin, meaning that
> +     * the plugin will be deleted when the agent is deleted unless another
> +     * shared_ptr instance still holds it at that time.
>       */
> -    virtual void Register(Plugin& plugin)=0;
> +    virtual void Register(Plugin *plugin)=0;
>  
>      /*!
> -     * Get options array.
> -     * Array is terminated with {nullptr, nullptr}.
> -     * Never nullptr.
> -     * \todo passing options to entry point instead?
> +     * Helper to get integer values from command-line options
>       */
> -    virtual const ConfigureOption* Options() const=0;
> +    virtual int IntOption(const Option &opt,
> +                          int min = INT_MIN, int max = INT_MAX,
> +                          bool sizeSuffixOK = false) const
> +        throw(OptionError) = 0;
>  };
>  
>  typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
> diff --git a/m4/spice-compile-warnings.m4 b/m4/spice-compile-warnings.m4
> index 66d7179..9e8bb6e 100644
> --- a/m4/spice-compile-warnings.m4
> +++ b/m4/spice-compile-warnings.m4
> @@ -86,6 +86,8 @@ AC_DEFUN([SPICE_COMPILE_WARNINGS],[
>      dontwarn="$dontwarn -Wpointer-sign"
>      dontwarn="$dontwarn -Wpointer-to-int-cast"
>      dontwarn="$dontwarn -Wstrict-prototypes"
> +    dontwarn="$dontwarn -Wsuggest-final-types"
> +    dontwarn="$dontwarn -Wsuggest-final-methods"
>  
>      # We want to enable these, but need to sort out the
>      # decl mess with  gtk/generated_*.c
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 51b4504..5c93e42 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -9,6 +9,10 @@
>  #include <syslog.h>
>  #include <glob.h>
>  #include <dlfcn.h>
> +#include <mutex>
> +#include <climits>
> +#include <sstream>
> +#include <string>
>  
>  #include "concrete-agent.hpp"
>  #include "static-plugin.hpp"
> @@ -28,7 +32,11 @@ static inline unsigned MinorVersion(unsigned version)
>  
>  ConcreteAgent::ConcreteAgent()
>  {
> -    options.push_back(ConcreteConfigureOption(nullptr, nullptr));
> +}
> +
> +void ConcreteAgent::Register(Plugin *plugin)
> +{
> +    plugins.push_back(shared_ptr<Plugin>(plugin));
>  }
>  
>  bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
> @@ -41,22 +49,83 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned
> pluginVersion) const
>          MinorVersion(version) >= MinorVersion(pluginVersion);
>  }
>  
> -void ConcreteAgent::Register(Plugin& plugin)
> +int ConcreteAgent::IntOption(const Option &opt,
> +                             int min, int max, bool sizeSuffixOK) const
> +    throw(OptionError)
>  {
> -    plugins.push_back(shared_ptr<Plugin>(&plugin));
> +    std::string name = opt.name;
> +    std::string value = opt.value;
> +    std::istringstream input(value);
> +    std::ostringstream err;
> +
> +    int result = 0;
> +    input >> result;
> +
> +    if (!input.fail() && !input.eof() && sizeSuffixOK) {
> +        std::string suffix;
> +        input >> suffix;
> +        bool ok = false;
> +        if (!input.fail() && suffix.length() == 1) {
> +            switch (suffix[0]) {
> +            case 'b': case 'B': ok = true;                      break;
> +            case 'k': case 'K': ok = true; result *= 1000;      break;
> +            case 'm': case 'M': ok = true; result *= 1000000;   break;
> +            default:                                            break;
> +            }
> +        }
> +        if (!ok) {
> +            err << "Unknown number suffix " << suffix
> +                << " for " << name << "\n";
> +            throw OptionError(err.str());
> +        }
> +    }
> +
> +    if (input.fail()) {
> +        err << "Value " << value << " for " << name
> +            << " is not a number\n";
> +        throw OptionError(err.str());
> +    }
> +    if (!input.eof()) {
> +        err << "Value " << value << " for " << name
> +            << " does not end like an integer\n";
> +        throw OptionError(err.str());
> +    }
> +    if (result < min || result > max) {
> +        err << "The value " << value << " for " << name
> +            << " is out of range (must be in " << min << ".." << max <<
> ")\n";
> +        throw OptionError(err.str());
> +    }
> +
> +    return result;
>  }
>  
> -const ConfigureOption* ConcreteAgent::Options() const
> +bool ConcreteAgent::StandardOption(Settings &settings, const Option &option)
> +    throw (OptionError)
>  {
> -    static_assert(sizeof(ConcreteConfigureOption) ==
> sizeof(ConfigureOption),
> -                  "ConcreteConfigureOption should be binary compatible with
> ConfigureOption");
> -    return static_cast<const ConfigureOption*>(&options[0]);
> +    string name = option.name;
> +    if (name == "framerate" || name == "fps") {
> +        settings.framerate = IntOption(option, 1, 240);
> +        return true;
> +    }
> +    if (name == "quality") {
> +        settings.quality = IntOption(option, 0, 100);
> +        return true;
> +    }
> +    if (name == "avg_bitrate" || name == "bitrate") {
> +        settings.avg_bitrate = IntOption(option, 10000, 32000000, true);
> +        return true;
> +    }
> +    if (name == "max_bitrate") {
> +        settings.max_bitrate = IntOption(option, 10000, 32000000, true);
> +        return true;
> +    }
> +
> +    return false;               // Unrecognized option
>  }
>  
>  void ConcreteAgent::AddOption(const char *name, const char *value)
>  {
> -    // insert before the last {nullptr, nullptr} value
> -    options.insert(--options.end(), ConcreteConfigureOption(name, value));
> +    options.push_back(Option(name, value));
>  }
>  
>  void ConcreteAgent::LoadPlugins(const char *directory)
> @@ -84,7 +153,8 @@ void ConcreteAgent::LoadPlugin(const char
> *plugin_filename)
>  {
>      void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW);
>      if (!dl) {
> -        syslog(LOG_ERR, "error loading plugin %s", plugin_filename);
> +        syslog(LOG_ERR, "error loading plugin %s: %s",
> +               plugin_filename, dlerror());
>          return;
>      }
>  
> @@ -106,7 +176,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>      vector<pair<unsigned, shared_ptr<Plugin>>> sorted_plugins;
>  
>      // sort plugins base on ranking, reverse order
> -    for (const auto& plugin: plugins) {
> +    for (auto& plugin: plugins) {
>          sorted_plugins.push_back(make_pair(plugin->Rank(), plugin));
>      }
>      sort(sorted_plugins.rbegin(), sorted_plugins.rend());
> @@ -116,6 +186,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>          if (plugin.first == DontUse) {
>              break;
>          }
> +        ApplyOptions(plugin.second.get());
>          FrameCapture *capture = plugin.second->CreateCapture();
>          if (capture) {
>              return capture;
> @@ -123,3 +194,53 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>      }
>      return nullptr;
>  }
> +
> +void ConcreteAgent::PluginsUsage()
> +{
> +    static const UsageInfo standardSettingsInfo[] =
> +    {
> +        { "framerate",  "[1-240]",      "Number of frames per second" },
> +        { "quality",    "[1-100]",      "Normalized quality, 0=low, 100 =
> high" },
> +        { "avg_bitrate","[1-10000000]", "Average bits per second for stream"
> },
> +        { "max_bitrate","[1-10000000]", "Maximum bits per second for stream"
> }
> +    };
> +    printf("\nsettings shared by all plugins:\n");
> +    for (const UsageInfo &info : standardSettingsInfo) {
> +        printf("%16s = %-16s : %s\n",
> +               info.name, info.range, info.description);
> +    }
> +
> +    for (auto &plugin: plugins) {
> +        const UsageInfo *info = plugin->Usage();
> +        if (info) {
> +            printf("\nsettings for %s:\n", plugin->Name());
> +            for (info = info; info->name; info++) {
> +                printf("%16s = %-16s : %s\n",
> +                       info->name, info->range, info->description);
> +            }
> +        }
> +    }
> +}
> +
> +void ConcreteAgent::ApplyOptions(Plugin *plugin)
> +{
> +    for (const auto &opt : options) {
> +        try
> +        {
> +            bool known = plugin->ApplyOption(opt);
> +            if (!known) {
> +                Settings &settings = plugin->PluginSettings();
> +                known = StandardOption(settings, opt);
> +            }
> +            if (!known) {
> +                syslog(LOG_ERR, "Option %s not recognized by plugin %s",
> +                       opt.name, plugin->Name());
> +            }
> +        }
> +        catch (OptionError &err)
> +        {
> +            syslog(LOG_ERR, "Plugin %s did not accept setting %s=%s: %s",
> +                   plugin->Name(), opt.name, opt.value, err.what());
> +        }
> +    }
> +}
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index 828368b..6852b51 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -12,33 +12,37 @@
>  
>  namespace SpiceStreamingAgent {
>  
> -struct ConcreteConfigureOption: ConfigureOption
> -{
> -    ConcreteConfigureOption(const char *name, const char *value)
> -    {
> -        this->name = name;
> -        this->value = value;
> -    }
> -};
> -
>  class ConcreteAgent final : public Agent
>  {
>  public:
>      ConcreteAgent();
> +
> +public:
> +    // Implementation of the Agent class virtual methods
>      unsigned Version() const override {
>          return PluginVersion;
>      }
> -    void Register(Plugin& plugin) override;
> -    const ConfigureOption* Options() const override;
> -    void LoadPlugins(const char *directory);
> -    // pointer must remain valid
> +    void Register(Plugin *plugin) override;
> +    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
> +    int IntOption(const Option &opt,
> +                  int min = INT_MIN, int max = INT_MAX,
> +                  bool sizeSuffixOK = false) const
> +        throw(OptionError) override;
> +
> +public:
> +    // Interface used by the main agent program
>      void AddOption(const char *name, const char *value);
> +    void LoadPlugins(const char *directory);
>      FrameCapture *GetBestFrameCapture();
> -    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
> +    bool StandardOption(Settings &settings, const Option &option)
> +        throw (OptionError);
> +    void PluginsUsage();
> +
>  private:
>      void LoadPlugin(const char *plugin_filename);
> +    void ApplyOptions(Plugin *plugin);
>      std::vector<std::shared_ptr<Plugin>> plugins;
> -    std::vector<ConcreteConfigureOption> options;
> +    std::vector<Option> options;
>  };
>  
>  }
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index f41e68a..6bd8bf5 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -41,16 +41,11 @@ static inline uint64_t get_time()
>  }
>  
>  namespace {
> -struct MjpegSettings
> -{
> -    int fps;
> -    int quality;
> -};
>  
>  class MjpegFrameCapture final: public FrameCapture
>  {
>  public:
> -    MjpegFrameCapture(const MjpegSettings &settings);
> +    MjpegFrameCapture(Settings &settings);
>      ~MjpegFrameCapture();
>      FrameInfo CaptureFrame() override;
>      void Reset() override;
> @@ -58,8 +53,8 @@ public:
>          return SPICE_VIDEO_CODEC_TYPE_MJPEG;
>      }
>  private:
> -    MjpegSettings settings;
>      Display *dpy;
> +    Settings &settings;
>  
>      vector<uint8_t> frame;
>  
> @@ -72,19 +67,21 @@ private:
>  class MjpegPlugin final: public Plugin
>  {
>  public:
> +    MjpegPlugin(Agent *agent): Plugin(agent) {}
> +    virtual const char *Name() override;
> +    virtual const UsageInfo *Usage() override;
>      FrameCapture *CreateCapture() override;
>      unsigned Rank() override;
> -    void ParseOptions(const ConfigureOption *options);
> -    SpiceVideoCodecType VideoCodecType() const {
> -        return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> -    }
> +    SpiceVideoCodecType VideoCodecType() const override;
> +    Settings &PluginSettings() override;
> +    bool ApplyOption(const Option &o) throw(OptionError) override;
>  private:
> -    MjpegSettings settings = { 10, 80 };
> +    Settings settings;
>  };
>  }
>  
> -MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& settings):
> -    settings(settings)
> +MjpegFrameCapture::MjpegFrameCapture(Settings &settings)
> +    : settings(settings)
>  {
>      dpy = XOpenDisplay(NULL);
>      if (!dpy)
> @@ -111,7 +108,7 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>      if (last_time == 0) {
>          last_time = now;
>      } else {
> -        const uint64_t delta = 1000000000u / settings.fps;
> +        const uint64_t delta = 1000000000u / settings.framerate;
>          if (now >= last_time + delta) {
>              last_time = now;
>          } else {
> @@ -148,13 +145,13 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>  
>      int format = ZPixmap;
>      // TODO handle errors
> -    XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
> +    XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
>                                win_info.width, win_info.height, AllPlanes,
>                                format);
>  
>      // TODO handle errors
>      // TODO multiple formats (only 32 bit)
> -    write_JPEG_file(frame, settings.quality, (uint8_t*) image->data,
> -                    image->width, image->height);
> +    write_JPEG_file(frame, settings.quality,
> +                    (uint8_t*) image->data, image->width, image->height);
>  
>      image->f.destroy_image(image);
>  
> @@ -166,6 +163,16 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>      return info;
>  }
>  
> +const char *MjpegPlugin::Name()
> +{
> +    return "MJPEG";
> +}
> +
> +const UsageInfo *MjpegPlugin::Usage()
> +{
> +    return NULL;
> +}
> +
>  FrameCapture *MjpegPlugin::CreateCapture()
>  {
>      return new MjpegFrameCapture(settings);
> @@ -176,33 +183,20 @@ unsigned MjpegPlugin::Rank()
>      return FallBackMin;
>  }
>  
> -void MjpegPlugin::ParseOptions(const ConfigureOption *options)
> +SpiceVideoCodecType MjpegPlugin::VideoCodecType() const
>  {
> -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> -
> -    for (; options->name; ++options) {
> -        const char *name = options->name;
> -        const char *value = options->value;
> -
> -        if (strcmp(name, "framerate") == 0) {
> -            int val = atoi(value);
> -            if (val > 0) {
> -                settings.fps = val;
> -            }
> -            else {
> -                arg_error("wrong framerate arg %s\n", value);
> -            }
> -        }
> -        if (strcmp(name, "mjpeg.quality") == 0) {
> -            int val = atoi(value);
> -            if (val > 0) {
> -                settings.quality = val;
> -            }
> -            else {
> -                arg_error("wrong mjpeg.quality arg %s\n", value);
> -            }
> -        }
> -    }
> +    return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> +}
> +
> +Settings &MjpegPlugin::PluginSettings()
> +{
> +    return settings;
> +}
> +
> +bool MjpegPlugin::ApplyOption(const Option &o) throw(OptionError)
> +{
> +    // This plugin only relies solely on base options
> +    return false;
>  }
>  
>  static bool
> @@ -211,12 +205,7 @@ mjpeg_plugin_init(Agent* agent)
>      if (agent->Version() != PluginVersion)
>          return false;
>  
> -    std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
> -
> -    plugin->ParseOptions(agent->Options());
> -
> -    agent->Register(*plugin.release());
> -
> +    agent->Register(new MjpegPlugin(agent));
>      return true;
>  }
>  
> 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
Frediano Ziglio writes:

>>
>> From: Christophe de Dinechin <dinechin@redhat.com>
>>
>> The overall objective is to pave the way for features that will likely
>> become important, notably the ability to dynamically adjust quality of
>> service to respond to network conditions or client load issues.
>>
>> Consequently, this patch cleans up the option-related aspects of the
>> plugin ABI and API as follows:
>>
>> 1. It adds standard settings that are shared across plugins, e.g. framerate.
>>    This will enable future QoS adjustments to be applied by the agent
>>    in a consistent way, irrespective of the actual capture plugin.
>>
>> 2. It makes the interface for option handling dynamic, so that the agent
>>    can in the future update settings without having to do expensive
>>    operations such as searching for pre-existing options.
>>
>> 3. It exposes a consistent way to deal witih settings values, so that
>
> typo
>
>>    error messages and acceptable values are consistent across plugins.
>>    The current interfaces only deals with integer values, possibly with
>>    a range of acceptable values, and possibly with a K/M suffix, e.g.
>>    to accept max_bitrate=100k.
>>
>
> Was thinking about this unit thing. Maybe we can use an enum making possible
> to add additional units in the future?

Well, if we want arbitrary units, we could pass an array of acceptable
units and corresponding multipliers. It can't just be an enum.

One of the things I realized after sending the patch is that we should
be able to write 8Mb and 8MB and have megabits and megabytes
respectively, since that's what the units normally means.

>
>> 4. It delegates usage information to the plugins, so that each plugin
>>    can document its specific options in a consistent way.
>>
>> 5. It cleans up some confusing or surprising aspects of the interace,
>>    notably null-terminated option vectors and the ownership of
>>    dynamically loaded plugins.
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>
> Maybe you should split the patch instead of having multiple points
> inside the comment? I would personally starts with some commit that
> can be merged without waiting, like adding a Name() method.

The Name() method by itself introduces an ABI incompatibility. For now,
I have not really understood your objections about the first patch
in the series, so I am awaiting for your response there, and then
I can decide how to split.

My preference is clearly to mark a strong ABI incompatiblity for now,
by calling the ABI 0.x, and add the rule that all 0.x versions are
assumed to be incompatible with one another. If we do that, then
it is no problem to bump the minor number for something like adding
Name().

If I have to bump the major number just because I add Name(), and
we end up in a month with ABI version 42.0, that's OK too, but I don't
like it.

>
>> ---
>>  include/spice-streaming-agent/plugin.hpp |  94 +++++++++++++++++---
>>  m4/spice-compile-warnings.m4             |   2 +
>>  src/concrete-agent.cpp                   | 143
>>  ++++++++++++++++++++++++++++---
>>  src/concrete-agent.hpp                   |  34 ++++----
>>  src/mjpeg-fallback.cpp                   |  89 +++++++++----------
>>  src/spice-streaming-agent.cpp            |  23 +++--
>>  6 files changed, 284 insertions(+), 101 deletions(-)
>>
>> diff --git a/include/spice-streaming-agent/plugin.hpp
>> b/include/spice-streaming-agent/plugin.hpp
>> index 607fabf..6202d2c 100644
>> --- a/include/spice-streaming-agent/plugin.hpp
>> +++ b/include/spice-streaming-agent/plugin.hpp
>> @@ -8,6 +8,10 @@
>>  #define SPICE_STREAMING_AGENT_PLUGIN_HPP
>>
>>  #include <spice/enums.h>
>> +#include <stdexcept>
>> +#include <vector>
>> +#include <climits>
>> +#include <cstdio>
>>
>>  /*!
>>   * \file
>> @@ -20,6 +24,7 @@
>>  namespace SpiceStreamingAgent {
>>
>>  class FrameCapture;
>> +class Agent;
>>
>>  /*!
>>   * Plugin version, only using few bits, schema is 0xMMmm
>> @@ -42,31 +47,75 @@ enum Ranks : unsigned {
>>  };
>>
>>  /*!
>> - * Configuration option.
>> - * An array of these will be passed to the plugin.
>> - * Simply a couple name and value passed as string.
>> - * For instance "framerate" and "20".
>> + * Settings that are common to all plugins
>>   */
>> -struct ConfigureOption
>> +struct Settings
>>  {
>> +    unsigned    framerate       =      30; // Frames per second (1-240)
>> +    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
>> +};
>> +
>> +/*!
>> + * Command line option
>> + */
>> +struct Option
>> +{
>> +    Option(const char *name, const char *value): name(name), value(value) {}
>>      const char *name;
>>      const char *value;
>>  };
>>
>>  /*!
>> + * Error applying an option
>> + */
>> +class OptionError : public std::runtime_error
>> +{
>> +public:
>> +    OptionError(const std::string &what): std::runtime_error(what) {}
>> +};
>> +
>> +/*!
>> + * Usage information
>> + */
>> +struct UsageInfo
>> +{
>> +    const char *name;
>> +    const char *range;
>> +    const char *description;
>> +};
>> +
>> +/*!
>>   * Interface a plugin should implement and register to the Agent.
>>   *
>>   * A plugin module can register multiple Plugin interfaces to handle
>>   * multiple codecs. In this case each Plugin will report data for a
>>   * specific codec.
>> + *
>> + * A plugin will typically extend the Settings struct to add its own
>> + * settings.
>>   */
>>  class Plugin
>>  {
>>  public:
>> +    Plugin(Agent *agent): agent(agent) {}
>> +
>>      /*!
>>       * Allows to free the plugin when not needed
>>       */
>> -    virtual ~Plugin() {};
>> +    virtual ~Plugin() { }
>> +
>> +    /*!
>> +     * Return the name of the plugin, e.g. for command-line usage
>> information
>> +     */
>> +    virtual const char *Name() = 0;
>> +
>> +    /*!
>> +     * Return usage information for the plug-in, e.g. command-line options
>> +     * Usage is returned as a NULL-terminated array of UsageInfo
>> +     */
>> +    virtual const UsageInfo *Usage() = 0;
>>
>>      /*!
>>       * Request an object for getting frames.
>> @@ -89,6 +138,22 @@ public:
>>       * Get video codec used to encode last frame
>>       */
>>      virtual SpiceVideoCodecType VideoCodecType() const=0;
>> +
>> +    /*!
>> +     * Return the concrete Settings for this plugin
>> +     */
>> +    virtual Settings &  PluginSettings() = 0;
>> +
>> +    /*!
>> +     * Apply a given option to the plugin settings.
>> +     * \return true if the option is valid, false if it is not recognized
>> +     * If there is an error applying the settings, set \arg error.
>> +     * If this returns false, agent will process standard options itself.
>> +     */
>> +    virtual bool ApplyOption(const Option &o) throw(OptionError) = 0;
>> +
>> +protected:
>> +    Agent *agent;
>>  };
>>
>>  /*!
>> @@ -113,20 +178,23 @@ public:
>>       * Check if a given plugin version is compatible with this agent
>>       * \return true is compatible
>>       */
>> -    virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const=0;
>> +    virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const =
>> 0;
>>
>>      /*!
>>       * Register a plugin in the system.
>> +     * The agent then has shared ownership of the plugin, meaning that
>> +     * the plugin will be deleted when the agent is deleted unless another
>> +     * shared_ptr instance still holds it at that time.
>>       */
>> -    virtual void Register(Plugin& plugin)=0;
>> +    virtual void Register(Plugin *plugin)=0;
>>
>>      /*!
>> -     * Get options array.
>> -     * Array is terminated with {nullptr, nullptr}.
>> -     * Never nullptr.
>> -     * \todo passing options to entry point instead?
>> +     * Helper to get integer values from command-line options
>>       */
>> -    virtual const ConfigureOption* Options() const=0;
>> +    virtual int IntOption(const Option &opt,
>> +                          int min = INT_MIN, int max = INT_MAX,
>> +                          bool sizeSuffixOK = false) const
>> +        throw(OptionError) = 0;
>>  };
>>
>>  typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
>> diff --git a/m4/spice-compile-warnings.m4 b/m4/spice-compile-warnings.m4
>> index 66d7179..9e8bb6e 100644
>> --- a/m4/spice-compile-warnings.m4
>> +++ b/m4/spice-compile-warnings.m4
>> @@ -86,6 +86,8 @@ AC_DEFUN([SPICE_COMPILE_WARNINGS],[
>>      dontwarn="$dontwarn -Wpointer-sign"
>>      dontwarn="$dontwarn -Wpointer-to-int-cast"
>>      dontwarn="$dontwarn -Wstrict-prototypes"
>> +    dontwarn="$dontwarn -Wsuggest-final-types"
>> +    dontwarn="$dontwarn -Wsuggest-final-methods"
>>
>>      # We want to enable these, but need to sort out the
>>      # decl mess with  gtk/generated_*.c
>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
>> index 51b4504..5c93e42 100644
>> --- a/src/concrete-agent.cpp
>> +++ b/src/concrete-agent.cpp
>> @@ -9,6 +9,10 @@
>>  #include <syslog.h>
>>  #include <glob.h>
>>  #include <dlfcn.h>
>> +#include <mutex>
>> +#include <climits>
>> +#include <sstream>
>> +#include <string>
>>
>>  #include "concrete-agent.hpp"
>>  #include "static-plugin.hpp"
>> @@ -28,7 +32,11 @@ static inline unsigned MinorVersion(unsigned version)
>>
>>  ConcreteAgent::ConcreteAgent()
>>  {
>> -    options.push_back(ConcreteConfigureOption(nullptr, nullptr));
>> +}
>> +
>> +void ConcreteAgent::Register(Plugin *plugin)
>> +{
>> +    plugins.push_back(shared_ptr<Plugin>(plugin));
>>  }
>>
>>  bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
>> @@ -41,22 +49,83 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned
>> pluginVersion) const
>>          MinorVersion(version) >= MinorVersion(pluginVersion);
>>  }
>>
>> -void ConcreteAgent::Register(Plugin& plugin)
>> +int ConcreteAgent::IntOption(const Option &opt,
>> +                             int min, int max, bool sizeSuffixOK) const
>> +    throw(OptionError)
>>  {
>> -    plugins.push_back(shared_ptr<Plugin>(&plugin));
>> +    std::string name = opt.name;
>> +    std::string value = opt.value;
>> +    std::istringstream input(value);
>> +    std::ostringstream err;
>> +
>> +    int result = 0;
>> +    input >> result;
>> +
>> +    if (!input.fail() && !input.eof() && sizeSuffixOK) {
>> +        std::string suffix;
>> +        input >> suffix;
>> +        bool ok = false;
>> +        if (!input.fail() && suffix.length() == 1) {
>> +            switch (suffix[0]) {
>> +            case 'b': case 'B': ok = true;                      break;
>> +            case 'k': case 'K': ok = true; result *= 1000;      break;
>> +            case 'm': case 'M': ok = true; result *= 1000000;   break;
>> +            default:                                            break;
>> +            }
>> +        }
>> +        if (!ok) {
>> +            err << "Unknown number suffix " << suffix
>> +                << " for " << name << "\n";
>> +            throw OptionError(err.str());
>> +        }
>> +    }
>> +
>> +    if (input.fail()) {
>> +        err << "Value " << value << " for " << name
>> +            << " is not a number\n";
>> +        throw OptionError(err.str());
>> +    }
>> +    if (!input.eof()) {
>> +        err << "Value " << value << " for " << name
>> +            << " does not end like an integer\n";
>> +        throw OptionError(err.str());
>> +    }
>> +    if (result < min || result > max) {
>> +        err << "The value " << value << " for " << name
>> +            << " is out of range (must be in " << min << ".." << max <<
>> ")\n";
>> +        throw OptionError(err.str());
>> +    }
>> +
>> +    return result;
>>  }
>>
>> -const ConfigureOption* ConcreteAgent::Options() const
>> +bool ConcreteAgent::StandardOption(Settings &settings, const Option &option)
>> +    throw (OptionError)
>>  {
>> -    static_assert(sizeof(ConcreteConfigureOption) ==
>> sizeof(ConfigureOption),
>> -                  "ConcreteConfigureOption should be binary compatible with
>> ConfigureOption");
>> -    return static_cast<const ConfigureOption*>(&options[0]);
>> +    string name = option.name;
>> +    if (name == "framerate" || name == "fps") {
>> +        settings.framerate = IntOption(option, 1, 240);
>> +        return true;
>> +    }
>> +    if (name == "quality") {
>> +        settings.quality = IntOption(option, 0, 100);
>> +        return true;
>> +    }
>> +    if (name == "avg_bitrate" || name == "bitrate") {
>> +        settings.avg_bitrate = IntOption(option, 10000, 32000000, true);
>> +        return true;
>> +    }
>> +    if (name == "max_bitrate") {
>> +        settings.max_bitrate = IntOption(option, 10000, 32000000, true);
>> +        return true;
>> +    }
>> +
>> +    return false;               // Unrecognized option
>>  }
>>
>>  void ConcreteAgent::AddOption(const char *name, const char *value)
>>  {
>> -    // insert before the last {nullptr, nullptr} value
>> -    options.insert(--options.end(), ConcreteConfigureOption(name, value));
>> +    options.push_back(Option(name, value));
>>  }
>>
>>  void ConcreteAgent::LoadPlugins(const char *directory)
>> @@ -84,7 +153,8 @@ void ConcreteAgent::LoadPlugin(const char
>> *plugin_filename)
>>  {
>>      void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW);
>>      if (!dl) {
>> -        syslog(LOG_ERR, "error loading plugin %s", plugin_filename);
>> +        syslog(LOG_ERR, "error loading plugin %s: %s",
>> +               plugin_filename, dlerror());
>>          return;
>>      }
>>
>> @@ -106,7 +176,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>>      vector<pair<unsigned, shared_ptr<Plugin>>> sorted_plugins;
>>
>>      // sort plugins base on ranking, reverse order
>> -    for (const auto& plugin: plugins) {
>> +    for (auto& plugin: plugins) {
>>          sorted_plugins.push_back(make_pair(plugin->Rank(), plugin));
>>      }
>>      sort(sorted_plugins.rbegin(), sorted_plugins.rend());
>> @@ -116,6 +186,7 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>>          if (plugin.first == DontUse) {
>>              break;
>>          }
>> +        ApplyOptions(plugin.second.get());
>>          FrameCapture *capture = plugin.second->CreateCapture();
>>          if (capture) {
>>              return capture;
>> @@ -123,3 +194,53 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture()
>>      }
>>      return nullptr;
>>  }
>> +
>> +void ConcreteAgent::PluginsUsage()
>> +{
>> +    static const UsageInfo standardSettingsInfo[] =
>> +    {
>> +        { "framerate",  "[1-240]",      "Number of frames per second" },
>> +        { "quality",    "[1-100]",      "Normalized quality, 0=low, 100 =
>> high" },
>> +        { "avg_bitrate","[1-10000000]", "Average bits per second for stream"
>> },
>> +        { "max_bitrate","[1-10000000]", "Maximum bits per second for stream"
>> }
>> +    };
>> +    printf("\nsettings shared by all plugins:\n");
>> +    for (const UsageInfo &info : standardSettingsInfo) {
>> +        printf("%16s = %-16s : %s\n",
>> +               info.name, info.range, info.description);
>> +    }
>> +
>> +    for (auto &plugin: plugins) {
>> +        const UsageInfo *info = plugin->Usage();
>> +        if (info) {
>> +            printf("\nsettings for %s:\n", plugin->Name());
>> +            for (info = info; info->name; info++) {
>> +                printf("%16s = %-16s : %s\n",
>> +                       info->name, info->range, info->description);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +void ConcreteAgent::ApplyOptions(Plugin *plugin)
>> +{
>> +    for (const auto &opt : options) {
>> +        try
>> +        {
>> +            bool known = plugin->ApplyOption(opt);
>> +            if (!known) {
>> +                Settings &settings = plugin->PluginSettings();
>> +                known = StandardOption(settings, opt);
>> +            }
>> +            if (!known) {
>> +                syslog(LOG_ERR, "Option %s not recognized by plugin %s",
>> +                       opt.name, plugin->Name());
>> +            }
>> +        }
>> +        catch (OptionError &err)
>> +        {
>> +            syslog(LOG_ERR, "Plugin %s did not accept setting %s=%s: %s",
>> +                   plugin->Name(), opt.name, opt.value, err.what());
>> +        }
>> +    }
>> +}
>> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
>> index 828368b..6852b51 100644
>> --- a/src/concrete-agent.hpp
>> +++ b/src/concrete-agent.hpp
>> @@ -12,33 +12,37 @@
>>
>>  namespace SpiceStreamingAgent {
>>
>> -struct ConcreteConfigureOption: ConfigureOption
>> -{
>> -    ConcreteConfigureOption(const char *name, const char *value)
>> -    {
>> -        this->name = name;
>> -        this->value = value;
>> -    }
>> -};
>> -
>>  class ConcreteAgent final : public Agent
>>  {
>>  public:
>>      ConcreteAgent();
>> +
>> +public:
>> +    // Implementation of the Agent class virtual methods
>>      unsigned Version() const override {
>>          return PluginVersion;
>>      }
>> -    void Register(Plugin& plugin) override;
>> -    const ConfigureOption* Options() const override;
>> -    void LoadPlugins(const char *directory);
>> -    // pointer must remain valid
>> +    void Register(Plugin *plugin) override;
>> +    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
>> +    int IntOption(const Option &opt,
>> +                  int min = INT_MIN, int max = INT_MAX,
>> +                  bool sizeSuffixOK = false) const
>> +        throw(OptionError) override;
>> +
>> +public:
>> +    // Interface used by the main agent program
>>      void AddOption(const char *name, const char *value);
>> +    void LoadPlugins(const char *directory);
>>      FrameCapture *GetBestFrameCapture();
>> -    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
>> +    bool StandardOption(Settings &settings, const Option &option)
>> +        throw (OptionError);
>> +    void PluginsUsage();
>> +
>>  private:
>>      void LoadPlugin(const char *plugin_filename);
>> +    void ApplyOptions(Plugin *plugin);
>>      std::vector<std::shared_ptr<Plugin>> plugins;
>> -    std::vector<ConcreteConfigureOption> options;
>> +    std::vector<Option> options;
>>  };
>>
>>  }
>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>> index f41e68a..6bd8bf5 100644
>> --- a/src/mjpeg-fallback.cpp
>> +++ b/src/mjpeg-fallback.cpp
>> @@ -41,16 +41,11 @@ static inline uint64_t get_time()
>>  }
>>
>>  namespace {
>> -struct MjpegSettings
>> -{
>> -    int fps;
>> -    int quality;
>> -};
>>
>>  class MjpegFrameCapture final: public FrameCapture
>>  {
>>  public:
>> -    MjpegFrameCapture(const MjpegSettings &settings);
>> +    MjpegFrameCapture(Settings &settings);
>>      ~MjpegFrameCapture();
>>      FrameInfo CaptureFrame() override;
>>      void Reset() override;
>> @@ -58,8 +53,8 @@ public:
>>          return SPICE_VIDEO_CODEC_TYPE_MJPEG;
>>      }
>>  private:
>> -    MjpegSettings settings;
>>      Display *dpy;
>> +    Settings &settings;
>>
>>      vector<uint8_t> frame;
>>
>> @@ -72,19 +67,21 @@ private:
>>  class MjpegPlugin final: public Plugin
>>  {
>>  public:
>> +    MjpegPlugin(Agent *agent): Plugin(agent) {}
>> +    virtual const char *Name() override;
>> +    virtual const UsageInfo *Usage() override;
>>      FrameCapture *CreateCapture() override;
>>      unsigned Rank() override;
>> -    void ParseOptions(const ConfigureOption *options);
>> -    SpiceVideoCodecType VideoCodecType() const {
>> -        return SPICE_VIDEO_CODEC_TYPE_MJPEG;
>> -    }
>> +    SpiceVideoCodecType VideoCodecType() const override;
>> +    Settings &PluginSettings() override;
>> +    bool ApplyOption(const Option &o) throw(OptionError) override;
>>  private:
>> -    MjpegSettings settings = { 10, 80 };
>> +    Settings settings;
>>  };
>>  }
>>
>> -MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& settings):
>> -    settings(settings)
>> +MjpegFrameCapture::MjpegFrameCapture(Settings &settings)
>> +    : settings(settings)
>>  {
>>      dpy = XOpenDisplay(NULL);
>>      if (!dpy)
>> @@ -111,7 +108,7 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>>      if (last_time == 0) {
>>          last_time = now;
>>      } else {
>> -        const uint64_t delta = 1000000000u / settings.fps;
>> +        const uint64_t delta = 1000000000u / settings.framerate;
>>          if (now >= last_time + delta) {
>>              last_time = now;
>>          } else {
>> @@ -148,13 +145,13 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>>
>>      int format = ZPixmap;
>>      // TODO handle errors
>> -    XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
>> +    XImage *image = XGetImage(dpy, win, win_info.x, win_info.y,
>>                                win_info.width, win_info.height, AllPlanes,
>>                                format);
>>
>>      // TODO handle errors
>>      // TODO multiple formats (only 32 bit)
>> -    write_JPEG_file(frame, settings.quality, (uint8_t*) image->data,
>> -                    image->width, image->height);
>> +    write_JPEG_file(frame, settings.quality,
>> +                    (uint8_t*) image->data, image->width, image->height);
>>
>>      image->f.destroy_image(image);
>>
>> @@ -166,6 +163,16 @@ FrameInfo MjpegFrameCapture::CaptureFrame()
>>      return info;
>>  }
>>
>> +const char *MjpegPlugin::Name()
>> +{
>> +    return "MJPEG";
>> +}
>> +
>> +const UsageInfo *MjpegPlugin::Usage()
>> +{
>> +    return NULL;
>> +}
>> +
>>  FrameCapture *MjpegPlugin::CreateCapture()
>>  {
>>      return new MjpegFrameCapture(settings);
>> @@ -176,33 +183,20 @@ unsigned MjpegPlugin::Rank()
>>      return FallBackMin;
>>  }
>>
>> -void MjpegPlugin::ParseOptions(const ConfigureOption *options)
>> +SpiceVideoCodecType MjpegPlugin::VideoCodecType() const
>>  {
>> -#define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
>> -
>> -    for (; options->name; ++options) {
>> -        const char *name = options->name;
>> -        const char *value = options->value;
>> -
>> -        if (strcmp(name, "framerate") == 0) {
>> -            int val = atoi(value);
>> -            if (val > 0) {
>> -                settings.fps = val;
>> -            }
>> -            else {
>> -                arg_error("wrong framerate arg %s\n", value);
>> -            }
>> -        }
>> -        if (strcmp(name, "mjpeg.quality") == 0) {
>> -            int val = atoi(value);
>> -            if (val > 0) {
>> -                settings.quality = val;
>> -            }
>> -            else {
>> -                arg_error("wrong mjpeg.quality arg %s\n", value);
>> -            }
>> -        }
>> -    }
>> +    return SPICE_VIDEO_CODEC_TYPE_MJPEG;
>> +}
>> +
>> +Settings &MjpegPlugin::PluginSettings()
>> +{
>> +    return settings;
>> +}
>> +
>> +bool MjpegPlugin::ApplyOption(const Option &o) throw(OptionError)
>> +{
>> +    // This plugin only relies solely on base options
>> +    return false;
>>  }
>>
>>  static bool
>> @@ -211,12 +205,7 @@ mjpeg_plugin_init(Agent* agent)
>>      if (agent->Version() != PluginVersion)
>>          return false;
>>
>> -    std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
>> -
>> -    plugin->ParseOptions(agent->Options());
>> -
>> -    agent->Register(*plugin.release());
>> -
>> +    agent->Register(new MjpegPlugin(agent));
>>      return true;
>>  }
>>
>> 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
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


--
Cheers,
Christophe de Dinechin (IRC c3d)
On Thu, Nov 16, 2017 at 10:17:28AM -0500, Frediano Ziglio wrote:
> > 4. It delegates usage information to the plugins, so that each plugin
> >    can document its specific options in a consistent way.
> > 
> > 5. It cleans up some confusing or surprising aspects of the interace,
> >    notably null-terminated option vectors and the ownership of
> >    dynamically loaded plugins.
> > 
> > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> 
> Maybe you should split the patch instead of having multiple points
> inside the comment? I would personally starts with some commit that
> can be merged without waiting, like adding a Name() method.

Yes, that was my reaction when reading through it, this is trying to do far too
much in one commit.

Christophe