On Tue, Mar 12, 2019 at 09:10:07AM +0100, Peter Krempa wrote:
> 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@xxxxxxxxx>
> > > > ---
> > > > 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.
Well, in general, a check would be better than leaving the daemon to SIGSEGV,
but then again, I give you that this is a very simple helper and any such
issues would turn up even during our unit test suite.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
