[libvirt] [PATCH 0/2] Add function for XML yes|no string handling
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@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
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
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
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
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.
Thank you for your feedbacks. I’ll fix the problem and send the fixed patches. Regards, Shotaro
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
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@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
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@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;
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@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
participants (4)
-
Erik Skultety -
Peter Krempa -
Shotaro Gotanda -
五反田正太郎