4:30 a.m.
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.
4:30 a.m.
New subject: [PATCH libnbd v2 1/4] generator: Add attribute((nonnull)) annotations to non-NULL parameters
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
5:40 a.m.
New subject: [PATCH libnbd v2 1/4] generator: Add attribute((nonnull)) annotations to non-NULL parameters
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(a)redhat.com>
Laszlo
4:30 a.m.
New subject: [PATCH libnbd v2 2/4] 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 ++++++-
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
5:47 a.m.
New subject: [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL
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
12:18 p.m.
New subject: [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL
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
4:17 a.m.
New subject: [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL
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
4:58 a.m.
New subject: [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL
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
5:01 a.m.
New subject: [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL
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
8:52 a.m.
New subject: [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL
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.
4:30 a.m.
New subject: [PATCH libnbd v2 3/4] generator: Document non-NULL behaviour of some parameters
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
5:48 a.m.
New subject: [PATCH libnbd v2 3/4] generator: Document non-NULL behaviour of some parameters
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(a)redhat.com>
4:30 a.m.
New subject: [PATCH libnbd v2 4/4] lib/utils.c: Assert that argv != NULL and add comments
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
5:51 a.m.
New subject: [PATCH libnbd v2 4/4] lib/utils.c: Assert that argv != NULL and add comments
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(a)redhat.com>
784
days inactive
785
days old
13 comments
2 participants
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones