Re: [PATCH 04/18] xen: create xenstore areas for XenDevice-s

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+paul.durrant=citrix.com@nongnu.org] On Behalf Of Paul Durrant
> Sent: 05 December 2018 12:05
> To: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>; Stefano Stabellini
> <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu-devel@nongnu.org;
> Max Reitz <mreitz@redhat.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Qemu-devel] [PATCH 04/18] xen: create xenstore areas for
> XenDevice-s
> 
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 29 November 2018 18:49
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> > devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>;
> > Kevin Wolf <kwolf@redhat.com>; Max Reitz <mreitz@redhat.com>
> > Subject: Re: [PATCH 04/18] xen: create xenstore areas for XenDevice-s
> >
> > On Wed, Nov 21, 2018 at 03:11:57PM +0000, Paul Durrant wrote:
> > > This patch adds a new source module, xen-bus-helper.c, which builds on
> > > basic libxenstore primitives to provide functions to create (setting
> > > permissions appropriately) and destroy xenstore areas, and functions
> to
> > > 'printf' and 'scanf' nodes therein. The main xen-bus code then uses
> > > these primitives [1] to initialize and destroy the frontend and
> backend
> > > areas for a XenDevice during realize and unrealize respectively.
> > >
> > > The 'xen-qdisk' implementation is extended with a 'get_name' method
> that
> > > returns the VBD number. This number is reqired to 'name' the xenstore
> >
> >                                          ^ required
> >
> 
> Ok.
> 
> > > diff --git a/hw/xen/xen-bus-helper.c b/hw/xen/xen-bus-helper.c
> > > new file mode 100644
> > > index 0000000000..d9ee2ed6a0
> > > --- /dev/null
> > > +++ b/hw/xen/xen-bus-helper.c
> > [...]
> > > +void xs_node_destroy(struct xs_handle *xsh, const char *node)
> > > +{
> > > +    xs_rm(xsh, XBT_NULL, node);
> >
> > We should check for error, and grab errno.
> >
> 
> I'll make all of them fill in an Error *
> 
> > > +}
> > > +
> > > +void xs_node_vprintf(struct xs_handle *xsh, const char *node,
> > > +                     const char *key, const char *fmt, va_list ap)
> > > +{
> > > +    char *path, *value;
> > > +
> > > +    path = (strlen(node) != 0) ? g_strdup_printf("%s/%s", node, key)
> :
> > > +        g_strdup(key);
> >
> > A comment would be helpful to findout how to use that function,
> > especialy the fact that with node="", we write to $key instead of
> > $node/$key.
> 
> Ok, I'll add comments into the header.
> 
> >
> > > +    value = g_strdup_vprintf(fmt, ap);
> >
> > Looks like g_vasprintf() would be better, since it returns the lenght as
> > well.
> >
> 
> Yes.

I tried this and it appears not to exist in the version of glib in my environment so I guess I'll stick with g_strdup_printf().

  Paul

> 
> > > +
> > > +    xs_write(xsh, XBT_NULL, path, value, strlen(value));
> >
> > You should check for failures, and grab errno.
> >
> > > +    g_free(value);
> > > +    g_free(path);
> > > +}
> > > +
> > > +int xs_node_vscanf(struct xs_handle *xsh, const char *node, const
> char
> > *key,
> > > +                   const char *fmt, va_list ap)
> > > +{
> > > +    char *path, *value;
> > > +    int rc;
> > > +
> > > +    path = (strlen(node) != 0) ? g_strdup_printf("%s/%s", node, key)
> :
> > > +        g_strdup(key);
> > > +
> > > +    value = xs_read(xsh, XBT_NULL, path, NULL);
> >
> > The xenstore.h isn't clear about failure of this function, it is
> > supposed to return a malloced value. Do we actually need to check if
> value
> > is NULL?
> 
> The comment above xs_read() in xs.c is:
> 
> /* Get the value of a single file, nul terminated.
>  * Returns a malloced value: call free() on it after use.
>  * len indicates length in bytes, not including the nul.
>  */
> 
> and I think we should check it for NULL before passing it to vsscanf().
> 
> >
> > > +
> > > +    rc = value ? vsscanf(value, fmt, ap) : EOF;
> > > +
> > > +    free(value);
> > > +    g_free(path);
> > > +
> > > +    return rc;
> > > +}
> > > +
> > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > > index dede2d914a..663aa8e117 100644
> > > --- a/hw/xen/xen-bus.c
> > > +++ b/hw/xen/xen-bus.c
> > [...]
> >
> > > +static void xen_device_backend_destroy(XenDevice *xendev)
> > > +{
> > > +    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > > +
> > > +    if (!xendev->backend_path) {
> > > +        return;
> > > +    }
> > > +
> > > +    g_assert(xenbus->xsh);
> > > +
> > > +    xs_node_destroy(xenbus->xsh, xendev->backend_path);
> > > +    g_free(xendev->backend_path);
> >
> > It would be nice to also set backend_path to NULL.
> >
> 
> Yes, it should be for idempotency.
> 
> > > diff --git a/include/hw/xen/xen-bus-helper.h b/include/hw/xen/xen-bus-
> > helper.h
> > > new file mode 100644
> > > index 0000000000..53570650db
> > > --- /dev/null
> > > +++ b/include/hw/xen/xen-bus-helper.h
> > > @@ -0,0 +1,26 @@
> > > +/*
> > > + * Copyright (c) Citrix Systems Inc.
> > > + * All rights reserved.
> > > + */
> > > +
> > > +#ifndef HW_XEN_BUS_HELPER_H
> > > +#define HW_XEN_BUS_HELPER_H
> >
> > That should probably include xen_common.h, to have `enum xenbus_state`,
> > `struct xs_handle`, ..
> 
> Ok.
> 
> >
> > > +const char *xs_strstate(enum xenbus_state state);
> > > +
> > > +void xs_node_create(struct xs_handle *xsh, const char *node,
> > > +                    struct xs_permissions perms[],
> > > +                    unsigned int nr_perms, Error **errp);
> > > +void xs_node_destroy(struct xs_handle *xsh, const char *node);
> > > +
> > > +void xs_node_vprintf(struct xs_handle *xsh, const char *node,
> > > +                     const char *key, const char *fmt, va_list ap);
> > > +void xs_node_printf(struct xs_handle *xsh, const char *node, const
> char
> > *key,
> > > +                    const char *fmt, ...);
> >
> > This prototype needs GCC_FMT_ATTR(), that's the printf format
> > __attribute__.
> >
> > > +
> > > +int xs_node_vscanf(struct xs_handle *xsh, const char *node, const
> char
> > *key,
> > > +                   const char *fmt, va_list ap);
> > > +int xs_node_scanf(struct xs_handle *xsh, const char *node, const char
> > *key,
> > > +                  const char *fmt, ...);
> >
> > Maybe here as well.
> 
> Will do.
> 
>  Paul
> 
> >
> >
> > --
> > Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel