[libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage
Since the functions only return 0 or 1, they should return bool (missed the change in the first commit). That way it's clearer that the check for non-existing group should be either "== 0" instead. Fix this by using proper negation instead. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- src/util/virutil.c | 12 ++++++------ src/util/virutil.h | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 32da9a735184..a946b05d5d47 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -193,10 +193,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (virAsprintf(&cfg->swtpmStorageDir, "%s/lib/libvirt/swtpm", LOCALSTATEDIR) < 0) goto error; - if (virDoesUserExist("tss") != 0 || + if (!virDoesUserExist("tss") || virGetUserID("tss", &cfg->swtpm_user) < 0) cfg->swtpm_user = 0; /* fall back to root */ - if (virDoesGroupExist("tss") != 0 || + if (!virDoesGroupExist("tss") || virGetGroupID("tss", &cfg->swtpm_group) < 0) cfg->swtpm_group = 0; /* fall back to root */ } else { diff --git a/src/util/virutil.c b/src/util/virutil.c index c0783ecb285b..77baaa33f472 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1133,7 +1133,7 @@ virGetGroupID(const char *group, gid_t *gid) /* Silently checks if User @name exists. * Returns if the user exists and fallbacks to false on error. */ -int +bool virDoesUserExist(const char *name) { return virGetUserIDByName(name, NULL, true) == 0; @@ -1142,7 +1142,7 @@ virDoesUserExist(const char *name) /* Silently checks if Group @name exists. * Returns if the group exists and fallbacks to false on error. */ -int +bool virDoesGroupExist(const char *name) { return virGetGroupIDByName(name, NULL, true) == 0; @@ -1243,16 +1243,16 @@ virGetGroupList(uid_t uid ATTRIBUTE_UNUSED, gid_t gid ATTRIBUTE_UNUSED, return 0; } -int +bool virDoesUserExist(const char *name ATTRIBUTE_UNUSED) { - return 0; + return false } -int +bool virDoesGroupExist(const char *name ATTRIBUTE_UNUSED) { - return 0; + return false; } # ifdef WIN32 diff --git a/src/util/virutil.h b/src/util/virutil.h index 2407f54efd47..e0ab0da0f2fc 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -152,8 +152,8 @@ int virGetUserID(const char *name, int virGetGroupID(const char *name, gid_t *gid) ATTRIBUTE_RETURN_CHECK; -int virDoesUserExist(const char *name); -int virDoesGroupExist(const char *name); +bool virDoesUserExist(const char *name); +bool virDoesGroupExist(const char *name); bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); -- 2.19.1
On Mon, Nov 19, 2018 at 03:10:16PM +0100, Martin Kletzander wrote:
Since the functions only return 0 or 1, they should return bool (missed the change in the first commit). That way it's clearer that the check for non-existing group should be either "== 0" instead. Fix this by using proper negation instead.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- src/util/virutil.c | 12 ++++++------ src/util/virutil.h | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 32da9a735184..a946b05d5d47 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -193,10 +193,10 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (virAsprintf(&cfg->swtpmStorageDir, "%s/lib/libvirt/swtpm", LOCALSTATEDIR) < 0) goto error; - if (virDoesUserExist("tss") != 0 || + if (!virDoesUserExist("tss") || virGetUserID("tss", &cfg->swtpm_user) < 0) cfg->swtpm_user = 0; /* fall back to root */ - if (virDoesGroupExist("tss") != 0 || + if (!virDoesGroupExist("tss") || virGetGroupID("tss", &cfg->swtpm_group) < 0) cfg->swtpm_group = 0; /* fall back to root */ } else { diff --git a/src/util/virutil.c b/src/util/virutil.c index c0783ecb285b..77baaa33f472 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1133,7 +1133,7 @@ virGetGroupID(const char *group, gid_t *gid) /* Silently checks if User @name exists. * Returns if the user exists and fallbacks to false on error. */ -int +bool virDoesUserExist(const char *name) { return virGetUserIDByName(name, NULL, true) == 0; @@ -1142,7 +1142,7 @@ virDoesUserExist(const char *name) /* Silently checks if Group @name exists. * Returns if the group exists and fallbacks to false on error. */ -int +bool virDoesGroupExist(const char *name) { return virGetGroupIDByName(name, NULL, true) == 0; @@ -1243,16 +1243,16 @@ virGetGroupList(uid_t uid ATTRIBUTE_UNUSED, gid_t gid ATTRIBUTE_UNUSED, return 0; }
-int +bool virDoesUserExist(const char *name ATTRIBUTE_UNUSED) { - return 0; + return false
Consider semicolon added here, already fixed locally.
}
-int +bool virDoesGroupExist(const char *name ATTRIBUTE_UNUSED) { - return 0; + return false; }
# ifdef WIN32 diff --git a/src/util/virutil.h b/src/util/virutil.h index 2407f54efd47..e0ab0da0f2fc 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -152,8 +152,8 @@ int virGetUserID(const char *name, int virGetGroupID(const char *name, gid_t *gid) ATTRIBUTE_RETURN_CHECK;
-int virDoesUserExist(const char *name); -int virDoesGroupExist(const char *name); +bool virDoesUserExist(const char *name); +bool virDoesGroupExist(const char *name);
bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); -- 2.19.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2018-11-19 at 15:10 +0100, Martin Kletzander wrote:
Since the functions only return 0 or 1, they should return bool (missed the change in the first commit). That way it's clearer that the check for non-existing group should be either "== 0" instead.
I don't get this part of the commit message... With the original implementation, calling virDoesGroupExist() on a non-existing group would have returned 0, correct? And now it will return false instead... Oh, I think I see what you mean: the original code expected the integer return value to follow the usual libvirt conventions where 0 means success and <0 means failure, but in this case the functions returned 1 on success and 0 on failure, so the logic in virQEMUDriverConfigPtr() was wrong. Gotcha. So, here's what I would do: I would split this into two patches, the first one which contains only the first hunk - it will work even if the return types are int - and fixes the bug, and the second one which contains the remaining hunks and makes usage of the functions more obvious and hopefully prevents more bugs from slipping in. Does that sound reasonable? -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Nov 19, 2018 at 05:27:01PM +0100, Andrea Bolognani wrote:
On Mon, 2018-11-19 at 15:10 +0100, Martin Kletzander wrote:
Since the functions only return 0 or 1, they should return bool (missed the change in the first commit). That way it's clearer that the check for non-existing group should be either "== 0" instead.
I don't get this part of the commit message... With the original implementation, calling virDoesGroupExist() on a non-existing group would have returned 0, correct? And now it will return false instead...
Oh, I think I see what you mean: the original code expected the integer return value to follow the usual libvirt conventions where 0 means success and <0 means failure, but in this case the functions returned 1 on success and 0 on failure, so the logic in virQEMUDriverConfigPtr() was wrong. Gotcha.
So, here's what I would do: I would split this into two patches, the first one which contains only the first hunk - it will work even if the return types are int - and fixes the bug, and the second one which contains the remaining hunks and makes usage of the functions more obvious and hopefully prevents more bugs from slipping in.
Does that sound reasonable?
Brutal honesty? No. For a patch that I would consider trivial I think it's a waste of time, but nevertheless I'll do it.
-- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Martin Kletzander