Back when the original version of this chunk of code was added (commit
41b087198 in libvirt-0.8.1 in April 2010), we used virExecDaemonize()
to start the qemu process, and would continue on in the function
(which at that time was called qemudStartVMDaemon()) even if a -1 was
returned. So it was possible to get to this code with rv == -1 (it was
called "ret" in that version of the code).

In modern libvirt code, qemu is started with virCommandRun(); then we
call virPidFileReadPath(); those are the only two ways of setting "rv"
prior to this code being removed, and in either case if the new value
of rv < 0, then we immediately skip over the rest of the code to the
cleanup: label.

This means that the code being removed by this patch is
unreachable.

(NB: anyway, attempting to fend off accidental removal of some other
guest's nwfilter rules by carefully ordering all nwfilter teardowns to
happen "before tap device deletion" is a fruitless pursuit, because a
tap device could be deleted by the qemu process being terminated
external to libvirt, i.e. we must instead avoid re-using tap device
names. That is coming.)

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/qemu/qemu_process.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 126fabf5ef..832b2e6870 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6907,12 +6907,6 @@ qemuProcessLaunch(virConnectPtr conn,
         goto cleanup;
     VIR_DEBUG("Handshake complete, child running");
 
-    if (rv == -1) {
-        /* The VM failed to start; tear filters before taps */
-        virDomainConfVMNWFilterTeardown(vm);
-        goto cleanup;
-    }
-
     if (qemuDomainObjStartWorker(vm) < 0)
         goto cleanup;
 
-- 
2.26.2

On 8/23/20 1:51 AM, Laine Stump wrote:
> Back when the original version of this chunk of code was added (commit
> 41b087198 in libvirt-0.8.1 in April 2010), we used virExecDaemonize()
> to start the qemu process, and would continue on in the function
> (which at that time was called qemudStartVMDaemon()) even if a -1 was
> returned. So it was possible to get to this code with rv == -1 (it was
> called "ret" in that version of the code).
> 
> In modern libvirt code, qemu is started with virCommandRun(); then we
> call virPidFileReadPath(); those are the only two ways of setting "rv"
> prior to this code being removed, and in either case if the new value
> of rv < 0, then we immediately skip over the rest of the code to the
> cleanup: label.
> 
> This means that the code being removed by this patch is
> unreachable.
> 
> (NB: anyway, attempting to fend off accidental removal of some other
> guest's nwfilter rules by carefully ordering all nwfilter teardowns to
> happen "before tap device deletion" is a fruitless pursuit, because a
> tap device could be deleted by the qemu process being terminated
> external to libvirt, i.e. we must instead avoid re-using tap device
> names. That is coming.)


Is it better for this patch to be pushed after "conf: properly clear out
autogenerated macvtap ..." then?

> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>



>   src/qemu/qemu_process.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 126fabf5ef..832b2e6870 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6907,12 +6907,6 @@ qemuProcessLaunch(virConnectPtr conn,
>           goto cleanup;
>       VIR_DEBUG("Handshake complete, child running");
>   
> -    if (rv == -1) {
> -        /* The VM failed to start; tear filters before taps */
> -        virDomainConfVMNWFilterTeardown(vm);
> -        goto cleanup;
> -    }
> -
>       if (qemuDomainObjStartWorker(vm) < 0)
>           goto cleanup;
>   
> 

On 8/24/20 6:30 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 8/23/20 1:51 AM, Laine Stump wrote:
>> Back when the original version of this chunk of code was added (commit
>> 41b087198 in libvirt-0.8.1 in April 2010), we used virExecDaemonize()
>> to start the qemu process, and would continue on in the function
>> (which at that time was called qemudStartVMDaemon()) even if a -1 was
>> returned. So it was possible to get to this code with rv == -1 (it was
>> called "ret" in that version of the code).
>>
>> In modern libvirt code, qemu is started with virCommandRun(); then we
>> call virPidFileReadPath(); those are the only two ways of setting "rv"
>> prior to this code being removed, and in either case if the new value
>> of rv < 0, then we immediately skip over the rest of the code to the
>> cleanup: label.
>>
>> This means that the code being removed by this patch is
>> unreachable.
>>
>> (NB: anyway, attempting to fend off accidental removal of some other
>> guest's nwfilter rules by carefully ordering all nwfilter teardowns to
>> happen "before tap device deletion" is a fruitless pursuit, because a
>> tap device could be deleted by the qemu process being terminated
>> external to libvirt, i.e. we must instead avoid re-using tap device
>> names. That is coming.)
> 
> 
> Is it better for this patch to be pushed after "conf: properly clear out
> autogenerated macvtap ..." then?

Nah, it's unreachable code in any case, so its removal is independent of 
anything else.

> 
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Thanks!