6:30 a.m.
diff to v1:
* split one commit into two
* rewrite the functions in the second one to use the
switch (suggested by Daniel, thanks)
v1: https://listman.redhat.com/archives/libvir-list/2022-July/232989.html
Kristina Hanicova (2):
domain_conf: remove else after return / goto
domain_conf: rewrite if else functions to switch
src/conf/domain_conf.c | 77 +++++++++++++++++++++++++++---------------
1 file changed, 49 insertions(+), 28 deletions(-)
--
2.35.3
6:30 a.m.
New subject: [PATCH v2 1/2] domain_conf: remove else after return / goto
The else branches are redundant because the execution will never
reach them if the conditions in the previous 'if' branches are
true.
I think this looks cleaner and is more readable, because having
'else' branch indicates that no return / break / goto is in the
previous branch and the function can reach it.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
src/conf/domain_conf.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 26a241db38..41d23cfe2a 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;
}
@@ -6028,7 +6028,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"));
@@ -6040,8 +6042,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;
@@ -9913,9 +9916,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. */
@@ -9924,7 +9928,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"));
@@ -10004,9 +10009,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) {
@@ -10016,9 +10022,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
7:58 a.m.
New subject: [PATCH v2 1/2] domain_conf: remove else after return / goto
On Thu, Jul 21, 2022 at 01:30:44PM +0200, Kristina Hanicova wrote:
The else branches are redundant because the execution will never
reach them if the conditions in the previous 'if' branches are
true.
I think this looks cleaner and is more readable, because having
'else' branch indicates that no return / break / goto is in the
previous branch and the function can reach it.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
6:30 a.m.
New subject: [PATCH v2 2/2] domain_conf: rewrite if else functions to switch
Pattern of using switch instead of a long if else construction is
used everywhere, so I used it here as well to make the code more
consistent (and remove that else after return). I also included
all the values from the enum.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
src/conf/domain_conf.c | 42 ++++++++++++++++++++++++++++--------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 41d23cfe2a..a4b3a49086 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8055,19 +8055,26 @@ static int
virDomainControllerModelTypeFromString(const virDomainControllerDef *def,
const char *model)
{
- if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+ switch ((virDomainControllerType)def->type) {
+ case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
return virDomainControllerModelSCSITypeFromString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
+ case VIR_DOMAIN_CONTROLLER_TYPE_USB:
return virDomainControllerModelUSBTypeFromString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+ case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
return virDomainControllerModelPCITypeFromString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
return virDomainControllerModelIDETypeFromString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
+ case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
return virDomainControllerModelVirtioSerialTypeFromString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
+ case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
return virDomainControllerModelISATypeFromString(model);
-
+ case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+ case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+ case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
+ case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+ return -1;
+ }
return -1;
}
@@ -8076,19 +8083,26 @@ static const char *
virDomainControllerModelTypeToString(virDomainControllerDef *def,
int model)
{
- if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+ switch ((virDomainControllerType)def->type) {
+ case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
return virDomainControllerModelSCSITypeToString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB)
+ case VIR_DOMAIN_CONTROLLER_TYPE_USB:
return virDomainControllerModelUSBTypeToString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI)
+ case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
return virDomainControllerModelPCITypeToString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
+ case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
return virDomainControllerModelIDETypeToString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
+ case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
return virDomainControllerModelVirtioSerialTypeToString(model);
- else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA)
+ case VIR_DOMAIN_CONTROLLER_TYPE_ISA:
return virDomainControllerModelISATypeToString(model);
-
+ case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
+ case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
+ case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
+ case VIR_DOMAIN_CONTROLLER_TYPE_XENBUS:
+ case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
+ return NULL;
+ }
return NULL;
}
--
2.35.3
7:58 a.m.
New subject: [PATCH v2 2/2] domain_conf: rewrite if else functions to switch
On Thu, Jul 21, 2022 at 01:30:45PM +0200, Kristina Hanicova wrote:
Pattern of using switch instead of a long if else construction is
used everywhere, so I used it here as well to make the code more
consistent (and remove that else after return). I also included
all the values from the enum.
The commit messages are usually non-subjective, as in:
"... use it here..." and "Also include ...", but it's not a rule.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
Reviewed-by: Martin Kletzander <mkletzan(a)redhat.com>
890
days inactive
890
days old
4 comments
2 participants
participants (2)
-
Kristina Hanicova -
Martin Kletzander