10:38 a.m.
These patches are based on a bite-sized task proposed in
https://wiki.libvirt.org/page/BiteSizedTasks
and the mentor of this bite-sized task is Cole Robinson
<crobinso(a)redhat.com>
(Do I need to put the mentor in cc, in this case?)
The function virStringParseYesNo() convert the string "yes" into bool
true and "no" into false, and error if we receive anything other than
those values.
To be honest, I'm not confident on the error handling in libvirt.
So, please check that point.
Regards,
Shotaro
Shotaro Gotanda (2):
util: add virStringParseYesNo()
conf: Use virStringParseYesNo()
src/conf/domain_conf.c | 30 +++++-------------------------
src/conf/secret_conf.c | 12 ++----------
src/util/virstring.c | 32 ++++++++++++++++++++++++++++++++
src/util/virstring.h | 3 +++
4 files changed, 42 insertions(+), 35 deletions(-)
--
2.19.1
10:38 a.m.
New subject: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
This function parse and convert string "yes|no" into bool true|false.
Signed-off-by: Shotaro Gotanda <g.sho1500(a)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
2:38 a.m.
New subject: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
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>
---
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:54 a.m.
New subject: [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>
> ---
> 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
3:10 a.m.
New subject: [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>
> > ---
> > 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.
3:41 a.m.
New subject: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
Thank you for your feedbacks.
I’ll fix the problem and send the fixed patches.
Regards,
Shotaro
3:48 a.m.
New subject: [libvirt] [PATCH 1/2] util: add virStringParseYesNo()
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(a)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
10:38 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Use virStringParseYesNo()
This commit remove redundant common pattern in our XML parsing
that convert string 'yes' into true and 'no' into false,
and error if we receive anything other values.
Signed-off-by: Shotaro Gotanda <g.sho1500(a)gmail.com>
---
src/conf/domain_conf.c | 30 +++++-------------------------
src/conf/secret_conf.c | 12 ++----------
2 files changed, 7 insertions(+), 35 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 995f87bcbe..3830926f3d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8412,11 +8412,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
p = virXMLPropStringLimit(ctxt->node, "relabel",
VIR_SECURITY_LABEL_BUFLEN-1);
if (p) {
- if (STREQ(p, "yes")) {
- seclabel->relabel = true;
- } else if (STREQ(p, "no")) {
- seclabel->relabel = false;
- } else {
+ if (virStringParseYesNo(p, &seclabel->relabel) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("invalid security relabel value %s"), p);
goto error;
@@ -8646,11 +8642,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr
**seclabels_rtn,
relabel = virXMLPropString(list[i], "relabel");
if (relabel != NULL) {
- if (STREQ(relabel, "yes")) {
- seclabels[i]->relabel = true;
- } else if (STREQ(relabel, "no")) {
- seclabels[i]->relabel = false;
- } else {
+ if (virStringParseYesNo(relabel, &seclabels[i]->relabel) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("invalid security relabel value %s"),
relabel);
@@ -13653,11 +13645,7 @@ virDomainGraphicsDefParseXMLSDL(virDomainGraphicsDefPtr def,
ctxt->node = node;
if (fullscreen != NULL) {
- if (STREQ(fullscreen, "yes")) {
- def->data.sdl.fullscreen = true;
- } else if (STREQ(fullscreen, "no")) {
- def->data.sdl.fullscreen = false;
- } else {
+ if (virStringParseYesNo(fullscreen, &def->data.sdl.fullscreen) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unknown fullscreen value '%s'"),
fullscreen);
goto cleanup;
@@ -13745,11 +13733,7 @@ virDomainGraphicsDefParseXMLDesktop(virDomainGraphicsDefPtr def,
VIR_AUTOFREE(char *) fullscreen = virXMLPropString(node, "fullscreen");
if (fullscreen != NULL) {
- if (STREQ(fullscreen, "yes")) {
- def->data.desktop.fullscreen = true;
- } else if (STREQ(fullscreen, "no")) {
- def->data.desktop.fullscreen = false;
- } else {
+ if (virStringParseYesNo(fullscreen, &def->data.desktop.fullscreen) < 0)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unknown fullscreen value '%s'"),
fullscreen);
return -1;
@@ -15458,11 +15442,7 @@ virDomainRedirFilterUSBDevDefParseXML(xmlNodePtr node)
allow = virXMLPropString(node, "allow");
if (allow) {
- if (STREQ(allow, "yes")) {
- def->allow = true;
- } else if (STREQ(allow, "no")) {
- def->allow = false;
- } else {
+ if (virStringParseYesNo(allow, &def->allow) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Invalid allow value, either 'yes' or
'no'"));
goto error;
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index ca6cc194a2..5b85a7c0be 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -147,11 +147,7 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root)
prop = virXPathString("string(./@ephemeral)", ctxt);
if (prop != NULL) {
- if (STREQ(prop, "yes")) {
- def->isephemeral = true;
- } else if (STREQ(prop, "no")) {
- def->isephemeral = false;
- } else {
+ if (virStringParseYesNo(prop, &def->isephemeral) < 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("invalid value of 'ephemeral'"));
goto cleanup;
@@ -161,11 +157,7 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root)
prop = virXPathString("string(./@private)", ctxt);
if (prop != NULL) {
- if (STREQ(prop, "yes")) {
- def->isprivate = true;
- } else if (STREQ(prop, "no")) {
- def->isprivate = false;
- } else {
+ if (virStringParseYesNo(prop, &def->isprivate) < 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("invalid value of 'private'"));
goto cleanup;
--
2.19.1
2:40 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Use virStringParseYesNo()
On Tue, Mar 12, 2019 at 00:38:06 +0900, Shotaro Gotanda wrote:
This commit remove redundant common pattern in our XML parsing
that convert string 'yes' into true and 'no' into false,
and error if we receive anything other values.
Signed-off-by: Shotaro Gotanda <g.sho1500(a)gmail.com>
---
src/conf/domain_conf.c | 30 +++++-------------------------
src/conf/secret_conf.c | 12 ++----------
2 files changed, 7 insertions(+), 35 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 995f87bcbe..3830926f3d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8412,11 +8412,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
p = virXMLPropStringLimit(ctxt->node, "relabel",
VIR_SECURITY_LABEL_BUFLEN-1);
if (p) {
- if (STREQ(p, "yes")) {
- seclabel->relabel = true;
- } else if (STREQ(p, "no")) {
- seclabel->relabel = false;
- } else {
+ if (virStringParseYesNo(p, &seclabel->relabel) < 0) {
Since virStringParseYesNo reports an error this usage will result in the
error being overwritten. Since the error message here is better as the
one reported in virStringParseYesNo we should keep the original error
handling.
Probably the best solution will be for virStringParseYesNo to avoid
reporting an error and just return -1 in case when the value could not
be converted.
virReportError(VIR_ERR_XML_ERROR,
_("invalid security relabel value %s"), p);
goto error;
3 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Use virStringParseYesNo()
On Tue, Mar 12, 2019 at 08:40:53AM +0100, Peter Krempa wrote:
On Tue, Mar 12, 2019 at 00:38:06 +0900, Shotaro Gotanda wrote:
> This commit remove redundant common pattern in our XML parsing
> that convert string 'yes' into true and 'no' into false,
> and error if we receive anything other values.
>
> Signed-off-by: Shotaro Gotanda <g.sho1500(a)gmail.com>
> ---
> src/conf/domain_conf.c | 30 +++++-------------------------
> src/conf/secret_conf.c | 12 ++----------
> 2 files changed, 7 insertions(+), 35 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 995f87bcbe..3830926f3d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8412,11 +8412,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
> p = virXMLPropStringLimit(ctxt->node, "relabel",
> VIR_SECURITY_LABEL_BUFLEN-1);
> if (p) {
> - if (STREQ(p, "yes")) {
> - seclabel->relabel = true;
> - } else if (STREQ(p, "no")) {
> - seclabel->relabel = false;
> - } else {
> + if (virStringParseYesNo(p, &seclabel->relabel) < 0) {
Since virStringParseYesNo reports an error this usage will result in the
error being overwritten. Since the error message here is better as the
one reported in virStringParseYesNo we should keep the original error
handling.
Probably the best solution will be for virStringParseYesNo to avoid
reporting an error and just return -1 in case when the value could not
be converted.
I agree, we should treat this as our virStrToX helpers.
Erik
2048
days inactive
2049
days old
9 comments
4 participants
participants (4)
-
Erik Skultety -
Peter Krempa -
Shotaro Gotanda -
五反田正太郎