Re: [Qemu-devel] [PATCH 02/10] xen: backend driver core

[Anthony Liguori <aliguori@us.ibm.com>]

Gerd Hoffmann wrote:
> This patch adds infrastructure for xen backend drivers living in qemu,
> so drivers don't need to implement common stuff on their own.  It's
> mostly xenbus management stuff: some functions to access xentore,
> setting up xenstore watches, callbacks on device discovery and state
> changes, handle event channel, ...
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.target     |    2 +-
>  hw/xen_backend.c    |  691 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/xen_backend.h    |   86 +++++++
>  hw/xen_common.h     |   34 +++
>  hw/xen_machine_pv.c |    8 +-
>  5 files changed, 819 insertions(+), 2 deletions(-)
>  create mode 100644 hw/xen_backend.c
>  create mode 100644 hw/xen_backend.h
>  create mode 100644 hw/xen_common.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 1cad75e..5d9dfc7 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -558,7 +558,7 @@ LIBS += $(CONFIG_BLUEZ_LIBS)
>  endif
>  
>  # xen backend driver support
> -XEN_OBJS := xen_machine_pv.o
> +XEN_OBJS := xen_machine_pv.o xen_backend.o
>  ifeq ($(CONFIG_XEN), yes)
>    OBJS += $(XEN_OBJS)
>    LIBS += $(XEN_LIBS)
> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
> new file mode 100644
> index 0000000..b13c3f9
> --- /dev/null
> +++ b/hw/xen_backend.c
> @@ -0,0 +1,691 @@
> +/*
> + *  xen backend driver infrastructure
> + *  (c) 2008 Gerd Hoffmann <kraxel@redhat.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; under version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +/*
> + * TODO: add some xenbus / xenstore concepts overview here.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/signal.h>
> +
> +#include <xs.h>
> +#include <xenctrl.h>
> +#include <xen/grant_table.h>
> +
> +#include "hw.h"
> +#include "qemu-char.h"
> +#include "xen_backend.h"
> +
> +/* ------------------------------------------------------------- */
> +
> +/* public */
> +int xen_xc;
> +struct xs_handle *xenstore = NULL;
> +
> +/* private */
> +static TAILQ_HEAD(XenDeviceHead, XenDevice) xendevs = TAILQ_HEAD_INITIALIZER(xendevs);
> +static int debug = 0;
>   

Would be better to have all of this in a structure that had a single 
static instance.  Would be even better if you could avoid requiring the 
static instance.

> +/* ------------------------------------------------------------- */
> +
> +int xenstore_write_str(const char *base, const char *node, const char *val)
> +{
> +    char abspath[XEN_BUFSIZE];
> +
> +    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
> +    if (!xs_write(xenstore, 0, abspath, val, strlen(val)))
> +	return -1;
> +    return 0;
> +}
> +
> +char *xenstore_read_str(const char *base, const char *node)
> +{
> +    char abspath[XEN_BUFSIZE];
> +    unsigned int len;
> +
> +    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
> +    return xs_read(xenstore, 0, abspath, &len);
>   

xs_read() is a xenstore API, it's returning malloc()'d memory.

> +}
> +
> +int xenstore_write_int(const char *base, const char *node, int ival)
> +{
> +    char val[32];
> +
> +    snprintf(val, sizeof(val), "%d", ival);
> +    return xenstore_write_str(base, node, val);
> +}
> +
> +int xenstore_read_int(const char *base, const char *node, int *ival)
> +{
> +    char *val;
> +    int rc = -1;
> +
> +    val = xenstore_read_str(base, node);
> +    if (val && 1 == sscanf(val, "%d", ival))
> +	rc = 0;
> +    qemu_free(val);
>   

And here you're free()'ing with qemu_free.

> +/*
> + * get xen backend device, allocate a new one if it doesn't exist.
> + */
> +static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
> +                                           struct XenDevOps *ops)
> +{
> +    struct XenDevice *xendev;
> +    char *dom0;
> +
> +    xendev = xen_be_find_xendev(type, dom, dev);
> +    if (xendev)
> +	return xendev;
> +
> +    /* init new xendev */
> +    xendev = qemu_mallocz(ops->size);
> +    if (!xendev)
> +        return NULL;
>   

No need to check malloc failures.

> +
> +    /* look for backends */
> +    dev = xs_directory(xenstore, 0, path, &cdev);
> +    if (!dev)
> +	return 0;
> +    for (j = 0; j < cdev; j++) {
> +	xendev = xen_be_get_xendev(type, dom, atoi(dev[j]), ops);
> +	if (xendev == NULL)
> +	    continue;
> +	xen_be_check_state(xendev);
> +    }
> +    qemu_free(dev);
>   

Mixing qemu_free() with malloc'd memory.

> +static void xenstore_update_be(char *watch, char *type, int dom,
> +			       struct XenDevOps *ops)
> +{
> +    struct XenDevice *xendev;
> +    char path[XEN_BUFSIZE], *dom0;
> +    unsigned int len, dev;
> +
> +    dom0 = xs_get_domain_path(xenstore, 0);
> +    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
> +    free(dom0);
> +    if (0 != strncmp(path, watch, len))
> +	return;
> +    if (2 != sscanf(watch+len, "/%u/%255s", &dev, path)) {
> +	strcpy(path, "");
> +	if (1 != sscanf(watch+len, "/%u", &dev))
> +	    dev = -1;
> +    }
>   
Overly defensive ifs.  You also have open coded calls to fprintf(stderr) 
whereas you've introduced a higher level function.

Regards,

Anthony Liguori