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

[libvirt] [PATCH 2/2] conf: Use virStringParseYesNo()

Shotaro Gotanda posted 2 patches 5 years, 8 months ago
There is a newer version of this series
[libvirt] [PATCH 2/2] conf: Use virStringParseYesNo()
Posted by Shotaro Gotanda 5 years, 8 months ago 
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

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

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