Re: [PATCH-for-4.15] tools/libs/store: cleanup libxenstore interface

[Jürgen Groß <jgross@xxxxxxxx>]

On 24.03.21 12:02, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH-for-4.15] tools/libs/store: cleanup libxenstore 
> interface"):
> > There are some internals in the libxenstore interface which should be
> > removed.
> >
> > Move those functions into xs_lib.c and the related definitions into
> > xs_lib.h. Remove the functions from the mapfile. Add xs_lib.o to
> > xenstore_client as some of the internal functions are needed there.
> This seems wider in scope than I was expecting.
>
> Reviewing it again makes me think that there are more concers than I
> anticipated and I am now doubtful whether I want to take it in 4.15.
I'm fine with that. TBH I would have been surprised if you'd just take
it. :-)

> I thought at this stage we were just going to fix the
> accidentally-exported symbols with improperly namespaced names.  It is
> those for which I think that withdrawing them without an ABI soname
> bump, in contravention of usual library ABI stability rules, will not
> cause trouble in pracice.
Just removing them from the mapfile doesn't work.

Either we need to keep them (maybe with "xs_" prefixed), or we need
to go the way I've done in this patch.

> My current thoughts are that several of these really ought not to be
> withdrawn as they might cause actual trouble:
>
> >   /* Path for various daemon things: env vars can override. */
> > -const char *xs_daemon_rootdir(void);
> > -const char *xs_domain_dev(void);
> > -const char *xs_daemon_tdb(void);
> Someone who was writing bindings might have exposed these without
> knowing what they were, resulting in linkage to these symbols.
This patch is removing everything not being used in the (known) Xen
ecosystem (Xen, qemu, qemu-trad, mini-os).

> >   bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num,
> >                          const char *strings);
> >   
> > -/* Convert permissions to a string (up to len MAX_STRLEN(unsigned int)+1). */
> > -bool xs_perm_to_string(const struct xs_permissions *perm,
> > -                       char *buffer, size_t buf_len);
> Isn't this function potentially useful ?  It seems funny to have only
> one of the conversion directions.
As stated above: this patch is doing the absolute possible maximum.
I'm absolutely fine to drop some of the removals.

> > +void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
> Is it possible to do sort this out in a more minimal way ?  Eg we
> could change the name to namespace it properly.  (I haven't looked at
> the code in detail and am still rather under-caffeinated so maybe I am
> talking nonsense here.)
No nonsense. This would be the really minimum option (apart from doing
nothing).

I can setup the patch for that and keep the rest for 4.16 (which will
then probably need to bump the so version).


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature