[PATCH] domain_conf: remove else after return / goto
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 55 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44a01ab628..bc4b74c1c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption *xmlopt, if (domain->newDef) return domain->newDef; - else - return domain->def; + + return domain->def; } @@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm, if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; - else - return vm->def; + + return vm->def; } @@ -6029,7 +6029,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONZERO, &scsisrc->sgio)) < 0) { return -1; - } else if (rv > 0) { + } + + if (rv > 0) { if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("sgio is only supported for scsi host device")); @@ -6041,8 +6043,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONE, &scsisrc->rawio)) < 0) { return -1; - } else if (rv > 0 && - def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + } + + if (rv > 0 && def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("rawio is only supported for scsi host device")); return -1; @@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def, { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) return virDomainControllerModelSCSITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) return virDomainControllerModelUSBTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) return virDomainControllerModelIDETypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) return virDomainControllerModelVirtioSerialTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) return virDomainControllerModelISATypeFromString(model); return -1; @@ -8077,15 +8080,15 @@ virDomainControllerModelTypeToString(virDomainControllerDef *def, { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) return virDomainControllerModelSCSITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) return virDomainControllerModelUSBTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) return virDomainControllerModelIDETypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) return virDomainControllerModelVirtioSerialTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) return virDomainControllerModelISATypeToString(model); return NULL; @@ -9915,9 +9918,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, ctxt->node = cur; - if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) { + if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) goto error; - } else if (nsources > 0) { + + if (nsources > 0) { /* Parse only the first source element since only one is used * for chardev devices, the only exception is UDP type, where * user can specify two source elements. */ @@ -9926,7 +9930,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, _("only one source element is allowed for " "character device")); goto error; - } else if (nsources > 2) { + } + if (nsources > 2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only two source elements are allowed for " "character device")); @@ -10006,9 +10011,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, } } - if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) { + if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) goto error; - } else if (nlogs == 1) { + + if (nlogs == 1) { if (virDomainChrSourceDefParseLog(def, logs[0]) < 0) goto error; } else if (nlogs > 1) { @@ -10018,9 +10024,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, goto error; } - if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) { + if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) goto error; - } else if (nprotocols == 1) { + + if (nprotocols == 1) { if (virDomainChrSourceDefParseProtocol(def, protocols[0]) < 0) goto error; } else if (nprotocols > 1) { -- 2.35.3
What's the reasoning for making this change ? On Wed, Jul 20, 2022 at 02:59:58PM +0200, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 55 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44a01ab628..bc4b74c1c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption *xmlopt,
if (domain->newDef) return domain->newDef; - else - return domain->def; + + return domain->def; } @@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; - else - return vm->def; + + return vm->def; }
I'm not really convinced these two changes are better.
@@ -6029,7 +6029,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONZERO, &scsisrc->sgio)) < 0) { return -1; - } else if (rv > 0) { + } + + if (rv > 0) { if (def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("sgio is only supported for scsi host device")); @@ -6041,8 +6043,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONE, &scsisrc->rawio)) < 0) { return -1; - } else if (rv > 0 && - def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + } + + if (rv > 0 && def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("rawio is only supported for scsi host device")); return -1;
I don't mind eliminating the else, when the first 'if' is just an error return/goto case.
@@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def, { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) return virDomainControllerModelSCSITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) return virDomainControllerModelUSBTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) return virDomainControllerModelIDETypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) return virDomainControllerModelVirtioSerialTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) return virDomainControllerModelISATypeFromString(model);
This giant if/else should be a switch instead
return -1; @@ -8077,15 +8080,15 @@ virDomainControllerModelTypeToString(virDomainControllerDef *def, { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) return virDomainControllerModelSCSITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) return virDomainControllerModelUSBTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) return virDomainControllerModelIDETypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) return virDomainControllerModelVirtioSerialTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) return virDomainControllerModelISATypeToString(model);
Likewise a switch.
return NULL; @@ -9915,9 +9918,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
ctxt->node = cur;
- if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) { + if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) goto error; - } else if (nsources > 0) { + + if (nsources > 0) { /* Parse only the first source element since only one is used * for chardev devices, the only exception is UDP type, where * user can specify two source elements. */ @@ -9926,7 +9930,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, _("only one source element is allowed for " "character device")); goto error; - } else if (nsources > 2) { + } + if (nsources > 2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only two source elements are allowed for " "character device")); @@ -10006,9 +10011,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, } }
- if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) { + if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) goto error; - } else if (nlogs == 1) { + + if (nlogs == 1) { if (virDomainChrSourceDefParseLog(def, logs[0]) < 0) goto error; } else if (nlogs > 1) { @@ -10018,9 +10024,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDef *def, goto error; }
- if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) { + if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) goto error; - } else if (nprotocols == 1) { + + if (nprotocols == 1) { if (virDomainChrSourceDefParseProtocol(def, protocols[0]) < 0) goto error; } else if (nprotocols > 1) {
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jul 20, 2022 at 3:34 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
What's the reasoning for making this change ?
I stumbled upon this and decided to rewrite code in src/conf/ that could be easily improved.
On Wed, Jul 20, 2022 at 02:59:58PM +0200, Kristina Hanicova wrote:
Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- src/conf/domain_conf.c | 55 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44a01ab628..bc4b74c1c8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4084,8 +4084,8 @@ virDomainObjGetPersistentDef(virDomainXMLOption *xmlopt,
if (domain->newDef) return domain->newDef; - else - return domain->def; + + return domain->def; } @@ -4223,8 +4223,8 @@ virDomainObjGetOneDefState(virDomainObj *vm,
if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) return vm->newDef; - else - return vm->def; + + return vm->def; }
I'm not really convinced these two changes are better.
Well, the else after return is redundant because it will never reach the 'else' branch if the condition is true. I think this looks cleaner and is more readable, because having 'else' branch indicates to me that no return / break / goto is in the previous branch and the funcion can reach it.
@@ -6029,7 +6029,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONZERO, &scsisrc->sgio)) < 0) { return -1; - } else if (rv > 0) { + } + + if (rv > 0) { if (def->source.subsys.type !=
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
virReportError(VIR_ERR_XML_ERROR, "%s", _("sgio is only supported for scsi host
device"));
@@ -6041,8 +6043,9 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, VIR_XML_PROP_NONE, &scsisrc->rawio)) < 0) { return -1; - } else if (rv > 0 && - def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + } + + if (rv > 0 && def->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { virReportError(VIR_ERR_XML_ERROR, "%s", _("rawio is only supported for scsi host device")); return -1;
I don't mind eliminating the else, when the first 'if' is just an error return/goto case.
@@ -8056,15 +8059,15 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def, { if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) return virDomainControllerModelSCSITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) return virDomainControllerModelUSBTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) return virDomainControllerModelIDETypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) return virDomainControllerModelVirtioSerialTypeFromString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) return virDomainControllerModelISATypeFromString(model);
This giant if/else should be a switch instead
Thanks, way better idea.
return -1; @@ -8077,15 +8080,15 @@
virDomainControllerModelTypeToString(virDomainControllerDef *def,
{ if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) return virDomainControllerModelSCSITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) return virDomainControllerModelUSBTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) return virDomainControllerModelIDETypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) return virDomainControllerModelVirtioSerialTypeToString(model); - else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) + if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA) return virDomainControllerModelISATypeToString(model);
Likewise a switch.
return NULL; @@ -9915,9 +9918,10 @@
virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
ctxt->node = cur;
- if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) { + if ((nsources = virXPathNodeSet("./source", ctxt, &sources)) < 0) goto error; - } else if (nsources > 0) { + + if (nsources > 0) { /* Parse only the first source element since only one is used * for chardev devices, the only exception is UDP type, where * user can specify two source elements. */ @@ -9926,7 +9930,8 @@
virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
_("only one source element is allowed for " "character device")); goto error; - } else if (nsources > 2) { + } + if (nsources > 2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("only two source elements are allowed for " "character device")); @@ -10006,9 +10011,10 @@
virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
} }
- if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) { + if ((nlogs = virXPathNodeSet("./log", ctxt, &logs)) < 0) goto error; - } else if (nlogs == 1) { + + if (nlogs == 1) { if (virDomainChrSourceDefParseLog(def, logs[0]) < 0) goto error; } else if (nlogs > 1) { @@ -10018,9 +10024,10 @@
virDomainChrSourceDefParseXML(virDomainChrSourceDef *def,
goto error; }
- if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols))
< 0) {
+ if ((nprotocols = virXPathNodeSet("./protocol", ctxt, &protocols)) < 0) goto error; - } else if (nprotocols == 1) { + + if (nprotocols == 1) { if (virDomainChrSourceDefParseProtocol(def, protocols[0]) < 0) goto error; } else if (nprotocols > 1) {
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Regards, Kristina
participants (2)
-
Daniel P. Berrangé -
Kristina Hanicova