* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
@ 2016-07-17 7:41 ` Quan Xu
0 siblings, 0 replies; 14+ messages in thread
From: Quan Xu @ 2016-07-17 7:41 UTC (permalink / raw)
To: rea, Stefano Stabellini, anthony.perard
Cc: qemu-devel, stefanb, xen-devel, dgdegra, wei.liu2, eblake, quan.xu2
[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 <quan.xu@xxxxxxxxx>
Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
---
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?
- 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?
- 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. the same comment for the other 'val'
- if (val && 1 == sscanf(val, "%d", ival)) {
- rc = 0;
- }
- g_free(val);
- return rc;
-}
-
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
-{
- char *val;
- int rc = -1;
-
- val = xenstore_read_str(base, node);- if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
- rc = 0;
- }
- g_free(val);
- return rc;
-}
-
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
char *val)
{
return xenstore_write_str(xendev->be, node, val);
@@ -212,20 +141,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev,
const char *node, uint64_t
/* ------------------------------------------------------------- */
-const char *xenbus_strstate(enum xenbus_state state)
-{
- static const char *const name[] = {
- [ XenbusStateUnknown ] = "Unknown",
- [ XenbusStateInitialising ] = "Initialising",
- [ XenbusStateInitWait ] = "InitWait",
- [ XenbusStateInitialised ] = "Initialised",
- [ XenbusStateConnected ] = "Connected",
- [ XenbusStateClosing ] = "Closing",
- [ XenbusStateClosed ] = "Closed",
- };
- return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
-}
-
int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
{
int rc;
@@ -833,44 +748,6 @@ int xen_be_send_notify(struct XenDevice *xendev)
return xenevtchn_notify(xendev->evtchndev, xendev->local_port);
}
-/*
- * msg_level:
- * 0 == errors (stderr + logfile).
- * 1 == informative debug messages (logfile only).
- * 2 == noisy debug messages (logfile only).
- * 3 == will flood your log (logfile only).
- */
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
-{
- va_list args;
-
- if (xendev) {
- if (msg_level > xendev->debug) {
- return;
- }
- qemu_log("xen be: %s: ", xendev->name);
- if (msg_level == 0) {
- fprintf(stderr, "xen be: %s: ", xendev->name);
- }
- } else {
- if (msg_level > debug) {
- return;
- }
- qemu_log("xen be core: ");
- if (msg_level == 0) {
- fprintf(stderr, "xen be core: ");
- }
- }
- va_start(args, fmt);
- qemu_log_vprintf(fmt, args);
- va_end(args);
- if (msg_level == 0) {
- va_start(args, fmt);
- vfprintf(stderr, fmt, args);
- va_end(args);
- }
- qemu_log_flush();
-}
static int xen_sysdev_init(SysBusDevice *dev)
{
diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
new file mode 100644
index 0000000..a444855
--- /dev/null
+++ b/hw/xen/xen_pvdev.c
@@ -0,0 +1,149 @@
+/*
+ * Xen para-virtualization device
+ *
+ * (c) 2008 Gerd Hoffmann <kraxel@xxxxxxxxxx>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/xen/xen_backend.h"
+#include "hw/xen/xen_pvdev.h"
+
+static int debug = 0;
+/* ------------------------------------------------------------- */
+
+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_write_int(const char *base, const char *node, int ival)
+{
+ char val[12];
+
+ 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];
+
+ 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);+ if (val && 1 == sscanf(val, "%d", ival)) {
+ rc = 0;
+ }
+ g_free(val);
+ return rc;
+}
+
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
+{
+ char *val;
+ int rc = -1;
+
+ val = xenstore_read_str(base, node);
+ if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
+ rc = 0;
+ }
+ g_free(val);
+ return rc;
+}
+
+const char *xenbus_strstate(enum xenbus_state state)
+{
+ static const char *const name[] = {
+ [ XenbusStateUnknown ] = "Unknown",
+ [ XenbusStateInitialising ] = "Initialising",
+ [ XenbusStateInitWait ] = "InitWait",
+ [ XenbusStateInitialised ] = "Initialised",
+ [ XenbusStateConnected ] = "Connected",
+ [ XenbusStateClosing ] = "Closing",
+ [ XenbusStateClosed ] = "Closed",
+ };
+ return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
+}
+
+/*
+ * msg_level:
+ * 0 == errors (stderr + logfile).
+ * 1 == informative debug messages (logfile only).
+ * 2 == noisy debug messages (logfile only).
+ * 3 == will flood your log (logfile only).
+ */
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
+{
+ va_list args;
+
+ if (xendev) {
+ if (msg_level > xendev->debug) {
+ return;
+ }
+ qemu_log("xen be: %s: ", xendev->name);
+ if (msg_level == 0) {
+ fprintf(stderr, "xen be: %s: ", xendev->name);
+ }
+ } else {
+ if (msg_level > debug) {
+ return;
+ }
+ qemu_log("xen be core: ");
+ if (msg_level == 0) {
+ fprintf(stderr, "xen be core: ");
+ }
+ }
+ va_start(args, fmt);
+ qemu_log_vprintf(fmt, args);
+ va_end(args);
+ if (msg_level == 0) {
+ va_start(args, fmt);
+ vfprintf(stderr, fmt, args);
+ va_end(args);
+ }
+ qemu_log_flush();
+}
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 6e18a46..0f009e3 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -2,60 +2,10 @@
#define QEMU_HW_XEN_BACKEND_H 1
#include "hw/xen/xen_common.h"
+#include "hw/xen/xen_pvdev.h"
#include "sysemu/sysemu.h"
#include "net/net.h"
-/* ------------------------------------------------------------- */
-
-#define XEN_BUFSIZE 1024
-
-struct XenDevice;
-
-/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
-#define DEVOPS_FLAG_NEED_GNTDEV 1
-/* don't expect frontend doing correct state transitions (aka console quirk) */
-#define DEVOPS_FLAG_IGNORE_STATE 2
-
-struct XenDevOps {
- size_t size;
- uint32_t flags;
- void (*alloc)(struct XenDevice *xendev);
- int (*init)(struct XenDevice *xendev);
- int (*initialise)(struct XenDevice *xendev);
- void (*connected)(struct XenDevice *xendev);
- void (*event)(struct XenDevice *xendev);
- void (*disconnect)(struct XenDevice *xendev);
- int (*free)(struct XenDevice *xendev);
- void (*backend_changed)(struct XenDevice *xendev, const char *node);
- void (*frontend_changed)(struct XenDevice *xendev, const char *node);
- int (*backend_register)(void);
-};
-
-struct XenDevice {
- const char *type;
- int dom;
- int dev;
- char name[64];
- int debug;
-
- enum xenbus_state be_state;
- enum xenbus_state fe_state;
- int online;
- char be[XEN_BUFSIZE];
- char *fe;
- char *protocol;
- int remote_port;
- int local_port;
-
- xenevtchn_handle *evtchndev;
- xengnttab_handle *gnttabdev;
-
- struct XenDevOps *ops;
- QTAILQ_ENTRY(XenDevice) next;
-};
-
-/* ------------------------------------------------------------- */
-
/* variables */
extern xc_interface *xen_xc;
extern xenforeignmemory_handle *xen_fmem;
@@ -63,14 +13,7 @@ extern struct xs_handle *xenstore;
extern const char *xen_protocol;
extern DeviceState *xen_sysdev;
-/* xenstore helper functions */
int xenstore_mkdir(char *path, int p);
-int xenstore_write_str(const char *base, const char *node, const char *val);
-int xenstore_write_int(const char *base, const char *node, int ival);
-int xenstore_write_int64(const char *base, const char *node, int64_t ival);
-char *xenstore_read_str(const char *base, const char *node);
-int xenstore_read_int(const char *base, const char *node, int *ival);
-
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
char *val);
int xenstore_write_be_int(struct XenDevice *xendev, const char *node, int
ival);
int xenstore_write_be_int64(struct XenDevice *xendev, const char *node,
int64_t ival);
@@ -78,10 +21,8 @@ char *xenstore_read_be_str(struct XenDevice *xendev, const
char *node);
int xenstore_read_be_int(struct XenDevice *xendev, const char *node, int
*ival);
char *xenstore_read_fe_str(struct XenDevice *xendev, const char *node);
int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int
*ival);
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node,
uint64_t *uval);
-const char *xenbus_strstate(enum xenbus_state state);
struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev);
void xen_be_check_state(struct XenDevice *xendev);
@@ -92,8 +33,6 @@ int xen_be_set_state(struct XenDevice *xendev, enum
xenbus_state state);
int xen_be_bind_evtchn(struct XenDevice *xendev);
void xen_be_unbind_evtchn(struct XenDevice *xendev);
int xen_be_send_notify(struct XenDevice *xendev);
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
- GCC_FMT_ATTR(3, 4);
/* actual backend drivers */
extern struct XenDevOps xen_console_ops; /* xen_console.c */
diff --git a/include/hw/xen/xen_pvdev.h b/include/hw/xen/xen_pvdev.h
new file mode 100644
index 0000000..f60bfae
--- /dev/null
+++ b/include/hw/xen/xen_pvdev.h
@@ -0,0 +1,71 @@
+#ifndef QEMU_HW_XEN_PVDEV_H
+#define QEMU_HW_XEN_PVDEV_H 1
+
+#include "hw/xen/xen_common.h"
+#include <inttypes.h>
+
+/* ------------------------------------------------------------- */
+
+#define XEN_BUFSIZE 1024
+
+struct XenDevice;
+
+/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
+#define DEVOPS_FLAG_NEED_GNTDEV 1
+/* don't expect frontend doing correct state transitions (aka console quirk) */
+#define DEVOPS_FLAG_IGNORE_STATE 2
+
+struct XenDevOps {
+ size_t size;
+ uint32_t flags;
+ void (*alloc)(struct XenDevice *xendev);
+ int (*init)(struct XenDevice *xendev);
+ int (*initialise)(struct XenDevice *xendev);
+ void (*connected)(struct XenDevice *xendev);
+ void (*event)(struct XenDevice *xendev);
+ void (*disconnect)(struct XenDevice *xendev);
+ int (*free)(struct XenDevice *xendev);
+ void (*backend_changed)(struct XenDevice *xendev, const char *node);
+ void (*frontend_changed)(struct XenDevice *xendev, const char *node);
+ int (*backend_register)(void);
+};
+
+struct XenDevice {
+ const char *type;
+ int dom;
+ int dev;
+ char name[64];
+ int debug;
+
+ enum xenbus_state be_state;
+ enum xenbus_state fe_state;
+ int online;
+ char be[XEN_BUFSIZE];
+ char *fe;
+ char *protocol;
+ int remote_port;
+ int local_port;
+
+ xenevtchn_handle *evtchndev;
+ xengnttab_handle *gnttabdev;
+
+ struct XenDevOps *ops;
+ QTAILQ_ENTRY(XenDevice) next;
+};
+
+/* ------------------------------------------------------------- */
+
+/* xenstore helper functions */
+int xenstore_write_str(const char *base, const char *node, const char *val);
+int xenstore_write_int(const char *base, const char *node, int ival);
+int xenstore_write_int64(const char *base, const char *node, int64_t ival);
+char *xenstore_read_str(const char *base, const char *node);
+int xenstore_read_int(const char *base, const char *node, int *ival);
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
+
+const char *xenbus_strstate(enum xenbus_state state);
+
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
+ GCC_FMT_ATTR(3, 4);
+
+#endif /* QEMU_HW_XEN_PVDEV_H */
--
1.9.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 01/19] xen: Create a new file xen_pvdev.c
@ 2016-07-17 7:41 ` Quan Xu
0 siblings, 0 replies; 14+ messages in thread
From: Quan Xu @ 2016-07-17 7:41 UTC (permalink / raw)
To: anthony.perard
Cc: Stefano Stabellini, wei.liu2, stefanb, quan.xu2, qemu-devel,
xen-devel, dgdegra, eblake, Emil Condrea
[-- Attachment #1.1: Type: text/plain, Size: 20310 bytes --]
[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 <quan.xu@xxxxxxxxx>
Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
---
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?
- 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?
- 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. the same comment for the other 'val'
- if (val && 1 == sscanf(val, "%d", ival)) {
- rc = 0;
- }
- g_free(val);
- return rc;
-}
-
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
-{
- char *val;
- int rc = -1;
-
- val = xenstore_read_str(base, node);- if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
- rc = 0;
- }
- g_free(val);
- return rc;
-}
-
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
char *val)
{
return xenstore_write_str(xendev->be, node, val);
@@ -212,20 +141,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev,
const char *node, uint64_t
/* ------------------------------------------------------------- */
-const char *xenbus_strstate(enum xenbus_state state)
-{
- static const char *const name[] = {
- [ XenbusStateUnknown ] = "Unknown",
- [ XenbusStateInitialising ] = "Initialising",
- [ XenbusStateInitWait ] = "InitWait",
- [ XenbusStateInitialised ] = "Initialised",
- [ XenbusStateConnected ] = "Connected",
- [ XenbusStateClosing ] = "Closing",
- [ XenbusStateClosed ] = "Closed",
- };
- return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
-}
-
int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
{
int rc;
@@ -833,44 +748,6 @@ int xen_be_send_notify(struct XenDevice *xendev)
return xenevtchn_notify(xendev->evtchndev, xendev->local_port);
}
-/*
- * msg_level:
- * 0 == errors (stderr + logfile).
- * 1 == informative debug messages (logfile only).
- * 2 == noisy debug messages (logfile only).
- * 3 == will flood your log (logfile only).
- */
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
-{
- va_list args;
-
- if (xendev) {
- if (msg_level > xendev->debug) {
- return;
- }
- qemu_log("xen be: %s: ", xendev->name);
- if (msg_level == 0) {
- fprintf(stderr, "xen be: %s: ", xendev->name);
- }
- } else {
- if (msg_level > debug) {
- return;
- }
- qemu_log("xen be core: ");
- if (msg_level == 0) {
- fprintf(stderr, "xen be core: ");
- }
- }
- va_start(args, fmt);
- qemu_log_vprintf(fmt, args);
- va_end(args);
- if (msg_level == 0) {
- va_start(args, fmt);
- vfprintf(stderr, fmt, args);
- va_end(args);
- }
- qemu_log_flush();
-}
static int xen_sysdev_init(SysBusDevice *dev)
{
diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
new file mode 100644
index 0000000..a444855
--- /dev/null
+++ b/hw/xen/xen_pvdev.c
@@ -0,0 +1,149 @@
+/*
+ * Xen para-virtualization device
+ *
+ * (c) 2008 Gerd Hoffmann <kraxel@xxxxxxxxxx>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/xen/xen_backend.h"
+#include "hw/xen/xen_pvdev.h"
+
+static int debug = 0;
+/* ------------------------------------------------------------- */
+
+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_write_int(const char *base, const char *node, int ival)
+{
+ char val[12];
+
+ 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];
+
+ 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);+ if (val && 1 == sscanf(val, "%d", ival)) {
+ rc = 0;
+ }
+ g_free(val);
+ return rc;
+}
+
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
+{
+ char *val;
+ int rc = -1;
+
+ val = xenstore_read_str(base, node);
+ if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
+ rc = 0;
+ }
+ g_free(val);
+ return rc;
+}
+
+const char *xenbus_strstate(enum xenbus_state state)
+{
+ static const char *const name[] = {
+ [ XenbusStateUnknown ] = "Unknown",
+ [ XenbusStateInitialising ] = "Initialising",
+ [ XenbusStateInitWait ] = "InitWait",
+ [ XenbusStateInitialised ] = "Initialised",
+ [ XenbusStateConnected ] = "Connected",
+ [ XenbusStateClosing ] = "Closing",
+ [ XenbusStateClosed ] = "Closed",
+ };
+ return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
+}
+
+/*
+ * msg_level:
+ * 0 == errors (stderr + logfile).
+ * 1 == informative debug messages (logfile only).
+ * 2 == noisy debug messages (logfile only).
+ * 3 == will flood your log (logfile only).
+ */
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
+{
+ va_list args;
+
+ if (xendev) {
+ if (msg_level > xendev->debug) {
+ return;
+ }
+ qemu_log("xen be: %s: ", xendev->name);
+ if (msg_level == 0) {
+ fprintf(stderr, "xen be: %s: ", xendev->name);
+ }
+ } else {
+ if (msg_level > debug) {
+ return;
+ }
+ qemu_log("xen be core: ");
+ if (msg_level == 0) {
+ fprintf(stderr, "xen be core: ");
+ }
+ }
+ va_start(args, fmt);
+ qemu_log_vprintf(fmt, args);
+ va_end(args);
+ if (msg_level == 0) {
+ va_start(args, fmt);
+ vfprintf(stderr, fmt, args);
+ va_end(args);
+ }
+ qemu_log_flush();
+}
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 6e18a46..0f009e3 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -2,60 +2,10 @@
#define QEMU_HW_XEN_BACKEND_H 1
#include "hw/xen/xen_common.h"
+#include "hw/xen/xen_pvdev.h"
#include "sysemu/sysemu.h"
#include "net/net.h"
-/* ------------------------------------------------------------- */
-
-#define XEN_BUFSIZE 1024
-
-struct XenDevice;
-
-/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
-#define DEVOPS_FLAG_NEED_GNTDEV 1
-/* don't expect frontend doing correct state transitions (aka console quirk) */
-#define DEVOPS_FLAG_IGNORE_STATE 2
-
-struct XenDevOps {
- size_t size;
- uint32_t flags;
- void (*alloc)(struct XenDevice *xendev);
- int (*init)(struct XenDevice *xendev);
- int (*initialise)(struct XenDevice *xendev);
- void (*connected)(struct XenDevice *xendev);
- void (*event)(struct XenDevice *xendev);
- void (*disconnect)(struct XenDevice *xendev);
- int (*free)(struct XenDevice *xendev);
- void (*backend_changed)(struct XenDevice *xendev, const char *node);
- void (*frontend_changed)(struct XenDevice *xendev, const char *node);
- int (*backend_register)(void);
-};
-
-struct XenDevice {
- const char *type;
- int dom;
- int dev;
- char name[64];
- int debug;
-
- enum xenbus_state be_state;
- enum xenbus_state fe_state;
- int online;
- char be[XEN_BUFSIZE];
- char *fe;
- char *protocol;
- int remote_port;
- int local_port;
-
- xenevtchn_handle *evtchndev;
- xengnttab_handle *gnttabdev;
-
- struct XenDevOps *ops;
- QTAILQ_ENTRY(XenDevice) next;
-};
-
-/* ------------------------------------------------------------- */
-
/* variables */
extern xc_interface *xen_xc;
extern xenforeignmemory_handle *xen_fmem;
@@ -63,14 +13,7 @@ extern struct xs_handle *xenstore;
extern const char *xen_protocol;
extern DeviceState *xen_sysdev;
-/* xenstore helper functions */
int xenstore_mkdir(char *path, int p);
-int xenstore_write_str(const char *base, const char *node, const char *val);
-int xenstore_write_int(const char *base, const char *node, int ival);
-int xenstore_write_int64(const char *base, const char *node, int64_t ival);
-char *xenstore_read_str(const char *base, const char *node);
-int xenstore_read_int(const char *base, const char *node, int *ival);
-
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
char *val);
int xenstore_write_be_int(struct XenDevice *xendev, const char *node, int
ival);
int xenstore_write_be_int64(struct XenDevice *xendev, const char *node,
int64_t ival);
@@ -78,10 +21,8 @@ char *xenstore_read_be_str(struct XenDevice *xendev, const
char *node);
int xenstore_read_be_int(struct XenDevice *xendev, const char *node, int
*ival);
char *xenstore_read_fe_str(struct XenDevice *xendev, const char *node);
int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int
*ival);
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node,
uint64_t *uval);
-const char *xenbus_strstate(enum xenbus_state state);
struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev);
void xen_be_check_state(struct XenDevice *xendev);
@@ -92,8 +33,6 @@ int xen_be_set_state(struct XenDevice *xendev, enum
xenbus_state state);
int xen_be_bind_evtchn(struct XenDevice *xendev);
void xen_be_unbind_evtchn(struct XenDevice *xendev);
int xen_be_send_notify(struct XenDevice *xendev);
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
- GCC_FMT_ATTR(3, 4);
/* actual backend drivers */
extern struct XenDevOps xen_console_ops; /* xen_console.c */
diff --git a/include/hw/xen/xen_pvdev.h b/include/hw/xen/xen_pvdev.h
new file mode 100644
index 0000000..f60bfae
--- /dev/null
+++ b/include/hw/xen/xen_pvdev.h
@@ -0,0 +1,71 @@
+#ifndef QEMU_HW_XEN_PVDEV_H
+#define QEMU_HW_XEN_PVDEV_H 1
+
+#include "hw/xen/xen_common.h"
+#include <inttypes.h>
+
+/* ------------------------------------------------------------- */
+
+#define XEN_BUFSIZE 1024
+
+struct XenDevice;
+
+/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
+#define DEVOPS_FLAG_NEED_GNTDEV 1
+/* don't expect frontend doing correct state transitions (aka console quirk) */
+#define DEVOPS_FLAG_IGNORE_STATE 2
+
+struct XenDevOps {
+ size_t size;
+ uint32_t flags;
+ void (*alloc)(struct XenDevice *xendev);
+ int (*init)(struct XenDevice *xendev);
+ int (*initialise)(struct XenDevice *xendev);
+ void (*connected)(struct XenDevice *xendev);
+ void (*event)(struct XenDevice *xendev);
+ void (*disconnect)(struct XenDevice *xendev);
+ int (*free)(struct XenDevice *xendev);
+ void (*backend_changed)(struct XenDevice *xendev, const char *node);
+ void (*frontend_changed)(struct XenDevice *xendev, const char *node);
+ int (*backend_register)(void);
+};
+
+struct XenDevice {
+ const char *type;
+ int dom;
+ int dev;
+ char name[64];
+ int debug;
+
+ enum xenbus_state be_state;
+ enum xenbus_state fe_state;
+ int online;
+ char be[XEN_BUFSIZE];
+ char *fe;
+ char *protocol;
+ int remote_port;
+ int local_port;
+
+ xenevtchn_handle *evtchndev;
+ xengnttab_handle *gnttabdev;
+
+ struct XenDevOps *ops;
+ QTAILQ_ENTRY(XenDevice) next;
+};
+
+/* ------------------------------------------------------------- */
+
+/* xenstore helper functions */
+int xenstore_write_str(const char *base, const char *node, const char *val);
+int xenstore_write_int(const char *base, const char *node, int ival);
+int xenstore_write_int64(const char *base, const char *node, int64_t ival);
+char *xenstore_read_str(const char *base, const char *node);
+int xenstore_read_int(const char *base, const char *node, int *ival);
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
+
+const char *xenbus_strstate(enum xenbus_state state);
+
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
...)
+ GCC_FMT_ATTR(3, 4);
+
+#endif /* QEMU_HW_XEN_PVDEV_H */
--
1.9.1
[-- Attachment #1.2: Type: text/html, Size: 38012 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-17 7:41 ` Quan Xu
(?)
(?)
@ 2016-07-17 17:02 ` Emil Condrea
-1 siblings, 0 replies; 14+ messages in thread
From: Emil Condrea @ 2016-07-17 17:02 UTC (permalink / raw)
To: Quan Xu
Cc: qemu-devel, eblake, Daniel De Graaf, xen-devel, wei.liu2,
stefanb, sstabellini, anthony.perard
On Jul 17, 2016 10:41, "Quan Xu" <quan.xu2@aliyun.com> wrote:
>
>
> [Quan:]: comment starts with [Quan:]
>
Thanks, Quan for your comments.
The first patches from this series just move some code from xen_backend to
xen_pvdev file. I would not group the reorg from xen_backend with
refactoring in the same patch. Eventually this can be done in another patch
later.
>
>
>
The purpose of the new file is to store generic functions shared by frontend
> and backends such as xenstore operations, xendevs.
>
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
> ---
> 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?
>
> - 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?
>
>
> - 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. the same
comment for the other 'val'
>
> - if (val && 1 == sscanf(val, "%d", ival)) {
> - rc = 0;
> - }
> - g_free(val);
> - return rc;
> -}
> -
>
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> -{
> - char *val;
> - int rc = -1;
> -
> - val = xenstore_read_str(base, node);
> - if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
> - rc = 0;
> - }
> - g_free(val);
> - return rc;
> -}
> -
>
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
> char *val)
> {
> return xenstore_write_str(xendev->be, node, val);
>
@@ -212,20 +141,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev,
> const char *node, uint64_t
>
> /* ------------------------------------------------------------- */
>
> -const char *xenbus_strstate(enum xenbus_state state)
> -{
> - static const char *const name[] = {
> - [ XenbusStateUnknown ] = "Unknown",
> - [ XenbusStateInitialising ] = "Initialising",
> - [ XenbusStateInitWait ] = "InitWait",
> - [ XenbusStateInitialised ] = "Initialised",
> - [ XenbusStateConnected ] = "Connected",
> - [ XenbusStateClosing ] = "Closing",
> - [ XenbusStateClosed ] = "Closed",
> - };
> - return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
> -}
> -
> int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
> {
> int rc;
> @@ -833,44 +748,6 @@ int xen_be_send_notify(struct XenDevice *xendev)
> return xenevtchn_notify(xendev->evtchndev, xendev->local_port);
> }
>
> -/*
> - * msg_level:
> - * 0 == errors (stderr + logfile).
> - * 1 == informative debug messages (logfile only).
> - * 2 == noisy debug messages (logfile only).
> - * 3 == will flood your log (logfile only).
> - */
>
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> -{
> - va_list args;
> -
> - if (xendev) {
> - if (msg_level > xendev->debug) {
> - return;
> - }
> - qemu_log("xen be: %s: ", xendev->name);
> - if (msg_level == 0) {
> - fprintf(stderr, "xen be: %s: ", xendev->name);
> - }
> - } else {
> - if (msg_level > debug) {
> - return;
> - }
> - qemu_log("xen be core: ");
> - if (msg_level == 0) {
> - fprintf(stderr, "xen be core: ");
> - }
> - }
> - va_start(args, fmt);
> - qemu_log_vprintf(fmt, args);
> - va_end(args);
> - if (msg_level == 0) {
> - va_start(args, fmt);
> - vfprintf(stderr, fmt, args);
> - va_end(args);
> - }
> - qemu_log_flush();
> -}
>
> static int xen_sysdev_init(SysBusDevice *dev)
> {
> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> new file mode 100644
> index 0000000..a444855
> --- /dev/null
> +++ b/hw/xen/xen_pvdev.c
> @@ -0,0 +1,149 @@
> +/*
> + * Xen para-virtualization device
> + *
> + * (c) 2008 Gerd Hoffmann <kraxel@xxxxxxxxxx>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <
http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/xen/xen_backend.h"
> +#include "hw/xen/xen_pvdev.h"
> +
> +static int debug = 0;
> +/* ------------------------------------------------------------- */
> +
>
+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_write_int(const char *base, const char *node, int ival)
> +{
> + char val[12];
> +
> + 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];
> +
> + 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);
> + if (val && 1 == sscanf(val, "%d", ival)) {
> + rc = 0;
> + }
> + g_free(val);
> + return rc;
> +}
> +
>
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> +{
> + char *val;
> + int rc = -1;
> +
> + val = xenstore_read_str(base, node);
> + if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
> + rc = 0;
> + }
> + g_free(val);
> + return rc;
> +}
> +
> +const char *xenbus_strstate(enum xenbus_state state)
> +{
> + static const char *const name[] = {
> + [ XenbusStateUnknown ] = "Unknown",
> + [ XenbusStateInitialising ] = "Initialising",
> + [ XenbusStateInitWait ] = "InitWait",
> + [ XenbusStateInitialised ] = "Initialised",
> + [ XenbusStateConnected ] = "Connected",
> + [ XenbusStateClosing ] = "Closing",
> + [ XenbusStateClosed ] = "Closed",
> + };
> + return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
> +}
> +
> +/*
> + * msg_level:
> + * 0 == errors (stderr + logfile).
> + * 1 == informative debug messages (logfile only).
> + * 2 == noisy debug messages (logfile only).
> + * 3 == will flood your log (logfile only).
> + */
>
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> +{
> + va_list args;
> +
> + if (xendev) {
> + if (msg_level > xendev->debug) {
> + return;
> + }
> + qemu_log("xen be: %s: ", xendev->name);
> + if (msg_level == 0) {
> + fprintf(stderr, "xen be: %s: ", xendev->name);
> + }
> + } else {
> + if (msg_level > debug) {
> + return;
> + }
> + qemu_log("xen be core: ");
> + if (msg_level == 0) {
> + fprintf(stderr, "xen be core: ");
> + }
> + }
> + va_start(args, fmt);
> + qemu_log_vprintf(fmt, args);
> + va_end(args);
> + if (msg_level == 0) {
> + va_start(args, fmt);
> + vfprintf(stderr, fmt, args);
> + va_end(args);
> + }
> + qemu_log_flush();
> +}
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index 6e18a46..0f009e3 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -2,60 +2,10 @@
> #define QEMU_HW_XEN_BACKEND_H 1
>
> #include "hw/xen/xen_common.h"
> +#include "hw/xen/xen_pvdev.h"
> #include "sysemu/sysemu.h"
> #include "net/net.h"
>
> -/* ------------------------------------------------------------- */
> -
> -#define XEN_BUFSIZE 1024
> -
> -struct XenDevice;
> -
>
-/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
> -#define DEVOPS_FLAG_NEED_GNTDEV 1
>
-/* don't expect frontend doing correct state transitions (aka console quirk) */
> -#define DEVOPS_FLAG_IGNORE_STATE 2
> -
> -struct XenDevOps {
> - size_t size;
> - uint32_t flags;
> - void (*alloc)(struct XenDevice *xendev);
> - int (*init)(struct XenDevice *xendev);
> - int (*initialise)(struct XenDevice *xendev);
> - void (*connected)(struct XenDevice *xendev);
> - void (*event)(struct XenDevice *xendev);
> - void (*disconnect)(struct XenDevice *xendev);
> - int (*free)(struct XenDevice *xendev);
>
- void (*backend_changed)(struct XenDevice *xendev, const char *node);
>
- void (*frontend_changed)(struct XenDevice *xendev, const char *node);
> - int (*backend_register)(void);
> -};
> -
> -struct XenDevice {
> - const char *type;
> - int dom;
> - int dev;
> - char name[64];
> - int debug;
> -
> - enum xenbus_state be_state;
> - enum xenbus_state fe_state;
> - int online;
> - char be[XEN_BUFSIZE];
> - char *fe;
> - char *protocol;
> - int remote_port;
> - int local_port;
> -
> - xenevtchn_handle *evtchndev;
> - xengnttab_handle *gnttabdev;
> -
> - struct XenDevOps *ops;
> - QTAILQ_ENTRY(XenDevice) next;
> -};
> -
> -/* ------------------------------------------------------------- */
> -
> /* variables */
> extern xc_interface *xen_xc;
> extern xenforeignmemory_handle *xen_fmem;
> @@ -63,14 +13,7 @@ extern struct xs_handle *xenstore;
> extern const char *xen_protocol;
> extern DeviceState *xen_sysdev;
>
> -/* xenstore helper functions */
> int xenstore_mkdir(char *path, int p);
>
-int xenstore_write_str(const char *base, const char *node, const char *val);
> -int xenstore_write_int(const char *base, const char *node, int ival);
>
-int xenstore_write_int64(const char *base, const char *node, int64_t ival);
> -char *xenstore_read_str(const char *base, const char *node);
> -int xenstore_read_int(const char *base, const char *node, int *ival);
> -
>
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
> char *val);
>
int xenstore_write_be_int(struct XenDevice *xendev, const char *node, int
> ival);
> int xenstore_write_be_int64(struct XenDevice *xendev, const char *node,
> int64_t ival);
>
@@ -78,10 +21,8 @@ char *xenstore_read_be_str(struct XenDevice *xendev, const
> char *node);
> int xenstore_read_be_int(struct XenDevice *xendev, const char *node, int
> *ival);
> char *xenstore_read_fe_str(struct XenDevice *xendev, const char *node);
> int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int
> *ival);
>
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
> int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node,
> uint64_t *uval);
>
> -const char *xenbus_strstate(enum xenbus_state state);
> struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev);
> void xen_be_check_state(struct XenDevice *xendev);
>
> @@ -92,8 +33,6 @@ int xen_be_set_state(struct XenDevice *xendev, enum
> xenbus_state state);
> int xen_be_bind_evtchn(struct XenDevice *xendev);
> void xen_be_unbind_evtchn(struct XenDevice *xendev);
> int xen_be_send_notify(struct XenDevice *xendev);
>
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> - GCC_FMT_ATTR(3, 4);
>
> /* actual backend drivers */
> extern struct XenDevOps xen_console_ops; /* xen_console.c */
> diff --git a/include/hw/xen/xen_pvdev.h b/include/hw/xen/xen_pvdev.h
> new file mode 100644
> index 0000000..f60bfae
> --- /dev/null
> +++ b/include/hw/xen/xen_pvdev.h
> @@ -0,0 +1,71 @@
> +#ifndef QEMU_HW_XEN_PVDEV_H
> +#define QEMU_HW_XEN_PVDEV_H 1
> +
> +#include "hw/xen/xen_common.h"
> +#include <inttypes.h>
> +
> +/* ------------------------------------------------------------- */
> +
> +#define XEN_BUFSIZE 1024
> +
> +struct XenDevice;
> +
>
+/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
> +#define DEVOPS_FLAG_NEED_GNTDEV 1
>
+/* don't expect frontend doing correct state transitions (aka console quirk) */
> +#define DEVOPS_FLAG_IGNORE_STATE 2
> +
> +struct XenDevOps {
> + size_t size;
> + uint32_t flags;
> + void (*alloc)(struct XenDevice *xendev);
> + int (*init)(struct XenDevice *xendev);
> + int (*initialise)(struct XenDevice *xendev);
> + void (*connected)(struct XenDevice *xendev);
> + void (*event)(struct XenDevice *xendev);
> + void (*disconnect)(struct XenDevice *xendev);
> + int (*free)(struct XenDevice *xendev);
>
+ void (*backend_changed)(struct XenDevice *xendev, const char *node);
>
+ void (*frontend_changed)(struct XenDevice *xendev, const char *node);
> + int (*backend_register)(void);
> +};
> +
> +struct XenDevice {
> + const char *type;
> + int dom;
> + int dev;
> + char name[64];
> + int debug;
> +
> + enum xenbus_state be_state;
> + enum xenbus_state fe_state;
> + int online;
> + char be[XEN_BUFSIZE];
> + char *fe;
> + char *protocol;
> + int remote_port;
> + int local_port;
> +
> + xenevtchn_handle *evtchndev;
> + xengnttab_handle *gnttabdev;
> +
> + struct XenDevOps *ops;
> + QTAILQ_ENTRY(XenDevice) next;
> +};
> +
> +/* ------------------------------------------------------------- */
> +
> +/* xenstore helper functions */
>
+int xenstore_write_str(const char *base, const char *node, const char *val);
> +int xenstore_write_int(const char *base, const char *node, int ival);
>
+int xenstore_write_int64(const char *base, const char *node, int64_t ival);
> +char *xenstore_read_str(const char *base, const char *node);
> +int xenstore_read_int(const char *base, const char *node, int *ival);
>
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
> +
> +const char *xenbus_strstate(enum xenbus_state state);
> +
>
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> + GCC_FMT_ATTR(3, 4);
> +
> +#endif /* QEMU_HW_XEN_PVDEV_H */
> --
> 1.9.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-17 7:41 ` Quan Xu
(?)
@ 2016-07-17 17:02 ` Emil Condrea
-1 siblings, 0 replies; 14+ messages in thread
From: Emil Condrea @ 2016-07-17 17:02 UTC (permalink / raw)
To: Quan Xu
Cc: sstabellini, wei.liu2, stefanb, qemu-devel, xen-devel,
anthony.perard, Daniel De Graaf, eblake
[-- Attachment #1.1: Type: text/plain, Size: 18531 bytes --]
On Jul 17, 2016 10:41, "Quan Xu" <quan.xu2@aliyun.com> wrote:
>
>
> [Quan:]: comment starts with [Quan:]
>
Thanks, Quan for your comments.
The first patches from this series just move some code from xen_backend to
xen_pvdev file. I would not group the reorg from xen_backend with
refactoring in the same patch. Eventually this can be done in another patch
later.
>
>
>
The purpose of the new file is to store generic functions shared by frontend
> and backends such as xenstore operations, xendevs.
>
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
> ---
> 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?
>
> - 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?
>
>
> - 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. the same
comment for the other 'val'
>
> - if (val && 1 == sscanf(val, "%d", ival)) {
> - rc = 0;
> - }
> - g_free(val);
> - return rc;
> -}
> -
>
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> -{
> - char *val;
> - int rc = -1;
> -
> - val = xenstore_read_str(base, node);
> - if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
> - rc = 0;
> - }
> - g_free(val);
> - return rc;
> -}
> -
>
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
> char *val)
> {
> return xenstore_write_str(xendev->be, node, val);
>
@@ -212,20 +141,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev,
> const char *node, uint64_t
>
> /* ------------------------------------------------------------- */
>
> -const char *xenbus_strstate(enum xenbus_state state)
> -{
> - static const char *const name[] = {
> - [ XenbusStateUnknown ] = "Unknown",
> - [ XenbusStateInitialising ] = "Initialising",
> - [ XenbusStateInitWait ] = "InitWait",
> - [ XenbusStateInitialised ] = "Initialised",
> - [ XenbusStateConnected ] = "Connected",
> - [ XenbusStateClosing ] = "Closing",
> - [ XenbusStateClosed ] = "Closed",
> - };
> - return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
> -}
> -
> int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
> {
> int rc;
> @@ -833,44 +748,6 @@ int xen_be_send_notify(struct XenDevice *xendev)
> return xenevtchn_notify(xendev->evtchndev, xendev->local_port);
> }
>
> -/*
> - * msg_level:
> - * 0 == errors (stderr + logfile).
> - * 1 == informative debug messages (logfile only).
> - * 2 == noisy debug messages (logfile only).
> - * 3 == will flood your log (logfile only).
> - */
>
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> -{
> - va_list args;
> -
> - if (xendev) {
> - if (msg_level > xendev->debug) {
> - return;
> - }
> - qemu_log("xen be: %s: ", xendev->name);
> - if (msg_level == 0) {
> - fprintf(stderr, "xen be: %s: ", xendev->name);
> - }
> - } else {
> - if (msg_level > debug) {
> - return;
> - }
> - qemu_log("xen be core: ");
> - if (msg_level == 0) {
> - fprintf(stderr, "xen be core: ");
> - }
> - }
> - va_start(args, fmt);
> - qemu_log_vprintf(fmt, args);
> - va_end(args);
> - if (msg_level == 0) {
> - va_start(args, fmt);
> - vfprintf(stderr, fmt, args);
> - va_end(args);
> - }
> - qemu_log_flush();
> -}
>
> static int xen_sysdev_init(SysBusDevice *dev)
> {
> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> new file mode 100644
> index 0000000..a444855
> --- /dev/null
> +++ b/hw/xen/xen_pvdev.c
> @@ -0,0 +1,149 @@
> +/*
> + * Xen para-virtualization device
> + *
> + * (c) 2008 Gerd Hoffmann <kraxel@xxxxxxxxxx>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library 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
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <
http://www.gnu.org/licenses/>
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/xen/xen_backend.h"
> +#include "hw/xen/xen_pvdev.h"
> +
> +static int debug = 0;
> +/* ------------------------------------------------------------- */
> +
>
+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_write_int(const char *base, const char *node, int ival)
> +{
> + char val[12];
> +
> + 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];
> +
> + 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);
> + if (val && 1 == sscanf(val, "%d", ival)) {
> + rc = 0;
> + }
> + g_free(val);
> + return rc;
> +}
> +
>
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> +{
> + char *val;
> + int rc = -1;
> +
> + val = xenstore_read_str(base, node);
> + if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
> + rc = 0;
> + }
> + g_free(val);
> + return rc;
> +}
> +
> +const char *xenbus_strstate(enum xenbus_state state)
> +{
> + static const char *const name[] = {
> + [ XenbusStateUnknown ] = "Unknown",
> + [ XenbusStateInitialising ] = "Initialising",
> + [ XenbusStateInitWait ] = "InitWait",
> + [ XenbusStateInitialised ] = "Initialised",
> + [ XenbusStateConnected ] = "Connected",
> + [ XenbusStateClosing ] = "Closing",
> + [ XenbusStateClosed ] = "Closed",
> + };
> + return (state < ARRAY_SIZE(name)) ? name[state] : "INVALID";
> +}
> +
> +/*
> + * msg_level:
> + * 0 == errors (stderr + logfile).
> + * 1 == informative debug messages (logfile only).
> + * 2 == noisy debug messages (logfile only).
> + * 3 == will flood your log (logfile only).
> + */
>
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> +{
> + va_list args;
> +
> + if (xendev) {
> + if (msg_level > xendev->debug) {
> + return;
> + }
> + qemu_log("xen be: %s: ", xendev->name);
> + if (msg_level == 0) {
> + fprintf(stderr, "xen be: %s: ", xendev->name);
> + }
> + } else {
> + if (msg_level > debug) {
> + return;
> + }
> + qemu_log("xen be core: ");
> + if (msg_level == 0) {
> + fprintf(stderr, "xen be core: ");
> + }
> + }
> + va_start(args, fmt);
> + qemu_log_vprintf(fmt, args);
> + va_end(args);
> + if (msg_level == 0) {
> + va_start(args, fmt);
> + vfprintf(stderr, fmt, args);
> + va_end(args);
> + }
> + qemu_log_flush();
> +}
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index 6e18a46..0f009e3 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -2,60 +2,10 @@
> #define QEMU_HW_XEN_BACKEND_H 1
>
> #include "hw/xen/xen_common.h"
> +#include "hw/xen/xen_pvdev.h"
> #include "sysemu/sysemu.h"
> #include "net/net.h"
>
> -/* ------------------------------------------------------------- */
> -
> -#define XEN_BUFSIZE 1024
> -
> -struct XenDevice;
> -
>
-/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
> -#define DEVOPS_FLAG_NEED_GNTDEV 1
>
-/* don't expect frontend doing correct state transitions (aka console quirk) */
> -#define DEVOPS_FLAG_IGNORE_STATE 2
> -
> -struct XenDevOps {
> - size_t size;
> - uint32_t flags;
> - void (*alloc)(struct XenDevice *xendev);
> - int (*init)(struct XenDevice *xendev);
> - int (*initialise)(struct XenDevice *xendev);
> - void (*connected)(struct XenDevice *xendev);
> - void (*event)(struct XenDevice *xendev);
> - void (*disconnect)(struct XenDevice *xendev);
> - int (*free)(struct XenDevice *xendev);
>
- void (*backend_changed)(struct XenDevice *xendev, const char *node);
>
- void (*frontend_changed)(struct XenDevice *xendev, const char *node);
> - int (*backend_register)(void);
> -};
> -
> -struct XenDevice {
> - const char *type;
> - int dom;
> - int dev;
> - char name[64];
> - int debug;
> -
> - enum xenbus_state be_state;
> - enum xenbus_state fe_state;
> - int online;
> - char be[XEN_BUFSIZE];
> - char *fe;
> - char *protocol;
> - int remote_port;
> - int local_port;
> -
> - xenevtchn_handle *evtchndev;
> - xengnttab_handle *gnttabdev;
> -
> - struct XenDevOps *ops;
> - QTAILQ_ENTRY(XenDevice) next;
> -};
> -
> -/* ------------------------------------------------------------- */
> -
> /* variables */
> extern xc_interface *xen_xc;
> extern xenforeignmemory_handle *xen_fmem;
> @@ -63,14 +13,7 @@ extern struct xs_handle *xenstore;
> extern const char *xen_protocol;
> extern DeviceState *xen_sysdev;
>
> -/* xenstore helper functions */
> int xenstore_mkdir(char *path, int p);
>
-int xenstore_write_str(const char *base, const char *node, const char *val);
> -int xenstore_write_int(const char *base, const char *node, int ival);
>
-int xenstore_write_int64(const char *base, const char *node, int64_t ival);
> -char *xenstore_read_str(const char *base, const char *node);
> -int xenstore_read_int(const char *base, const char *node, int *ival);
> -
>
int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const
> char *val);
>
int xenstore_write_be_int(struct XenDevice *xendev, const char *node, int
> ival);
> int xenstore_write_be_int64(struct XenDevice *xendev, const char *node,
> int64_t ival);
>
@@ -78,10 +21,8 @@ char *xenstore_read_be_str(struct XenDevice *xendev, const
> char *node);
> int xenstore_read_be_int(struct XenDevice *xendev, const char *node, int
> *ival);
> char *xenstore_read_fe_str(struct XenDevice *xendev, const char *node);
> int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int
> *ival);
>
-int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
> int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node,
> uint64_t *uval);
>
> -const char *xenbus_strstate(enum xenbus_state state);
> struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev);
> void xen_be_check_state(struct XenDevice *xendev);
>
> @@ -92,8 +33,6 @@ int xen_be_set_state(struct XenDevice *xendev, enum
> xenbus_state state);
> int xen_be_bind_evtchn(struct XenDevice *xendev);
> void xen_be_unbind_evtchn(struct XenDevice *xendev);
> int xen_be_send_notify(struct XenDevice *xendev);
>
-void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> - GCC_FMT_ATTR(3, 4);
>
> /* actual backend drivers */
> extern struct XenDevOps xen_console_ops; /* xen_console.c */
> diff --git a/include/hw/xen/xen_pvdev.h b/include/hw/xen/xen_pvdev.h
> new file mode 100644
> index 0000000..f60bfae
> --- /dev/null
> +++ b/include/hw/xen/xen_pvdev.h
> @@ -0,0 +1,71 @@
> +#ifndef QEMU_HW_XEN_PVDEV_H
> +#define QEMU_HW_XEN_PVDEV_H 1
> +
> +#include "hw/xen/xen_common.h"
> +#include <inttypes.h>
> +
> +/* ------------------------------------------------------------- */
> +
> +#define XEN_BUFSIZE 1024
> +
> +struct XenDevice;
> +
>
+/* driver uses grant tables -> open gntdev device (xendev->gnttabdev) */
> +#define DEVOPS_FLAG_NEED_GNTDEV 1
>
+/* don't expect frontend doing correct state transitions (aka console quirk) */
> +#define DEVOPS_FLAG_IGNORE_STATE 2
> +
> +struct XenDevOps {
> + size_t size;
> + uint32_t flags;
> + void (*alloc)(struct XenDevice *xendev);
> + int (*init)(struct XenDevice *xendev);
> + int (*initialise)(struct XenDevice *xendev);
> + void (*connected)(struct XenDevice *xendev);
> + void (*event)(struct XenDevice *xendev);
> + void (*disconnect)(struct XenDevice *xendev);
> + int (*free)(struct XenDevice *xendev);
>
+ void (*backend_changed)(struct XenDevice *xendev, const char *node);
>
+ void (*frontend_changed)(struct XenDevice *xendev, const char *node);
> + int (*backend_register)(void);
> +};
> +
> +struct XenDevice {
> + const char *type;
> + int dom;
> + int dev;
> + char name[64];
> + int debug;
> +
> + enum xenbus_state be_state;
> + enum xenbus_state fe_state;
> + int online;
> + char be[XEN_BUFSIZE];
> + char *fe;
> + char *protocol;
> + int remote_port;
> + int local_port;
> +
> + xenevtchn_handle *evtchndev;
> + xengnttab_handle *gnttabdev;
> +
> + struct XenDevOps *ops;
> + QTAILQ_ENTRY(XenDevice) next;
> +};
> +
> +/* ------------------------------------------------------------- */
> +
> +/* xenstore helper functions */
>
+int xenstore_write_str(const char *base, const char *node, const char *val);
> +int xenstore_write_int(const char *base, const char *node, int ival);
>
+int xenstore_write_int64(const char *base, const char *node, int64_t ival);
> +char *xenstore_read_str(const char *base, const char *node);
> +int xenstore_read_int(const char *base, const char *node, int *ival);
>
+int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
> +
> +const char *xenbus_strstate(enum xenbus_state state);
> +
>
+void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt,
> ...)
> + GCC_FMT_ATTR(3, 4);
> +
> +#endif /* QEMU_HW_XEN_PVDEV_H */
> --
> 1.9.1
[-- Attachment #1.2: Type: text/html, Size: 27042 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-17 7:41 ` Quan Xu
` (3 preceding siblings ...)
(?)
@ 2016-07-18 14:50 ` Anthony PERARD
-1 siblings, 0 replies; 14+ messages in thread
From: Anthony PERARD @ 2016-07-18 14:50 UTC (permalink / raw)
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 <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
> ---
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-17 7:41 ` Quan Xu
` (2 preceding siblings ...)
(?)
@ 2016-07-18 14:50 ` Anthony PERARD
-1 siblings, 0 replies; 14+ messages in thread
From: Anthony PERARD @ 2016-07-18 14:50 UTC (permalink / raw)
To: Quan Xu
Cc: Stefano Stabellini, wei.liu2, stefanb, qemu-devel, xen-devel,
dgdegra, eblake, rea
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 <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
> ---
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-17 7:41 ` Quan Xu
@ 2016-07-18 14:57 ` Eric Blake
-1 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2016-07-18 14:57 UTC (permalink / raw)
To: Quan Xu, rea, Stefano Stabellini, anthony.perard
Cc: qemu-devel, stefanb, xen-devel, dgdegra, wei.liu2
[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]
On 07/17/2016 01:41 AM, Quan Xu wrote:
>
> [Quan:]: comment starts with [Quan:]
>
This line doesn't belong in a commit message; it's fine to put it after
the --- separator though, if it aids mailing list reviewers.
>
> The purpose of the new file is to store generic functions shared by frontendand backends such as xenstore operations, xendevs.
>
s/frontendand/front end and/
Please wrap your commit message lines. Since 'git log' displays logs
with indentation, wrapping around 72 characters is ideal.
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
These are not valid S-o-b, therefore this patch cannot be applied as-is.
> -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. the same comment for the other 'val'
> - if (val && 1 == sscanf(val, "%d", ival)) {
This is not a valid patch. Are you replying to a patch that someone
else posted? If so, your quoting style is VERY difficult to read.
Please consider using a leading > before every line that you are quoting
(rather than pasting it verbatim as if you had written it), and include
a blank line both before and after every line that you insert, to call
visual attention to what is your reply vs. what you are quoting.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 01/19] xen: Create a new file xen_pvdev.c
@ 2016-07-18 14:57 ` Eric Blake
0 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2016-07-18 14:57 UTC (permalink / raw)
To: Quan Xu, rea, Stefano Stabellini, anthony.perard
Cc: dgdegra, xen-devel, wei.liu2, qemu-devel, stefanb
[-- Attachment #1.1.1: Type: text/plain, Size: 1571 bytes --]
On 07/17/2016 01:41 AM, Quan Xu wrote:
>
> [Quan:]: comment starts with [Quan:]
>
This line doesn't belong in a commit message; it's fine to put it after
the --- separator though, if it aids mailing list reviewers.
>
> The purpose of the new file is to store generic functions shared by frontendand backends such as xenstore operations, xendevs.
>
s/frontendand/front end and/
Please wrap your commit message lines. Since 'git log' displays logs
with indentation, wrapping around 72 characters is ideal.
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
These are not valid S-o-b, therefore this patch cannot be applied as-is.
> -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. the same comment for the other 'val'
> - if (val && 1 == sscanf(val, "%d", ival)) {
This is not a valid patch. Are you replying to a patch that someone
else posted? If so, your quoting style is VERY difficult to read.
Please consider using a leading > before every line that you are quoting
(rather than pasting it verbatim as if you had written it), and include
a blank line both before and after every line that you insert, to call
visual attention to what is your reply vs. what you are quoting.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-18 14:57 ` Eric Blake
(?)
(?)
@ 2016-07-18 16:54 ` Emil Condrea
-1 siblings, 0 replies; 14+ messages in thread
From: Emil Condrea @ 2016-07-18 16:54 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, Daniel De Graaf, xen-devel, Stefano Stabellini,
Quan Xu, wei.liu2, stefanb, anthony.perard
Eric, this is the link to the original patch which is well formatted:
http://marc.info/?l=xen-devel&m=146815138831762&w=2
I think that the formatting and s-o-b was broken in the reply from Quan.
On Jul 18, 2016 17:57, "Eric Blake" <eblake@redhat.com> wrote:
> On 07/17/2016 01:41 AM, Quan Xu wrote:
> >
> > [Quan:]: comment starts with [Quan:]
> >
>
> This line doesn't belong in a commit message; it's fine to put it after
> the --- separator though, if it aids mailing list reviewers.
>
> >
> > The purpose of the new file is to store generic functions shared by
> frontendand backends such as xenstore operations, xendevs.
> >
>
> s/frontendand/front end and/
>
> Please wrap your commit message lines. Since 'git log' displays logs
> with indentation, wrapping around 72 characters is ideal.
>
> > Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> > Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
>
> These are not valid S-o-b, therefore this patch cannot be applied as-is.
>
>
> > -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. the same
> comment for the other 'val'
> > - if (val && 1 == sscanf(val, "%d", ival)) {
>
> This is not a valid patch. Are you replying to a patch that someone
> else posted? If so, your quoting style is VERY difficult to read.
> Please consider using a leading > before every line that you are quoting
> (rather than pasting it verbatim as if you had written it), and include
> a blank line both before and after every line that you insert, to call
> visual attention to what is your reply vs. what you are quoting.
>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-18 14:57 ` Eric Blake
(?)
@ 2016-07-18 16:54 ` Emil Condrea
-1 siblings, 0 replies; 14+ messages in thread
From: Emil Condrea @ 2016-07-18 16:54 UTC (permalink / raw)
To: Eric Blake
Cc: Stefano Stabellini, wei.liu2, stefanb, qemu-devel, xen-devel,
anthony.perard, Quan Xu, Daniel De Graaf
[-- Attachment #1.1: Type: text/plain, Size: 1864 bytes --]
Eric, this is the link to the original patch which is well formatted:
http://marc.info/?l=xen-devel&m=146815138831762&w=2
I think that the formatting and s-o-b was broken in the reply from Quan.
On Jul 18, 2016 17:57, "Eric Blake" <eblake@redhat.com> wrote:
> On 07/17/2016 01:41 AM, Quan Xu wrote:
> >
> > [Quan:]: comment starts with [Quan:]
> >
>
> This line doesn't belong in a commit message; it's fine to put it after
> the --- separator though, if it aids mailing list reviewers.
>
> >
> > The purpose of the new file is to store generic functions shared by
> frontendand backends such as xenstore operations, xendevs.
> >
>
> s/frontendand/front end and/
>
> Please wrap your commit message lines. Since 'git log' displays logs
> with indentation, wrapping around 72 characters is ideal.
>
> > Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> > Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
>
> These are not valid S-o-b, therefore this patch cannot be applied as-is.
>
>
> > -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. the same
> comment for the other 'val'
> > - if (val && 1 == sscanf(val, "%d", ival)) {
>
> This is not a valid patch. Are you replying to a patch that someone
> else posted? If so, your quoting style is VERY difficult to read.
> Please consider using a leading > before every line that you are quoting
> (rather than pasting it verbatim as if you had written it), and include
> a blank line both before and after every line that you insert, to call
> visual attention to what is your reply vs. what you are quoting.
>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
[-- Attachment #1.2: Type: text/html, Size: 2587 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-18 14:57 ` Eric Blake
@ 2016-07-22 14:00 ` Quan Xu
-1 siblings, 0 replies; 14+ messages in thread
From: Quan Xu @ 2016-07-22 14:00 UTC (permalink / raw)
To: Eric Blake, Emil Condrea
Cc: qemu-devel, Daniel De Graaf, xen-devel, Stefano Stabellini,
wei.liu2, stefanb, anthony.perard
sorry for the bad format from web email, and later review (neo training in new company).. patch 6 -- patch 12, rename * patch, are good to me.
Quan
------------------------------------------------------------------From:Emil Condrea <emilcondrea@gmail.com>Time:2016 Jul 19 (Tue) 00:54To:Eric Blake <eblake@redhat.com>Cc:qemu-devel <qemu-devel@nongnu.org>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; xen-devel <xen-devel@lists.xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Quan Xu <quan.xu2@aliyun.com>; wei.liu2 <wei.liu2@citrix.com>; stefanb <stefanb@linux.vnet.ibm.com>; anthony.perard <anthony.perard@citrix.com>Subject:Re: [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
Eric, this is the link to the original patch which is well formatted: http://marc.info/?l=xen-devel&m=146815138831762&w=2I think that the formatting and s-o-b was broken in the reply from Quan.On Jul 18, 2016 17:57, "Eric Blake" <eblake@redhat.com> wrote:
On 07/17/2016 01:41 AM, Quan Xu wrote:
>
> [Quan:]: comment starts with [Quan:]
>
This line doesn't belong in a commit message; it's fine to put it after
the --- separator though, if it aids mailing list reviewers.
>
> The purpose of the new file is to store generic functions shared by frontendand backends such as xenstore operations, xendevs.
>
s/frontendand/front end and/
Please wrap your commit message lines. Since 'git log' displays logs
with indentation, wrapping around 72 characters is ideal.
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
These are not valid S-o-b, therefore this patch cannot be applied as-is.
> -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. the same comment for the other 'val'
> - if (val && 1 == sscanf(val, "%d", ival)) {
This is not a valid patch. Are you replying to a patch that someone
else posted? If so, your quoting style is VERY difficult to read.
Please consider using a leading > before every line that you are quoting
(rather than pasting it verbatim as if you had written it), and include
a blank line both before and after every line that you insert, to call
visual attention to what is your reply vs. what you are quoting.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 01/19] xen: Create a new file xen_pvdev.c
@ 2016-07-22 14:00 ` Quan Xu
0 siblings, 0 replies; 14+ messages in thread
From: Quan Xu @ 2016-07-22 14:00 UTC (permalink / raw)
To: Eric Blake, Emil Condrea
Cc: Stefano Stabellini, wei.liu2, stefanb, qemu-devel, xen-devel,
anthony.perard, Daniel De Graaf
[-- Attachment #1.1: Type: text/plain, Size: 2542 bytes --]
sorry for the bad format from web email, and later review (neo training in new company).. patch 6 -- patch 12, rename * patch, are good to me.
Quan
------------------------------------------------------------------From:Emil Condrea <emilcondrea@gmail.com>Time:2016 Jul 19 (Tue) 00:54To:Eric Blake <eblake@redhat.com>Cc:qemu-devel <qemu-devel@nongnu.org>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; xen-devel <xen-devel@lists.xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Quan Xu <quan.xu2@aliyun.com>; wei.liu2 <wei.liu2@citrix.com>; stefanb <stefanb@linux.vnet.ibm.com>; anthony.perard <anthony.perard@citrix.com>Subject:Re: [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
Eric, this is the link to the original patch which is well formatted: http://marc.info/?l=xen-devel&m=146815138831762&w=2I think that the formatting and s-o-b was broken in the reply from Quan.On Jul 18, 2016 17:57, "Eric Blake" <eblake@redhat.com> wrote:
On 07/17/2016 01:41 AM, Quan Xu wrote:
>
> [Quan:]: comment starts with [Quan:]
>
This line doesn't belong in a commit message; it's fine to put it after
the --- separator though, if it aids mailing list reviewers.
>
> The purpose of the new file is to store generic functions shared by frontendand backends such as xenstore operations, xendevs.
>
s/frontendand/front end and/
Please wrap your commit message lines. Since 'git log' displays logs
with indentation, wrapping around 72 characters is ideal.
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
These are not valid S-o-b, therefore this patch cannot be applied as-is.
> -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. the same comment for the other 'val'
> - if (val && 1 == sscanf(val, "%d", ival)) {
This is not a valid patch. Are you replying to a patch that someone
else posted? If so, your quoting style is VERY difficult to read.
Please consider using a leading > before every line that you are quoting
(rather than pasting it verbatim as if you had written it), and include
a blank line both before and after every line that you insert, to call
visual attention to what is your reply vs. what you are quoting.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #1.2: Type: text/html, Size: 5129 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
2016-07-22 14:24 [Qemu-devel] [Xen-devel] " Quan Xu
@ 2016-07-22 14:28 ` Emil Condrea
0 siblings, 0 replies; 14+ messages in thread
From: Emil Condrea @ 2016-07-22 14:28 UTC (permalink / raw)
To: Quan Xu
Cc: Daniel De Graaf, qemu-devel, xen-devel, Stefano Stabellini,
Eric Blake, stefanb, wei.liu2, anthony.perard
Sure, I will continue to send revisions until it is approved upstream.
On Jul 22, 2016 5:24 PM, "Quan Xu" <quan.xu2@aliyun.com> wrote:
> Anthony, thanks for your explaination.
> IMO, patch 1 and patch 2 need your detailed review.. IMO the reset
> patches are good in general..
> Emil, if patch 1 / patch 2 are reviewed from anthony, could you send out
> v10? :) i know it's not an easy task, thanks in advence!!
>
> Quan
>
>
> On Mon, 18 Jul 2016 15:50:27 +0100, anthony.perard<
> anthony.perard@citrix.com> wrote:
> > On Sun, Jul 17, 2016 at 03:41:26PM +0800, Quan Xu wrote:
> > -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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c
@ 2016-07-22 14:24 Quan Xu
2016-07-22 14:28 ` Emil Condrea
0 siblings, 1 reply; 14+ messages in thread
From: Quan Xu @ 2016-07-22 14:24 UTC (permalink / raw)
To: anthony.perard, Emil Condrea
Cc: qemu-devel, Daniel De Graaf, xen-devel, Stefano Stabellini,
wei.liu2, stefanb, Eric Blake
Anthony, thanks for your explaination.IMO, patch 1 and patch 2 need your detailed review.. IMO the reset patches are good in general..Emil, if patch 1 / patch 2 are reviewed from anthony, could you send out v10? :) i know it's not an easy task, thanks in advence!!
Quan
On Mon, 18 Jul 2016 15:50:27 +0100, anthony.perard<anthony.perard@citrix.com> wrote:> On Sun, Jul 17, 2016 at 03:41:26PM +0800, Quan Xu wrote:
> -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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-07-22 14:29 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-17 7:41 [Qemu-devel] [Xen-devel] [PATCH 01/19] xen: Create a new file xen_pvdev.c Quan Xu
2016-07-17 7:41 ` Quan Xu
2016-07-17 17:02 ` Emil Condrea
2016-07-17 17:02 ` [Qemu-devel] [Xen-devel] " Emil Condrea
2016-07-18 14:50 ` Anthony PERARD
2016-07-18 14:50 ` [Qemu-devel] [Xen-devel] " Anthony PERARD
2016-07-18 14:57 ` Eric Blake
2016-07-18 14:57 ` Eric Blake
2016-07-18 16:54 ` Emil Condrea
2016-07-18 16:54 ` [Qemu-devel] [Xen-devel] " Emil Condrea
2016-07-22 14:00 ` Quan Xu
2016-07-22 14:00 ` Quan Xu
2016-07-22 14:24 [Qemu-devel] [Xen-devel] " Quan Xu
2016-07-22 14:28 ` Emil Condrea
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.