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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
