[libvirt] [PATCH] qemuDomainNamespaceTeardownHostdev: Drop useless check
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
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"...
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
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
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
participants (4)
-
John Ferlan -
Ján Tomko -
Michal Privoznik -
Michal Prívozník