Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c

[Anthony PERARD]

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 <address@hidden>
> Signed-off-by: Emil Condrea <address@hidden>
> ---
>  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 <xen/grant_table.h>
>  
> @@ -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