Use g_strcmp0(), so that NULL is considered an invalid parameter value.

Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 qapi/qapi-util.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 65a7d184372..c6ae829f904 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -86,17 +86,17 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
 
 bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error **errp)
 {
-    if (g_str_equal(value, "on") ||
-        g_str_equal(value, "yes") ||
-        g_str_equal(value, "true") ||
-        g_str_equal(value, "y")) {
+    if (!g_strcmp0(value, "on") ||
+        !g_strcmp0(value, "yes") ||
+        !g_strcmp0(value, "true") ||
+        !g_strcmp0(value, "y")) {
         *obj = true;
         return true;
     }
-    if (g_str_equal(value, "off") ||
-        g_str_equal(value, "no") ||
-        g_str_equal(value, "false") ||
-        g_str_equal(value, "n")) {
+    if (!g_strcmp0(value, "off") ||
+        !g_strcmp0(value, "no") ||
+        !g_strcmp0(value, "false") ||
+        !g_strcmp0(value, "n")) {
         *obj = false;
         return true;
     }
-- 
2.47.1

On Wed, Jan 08, 2025 at 09:04:56PM +0100, Ilya Leoshkevich wrote:
> Use g_strcmp0(), so that NULL is considered an invalid parameter value.

Why are we calling qapi_bool_parse with a NULL value in the first
place ? IMHO this is a sign of a bug higher up the call chain
that ought to be fixed, as in general all our input parsing code
would expect non-NULL input values.

> 
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  qapi/qapi-util.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 65a7d184372..c6ae829f904 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -86,17 +86,17 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>  
>  bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error **errp)
>  {
> -    if (g_str_equal(value, "on") ||
> -        g_str_equal(value, "yes") ||
> -        g_str_equal(value, "true") ||
> -        g_str_equal(value, "y")) {
> +    if (!g_strcmp0(value, "on") ||
> +        !g_strcmp0(value, "yes") ||
> +        !g_strcmp0(value, "true") ||
> +        !g_strcmp0(value, "y")) {
>          *obj = true;
>          return true;
>      }
> -    if (g_str_equal(value, "off") ||
> -        g_str_equal(value, "no") ||
> -        g_str_equal(value, "false") ||
> -        g_str_equal(value, "n")) {
> +    if (!g_strcmp0(value, "off") ||
> +        !g_strcmp0(value, "no") ||
> +        !g_strcmp0(value, "false") ||
> +        !g_strcmp0(value, "n")) {
>          *obj = false;
>          return true;
>      }
> -- 
> 2.47.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, 2025-01-10 at 11:33 +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 08, 2025 at 09:04:56PM +0100, Ilya Leoshkevich wrote:
> > Use g_strcmp0(), so that NULL is considered an invalid parameter
> > value.
> 
> Why are we calling qapi_bool_parse with a NULL value in the first
> place ? IMHO this is a sign of a bug higher up the call chain
> that ought to be fixed, as in general all our input parsing code
> would expect non-NULL input values.

The intended use case is the following (patch 7/9):

    g_auto(GStrv) tokens = g_strsplit(*arg, "=", 2);
    Error *err = NULL;

    if (g_strcmp0(tokens[0], "suspend") == 0) {
        if (!qapi_bool_parse(tokens[0], tokens[1], &suspend, &err)) {
            warn_report_err(err);
            [...]

The idea is to uniformly handle "suspend=y", "suspend=invalid" and
"suspend"; the latter requires checking whether token[1] is NULL.
Of course, this can be special-cased in the caller, but this would be
less elegant.

[...]
> 

On Fri, Jan 10, 2025 at 02:03:09PM +0100, Ilya Leoshkevich wrote:
> On Fri, 2025-01-10 at 11:33 +0000, Daniel P. Berrangé wrote:
> > On Wed, Jan 08, 2025 at 09:04:56PM +0100, Ilya Leoshkevich wrote:
> > > Use g_strcmp0(), so that NULL is considered an invalid parameter
> > > value.
> > 
> > Why are we calling qapi_bool_parse with a NULL value in the first
> > place ? IMHO this is a sign of a bug higher up the call chain
> > that ought to be fixed, as in general all our input parsing code
> > would expect non-NULL input values.
> 
> The intended use case is the following (patch 7/9):
> 
>     g_auto(GStrv) tokens = g_strsplit(*arg, "=", 2);
>     Error *err = NULL;
> 
>     if (g_strcmp0(tokens[0], "suspend") == 0) {
>         if (!qapi_bool_parse(tokens[0], tokens[1], &suspend, &err)) {
>             warn_report_err(err);
>             [...]
> 
> The idea is to uniformly handle "suspend=y", "suspend=invalid" and
> "suspend"; the latter requires checking whether token[1] is NULL.
> Of course, this can be special-cased in the caller, but this would be
> less elegant.

On the contrary, I think handling the latter in the caller explicitly
is the correct approach.

As written this code snippet gives readers the misleading impression
that 'tokens[1]' is expected to be a non-NULL bool value. It is not
at all obvious that the code is intentionally trying to support a
NULL value for tokens[1]. Even if we can see that qapi_bool_parse
accepts NULL, a reader has no idea whether this caller has
intentionally passed NULL or simply forgot to raise an error when
NULL is seen.

IMHO this patch should be dropped, and the desired behaviour made
explicit in patch 7.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|