[Spice-devel] [PATCH spice-streaming-agent v2 1/3] mjpeg-fallback: a more C++ way of handling options

[Frediano Ziglio]

I would use a more "polite": "a more high level way of handling options"

> 
> Use C++ standard library:
> - std::string
> - std::stoi() to parse the numbers (also note atoi() behavior is undefined in
>   case of errors)
> - exceptions for errors, makes testing and potential future changes to
>   error handling easier.
> 
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
>  src/mjpeg-fallback.cpp | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 74682f3..1b51ee0 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -159,29 +159,24 @@ unsigned MjpegPlugin::Rank()
>  
>  void MjpegPlugin::ParseOptions(const ConfigureOption *options)
>  {
> -#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;
> +        const std::string name = options->name;
> +        const std::string value = options->value;
> +
> +        if (name == "framerate") {
> +            try {
> +                settings.fps = stoi(value);
> +            } catch (const std::exception &e) {

would be better to catch just std::logic_error instead?
(same below)

> +                throw std::runtime_error("Invalid value '" + value + "' for
> option 'framerate'.");
>              }
> -            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);
> +        } else if (name == "mjpeg.quality") {
> +            try {
> +                settings.quality = stoi(value);
> +            } catch (const std::exception &e) {
> +                throw std::runtime_error("Invalid value '" + value + "' for
> option 'mjpeg.quality'.");
>              }
> +        } else {
> +            throw std::runtime_error("Invalid option '" + name + "'.");
>          }
>      }
>  }
> @@ -198,7 +193,11 @@ mjpeg_plugin_init(Agent* agent)
>  
>      std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
>  
> -    plugin->ParseOptions(agent->Options());
> +    try {
> +        plugin->ParseOptions(agent->Options());
> +    } catch (const std::exception &e) {
> +        syslog(LOG_ERR, "Error parsing plugin option: %s\n", e.what());

I think in this case "options" is better.
Isn't std::exception catching too much (not much strong about it)?

> +    }
>  
>      agent->Register(*plugin.release());
>  

Frediano