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

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(a)gmail.com&gt; > > --- > > 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.