[libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage

[Martin Kletzander]

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 <***@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);

[Martin Kletzander]

Post by Martin Kletzander
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.
---
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)
* 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)
* 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.

Post by Martin Kletzander
}
-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
https://www.redhat.com/mailman/listinfo/libvir-list

[Andrea Bolognani]

Post by Martin Kletzander
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?

[Martin Kletzander]

Post by Andrea Bolognani

Post by Martin Kletzander
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.

Post by Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization