Libguestfs - Aug 2019 - Re: [PATCH libnbd v2 10/10] generator: Check requirements for BytesPersistIn/Out and completion callbacks.

On 8/15/19 4:56 AM, Richard W.M. Jones wrote:
> ---
>  generator/generator | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/generator/generator b/generator/generator
> index 1252bdb..13cb0b9 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3121,6 +3121,31 @@ let () >          failwithf "%s:
first_version must be 1.x" name;
>        if minor mod 2 <> 0 then
>          failwithf "%s: first_version must refer to a stable
release" name
> +  ) handle_calls;
> +
> +  (* Because of the way we use completion free callbacks to
> +   * free persistent buffers in non-C languages, any function
> +   * with a BytesPersistIn/Out parameter must have only one.
> +   * And it must have an OClosure completion optarg.
> +   *)
I like it.  Other than the unintended semantic change in patch 4, I
think this series is ready to go.

We still had another potential API change to squeeze into 0.9.8:
Right now, the API is overloading 'command':
nbd_connect_command/nbd_kill_command
vs.
nbd_aio_command_completed/nbd_aio_peek_command_completed

Keeping nbd_connect_command may be okay (it takes a command line to
create a subprocess), but we may want to rename nbd_kill_command to
nbd_kill_child or similar, to make it obvious that it is NOT associated
with attempting an early abort of any synchronous nbd_pread or other
commands issued to the server.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Aug 15, 2019 at 07:01:49AM -0500, Eric Blake
wrote:
> We still had another potential API change to squeeze into 0.9.8:
> Right now, the API is overloading 'command':
> nbd_connect_command/nbd_kill_command
> vs.
> nbd_aio_command_completed/nbd_aio_peek_command_completed
> 
> Keeping nbd_connect_command may be okay (it takes a command line to
> create a subprocess), but we may want to rename nbd_kill_command to
> nbd_kill_child or similar, to make it obvious that it is NOT associated
> with attempting an early abort of any synchronous nbd_pread or other
> commands issued to the server.
Shall we rename nbd_kill_command -> nbd_kill_subprocess?  (kill_child
sounds a bit weird.)

As for the other API changes added to TODO, I think we can deal with
them by adding variants at a later date.

For example we wanted some "allow" flags for nbd_connect_uri, but
didn't really come to a conclusion about how to do that right now.
However in future we could do it by either adding

  nbd_connect_uri_allow (..., allow_flags);

or (probably my preferred choice) using ELF symbol versions.

Are there other libraries apart from glibc where they are using ELF
symbol versioning to replace APIs?  AIUI if done carefully it doesn't
need to lead to API breaks, because as with nbdkit we can require
callers to opt in to the new APIs by defining a symbol like

  #define LIBNBD_API_VERSION 2

(As you can see, I'm keen to release a final prerelease, get fio
updated, and go for 1.0 as soon as we can.)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 8/15/19 7:07 AM, Richard W.M. Jones wrote:
> On Thu, Aug 15, 2019 at 07:01:49AM -0500, Eric Blake wrote:
>> We still had another potential API change to squeeze into 0.9.8:
>> Right now, the API is overloading 'command':
>> nbd_connect_command/nbd_kill_command
>> vs.
>> nbd_aio_command_completed/nbd_aio_peek_command_completed
>>
>> Keeping nbd_connect_command may be okay (it takes a command line to
>> create a subprocess), but we may want to rename nbd_kill_command to
>> nbd_kill_child or similar, to make it obvious that it is NOT associated
>> with attempting an early abort of any synchronous nbd_pread or other
>> commands issued to the server.
> 
> Shall we rename nbd_kill_command -> nbd_kill_subprocess?  (kill_child
> sounds a bit weird.)
> 
Yes, that sounds nicer.
> As for the other API changes added to TODO, I think we can deal with
> them by adding variants at a later date.
> 
> For example we wanted some "allow" flags for nbd_connect_uri, but
> didn't really come to a conclusion about how to do that right now.
> However in future we could do it by either adding
> 
>   nbd_connect_uri_allow (..., allow_flags);
> 
> or (probably my preferred choice) using ELF symbol versions.
> 
> Are there other libraries apart from glibc where they are using ELF
> symbol versioning to replace APIs?
I'm not immediately aware of any (libvirt uses versioning, but has not
replaced symbols, and many shared libraries are abysmal when it comes to
backwards compatibility), but it does seem doable.
>  AIUI if done carefully it doesn't
> need to lead to API breaks, because as with nbdkit we can require
> callers to opt in to the new APIs by defining a symbol like
> 
>   #define LIBNBD_API_VERSION 2
Indeed, with enough care, you can ensure that both old and new programs
continue to compile, and when linked with a new library, behave the same
as they did with the version of the library they were compiled against.
> 
> (As you can see, I'm keen to release a final prerelease, get fio
> updated, and go for 1.0 as soon as we can.)
> 
> Rich.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org