Re: [PATCH] domain_conf: remove else after return / goto

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(a)redhat.com&gt; > --- > 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 :|