[libvirt] [PATCH] qemu: Dissolve qemuBuildVhostuserCommandLine in qemuBuildInterfaceCommandLine

Michal Privoznik posted 1 patch 5 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/9424bef1ccf873c1718e6117e2e5976d5904e454.1541163377.git.mprivozn@redhat.com
src/qemu/qemu_command.c | 113 +++++++++++++++++-----------------------
1 file changed, 48 insertions(+), 65 deletions(-)
[libvirt] [PATCH] qemu: Dissolve qemuBuildVhostuserCommandLine in qemuBuildInterfaceCommandLine
Posted by Michal Privoznik 5 years, 4 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1524230

The qemuBuildVhostuserCommandLine builds command line for
vhostuser type interfaces. It is duplicating some code of the
function it is called from (qemuBuildInterfaceCommandLine)
because of the way it's called. If we merge it into the caller
not only we save a few lines but we also enable checks that we
would have to duplicate otherwise (e.g. QoS availability).

While at it, reorder some VIR_FREE() in
qemuBuildInterfaceCommandLine so that it is easier to track which
variables are freed and which are not.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c | 113 +++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 65 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6e3ff67660..e338d3172e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8224,34 +8224,24 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
 }
 
 static int
-qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
+qemuInterfaceVhostuserConnect(virQEMUDriverPtr driver,
                               virLogManagerPtr logManager,
                               virSecurityManagerPtr secManager,
                               virCommandPtr cmd,
                               virDomainDefPtr def,
                               virDomainNetDefPtr net,
                               virQEMUCapsPtr qemuCaps,
-                              unsigned int bootindex)
+                              char **chardev)
 {
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-    char *chardev = NULL;
-    char *netdev = NULL;
-    unsigned int queues = net->driver.virtio.queues;
-    char *nic = NULL;
     int ret = -1;
 
-    if (!qemuDomainSupportsNicdev(def, net)) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "%s", _("Nicdev support unavailable"));
-        goto cleanup;
-    }
-
     switch ((virDomainChrType)net->data.vhostuser->type) {
     case VIR_DOMAIN_CHR_TYPE_UNIX:
-        if (!(chardev = qemuBuildChrChardevStr(logManager, secManager,
-                                               cmd, cfg, def,
-                                               net->data.vhostuser,
-                                               net->info.alias, qemuCaps, 0)))
+        if (!(*chardev = qemuBuildChrChardevStr(logManager, secManager,
+                                                cmd, cfg, def,
+                                                net->data.vhostuser,
+                                                net->info.alias, qemuCaps, 0)))
             goto cleanup;
         break;
 
@@ -8274,42 +8264,9 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (queues > 1 &&
-        !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("multi-queue is not supported for vhost-user "
-                         "with this QEMU binary"));
-        goto cleanup;
-    }
-
-    if (!(netdev = qemuBuildHostNetStr(net, driver,
-                                       NULL, 0, NULL, 0)))
-        goto cleanup;
-
-    if (virNetDevOpenvswitchGetVhostuserIfname(net->data.vhostuser->data.nix.path,
-                                               &net->ifname) < 0)
-        goto cleanup;
-
-    virCommandAddArg(cmd, "-chardev");
-    virCommandAddArg(cmd, chardev);
-
-    virCommandAddArg(cmd, "-netdev");
-    virCommandAddArg(cmd, netdev);
-
-    if (!(nic = qemuBuildNicDevStr(def, net, bootindex,
-                                   queues, qemuCaps))) {
-        goto cleanup;
-    }
-
-    virCommandAddArgList(cmd, "-device", nic, NULL);
-
     ret = 0;
  cleanup:
     virObjectUnref(cfg);
-    VIR_FREE(netdev);
-    VIR_FREE(chardev);
-    VIR_FREE(nic);
-
     return ret;
 }
 
