qemu: only stop external devices after the domain

[libvirt PATCH] qemu: only stop external devices after the domain

Posted by Ján Tomko 3 years, 11 months ago

A failure in qemuProcessLaunch would lead to qemuExtStopDevices
being called twice - once in the cleanup section and then again
in qemuProcessStop.

However, the first one is called while the QEMU process is
still running, which is too soon for the swtpm process, because
the swtmp_ioctl command can lock up::

   https://bugzilla.redhat.com/show_bug.cgi?id=1822523

Remove the first call and only leave the one in qemuProcessStop,
which is called after the QEMU process is killed.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_process.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dee3f3fb63..f7f6793113 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
     ret = 0;
 
  cleanup:
-    if (ret < 0)
-        qemuExtDevicesStop(driver, vm);
     qemuDomainSecretDestroy(vm);
     return ret;
 }
-- 
2.25.4

Re: [libvirt PATCH] qemu: only stop external devices after the domain

Posted by Daniel Henrique Barboza 3 years, 11 months ago


On 5/12/20 8:23 AM, Ján Tomko wrote:
> A failure in qemuProcessLaunch would lead to qemuExtStopDevices
> being called twice - once in the cleanup section and then again
> in qemuProcessStop.
> 
> However, the first one is called while the QEMU process is
> still running, which is too soon for the swtpm process, because
> the swtmp_ioctl command can lock up::
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=1822523
> 
> Remove the first call and only leave the one in qemuProcessStop,
> which is called after the QEMU process is killed.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
>   src/qemu/qemu_process.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dee3f3fb63..f7f6793113 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
>       ret = 0;
>   
>    cleanup:
> -    if (ret < 0)
> -        qemuExtDevicesStop(driver, vm);


This change makes this code obsolete:


+++ b/src/qemu/qemu_process.c
@@ -6866,11 +6866,6 @@ qemuProcessLaunch(virConnectPtr conn,
                                  incoming != NULL) < 0)
          goto cleanup;
  
-    /* Security manager labeled all devices, therefore
-     * if any operation from now on fails, we need to ask the caller to
-     * restore labels.
-     */
-    ret = -2;
  
      if (incoming && incoming->fd != -1) {


You can remove the 'ret' variable altogether because no one in qemuProcessLaunch() is
doing anything with it.



Thanks,


DHB


>       qemuDomainSecretDestroy(vm);
>       return ret;
>   }
>

Re: [libvirt PATCH] qemu: only stop external devices after the domain

Posted by Daniel Henrique Barboza 3 years, 11 months ago


On 5/12/20 10:48 AM, Daniel Henrique Barboza wrote:
> 
> 
> On 5/12/20 8:23 AM, Ján Tomko wrote:
>> A failure in qemuProcessLaunch would lead to qemuExtStopDevices
>> being called twice - once in the cleanup section and then again
>> in qemuProcessStop.
>>
>> However, the first one is called while the QEMU process is
>> still running, which is too soon for the swtpm process, because
>> the swtmp_ioctl command can lock up::
>>
>>     https://bugzilla.redhat.com/show_bug.cgi?id=1822523
>>
>> Remove the first call and only leave the one in qemuProcessStop,
>> which is called after the QEMU process is killed.
>>
>> Signed-off-by: Ján Tomko <jtomko@redhat.com>
>> ---
>>   src/qemu/qemu_process.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index dee3f3fb63..f7f6793113 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
>>       ret = 0;
>>    cleanup:
>> -    if (ret < 0)
>> -        qemuExtDevicesStop(driver, vm);
> 
> 
> This change makes this code obsolete:
> 
> 
> +++ b/src/qemu/qemu_process.c
> @@ -6866,11 +6866,6 @@ qemuProcessLaunch(virConnectPtr conn,
>                                   incoming != NULL) < 0)
>           goto cleanup;
> 
> -    /* Security manager labeled all devices, therefore
> -     * if any operation from now on fails, we need to ask the caller to
> -     * restore labels.
> -     */
> -    ret = -2;
> 
>       if (incoming && incoming->fd != -1) {
> 
> 
> You can remove the 'ret' variable altogether because no one in qemuProcessLaunch() is
> doing anything with it.


Nevermind, I misread what the code was doing. We need to differentiate the -1 and -2
return codes on error.



> 
> 
> 
> Thanks,
> 
> 
> DHB
> 
> 
>>       qemuDomainSecretDestroy(vm);
>>       return ret;
>>   }
>>

Re: [libvirt PATCH] qemu: only stop external devices after the domain

Posted by Erik Skultety 3 years, 11 months ago

On Tue, May 12, 2020 at 01:23:13PM +0200, Ján Tomko wrote:
> A failure in qemuProcessLaunch would lead to qemuExtStopDevices

qemuExtDevicesStop

> being called twice - once in the cleanup section and then again
> in qemuProcessStop.
> 
> However, the first one is called while the QEMU process is
> still running, which is too soon for the swtpm process, because
> the swtmp_ioctl command can lock up::
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=1822523
> 
> Remove the first call and only leave the one in qemuProcessStop,
> which is called after the QEMU process is killed.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>

Re: [libvirt PATCH] qemu: only stop external devices after the domain

Posted by Daniel Henrique Barboza 3 years, 11 months ago


On 5/12/20 8:23 AM, Ján Tomko wrote:
> A failure in qemuProcessLaunch would lead to qemuExtStopDevices
> being called twice - once in the cleanup section and then again
> in qemuProcessStop.
> 
> However, the first one is called while the QEMU process is
> still running, which is too soon for the swtpm process, because
> the swtmp_ioctl command can lock up::
> 
>     https://bugzilla.redhat.com/show_bug.cgi?id=1822523
> 
> Remove the first call and only leave the one in qemuProcessStop,
> which is called after the QEMU process is killed.
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---


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


>   src/qemu/qemu_process.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dee3f3fb63..f7f6793113 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6992,8 +6992,6 @@ qemuProcessLaunch(virConnectPtr conn,
>       ret = 0;
>   
>    cleanup:
> -    if (ret < 0)
> -        qemuExtDevicesStop(driver, vm);
>       qemuDomainSecretDestroy(vm);
>       return ret;
>   }
>