qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 02/10] xen: backend driver core
Date: Tue, 07 Apr 2009 14:41:08 -0500
User-agent: Thunderbird 2.0.0.21 (X11/20090320)

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 <address@hidden>
---
 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 <address@hidden>
+ *
+ *  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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]