[libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check

[Michal Privoznik]

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 at 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 at 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