Re: [Xen-devel] [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for ~/control/shutdown

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: 16 November 2015 12:55
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Ian Jackson; Stefano Stabellini; Wei Liu
> Subject: Re: [RFC PATCH 2/6] libxl: stop using libxl__xs_mkdir() for
> ~/control/shutdown
> 
> On Wed, 2015-11-11 at 16:18 +0000, Paul Durrant wrote:
> > As documented in docs/misc/xenstore, the XS_MKDIR operation merely
> makes
> > sure a path exists whereas ~/control/shutdown needs to start empty. Also
> > using XS_MKDIR for a node which is never supposed to have children is
> > somewhat counterintuitive.
> >
> > This patch introduces a new libxl__xs_printf_perms() function analogous
> > to libxl__xs_printf() but taking explicit permissions arguments, and then
> > uses this to create an empty ~/control/shutdown node.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> > Âtools/libxl/libxl_create.cÂÂÂ| 16 ++++++++++------
> > Âtools/libxl/libxl_internal.h | 10 ++++++++++
> > Âtools/libxl/libxl_xshelp.cÂÂÂ| 44
> > ++++++++++++++++++++++++++++++++++++++------
> > Â3 files changed, 58 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 921d155..279deda 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -484,7 +484,7 @@ int libxl__domain_make(libxl__gc *gc,
> > libxl_domain_config *d_config,
> > ÂÂÂÂÂlibxl_ctx *ctx = libxl__gc_owner(gc);
> > ÂÂÂÂÂint flags, ret, rc, nb_vm;
> > ÂÂÂÂÂchar *uuid_string;
> > -ÂÂÂÂchar *dom_path, *vm_path, *libxl_path;
> > +ÂÂÂÂchar *dom_path, *vm_path, *libxl_path, *control_path;
> > ÂÂÂÂÂstruct xs_permissions roperm[2];
> > ÂÂÂÂÂstruct xs_permissions rwperm[1];
> > ÂÂÂÂÂstruct xs_permissions noperm[1];
> > @@ -605,17 +605,21 @@ retry_transaction:
> > ÂÂÂÂÂlibxl__xs_mkdir(gc, t,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__sprintf(gc, "%s/device", dom_path),
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂroperm, ARRAY_SIZE(roperm));
> > -ÂÂÂÂlibxl__xs_mkdir(gc, t,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__sprintf(gc, "%s/control", dom_path),
> > +
> > +ÂÂÂÂcontrol_path = libxl__sprintf(gc, "%s/control", dom_path);
> > +
> > +ÂÂÂÂlibxl__xs_mkdir(gc, t, control_path,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂroperm, ARRAY_SIZE(roperm));
> > ÂÂÂÂÂif (info->type == LIBXL_DOMAIN_TYPE_HVM)
> > ÂÂÂÂÂÂÂÂÂlibxl__xs_mkdir(gc, t,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__sprintf(gc, "%s/hvmloader", dom_path),
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂroperm, ARRAY_SIZE(roperm));
> >
> > -ÂÂÂÂlibxl__xs_mkdir(gc, t,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__sprintf(gc, "%s/control/shutdown", dom_path),
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrwperm, ARRAY_SIZE(rwperm));
> > +ÂÂÂÂlibxl__xs_printf_perms(gc, t,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__sprintf(gc, "%s/shutdown",
> > control_path),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrwperm, ARRAY_SIZE(rwperm),
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"");
> > +
> > ÂÂÂÂÂlibxl__xs_mkdir(gc, t,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl__sprintf(gc, "%s/device/suspend/event-
> > channel", dom_path),
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrwperm, ARRAY_SIZE(rwperm));
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 4710804..04d8a29 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -666,6 +666,16 @@ _hidden int libxl__xs_writev_perms(libxl__gc *gc,
> > xs_transaction_t t,
> > Â_hidden int libxl__xs_writev_atonce(libxl__gc *gc,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *dir, char **kvs);
> >
> > +_hidden int libxl__xs_vprintf_perms(libxl__gc *gc, xs_transaction_t t,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct xs_permissions *perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int num_perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *fmt, va_list ap);
> > +_hidden int libxl__xs_printf_perms(libxl__gc *gc, xs_transaction_t t,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct xs_permissions *perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int num_perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *fmt, ...)
> > PRINTF_ATTRIBUTE(6, 7);
> > Â_hidden int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path, const char *fmt, ...)
> > PRINTF_ATTRIBUTE(4, 5);
> > ÂÂÂÂ/* Each fn returns 0 on success.
> > diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> > index 3cac4f2..0b56f8b 100644
> > --- a/tools/libxl/libxl_xshelp.c
> > +++ b/tools/libxl/libxl_xshelp.c
> > @@ -96,25 +96,57 @@ out:
> >
> > Â}
> >
> > -int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path, const char *fmt, ...)
> > +int libxl__xs_vprintf_perms(libxl__gc *gc, xs_transaction_t t,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct xs_permissions *perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int num_perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *fmt,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂva_list ap)
> > Â{
> > ÂÂÂÂÂlibxl_ctx *ctx = libxl__gc_owner(gc);
> > ÂÂÂÂÂchar *s;
> > -ÂÂÂÂva_list ap;
> > ÂÂÂÂÂint ret;
> > -ÂÂÂÂva_start(ap, fmt);
> > -ÂÂÂÂret = vasprintf(&s, fmt, ap);
> > -ÂÂÂÂva_end(ap);
> >
> > +ÂÂÂÂret = vasprintf(&s, fmt, ap);
> > ÂÂÂÂÂif (ret == -1) {
> > ÂÂÂÂÂÂÂÂÂreturn -1;
> > ÂÂÂÂÂ}
> > ÂÂÂÂÂxs_write(ctx->xsh, t, path, s, ret);
> > +ÂÂÂÂif (perms)
> > +ÂÂÂÂÂÂÂÂxs_set_permissions(ctx->xsh, t, path, perms, num_perms);
> 
> This can fail, can't it? (OTOH so can xs_write, so maybe there is some
> reason we apparently don't care for such things here?)
> 

That's a good point. I would have thought wiring an xs_write() failure back 
through to the caller would be a good idea (and same goes for 
xs_set_permissions, I guess).

  Paul

> > ÂÂÂÂÂfree(s);
> > ÂÂÂÂÂreturn 0;
> > Â}
> >
> > +int libxl__xs_printf_perms(libxl__gc *gc, xs_transaction_t t,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct xs_permissions *perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int num_perms,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *fmt, ...)
> > +{
> > +ÂÂÂÂva_list ap;
> > +ÂÂÂÂint ret;
> > +
> > +ÂÂÂÂva_start(ap, fmt);
> > +ÂÂÂÂret = libxl__xs_vprintf_perms(gc, t, path, perms, num_perms, fmt,
> > ap);
> > +ÂÂÂÂva_end(ap);
> > +
> > +ÂÂÂÂreturn ret;
> > +}
> > +
> > +int libxl__xs_printf(libxl__gc *gc, xs_transaction_t t,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst char *path, const char *fmt, ...)
> > +{
> > +ÂÂÂÂva_list ap;
> > +ÂÂÂÂint ret;
> > +
> > +ÂÂÂÂva_start(ap, fmt);
> > +ÂÂÂÂret = libxl__xs_vprintf_perms(gc, t, path, NULL, 0, fmt, ap);
> > +ÂÂÂÂva_end(ap);
> > +
> > +ÂÂÂÂreturn ret;
> > +}
> > +
> > Âchar * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char
> > *path)
> > Â{
> > ÂÂÂÂÂlibxl_ctx *ctx = libxl__gc_owner(gc);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel