qemuDomainNamespaceTeardownHostdev: Drop useless check

[libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check

Posted by Michal Privoznik 5 years, 7 months ago

There is no need to check if @npaths is not zero. Let's
qemuDomainNamespaceUnlinkPaths() handle that.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 05e90c3615..d43b6978ad 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12606,8 +12606,7 @@ qemuDomainNamespaceTeardownHostdev(virDomainObjPtr vm,
                                  &npaths, &paths, NULL) < 0)
         goto cleanup;
 
-    if (npaths != 0 &&
-        qemuDomainNamespaceUnlinkPaths(vm, (const char **)paths, npaths) < 0)
+    if (qemuDomainNamespaceUnlinkPaths(vm, (const char **)paths, npaths) < 0)
         goto cleanup;
 
     ret = 0;
-- 
2.16.4

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

Re: [libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check

Posted by John Ferlan 5 years, 7 months ago

On 09/06/2018 12:16 PM, Michal Privoznik wrote:
> There is no need to check if @npaths is not zero. Let's
> qemuDomainNamespaceUnlinkPaths() handle that.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

At the cost of a possible unnecessary, but perhaps expensive call to
qemuDomainGetPreservedMounts when npaths == 0?

I think if you add a "filter" of npaths == 0, then return 0 in
qemuDomainNamespaceUnlinkPaths, then that'd be a good thing...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

I also wonder if the :

    if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
        return 0;

that's "duplicated" in qemuDomainNamespaceTeardownHostdev and
qemuDomainNamespaceUnlinkPaths could be "reworked"...

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

Re: [libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check

Posted by Michal Prívozník 5 years, 7 months ago

On 09/07/2018 12:52 AM, John Ferlan wrote:
> 
> 
> On 09/06/2018 12:16 PM, Michal Privoznik wrote:
>> There is no need to check if @npaths is not zero. Let's
>> qemuDomainNamespaceUnlinkPaths() handle that.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
> 
> At the cost of a possible unnecessary, but perhaps expensive call to
> qemuDomainGetPreservedMounts when npaths == 0?

Sure. But at least with my patch we are consistent. If it really bothers
us, we can have a check at the beginning of
qemuDomainNamespaceMknodPaths() and qemuDomainNamespaceUnlinkPaths(),
right after namespace check to return early if npaths is zero.

> 
> I think if you add a "filter" of npaths == 0, then return 0 in
> qemuDomainNamespaceUnlinkPaths, then that'd be a good thing...
> 

Okay.

> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> I also wonder if the :
> 
>     if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>         return 0;
> 
> that's "duplicated" in qemuDomainNamespaceTeardownHostdev and
> qemuDomainNamespaceUnlinkPaths could be "reworked"...
> 

Oh sure it could. We have two sets of functions apparently: one does the
check themselves and return early (e.g. qemuDomainNamespaceSetupDisk())
and the other leave it to qemuDomainNamespaceMknodPaths() to return
return early (e.g. qemuDomainNamespaceSetupMemory()).

Michal

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

Re: [libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check

Posted by Ján Tomko 5 years, 7 months ago

On Fri, Sep 07, 2018 at 07:21:18AM +0200, Michal Prívozník wrote:
>On 09/07/2018 12:52 AM, John Ferlan wrote:
>>
>>
>> On 09/06/2018 12:16 PM, Michal Privoznik wrote:
>>> There is no need to check if @npaths is not zero. Let's
>>> qemuDomainNamespaceUnlinkPaths() handle that.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>
>> At the cost of a possible unnecessary, but perhaps expensive call to
>> qemuDomainGetPreservedMounts when npaths == 0?
>

Yes, this was exactly my reasoning when I touched this


>Sure. But at least with my patch we are consistent. If it really bothers
>us, we can have a check at the beginning of
>qemuDomainNamespaceMknodPaths() and qemuDomainNamespaceUnlinkPaths(),
>right after namespace check to return early if npaths is zero.
>
>>
>> I think if you add a "filter" of npaths == 0, then return 0 in
>> qemuDomainNamespaceUnlinkPaths, then that'd be a good thing...
>>
>
>Okay.
>
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> John
>>
>> I also wonder if the :
>>
>>     if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>>         return 0;
>>
>> that's "duplicated" in qemuDomainNamespaceTeardownHostdev and
>> qemuDomainNamespaceUnlinkPaths could be "reworked"...
>>
>
>Oh sure it could. We have two sets of functions apparently: one does the
>check themselves and return early (e.g. qemuDomainNamespaceSetupDisk())
>and the other leave it to qemuDomainNamespaceMknodPaths() to return
>return early (e.g. qemuDomainNamespaceSetupMemory()).

and I left the early check in some places for the same reason.
(For other places, it might've been just an oversight)

Jano

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

Re: [libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check

Posted by Michal Privoznik 5 years, 7 months ago

On 09/07/2018 10:35 AM, Ján Tomko wrote:
> On Fri, Sep 07, 2018 at 07:21:18AM +0200, Michal Prívozník wrote:
>> On 09/07/2018 12:52 AM, John Ferlan wrote:
>>>
>>>
>>> On 09/06/2018 12:16 PM, Michal Privoznik wrote:
>>>> There is no need to check if @npaths is not zero. Let's
>>>> qemuDomainNamespaceUnlinkPaths() handle that.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/qemu/qemu_domain.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>
>>> At the cost of a possible unnecessary, but perhaps expensive call to
>>> qemuDomainGetPreservedMounts when npaths == 0?
>>
> 
> Yes, this was exactly my reasoning when I touched this
> 
> 
>> Sure. But at least with my patch we are consistent. If it really bothers
>> us, we can have a check at the beginning of
>> qemuDomainNamespaceMknodPaths() and qemuDomainNamespaceUnlinkPaths(),
>> right after namespace check to return early if npaths is zero.
>>
>>>
>>> I think if you add a "filter" of npaths == 0, then return 0 in
>>> qemuDomainNamespaceUnlinkPaths, then that'd be a good thing...
>>>
>>
>> Okay.
>>
>>>
>>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>>
>>> John
>>>
>>> I also wonder if the :
>>>
>>>     if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
>>>         return 0;
>>>
>>> that's "duplicated" in qemuDomainNamespaceTeardownHostdev and
>>> qemuDomainNamespaceUnlinkPaths could be "reworked"...
>>>
>>
>> Oh sure it could. We have two sets of functions apparently: one does the
>> check themselves and return early (e.g. qemuDomainNamespaceSetupDisk())
>> and the other leave it to qemuDomainNamespaceMknodPaths() to return
>> return early (e.g. qemuDomainNamespaceSetupMemory()).
> 
> and I left the early check in some places for the same reason.
> (For other places, it might've been just an oversight)

Well, it creates inconsistency. Because some functions check for
namespaces as the first thing, and some relies on
qemuDomainNamespaceMknodPaths() or qemuDomainNamespaceUnlinkPaths() to
do so.

I don't have preference where this should be checked (even though I lean
slightly towards checking in fewer places than checking at lot places -
it's clear that putting such check is easy to forget). I'll post a patch
that does just that.

Penalty for calling a function is not something we care about. Libvirt's
full of short functions calling even shorter ones. Also, these functions
I'm modifying are not called that frequently at all (domain startup,
hotplug/unplug). At this point, the fork() to enter domain namespace is
probably the biggest consumer of time anyway.
And I think it's fair to assume that more users do use namespaces (as
they are turned on by default and users have to opt out) so the check is
more frequently false than true.

Michal

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