@@ -8328,7 +8285,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
                               int **nicindexes)
 {
     int ret = -1;
-    char *nic = NULL, *host = NULL;
+    char *nic = NULL;
+    char *host = NULL;
+    char *chardev = NULL;
     int *tapfd = NULL;
     size_t tapfdSize = 0;
     int *vhostfd = NULL;
@@ -8337,6 +8296,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
     char **vhostfdName = NULL;
     virDomainNetType actualType = virDomainNetGetActualType(net);
     virNetDevBandwidthPtr actualBandwidth;
+    bool requireNicdev = false;
     size_t i;
 
 
@@ -8447,9 +8407,24 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
         break;
 
     case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
-        ret = qemuBuildVhostuserCommandLine(driver, logManager, secManager, cmd, def,
-                                            net, qemuCaps, bootindex);
-        goto cleanup;
+        requireNicdev = true;
+
+        if (net->driver.virtio.queues > 1 &&
+            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VHOSTUSER_MULTIQUEUE)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("multi-queue is not supported for vhost-user "
+                             "with this QEMU binary"));
+            goto cleanup;
+        }
+
+        if (qemuInterfaceVhostuserConnect(driver, logManager, secManager,
+                                          cmd, def, net, qemuCaps, &chardev) < 0)
+            goto cleanup;
+
+        if (virNetDevOpenvswitchGetVhostuserIfname(net->data.vhostuser->data.nix.path,
+                                                   &net->ifname) < 0)
+            goto cleanup;
+
         break;
 
     case VIR_DOMAIN_NET_TYPE_USER:
@@ -8564,6 +8539,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
             goto cleanup;
     }
 
+    if (chardev)
+        virCommandAddArgList(cmd, "-chardev", chardev, NULL);
+
     if (!(host = qemuBuildHostNetStr(net, driver,
                                      tapfdName, tapfdSize,
                                      vhostfdName, vhostfdSize)))
@@ -8578,13 +8556,17 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
      */
     if (qemuDomainSupportsNicdev(def, net)) {
         if (!(nic = qemuBuildNicDevStr(def, net, bootindex,
-                                       vhostfdSize, qemuCaps)))
+                                       net->driver.virtio.queues, qemuCaps)))
             goto cleanup;
         virCommandAddArgList(cmd, "-device", nic, NULL);
-    } else {
+    } else if (!requireNicdev) {
         if (!(nic = qemuBuildLegacyNicStr(net)))
             goto cleanup;
         virCommandAddArgList(cmd, "-net", nic, NULL);
+    } else {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "%s", _("Nicdev support unavailable"));
+        goto cleanup;
     }
 
     ret = 0;
@@ -8595,24 +8577,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
         virSetError(saved_err);
         virFreeError(saved_err);
     }
-    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
-        if (ret < 0)
-            VIR_FORCE_CLOSE(tapfd[i]);
-        if (tapfdName)
-            VIR_FREE(tapfdName[i]);
-    }
     for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
         if (ret < 0)
             VIR_FORCE_CLOSE(vhostfd[i]);
         if (vhostfdName)
             VIR_FREE(vhostfdName[i]);
     }
-    VIR_FREE(tapfd);
+    VIR_FREE(vhostfdName);
+    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
+        if (ret < 0)
+            VIR_FORCE_CLOSE(tapfd[i]);
+        if (tapfdName)
+            VIR_FREE(tapfdName[i]);
+    }
+    VIR_FREE(tapfdName);
     VIR_FREE(vhostfd);
-    VIR_FREE(nic);
+    VIR_FREE(tapfd);
+    VIR_FREE(chardev);
     VIR_FREE(host);
-    VIR_FREE(tapfdName);
-    VIR_FREE(vhostfdName);
+    VIR_FREE(nic);
     return ret;
 }
 
-- 
2.18.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Dissolve qemuBuildVhostuserCommandLine in qemuBuildInterfaceCommandLine
Posted by Erik Skultety 5 years, 4 months ago
On Fri, Nov 02, 2018 at 01:56:17PM +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
>
> The qemuBuildVhostuserCommandLine builds command line for
> vhostuser type interfaces. It is duplicating some code of the
> function it is called from (qemuBuildInterfaceCommandLine)
> because of the way it's called. If we merge it into the caller
> not only we save a few lines but we also enable checks that we
> would have to duplicate otherwise (e.g. QoS availability).
>
> While at it, reorder some VIR_FREE() in
> qemuBuildInterfaceCommandLine so that it is easier to track which
> variables are freed and which are not.

