Libguestfs - Sep 2020 - Re: [libnbd PATCH 1/3] generator: Introduce REnum/RFlags return types

On Sat, Sep 05, 2020 at 08:40:58PM -0500, Eric Blake
wrote:
> diff --git a/generator/API.mli b/generator/API.mli
> index 712e837..e45b5c0 100644
> --- a/generator/API.mli
> +++ b/generator/API.mli
> @@ -78,6 +78,8 @@ and ret >  | RString                  (** return a
newly allocated string,
>                                 caller frees, NULL for error *)
>  | RUInt                    (** return a bitmask, no error possible *)
> +| REnum of enum            (** return an enum, no error possible *)
> +| RFlags of flags          (** return bitmask of flags, no error possible
*)
>  and closure = {
>    cbname : string;         (** name of callback function *)
>    cbargs : cbarg list;     (** all closures return int for now *)
> diff --git a/generator/C.ml b/generator/C.ml
> index 6b65f6e..1eb5e85 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -66,15 +66,15 @@ let errcode_of_ret >    function
>    | RBool | RErr | RFd | RInt | RInt64 | RCookie -> Some "-1"
>    | RStaticString | RString -> Some "NULL"
> -  | RUInt -> None (* errors not possible *)
> +  | RUInt | REnum (_) | RFlags (_) -> None (* errors not possible *)
> 
>  let type_of_ret >    function
> -  | RBool | RErr | RFd | RInt -> "int"
> +  | RBool | RErr | RFd | RInt | REnum (_) -> "int"
This is good because Enum is passed as int.
> [...]
You used unsigned for RFlags, but shouldn't it be this instead?

+  | RFlags (_) -> "uint32_t"

The rest of the patch looks fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 9/7/20 9:13 AM, Richard W.M. Jones wrote:
>> +++ b/generator/C.ml
>> @@ -66,15 +66,15 @@ let errcode_of_ret >>     function
>>     | RBool | RErr | RFd | RInt | RInt64 | RCookie -> Some
"-1"
>>     | RStaticString | RString -> Some "NULL"
>> -  | RUInt -> None (* errors not possible *)
>> +  | RUInt | REnum (_) | RFlags (_) -> None (* errors not possible
*)
>>
>>   let type_of_ret >>     function
>> -  | RBool | RErr | RFd | RInt -> "int"
>> +  | RBool | RErr | RFd | RInt | REnum (_) -> "int"
> 
> This is good because Enum is passed as int.
> 
>> [...]
> 
> You used unsigned for RFlags, but shouldn't it be this instead?
> 
> +  | RFlags (_) -> "uint32_t"
Hmm.  For Flags as input, we take uint32_t, but for get_handshake_flags 
which used to return RUint, we returned unsigned; as written, my patch 
preserved generated code in its entirety.  Changing it to return 
uint32_t makes sense, but is a minor API change.  Fortunately, it would 
not be an ABI change on any of the platforms we compile on.  So I can go 
ahead and return the smarter type unless we have a strong reason not to 
(the API change might affect anyone that was storing a function pointer 
to nbd_get_handshake_flags, and more so in C++ than C).
> 
> The rest of the patch looks fine.
> 
> Rich.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Sep 07, 2020 at 09:38:15AM -0500, Eric Blake
wrote:
> On 9/7/20 9:13 AM, Richard W.M. Jones wrote:
> 
> >>+++ b/generator/C.ml
> >>@@ -66,15 +66,15 @@ let errcode_of_ret > >>    function
> >>    | RBool | RErr | RFd | RInt | RInt64 | RCookie -> Some
"-1"
> >>    | RStaticString | RString -> Some "NULL"
> >>-  | RUInt -> None (* errors not possible *)
> >>+  | RUInt | REnum (_) | RFlags (_) -> None (* errors not
possible *)
> >>
> >>  let type_of_ret > >>    function
> >>-  | RBool | RErr | RFd | RInt -> "int"
> >>+  | RBool | RErr | RFd | RInt | REnum (_) -> "int"
> >
> >This is good because Enum is passed as int.
> >
> >>[...]
> >
> >You used unsigned for RFlags, but shouldn't it be this instead?
> >
> >+  | RFlags (_) -> "uint32_t"
> 
> Hmm.  For Flags as input, we take uint32_t, but for
> get_handshake_flags which used to return RUint, we returned
> unsigned; as written, my patch preserved generated code in its
> entirety.  Changing it to return uint32_t makes sense, but is a
> minor API change.
Oh I see, good point.

I think although technically it might not be an ABI change,
best to leave it as you wrote it.

Rich.
> Fortunately, it would not be an ABI change on any
> of the platforms we compile on.  So I can go ahead and return the
> smarter type unless we have a strong reason not to (the API change
> might affect anyone that was storing a function pointer to
> nbd_get_handshake_flags, and more so in C++ than C).
> 
> >
> >The rest of the patch looks fine.
> >
> >Rich.
> >
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/