src/conf/domain_conf.c | 55 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-)
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
© 2016 - 2024 Red Hat, Inc.