Libguestfs - Sep 2022 - [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

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