[PATCH libnbd v2 0/4] Additional non-NULL warnings, checks and docs
v1 was: https://listman.redhat.com/archives/libguestfs/2022-September/029982.html This series is actually quite a bit different from v1 because it is rebased on top of Eric's series and takes that into account. Generally this heads down the return error instead of asserting route, hoping that the GCC annotations will be sufficient to catch as many errors as possible at compile time. Rich.
For API parameters that are pointers and must not be NULL, add the appropriate GCC annotations. These are only enabled in very recent GCC (>= 12) because we have concerns with earlier versions, see for example: https://bugzilla.redhat.com/show_bug.cgi?id=1041336 --- generator/C.ml | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/generator/C.ml b/generator/C.ml index 66498a7d88..cafc5590e2 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -107,6 +107,26 @@ let | UInt64 n -> [n] | UIntPtr n -> [n] +let arg_attr_nonnull = + function + (* BytesIn/Out are passed using a non-null pointer, and size_t *) + | BytesIn _ + | BytesOut _ + | BytesPersistIn _ + | BytesPersistOut _ -> [ true; false ] + (* sockaddr is also non-null pointer, and length *) + | SockAddrAndLen (n, len) -> [ true; false ] + (* strings should be marked as non-null *) + | Path _ | String _ -> [ true ] + (* list of strings should be marked as non-null *) + | StringList n -> [ true ] + (* other non-pointer types can never be null *) + | Bool _ | Closure _ | Enum _ | Fd _ | Flags _ + | Int _ | Int64 _ | SizeT _ + | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ] + +let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ] + let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true) ?closure_style args optargs = if parens then pr "("; @@ -216,7 +236,17 @@ let let print_fndecl ?wrap ?closure_style name args optargs ret = pr "extern "; print_call ?wrap ?closure_style name args optargs ret; - pr ";\n" + + (* Non-null attribute. *) + let nns = + [ [ true ] ] (* for struct nbd_handle pointer *) + @ List.map arg_attr_nonnull args + @ List.map optarg_attr_nonnull optargs in + let nns : bool list = List.flatten nns in + let nns = List.mapi (fun i b -> (i+1, b)) nns in + let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in + let nns : string list = List.map string_of_int nns in + pr "\n LIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns) let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true) cbargs = @@ -349,6 +379,19 @@ let pr "extern \"C\" {\n"; pr "#endif\n"; pr "\n"; + pr "#if defined(__GNUC__)\n"; + pr "#define LIBNBD_GCC_VERSION \\\n"; + pr " (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)\n"; + pr "#endif\n"; + pr "\n"; + pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n"; + pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /* gcc >= 12.0 */\n"; + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ s))\n"; + pr "#else\n"; + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n"; + pr "#endif\n"; + pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n"; + pr "\n"; pr "struct nbd_handle;\n"; pr "\n"; List.iter ( @@ -382,7 +425,7 @@ let pr "extern struct nbd_handle *nbd_create (void);\n"; pr "#define LIBNBD_HAVE_NBD_CREATE 1\n"; pr "\n"; - pr "extern void nbd_close (struct nbd_handle *h);\n"; + pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL */\n"; pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n"; pr "\n"; pr "extern const char *nbd_get_error (void);\n"; @@ -770,6 +813,12 @@ let pr "\n"; pr "#include <pthread.h>\n"; pr "\n"; + pr "/* GCC will remove NULL checks from this file for any parameter\n"; + pr " * annotated with attribute((nonnull)). See RHBZ#1041336. To\n"; + pr " * avoid this, disable the attribute when including libnbd.h.\n"; + pr " */\n"; + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n"; + pr "\n"; pr "#include \"libnbd.h\"\n"; pr "#include \"internal.h\"\n"; pr "\n"; -- 2.37.0.rc2
On 09/28/22 11:30, Richard W.M. Jones wrote:
For API parameters that are pointers and must not be NULL, add the appropriate GCC annotations. These are only enabled in very recent GCC (>= 12) because we have concerns with earlier versions, see for example: https://bugzilla.redhat.com/show_bug.cgi?id=1041336 --- generator/C.ml | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-)
This patch looks good to me, but a whole lot more documentation would have been necessary -- the idea should be summarized in the commit message, or in code comments:
diff --git a/generator/C.ml b/generator/C.ml index 66498a7d88..cafc5590e2 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -107,6 +107,26 @@ let | UInt64 n -> [n] | UIntPtr n -> [n]
+let arg_attr_nonnull = + function + (* BytesIn/Out are passed using a non-null pointer, and size_t *) + | BytesIn _ + | BytesOut _ + | BytesPersistIn _ + | BytesPersistOut _ -> [ true; false ] + (* sockaddr is also non-null pointer, and length *) + | SockAddrAndLen (n, len) -> [ true; false ] + (* strings should be marked as non-null *) + | Path _ | String _ -> [ true ] + (* list of strings should be marked as non-null *) + | StringList n -> [ true ] + (* other non-pointer types can never be null *)
side comment: I propose "are unable to be null". "Cannot" is ambiguous between "must not" and "incapable of" (or "unable to"). Usually the context resolves this, but here it doesn't.
+ | Bool _ | Closure _ | Enum _ | Fd _ | Flags _ + | Int _ | Int64 _ | SizeT _ + | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ] +
IMO it is not trivial at all why we're outputting lists here. Once I read through the entire patch and then re-read it from the start, I understood the idea (i.e., at second reading). That's precisely what the commit message should prepare the reviewer for. The idea is that a particular API parameter may map to an indivisible sequence of C-language parameters, and only a subset of these C-level parameters need to (or can) be marked as nonnull. Thus, the above mapping deals with these indivisible sequences, and then:
+let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ] + let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true) ?closure_style args optargs = if parens then pr "("; @@ -216,7 +236,17 @@ let let print_fndecl ?wrap ?closure_style name args optargs ret = pr "extern "; print_call ?wrap ?closure_style name args optargs ret; - pr ";\n" + + (* Non-null attribute. *) + let nns = + [ [ true ] ] (* for struct nbd_handle pointer *) + @ List.map arg_attr_nonnull args + @ List.map optarg_attr_nonnull optargs in + let nns : bool list = List.flatten nns in + let nns = List.mapi (fun i b -> (i+1, b)) nns in + let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in + let nns : string list = List.map string_of_int nns in + pr "\n LIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns)
here we collect the marked parameters from a *list* of such indivisible sequences, globally counting upwards from 1, so that we can ultimately dump the list of the flat sequence numbers of the affected C-level parameters into a single __attribute__((__nonnull__ ....)) construct. I really shouldn't have to re-compose the goal from the bottom up, when reviewing a patch :/ OCaml is hugely expressive, a lot can be accomplished in a few lines of code.
let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true) cbargs = @@ -349,6 +379,19 @@ let pr "extern \"C\" {\n"; pr "#endif\n"; pr "\n"; + pr "#if defined(__GNUC__)\n"; + pr "#define LIBNBD_GCC_VERSION \\\n"; + pr " (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)\n"; + pr "#endif\n"; + pr "\n"; + pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n"; + pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /* gcc >= 12.0 */\n"; + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ s))\n"; + pr "#else\n"; + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n"; + pr "#endif\n"; + pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n"; + pr "\n"; pr "struct nbd_handle;\n"; pr "\n"; List.iter ( @@ -382,7 +425,7 @@ let pr "extern struct nbd_handle *nbd_create (void);\n"; pr "#define LIBNBD_HAVE_NBD_CREATE 1\n"; pr "\n"; - pr "extern void nbd_close (struct nbd_handle *h);\n"; + pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL */\n"; pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n"; pr "\n"; pr "extern const char *nbd_get_error (void);\n"; @@ -770,6 +813,12 @@ let pr "\n"; pr "#include <pthread.h>\n"; pr "\n"; + pr "/* GCC will remove NULL checks from this file for any parameter\n"; + pr " * annotated with attribute((nonnull)). See RHBZ#1041336. To\n"; + pr " * avoid this, disable the attribute when including libnbd.h.\n"; + pr " */\n"; + pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n"; + pr "\n"; pr "#include \"libnbd.h\"\n"; pr "#include \"internal.h\"\n"; pr "\n";
The code is absolutely great; please be more considerate & gentle with your reviewers. With some comments added: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Laszlo
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 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 6e3213ad26..0975a407c1 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 _ -> 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 cafc5590e2..bfc216609e 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -614,7 +614,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 -- 2.37.0.rc2
On 09/28/22 11:30, 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 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml index 6e3213ad26..0975a407c1 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 _ -> 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 cafc5590e2..bfc216609e 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -614,7 +614,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
The first patch marks (some parts of the C-language projection of) parameter types BytesIn, BytesOut, BytesPersistIn, BytesPersistOut, SockAddrAndLen, Path, String, and StringList, as non-null. The second hunk in this patch covers all of those, but the first hunk does not cover StringList. Is that intentional? Laszlo
On Wed, Sep 28, 2022 at 12:47:24PM +0200, Laszlo Ersek wrote:
On 09/28/22 11:30, 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 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml index 6e3213ad26..0975a407c1 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 _ -> 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 cafc5590e2..bfc216609e 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -614,7 +614,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
The first patch marks (some parts of the C-language projection of) parameter types BytesIn, BytesOut, BytesPersistIn, BytesPersistOut, SockAddrAndLen, Path, String, and StringList, as non-null.
The second hunk in this patch covers all of those, but the first hunk does not cover StringList. Is that intentional?
No that's a mistake. I was actually trying to think of a way where I don't need to list these out in two places, but I couldn't think of how to do it except to use polymorphic variants (`Variants). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 09/28/22 19:18, Richard W.M. Jones wrote:
On Wed, Sep 28, 2022 at 12:47:24PM +0200, Laszlo Ersek wrote:
On 09/28/22 11:30, 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 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/generator/API.ml b/generator/API.ml index 6e3213ad26..0975a407c1 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 _ -> 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 cafc5590e2..bfc216609e 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -614,7 +614,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
The first patch marks (some parts of the C-language projection of) parameter types BytesIn, BytesOut, BytesPersistIn, BytesPersistOut, SockAddrAndLen, Path, String, and StringList, as non-null.
The second hunk in this patch covers all of those, but the first hunk does not cover StringList. Is that intentional?
No that's a mistake.
I was actually trying to think of a way where I don't need to list these out in two places, but I couldn't think of how to do it except to use polymorphic variants (`Variants).
I still don't have a good use case for those (are they also called open unions?). How would they work in this instance? (BTW, I almost said, "with polymorphic variants, you lose the compiler checking for all the data constructors" -- but then I noticed that this particular mistake was possible in the first place because the code around the first hunk had already used a "_" catch-all pattern!) Laszlo
It seems to be possible with phantom types & GADTs which are a very dark corner of OCaml and one which we shouldn't use, but here's how: ---------------------------------------------------------------------- type nullable type not_nullable type _ arg = | Char : not_nullable arg | Int : not_nullable arg | String : nullable arg | List : nullable arg (* # Char ;; - : not_nullable arg = Char # String ;; - : nullable arg = String *) let only_for_nullables = function | String -> "do something" | List -> "do something else" | not_nullable -> . (* val only_for_nullables : nullable arg -> string = <fun> *) ---------------------------------------------------------------------- Notes: (1) The “| not_nullable -> .” line can be omitted. It's just there to document the unreachable case, but (in this case) the compiler can infer this. (2) If you omit either of the “| String“ or “| List“ cases then the compile will warn about incomplete matching, which is good. Extending the example further, you can see how GADTs make the common case (second one below) possible, but harder to write: ---------------------------------------------------------------------- # let only_not_nullables = function | Char -> "char" | Int -> "int" ;; val only_not_nullables : not_nullable arg -> string = <fun> # let all : type a. a arg -> string = function | Char -> "char" | Int -> "int" | String -> "string" | List -> "list";; val all : 'a arg -> string = <fun> ---------------------------------------------------------------------- Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
On Thu, Sep 29, 2022 at 10:58:36AM +0100, Richard W.M. Jones wrote:
It seems to be possible with phantom types & GADTs which are a very dark corner of OCaml and one which we shouldn't use, but here's how:
----------------------------------------------------------------------
type nullable type not_nullable
Probably would have been better if I'd called those phantom types "primitive" and "boxed" :-) Rich.
type _ arg = | Char : not_nullable arg | Int : not_nullable arg | String : nullable arg | List : nullable arg
(* # Char ;; - : not_nullable arg = Char # String ;; - : nullable arg = String *)
let only_for_nullables = function | String -> "do something" | List -> "do something else" | not_nullable -> . (* val only_for_nullables : nullable arg -> string = <fun> *)
----------------------------------------------------------------------
Notes:
(1) The “| not_nullable -> .” line can be omitted. It's just there to document the unreachable case, but (in this case) the compiler can infer this.
(2) If you omit either of the “| String“ or “| List“ cases then the compile will warn about incomplete matching, which is good.
Extending the example further, you can see how GADTs make the common case (second one below) possible, but harder to write:
---------------------------------------------------------------------- # let only_not_nullables = function | Char -> "char" | Int -> "int" ;; val only_not_nullables : not_nullable arg -> string = <fun>
# let all : type a. a arg -> string = function | Char -> "char" | Int -> "int" | String -> "string" | List -> "list";; val all : 'a arg -> string = <fun> ----------------------------------------------------------------------
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On 09/29/22 11:58, Richard W.M. Jones wrote:
It seems to be possible with phantom types & GADTs which are a very dark corner of OCaml and one which we shouldn't use, but here's how:
*shudder* thanks for writing this all up, but I think I'm *swiftly* taking the above hint! :) Laszlo
----------------------------------------------------------------------
type nullable type not_nullable
type _ arg = | Char : not_nullable arg | Int : not_nullable arg | String : nullable arg | List : nullable arg
(* # Char ;; - : not_nullable arg = Char # String ;; - : nullable arg = String *)
let only_for_nullables = function | String -> "do something" | List -> "do something else" | not_nullable -> . (* val only_for_nullables : nullable arg -> string = <fun> *)
----------------------------------------------------------------------
Notes:
(1) The “| not_nullable -> .” line can be omitted. It's just there to document the unreachable case, but (in this case) the compiler can infer this.
(2) If you omit either of the “| String“ or “| List“ cases then the compile will warn about incomplete matching, which is good.
Extending the example further, you can see how GADTs make the common case (second one below) possible, but harder to write:
---------------------------------------------------------------------- # let only_not_nullables = function | Char -> "char" | Int -> "int" ;; val only_not_nullables : not_nullable arg -> string = <fun>
# let all : type a. a arg -> string = function | Char -> "char" | Int -> "int" | String -> "string" | List -> "list";; val all : 'a arg -> string = <fun> ----------------------------------------------------------------------
Rich.
In the C API documentation mention the potential problems of calling non-nullable parameters with NULL. Usually an error is returned, but warnings and worse might happen too. Thanks: Eric Blake --- docs/libnbd.pod | 18 ++++++++++++++++++ generator/C.ml | 21 +++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/docs/libnbd.pod b/docs/libnbd.pod index eae9bb413b..170fc1fa07 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -387,8 +387,26 @@ The library ran out of memory while performing some operation. A request is too large, for example if you try to read too many bytes in a single L<nbd_pread(3)> call. +=item C<EFAULT> + +A pointer parameter was C<NULL> when it should be non-NULL. +See the section below. + =back +=head2 Non-NULL parameters + +Almost all libnbd functions when called from C take one or more +pointer parameters that must not be C<NULL>. For example, the handle +parameter, strings and buffers should usually not be C<NULL>. + +If a C<NULL> is passed as one of these parameters, libnbd attempts to +return an error with L<nbd_get_errno(3)> returning C<EFAULT>. + +However it may cause other compiler-related warnings and even +undefined behaviour, so you should try to avoid this programming +mistake. + =head1 DEBUGGING MESSAGES Libnbd can print lots of debugging messages, useful if you have a diff --git a/generator/C.ml b/generator/C.ml index bfc216609e..dc123d20a4 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -107,6 +107,11 @@ let | UInt64 n -> [n] | UIntPtr n -> [n] +let name_of_optarg = + function + | OClosure { cbname } -> cbname + | OFlags (n, _, _) -> n + let arg_attr_nonnull = function (* BytesIn/Out are passed using a non-null pointer, and size_t *) @@ -974,6 +979,7 @@ let pr "=head1 ERRORS\n"; pr "\n"; + if may_set_error then ( let value = match errcode with | Some value -> value @@ -987,6 +993,21 @@ let pr "This function does not fail.\n"; pr "\n"; + pr "The following parameters must not be NULL: C<h>"; + List.iter ( + fun arg -> + let nns = arg_attr_nonnull arg in + if List.mem true nns then pr ", C<%s>" (List.hd (name_of_arg arg)) + ) args; + List.iter ( + fun arg -> + let nns = optarg_attr_nonnull arg in + if List.mem true nns then pr ", C<%s>" (name_of_optarg arg) + ) optargs; + pr ".\n"; + pr "For more information see L<libnbd(3)/Non-NULL parameters>.\n"; + pr "\n"; + if permitted_states <> [] then ( pr "=head1 HANDLE STATE\n"; pr "\n"; -- 2.37.0.rc2
On 09/28/22 11:30, Richard W.M. Jones wrote:
In the C API documentation mention the potential problems of calling non-nullable parameters with NULL. Usually an error is returned, but warnings and worse might happen too.
Thanks: Eric Blake --- docs/libnbd.pod | 18 ++++++++++++++++++ generator/C.ml | 21 +++++++++++++++++++++ 2 files changed, 39 insertions(+)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod index eae9bb413b..170fc1fa07 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -387,8 +387,26 @@ The library ran out of memory while performing some operation. A request is too large, for example if you try to read too many bytes in a single L<nbd_pread(3)> call.
+=item C<EFAULT> + +A pointer parameter was C<NULL> when it should be non-NULL. +See the section below. + =back
+=head2 Non-NULL parameters + +Almost all libnbd functions when called from C take one or more +pointer parameters that must not be C<NULL>. For example, the handle +parameter, strings and buffers should usually not be C<NULL>. + +If a C<NULL> is passed as one of these parameters, libnbd attempts to +return an error with L<nbd_get_errno(3)> returning C<EFAULT>. + +However it may cause other compiler-related warnings and even +undefined behaviour, so you should try to avoid this programming +mistake. + =head1 DEBUGGING MESSAGES
Libnbd can print lots of debugging messages, useful if you have a diff --git a/generator/C.ml b/generator/C.ml index bfc216609e..dc123d20a4 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -107,6 +107,11 @@ let | UInt64 n -> [n] | UIntPtr n -> [n]
+let name_of_optarg = + function + | OClosure { cbname } -> cbname + | OFlags (n, _, _) -> n + let arg_attr_nonnull = function (* BytesIn/Out are passed using a non-null pointer, and size_t *) @@ -974,6 +979,7 @@ let
pr "=head1 ERRORS\n"; pr "\n"; + if may_set_error then ( let value = match errcode with | Some value -> value @@ -987,6 +993,21 @@ let pr "This function does not fail.\n"; pr "\n";
+ pr "The following parameters must not be NULL: C<h>"; + List.iter ( + fun arg -> + let nns = arg_attr_nonnull arg in + if List.mem true nns then pr ", C<%s>" (List.hd (name_of_arg arg)) + ) args; + List.iter ( + fun arg -> + let nns = optarg_attr_nonnull arg in + if List.mem true nns then pr ", C<%s>" (name_of_optarg arg) + ) optargs; + pr ".\n"; + pr "For more information see L<libnbd(3)/Non-NULL parameters>.\n"; + pr "\n"; + if permitted_states <> [] then ( pr "=head1 HANDLE STATE\n"; pr "\n";
Acked-by: Laszlo Ersek <lersek@redhat.com>
Change check into an assertion, and add detailed comments explaining our assumptions. Updates: commit d0fbb769286a97728b0d1358e7accc2eb708d795 --- lib/utils.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/utils.c b/lib/utils.c index cc31b00309..93e520d2fb 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -79,11 +79,21 @@ nbd_internal_copy_string_list (string_vector *v, char **in) int nbd_internal_set_argv (struct nbd_handle *h, char **argv) { - /* FIXME: Do we want to assert(argv)? */ - if (argv == NULL || argv[0] == NULL) { + /* This should never be NULL. Normally the generator adds code to + * each StringList call in lib/api.c to check this and return an + * error. + */ + assert (argv); + + /* Because this function is only called from functions that take + * argv-style lists of strings (such as nbd_connect_command) we can + * check here that the command name is present. + */ + if (argv[0] == NULL) { set_error (EINVAL, "missing command name in argv list"); return -1; } + if (nbd_internal_copy_string_list (&h->argv, argv) == -1) { set_error (errno, "realloc"); return -1; -- 2.37.0.rc2
On 09/28/22 11:30, Richard W.M. Jones wrote:
Change check into an assertion, and add detailed comments explaining our assumptions.
Updates: commit d0fbb769286a97728b0d1358e7accc2eb708d795 --- lib/utils.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/lib/utils.c b/lib/utils.c index cc31b00309..93e520d2fb 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -79,11 +79,21 @@ nbd_internal_copy_string_list (string_vector *v, char **in) int nbd_internal_set_argv (struct nbd_handle *h, char **argv) { - /* FIXME: Do we want to assert(argv)? */ - if (argv == NULL || argv[0] == NULL) { + /* This should never be NULL. Normally the generator adds code to + * each StringList call in lib/api.c to check this and return an + * error. + */ + assert (argv); + + /* Because this function is only called from functions that take + * argv-style lists of strings (such as nbd_connect_command) we can + * check here that the command name is present. + */ + if (argv[0] == NULL) { set_error (EINVAL, "missing command name in argv list"); return -1; } + if (nbd_internal_copy_string_list (&h->argv, argv) == -1) { set_error (errno, "realloc"); return -1;
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones