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

[Peter Krempa]

On Tue, Mar 12, 2019 at 08:54:12 +0100, Erik Skultety wrote:
> 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

Yes that is true. That's the reason why it expands to nothing with GCC.

> 
> if (!var)
>     statements;

So that these work.

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

That depends on the situation. Baby proofing against programmer errors
(which would be checking that 'result' is not null in this case) would
be too much here.

Similarly the check above is bad as it does not report an error. If you
can't come up with a reasonable error which to show to the user the
check probably does not make sense. This would apply to checking that
'result' is not NULL as that messge would somehow have to imply that
it's a programming failure, which is not entirely desirable.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190312/cd903449/attachment-0001.sig>