1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] Add function for XML yes|no string handling
  4. View patch

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

Shotaro Gotanda posted 2 patches 5 years, 8 months ago
There is a newer version of this series
[libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Posted by Shotaro Gotanda 5 years, 8 months ago 
This function parse and convert string "yes|no" into bool true|false.

Signed-off-by: Shotaro Gotanda <g.sho1500@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)
+    return -1;
+
+  if (STREQ(str, "yes")) {
+    r = true;
+  } else if (STREQ(str, "no")) {
+    r = false;
+  } else {
+    virReportError(VIR_ERR_INTERNAL_ERROR,
+                 _("Invalid value, '%s' must be either 'yes' or 'no'"), str);
+    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);
+
 /**
  * VIR_AUTOSTRINGLIST:
  *
-- 
2.19.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Posted by Peter Krempa 5 years, 8 months ago 
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@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
without checking and also ATTRIBUTE_RETURN_CHECK as it reports an error.

> +
>  /**
>   * VIR_AUTOSTRINGLIST:
>   *
> -- 
> 2.19.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Posted by Erik Skultety 5 years, 8 months ago 
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Posted by Peter Krempa 5 years, 8 months ago 
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@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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Posted by Erik Skultety 5 years, 8 months ago 
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@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.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Posted by 五反田正太郎 5 years, 8 months ago 
Thank you for your feedbacks.
I’ll fix the problem and send the fixed patches.

Regards,
Shotaro
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.