Re: [PATCH v4 1/9] qapi: Make qapi_bool_parse() gracefully handle NULL value

[Daniel P . Berrangé]

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 :|