Re: [Libguestfs] [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).