Libguestfs - Sep 2022 - [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL

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?

Laszlo

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