Sounds like ^this would go into a separate trivial patch.

>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c | 113 +++++++++++++++++-----------------------
>  1 file changed, 48 insertions(+), 65 deletions(-)
>
...

> @@ -8595,24 +8577,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>          virSetError(saved_err);
>          virFreeError(saved_err);
>      }
> -    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
> -        if (ret < 0)
> -            VIR_FORCE_CLOSE(tapfd[i]);
> -        if (tapfdName)
> -            VIR_FREE(tapfdName[i]);
> -    }
>      for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
>          if (ret < 0)
>              VIR_FORCE_CLOSE(vhostfd[i]);
>          if (vhostfdName)
>              VIR_FREE(vhostfdName[i]);
>      }
> -    VIR_FREE(tapfd);
> +    VIR_FREE(vhostfdName);
> +    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
> +        if (ret < 0)
> +            VIR_FORCE_CLOSE(tapfd[i]);
> +        if (tapfdName)
> +            VIR_FREE(tapfdName[i]);
> +    }
> +    VIR_FREE(tapfdName);
>      VIR_FREE(vhostfd);
> -    VIR_FREE(nic);
> +    VIR_FREE(tapfd);
> +    VIR_FREE(chardev);
>      VIR_FREE(host);
> -    VIR_FREE(tapfdName);
> -    VIR_FREE(vhostfdName);
> +    VIR_FREE(nic);

I don't see how ^this hunk made it better. If anything, then the VIR_FREEs
should be probably coupled like:

VIR_FREE(tapfd);
VIR_FREE(tapfdName);
VIR_FREE(vhostfd);
VIR_FREE(vhostfdName);
<the rest of them...>

It would also need to be a separate patch. To the rest of the changes:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Dissolve qemuBuildVhostuserCommandLine in qemuBuildInterfaceCommandLine
Posted by Michal Privoznik 5 years, 4 months ago
On 11/02/2018 03:44 PM, Erik Skultety wrote:
> On Fri, Nov 02, 2018 at 01:56:17PM +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
>>
>> The qemuBuildVhostuserCommandLine builds command line for
>> vhostuser type interfaces. It is duplicating some code of the
>> function it is called from (qemuBuildInterfaceCommandLine)
>> because of the way it's called. If we merge it into the caller
>> not only we save a few lines but we also enable checks that we
>> would have to duplicate otherwise (e.g. QoS availability).
>>
>> While at it, reorder some VIR_FREE() in
>> qemuBuildInterfaceCommandLine so that it is easier to track which
>> variables are freed and which are not.
> 
> Sounds like ^this would go into a separate trivial patch.
> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_command.c | 113 +++++++++++++++++-----------------------
>>  1 file changed, 48 insertions(+), 65 deletions(-)
>>
> ...
> 
>> @@ -8595,24 +8577,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>>          virSetError(saved_err);
>>          virFreeError(saved_err);
>>      }
>> -    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
>> -        if (ret < 0)
>> -            VIR_FORCE_CLOSE(tapfd[i]);
>> -        if (tapfdName)
>> -            VIR_FREE(tapfdName[i]);
>> -    }
>>      for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
>>          if (ret < 0)
>>              VIR_FORCE_CLOSE(vhostfd[i]);
>>          if (vhostfdName)
>>              VIR_FREE(vhostfdName[i]);
>>      }
>> -    VIR_FREE(tapfd);
>> +    VIR_FREE(vhostfdName);
>> +    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
>> +        if (ret < 0)
>> +            VIR_FORCE_CLOSE(tapfd[i]);
>> +        if (tapfdName)
>> +            VIR_FREE(tapfdName[i]);
>> +    }
>> +    VIR_FREE(tapfdName);
>>      VIR_FREE(vhostfd);
>> -    VIR_FREE(nic);
>> +    VIR_FREE(tapfd);
>> +    VIR_FREE(chardev);
>>      VIR_FREE(host);
>> -    VIR_FREE(tapfdName);
>> -    VIR_FREE(vhostfdName);
>> +    VIR_FREE(nic);
> 
> I don't see how ^this hunk made it better. If anything, then the VIR_FREEs
> should be probably coupled like:
> 
> VIR_FREE(tapfd);
> VIR_FREE(tapfdName);
> VIR_FREE(vhostfd);
> VIR_FREE(vhostfdName);
> <the rest of them...>

