Laszlo Ersek
2022-Sep-29 09:17 UTC
[Libguestfs] [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
Richard W.M. Jones
2022-Sep-29 09:58 UTC
[Libguestfs] [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