[libvirt] [PATCH 0/3] use virStringParseYesNo helper
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks. Cc: abologna@redhat.com Cc: berrange@redhat.com Cc: crobinso@redhat.com Cc: gene@czarc.net Cc: g.sho1500@gmail.com Cc: jdenemar@redhat.com Cc: laine@laine.org Cc: mkletzan@redhat.com Cc: phrdina@redhat.com Mao Zhongyi (3): conf/domain_conf: use virStringParseYesNo helper conf/network_conf: use virStringParseYesNo helper qemu/qemu_migration_params: use virStringParseYesNo helper src/conf/domain_conf.c | 30 ++++++++++++++---------------- src/conf/network_conf.c | 4 +--- src/qemu/qemu_migration_params.c | 6 +----- 3 files changed, 16 insertions(+), 24 deletions(-) -- 2.17.1
This helper performs a conversion from a "yes|no" string to a corresponding boolean, and several conversions were already done, but there are still some omissions. For most of the remaining usages in domain_conf.c only "yes" is explicitly checked for. This means all other values are implicitly handled as 'false'. In this case, use virStringParseYesNo, but if it returns -1, don't raise an error but set the bool value to false. Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: abologna@redhat.com Cc: laine@laine.org Cc: phrdina@redhat.com Cc: mkletzan@redhat.com Cc: g.sho1500@gmail.com Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/domain_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10d6bf0eea..7420658726 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7601,8 +7601,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, } if ((autoAddress = virXMLPropString(node, "autoAddress"))) { - if (STREQ(autoAddress, "yes")) - usbsrc->autoAddress = true; + if (virStringParseYesNo(autoAddress, &usbsrc->autoAddress) < 0) + usbsrc->autoAddress = false; } /* Product can validly be 0, so we need some extra help to determine @@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, * (e.g. <interface>) with type='hostdev') */ if ((managed = virXMLPropString(node, "managed")) != NULL) { - if (STREQ(managed, "yes")) - def->managed = true; + if (virStringParseYesNo(managed, &def->managed) < 0) + def->managed = false; } sgio = virXMLPropString(node, "sgio"); @@ -13807,9 +13807,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, if (autoGenerated && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { - if (STREQ(autoGenerated, "yes")) { - def->autoGenerated = true; - } else if (STRNEQ(autoGenerated, "no")) { + if (virStringParseYesNo(autoGenerated, &def->autoGenerated) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid autoGenerated value: %s"), autoGenerated); @@ -13939,12 +13937,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } if (autoport) { - if (STREQ(autoport, "yes")) { - if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) - def->data.vnc.port = 0; - def->data.vnc.autoport = true; - } else { + if (virStringParseYesNo(autoport, &def->data.vnc.autoport) < 0) { def->data.vnc.autoport = false; + } else { + if (def->data.vnc.autoport && flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) + def->data.vnc.port = 0; } } @@ -13958,8 +13955,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } } - if (websocketGenerated && STREQ(websocketGenerated, "yes")) - def->data.vnc.websocketGenerated = true; + if (websocketGenerated && + virStringParseYesNo(websocketGenerated, &def->data.vnc.websocketGenerated)) + def->data.vnc.websocketGenerated = false; if (sharePolicy) { int policy = @@ -15479,8 +15477,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, heads = virXMLPropString(cur, "heads"); if ((primary = virXMLPropString(cur, "primary")) != NULL) { - if (STREQ(primary, "yes")) - def->primary = true; + if (virStringParseYesNo(primary, &def->primary) < 0) + def->primary = false; VIR_FREE(primary); } -- 2.17.1
On 10/16/19 4:39 AM, Mao Zhongyi wrote:
This helper performs a conversion from a "yes|no" string to a corresponding boolean, and several conversions were already done, but there are still some omissions.
For most of the remaining usages in domain_conf.c only "yes" is explicitly checked for. This means all other values are implicitly handled as 'false'. In this case, use virStringParseYesNo, but if it returns -1, don't raise an error but set the bool value to false.
Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: abologna@redhat.com Cc: laine@laine.org Cc: phrdina@redhat.com Cc: mkletzan@redhat.com Cc: g.sho1500@gmail.com
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/domain_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10d6bf0eea..7420658726 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7601,8 +7601,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, }
if ((autoAddress = virXMLPropString(node, "autoAddress"))) { - if (STREQ(autoAddress, "yes")) - usbsrc->autoAddress = true; + if (virStringParseYesNo(autoAddress, &usbsrc->autoAddress) < 0) + usbsrc->autoAddress = false;
I think we should either report a proper error here or don't touch this line (because this attribute is generated by libvirt itself, it doesn't come from an user).
}
/* Product can validly be 0, so we need some extra help to determine @@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, * (e.g. <interface>) with type='hostdev') */ if ((managed = virXMLPropString(node, "managed")) != NULL) { - if (STREQ(managed, "yes")) - def->managed = true; + if (virStringParseYesNo(managed, &def->managed) < 0) + def->managed = false;
Here we need to report a proper error because "managed" attribute comes from user, therefore risk of an user providing invalid value is high compared to the first hunk. But if we want to be consistent we should report error in both cases.
}
sgio = virXMLPropString(node, "sgio"); @@ -13807,9 +13807,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
if (autoGenerated && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { - if (STREQ(autoGenerated, "yes")) { - def->autoGenerated = true; - } else if (STRNEQ(autoGenerated, "no")) { + if (virStringParseYesNo(autoGenerated, &def->autoGenerated) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid autoGenerated value: %s"), autoGenerated);
This one is correct.
@@ -13939,12 +13937,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, }
if (autoport) { - if (STREQ(autoport, "yes")) { - if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) - def->data.vnc.port = 0; - def->data.vnc.autoport = true; - } else { + if (virStringParseYesNo(autoport, &def->data.vnc.autoport) < 0) { def->data.vnc.autoport = false; + } else { + if (def->data.vnc.autoport && flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) + def->data.vnc.port = 0;
This doesn't need to go under else branch really.
} }
@@ -13958,8 +13955,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } }
- if (websocketGenerated && STREQ(websocketGenerated, "yes")) - def->data.vnc.websocketGenerated = true; + if (websocketGenerated && + virStringParseYesNo(websocketGenerated, &def->data.vnc.websocketGenerated))
Ooops, you've missed '< 0' comparison.
+ def->data.vnc.websocketGenerated = false;
if (sharePolicy) { int policy = @@ -15479,8 +15477,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, heads = virXMLPropString(cur, "heads");
if ((primary = virXMLPropString(cur, "primary")) != NULL) { - if (STREQ(primary, "yes")) - def->primary = true; + if (virStringParseYesNo(primary, &def->primary) < 0) + def->primary = false;
Again, I'd rather see an error reported here.
VIR_FREE(primary); }
Michal
Hi, Michal On 10/16/19 4:38 PM, Michal Privoznik wrote:
On 10/16/19 4:39 AM, Mao Zhongyi wrote:
This helper performs a conversion from a "yes|no" string to a corresponding boolean, and several conversions were already done, but there are still some omissions.
For most of the remaining usages in domain_conf.c only "yes" is explicitly checked for. This means all other values are implicitly handled as 'false'. In this case, use virStringParseYesNo, but if it returns -1, don't raise an error but set the bool value to false.
Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: abologna@redhat.com Cc: laine@laine.org Cc: phrdina@redhat.com Cc: mkletzan@redhat.com Cc: g.sho1500@gmail.com
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/domain_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10d6bf0eea..7420658726 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7601,8 +7601,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, } if ((autoAddress = virXMLPropString(node, "autoAddress"))) { - if (STREQ(autoAddress, "yes")) - usbsrc->autoAddress = true; + if (virStringParseYesNo(autoAddress, &usbsrc->autoAddress) < 0) + usbsrc->autoAddress = false;
I think we should either report a proper error here or don't touch this line (because this attribute is generated by libvirt itself, it doesn't come from an user).
This patch replaces all STREQs that explicitly handle 'yes' to true, reserving the original implicitly handling of other values to false, so not report an error, which is also the recommendation of Cole Robinson in BiteSizedtasks. Therefor, I'm not very sure whether it is better to report an error in this case? If indeed, will fix it in v2.
} /* Product can validly be 0, so we need some extra help to determine @@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, * (e.g. <interface>) with type='hostdev') */ if ((managed = virXMLPropString(node, "managed")) != NULL) { - if (STREQ(managed, "yes")) - def->managed = true; + if (virStringParseYesNo(managed, &def->managed) < 0) + def->managed = false;
Here we need to report a proper error because "managed" attribute comes from user, therefore risk of an user providing invalid value is high compared to the first hunk. But if we want to be consistent we should report error in both cases.
} sgio = virXMLPropString(node, "sgio"); @@ -13807,9 +13807,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, if (autoGenerated && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { - if (STREQ(autoGenerated, "yes")) { - def->autoGenerated = true; - } else if (STRNEQ(autoGenerated, "no")) { + if (virStringParseYesNo(autoGenerated, &def->autoGenerated) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid autoGenerated value: %s"), autoGenerated);
This one is correct.
@@ -13939,12 +13937,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } if (autoport) { - if (STREQ(autoport, "yes")) { - if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) - def->data.vnc.port = 0; - def->data.vnc.autoport = true; - } else { + if (virStringParseYesNo(autoport, &def->data.vnc.autoport) < 0) { def->data.vnc.autoport = false; + } else { + if (def->data.vnc.autoport && flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) + def->data.vnc.port = 0;
This doesn't need to go under else branch really.
right
} } @@ -13958,8 +13955,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } } - if (websocketGenerated && STREQ(websocketGenerated, "yes")) - def->data.vnc.websocketGenerated = true; + if (websocketGenerated && + virStringParseYesNo(websocketGenerated, &def->data.vnc.websocketGenerated))
Ooops, you've missed '< 0' comparison.
virStringParseYesNo return 0 on succeed and -1 on error, and here is && operation, so, no explicit comparison '< 0'. Thank you very much for your review. :) Mao
+ def->data.vnc.websocketGenerated = false; if (sharePolicy) { int policy = @@ -15479,8 +15477,8 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, heads = virXMLPropString(cur, "heads"); if ((primary = virXMLPropString(cur, "primary")) != NULL) { - if (STREQ(primary, "yes")) - def->primary = true; + if (virStringParseYesNo(primary, &def->primary) < 0) + def->primary = false;
Again, I'd rather see an error reported here.
VIR_FREE(primary); }
Michal
On 10/16/19 12:10 PM, maozy wrote:
Hi, Michal
On 10/16/19 4:38 PM, Michal Privoznik wrote:
On 10/16/19 4:39 AM, Mao Zhongyi wrote:
This helper performs a conversion from a "yes|no" string to a corresponding boolean, and several conversions were already done, but there are still some omissions.
For most of the remaining usages in domain_conf.c only "yes" is explicitly checked for. This means all other values are implicitly handled as 'false'. In this case, use virStringParseYesNo, but if it returns -1, don't raise an error but set the bool value to false.
Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: abologna@redhat.com Cc: laine@laine.org Cc: phrdina@redhat.com Cc: mkletzan@redhat.com Cc: g.sho1500@gmail.com
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/domain_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10d6bf0eea..7420658726 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7601,8 +7601,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, } if ((autoAddress = virXMLPropString(node, "autoAddress"))) { - if (STREQ(autoAddress, "yes")) - usbsrc->autoAddress = true; + if (virStringParseYesNo(autoAddress, &usbsrc->autoAddress) < 0) + usbsrc->autoAddress = false;
I think we should either report a proper error here or don't touch this line (because this attribute is generated by libvirt itself, it doesn't come from an user).
This patch replaces all STREQs that explicitly handle 'yes' to true, reserving the original implicitly handling of other values to false, so not report an error, which is also the recommendation of Cole Robinson in BiteSizedtasks. Therefor, I'm not very sure whether it is better to report an error in this case? If indeed, will fix it in v2.
Alright then. But what we can use then is: ignore_value(virStringParseYesNo(autoAddress, &usbsrc->autoAddress)); which is equivallent to existing code and also shorter.
} /* Product can validly be 0, so we need some extra help to determine @@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, * (e.g. <interface>) with type='hostdev') */ if ((managed = virXMLPropString(node, "managed")) != NULL) { - if (STREQ(managed, "yes")) - def->managed = true; + if (virStringParseYesNo(managed, &def->managed) < 0) + def->managed = false;
Here we need to report a proper error because "managed" attribute comes from user, therefore risk of an user providing invalid value is high compared to the first hunk. But if we want to be consistent we should report error in both cases.
} sgio = virXMLPropString(node, "sgio"); @@ -13807,9 +13807,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, if (autoGenerated && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { - if (STREQ(autoGenerated, "yes")) { - def->autoGenerated = true; - } else if (STRNEQ(autoGenerated, "no")) { + if (virStringParseYesNo(autoGenerated, &def->autoGenerated) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid autoGenerated value: %s"), autoGenerated);
This one is correct.
@@ -13939,12 +13937,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } if (autoport) { - if (STREQ(autoport, "yes")) { - if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) - def->data.vnc.port = 0; - def->data.vnc.autoport = true; - } else { + if (virStringParseYesNo(autoport, &def->data.vnc.autoport) < 0) { def->data.vnc.autoport = false; + } else { + if (def->data.vnc.autoport && flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) + def->data.vnc.port = 0;
This doesn't need to go under else branch really.
right
} } @@ -13958,8 +13955,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } } - if (websocketGenerated && STREQ(websocketGenerated, "yes")) - def->data.vnc.websocketGenerated = true; + if (websocketGenerated && + virStringParseYesNo(websocketGenerated, &def->data.vnc.websocketGenerated))
Ooops, you've missed '< 0' comparison.
virStringParseYesNo return 0 on succeed and -1 on error, and here is && operation, so, no explicit comparison '< 0'.
The comparison is required because even though the function doesn't return any positive value now it might at some point in the future. In libvirt, there's the following semantics for integer return values: < 0 : an error state, 0 : success,
0 : success, but also something is signalized to the caller
There's plenty of examples, but let me pick just one: The retvals for virConfGetValueBool() are defined as follows: -1 on error, 0 if missing, 1 if the value was present And in this code you're adding, you're interested whether the function failed or not. If we ever add another success value (e.g. 1), we don't have to fix all the places where the function is called. Michal
On 10/16/19 7:02 PM, Michal Privoznik wrote:
On 10/16/19 12:10 PM, maozy wrote:
Hi, Michal
On 10/16/19 4:38 PM, Michal Privoznik wrote:
On 10/16/19 4:39 AM, Mao Zhongyi wrote:
This helper performs a conversion from a "yes|no" string to a corresponding boolean, and several conversions were already done, but there are still some omissions.
For most of the remaining usages in domain_conf.c only "yes" is explicitly checked for. This means all other values are implicitly handled as 'false'. In this case, use virStringParseYesNo, but if it returns -1, don't raise an error but set the bool value to false.
Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: abologna@redhat.com Cc: laine@laine.org Cc: phrdina@redhat.com Cc: mkletzan@redhat.com Cc: g.sho1500@gmail.com
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/domain_conf.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10d6bf0eea..7420658726 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7601,8 +7601,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, } if ((autoAddress = virXMLPropString(node, "autoAddress"))) { - if (STREQ(autoAddress, "yes")) - usbsrc->autoAddress = true; + if (virStringParseYesNo(autoAddress, &usbsrc->autoAddress) < 0) + usbsrc->autoAddress = false;
I think we should either report a proper error here or don't touch this line (because this attribute is generated by libvirt itself, it doesn't come from an user).
This patch replaces all STREQs that explicitly handle 'yes' to true, reserving the original implicitly handling of other values to false, so not report an error, which is also the recommendation of Cole Robinson in BiteSizedtasks. Therefor, I'm not very sure whether it is better to report an error in this case? If indeed, will fix it in v2.
Alright then. But what we can use then is:
ignore_value(virStringParseYesNo(autoAddress, &usbsrc->autoAddress));
which is equivallent to existing code and also shorter.
} /* Product can validly be 0, so we need some extra help to determine @@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, * (e.g. <interface>) with type='hostdev') */ if ((managed = virXMLPropString(node, "managed")) != NULL) { - if (STREQ(managed, "yes")) - def->managed = true; + if (virStringParseYesNo(managed, &def->managed) < 0) + def->managed = false;
Here we need to report a proper error because "managed" attribute comes from user, therefore risk of an user providing invalid value is high compared to the first hunk. But if we want to be consistent we should report error in both cases.
} sgio = virXMLPropString(node, "sgio"); @@ -13807,9 +13807,7 @@ virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def, if (autoGenerated && flags & VIR_DOMAIN_DEF_PARSE_STATUS) { - if (STREQ(autoGenerated, "yes")) { - def->autoGenerated = true; - } else if (STRNEQ(autoGenerated, "no")) { + if (virStringParseYesNo(autoGenerated, &def->autoGenerated) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid autoGenerated value: %s"), autoGenerated);
This one is correct.
@@ -13939,12 +13937,11 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } if (autoport) { - if (STREQ(autoport, "yes")) { - if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) - def->data.vnc.port = 0; - def->data.vnc.autoport = true; - } else { + if (virStringParseYesNo(autoport, &def->data.vnc.autoport) < 0) { def->data.vnc.autoport = false; + } else { + if (def->data.vnc.autoport && flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) + def->data.vnc.port = 0;
This doesn't need to go under else branch really.
right
} } @@ -13958,8 +13955,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } } - if (websocketGenerated && STREQ(websocketGenerated, "yes")) - def->data.vnc.websocketGenerated = true; + if (websocketGenerated && + virStringParseYesNo(websocketGenerated, &def->data.vnc.websocketGenerated))
Ooops, you've missed '< 0' comparison.
virStringParseYesNo return 0 on succeed and -1 on error, and here is && operation, so, no explicit comparison '< 0'.
The comparison is required because even though the function doesn't return any positive value now it might at some point in the future. In libvirt, there's the following semantics for integer return values:
< 0 : an error state, 0 : success,
0 : success, but also something is signalized to the caller
There's plenty of examples, but let me pick just one: The retvals for virConfGetValueBool() are defined as follows:
-1 on error, 0 if missing, 1 if the value was present
And in this code you're adding, you're interested whether the function failed or not. If we ever add another success value (e.g. 1), we don't have to fix all the places where the function is called.
Well, very far-sighted. I got it. Thanks for the detailed and kind reply. Mao
Michal
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks. Cc: gene@czarc.net Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: g.sho1500@gmail.com Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/network_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 75ec5ccc27..9954c3d25f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1682,9 +1682,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, */ ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt); if (ipv6nogwStr) { - if (STREQ(ipv6nogwStr, "yes")) { - def->ipv6nogw = true; - } else if (STRNEQ(ipv6nogwStr, "no")) { + if (virStringParseYesNo(ipv6nogwStr, &def->ipv6nogw) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid ipv6 setting '%s' in network '%s'"), ipv6nogwStr, def->name); -- 2.17.1
On 10/16/19 4:39 AM, Mao Zhongyi wrote:
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks.
Cc: gene@czarc.net Cc: crobinso@redhat.com Cc: berrange@redhat.com Cc: g.sho1500@gmail.com
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/conf/network_conf.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 75ec5ccc27..9954c3d25f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1682,9 +1682,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt, */ ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt); if (ipv6nogwStr) { - if (STREQ(ipv6nogwStr, "yes")) { - def->ipv6nogw = true; - } else if (STRNEQ(ipv6nogwStr, "no")) { + if (virStringParseYesNo(ipv6nogwStr, &def->ipv6nogw) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid ipv6 setting '%s' in network '%s'"), ipv6nogwStr, def->name);
ACK Michal
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks. Cc: jdenemar@redhat.com Cc: g.sho1500@gmail.com Cc: crobinso@redhat.com Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/qemu/qemu_migration_params.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 85fa8f8de5..acd42042fe 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1326,11 +1326,7 @@ qemuMigrationParamsParse(xmlXPathContextPtr ctxt, break; case QEMU_MIGRATION_PARAM_TYPE_BOOL: - if (STREQ(value, "yes")) - pv->value.b = true; - else if (STREQ(value, "no")) - pv->value.b = false; - else + if (virStringParseYesNo(value, &pv->value.b) < 0) rc = -1; break; -- 2.17.1
On 10/16/19 4:39 AM, Mao Zhongyi wrote:
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks.
Cc: jdenemar@redhat.com Cc: g.sho1500@gmail.com Cc: crobinso@redhat.com
Signed-off-by: Mao Zhongyi <maozhongyi@cmss.chinamobile.com> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> --- src/qemu/qemu_migration_params.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 85fa8f8de5..acd42042fe 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -1326,11 +1326,7 @@ qemuMigrationParamsParse(xmlXPathContextPtr ctxt, break;
case QEMU_MIGRATION_PARAM_TYPE_BOOL: - if (STREQ(value, "yes")) - pv->value.b = true; - else if (STREQ(value, "no")) - pv->value.b = false; - else + if (virStringParseYesNo(value, &pv->value.b) < 0) rc = -1; break;
Or simply rc = virStringParseYesNo(...), but your change is good too. Michal
On Wed, 2019-10-16 at 10:39 +0800, Mao Zhongyi wrote:
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks.
Cc: abologna@redhat.com Cc: berrange@redhat.com Cc: crobinso@redhat.com Cc: gene@czarc.net Cc: g.sho1500@gmail.com Cc: jdenemar@redhat.com Cc: laine@laine.org Cc: mkletzan@redhat.com Cc: phrdina@redhat.com
Please do *not* CC random libvirt developer when sending patches to the list: we are all subscribed, and someone will get to your patches eventually; if you haven't gotten any replies after two weeks or so, then it's perfectly fine to ping but once again do that on the list, not targeting specific individuals. -- Andrea Bolognani / Red Hat / Virtualization
On 10/16/19 3:51 PM, Andrea Bolognani wrote:
On Wed, 2019-10-16 at 10:39 +0800, Mao Zhongyi wrote:
A function virStringParseYesNo was added to convert string 'yes' to true and 'no' to false, so use this helper to replace 'STREQ(.*, \"yes\")' and 'STREQ(.*, \"no\")' as it allows us to drop several repetitive if-then-else string->bool conversion blocks.
Cc: abologna@redhat.com Cc: berrange@redhat.com Cc: crobinso@redhat.com Cc: gene@czarc.net Cc: g.sho1500@gmail.com Cc: jdenemar@redhat.com Cc: laine@laine.org Cc: mkletzan@redhat.com Cc: phrdina@redhat.com
Please do *not* CC random libvirt developer when sending patches to the list: we are all subscribed, and someone will get to your patches eventually; if you haven't gotten any replies after two weeks or so, then it's perfectly fine to ping but once again do that on the list, not targeting specific individuals.
Ok, I got it, thanks for the clarification. Thanks, Mao
participants (4)
-
Andrea Bolognani -
Mao Zhongyi -
maozy
-
Michal Privoznik