Libguestfs - Sep 2022 - [PATCH libnbd v3 4/6] generator: Check that more parameters are not NULL

We previously checked only that String parameters are not NULL,
returning an error + EFAULT if so.

However we did not check Bytes*, SockAddrAndLen, Path or StringList
parameters, also never NULL.  Be consistent about checks.

Thanks: Eric Blake for help and an earlier version of the patch
---
 generator/API.ml            | 7 +++++--
 generator/C.ml              | 7 ++++++-
 tests/errors-connect-null.c | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 6e3213ad26..69849102cf 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -3851,13 +3851,16 @@ let ()  
     (* !may_set_error is incompatible with certain parameters because
      * we have to do a NULL-check on those which may return an error.
+     * Refer to generator/C.ml:generator_lib_api_c.
      *)
     | name, { args; may_set_error = false }
          when List.exists
                 (function
-                 | Closure _ | Enum _ | Flags _ | String _ -> true
+                 | Closure _ | Enum _ | Flags _
+                 | BytesIn _ | BytesOut _ | BytesPersistIn _ | BytesPersistOut
_
+                 | SockAddrAndLen _ | Path _ | String _ | StringList _->
true
                  | _ -> false) args ->
-       failwithf "%s: if args contains Closure/Enum/Flags/String
parameter, may_set_error must be false" name
+       failwithf "%s: if args contains any non-null pointer parameter,
may_set_error must be false" name
 
     (* !may_set_error is incompatible with certain optargs too.
      *)
diff --git a/generator/C.ml b/generator/C.ml
index 6e4538fd34..2f22ad1e59 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -620,7 +620,12 @@ let
          need_out_label := true
       | Flags (n, flags) ->
          print_flags_check n flags None
-      | String n ->
+      | BytesIn (n, _) | BytesOut (n, _)
+      | BytesPersistIn (n, _) | BytesPersistOut (n, _)
+      | SockAddrAndLen (n, _)
+      | Path n
+      | String n
+      | StringList n ->
          let value = match errcode with
            | Some value -> value
            | None -> assert false in
diff --git a/tests/errors-connect-null.c b/tests/errors-connect-null.c
index b975181afb..5313c4c394 100644
--- a/tests/errors-connect-null.c
+++ b/tests/errors-connect-null.c
@@ -78,7 +78,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
   /* FIXME: If we add nonnull attributes, this might change to EFAULT */
-  check (EINVAL, "nbd_connect_command: ");
+  check (EFAULT, "nbd_connect_command: ");
 
   if (nbd_connect_command (nbd, (char **) cmd) != -1) {
     fprintf (stderr, "%s: test failed: "
-- 
2.37.0.rc2

On Wed, Sep 28, 2022 at 06:25:37PM +0100, Richard W.M. Jones
wrote:
> We previously checked only that String parameters are not NULL,
> returning an error + EFAULT if so.
> 
> However we did not check Bytes*, SockAddrAndLen, Path or StringList
> parameters, also never NULL.  Be consistent about checks.
> 
> Thanks: Eric Blake for help and an earlier version of the patch
> ---
>  generator/API.ml            | 7 +++++--
>  generator/C.ml              | 7 ++++++-
>  tests/errors-connect-null.c | 2 +-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> +++ b/tests/errors-connect-null.c
> @@ -78,7 +78,7 @@ main (int argc, char *argv[])
>      exit (EXIT_FAILURE);
>    }
>    /* FIXME: If we add nonnull attributes, this might change to EFAULT */
> -  check (EINVAL, "nbd_connect_command: ");
> +  check (EFAULT, "nbd_connect_command: ");
The FIXME comment is now stale and can be removed.

With that fixed, this one looks good.

tests/opt-list-meta-queries.c has a similar FIXME from commit
05b274ef, but where I ripped out the code that was in the version of
the patch I had posted to the mailing list when I ripped out the
EFAULT handling.  Can be reinstated in a separate patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org