Re: [PATCH] qemu: hotplug: Fix the condition check for net->downscript

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/29/20 5:52 PM, Yi Wang wrote:
From: Liao Pingfang <liao.pingfang@xxxxxxxxxx>

According to the context, here we are checking net->downscript's validity,

Signed-off-by: Liao Pingfang <liao.pingfang@xxxxxxxxxx>
---
  src/qemu/qemu_hotplug.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d1a2be1..8c99dfc 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4672,9 +4672,8 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
              virDomainNetReleaseActualDevice(conn, vm->def, net);
          else
              VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname));
-    } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET) {
-        if (net->script)
-           virNetDevRunEthernetScript(net->ifname, net->downscript);
+    } else if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET && net->downscript) {
+        virNetDevRunEthernetScript(net->ifname, net->downscript);
      }
      virDomainNetDefFree(net);
      return 0;


I'm not quite sure why this change is needed and the commit message doesn't explain it well. My idea, when merging the original patch, was to have the following pattern:

  if (net->type == VIR_DOMAIN_NET_TYPE_NET) {
  } else if (net->type == VIR_DOMAAIN_NET_TYPE_ETHERNET) {
  } else if (net->type == ...) {
  }

because it's easily extensible (e.g. if another action needs to be taken for say ethernet type interface then all that's needed is to call a function from corresponding if(). If your patch is merged then it needs to be (effectively) reverted.

Can you elaborate on this please?

Michal




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux