From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LrHBs-00046Q-CZ for qemu-devel@nongnu.org; Tue, 07 Apr 2009 15:42:36 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LrHBm-000466-Ul for qemu-devel@nongnu.org; Tue, 07 Apr 2009 15:42:36 -0400 Received: from [199.232.76.173] (port=48384 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LrHBm-000463-S3 for qemu-devel@nongnu.org; Tue, 07 Apr 2009 15:42:30 -0400 Received: from e4.ny.us.ibm.com ([32.97.182.144]:37889) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LrHBm-0006za-Dc for qemu-devel@nongnu.org; Tue, 07 Apr 2009 15:42:30 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e4.ny.us.ibm.com (8.13.1/8.13.1) with ESMTP id n37Jd74s024635 for ; Tue, 7 Apr 2009 15:39:07 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n37JfBPX191086 for ; Tue, 7 Apr 2009 15:41:11 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n37JdXko029836 for ; Tue, 7 Apr 2009 15:39:33 -0400 Message-ID: <49DBAC54.7010500@us.ibm.com> Date: Tue, 07 Apr 2009 14:41:08 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 02/10] xen: backend driver core References: <1239115497-1884-1-git-send-email-kraxel@redhat.com> <1239115497-1884-3-git-send-email-kraxel@redhat.com> In-Reply-To: <1239115497-1884-3-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: xen-devel@lists.xensource.com, Gerd Hoffmann 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 > --- > 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#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