1. Patchew
  2. Libvirt
  3. View series

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

Martin Kletzander posted 1 patch 5 years, 4 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e7e3a951df2690f93dd0909da46543954dc1cac4.1542636616.git.mkletzan@redhat.com
There is a newer version of this series
src/qemu/qemu_conf.c |  4 ++--
src/util/virutil.c   | 12 ++++++------
src/util/virutil.h   |  4 ++--
3 files changed, 10 insertions(+), 10 deletions(-)
[libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage
Posted by Martin Kletzander 5 years, 4 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage
Posted by Martin Kletzander 5 years, 4 months ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage
Posted by Andrea Bolognani 5 years, 4 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] util, qemu: Fix virDoes*Exist usage
Posted by Martin Kletzander 5 years, 4 months ago 
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
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.