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