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

[Lukáš Hrázký]

On Tue, 2018-02-13 at 09:18 -0500, Frediano Ziglio wrote:
> I would use a more "polite": "a more high level way of handling options"

Ok :)

> > 
> > 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)

Wow, the exceptions thrown from the function really inherit from
logic_error... This seems totally wrong, because logic_error should
represent errors in the logic of the program, but here the outcome
depends on 'user input'.

If it weren't for this, I'd be inclined to catch the common base class
of the exception, but I feel really reluctant to catch logic_error in
this way...

In any case, the function should not be throwing other exceptions, so
this should be more of a formality.

> > +                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)?

The only other exception I can see the method throwing would be
std::bad_alloc (from the std::string constructors). Since we are lazy
to define our own exception classes and are using runtime_errors, I'd
keep it this way. If we wanted to differentiate, we'd have to go to the
"define your exceptions" land :) I don't think it matters much here?

The way I'm looking at it is: "Do we want to fail harder than an error
message if we get some more serious exceptions than a parsing error?"
It probably doesn't matter.

Lukas

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