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

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(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 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.