Why is that? I can see two reasonable orderings. If you have variables
A, B, C, you either free them in the same order or in reverse C, B, A.
Any other is just hard to follow.

Michal

> 
> It would also need to be a separate patch. To the rest of the changes:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 

Okay, I'll separate the reordering in a separate trivial patch.

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Dissolve qemuBuildVhostuserCommandLine in qemuBuildInterfaceCommandLine
Posted by Erik Skultety 5 years, 4 months ago
On Fri, Nov 02, 2018 at 04:01:44PM +0100, Michal Privoznik wrote:
> On 11/02/2018 03:44 PM, Erik Skultety wrote:
> > On Fri, Nov 02, 2018 at 01:56:17PM +0100, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1524230
> >>
> >> The qemuBuildVhostuserCommandLine builds command line for
> >> vhostuser type interfaces. It is duplicating some code of the
> >> function it is called from (qemuBuildInterfaceCommandLine)
> >> because of the way it's called. If we merge it into the caller
> >> not only we save a few lines but we also enable checks that we
> >> would have to duplicate otherwise (e.g. QoS availability).
> >>
> >> While at it, reorder some VIR_FREE() in
> >> qemuBuildInterfaceCommandLine so that it is easier to track which
> >> variables are freed and which are not.
> >
> > Sounds like ^this would go into a separate trivial patch.
> >
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_command.c | 113 +++++++++++++++++-----------------------
> >>  1 file changed, 48 insertions(+), 65 deletions(-)
> >>
> > ...
> >
> >> @@ -8595,24 +8577,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
> >>          virSetError(saved_err);
> >>          virFreeError(saved_err);
> >>      }
> >> -    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
> >> -        if (ret < 0)
> >> -            VIR_FORCE_CLOSE(tapfd[i]);
> >> -        if (tapfdName)
> >> -            VIR_FREE(tapfdName[i]);
> >> -    }
> >>      for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
> >>          if (ret < 0)
> >>              VIR_FORCE_CLOSE(vhostfd[i]);
> >>          if (vhostfdName)
> >>              VIR_FREE(vhostfdName[i]);
> >>      }
> >> -    VIR_FREE(tapfd);
> >> +    VIR_FREE(vhostfdName);
> >> +    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
> >> +        if (ret < 0)
> >> +            VIR_FORCE_CLOSE(tapfd[i]);
> >> +        if (tapfdName)
> >> +            VIR_FREE(tapfdName[i]);
> >> +    }
> >> +    VIR_FREE(tapfdName);
> >>      VIR_FREE(vhostfd);
> >> -    VIR_FREE(nic);
> >> +    VIR_FREE(tapfd);
> >> +    VIR_FREE(chardev);
> >>      VIR_FREE(host);
> >> -    VIR_FREE(tapfdName);
> >> -    VIR_FREE(vhostfdName);
> >> +    VIR_FREE(nic);
> >
> > I don't see how ^this hunk made it better. If anything, then the VIR_FREEs
> > should be probably coupled like:
> >
> > VIR_FREE(tapfd);
> > VIR_FREE(tapfdName);
> > VIR_FREE(vhostfd);
> > VIR_FREE(vhostfdName);
> > <the rest of them...>
>
> Why is that? I can see two reasonable orderings. If you have variables
> A, B, C, you either free them in the same order or in reverse C, B, A.
> Any other is just hard to follow.

Well, additionally to what you just said, if you see similarly
named  variables, it induces some kind of relation between such variables, it
may be completely misleading, but not in our case. Now, if you couple these
together in the "free block" it's imho easier for both the writer and the reader
to make sure/verify that no variable is missing from the block, so your example
becomes:

A
AA
B
BB
C

which is imho more readable than:

A
B
AA
C
BB

All in all this is a bike shed we should not spend any more minute with...

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list