1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: fix pr-helper0 remain

Jie Wang posted 1 patch 4 years, 11 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1559123075-47568-1-git-send-email-wangjie88@huawei.com
There is a newer version of this series
src/qemu/qemu_hotplug.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by Jie Wang 4 years, 11 months ago 
if libvirt receive DISCONNECTED event and set prDaemonRunning to false,
and qemuDomainRemoveDiskDevice is performing in the meantime.
qemuDomainRemoveDiskDevice will return directly by prDaemonRunning
check, so the pr-helper0 object will remain. I think it is no need to
check prDaemonRunning in qemuHotplugRemoveManagedPR.

Signed-off-by: Jie Wang <wangjie88@huawei.com>
---
 src/qemu/qemu_hotplug.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 34249bd030..5e4a929738 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -465,8 +465,7 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver,
     virErrorPtr orig_err;
     int ret = -1;
 
-    if (!priv->prDaemonRunning ||
-        virDomainDefHasManagedPR(vm->def))
+    if (virDomainDefHasManagedPR(vm->def))
         return 0;
 
     virErrorPreserveLast(&orig_err);
-- 
2.16.2.windows.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by Michal Privoznik 4 years, 11 months ago 
On 5/29/19 11:44 AM, Jie Wang wrote:
> if libvirt receive DISCONNECTED event and set prDaemonRunning to false,
> and qemuDomainRemoveDiskDevice is performing in the meantime.
> qemuDomainRemoveDiskDevice will return directly by prDaemonRunning
> check, so the pr-helper0 object will remain. I think it is no need to
> check prDaemonRunning in qemuHotplugRemoveManagedPR.
> 
> Signed-off-by: Jie Wang <wangjie88@huawei.com>
> ---
>   src/qemu/qemu_hotplug.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 34249bd030..5e4a929738 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -465,8 +465,7 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver,
>       virErrorPtr orig_err;
>       int ret = -1;
>   
> -    if (!priv->prDaemonRunning ||
> -        virDomainDefHasManagedPR(vm->def))
> +    if (virDomainDefHasManagedPR(vm->def))
>           return 0;
>   
>       virErrorPreserveLast(&orig_err);
> 

Right, because we unlock the domain object while talking on monitor. 
Well, in that case I guess we should try harder and also kill the 
pr-helper process. IOW qemuProcessKillManagedPRDaemon() could use the 
same treatment.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by wangjie (P) 4 years, 11 months ago 
Hi, Michal:

     Do you mean we should also remove "if (!priv->prDaemonRunning)" in 
qemuProcessKillManagedPRDaemon ? I don't understand why do that, can you 
tell me more details?

On 2019/5/30 18:22, Michal Privoznik wrote:
> On 5/29/19 11:44 AM, Jie Wang wrote:
>> if libvirt receive DISCONNECTED event and set prDaemonRunning to false,
>> and qemuDomainRemoveDiskDevice is performing in the meantime.
>> qemuDomainRemoveDiskDevice will return directly by prDaemonRunning
>> check, so the pr-helper0 object will remain. I think it is no need to
>> check prDaemonRunning in qemuHotplugRemoveManagedPR.
>>
>> Signed-off-by: Jie Wang <wangjie88@huawei.com>
>> ---
>>   src/qemu/qemu_hotplug.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 34249bd030..5e4a929738 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -465,8 +465,7 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver,
>>       virErrorPtr orig_err;
>>       int ret = -1;
>>   -    if (!priv->prDaemonRunning ||
>> -        virDomainDefHasManagedPR(vm->def))
>> +    if (virDomainDefHasManagedPR(vm->def))
>>           return 0;
>>         virErrorPreserveLast(&orig_err);
>>
>
> Right, because we unlock the domain object while talking on monitor. 
> Well, in that case I guess we should try harder and also kill the 
> pr-helper process. IOW qemuProcessKillManagedPRDaemon() could use the 
> same treatment.
>
> Michal
>
> .
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by Michal Privoznik 4 years, 11 months ago 
On 5/31/19 9:19 AM, wangjie (P) wrote:
> Hi, Michal:
> 
>      Do you mean we should also remove "if (!priv->prDaemonRunning)" in 
> qemuProcessKillManagedPRDaemon ? I don't understand why do that, can you 
> tell me more details?
> 

Yes. I mean exactly that.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by wangjie (P) 4 years, 11 months ago 
Hi, Michal:

     Sorry, I don't think out which case make us to do that, what are 
the risks? can you give me an example?

On 2019/5/31 15:21, Michal Privoznik wrote:
> On 5/31/19 9:19 AM, wangjie (P) wrote:
>> Hi, Michal:
>>
>>      Do you mean we should also remove "if (!priv->prDaemonRunning)" 
>> in qemuProcessKillManagedPRDaemon ? I don't understand why do that, 
>> can you tell me more details?
>>
>
> Yes. I mean exactly that.
>
> Michal
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by Michal Privoznik 4 years, 11 months ago 
On 5/31/19 10:06 AM, wangjie (P) wrote:
> Hi, Michal:
> 
>      Sorry, I don't think out which case make us to do that, what are 
> the risks? can you give me an example?

Well, qemuProcessKillManagedPRDaemon() not only kills pr-helper process 
but it also cleans up after it. If we receive DISCONNECTED event then 
qemu is probably unable to talk to pr-helper process even if it is 
running. It makes sense then to kill it and start a new one.
If it is not running then we might clean up after it.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix pr-helper0 remain
Posted by wangjie (P) 4 years, 10 months ago 
Thanks，Michal

     I understand what you mean， and I have sent the second patch “PATCH 
v2”，please review it.   :)

Jie

On 2019/5/31 16:55, Michal Privoznik wrote:
> On 5/31/19 10:06 AM, wangjie (P) wrote:
>> Hi, Michal:
>>
>>      Sorry, I don't think out which case make us to do that, what are 
>> the risks? can you give me an example?
>
> Well, qemuProcessKillManagedPRDaemon() not only kills pr-helper 
> process but it also cleans up after it. If we receive DISCONNECTED 
> event then qemu is probably unable to talk to pr-helper process even 
> if it is running. It makes sense then to kill it and start a new one.
> If it is not running then we might clean up after it.
>
> Michal
>

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.