Re: xenstore_lib.h and libxenstore API/ABI problems

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Andrew Cooper writes ("xenstore_lib.h and libxenstore API/ABI problems"):
> libxenstored does not have a stable ABI.  xenstore_lib.h contains:
> 
> * struct xs_permissions, which contains an enum - one of the very common
> ABI traps.

enums are a problem when used as paraeter or return values or in
structs.  The mere *definition* of an enum is not a problem, although
it is a programmer hazard because it's an invitation to ABI fail.

But I think this is less bad in practice than we fear because we need
only concern ourselves with ABIs which can run on Xen-supported CPUs,
and for which these tools libraries might plausibly have been
compiled.

That means modern 32- and 64-bit x86 and ARM.  I think on all such
systems, an enum with values that will fit into an int, is precisely
an int in the ABI.  (I checked that this is true in practice on an
ARM32 Linux poerterbox belonging to the Debian Project.)

In which case we can change all of the formal parameters and struct
members from "enum xs_perm_type" to "int" and their ABI will remain
the same on all supported platforms.

> * the prototypes for expanding_buffer_ensure(), sanitise_value() and
> unsanitise_value(), which are inappropriately-namespaced symbols in
> libxenstore

I think it is unlikely that anything outside the Xen tools libraries
will link against these.  If anything, it is more likely that any such
linkage is a name clash.

I also think that a breach of ABI rules has no actual bad consequences
unless the symbol is actually referenced from a different executable
or DSO - and, specifically, an executable or DSO which is not
co-upgraded with the one at issue.

> * struct expanding_buffer with a classic string-handling trap (int len).

You mean that it should be a size_t, I suppose.  But I think this is
academic given that we want to de-publicise this.

> * Various xenstored-internal details such as struct xs_tdb_record_hdr,
> the internal permission bits, as well as stuff like
> xs_daemon_{root,run}dir(), and worse xs_daemon_tdb(),  which have
> absolutely no business being external to xenstored.

I think it is almost inconceivable that anyone has linked against
these symbols.

> My best suggestion is that we freeze 3.0.3 where it is, and create a 4.0
> which has all of the xenstore_lib.h content deleted.  That, AFAICT, gets
> us something which is 99% compatible, and stands a chance of being able
> to kept properly stable.

I suggest, instead, that we:

In 4.15:

 * Retain the current soname, but:
 * Delete the tdb internals from the header file and cease to export
   those symbols.
 * Rename the expanding_buffer and sanitise_value functions, to
   properly namespace them, and move them to a private header.

This is of course technically a breach of the ABI stability rules but
for the reasons I giveabove I don't think it will cause anyone any
trouble.

In xen-next:

 * Prophilactically change the uses of "enum xs_perm_type" to "int"
 * Or, add   XS__PERM_TYPE_ENSURE_ABI = 0x7fffffff  to the enum.

Ian.