[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 04/18] xen: create xenstore areas for XenDevice-
From: |
Paul Durrant |
Subject: |
Re: [Qemu-block] [PATCH 04/18] xen: create xenstore areas for XenDevice-s |
Date: |
Wed, 5 Dec 2018 12:05:23 +0000 |
> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 29 November 2018 18:49
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; xen-
> address@hidden; Stefano Stabellini <address@hidden>;
> Kevin Wolf <address@hidden>; Max Reitz <address@hidden>
> 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.
> > +
> > + 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
- Re: [Qemu-block] [PATCH 04/18] xen: create xenstore areas for XenDevice-s,
Paul Durrant <=