[PATCH v2 2/2] qemu_alias: change return type to void if possible

Kristina Hanicova posted 2 patches 4 years, 2 months ago
[PATCH v2 2/2] qemu_alias: change return type to void if possible
Posted by Kristina Hanicova 4 years, 2 months ago
These functions always return success so it seems logical to not
return anything and remove unnecessary checks.

Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
---
 src/qemu/qemu_alias.c   | 78 +++++++++++++++++------------------------
 src/qemu/qemu_alias.h   | 34 +++++++++---------
 src/qemu/qemu_hotplug.c | 34 +++++++-----------
 3 files changed, 61 insertions(+), 85 deletions(-)

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index ff5c7f1bd8..1b80b7559e 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -120,14 +120,14 @@ qemuAssignDeviceChrAlias(virDomainDef *def,
 }
 
 
-int
+void
 qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
                                 virDomainControllerDef *controller)
 {
     const char *prefix = virDomainControllerTypeToString(controller->type);
 
     if (controller->info.alias)
-        return 0;
+        return;
 
     if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
         if (!virQEMUCapsHasPCIMultiBus(domainDef)) {
@@ -136,21 +136,21 @@ qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
              * "pci".
              */
             controller->info.alias = g_strdup("pci");
-            return 0;
+            return;
         } else if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
             /* The pcie-root controller on Q35 machinetypes uses a
              * different naming convention ("pcie.0"), because it is
              * hardcoded that way in qemu.
              */
             controller->info.alias = g_strdup_printf("pcie.%d", controller->idx);
-            return 0;
+            return;
         }
         /* All other PCI controllers use the consistent "pci.%u"
          * (including the hardcoded pci-root controller on
          * multibus-capable qemus).
          */
         controller->info.alias = g_strdup_printf("pci.%d", controller->idx);
-        return 0;
+        return;
     } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
         /* for any machine based on e.g. I440FX or G3Beige, the
          * first (and currently only) IDE controller is an integrated
@@ -159,7 +159,7 @@ qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
         if (qemuDomainHasBuiltinIDE(domainDef) &&
             controller->idx == 0) {
             controller->info.alias = g_strdup("ide");
-            return 0;
+            return;
         }
     } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) {
         /* for any Q35 machine, the first SATA controller is the
@@ -167,26 +167,25 @@ qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
          */
         if (qemuDomainIsQ35(domainDef) && controller->idx == 0) {
             controller->info.alias = g_strdup("ide");
-            return 0;
+            return;
         }
     } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
         /* first USB device is "usb", others are normal "usb%d" */
         if (controller->idx == 0) {
             controller->info.alias = g_strdup("usb");
-            return 0;
+            return;
         }
     } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
         if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_NCR53C90 &&
             controller->idx == 0) {
             controller->info.alias = g_strdup("scsi");
-            return 0;
+            return;
         }
     }
     /* all other controllers use the default ${type}${index} naming
      * scheme for alias/id.
      */
     controller->info.alias = g_strdup_printf("%s%d", prefix, controller->idx);
-    return 0;
 }
 
 
@@ -264,13 +263,13 @@ qemuAssignDeviceDiskAlias(virDomainDef *def,
 }
 
 
-int
+void
 qemuAssignDeviceHostdevAlias(virDomainDef *def,
                              char **alias,
                              int idx)
 {
     if (*alias)
-        return 0;
+        return;
 
     if (idx == -1) {
         size_t i;
@@ -296,25 +295,25 @@ qemuAssignDeviceHostdevAlias(virDomainDef *def,
     }
 
     *alias = g_strdup_printf("hostdev%d", idx);
-
-    return 0;
 }
 
 
-int
+void
 qemuAssignDeviceNetAlias(virDomainDef *def,
                          virDomainNetDef *net,
                          int idx)
 {
     if (net->info.alias)
-        return 0;
+        return;
 
     /* <interface type='hostdev'> uses "hostdevN" as the alias
      * We must use "-1" as the index because the caller doesn't know
      * that we're now looking for a unique hostdevN rather than netN
      */
-    if (virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV)
-        return qemuAssignDeviceHostdevAlias(def, &net->info.alias, -1);
+    if (virDomainNetResolveActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+        qemuAssignDeviceHostdevAlias(def, &net->info.alias, -1);
+        return;
+    }
 
     if (idx == -1) {
         size_t i;
@@ -331,11 +330,10 @@ qemuAssignDeviceNetAlias(virDomainDef *def,
     }
 
     net->info.alias = g_strdup_printf("net%d", idx);
-    return 0;
 }
 
 
-int
+void
 qemuAssignDeviceFSAlias(virDomainDef *def,
                         virDomainFSDef *fss)
 {
@@ -343,7 +341,7 @@ qemuAssignDeviceFSAlias(virDomainDef *def,
     int maxidx = 0;
 
     if (fss->info.alias)
-        return 0;
+        return;
 
     for (i = 0; i < def->nfss; i++) {
         int idx;
@@ -353,7 +351,6 @@ qemuAssignDeviceFSAlias(virDomainDef *def,
     }
 
     fss->info.alias = g_strdup_printf("fs%d", maxidx);
-    return 0;
 }
 
 
@@ -411,13 +408,13 @@ qemuAssignDeviceTPMAlias(virDomainTPMDef *tpm,
 }
 
 
-int
+void
 qemuAssignDeviceRedirdevAlias(virDomainDef *def,
                               virDomainRedirdevDef *redirdev,
                               int idx)
 {
     if (redirdev->info.alias)
-        return 0;
+        return;
 
     if (idx == -1) {
         size_t i;
@@ -432,11 +429,10 @@ qemuAssignDeviceRedirdevAlias(virDomainDef *def,
     }
 
     redirdev->info.alias = g_strdup_printf("redir%d", idx);
-    return 0;
 }
 
 
-int
+void
 qemuAssignDeviceRNGAlias(virDomainDef *def,
                          virDomainRNGDef *rng)
 {
@@ -445,7 +441,7 @@ qemuAssignDeviceRNGAlias(virDomainDef *def,
     int idx;
 
     if (rng->info.alias)
-        return 0;
+        return;
 
     for (i = 0; i < def->nrngs; i++) {
         if ((idx = qemuDomainDeviceAliasIndex(&def->rngs[i]->info, "rng")) >= maxidx)
@@ -453,8 +449,6 @@ qemuAssignDeviceRNGAlias(virDomainDef *def,
     }
 
     rng->info.alias = g_strdup_printf("rng%d", maxidx);
-
-    return 0;
 }
 
 
@@ -535,13 +529,13 @@ qemuAssignDeviceMemoryAlias(virDomainDef *def,
 }
 
 
-int
+void
 qemuAssignDeviceShmemAlias(virDomainDef *def,
                            virDomainShmemDef *shmem,
                            int idx)
 {
     if (shmem->info.alias)
-        return 0;
+        return;
 
     if (idx == -1) {
         size_t i;
@@ -559,7 +553,6 @@ qemuAssignDeviceShmemAlias(virDomainDef *def,
     }
 
     shmem->info.alias = g_strdup_printf("shmem%d", idx);
-    return 0;
 }
 
 
@@ -613,13 +606,11 @@ qemuAssignDeviceAliases(virDomainDef *def, virQEMUCaps *qemuCaps)
             return -1;
     }
     for (i = 0; i < def->nnets; i++) {
-        if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0)
-            return -1;
+        qemuAssignDeviceNetAlias(def, def->nets[i], -1);
     }
 
     for (i = 0; i < def->nfss; i++) {
-        if (qemuAssignDeviceFSAlias(def, def->fss[i]) < 0)
-            return -1;
+        qemuAssignDeviceFSAlias(def, def->fss[i]);
     }
     for (i = 0; i < def->nsounds; i++) {
         qemuAssignDeviceSoundAlias(def->sounds[i], i);
@@ -630,19 +621,16 @@ qemuAssignDeviceAliases(virDomainDef *def, virQEMUCaps *qemuCaps)
          * linked to a NetDef, they will share an info and the alias
          * will already be set, so don't try to set it again.
          */
-        if (qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0)
-            return -1;
+        qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1);
     }
     for (i = 0; i < def->nredirdevs; i++) {
-        if (qemuAssignDeviceRedirdevAlias(def, def->redirdevs[i], i) < 0)
-            return -1;
+        qemuAssignDeviceRedirdevAlias(def, def->redirdevs[i], i);
     }
     for (i = 0; i < def->nvideos; i++) {
         qemuAssignDeviceVideoAlias(def->videos[i], i);
     }
     for (i = 0; i < def->ncontrollers; i++) {
-        if (qemuAssignDeviceControllerAlias(def, def->controllers[i]) < 0)
-            return -1;
+        qemuAssignDeviceControllerAlias(def, def->controllers[i]);
     }
     for (i = 0; i < def->ninputs; i++) {
         qemuAssignDeviceInputAlias(def, def->inputs[i], i);
@@ -667,8 +655,7 @@ qemuAssignDeviceAliases(virDomainDef *def, virQEMUCaps *qemuCaps)
         qemuAssignDeviceHubAlias(def->hubs[i], i);
     }
     for (i = 0; i < def->nshmems; i++) {
-        if (qemuAssignDeviceShmemAlias(def, def->shmems[i], i) < 0)
-            return -1;
+        qemuAssignDeviceShmemAlias(def, def->shmems[i], i);
     }
     for (i = 0; i < def->nsmartcards; i++) {
         qemuAssignDeviceSmartcardAlias(def->smartcards[i], i);
@@ -681,8 +668,7 @@ qemuAssignDeviceAliases(virDomainDef *def, virQEMUCaps *qemuCaps)
         qemuAssignDeviceMemballoonAlias(def->memballoon, 0);
     }
     for (i = 0; i < def->nrngs; i++) {
-        if (qemuAssignDeviceRNGAlias(def, def->rngs[i]) < 0)
-            return -1;
+        qemuAssignDeviceRNGAlias(def, def->rngs[i]);
     }
     for (i = 0; i < def->ntpms; i++) {
         qemuAssignDeviceTPMAlias(def->tpms[i], i);
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 35db0dc736..8fc27f4696 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -31,39 +31,39 @@ int qemuAssignDeviceChrAlias(virDomainDef *def,
                              virDomainChrDef *chr,
                              ssize_t idx);
 
-int qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
-                                    virDomainControllerDef *controller);
+void qemuAssignDeviceControllerAlias(virDomainDef *domainDef,
+                                     virDomainControllerDef *controller);
 
 int qemuAssignDeviceDiskAlias(virDomainDef *def,
                               virDomainDiskDef *disk,
                               virQEMUCaps *qemuCaps);
 
-int qemuAssignDeviceHostdevAlias(virDomainDef *def,
-                                 char **alias,
-                                 int idx);
+void qemuAssignDeviceHostdevAlias(virDomainDef *def,
+                                  char **alias,
+                                  int idx);
 
-int qemuAssignDeviceNetAlias(virDomainDef *def,
-                             virDomainNetDef *net,
-                             int idx);
+void qemuAssignDeviceNetAlias(virDomainDef *def,
+                              virDomainNetDef *net,
+                              int idx);
 
-int
+void
 qemuAssignDeviceFSAlias(virDomainDef *def,
                         virDomainFSDef *fss);
 
-int qemuAssignDeviceRedirdevAlias(virDomainDef *def,
-                                  virDomainRedirdevDef *redirdev,
-                                  int idx);
+void qemuAssignDeviceRedirdevAlias(virDomainDef *def,
+                                   virDomainRedirdevDef *redirdev,
+                                   int idx);
 
-int qemuAssignDeviceRNGAlias(virDomainDef *def,
-                             virDomainRNGDef *rng);
+void qemuAssignDeviceRNGAlias(virDomainDef *def,
+                              virDomainRNGDef *rng);
 
 int qemuAssignDeviceMemoryAlias(virDomainDef *def,
                                 virDomainMemoryDef *mems,
                                 bool oldAlias);
 
-int qemuAssignDeviceShmemAlias(virDomainDef *def,
-                               virDomainShmemDef *shmem,
-                               int idx);
+void qemuAssignDeviceShmemAlias(virDomainDef *def,
+                                virDomainShmemDef *shmem,
+                                int idx);
 
 void qemuAssignDeviceWatchdogAlias(virDomainWatchdogDef *watchdog);
 
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 33466c40d6..426710786b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -869,8 +869,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriver *driver,
     if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev) < 0)
         return -1;
 
-    if (qemuAssignDeviceControllerAlias(vm->def, controller) < 0)
-        goto cleanup;
+    qemuAssignDeviceControllerAlias(vm->def, controller);
 
     if (qemuBuildControllerDevProps(vm->def, controller, priv->qemuCaps, &devprops) < 0)
         goto cleanup;
@@ -1221,8 +1220,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
 
     actualType = virDomainNetGetActualType(net);
 
-    if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0)
-        goto cleanup;
+    qemuAssignDeviceNetAlias(vm->def, net, -1);
 
     if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
         /* This is really a "smart hostdev", so it should be attached
@@ -1699,8 +1697,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriver *driver,
     if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO)
         teardownlabel = true;
 
-    if (qemuAssignDeviceHostdevAlias(vm->def, &info->alias, -1) < 0)
-        goto error;
+    qemuAssignDeviceHostdevAlias(vm->def, &info->alias, -1);
 
     if (qemuDomainIsPSeries(vm->def))
         /* Isolation groups are only relevant for pSeries guests */
@@ -1960,8 +1957,7 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriver *driver,
     const char *secAlias = NULL;
     virErrorPtr orig_err;
 
-    if (qemuAssignDeviceRedirdevAlias(def, redirdev, -1) < 0)
-        return -1;
+    qemuAssignDeviceRedirdevAlias(def, redirdev, -1);
 
     if (!(charAlias = qemuAliasChardevFromDevAlias(redirdev->info.alias)))
         return -1;
@@ -2310,8 +2306,7 @@ qemuDomainAttachRNGDevice(virQEMUDriver *driver,
     virJSONValue *props = NULL;
     int ret = -1;
 
-    if (qemuAssignDeviceRNGAlias(vm->def, rng) < 0)
-        goto cleanup;
+    qemuAssignDeviceRNGAlias(vm->def, rng);
 
     /* preallocate space for the device definition */
     VIR_REALLOC_N(vm->def->rngs, vm->def->nrngs + 1);
@@ -2587,8 +2582,8 @@ qemuDomainAttachHostUSBDevice(virQEMUDriver *driver,
         goto cleanup;
     teardownlabel = true;
 
-    if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
-        goto cleanup;
+    qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1);
+
     if (!(devprops = qemuBuildUSBHostdevDevProps(vm->def, hostdev, priv->qemuCaps)))
         goto cleanup;
 
@@ -2667,8 +2662,7 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriver *driver,
         goto cleanup;
     teardownlabel = true;
 
-    if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
-        goto cleanup;
+    qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1);
 
     if (qemuDomainPrepareHostdev(hostdev, priv) < 0)
         goto cleanup;
@@ -2786,8 +2780,7 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriver *driver,
     }
     releaseaddr = true;
 
-    if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
-        goto cleanup;
+    qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1);
 
     if (!(devprops = qemuBuildSCSIVHostHostdevDevProps(vm->def,
                                                        hostdev,
@@ -2899,8 +2892,7 @@ qemuDomainAttachMediatedDevice(virQEMUDriver *driver,
         goto cleanup;
     teardownlabel = true;
 
-    if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
-        goto cleanup;
+    qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1);
 
     if (!(devprops = qemuBuildHostdevMediatedDevProps(vm->def, hostdev)))
         goto cleanup;
@@ -3028,8 +3020,7 @@ qemuDomainAttachShmemDevice(virQEMUDriver *driver,
         return -1;
     }
 
-    if (qemuAssignDeviceShmemAlias(vm->def, shmem, -1) < 0)
-        return -1;
+    qemuAssignDeviceShmemAlias(vm->def, shmem, -1);
 
     qemuDomainPrepareShmemChardev(shmem);
 
@@ -3438,8 +3429,7 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver,
     if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev) < 0)
         return -1;
 
-    if (qemuAssignDeviceFSAlias(vm->def, fs) < 0)
-        goto cleanup;
+    qemuAssignDeviceFSAlias(vm->def, fs);
 
     chardev = virDomainChrSourceDefNew(priv->driver->xmlopt);
     chardev->type = VIR_DOMAIN_CHR_TYPE_UNIX;
-- 
2.31.1

Re: [PATCH v2 2/2] qemu_alias: change return type to void if possible
Posted by Michal Prívozník 4 years, 2 months ago
On 11/24/21 17:12, Kristina Hanicova wrote:
> These functions always return success so it seems logical to not
> return anything and remove unnecessary checks.
> 
> Signed-off-by: Kristina Hanicova <khanicov@redhat.com>
> ---
>  src/qemu/qemu_alias.c   | 78 +++++++++++++++++------------------------
>  src/qemu/qemu_alias.h   | 34 +++++++++---------
>  src/qemu/qemu_hotplug.c | 34 +++++++-----------
>  3 files changed, 61 insertions(+), 85 deletions(-)


Ooops, not rebased onto current master :-) But the conflict resolution
was trivial.

I think I see some more functions that can't fail really (can be fixed
in a follow up), for instance: qemuAssignDeviceChrAlias() can fail if
and only if qemuGetNextChrDevIndex() fails, but that can never happen.
This leaves us with qemuAssignDeviceDiskAlias() and
qemuAssignDeviceMemoryAlias(). In the latter, the error is returned only
in enum overflow (which I believe is a dead code). In the former -1 is
returned if qemuDomainFindSCSIControllerModel() fails, but controllers
should have been autofilled at this point.

Michal