Libguestfs - Aug 2019 - Re: [nbdkit PATCH] filters: Bump API version

We do not promise API compatibility for filters between stable
releases of nbdkit, however, we should at least ensure that when we do
break API, that we refuse to load a filter compiled against one
version of nbdkit with another server running a different API.  A
single bump once per stable release is good enough (rather than once
per API change).

We did this correctly for commits b0ce4411/cb309687/df0cc21d (bumping
to API version 2 for the combined changes between v1.2 and v1.4), but
failed to do so for f184fdc3 (affecting v1.10), 4ca66f70 (affecting
v1.12), or 5ee7bd29/ee61d232 (affecting the upcoming v1.14).  So do it
retroacively now, as well as backporting intermediate bumps to
affected stable branches.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

I'm pushing this soon.

 include/nbdkit-filter.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 94f17789..1ebd1cb6 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -43,7 +43,7 @@
 extern "C" {
 #endif

-#define NBDKIT_FILTER_API_VERSION 2
+#define NBDKIT_FILTER_API_VERSION 5

 struct nbdkit_extent {
   uint64_t offset;
@@ -100,8 +100,10 @@ struct nbdkit_filter {
    */
   int _api_version;

-  /* Because there is no ABI guarantee, new fields may be added
-   * where logically appropriate.  */
+  /* Because there is no ABI guarantee, new fields may be added where
+   * logically appropriate, as long as we correctly bump
+   * NBDKIT_FILTER_API_VERSION once per stable release.
+   */
   const char *name;
   const char *longname;
   const char *version;
-- 
2.21.0

On Mon, Aug 26, 2019 at 12:15:27PM -0500, Eric Blake
wrote:
> We do not promise API compatibility for filters between stable
> releases of nbdkit, however, we should at least ensure that when we do
> break API, that we refuse to load a filter compiled against one
> version of nbdkit with another server running a different API.  A
> single bump once per stable release is good enough (rather than once
> per API change).
> 
> We did this correctly for commits b0ce4411/cb309687/df0cc21d (bumping
> to API version 2 for the combined changes between v1.2 and v1.4), but
> failed to do so for f184fdc3 (affecting v1.10), 4ca66f70 (affecting
> v1.12), or 5ee7bd29/ee61d232 (affecting the upcoming v1.14).  So do it
> retroacively now, as well as backporting intermediate bumps to
> affected stable branches.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> I'm pushing this soon.
> 
>  include/nbdkit-filter.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
> index 94f17789..1ebd1cb6 100644
> --- a/include/nbdkit-filter.h
> +++ b/include/nbdkit-filter.h
> @@ -43,7 +43,7 @@
>  extern "C" {
>  #endif
> 
> -#define NBDKIT_FILTER_API_VERSION 2
> +#define NBDKIT_FILTER_API_VERSION 5
> 
>  struct nbdkit_extent {
>    uint64_t offset;
> @@ -100,8 +100,10 @@ struct nbdkit_filter {
>     */
>    int _api_version;
> 
> -  /* Because there is no ABI guarantee, new fields may be added
> -   * where logically appropriate.  */
> +  /* Because there is no ABI guarantee, new fields may be added where
> +   * logically appropriate, as long as we correctly bump
> +   * NBDKIT_FILTER_API_VERSION once per stable release.
> +   */
>    const char *name;
>    const char *longname;
>    const char *version;
ACK, although maybe we should just check the version strings are equal ...

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 8/27/19 7:27 AM, Richard W.M. Jones wrote:
> On Mon, Aug 26, 2019 at 12:15:27PM -0500, Eric Blake wrote:
>> We do not promise API compatibility for filters between stable
>> releases of nbdkit, however, we should at least ensure that when we do
>> break API, that we refuse to load a filter compiled against one
>> version of nbdkit with another server running a different API.  A
>> single bump once per stable release is good enough (rather than once
>> per API change).
>>
>> We did this correctly for commits b0ce4411/cb309687/df0cc21d (bumping
>> to API version 2 for the combined changes between v1.2 and v1.4), but
>> failed to do so for f184fdc3 (affecting v1.10), 4ca66f70 (affecting
>> v1.12), or 5ee7bd29/ee61d232 (affecting the upcoming v1.14).  So do it
>> retroacively now, as well as backporting intermediate bumps to
>> affected stable branches.
>> @@ -100,8 +100,10 @@ struct nbdkit_filter {
>>     */
>>    int _api_version;
>>
>> -  /* Because there is no ABI guarantee, new fields may be added
>> -   * where logically appropriate.  */
>> +  /* Because there is no ABI guarantee, new fields may be added where
>> +   * logically appropriate, as long as we correctly bump
>> +   * NBDKIT_FILTER_API_VERSION once per stable release.
>> +   */
>>    const char *name;
>>    const char *longname;
>>    const char *version;
> 
> ACK, although maybe we should just check the version strings are equal ...
We could s/int _api_version/char *_api_string/ (which is itself an
API-incompatible change). Doing so makes it that much tighter that you
have to run a filter from the same version as nbdkit (whereas our
current API check allows running 1.13.1 nbdkit with 1.13.2 filter or
vice-versa, and that may not always be accurate).  Historically, we
began filters with API version 1 (not 0), which works to our advantage:
all released nbdkit are looking for filters in the range [1,5).
ABI-wise, there are four cases to consider:

32-bit pointer, little-endian: no compiler puts a string constant in the
first page of memory, so any string pointer will be much larger than 5
and therefore show up as incompatible to older nbdkit.  Newer nbdkit
will have to be careful to not dereference a char* until first
validating that interpreting it as an int looks like a reasonable pointer.

64-bit pointer, little-endian: the first four bytes of any pointer are
the least significant; a compiler _could_ place the string constant of a
new filter at 0x100000004 which would look like API version 4 to old
nbdkit; so we get further in attempting to use the mismatched version.
Conversely, new nbdkit loading an old filter has to do the pre-screen to
attempt to not dereference a pointer where the first 4 bytes look like a
small integer; and this could mean nbdkit fails to load a new filter
where the compiler placed the version string at 0x100000004.  But this
placement is corner-case enough (depends on a platform ABI where the
memory map places read-only portions of the binary above 4G even when
the binary itself is not that large), so I don't think we need to worry
about it.

32-bit pointer, big-endian: similar story to little-endian; we can tell
whether the 4 bytes looks like a small integer or a valid pointer.

64-bit pointer, big-endian: the first four bytes of a pointer are likely
to be 0x00000000 (unless a compiler places read-only memory above 4G);
and since 0 is not a valid API version, old nbdkit will reject new
filters; new nbdkit can check if the first four bytes are 0 (assume
pointer) or small integer (reject old filter).


The goal here is some basic sanity checking; if a user mismatches
versions, they are already using nbdkit incorrectly, so if it crashes
instead of giving graceful failure, there's not much we can do about it.
 The sanity checking will work most of the time, however, giving us
graceful failure.

I'll post a patch along those lines shortly.

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