[libvirt] [PATCH 1/2] util: add virStringParseYesNo()

[Erik Skultety]

On Tue, Mar 12, 2019 at 08:38:18AM +0100, Peter Krempa wrote:
> On Tue, Mar 12, 2019 at 00:38:05 +0900, Shotaro Gotanda wrote:
> > This function parse and convert string "yes|no" into bool true|false.
> >
> > Signed-off-by: Shotaro Gotanda <g.sho1500 at gmail.com>
> > ---
> >  src/util/virstring.c | 32 ++++++++++++++++++++++++++++++++
> >  src/util/virstring.h |  3 +++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index 33f8191f45..70018459de 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -1548,3 +1548,35 @@ virStringParsePort(const char *str,
> >
> >      return 0;
> >  }
> > +
> > +
> > +/**
> > + * virStringParseYesNo:
> > + * @str: "yes|no" to parse
> > + * @port: pointer to parse and convert "yes|no" into
> > + *
> > + * Parses a string "yes|no" and convert it into true|false. Returns
> > + * 0 on success and -1 on error.
> > + */
> > +int virStringParseYesNo(const char *str,
> > +                       bool *result)
> > +{
> > +  bool r = false;
>
> > +
> > +  if (!str)
>
> Here you don't report an error ...
>
> > +    return -1;
> > +
> > +  if (STREQ(str, "yes")) {
> > +    r = true;
> > +  } else if (STREQ(str, "no")) {
> > +    r = false;
>
> This variable is not very useful. If the string matches you can
> unconditionally overwrite *result.
>
>
> > +  } else {
> > +    virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                 _("Invalid value, '%s' must be either 'yes' or 'no'"), str);
>
> ... but here you do. We try to do consistent error reporting since then it's
> hard to figure out whether to report an error or not.
>
> > +    return -1;
> > +  }
> > +
> > +  *result = r;
> > +
> > +  return 0;
> > +}
> > diff --git a/src/util/virstring.h b/src/util/virstring.h
> > index 1e36ac459c..b921d5407b 100644
> > --- a/src/util/virstring.h
> > +++ b/src/util/virstring.h
> > @@ -316,6 +316,9 @@ int virStringParsePort(const char *str,
> >                         unsigned int *port)
> >      ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> >
> > +int virStringParseYesNo(const char *str,
> > +                       bool *result);
>
> This should have a ATTRIBUTE_NONNULL(2) since 'result' is dereferenced

ATTRIBUTE_NONNULL expands only if we're running under static analysis, IIRC
there was an issue that if we enabled it unconditionally, GCC then optimized

if (!var)
    statements;

away...Instead, I believe we want to enforce 'if (!var)' checks.

Erik