From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58123) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bP9sT-00026c-Tj for qemu-devel@nongnu.org; Mon, 18 Jul 2016 10:50:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bP9sP-0003FH-JE for qemu-devel@nongnu.org; Mon, 18 Jul 2016 10:50:36 -0400 Received: from smtp.citrix.com ([66.165.176.89]:13940) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bP9sP-0003EM-0u for qemu-devel@nongnu.org; Mon, 18 Jul 2016 10:50:33 -0400 Date: Mon, 18 Jul 2016 15:50:27 +0100 From: Anthony PERARD Message-ID: <20160718145027.GC1835@perard.uk.xensource.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Quan Xu Cc: rea , Stefano Stabellini , qemu-devel , stefanb , xen-devel , dgdegra , "wei.liu2" , eblake On Sun, Jul 17, 2016 at 03:41:26PM +0800, Quan Xu wrote: > > [Quan:]: comment starts with [Quan:] > > > The purpose of the new file is to store generic functions shared by frontendand backends such as xenstore operations, xendevs. > > Signed-off-by: Quan Xu  > Signed-off-by: Emil Condrea  > --- >  hw/xen/Makefile.objs         |   2 +- >  hw/xen/xen_backend.c         | 125 +----------------------------------- >  hw/xen/xen_pvdev.c           | 149 +++++++++++++++++++++++++++++++++++++++++++ >  include/hw/xen/xen_backend.h |  63 +----------------- >  include/hw/xen/xen_pvdev.h   |  71 +++++++++++++++++++++ >  5 files changed, 223 insertions(+), 187 deletions(-) >  create mode 100644 hw/xen/xen_pvdev.c >  create mode 100644 include/hw/xen/xen_pvdev.h > > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs > index d367094..591cdc2 100644 > --- a/hw/xen/Makefile.objs > +++ b/hw/xen/Makefile.objs > @@ -1,5 +1,5 @@ >  # xen backend driver support > -common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o > +common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o xen_pvdev.o >   >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o >  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o  > xen_pt_graphics.o xen_pt_msi.o > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c > index bab79b1..a251a4a 100644 > --- a/hw/xen/xen_backend.c > +++ b/hw/xen/xen_backend.c > @@ -30,6 +30,7 @@ >  #include "sysemu/char.h" >  #include "qemu/log.h" >  #include "hw/xen/xen_backend.h" > +#include "hw/xen/xen_pvdev.h" >   >  #include  >   > @@ -56,8 +57,6 @@ static QTAILQ_HEAD(xs_dirs_head, xs_dirs) xs_cleanup = >  static QTAILQ_HEAD(XenDeviceHead, XenDevice) xendevs =  > QTAILQ_HEAD_INITIALIZER(xendevs); >  static int debug = 0; >   > -/* ------------------------------------------------------------- */ > - >  static void xenstore_cleanup_dir(char *dir) >  { >      struct xs_dirs *d; > @@ -76,34 +75,6 @@ void xen_config_cleanup(void) >      } >  } >   > -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; > -    char *str, *ret = NULL; > - > -    snprintf(abspath, sizeof(abspath), "%s/%s", base, node); > -    str = xs_read(xenstore, 0, abspath, &len); > -    if (str != NULL) { > -        /* move to qemu-allocated memory to make sure > -         * callers can savely g_free() stuff. */ > -        ret = g_strdup(str); > -        free(str); > -    } > -    return ret; > -} > - >  int xenstore_mkdir(char *path, int p) >  { >      struct xs_permissions perms[2] = { > @@ -128,48 +99,6 @@ int xenstore_mkdir(char *path, int p) >      return 0; >  } >   > -int xenstore_write_int(const char *base, const char *node, int ival) > -{ > -    char val[12]; > - > [Quan:]: why 12 ? what about XEN_BUFSIZE?  That is the number of digit in INT_MAX (10) + 1 for the sign + 1 for '\0'. > -    snprintf(val, sizeof(val), "%d", ival); > -    return xenstore_write_str(base, node, val); > -} > - > -int xenstore_write_int64(const char *base, const char *node, int64_t ival) > -{ > -    char val[21]; > - > [Quan:]: why 21 ? what about XEN_BUFSIZE? Same with INT64_MAX (19 digits). > > -    snprintf(val, sizeof(val), "%"PRId64, 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); > [Quan:]:  IMO, it is better to initialize val when declares. I think I prefer it this way. > -    if (val && 1 == sscanf(val, "%d", ival)) { > -        rc = 0; > -    } > -    g_free(val); > -    return rc; > -} -- Anthony PERARD