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

Yi Wang posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1590767544-11378-1-git-send-email-wang.yi59@zte.com.cn
src/qemu/qemu_hotplug.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH] qemu: hotplug: Fix the condition check for net->downscript
Posted by Yi Wang 3 years, 10 months ago
From: Liao Pingfang <liao.pingfang@zte.com.cn>

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

Signed-off-by: Liao Pingfang <liao.pingfang@zte.com.cn>
---
 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;
-- 
2.9.5

Re: [PATCH] qemu: hotplug: Fix the condition check for net->downscript
Posted by Michal Privoznik 3 years, 10 months ago
On 5/29/20 5:52 PM, Yi Wang wrote:
> From: Liao Pingfang <liao.pingfang@zte.com.cn>
> 
> According to the context, here we are checking net->downscript's validity,
> 
> Signed-off-by: Liao Pingfang <liao.pingfang@zte.com.cn>
> ---
>   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

Re: [PATCH] qemu: hotplug: Fix the condition check for net->downscript
Posted by Michal Privoznik 3 years, 10 months ago
On 6/1/20 3:51 PM, Michal Privoznik wrote:
> On 5/29/20 5:52 PM, Yi Wang wrote:
>> From: Liao Pingfang <liao.pingfang@zte.com.cn>
>>
>> According to the context, here we are checking net->downscript's 
>> validity,
>>
>> Signed-off-by: Liao Pingfang <liao.pingfang@zte.com.cn>
>> ---
>>   src/qemu/qemu_hotplug.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>

> 
> Can you elaborate on this please?
> 

Ah, how could I miss the important change from net->script to 
net->downscript is beyond me :-)

I've fixed the patch to follow the pattern (i.e. not join the two 
conditions into one), and pushed.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Sorry for the noise.

Michal