[Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Wenchao Xia]

  These patch added a seperated layer to take internal or external snapshots
in a unified way, the granularity is block device, so other functions can
just combine the request and submit, such as group snapshot, savevm.

Total goal are:
  Live back up vm in external or internal image, which need three functions:
1 live snapshot block device internal/external.
2 live save vmstate internal/external.
3 combination of the function unit.

  This serial provide part one.

v2:
  1) seperate patches for bdrv_deappend() and bdrv_snapshot_find().
  2) use error set function which will keep first error instead of replacing a
existing error.
  3) use qemu_timespec for snapshot timestamp, added a lock in localtime_r() on
windows and use it for timestamp.
  4) remove space in comments and use present tense.
  5) remove member async in BlkTransactionStates and async related structure,
which is not supported now.
  6) remove snapshot delete function.
  7) remove snapshot info function.
  8) redesign qmp interface, new interface is added instead of using parameter
to distinguish internal case in old external interface.
  9) renamed parameter in hmp interface, and better tips for it.
  10) patches are splitted to more smaller ones.
  11) code adjust according to comments.

Wenchao Xia (10):
  block: export function bdrv_find_snapshot()
  block: add function deappend()
  error: add function error_set_check()
  oslib-win32: add lock for time functions
  snapshot: design of internal common API to take snapshots
  snapshot: implemention of internal common API to take snapshots
  snapshot: qmp use new internal API for external snapshot transaction
  snapshot: qmp add internal snapshot transaction interface
  snapshot: qmp add blockdev-snapshot-internal-sync interface
  snapshot: hmp add internal snapshot support for block device

 block.c                   |   44 +++
 blockdev.c                |  753 +++++++++++++++++++++++++++++++++++++--------
 error.c                   |   19 ++
 hmp-commands.hx           |   28 +-
 hmp.c                     |   25 +-
 include/block/block.h     |    3 +
 include/qapi/error.h      |    5 +
 include/sysemu/blockdev.h |  119 +++++++
 oslib-win32.c             |   12 +-
 qapi-schema.json          |   50 +++-
 qmp-commands.hx           |   52 +++-
 savevm.c                  |   22 --
 12 files changed, 954 insertions(+), 178 deletions(-)

[Qemu-devel] [PATCH V2 01/10] block: export function bdrv_find_snapshot()

[Wenchao Xia]

  This function is moved from savevm.c to block.c, and exported.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |   23 +++++++++++++++++++++++
 include/block/block.h |    2 ++
 savevm.c              |   22 ----------------------
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 4e28c55..09208c2 100644
--- a/block.c
+++ b/block.c
@@ -3210,6 +3210,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
+
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/include/block/block.h b/include/block/block.h
index 0719339..a49fd71 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -330,6 +330,8 @@ int bdrv_snapshot_list(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/savevm.c b/savevm.c
index 529d60e..f16a920 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2036,28 +2036,6 @@ out:
     return ret;
 }
 
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
-{
-    QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
-
-    ret = -ENOENT;
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */
-- 
1.7.1

[Qemu-devel] [PATCH V2 02/10] block: add function deappend()

[Wenchao Xia]

  This function should revert the append operation.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |   21 +++++++++++++++++++++
 include/block/block.h |    1 +
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 09208c2..48ddf64 100644
--- a/block.c
+++ b/block.c
@@ -1376,6 +1376,27 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
             bs_new->drv ? bs_new->drv->format_name : "");
 }
 
+/*
+ * Remove bs contents which is at the top of an image chain while
+ * the chain is live. After bdrv_append(bs_new, bs_top) called,
+ * bs_top keeps as the top of the chain but bs_new become the 2nd
+ * top one, so call bdrv_deappend(bs_new, bs_top) to cancel the change.
+ * As a result, bs_top still keeps the top position but bs_new is
+ * is discarded.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_old and bs_top. Both bs_old and bs_top are modified.
+ *
+ * bs_old and bs_top should be the pairs which have been used in
+ * bdrv_append().
+ *
+ * This function does not delete any image files.
+ */
+void bdrv_deappend(BlockDriverState *bs_old, BlockDriverState *bs_top)
+{
+    bdrv_swap(bs_old, bs_top);
+}
+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
diff --git a/include/block/block.h b/include/block/block.h
index a49fd71..ae6a5ae 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -130,6 +130,7 @@ BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
+void bdrv_deappend(BlockDriverState *bs_old, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
-- 
1.7.1

[Qemu-devel] [PATCH V2 03/10] error: add function error_set_check()

[Wenchao Xia]

  This function will return instead of abort when *errp is not NULL.
Also macro error_setg_check is added.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 error.c              |   19 +++++++++++++++++++
 include/qapi/error.h |    5 +++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/error.c b/error.c
index 519f6b6..e4ec67c 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,25 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     *errp = err;
 }
 
+void error_set_check(Error **errp, ErrorClass err_class, const char *fmt, ...)
+{
+    Error *err;
+    va_list ap;
+
+    if ((errp == NULL) || (*errp != NULL)) {
+        return;
+    }
+
+    err = g_malloc0(sizeof(*err));
+
+    va_start(ap, fmt);
+    err->msg = g_strdup_vprintf(fmt, ap);
+    va_end(ap);
+    err->err_class = err_class;
+
+    *errp = err;
+}
+
 void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                      const char *fmt, ...)
 {
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 5cd2f0c..04b87a9 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -29,6 +29,9 @@ typedef struct Error Error;
  */
 void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 
+void error_set_check(Error **errp, ErrorClass err_class, const char *fmt, ...)
+GCC_FMT_ATTR(3, 4);
+
 /**
  * Set an indirect pointer to an error given a ErrorClass value and a
  * printf-style human message, followed by a strerror() string if
@@ -41,6 +44,8 @@ void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char
  */
 #define error_setg(err, fmt, ...) \
     error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+#define error_setg_check(err, fmt, ...) \
+    error_set_check(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 #define error_setg_errno(err, os_error, fmt, ...) \
     error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 
-- 
1.7.1

[Qemu-devel] [PATCH V2 04/10] oslib-win32: add lock for time functions

[Wenchao Xia]

  This patch adding lock for calling gmtime() and localtime()
on windows. If no other lib linked into qemu would call those
two function itself, then they are thread safe now on windows.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 oslib-win32.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/oslib-win32.c b/oslib-win32.c
index e7e283e..344e3dd 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -74,27 +74,35 @@ void qemu_vfree(void *ptr)
     VirtualFree(ptr, 0, MEM_RELEASE);
 }
 
-/* FIXME: add proper locking */
+/* WARN: if other lib call gmtime() itself, then it is not thread safe. */
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
+    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
+
+    g_static_mutex_lock(&lock);
     struct tm *p = gmtime(timep);
     memset(result, 0, sizeof(*result));
     if (p) {
         *result = *p;
         p = result;
     }
+    g_static_mutex_unlock(&lock);
     return p;
 }
 
-/* FIXME: add proper locking */
+/* WARN: if other lib call localtime() itself, then it is not thread safe. */
 struct tm *localtime_r(const time_t *timep, struct tm *result)
 {
+    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
+
+    g_static_mutex_lock(&lock);
     struct tm *p = localtime(timep);
     memset(result, 0, sizeof(*result));
     if (p) {
         *result = *p;
         p = result;
     }
+    g_static_mutex_unlock(&lock);
     return p;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V2 04/10] oslib-win32: add lock for time functions

[Wenchao Xia]

  This patch adding lock for calling gmtime() and localtime()
on windows. If no other lib linked into qemu would call those
two function itself, then they are thread safe now on windows.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 oslib-win32.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/oslib-win32.c b/oslib-win32.c
index e7e283e..344e3dd 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -74,27 +74,35 @@ void qemu_vfree(void *ptr)
     VirtualFree(ptr, 0, MEM_RELEASE);
 }
 
-/* FIXME: add proper locking */
+/* WARN: if other lib call gmtime() itself, then it is not thread safe. */
 struct tm *gmtime_r(const time_t *timep, struct tm *result)
 {
+    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
+
+    g_static_mutex_lock(&lock);
     struct tm *p = gmtime(timep);
     memset(result, 0, sizeof(*result));
     if (p) {
         *result = *p;
         p = result;
     }
+    g_static_mutex_unlock(&lock);
     return p;
 }
 
-/* FIXME: add proper locking */
+/* WARN: if other lib call localtime() itself, then it is not thread safe. */
 struct tm *localtime_r(const time_t *timep, struct tm *result)
 {
+    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
+
+    g_static_mutex_lock(&lock);
     struct tm *p = localtime(timep);
     memset(result, 0, sizeof(*result));
     if (p) {
         *result = *p;
         p = result;
     }
+    g_static_mutex_unlock(&lock);
     return p;
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V2 05/10] snapshot: design of internal common API to take snapshots

[Wenchao Xia]

  This patch added API to take snapshots in unified style for
both internal or external type. The core structure is based
on transaction, for that there is a qmp interface need to support
, qmp_transaction, so all operations are packed as requests.
  In this way a sperate internal layer for snapshot is splitted
out from qmp layer, and now qmp can just translate the user request
and fill in internal API. Internal API use params defined inside
qemu, so other component inside qemu can use it without considering
the qmp's parameter format.

v2:
  Renamed BlkTransaction* to BlkTrans* to make name shorter.
  Use present tense in comments.
  Deleted async snapshot related member.
  Use qemu_timeval in SNTime.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 include/sysemu/blockdev.h |  119 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 119 insertions(+), 0 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 1fe5332..4d61485 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -66,4 +66,123 @@ void qmp_change_blockdev(const char *device, const char *filename,
                          bool has_format, const char *format, Error **errp);
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
+
+/* snapshot transaction API.
+ * Split out a layer around core struct BlkTransStates, so other
+ * component in qemu can fill the request and simply use the API to submit,
+ * QMP may just use part of the API's function, no need to expose all internal
+ * function to user.
+ */
+
+/* sync snapshot */
+
+typedef enum BlkTransOperationSync {
+    BLK_SN_SYNC_CREATE = 0,
+    BLK_SN_SYNC_DELETE,
+} BlkTransOperationSync;
+
+/* internal snapshot */
+
+typedef struct SNTime {
+    qemu_timeval tv;
+    uint64_t vm_clock_nsec; /* VM clock relative to boot */
+} SNTime;
+
+typedef enum BlkSnapshotIntStep {
+    BLK_SNAPSHOT_INT_START = 0,
+    BLK_SNAPSHOT_INT_CREATED,
+    BLK_SNAPSHOT_INT_CANCELED,
+} BlkSnapshotIntStep;
+
+typedef struct BlkSnapshotInternal {
+    /* caller input */
+    const char *sn_name; /* must be set in create/delete. */
+    BlockDriverState *bs; /* must be set in create/delete */
+    SNTime time; /* must be set in create. */
+    uint64_t vm_state_size; /* optional, default is 0, only valid in create. */
+    /* following are used internal */
+    QEMUSnapshotInfo sn;
+    QEMUSnapshotInfo old_sn;
+    bool old_sn_exist;
+    BlkSnapshotIntStep step;
+} BlkSnapshotInternal;
+
+/* external snapshot */
+
+typedef enum BlkSnapshotExtStep {
+    BLK_SNAPSHOT_EXT_START = 0,
+    BLK_SNAPSHOT_EXT_CREATED,
+    BLK_SNAPSHOT_EXT_INVALIDATED,
+    BLK_SNAPSHOT_EXT_CANCELED,
+} BlkSnapshotExtStep;
+
+typedef struct BlkSnapshotExternal {
+    /* caller input */
+    const char *new_image_file; /* must be set in create/delete. */
+    BlockDriverState *old_bs; /* must be set in create/delete. */
+    const char *format; /* must be set in create. */
+    /* following are used internal */
+    BlockDriverState *new_bs;
+    BlockDriver *format_drv;
+    BlkSnapshotExtStep step;
+} BlkSnapshotExternal;
+
+typedef enum BlkSnapshotType {
+    BLK_SNAPSHOT_INTERNAL = 0,
+    BLK_SNAPSHOT_EXTERNAL,
+    BLK_SNAPSHOT_NOSUPPORT,
+} BlkSnapshotType;
+
+/* for simple sync type params are all put here ignoring the difference of
+   different operation type as create/delete. */
+typedef struct BlkTransStatesSync {
+    /* caller input */
+    BlkSnapshotType type;
+    union {
+        BlkSnapshotInternal internal;
+        BlkSnapshotExternal external;
+    };
+    bool use_existing;
+    BlkTransOperationSync op;
+} BlkTransStatesSync;
+
+/* Core structure for group snapshots, fill in it and then call the API. */
+typedef struct BlkTransStates BlkTransStates;
+
+struct BlkTransStates {
+    /* caller input */
+    BlkTransStatesSync st_sync;
+    /* following are used internal */
+    Error *err;
+    int (*blk_trans_do)(BlkTransStates *states, Error **errp);
+    int (*blk_trans_invalid)(BlkTransStates *states, Error **errp);
+    int (*blk_trans_cancel)(BlkTransStates *states, Error **errp);
+    QSIMPLEQ_ENTRY(BlkTransStates) entry;
+};
+
+typedef QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransStates) \
+                      BlkTransStatesList;
+
+/* API */
+BlkTransStates *blk_trans_st_new(void);
+void blk_trans_st_delete(BlkTransStates **p_st);
+BlkTransStatesList *blk_trans_st_list_new(void);
+void blk_trans_st_list_delete(BlkTransStatesList **p_list);
+
+/* add a request to list, request would be checked to see if it is valid,
+   return -1 when met error and states would not be queued. */
+int add_transaction(BlkTransStatesList *list,
+                    BlkTransStates *states,
+                    Error **errp);
+
+/* 'Atomic' submit the request. In snapshot creation case, if any
+ * fail then we do not pivot any of the devices in the group, and abandon the
+ * snapshots
+ */
+int submit_transaction(BlkTransStatesList *list, Error **errp);
+
+/* helper */
+SNTime get_sn_time(void);
+void generate_sn_name_from_time(SNTime *time, char *time_str, int size);
 #endif
-- 
1.7.1

[Qemu-devel] [PATCH V2 06/10] snapshot: implemention of internal common API to take snapshots

[Wenchao Xia]

  This patch is the implemention of transaction based snapshot
internal API. Internal snapshot for specified block device
is added, which can be used as part of functionality in one way
to live full back up vm seperating internal block snapshot and
vmstate(vmstate goes to another file, not implemented in this
patch).
  Every request's handler need to have three function:
execution, updating, canceling. Also another check function
could be provided to check if request is valid before execition.
  internal snapshot implemention was based on the code from
dietmar@proxmox.com.

v2:
  Remove space in comments.
  Use qemu_timeval and localtime_r on both Linux and Windows.
  Use true instread of TRUE for bool flag.
  Factor out error dealing code in submit_transaction.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 blockdev.c |  484 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 484 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d724e2d..668506d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -660,6 +660,490 @@ void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
+
+/* Snapshot functions.
+ * Following are implemention around core struct BlkTransStates.
+ * to start, invalidate, cancel the action.
+ */
+
+/* Block internal snapshot(qcow2, sheepdog image...) support.
+ * Checking and execution was splitted to enable a check for every device
+ * before execution in group case.
+ */
+
+SNTime get_sn_time(void)
+{
+    SNTime time;
+    qemu_gettimeofday(&time.tv);
+    time.vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+    return time;
+}
+
+void generate_sn_name_from_time(SNTime *time, char *time_str, int size)
+{
+    time_t t = time->tv.tv_sec;
+    struct tm tm;
+    localtime_r(&t, &tm);
+    strftime(time_str, size, "vm-%Y%m%d%H%M%S", &tm);
+}
+
+static int blk_sn_check_create_internal_sync(BlkTransStates *states,
+                                             Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *device = bdrv_get_device_name(bs);
+    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
+    const char *name = st_sync->internal.sn_name;
+    bool use_existing = st_sync->use_existing;
+    int ret;
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set_check(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return -1;
+    }
+
+    if (bdrv_is_read_only(bs)) {
+        error_set_check(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return -1;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set_check(errp, QERR_NOT_SUPPORTED);
+        return -1;
+    }
+
+    ret = bdrv_snapshot_find(bs, old_sn, name);
+    if (ret >= 0) {
+        st_sync->internal.old_sn_exist = true;
+    } else {
+        if (use_existing) {
+            /* caller require use existing one */
+            error_set_check(errp, ERROR_CLASS_GENERIC_ERROR,
+                            "snapshot '%s' not exists on '%s' but caller "
+                            "want to use it.",
+                            name, device);
+            return -1;
+        }
+    }
+
+    st_sync->internal.step = BLK_SNAPSHOT_INT_START;
+    return 0;
+}
+
+static int blk_sn_create_internal_sync(BlkTransStates *states,
+                                       Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *name = st_sync->internal.sn_name;
+    QEMUSnapshotInfo *sn = &st_sync->internal.sn;
+    int ret = 0;
+    const char *device = bdrv_get_device_name(bs);
+    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
+
+    SNTime *time = &st_sync->internal.time;
+    if (time->vm_clock_nsec == 0) {
+        /* not preset, it need to be set */
+        error_setg_check(errp,
+                         "Invalid timestamp was set on for '%s' on '%s'\n",
+                         name, device);
+        return -1;
+    }
+
+    sn->date_sec = time->tv.tv_sec;
+    sn->date_nsec = time->tv.tv_usec * 1000;
+    sn->vm_clock_nsec = time->vm_clock_nsec;
+    sn->vm_state_size = st_sync->internal.vm_state_size;
+
+    if (st_sync->internal.old_sn_exist) {
+        ret = bdrv_snapshot_delete(bs, old_sn->id_str);
+        if (ret < 0) {
+            error_setg_check(errp,
+                       "Error in deleting existing snapshot %s on '%s'\n",
+                       name, device);
+            return -1;
+        }
+        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
+        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+    } else {
+        pstrcpy(sn->name, sizeof(sn->name), name);
+    }
+
+    ret = bdrv_snapshot_create(bs, sn);
+    if (ret < 0) {
+        error_set_check(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "Error while creating snapshot on '%s'\n", device);
+        return -1;
+    }
+
+    st_sync->internal.step = BLK_SNAPSHOT_INT_CREATED;
+    return 0;
+}
+
+static int blk_sn_cancel_internal_sync(BlkTransStates *states,
+                                       Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    const char *name = st_sync->internal.sn_name;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *device = bdrv_get_device_name(bs);
+    int ret;
+
+    if (st_sync->internal.step == BLK_SNAPSHOT_INT_CREATED) {
+        if (st_sync->internal.old_sn_exist) {
+            error_setg_check(errp,
+                       "sn '%s' on '%s' exist originally at it have been "
+                       "overwritten, can't cancel the action.\n",
+                       name, device);
+            return -1;
+        }
+        ret = bdrv_snapshot_delete(bs, name);
+        if (ret < 0) {
+            error_setg_check(errp,
+                       "Error while deleting snapshot '%s' on '%s' to "
+                       "cancel the operation.\n", name, device);
+            return -1;
+        }
+        st_sync->internal.step = BLK_SNAPSHOT_INT_CANCELED;
+    }
+    return 0;
+}
+
+static int blk_sn_check_delete_internal_sync(BlkTransStates *states,
+                                             Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *bs = st_sync->internal.bs;
+    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
+    const char *device = bdrv_get_device_name(bs);
+    const char *name = st_sync->internal.sn_name;
+    int ret;
+
+    if (bdrv_is_read_only(bs)) {
+        error_set_check(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return -1;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set_check(errp, QERR_NOT_SUPPORTED);
+        return -1;
+    }
+
+    ret = bdrv_snapshot_find(bs, old_sn, name);
+    if (ret < 0) {
+        /* caller require use existing one */
+        error_set_check(errp, ERROR_CLASS_GENERIC_ERROR,
+                        "snapshot '%s' not exists on '%s'.",
+                        name, device);
+        return -1;
+    }
+    return 0;
+}
+
+static int blk_sn_delete_internal_sync(BlkTransStates *states,
+                                       Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    int ret = 0;
+    const char *name = st_sync->internal.sn_name;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *device = bdrv_get_device_name(bs);
+
+    /* no need to check snapshot existence? */
+    ret = bdrv_snapshot_delete(bs, name);
+    if (ret < 0) {
+        error_setg_check(errp,
+                  "Error while deleting snapshot on '%s'\n", device);
+        return -1;
+    }
+    return 0;
+}
+
+
+/* Block external snapshot(backing file chain) support. */
+
+static int blk_sn_check_create_external_sync(BlkTransStates *states,
+                                             Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *old_bs = st_sync->external.old_bs;
+    const char *device = bdrv_get_device_name(old_bs);
+    const char *format = st_sync->external.format;
+    const char *new_image_file = st_sync->external.new_image_file;
+    BlockDriver *proto_drv;
+    BlockDriver *drv;
+
+    if (!bdrv_is_inserted(old_bs)) {
+        error_set_check(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return -1;
+    }
+
+    if (bdrv_in_use(old_bs)) {
+        error_set_check(errp, QERR_DEVICE_IN_USE, device);
+        return -1;
+    }
+
+    if (!bdrv_is_read_only(old_bs)) {
+        if (bdrv_flush(old_bs)) {
+            error_set_check(errp, QERR_IO_ERROR);
+            return -1;
+        }
+    }
+
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        error_set_check(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return -1;
+    }
+    st_sync->external.format_drv = drv;
+
+    proto_drv = bdrv_find_protocol(new_image_file);
+    if (!proto_drv) {
+        error_set_check(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return -1;
+    }
+    st_sync->external.step = BLK_SNAPSHOT_EXT_START;
+    return 0;
+}
+
+/* We don't do anything that commits us to the snapshot here, do it later. */
+static int blk_sn_create_external_sync(BlkTransStates *states,
+                                       Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *old_bs = st_sync->external.old_bs;
+    const char *format = st_sync->external.format;
+    BlockDriver *drv = st_sync->external.format_drv;
+    const char *new_image_file = st_sync->external.new_image_file;
+    bool use_existing = st_sync->use_existing;
+    BlockDriverState *new_bs;
+    Error *local_err = NULL;
+
+    int flags = old_bs->open_flags, ret;
+    /* create new image w/backing file */
+    if (!use_existing) {
+        bdrv_img_create(new_image_file, format,
+                        old_bs->filename,
+                        old_bs->drv->format_name,
+                        NULL, -1, flags, &local_err);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            return -1;
+        }
+    }
+
+    /* We will manually add the backing_hd field to the bs later */
+    new_bs = bdrv_new("");
+    ret = bdrv_open(new_bs, new_image_file,
+                    flags | BDRV_O_NO_BACKING, drv);
+    if (ret != 0) {
+        error_set_check(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+        return -1;
+    }
+
+    st_sync->external.new_bs = new_bs;
+    st_sync->external.step = BLK_SNAPSHOT_EXT_CREATED;
+    return 0;
+}
+
+/* refresh the blk device's states to emulator, always succeed now. */
+static int blk_sn_invalid_external_sync(BlkTransStates *states,
+                                        Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlkSnapshotExternal *ext = &st_sync->external;
+    /* This removes our old bs from the bdrv_states, and adds the new
+     * bs */
+    bdrv_append(ext->new_bs, ext->old_bs);
+    /* We don't need (or want) to use the transactional
+     * bdrv_reopen_multiple() across all the entries at once, because
+     * we don't want to abort all of them if one of them fails the
+     * reopen */
+    bdrv_reopen(ext->new_bs,
+                ext->new_bs->open_flags & ~BDRV_O_RDWR,
+                NULL);
+
+    st_sync->external.step = BLK_SNAPSHOT_EXT_INVALIDATED;
+    return 0;
+}
+
+/* cancel the job if it have been done before */
+static int blk_sn_cancel_external_sync(BlkTransStates *states,
+                                       Error **errp)
+{
+    BlkTransStatesSync *st_sync = &states->st_sync;
+    BlkSnapshotExternal *ext = &st_sync->external;
+    if (ext->step == BLK_SNAPSHOT_EXT_INVALIDATED) {
+        /* revert the bind */
+        bdrv_deappend(ext->new_bs, ext->old_bs);
+        bdrv_reopen(ext->old_bs,
+                    ext->old_bs->open_flags & ~BDRV_O_RDWR,
+                    NULL);
+        ext->step = BLK_SNAPSHOT_EXT_CREATED;
+    }
+    if (ext->step == BLK_SNAPSHOT_EXT_CREATED) {
+        /* abandon new bs, and keep using the original bs.
+         * no need to delete the image? */
+        bdrv_delete(ext->new_bs);
+        ext->step = BLK_SNAPSHOT_EXT_CANCELED;
+    }
+    return 0;
+}
+
+/*
+ * caller's API to add, submit request, support mixed request.
+ */
+
+BlkTransStates *blk_trans_st_new(void)
+{
+    BlkTransStates *states = g_malloc0(sizeof(BlkTransStates));
+    return states;
+}
+
+void blk_trans_st_delete(BlkTransStates **p_st)
+{
+    if ((*p_st)->err != NULL) {
+        error_free((*p_st)->err);
+    }
+    g_free(*p_st);
+    *p_st = NULL;
+    return;
+}
+
+BlkTransStatesList *blk_trans_st_list_new(void)
+{
+    BlkTransStatesList *list =
+                      g_malloc0(sizeof(BlkTransStatesList));
+    QSIMPLEQ_INIT(list);
+    return list;
+}
+
+void blk_trans_st_list_delete(BlkTransStatesList **p_list)
+{
+    BlkTransStates *states, *next;
+    QSIMPLEQ_FOREACH_SAFE(states, (*p_list), entry, next) {
+        blk_trans_st_delete(&states);
+    }
+    g_free(*p_list);
+    *p_list = NULL;
+    return;
+}
+
+int add_transaction(BlkTransStatesList *list,
+                    BlkTransStates *states,
+                    Error **errp)
+{
+    int ret;
+
+    switch (states->st_sync.op) {
+    case BLK_SN_SYNC_CREATE:
+        switch (states->st_sync.type) {
+        case BLK_SNAPSHOT_INTERNAL:
+            ret = blk_sn_check_create_internal_sync(states, errp);
+            if (ret < 0) {
+                return -1;
+            }
+            states->blk_trans_do = blk_sn_create_internal_sync;
+            states->blk_trans_cancel = blk_sn_cancel_internal_sync;
+            break;
+        case BLK_SNAPSHOT_EXTERNAL:
+            ret = blk_sn_check_create_external_sync(states, errp);
+            if (ret < 0) {
+                return -1;
+            }
+            states->blk_trans_do = blk_sn_create_external_sync;
+            states->blk_trans_invalid = blk_sn_invalid_external_sync;
+            states->blk_trans_cancel = blk_sn_cancel_external_sync;
+            break;
+        default:
+            error_setg_check(errp, "Operation %d for type %d not supprted.",
+                             states->st_sync.op, states->st_sync.type);
+            return -1;
+        }
+        break;
+    case BLK_SN_SYNC_DELETE:
+        switch (states->st_sync.type) {
+        case BLK_SNAPSHOT_INTERNAL:
+            ret = blk_sn_check_delete_internal_sync(states, errp);
+            if (ret < 0) {
+                return -1;
+            }
+            states->blk_trans_do = blk_sn_delete_internal_sync;
+            /* this operation can't be canceled. */
+            break;
+        case BLK_SNAPSHOT_EXTERNAL:
+            /* sync delete an external snapshot, not support now,
+               use blk commit instead. */
+            error_setg_check(errp, "Operation %d for type %d not supprted.",
+                             states->st_sync.op, states->st_sync.type);
+            return -1;
+        default:
+            error_setg_check(errp, "Operation %d for type %d not supprted.",
+                             states->st_sync.op, states->st_sync.type);
+            return -1;
+        }
+        break;
+    default:
+        return -1;
+    }
+
+    /* Todo, in async case const char* member should be duplicated, but
+    we do not support it now so directly enlist the request. */
+    QSIMPLEQ_INSERT_TAIL(list, states, entry);
+    return 0;
+}
+
+int submit_transaction(BlkTransStatesList *list, Error **errp)
+{
+    BlkTransStates *states = NULL;
+    int ret = 0;
+    Error *err;
+
+    /* drain all i/o before any snapshots */
+    bdrv_drain_all();
+
+    /* step 1, do the action, that is create/delete snapshots in sync case */
+    QSIMPLEQ_FOREACH(states, list, entry) {
+        ret = states->blk_trans_do(states, &states->err);
+        if (ret < 0) {
+            goto delete_and_fail;
+        }
+    }
+
+    /* step 2, update emulator */
+    QSIMPLEQ_FOREACH(states, list, entry) {
+        if (states->blk_trans_invalid) {
+            ret = states->blk_trans_invalid(states, &states->err);
+            if (ret < 0) {
+                goto delete_and_fail;
+            }
+        }
+    }
+
+    /* success */
+    return 0;
+
+delete_and_fail:
+    /*
+     * failure, and it is all-or-none, cancel all if error got.
+     */
+    err = states->err;
+    QSIMPLEQ_FOREACH(states, list, entry) {
+        if (states->blk_trans_cancel) {
+            ret = states->blk_trans_cancel(states, &states->err);
+            if (!err) {
+                    err = states->err;
+            }
+        }
+    }
+    if (err && errp && !*errp) {
+        *errp = error_copy(err);
+    }
+    return -1;
+}
+
+
 static void blockdev_do_action(int kind, void *data, Error **errp)
 {
     BlockdevAction action;
-- 
1.7.1

[Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

  This patch switch to internal common API to take group external
snapshots from qmp_transaction interface. qmp layer simply does
a translation from user input.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
 1 files changed, 87 insertions(+), 128 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 668506d..299039f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1173,157 +1173,116 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                        errp);
 }
 
-
-/* New and old BlockDriverState structs for group snapshots */
-typedef struct BlkTransactionStates {
-    BlockDriverState *old_bs;
-    BlockDriverState *new_bs;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
-} BlkTransactionStates;
-
-/*
- * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
- *  then we do not pivot any of the devices in the group, and abandon the
- *  snapshots
- */
-void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
+/* translation from qmp commands */
+static int fill_blk_trs_ext_create_sync(BlockdevSnapshot *create_sync,
+                                        BlkTransStatesSync *st_sync,
+                                        Error **errp)
 {
-    int ret = 0;
-    BlockdevActionList *dev_entry = dev_list;
-    BlkTransactionStates *states, *next;
-    Error *local_err = NULL;
-
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
+    const char *format = "qcow2";
+    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    BlockDriverState *bs;
+    const char *device = create_sync->device;
+    const char *name = create_sync->snapshot_file;
 
-    /* drain all i/o before any snapshots */
-    bdrv_drain_all();
+    if (create_sync->has_mode) {
+        mode = create_sync->mode;
+    }
+    if (create_sync->has_format) {
+        format = create_sync->format;
+    }
 
-    /* We don't do anything in this loop that commits us to the snapshot */
-    while (NULL != dev_entry) {
-        BlockdevAction *dev_info = NULL;
-        BlockDriver *proto_drv;
-        BlockDriver *drv;
-        int flags;
-        enum NewImageMode mode;
-        const char *new_image_file;
-        const char *device;
-        const char *format = "qcow2";
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
 
-        dev_info = dev_entry->value;
-        dev_entry = dev_entry->next;
+    switch (mode) {
+    case NEW_IMAGE_MODE_EXISTING:
+        st_sync->use_existing = true;
+        break;
+    case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
+        st_sync->use_existing = false;
+        break;
+    default:
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " mode %d.", device, mode);
+        return -1;
+    }
 
-        states = g_malloc0(sizeof(BlkTransactionStates));
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+    st_sync->external.new_image_file = name;
+    st_sync->external.format = format;
+    st_sync->external.old_bs = bs;
 
-        switch (dev_info->kind) {
-        case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            device = dev_info->blockdev_snapshot_sync->device;
-            if (!dev_info->blockdev_snapshot_sync->has_mode) {
-                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-            }
-            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
-            if (dev_info->blockdev_snapshot_sync->has_format) {
-                format = dev_info->blockdev_snapshot_sync->format;
-            }
-            mode = dev_info->blockdev_snapshot_sync->mode;
-            break;
-        default:
-            abort();
-        }
+    return 0;
+}
 
-        drv = bdrv_find_format(format);
-        if (!drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
+static int fill_blk_trs(BlockdevAction *dev_info,
+                        BlkTransStates *states,
+                        SNTime *time,
+                        const char *time_str,
+                        Error **errp)
+{
+    int ret = 0;
 
-        states->old_bs = bdrv_find(device);
-        if (!states->old_bs) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-            goto delete_and_fail;
-        }
+    switch (dev_info->kind) {
+    case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+        states->st_sync.type = BLK_SNAPSHOT_EXTERNAL;
+        states->st_sync.op = BLK_SN_SYNC_CREATE;
+        ret = fill_blk_trs_ext_create_sync(dev_info->blockdev_snapshot_sync,
+                                           &states->st_sync,
+                                           errp);
+        break;
+    default:
+        abort();
+    }
 
-        if (!bdrv_is_inserted(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-            goto delete_and_fail;
-        }
+    return ret;
+}
 
-        if (bdrv_in_use(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_IN_USE, device);
-            goto delete_and_fail;
-        }
+/* Here this funtion prepare the request list, submit for atomic snapshot. */
+void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
+{
+    BlockdevActionList *dev_entry = dev_list;
+    BlkTransStates *states;
+    int ret;
 
-        if (!bdrv_is_read_only(states->old_bs)) {
-            if (bdrv_flush(states->old_bs)) {
-                error_set(errp, QERR_IO_ERROR);
-                goto delete_and_fail;
-            }
-        }
+    BlkTransStatesList *snap_bdrv_states = blk_trans_st_list_new();
 
-        flags = states->old_bs->open_flags;
+    /* translate qmp request */
+    /* for group snapshot create we use same time stamp here */
+    SNTime time = get_sn_time();
+    char time_str[256];
+    generate_sn_name_from_time(&time, time_str, sizeof(time_str));
+    while (NULL != dev_entry) {
+        BlockdevAction *dev_info = NULL;
 
-        proto_drv = bdrv_find_protocol(new_image_file);
-        if (!proto_drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
+        dev_info = dev_entry->value;
+        dev_entry = dev_entry->next;
 
-        /* create new image w/backing file */
-        if (mode != NEW_IMAGE_MODE_EXISTING) {
-            bdrv_img_create(new_image_file, format,
-                            states->old_bs->filename,
-                            states->old_bs->drv->format_name,
-                            NULL, -1, flags, &local_err);
-            if (error_is_set(&local_err)) {
-                error_propagate(errp, local_err);
-                goto delete_and_fail;
-            }
+        states = blk_trans_st_new();
+        ret = fill_blk_trs(dev_info, states, &time, time_str, errp);
+        if (ret < 0) {
+            blk_trans_st_delete(&states);
+            goto exit;
         }
 
-        /* We will manually add the backing_hd field to the bs later */
-        states->new_bs = bdrv_new("");
-        ret = bdrv_open(states->new_bs, new_image_file,
-                        flags | BDRV_O_NO_BACKING, drv);
-        if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
-            goto delete_and_fail;
+        ret = add_transaction(snap_bdrv_states, states, errp);
+        if (ret < 0) {
+            blk_trans_st_delete(&states);
+            goto exit;
         }
     }
 
+    /* submit to internal API, no need to check return for no following
+       action now. */
+    submit_transaction(snap_bdrv_states, errp);
 
-    /* Now we are going to do the actual pivot.  Everything up to this point
-     * is reversible, but we are committed at this point */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        /* This removes our old bs from the bdrv_states, and adds the new bs */
-        bdrv_append(states->new_bs, states->old_bs);
-        /* We don't need (or want) to use the transactional
-         * bdrv_reopen_multiple() across all the entries at once, because we
-         * don't want to abort all of them if one of them fails the reopen */
-        bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
-                    NULL);
-    }
-
-    /* success */
-    goto exit;
-
-delete_and_fail:
-    /*
-    * failure, and it is all-or-none; abandon each new bs, and keep using
-    * the original bs for all images
-    */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        if (states->new_bs) {
-             bdrv_delete(states->new_bs);
-        }
-    }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-        g_free(states);
-    }
+    blk_trans_st_list_delete(&snap_bdrv_states);
 }
 
-
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
     if (bdrv_in_use(bs)) {
-- 
1.7.1

[Qemu-devel] [PATCH V2 08/10] snapshot: qmp add internal snapshot transaction interface

[Wenchao Xia]

  Now qmp_transaction support creating internal snapshot.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   29 +++++++++++++++++++++++++++++
 qmp-commands.hx  |   24 +++++++++++++++++-------
 3 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 299039f..09eeb7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1218,6 +1218,41 @@ static int fill_blk_trs_ext_create_sync(BlockdevSnapshot *create_sync,
     return 0;
 }
 
+static int fill_blk_trs_int_create_sync(BlockdevSnapshotInternal *create_sync,
+                                        BlkTransStatesSync *st_sync,
+                                        SNTime *time,
+                                        const char *time_str,
+                                        Error **errp)
+{
+    BlockDriverState *bs;
+    const char *device = create_sync->device;
+    const char *name = NULL;
+
+    if (create_sync->has_name) {
+        name = create_sync->name;
+    }
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    st_sync->use_existing = false;
+
+    /* internal case, if caller need create new one with default string */
+    if (name == NULL) {
+        st_sync->internal.sn_name = time_str;
+    } else {
+        st_sync->internal.sn_name = name;
+    }
+    st_sync->internal.bs = bs;
+    st_sync->internal.time = *time;
+
+    return 0;
+}
+
 static int fill_blk_trs(BlockdevAction *dev_info,
                         BlkTransStates *states,
                         SNTime *time,
@@ -1234,6 +1269,17 @@ static int fill_blk_trs(BlockdevAction *dev_info,
                                            &states->st_sync,
                                            errp);
         break;
+    case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC:
+        states->st_sync.type = BLK_SNAPSHOT_INTERNAL;
+        states->st_sync.op = BLK_SN_SYNC_CREATE;
+        BlockdevSnapshotInternal *create_sync;
+        create_sync = dev_info->blockdev_snapshot_internal_sync;
+        ret = fill_blk_trs_int_create_sync(create_sync,
+                                           &states->st_sync,
+                                           time,
+                                           time_str,
+                                           errp);
+        break;
     default:
         abort();
     }
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..ea3ef77 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1474,6 +1474,20 @@
   'data': [ 'existing', 'absolute-paths' ] }
 
 ##
+# @NewSnapshotMode
+#
+# An enumeration that tells QEMU how to create internal snapshot.
+#
+# @existing: QEMU should look for an existing snapshot.
+#
+# @new: QEMU should create a new internal snapshot, if it exist, overwrite it.
+#
+# Since: 1.4
+##
+{ 'enum': 'NewSnapshotMode'
+  'data': [ 'existing', 'new' ] }
+
+##
 # @BlockdevSnapshot
 #
 # @device:  the name of the device to generate the snapshot from.
@@ -1490,6 +1504,20 @@
             '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshotInternal
+#
+# @device:  the name of the device to generate the snapshot from.
+#
+# @name: #optional the name of the internal snapshot to create, default is to
+#         generate it automatically according to host time. If a snapshot with
+#         name exist, it will be overwritten.
+#
+# Since: 1.4
+##
+{ 'type': 'BlockdevSnapshotInternal',
+  'data': { 'device': 'str', '*name': 'str', } }
+
+##
 # @BlockdevAction
 #
 # A discriminated record of operations that can be performed with
@@ -1498,6 +1526,7 @@
 { 'union': 'BlockdevAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
    } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..3c2f468 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -863,10 +863,13 @@ any of the operations, all snapshots for the group are abandoned, and
 the original disks pre-snapshot attempt are used.
 
 A list of dictionaries is accepted, that contains the actions to be performed.
-For snapshots this is the device, the file to use for the new snapshot,
-and the format.  The default format, if not specified, is qcow2.
+For external snapshots this is the device, the file to use for the new
+snapshot, and the format.  The default format, if not specified, is qcow2.
+For internal snapshots this is the device, the snapshot name. If name is
+not specified, it would be generated automatically according to host time.
+If an internal snapshot with name exist, it will be over written.
 
-Each new snapshot defaults to being created by QEMU (wiping any
+Each new external snapshot defaults to being created by QEMU (wiping any
 contents if the file already exists), but it is also possible to reuse
 an externally-created file.  In the latter case, you should ensure that
 the new image file has the same contents as the current one; QEMU cannot
@@ -876,15 +879,20 @@ current image file as the backing file for the new image.
 Arguments:
 
 actions array:
-    - "type": the operation to perform.  The only supported
-      value is "blockdev-snapshot-sync". (json-string)
+    - "type": the operation to perform.  The supported values are
+      "blockdev-snapshot-sync" and "blockdev-snapshot-internal-sync".
+      (json-string)
     - "data": a dictionary.  The contents depend on the value
-      of "type".  When "type" is "blockdev-snapshot-sync":
+      of "type".
+      When "type" is "blockdev-snapshot-sync":
       - "device": device name to snapshot (json-string)
       - "snapshot-file": name of new image file (json-string)
       - "format": format of new image (json-string, optional)
       - "mode": whether and how QEMU should create the snapshot file
         (NewImageMode, optional, default "absolute-paths")
+      When "type" is "blockdev-snapshot-internal-sync":
+      - "device": device name to snapshot (json-string)
+      - "name": name of internal snapshot (json-string, optional)
 
 Example:
 
@@ -896,7 +904,9 @@ Example:
          { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
                                          "mode": "existing",
-                                         "format": "qcow2" } } ] } }
+                                         "format": "qcow2" } },
+         { 'type': 'blockdev-snapshot-internal-sync', 'data' : { "device": "ide-hd2",
+                                         "name": "snapshot0" } } ] } }
 <- { "return": {} }
 
 EQMP
-- 
1.7.1

[Qemu-devel] [PATCH V2 09/10] snapshot: qmp add blockdev-snapshot-internal-sync interface

[Wenchao Xia]

  This interface allow user to create internal snapshot on a single
block device.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |   14 ++++++++++++++
 qapi-schema.json |   21 ++++++++++++++++++++-
 qmp-commands.hx  |   28 ++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 09eeb7f..b2e6b3d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1173,6 +1173,20 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                        errp);
 }
 
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+                                         bool has_name, const char *name,
+                                         Error **errp)
+{
+    BlockdevSnapshotInternal snapshot = {
+        .device = (char *) device,
+        .has_name = has_name,
+        .name = (char *) name,
+    };
+    blockdev_do_action(BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+                       &snapshot,
+                       errp);
+}
+
 /* translation from qmp commands */
 static int fill_blk_trs_ext_create_sync(BlockdevSnapshot *create_sync,
                                         BlkTransStatesSync *st_sync,
diff --git a/qapi-schema.json b/qapi-schema.json
index ea3ef77..459850a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1555,7 +1555,7 @@
 ##
 # @blockdev-snapshot-sync
 #
-# Generates a synchronous snapshot of a block device.
+# Generates a synchronous external snapshot of a block device.
 #
 # @device:  the name of the device to generate the snapshot from.
 #
@@ -1578,6 +1578,25 @@
             '*mode': 'NewImageMode'} }
 
 ##
+# @blockdev-snapshot-internal-sync
+#
+# Generates a synchronous internal snapshot of a block device.
+#
+# @device:  the name of the device to generate the snapshot from.
+#
+# @name: #optional the name of snapshot to be created. If not specified, a name
+#        would be generated according to host time. If an internal snapshot with
+#        name exist, it will be overwritten.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#
+# Since 1.4
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+  'data': { 'device': 'str', '*name': 'str'} }
+
+##
 # @human-monitor-command:
 #
 # Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3c2f468..499e64e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -946,6 +946,34 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-internal-sync",
+        .args_type  = "device:B,name:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+    },
+
+SQMP
+blockdev-snapshot-internal-sync
+----------------------
+
+Synchronous snapshot of a block device. name specifies the internal snapshot
+name. If it is not specified, a name would be generated according to host time.
+If a snapshot with that name already exist, it will be overwritten.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "name": name of internal snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-internal-sync", "arguments": {
+                                                        "device": "ide-hd0",
+                                                        "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?",
-- 
1.7.1

[Qemu-devel] [PATCH V2 10/10] snapshot: hmp add internal snapshot support for block device

[Wenchao Xia]

  Now hmp can take snapshot for a single block device.

v2:
  Removed option -n in internal snapshot case.
  Better tips.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp-commands.hx |   28 +++++++++++++++++-----------
 hmp.c           |   25 +++++++++++++++----------
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..bd10349 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -983,17 +983,23 @@ ETEXI
 
     {
         .name       = "snapshot_blkdev",
-        .args_type  = "reuse:-n,device:B,snapshot-file:s?,format:s?",
-        .params     = "[-n] device [new-image-file] [format]",
-        .help       = "initiates a live snapshot\n\t\t\t"
-                      "of device. If a new image file is specified, the\n\t\t\t"
-                      "new image file will become the new root image.\n\t\t\t"
-                      "If format is specified, the snapshot file will\n\t\t\t"
-                      "be created in that format. Otherwise the\n\t\t\t"
-                      "snapshot will be internal! (currently unsupported).\n\t\t\t"
-                      "The default format is qcow2.  The -n flag requests QEMU\n\t\t\t"
-                      "to reuse the image found in new-image-file, instead of\n\t\t\t"
-                      "recreating it from scratch.",
+        .args_type  = "internal:-i,reuse:-n,device:B,name:s?,format:s?",
+        .params     = "[-i] [-n] device [name] [format]",
+        .help       = "initiates a live snapshot of device.\n\t\t\t"
+                      "  The -i flag requests QEMU to create internal snapshot\n\t\t\t"
+                      "instead of external one.\n\t\t\t"
+                      "  The -n flag requests QEMU to reuse the image found in\n\t\t\t"
+                      "in name, instead of recreating it from scratch. Only valid\n\t\t\t"
+                      "for external case.\n\t\t\t"
+                      "  The name is the snapshot's name. In external case\n\t\t\t"
+                      "it is the new image's name which will become the new root\n\t\t\t"
+                      "image and must be specified. In internal case it is the\n\t\t\t"
+                      "record's name and if not specified QEMU will create\n\t\t\t"
+                      "internal snapshot with name generated according to time.\n\t\t\t"
+                      "Any existing internal snapshot with name will be overwritten.\n\t\t\t"
+                      "  The format is the new snapshot image's format. If not\n\t\t\t"
+                      "sepcified, the default format is qcow2. Only valid for external\n\t\t\t"
+                      "case.",
         .mhandler.cmd = hmp_snapshot_blkdev,
     },
 
diff --git a/hmp.c b/hmp.c
index 9e9e624..d78d55f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -804,23 +804,28 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
-    const char *filename = qdict_get_try_str(qdict, "snapshot-file");
+    const char *name = qdict_get_try_str(qdict, "name");
     const char *format = qdict_get_try_str(qdict, "format");
     int reuse = qdict_get_try_bool(qdict, "reuse", 0);
+    int internal = qdict_get_try_bool(qdict, "internal", 0);
     enum NewImageMode mode;
+
     Error *errp = NULL;
 
-    if (!filename) {
-        /* In the future, if 'snapshot-file' is not specified, the snapshot
-           will be taken internally. Today it's actually required. */
-        error_set(&errp, QERR_MISSING_PARAMETER, "snapshot-file");
-        hmp_handle_error(mon, &errp);
-        return;
+    if (internal) {
+        qmp_blockdev_snapshot_internal_sync(device, !!name, name, &errp);
+    } else {
+        if (!name) {
+            /* Name must be specified for external case */
+            error_set(&errp, QERR_MISSING_PARAMETER, "name");
+            hmp_handle_error(mon, &errp);
+            return;
+        }
+        mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+        qmp_blockdev_snapshot_sync(device, name, !!format, format,
+                                   true, mode, &errp);
     }
 
-    mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    qmp_blockdev_snapshot_sync(device, filename, !!format, format,
-                               true, mode, &errp);
     hmp_handle_error(mon, &errp);
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH V2 04/10] oslib-win32: add lock for time functions

[Stefan Weil]

Am 07.01.2013 08:28, schrieb Wenchao Xia:
>    This patch adding lock for calling gmtime() and localtime()
> on windows. If no other lib linked into qemu would call those
> two function itself, then they are thread safe now on windows.
>
> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
> ---
>   oslib-win32.c |   12 ++++++++++--
>   1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/oslib-win32.c b/oslib-win32.c
> index e7e283e..344e3dd 100644
> --- a/oslib-win32.c
> +++ b/oslib-win32.c
> @@ -74,27 +74,35 @@ void qemu_vfree(void *ptr)
>       VirtualFree(ptr, 0, MEM_RELEASE);
>   }
>
> -/* FIXME: add proper locking */
> +/* WARN: if other lib call gmtime() itself, then it is not thread safe. */
>   struct tm *gmtime_r(const time_t *timep, struct tm *result)
>   {
> +    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
> +
> +    g_static_mutex_lock(&lock);
>       struct tm *p = gmtime(timep);
>       memset(result, 0, sizeof(*result));
>       if (p) {
>           *result = *p;
>           p = result;
>       }
> +    g_static_mutex_unlock(&lock);
>       return p;
>   }
>
> -/* FIXME: add proper locking */
> +/* WARN: if other lib call localtime() itself, then it is not thread safe. */
>   struct tm *localtime_r(const time_t *timep, struct tm *result)
>   {
> +    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
> +
> +    g_static_mutex_lock(&lock);
>       struct tm *p = localtime(timep);
>       memset(result, 0, sizeof(*result));
>       if (p) {
>           *result = *p;
>           p = result;
>       }
> +    g_static_mutex_unlock(&lock);
>       return p;
>   }
>

Please remove this patch from the series:

* It is unrelated to the other patches but tries to fix
   a separate problem. Even if the code now calls localtime_r,
   it is not worse than the old patch version which called
   localtime for MinGW.

* localtime_r and gmtime_r use the same global storage,
   so they must use a common mutex instead of one mutex
   for each function.

* Even with a common mutex, the comment
   "FIXME: add proper locking" still applies because there
   is no common locking mechanism for asctime, ctime, gmtime
   and localtime. I prefer FIXME or TODO comments for
   code deficiencies, not WARN.

The correct fix can only be done in MinGW / MinGW-w64.

Improving QEMU in a separate series with correct locking
is ok if that series also removes the remaining
calls of gmtime and localtime. Then the risk of
reentrancy problems is reduced to a minimum.

Regards

Stefan W.

Re: [Qemu-devel] [PATCH V2 04/10] oslib-win32: add lock for time functions

[Wenchao Xia]

 > Am 07.01.2013 08:28, schrieb Wenchao Xia:
>>    This patch adding lock for calling gmtime() and localtime()
>> on windows. If no other lib linked into qemu would call those
>> two function itself, then they are thread safe now on windows.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> ---
>>   oslib-win32.c |   12 ++++++++++--
>>   1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/oslib-win32.c b/oslib-win32.c
>> index e7e283e..344e3dd 100644
>> --- a/oslib-win32.c
>> +++ b/oslib-win32.c
>> @@ -74,27 +74,35 @@ void qemu_vfree(void *ptr)
>>       VirtualFree(ptr, 0, MEM_RELEASE);
>>   }
>>
>> -/* FIXME: add proper locking */
>> +/* WARN: if other lib call gmtime() itself, then it is not thread
>> safe. */
>>   struct tm *gmtime_r(const time_t *timep, struct tm *result)
>>   {
>> +    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
>> +
>> +    g_static_mutex_lock(&lock);
>>       struct tm *p = gmtime(timep);
>>       memset(result, 0, sizeof(*result));
>>       if (p) {
>>           *result = *p;
>>           p = result;
>>       }
>> +    g_static_mutex_unlock(&lock);
>>       return p;
>>   }
>>
>> -/* FIXME: add proper locking */
>> +/* WARN: if other lib call localtime() itself, then it is not thread
>> safe. */
>>   struct tm *localtime_r(const time_t *timep, struct tm *result)
>>   {
>> +    static GStaticMutex lock = G_STATIC_MUTEX_INIT;
>> +
>> +    g_static_mutex_lock(&lock);
>>       struct tm *p = localtime(timep);
>>       memset(result, 0, sizeof(*result));
>>       if (p) {
>>           *result = *p;
>>           p = result;
>>       }
>> +    g_static_mutex_unlock(&lock);
>>       return p;
>>   }
>>
>
> Please remove this patch from the series:
>
> * It is unrelated to the other patches but tries to fix
>    a separate problem. Even if the code now calls localtime_r,
>    it is not worse than the old patch version which called
>    localtime for MinGW.
>
   OK.

> * localtime_r and gmtime_r use the same global storage,
>    so they must use a common mutex instead of one mutex
>    for each function.
>
   Thanks for tipping that, I did not know this before.

> * Even with a common mutex, the comment
>    "FIXME: add proper locking" still applies because there
>    is no common locking mechanism for asctime, ctime, gmtime
>    and localtime. I prefer FIXME or TODO comments for
>    code deficiencies, not WARN.
>
> The correct fix can only be done in MinGW / MinGW-w64.
>
   I think so, will keeps "FIXME" comments and change the code
calling gmtime()/localtime().

> Improving QEMU in a separate series with correct locking
> is ok if that series also removes the remaining
> calls of gmtime and localtime. Then the risk of
> reentrancy problems is reduced to a minimum.
>
> Regards
>
> Stefan W.
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Stefan Hajnoczi]

On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>   This patch switch to internal common API to take group external
> snapshots from qmp_transaction interface. qmp layer simply does
> a translation from user input.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
>  1 files changed, 87 insertions(+), 128 deletions(-)

An internal API for snapshots is not necessary.  qmp_transaction() is
already usable both from the monitor and C code.

The QAPI code generator creates structs that can be accessed directly
from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
the snapshot API.  It just doesn't support internal snapshots yet, which
is what you are trying to add.

To add internal snapshot support, define a BlockdevInternalSnapshot type
in qapi-schema.json and add internal snapshot support in
qmp_transaction().

qmp_transaction() was designed with this in mind from the beginning and
dispatches based on BlockdevAction->kind.

The patch series will become much smaller while still adding internal
snapshot support.

Stefan

Re: [Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On 01/07/2013 12:27 AM, Wenchao Xia wrote:
>   These patch added a seperated layer to take internal or external snapshots
> in a unified way, the granularity is block device, so other functions can
> just combine the request and submit, such as group snapshot, savevm.
> 
> Total goal are:
>   Live back up vm in external or internal image, which need three functions:
> 1 live snapshot block device internal/external.
> 2 live save vmstate internal/external.
> 3 combination of the function unit.
> 
>   This serial provide part one.
> 

The design on this series needs to be coordinated with Pavel's
attempts[1] to convert the existing HMP snapshot commands into QMP.  I
don't want to spend too much time reviewing two different divergent
designs that both have the same goal of finally managing snapshots under
QMP.

[1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01326.html

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>>    This patch switch to internal common API to take group external
>> snapshots from qmp_transaction interface. qmp layer simply does
>> a translation from user input.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
>>   1 files changed, 87 insertions(+), 128 deletions(-)
>
> An internal API for snapshots is not necessary.  qmp_transaction() is
> already usable both from the monitor and C code.
>
> The QAPI code generator creates structs that can be accessed directly
> from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
> the snapshot API.  It just doesn't support internal snapshots yet, which
> is what you are trying to add.
>
> To add internal snapshot support, define a BlockdevInternalSnapshot type
> in qapi-schema.json and add internal snapshot support in
> qmp_transaction().
>
> qmp_transaction() was designed with this in mind from the beginning and
> dispatches based on BlockdevAction->kind.
>
> The patch series will become much smaller while still adding internal
> snapshot support.
>
> Stefan
>

   As API, qmp_transaction have following disadvantages:
1) interface is based on string not data type inside qemu, that means
other function calling it result in: bdrv->string->bdrv
2) all capability are forced to be exposed.
3) need structure to record each transaction state, such as
BlkTransactionStates. Extending it is equal to add an internal layer.

   Actually I started up by use qmp_transaction as API, but soon
found that work is almost done around BlkTransactionStates, so
added a layer around it clearly.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Wenchao Xia]

于 2013-1-10 6:34, Eric Blake 写道:
> On 01/07/2013 12:27 AM, Wenchao Xia wrote:
>>    These patch added a seperated layer to take internal or external snapshots
>> in a unified way, the granularity is block device, so other functions can
>> just combine the request and submit, such as group snapshot, savevm.
>>
>> Total goal are:
>>    Live back up vm in external or internal image, which need three functions:
>> 1 live snapshot block device internal/external.
>> 2 live save vmstate internal/external.
>> 3 combination of the function unit.
>>
>>    This serial provide part one.
>>
>
> The design on this series needs to be coordinated with Pavel's
> attempts[1] to convert the existing HMP snapshot commands into QMP.  I
> don't want to spend too much time reviewing two different divergent
> designs that both have the same goal of finally managing snapshots under
> QMP.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01326.html
>
   A bit difference: This serial are taking internal/external block
snapshots but Pavel's are focusing are vm whole internal snapshots. It
is OK to use Pavel's interface with a mark about "internal and sync".
I'd like to add new interface after vmstate live save is implemented.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Stefan Hajnoczi]

On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> >On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
> >>   This patch switch to internal common API to take group external
> >>snapshots from qmp_transaction interface. qmp layer simply does
> >>a translation from user input.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>---
> >>  blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
> >>  1 files changed, 87 insertions(+), 128 deletions(-)
> >
> >An internal API for snapshots is not necessary.  qmp_transaction() is
> >already usable both from the monitor and C code.
> >
> >The QAPI code generator creates structs that can be accessed directly
> >from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
> >the snapshot API.  It just doesn't support internal snapshots yet, which
> >is what you are trying to add.
> >
> >To add internal snapshot support, define a BlockdevInternalSnapshot type
> >in qapi-schema.json and add internal snapshot support in
> >qmp_transaction().
> >
> >qmp_transaction() was designed with this in mind from the beginning and
> >dispatches based on BlockdevAction->kind.
> >
> >The patch series will become much smaller while still adding internal
> >snapshot support.
> >
> >Stefan
> >
> 
>   As API, qmp_transaction have following disadvantages:
> 1) interface is based on string not data type inside qemu, that means
> other function calling it result in: bdrv->string->bdrv

Use bdrv_get_device_name().  You already need to fill in filename or
snapshot name strings.  This is not a big disadvantage.

> 2) all capability are forced to be exposed.

Is there something you cannot expose?

> 3) need structure to record each transaction state, such as
> BlkTransactionStates. Extending it is equal to add an internal layer.

I agree that extending it is equal coding effort to adding an internal
layer because you'll need to refactor qmp_transaction() a bit to really
support additional action types.

But it's the right thing to do.  Don't add unnecessary layers just
because writing new code is more fun than extending existing code.

>   Actually I started up by use qmp_transaction as API, but soon
> found that work is almost done around BlkTransactionStates, so
> added a layer around it clearly.

BlkTransactionStates is only relevant to external snapshots because they
change the BlkDriverState chain and must be able to undo that.

For internal snapshots you simply need to store the snapshot name so you
can delete it if there is an error partway through.  (BTW it's not
possible to completely restore state if you allow overwriting existing
internal snapshots unless you do something like taking a backup snapshot
of the existing snapshot first!)

Stefan

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-1-10 20:41, Stefan Hajnoczi 写道:
> On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
>> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
>>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>>>>    This patch switch to internal common API to take group external
>>>> snapshots from qmp_transaction interface. qmp layer simply does
>>>> a translation from user input.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>>   blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
>>>>   1 files changed, 87 insertions(+), 128 deletions(-)
>>>
>>> An internal API for snapshots is not necessary.  qmp_transaction() is
>>> already usable both from the monitor and C code.
>>>
>>> The QAPI code generator creates structs that can be accessed directly
>> >from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
>>> the snapshot API.  It just doesn't support internal snapshots yet, which
>>> is what you are trying to add.
>>>
>>> To add internal snapshot support, define a BlockdevInternalSnapshot type
>>> in qapi-schema.json and add internal snapshot support in
>>> qmp_transaction().
>>>
>>> qmp_transaction() was designed with this in mind from the beginning and
>>> dispatches based on BlockdevAction->kind.
>>>
>>> The patch series will become much smaller while still adding internal
>>> snapshot support.
>>>
>>> Stefan
>>>
>>
>>    As API, qmp_transaction have following disadvantages:
>> 1) interface is based on string not data type inside qemu, that means
>> other function calling it result in: bdrv->string->bdrv
>
> Use bdrv_get_device_name().  You already need to fill in filename or
> snapshot name strings.  This is not a big disadvantage.
>
   Yes, not a big disadvantage, but why not save string operation but
use (bdrv*) as much as possible?

what happens will be:

hmp-snapshot
     |
qmp-snapshot
     |---------
              |
         qmp-transaction            savevm(may be other..)
              |----------------------|
                             |
               internal transaction layer

>> 2) all capability are forced to be exposed.
>
> Is there something you cannot expose?
>
   As other component in qemu can use it, some option may
be used only in qemu not to user. For eg, vm-state-size.

>> 3) need structure to record each transaction state, such as
>> BlkTransactionStates. Extending it is equal to add an internal layer.
>
> I agree that extending it is equal coding effort to adding an internal
> layer because you'll need to refactor qmp_transaction() a bit to really
> support additional action types.
>
> But it's the right thing to do.  Don't add unnecessary layers just
> because writing new code is more fun than extending existing code.
>
  If this layer is not added but depending only qmp_transaction, there
will be many "if else" fragment. I have tried that and the code
is awkful, this layer did not bring extra burden only make what
happens inside qmp_transaction clearer, I did not add this layer just
for fun.


>>    Actually I started up by use qmp_transaction as API, but soon
>> found that work is almost done around BlkTransactionStates, so
>> added a layer around it clearly.
>
> BlkTransactionStates is only relevant to external snapshots because they
> change the BlkDriverState chain and must be able to undo that.
>
> For internal snapshots you simply need to store the snapshot name so you
> can delete it if there is an error partway through.  (BTW it's not
> possible to completely restore state if you allow overwriting existing
> internal snapshots unless you do something like taking a backup snapshot
> of the existing snapshot first!)
>
   Still I need BlkTransactionStates to record BlkDriverState because
the granularity is block device. And yes it is not possible to
completely restore state in overwrite case, which can be solved later
and mark it in comments now.

> Stefan
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Stefan Hajnoczi]

On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
> 于 2013-1-10 20:41, Stefan Hajnoczi 写道:
> >On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
> >>于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> >>>On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
> >>>>   This patch switch to internal common API to take group external
> >>>>snapshots from qmp_transaction interface. qmp layer simply does
> >>>>a translation from user input.
> >>>>
> >>>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>---
> >>>>  blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
> >>>>  1 files changed, 87 insertions(+), 128 deletions(-)
> >>>
> >>>An internal API for snapshots is not necessary.  qmp_transaction() is
> >>>already usable both from the monitor and C code.
> >>>
> >>>The QAPI code generator creates structs that can be accessed directly
> >>>from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
> >>>the snapshot API.  It just doesn't support internal snapshots yet, which
> >>>is what you are trying to add.
> >>>
> >>>To add internal snapshot support, define a BlockdevInternalSnapshot type
> >>>in qapi-schema.json and add internal snapshot support in
> >>>qmp_transaction().
> >>>
> >>>qmp_transaction() was designed with this in mind from the beginning and
> >>>dispatches based on BlockdevAction->kind.
> >>>
> >>>The patch series will become much smaller while still adding internal
> >>>snapshot support.
> >>>
> >>>Stefan
> >>>
> >>
> >>   As API, qmp_transaction have following disadvantages:
> >>1) interface is based on string not data type inside qemu, that means
> >>other function calling it result in: bdrv->string->bdrv
> >
> >Use bdrv_get_device_name().  You already need to fill in filename or
> >snapshot name strings.  This is not a big disadvantage.
> >
>   Yes, not a big disadvantage, but why not save string operation but
> use (bdrv*) as much as possible?
> 
> what happens will be:
> 
> hmp-snapshot
>     |
> qmp-snapshot
>     |---------
>              |
>         qmp-transaction            savevm(may be other..)
>              |----------------------|
>                             |
>               internal transaction layer

Saving the string operation is not worth duplicating the API.

> >>2) all capability are forced to be exposed.
> >
> >Is there something you cannot expose?
> >
>   As other component in qemu can use it, some option may
> be used only in qemu not to user. For eg, vm-state-size.

When we hit a limitation of QAPI then it needs to be extended.  I'm sure
there's a solution for splitting or hiding parts of the QAPI generated
API.

> >>3) need structure to record each transaction state, such as
> >>BlkTransactionStates. Extending it is equal to add an internal layer.
> >
> >I agree that extending it is equal coding effort to adding an internal
> >layer because you'll need to refactor qmp_transaction() a bit to really
> >support additional action types.
> >
> >But it's the right thing to do.  Don't add unnecessary layers just
> >because writing new code is more fun than extending existing code.
> >
>  If this layer is not added but depending only qmp_transaction, there
> will be many "if else" fragment. I have tried that and the code
> is awkful, this layer did not bring extra burden only make what
> happens inside qmp_transaction clearer, I did not add this layer just
> for fun.
> 
> 
> >>   Actually I started up by use qmp_transaction as API, but soon
> >>found that work is almost done around BlkTransactionStates, so
> >>added a layer around it clearly.

The qmp_transaction() implementation can be changed, I'm not saying you
have to hack in more if statements.  It's cleanest to introduce a
BdrvActionOps abstraction:

typedef struct BdrvActionOps BdrvActionOps;
typedef struct BdrvTransactionState {
    const BdrvActionOps *ops;
    QLIST_ENTRY(BdrvTransactionState);
} BdrvTransactionState;

struct BdrvActionOps {
    int (*prepare)(BdrvTransactionState *s, ...);
    int (*commit)(BdrvTransactionState *s, ...);
    int (*rollback)(BdrvTransactionState *s, ...);
};

BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);

Then qmp_transaction() can be generic code that steps through the
transactions.  This is similar to what your series does and I think it's
the right direction.

But please don't duplicate the qmp_transaction() and
BlockdevAction/BlockdevActionList APIs.  In other words, change the
engine, not the whole car.

Stefan

Re: [Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Luiz Capitulino]

On Thu, 10 Jan 2013 14:01:27 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-1-10 6:34, Eric Blake 写道:
> > On 01/07/2013 12:27 AM, Wenchao Xia wrote:
> >>    These patch added a seperated layer to take internal or external snapshots
> >> in a unified way, the granularity is block device, so other functions can
> >> just combine the request and submit, such as group snapshot, savevm.
> >>
> >> Total goal are:
> >>    Live back up vm in external or internal image, which need three functions:
> >> 1 live snapshot block device internal/external.
> >> 2 live save vmstate internal/external.
> >> 3 combination of the function unit.
> >>
> >>    This serial provide part one.
> >>
> >
> > The design on this series needs to be coordinated with Pavel's
> > attempts[1] to convert the existing HMP snapshot commands into QMP.  I
> > don't want to spend too much time reviewing two different divergent
> > designs that both have the same goal of finally managing snapshots under
> > QMP.
> >
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01326.html
> >
>    A bit difference: This serial are taking internal/external block
> snapshots but Pavel's are focusing are vm whole internal snapshots. It

Pavel's work makes savevm, loadvm and delvm available in QMP. Also note
that we do have blockdev-snapshot-sync. How does this series relate to it?

I don't know if it's just me but, honestly speaking, I'm getting confused
about the various proposals and APIs for snapshots.

IMHO, it's time to go back a bit from code and discuss the status of the
current APIs, what's missing and what the proposals are.

This should also include writing a wiki page, as Anthony has done for
previous ideas (which worked quite well).

Re: [Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Wenchao Xia]

于 2013-1-11 21:56, Luiz Capitulino 写道:
> On Thu, 10 Jan 2013 14:01:27 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-1-10 6:34, Eric Blake 写道:
>>> On 01/07/2013 12:27 AM, Wenchao Xia wrote:
>>>>     These patch added a seperated layer to take internal or external snapshots
>>>> in a unified way, the granularity is block device, so other functions can
>>>> just combine the request and submit, such as group snapshot, savevm.
>>>>
>>>> Total goal are:
>>>>     Live back up vm in external or internal image, which need three functions:
>>>> 1 live snapshot block device internal/external.
>>>> 2 live save vmstate internal/external.
>>>> 3 combination of the function unit.
>>>>
>>>>     This serial provide part one.
>>>>
>>>
>>> The design on this series needs to be coordinated with Pavel's
>>> attempts[1] to convert the existing HMP snapshot commands into QMP.  I
>>> don't want to spend too much time reviewing two different divergent
>>> designs that both have the same goal of finally managing snapshots under
>>> QMP.
>>>
>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01326.html
>>>
>>     A bit difference: This serial are taking internal/external block
>> snapshots but Pavel's are focusing are vm whole internal snapshots. It
>
> Pavel's work makes savevm, loadvm and delvm available in QMP. Also note
> that we do have blockdev-snapshot-sync. How does this series relate to it?
>
> I don't know if it's just me but, honestly speaking, I'm getting confused
> about the various proposals and APIs for snapshots.
>
> IMHO, it's time to go back a bit from code and discuss the status of the
> current APIs, what's missing and what the proposals are.
>
> This should also include writing a wiki page, as Anthony has done for
> previous ideas (which worked quite well).
>
   I agree, can you help create an account on qemu wiki for me?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-1-11 17:12, Stefan Hajnoczi 写道:
> On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
>> 于 2013-1-10 20:41, Stefan Hajnoczi 写道:
>>> On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
>>>> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
>>>>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>>>>>>    This patch switch to internal common API to take group external
>>>>>> snapshots from qmp_transaction interface. qmp layer simply does
>>>>>> a translation from user input.
>>>>>>
>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>   blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
>>>>>>   1 files changed, 87 insertions(+), 128 deletions(-)
>>>>>
>>>>> An internal API for snapshots is not necessary.  qmp_transaction() is
>>>>> already usable both from the monitor and C code.
>>>>>
>>>>> The QAPI code generator creates structs that can be accessed directly
>>>> >from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
>>>>> the snapshot API.  It just doesn't support internal snapshots yet, which
>>>>> is what you are trying to add.
>>>>>
>>>>> To add internal snapshot support, define a BlockdevInternalSnapshot type
>>>>> in qapi-schema.json and add internal snapshot support in
>>>>> qmp_transaction().
>>>>>
>>>>> qmp_transaction() was designed with this in mind from the beginning and
>>>>> dispatches based on BlockdevAction->kind.
>>>>>
>>>>> The patch series will become much smaller while still adding internal
>>>>> snapshot support.
>>>>>
>>>>> Stefan
>>>>>
>>>>
>>>>    As API, qmp_transaction have following disadvantages:
>>>> 1) interface is based on string not data type inside qemu, that means
>>>> other function calling it result in: bdrv->string->bdrv
>>>
>>> Use bdrv_get_device_name().  You already need to fill in filename or
>>> snapshot name strings.  This is not a big disadvantage.
>>>
>>    Yes, not a big disadvantage, but why not save string operation but
>> use (bdrv*) as much as possible?
>>
>> what happens will be:
>>
>> hmp-snapshot
>>      |
>> qmp-snapshot
>>      |---------
>>               |
>>          qmp-transaction            savevm(may be other..)
>>               |----------------------|
>>                              |
>>                internal transaction layer
>
> Saving the string operation is not worth duplicating the API.
>
   I agree with you for this line:), but,  it is a weight on the balance
of choice, pls consider it together with issues below.

>>>> 2) all capability are forced to be exposed.
>>>
>>> Is there something you cannot expose?
>>>
>>    As other component in qemu can use it, some option may
>> be used only in qemu not to user. For eg, vm-state-size.
>
> When we hit a limitation of QAPI then it needs to be extended.  I'm sure
> there's a solution for splitting or hiding parts of the QAPI generated
> API.
>
   I can't think it out now, it seems to be a bit tricky.

>>>> 3) need structure to record each transaction state, such as
>>>> BlkTransactionStates. Extending it is equal to add an internal layer.
>>>
>>> I agree that extending it is equal coding effort to adding an internal
>>> layer because you'll need to refactor qmp_transaction() a bit to really
>>> support additional action types.
>>>
>>> But it's the right thing to do.  Don't add unnecessary layers just
>>> because writing new code is more fun than extending existing code.
>>>
>>   If this layer is not added but depending only qmp_transaction, there
>> will be many "if else" fragment. I have tried that and the code
>> is awkful, this layer did not bring extra burden only make what
>> happens inside qmp_transaction clearer, I did not add this layer just
>> for fun.
>>
>>
>>>>    Actually I started up by use qmp_transaction as API, but soon
>>>> found that work is almost done around BlkTransactionStates, so
>>>> added a layer around it clearly.
>
> The qmp_transaction() implementation can be changed, I'm not saying you
> have to hack in more if statements.  It's cleanest to introduce a
> BdrvActionOps abstraction:
>
> typedef struct BdrvActionOps BdrvActionOps;
> typedef struct BdrvTransactionState {
>      const BdrvActionOps *ops;
>      QLIST_ENTRY(BdrvTransactionState);
> } BdrvTransactionState;
>
> struct BdrvActionOps {
>      int (*prepare)(BdrvTransactionState *s, ...);
>      int (*commit)(BdrvTransactionState *s, ...);
>      int (*rollback)(BdrvTransactionState *s, ...);
> };
>
> BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
>
> Then qmp_transaction() can be generic code that steps through the
> transactions.
   With internal API, qmp_transaction can still be generic code with
a translate from bdrv* to char* at caller level.

   This is similar to what your series does and I think it's
> the right direction.
>
> But please don't duplicate the qmp_transaction() and
> BlockdevAction/BlockdevActionList APIs.  In other words, change the
> engine, not the whole car.
>
> Stefan
>

   If my understanding is correct, the BdrvActionOps need to be extended
as following:
struct BdrvActionOps {
      /* need following for callback functions */
      const char *sn_name;
      BlockDriverState *bs;
      ...
      int (*prepare)(BdrvTransactionState *s, ...);
      int (*commit)(BdrvTransactionState *s, ...);
      int (*rollback)(BdrvTransactionState *s, ...);
};
Or an opaque* should used for every BdrvActionOps.

Comparation:
The way above:
  1) translate from BlockdevAction to BdrvTransactionState by
     bdrv_transaction_create().
  2) enqueue BdrvTransactionState by
     some code.
  3) execute them by
     a new function, name it as BdrvActionOpsRun().

Internal API way:
  1) translate BlockdevAction to BlkTransStates by
     fill_blk_trs().
  2) enqueue BlkTransStates to BlkTransStates by
     add_transaction().
  3) execute them by
     submit_transaction().

   It seems the way above will end as something like an internal
layer, but without clear APIs tips what it is doing. Please reconsider
the advantages about a clear internal API layer.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Stefan Hajnoczi]

On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
> 于 2013-1-11 17:12, Stefan Hajnoczi 写道:
> >On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
> >>于 2013-1-10 20:41, Stefan Hajnoczi 写道:
> >>>On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
> >>>>于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> >>>>>On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
> >>>>>>   This patch switch to internal common API to take group external
> >>>>>>snapshots from qmp_transaction interface. qmp layer simply does
> >>>>>>a translation from user input.
> >>>>>>
> >>>>>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>>>---
> >>>>>>  blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
> >>>>>>  1 files changed, 87 insertions(+), 128 deletions(-)
> >>>>>
> >>>>>An internal API for snapshots is not necessary.  qmp_transaction() is
> >>>>>already usable both from the monitor and C code.
> >>>>>
> >>>>>The QAPI code generator creates structs that can be accessed directly
> >>>>>from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
> >>>>>the snapshot API.  It just doesn't support internal snapshots yet, which
> >>>>>is what you are trying to add.
> >>>>>
> >>>>>To add internal snapshot support, define a BlockdevInternalSnapshot type
> >>>>>in qapi-schema.json and add internal snapshot support in
> >>>>>qmp_transaction().
> >>>>>
> >>>>>qmp_transaction() was designed with this in mind from the beginning and
> >>>>>dispatches based on BlockdevAction->kind.
> >>>>>
> >>>>>The patch series will become much smaller while still adding internal
> >>>>>snapshot support.
> >>>>>
> >>>>>Stefan
> >>>>>
> >>>>
> >>>>   As API, qmp_transaction have following disadvantages:
> >>>>1) interface is based on string not data type inside qemu, that means
> >>>>other function calling it result in: bdrv->string->bdrv
> >>>
> >>>Use bdrv_get_device_name().  You already need to fill in filename or
> >>>snapshot name strings.  This is not a big disadvantage.
> >>>
> >>   Yes, not a big disadvantage, but why not save string operation but
> >>use (bdrv*) as much as possible?
> >>
> >>what happens will be:
> >>
> >>hmp-snapshot
> >>     |
> >>qmp-snapshot
> >>     |---------
> >>              |
> >>         qmp-transaction            savevm(may be other..)
> >>              |----------------------|
> >>                             |
> >>               internal transaction layer
> >
> >Saving the string operation is not worth duplicating the API.
> >
>   I agree with you for this line:), but,  it is a weight on the balance
> of choice, pls consider it together with issues below.
> 
> >>>>2) all capability are forced to be exposed.
> >>>
> >>>Is there something you cannot expose?
> >>>
> >>   As other component in qemu can use it, some option may
> >>be used only in qemu not to user. For eg, vm-state-size.
> >
> >When we hit a limitation of QAPI then it needs to be extended.  I'm sure
> >there's a solution for splitting or hiding parts of the QAPI generated
> >API.
> >
>   I can't think it out now, it seems to be a bit tricky.
> 
> >>>>3) need structure to record each transaction state, such as
> >>>>BlkTransactionStates. Extending it is equal to add an internal layer.
> >>>
> >>>I agree that extending it is equal coding effort to adding an internal
> >>>layer because you'll need to refactor qmp_transaction() a bit to really
> >>>support additional action types.
> >>>
> >>>But it's the right thing to do.  Don't add unnecessary layers just
> >>>because writing new code is more fun than extending existing code.
> >>>
> >>  If this layer is not added but depending only qmp_transaction, there
> >>will be many "if else" fragment. I have tried that and the code
> >>is awkful, this layer did not bring extra burden only make what
> >>happens inside qmp_transaction clearer, I did not add this layer just
> >>for fun.
> >>
> >>
> >>>>   Actually I started up by use qmp_transaction as API, but soon
> >>>>found that work is almost done around BlkTransactionStates, so
> >>>>added a layer around it clearly.
> >
> >The qmp_transaction() implementation can be changed, I'm not saying you
> >have to hack in more if statements.  It's cleanest to introduce a
> >BdrvActionOps abstraction:
> >
> >typedef struct BdrvActionOps BdrvActionOps;
> >typedef struct BdrvTransactionState {
> >     const BdrvActionOps *ops;
> >     QLIST_ENTRY(BdrvTransactionState);
> >} BdrvTransactionState;
> >
> >struct BdrvActionOps {
> >     int (*prepare)(BdrvTransactionState *s, ...);
> >     int (*commit)(BdrvTransactionState *s, ...);
> >     int (*rollback)(BdrvTransactionState *s, ...);
> >};
> >
> >BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
> >
> >Then qmp_transaction() can be generic code that steps through the
> >transactions.
>   With internal API, qmp_transaction can still be generic code with
> a translate from bdrv* to char* at caller level.
> 
>   This is similar to what your series does and I think it's
> >the right direction.
> >
> >But please don't duplicate the qmp_transaction() and
> >BlockdevAction/BlockdevActionList APIs.  In other words, change the
> >engine, not the whole car.
> >
> >Stefan
> >
> 
>   If my understanding is correct, the BdrvActionOps need to be extended
> as following:
> struct BdrvActionOps {
>      /* need following for callback functions */
>      const char *sn_name;
>      BlockDriverState *bs;
>      ...
>      int (*prepare)(BdrvTransactionState *s, ...);
>      int (*commit)(BdrvTransactionState *s, ...);
>      int (*rollback)(BdrvTransactionState *s, ...);
> };
> Or an opaque* should used for every BdrvActionOps.

It is nice to keep *Ops structs read-only so they can be static const.
This way the ops are shared between all instances of the same action
type.  Also the function pointers can be in read-only memory pages,
which is a slight security win since it prevents memory corruption
exploits from taking advantage of function pointers to execute arbitrary
code.

In the pseudo-code I posted the sn_name or bs fields go into an
action-specific state struct:

typedef struct {
    BdrvTransactionState common;
    char *backup_sn_name;
} InternalSnapshotTransactionState;

typedef struct {
    BdrvTransactionState common;
    BlockDriverState *old_bs;
    BlockDriverState *new_bs;
} ExternalSnapshotTransactionState;

> Comparation:
> The way above:
>  1) translate from BlockdevAction to BdrvTransactionState by
>     bdrv_transaction_create().
>  2) enqueue BdrvTransactionState by
>     some code.
>  3) execute them by
>     a new function, name it as BdrvActionOpsRun().

If you include .prepare() in the transaction creation, then it becomes
simpler:

states = []
for action in actions:
    result = bdrv_transaction_create(action)  # invokes .prepare()
    if result is error:
        for state in states:
	    state.rollback()
	return
    states.append(result)
for state in states:
    state.commit()

Because we don't wait until BdrvActionOpsRun() before processing the
transaction, there's no need to translate from BlockdevAction to
BdrvTransactionState.  The BdrvTransactionState struct really only has
state required to commit/rollback the transaction.

(Even if it becomes necessary to keep information from BlockdevAction
after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
duplicate it.)

> Internal API way:
>  1) translate BlockdevAction to BlkTransStates by
>     fill_blk_trs().
>  2) enqueue BlkTransStates to BlkTransStates by
>     add_transaction().
>  3) execute them by
>     submit_transaction().
> 
>   It seems the way above will end as something like an internal
> layer, but without clear APIs tips what it is doing. Please reconsider
> the advantages about a clear internal API layer.

I'm not convinced by the internal API approach.  It took me a while when
reviewing the code before I understood what was actually going on
because of the qmp_transaction() and BlockdevAction duplication code.

I see the internal API approach as an unnecessary layer of indirection.
It makes the code more complicated to understand and maintain.  Next
time we add something to qmp_transaction() it would also be necessary to
duplicate that change for the internal API.  It creates unnecessary
work.

Just embrace QAPI, the point of it was to eliminate these external <->
internal translations we were doing all the time.

Stefan

Re: [Qemu-devel] [PATCH V2 00/10] snapshot: take block snapshots in unified way

[Stefan Hajnoczi]

On Mon, Jan 14, 2013 at 10:09:45AM +0800, Wenchao Xia wrote:
> 于 2013-1-11 21:56, Luiz Capitulino 写道:
> >On Thu, 10 Jan 2013 14:01:27 +0800
> >Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >>于 2013-1-10 6:34, Eric Blake 写道:
> >>>On 01/07/2013 12:27 AM, Wenchao Xia wrote:
> >>>>    These patch added a seperated layer to take internal or external snapshots
> >>>>in a unified way, the granularity is block device, so other functions can
> >>>>just combine the request and submit, such as group snapshot, savevm.
> >>>>
> >>>>Total goal are:
> >>>>    Live back up vm in external or internal image, which need three functions:
> >>>>1 live snapshot block device internal/external.
> >>>>2 live save vmstate internal/external.
> >>>>3 combination of the function unit.
> >>>>
> >>>>    This serial provide part one.
> >>>>
> >>>
> >>>The design on this series needs to be coordinated with Pavel's
> >>>attempts[1] to convert the existing HMP snapshot commands into QMP.  I
> >>>don't want to spend too much time reviewing two different divergent
> >>>designs that both have the same goal of finally managing snapshots under
> >>>QMP.
> >>>
> >>>[1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01326.html
> >>>
> >>    A bit difference: This serial are taking internal/external block
> >>snapshots but Pavel's are focusing are vm whole internal snapshots. It
> >
> >Pavel's work makes savevm, loadvm and delvm available in QMP. Also note
> >that we do have blockdev-snapshot-sync. How does this series relate to it?
> >
> >I don't know if it's just me but, honestly speaking, I'm getting confused
> >about the various proposals and APIs for snapshots.
> >
> >IMHO, it's time to go back a bit from code and discuss the status of the
> >current APIs, what's missing and what the proposals are.
> >
> >This should also include writing a wiki page, as Anthony has done for
> >previous ideas (which worked quite well).
> >
>   I agree, can you help create an account on qemu wiki for me?

I have created an account for you.  Please see the private message.

Stefan

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-1-14 18:06, Stefan Hajnoczi 写道:
> On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
>> 于 2013-1-11 17:12, Stefan Hajnoczi 写道:
>>> On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
>>>> 于 2013-1-10 20:41, Stefan Hajnoczi 写道:
>>>>> On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
>>>>>> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
>>>>>>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>>>>>>>>    This patch switch to internal common API to take group external
>>>>>>>> snapshots from qmp_transaction interface. qmp layer simply does
>>>>>>>> a translation from user input.
>>>>>>>>
>>>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>>>>> ---
>>>>>>>>   blockdev.c |  215 ++++++++++++++++++++++++------------------------------------
>>>>>>>>   1 files changed, 87 insertions(+), 128 deletions(-)
>>>>>>>
>>>>>>> An internal API for snapshots is not necessary.  qmp_transaction() is
>>>>>>> already usable both from the monitor and C code.
>>>>>>>
>>>>>>> The QAPI code generator creates structs that can be accessed directly
>>>>>> >from C.  qmp_transaction(), BlockdevAction, and BlockdevActionList *is*
>>>>>>> the snapshot API.  It just doesn't support internal snapshots yet, which
>>>>>>> is what you are trying to add.
>>>>>>>
>>>>>>> To add internal snapshot support, define a BlockdevInternalSnapshot type
>>>>>>> in qapi-schema.json and add internal snapshot support in
>>>>>>> qmp_transaction().
>>>>>>>
>>>>>>> qmp_transaction() was designed with this in mind from the beginning and
>>>>>>> dispatches based on BlockdevAction->kind.
>>>>>>>
>>>>>>> The patch series will become much smaller while still adding internal
>>>>>>> snapshot support.
>>>>>>>
>>>>>>> Stefan
>>>>>>>
>>>>>>
>>>>>>    As API, qmp_transaction have following disadvantages:
>>>>>> 1) interface is based on string not data type inside qemu, that means
>>>>>> other function calling it result in: bdrv->string->bdrv
>>>>>
>>>>> Use bdrv_get_device_name().  You already need to fill in filename or
>>>>> snapshot name strings.  This is not a big disadvantage.
>>>>>
>>>>    Yes, not a big disadvantage, but why not save string operation but
>>>> use (bdrv*) as much as possible?
>>>>
>>>> what happens will be:
>>>>
>>>> hmp-snapshot
>>>>      |
>>>> qmp-snapshot
>>>>      |---------
>>>>               |
>>>>          qmp-transaction            savevm(may be other..)
>>>>               |----------------------|
>>>>                              |
>>>>                internal transaction layer
>>>
>>> Saving the string operation is not worth duplicating the API.
>>>
>>    I agree with you for this line:), but,  it is a weight on the balance
>> of choice, pls consider it together with issues below.
>>
>>>>>> 2) all capability are forced to be exposed.
>>>>>
>>>>> Is there something you cannot expose?
>>>>>
>>>>    As other component in qemu can use it, some option may
>>>> be used only in qemu not to user. For eg, vm-state-size.
>>>
>>> When we hit a limitation of QAPI then it needs to be extended.  I'm sure
>>> there's a solution for splitting or hiding parts of the QAPI generated
>>> API.
>>>
>>    I can't think it out now, it seems to be a bit tricky.
>>
>>>>>> 3) need structure to record each transaction state, such as
>>>>>> BlkTransactionStates. Extending it is equal to add an internal layer.
>>>>>
>>>>> I agree that extending it is equal coding effort to adding an internal
>>>>> layer because you'll need to refactor qmp_transaction() a bit to really
>>>>> support additional action types.
>>>>>
>>>>> But it's the right thing to do.  Don't add unnecessary layers just
>>>>> because writing new code is more fun than extending existing code.
>>>>>
>>>>   If this layer is not added but depending only qmp_transaction, there
>>>> will be many "if else" fragment. I have tried that and the code
>>>> is awkful, this layer did not bring extra burden only make what
>>>> happens inside qmp_transaction clearer, I did not add this layer just
>>>> for fun.
>>>>
>>>>
>>>>>>    Actually I started up by use qmp_transaction as API, but soon
>>>>>> found that work is almost done around BlkTransactionStates, so
>>>>>> added a layer around it clearly.
>>>
>>> The qmp_transaction() implementation can be changed, I'm not saying you
>>> have to hack in more if statements.  It's cleanest to introduce a
>>> BdrvActionOps abstraction:
>>>
>>> typedef struct BdrvActionOps BdrvActionOps;
>>> typedef struct BdrvTransactionState {
>>>      const BdrvActionOps *ops;
>>>      QLIST_ENTRY(BdrvTransactionState);
>>> } BdrvTransactionState;
>>>
>>> struct BdrvActionOps {
>>>      int (*prepare)(BdrvTransactionState *s, ...);
>>>      int (*commit)(BdrvTransactionState *s, ...);
>>>      int (*rollback)(BdrvTransactionState *s, ...);
>>> };
>>>
>>> BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
>>>
>>> Then qmp_transaction() can be generic code that steps through the
>>> transactions.
>>    With internal API, qmp_transaction can still be generic code with
>> a translate from bdrv* to char* at caller level.
>>
>>    This is similar to what your series does and I think it's
>>> the right direction.
>>>
>>> But please don't duplicate the qmp_transaction() and
>>> BlockdevAction/BlockdevActionList APIs.  In other words, change the
>>> engine, not the whole car.
>>>
>>> Stefan
>>>
>>
>>    If my understanding is correct, the BdrvActionOps need to be extended
>> as following:
>> struct BdrvActionOps {
>>       /* need following for callback functions */
>>       const char *sn_name;
>>       BlockDriverState *bs;
>>       ...
>>       int (*prepare)(BdrvTransactionState *s, ...);
>>       int (*commit)(BdrvTransactionState *s, ...);
>>       int (*rollback)(BdrvTransactionState *s, ...);
>> };
>> Or an opaque* should used for every BdrvActionOps.
>
> It is nice to keep *Ops structs read-only so they can be static const.
> This way the ops are shared between all instances of the same action
> type.  Also the function pointers can be in read-only memory pages,
> which is a slight security win since it prevents memory corruption
> exploits from taking advantage of function pointers to execute arbitrary
> code.
>
   Seems good, I will package callback functions into *Ops, thanks.

> In the pseudo-code I posted the sn_name or bs fields go into an
> action-specific state struct:
>
> typedef struct {
>      BdrvTransactionState common;
>      char *backup_sn_name;
> } InternalSnapshotTransactionState;
>
> typedef struct {
>      BdrvTransactionState common;
>      BlockDriverState *old_bs;
>      BlockDriverState *new_bs;
> } ExternalSnapshotTransactionState;
>
>> Comparation:
>> The way above:
>>   1) translate from BlockdevAction to BdrvTransactionState by
>>      bdrv_transaction_create().
>>   2) enqueue BdrvTransactionState by
>>      some code.
>>   3) execute them by
>>      a new function, name it as BdrvActionOpsRun().
>
> If you include .prepare() in the transaction creation, then it becomes
> simpler:
>
> states = []
> for action in actions:
>      result = bdrv_transaction_create(action)  # invokes .prepare()
>      if result is error:
>          for state in states:
> 	    state.rollback()
> 	return
>      states.append(result)
> for state in states:
>      state.commit()
>
> Because we don't wait until BdrvActionOpsRun() before processing the
> transaction, there's no need to translate from BlockdevAction to
> BdrvTransactionState.  The BdrvTransactionState struct really only has
> state required to commit/rollback the transaction.
>
> (Even if it becomes necessary to keep information from BlockdevAction
> after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
> duplicate it.)
>
   OK, *BlockdevAction plus *BlockDriverState and some other
data used internal will be added in states.

>> Internal API way:
>>   1) translate BlockdevAction to BlkTransStates by
>>      fill_blk_trs().
>>   2) enqueue BlkTransStates to BlkTransStates by
>>      add_transaction().
>>   3) execute them by
>>      submit_transaction().
>>
>>    It seems the way above will end as something like an internal
>> layer, but without clear APIs tips what it is doing. Please reconsider
>> the advantages about a clear internal API layer.
>
> I'm not convinced by the internal API approach.  It took me a while when
> reviewing the code before I understood what was actually going on
> because of the qmp_transaction() and BlockdevAction duplication code.
>
> I see the internal API approach as an unnecessary layer of indirection.
> It makes the code more complicated to understand and maintain.  Next
> time we add something to qmp_transaction() it would also be necessary to
> duplicate that change for the internal API.  It creates unnecessary
> work.
>
   Basic process is almost the same in two approaches, I'd like to
adjust the code to avoid data duplication as much as possible, and
see if some function can be removed when code keeps clear, in next
version.

> Just embrace QAPI, the point of it was to eliminate these external <->
> internal translations we were doing all the time.
>
> Stefan
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-1-15 15:03, Wenchao Xia 写道:
> 于 2013-1-14 18:06, Stefan Hajnoczi 写道:
>> On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
>>> 于 2013-1-11 17:12, Stefan Hajnoczi 写道:
>>>> On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
>>>>> 于 2013-1-10 20:41, Stefan Hajnoczi 写道:
>>>>>> On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
>>>>>>> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
>>>>>>>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>>>>>>>>>    This patch switch to internal common API to take group external
>>>>>>>>> snapshots from qmp_transaction interface. qmp layer simply does
>>>>>>>>> a translation from user input.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>>>>>> ---
>>>>>>>>>   blockdev.c |  215
>>>>>>>>> ++++++++++++++++++++++++------------------------------------
>>>>>>>>>   1 files changed, 87 insertions(+), 128 deletions(-)
>>>>>>>>
>>>>>>>> An internal API for snapshots is not necessary.
>>>>>>>> qmp_transaction() is
>>>>>>>> already usable both from the monitor and C code.
>>>>>>>>
>>>>>>>> The QAPI code generator creates structs that can be accessed
>>>>>>>> directly
>>>>>>> >from C.  qmp_transaction(), BlockdevAction, and
>>>>>>> BlockdevActionList *is*
>>>>>>>> the snapshot API.  It just doesn't support internal snapshots
>>>>>>>> yet, which
>>>>>>>> is what you are trying to add.
>>>>>>>>
>>>>>>>> To add internal snapshot support, define a
>>>>>>>> BlockdevInternalSnapshot type
>>>>>>>> in qapi-schema.json and add internal snapshot support in
>>>>>>>> qmp_transaction().
>>>>>>>>
>>>>>>>> qmp_transaction() was designed with this in mind from the
>>>>>>>> beginning and
>>>>>>>> dispatches based on BlockdevAction->kind.
>>>>>>>>
>>>>>>>> The patch series will become much smaller while still adding
>>>>>>>> internal
>>>>>>>> snapshot support.
>>>>>>>>
>>>>>>>> Stefan
>>>>>>>>
>>>>>>>
>>>>>>>    As API, qmp_transaction have following disadvantages:
>>>>>>> 1) interface is based on string not data type inside qemu, that
>>>>>>> means
>>>>>>> other function calling it result in: bdrv->string->bdrv
>>>>>>
>>>>>> Use bdrv_get_device_name().  You already need to fill in filename or
>>>>>> snapshot name strings.  This is not a big disadvantage.
>>>>>>
>>>>>    Yes, not a big disadvantage, but why not save string operation but
>>>>> use (bdrv*) as much as possible?
>>>>>
>>>>> what happens will be:
>>>>>
>>>>> hmp-snapshot
>>>>>      |
>>>>> qmp-snapshot
>>>>>      |---------
>>>>>               |
>>>>>          qmp-transaction            savevm(may be other..)
>>>>>               |----------------------|
>>>>>                              |
>>>>>                internal transaction layer
>>>>
>>>> Saving the string operation is not worth duplicating the API.
>>>>
>>>    I agree with you for this line:), but,  it is a weight on the balance
>>> of choice, pls consider it together with issues below.
>>>
>>>>>>> 2) all capability are forced to be exposed.
>>>>>>
>>>>>> Is there something you cannot expose?
>>>>>>
>>>>>    As other component in qemu can use it, some option may
>>>>> be used only in qemu not to user. For eg, vm-state-size.
>>>>
>>>> When we hit a limitation of QAPI then it needs to be extended.  I'm
>>>> sure
>>>> there's a solution for splitting or hiding parts of the QAPI generated
>>>> API.
>>>>
>>>    I can't think it out now, it seems to be a bit tricky.
>>>
>>>>>>> 3) need structure to record each transaction state, such as
>>>>>>> BlkTransactionStates. Extending it is equal to add an internal
>>>>>>> layer.
>>>>>>
>>>>>> I agree that extending it is equal coding effort to adding an
>>>>>> internal
>>>>>> layer because you'll need to refactor qmp_transaction() a bit to
>>>>>> really
>>>>>> support additional action types.
>>>>>>
>>>>>> But it's the right thing to do.  Don't add unnecessary layers just
>>>>>> because writing new code is more fun than extending existing code.
>>>>>>
>>>>>   If this layer is not added but depending only qmp_transaction, there
>>>>> will be many "if else" fragment. I have tried that and the code
>>>>> is awkful, this layer did not bring extra burden only make what
>>>>> happens inside qmp_transaction clearer, I did not add this layer just
>>>>> for fun.
>>>>>
>>>>>
>>>>>>>    Actually I started up by use qmp_transaction as API, but soon
>>>>>>> found that work is almost done around BlkTransactionStates, so
>>>>>>> added a layer around it clearly.
>>>>
>>>> The qmp_transaction() implementation can be changed, I'm not saying you
>>>> have to hack in more if statements.  It's cleanest to introduce a
>>>> BdrvActionOps abstraction:
>>>>
>>>> typedef struct BdrvActionOps BdrvActionOps;
>>>> typedef struct BdrvTransactionState {
>>>>      const BdrvActionOps *ops;
>>>>      QLIST_ENTRY(BdrvTransactionState);
>>>> } BdrvTransactionState;
>>>>
>>>> struct BdrvActionOps {
>>>>      int (*prepare)(BdrvTransactionState *s, ...);
>>>>      int (*commit)(BdrvTransactionState *s, ...);
>>>>      int (*rollback)(BdrvTransactionState *s, ...);
>>>> };
>>>>
>>>> BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
>>>>
>>>> Then qmp_transaction() can be generic code that steps through the
>>>> transactions.
>>>    With internal API, qmp_transaction can still be generic code with
>>> a translate from bdrv* to char* at caller level.
>>>
>>>    This is similar to what your series does and I think it's
>>>> the right direction.
>>>>
>>>> But please don't duplicate the qmp_transaction() and
>>>> BlockdevAction/BlockdevActionList APIs.  In other words, change the
>>>> engine, not the whole car.
>>>>
>>>> Stefan
>>>>
>>>
>>>    If my understanding is correct, the BdrvActionOps need to be extended
>>> as following:
>>> struct BdrvActionOps {
>>>       /* need following for callback functions */
>>>       const char *sn_name;
>>>       BlockDriverState *bs;
>>>       ...
>>>       int (*prepare)(BdrvTransactionState *s, ...);
>>>       int (*commit)(BdrvTransactionState *s, ...);
>>>       int (*rollback)(BdrvTransactionState *s, ...);
>>> };
>>> Or an opaque* should used for every BdrvActionOps.
>>
>> It is nice to keep *Ops structs read-only so they can be static const.
>> This way the ops are shared between all instances of the same action
>> type.  Also the function pointers can be in read-only memory pages,
>> which is a slight security win since it prevents memory corruption
>> exploits from taking advantage of function pointers to execute arbitrary
>> code.
>>
>    Seems good, I will package callback functions into *Ops, thanks.
>
>> In the pseudo-code I posted the sn_name or bs fields go into an
>> action-specific state struct:
>>
>> typedef struct {
>>      BdrvTransactionState common;
>>      char *backup_sn_name;
>> } InternalSnapshotTransactionState;
>>
>> typedef struct {
>>      BdrvTransactionState common;
>>      BlockDriverState *old_bs;
>>      BlockDriverState *new_bs;
>> } ExternalSnapshotTransactionState;
>>
>>> Comparation:
>>> The way above:
>>>   1) translate from BlockdevAction to BdrvTransactionState by
>>>      bdrv_transaction_create().
>>>   2) enqueue BdrvTransactionState by
>>>      some code.
>>>   3) execute them by
>>>      a new function, name it as BdrvActionOpsRun().
>>
>> If you include .prepare() in the transaction creation, then it becomes
>> simpler:
>>
>> states = []
>> for action in actions:
>>      result = bdrv_transaction_create(action)  # invokes .prepare()
>>      if result is error:
>>          for state in states:
>>         state.rollback()
>>     return
>>      states.append(result)
>> for state in states:
>>      state.commit()
>>
>> Because we don't wait until BdrvActionOpsRun() before processing the
>> transaction, there's no need to translate from BlockdevAction to
>> BdrvTransactionState.  The BdrvTransactionState struct really only has
>> state required to commit/rollback the transaction.
>>
>> (Even if it becomes necessary to keep information from BlockdevAction
>> after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
>> duplicate it.)
>>
>    OK, *BlockdevAction plus *BlockDriverState and some other
> data used internal will be added in states.
>
>>> Internal API way:
>>>   1) translate BlockdevAction to BlkTransStates by
>>>      fill_blk_trs().
>>>   2) enqueue BlkTransStates to BlkTransStates by
>>>      add_transaction().
>>>   3) execute them by
>>>      submit_transaction().
>>>
>>>    It seems the way above will end as something like an internal
>>> layer, but without clear APIs tips what it is doing. Please reconsider
>>> the advantages about a clear internal API layer.
>>
>> I'm not convinced by the internal API approach.  It took me a while when
>> reviewing the code before I understood what was actually going on
>> because of the qmp_transaction() and BlockdevAction duplication code.
>>
>> I see the internal API approach as an unnecessary layer of indirection.
>> It makes the code more complicated to understand and maintain.  Next
>> time we add something to qmp_transaction() it would also be necessary to
>> duplicate that change for the internal API.  It creates unnecessary
>> work.
>>
>    Basic process is almost the same in two approaches, I'd like to
> adjust the code to avoid data duplication as much as possible, and
> see if some function can be removed when code keeps clear, in next
> version.
>
>> Just embrace QAPI, the point of it was to eliminate these external <->
>> internal translations we were doing all the time.
>>
>> Stefan
>>
>
>
Hi, Stefan
   I redesigned the structure, Following is the fake code:

typedef struct BdrvActionOps {
     /* check the request's validation, allocate p_opaque if needed */
     int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
     /* take the action */
     int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
     /* update emulator */
     int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
     /* cancel the action */
     int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
} BdrvActionOps;

typedef struct BlkTransactionStates {
     BlockdevAction *action;
     void *opaque;
     BdrvActionOps *ops;
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
} BlkTransactionStates;

/* call ops->check and return state* to be enqueued */
static BlkTransactionStates *transaction_create(BlockdevAction *action,
                                                 Error **errp);

void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
{
     BlockdevActionList *dev_entry = dev_list;
     BlkTransactionStates *state;

     while (NULL != dev_entry) {
         state = transaction_create(dev_entry->value, errp);
         /* enqueue */
         dev_entry = dev_entry->next;
     }

     /* use queue with submit, commit, rollback callback */
}


   In this way, parameter duplication is saved, but one problem remains:
parameter can't be hidden to user such as vm_state_size, but this would
not be a problem if hmp "savevm" use his own code about block snapshot
later, I mean not use qmp_transaction(). What do you think about the
design? Do you have a better way to solve this problem?


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Stefan Hajnoczi]

On Tue, Mar 12, 2013 at 04:30:41PM +0800, Wenchao Xia wrote:
> 于 2013-1-15 15:03, Wenchao Xia 写道:
> >于 2013-1-14 18:06, Stefan Hajnoczi 写道:
> >>On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
> >>>于 2013-1-11 17:12, Stefan Hajnoczi 写道:
> >>>>On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
> >>>>>于 2013-1-10 20:41, Stefan Hajnoczi 写道:
> >>>>>>On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
> >>>>>>>于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> >>>>>>>>On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
> >>>>>>>>>   This patch switch to internal common API to take group external
> >>>>>>>>>snapshots from qmp_transaction interface. qmp layer simply does
> >>>>>>>>>a translation from user input.
> >>>>>>>>>
> >>>>>>>>>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>>>>>>---
> >>>>>>>>>  blockdev.c |  215
> >>>>>>>>>++++++++++++++++++++++++------------------------------------
> >>>>>>>>>  1 files changed, 87 insertions(+), 128 deletions(-)
> >>>>>>>>
> >>>>>>>>An internal API for snapshots is not necessary.
> >>>>>>>>qmp_transaction() is
> >>>>>>>>already usable both from the monitor and C code.
> >>>>>>>>
> >>>>>>>>The QAPI code generator creates structs that can be accessed
> >>>>>>>>directly
> >>>>>>>>from C.  qmp_transaction(), BlockdevAction, and
> >>>>>>>BlockdevActionList *is*
> >>>>>>>>the snapshot API.  It just doesn't support internal snapshots
> >>>>>>>>yet, which
> >>>>>>>>is what you are trying to add.
> >>>>>>>>
> >>>>>>>>To add internal snapshot support, define a
> >>>>>>>>BlockdevInternalSnapshot type
> >>>>>>>>in qapi-schema.json and add internal snapshot support in
> >>>>>>>>qmp_transaction().
> >>>>>>>>
> >>>>>>>>qmp_transaction() was designed with this in mind from the
> >>>>>>>>beginning and
> >>>>>>>>dispatches based on BlockdevAction->kind.
> >>>>>>>>
> >>>>>>>>The patch series will become much smaller while still adding
> >>>>>>>>internal
> >>>>>>>>snapshot support.
> >>>>>>>>
> >>>>>>>>Stefan
> >>>>>>>>
> >>>>>>>
> >>>>>>>   As API, qmp_transaction have following disadvantages:
> >>>>>>>1) interface is based on string not data type inside qemu, that
> >>>>>>>means
> >>>>>>>other function calling it result in: bdrv->string->bdrv
> >>>>>>
> >>>>>>Use bdrv_get_device_name().  You already need to fill in filename or
> >>>>>>snapshot name strings.  This is not a big disadvantage.
> >>>>>>
> >>>>>   Yes, not a big disadvantage, but why not save string operation but
> >>>>>use (bdrv*) as much as possible?
> >>>>>
> >>>>>what happens will be:
> >>>>>
> >>>>>hmp-snapshot
> >>>>>     |
> >>>>>qmp-snapshot
> >>>>>     |---------
> >>>>>              |
> >>>>>         qmp-transaction            savevm(may be other..)
> >>>>>              |----------------------|
> >>>>>                             |
> >>>>>               internal transaction layer
> >>>>
> >>>>Saving the string operation is not worth duplicating the API.
> >>>>
> >>>   I agree with you for this line:), but,  it is a weight on the balance
> >>>of choice, pls consider it together with issues below.
> >>>
> >>>>>>>2) all capability are forced to be exposed.
> >>>>>>
> >>>>>>Is there something you cannot expose?
> >>>>>>
> >>>>>   As other component in qemu can use it, some option may
> >>>>>be used only in qemu not to user. For eg, vm-state-size.
> >>>>
> >>>>When we hit a limitation of QAPI then it needs to be extended.  I'm
> >>>>sure
> >>>>there's a solution for splitting or hiding parts of the QAPI generated
> >>>>API.
> >>>>
> >>>   I can't think it out now, it seems to be a bit tricky.
> >>>
> >>>>>>>3) need structure to record each transaction state, such as
> >>>>>>>BlkTransactionStates. Extending it is equal to add an internal
> >>>>>>>layer.
> >>>>>>
> >>>>>>I agree that extending it is equal coding effort to adding an
> >>>>>>internal
> >>>>>>layer because you'll need to refactor qmp_transaction() a bit to
> >>>>>>really
> >>>>>>support additional action types.
> >>>>>>
> >>>>>>But it's the right thing to do.  Don't add unnecessary layers just
> >>>>>>because writing new code is more fun than extending existing code.
> >>>>>>
> >>>>>  If this layer is not added but depending only qmp_transaction, there
> >>>>>will be many "if else" fragment. I have tried that and the code
> >>>>>is awkful, this layer did not bring extra burden only make what
> >>>>>happens inside qmp_transaction clearer, I did not add this layer just
> >>>>>for fun.
> >>>>>
> >>>>>
> >>>>>>>   Actually I started up by use qmp_transaction as API, but soon
> >>>>>>>found that work is almost done around BlkTransactionStates, so
> >>>>>>>added a layer around it clearly.
> >>>>
> >>>>The qmp_transaction() implementation can be changed, I'm not saying you
> >>>>have to hack in more if statements.  It's cleanest to introduce a
> >>>>BdrvActionOps abstraction:
> >>>>
> >>>>typedef struct BdrvActionOps BdrvActionOps;
> >>>>typedef struct BdrvTransactionState {
> >>>>     const BdrvActionOps *ops;
> >>>>     QLIST_ENTRY(BdrvTransactionState);
> >>>>} BdrvTransactionState;
> >>>>
> >>>>struct BdrvActionOps {
> >>>>     int (*prepare)(BdrvTransactionState *s, ...);
> >>>>     int (*commit)(BdrvTransactionState *s, ...);
> >>>>     int (*rollback)(BdrvTransactionState *s, ...);
> >>>>};
> >>>>
> >>>>BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
> >>>>
> >>>>Then qmp_transaction() can be generic code that steps through the
> >>>>transactions.
> >>>   With internal API, qmp_transaction can still be generic code with
> >>>a translate from bdrv* to char* at caller level.
> >>>
> >>>   This is similar to what your series does and I think it's
> >>>>the right direction.
> >>>>
> >>>>But please don't duplicate the qmp_transaction() and
> >>>>BlockdevAction/BlockdevActionList APIs.  In other words, change the
> >>>>engine, not the whole car.
> >>>>
> >>>>Stefan
> >>>>
> >>>
> >>>   If my understanding is correct, the BdrvActionOps need to be extended
> >>>as following:
> >>>struct BdrvActionOps {
> >>>      /* need following for callback functions */
> >>>      const char *sn_name;
> >>>      BlockDriverState *bs;
> >>>      ...
> >>>      int (*prepare)(BdrvTransactionState *s, ...);
> >>>      int (*commit)(BdrvTransactionState *s, ...);
> >>>      int (*rollback)(BdrvTransactionState *s, ...);
> >>>};
> >>>Or an opaque* should used for every BdrvActionOps.
> >>
> >>It is nice to keep *Ops structs read-only so they can be static const.
> >>This way the ops are shared between all instances of the same action
> >>type.  Also the function pointers can be in read-only memory pages,
> >>which is a slight security win since it prevents memory corruption
> >>exploits from taking advantage of function pointers to execute arbitrary
> >>code.
> >>
> >   Seems good, I will package callback functions into *Ops, thanks.
> >
> >>In the pseudo-code I posted the sn_name or bs fields go into an
> >>action-specific state struct:
> >>
> >>typedef struct {
> >>     BdrvTransactionState common;
> >>     char *backup_sn_name;
> >>} InternalSnapshotTransactionState;
> >>
> >>typedef struct {
> >>     BdrvTransactionState common;
> >>     BlockDriverState *old_bs;
> >>     BlockDriverState *new_bs;
> >>} ExternalSnapshotTransactionState;
> >>
> >>>Comparation:
> >>>The way above:
> >>>  1) translate from BlockdevAction to BdrvTransactionState by
> >>>     bdrv_transaction_create().
> >>>  2) enqueue BdrvTransactionState by
> >>>     some code.
> >>>  3) execute them by
> >>>     a new function, name it as BdrvActionOpsRun().
> >>
> >>If you include .prepare() in the transaction creation, then it becomes
> >>simpler:
> >>
> >>states = []
> >>for action in actions:
> >>     result = bdrv_transaction_create(action)  # invokes .prepare()
> >>     if result is error:
> >>         for state in states:
> >>        state.rollback()
> >>    return
> >>     states.append(result)
> >>for state in states:
> >>     state.commit()
> >>
> >>Because we don't wait until BdrvActionOpsRun() before processing the
> >>transaction, there's no need to translate from BlockdevAction to
> >>BdrvTransactionState.  The BdrvTransactionState struct really only has
> >>state required to commit/rollback the transaction.
> >>
> >>(Even if it becomes necessary to keep information from BlockdevAction
> >>after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
> >>duplicate it.)
> >>
> >   OK, *BlockdevAction plus *BlockDriverState and some other
> >data used internal will be added in states.
> >
> >>>Internal API way:
> >>>  1) translate BlockdevAction to BlkTransStates by
> >>>     fill_blk_trs().
> >>>  2) enqueue BlkTransStates to BlkTransStates by
> >>>     add_transaction().
> >>>  3) execute them by
> >>>     submit_transaction().
> >>>
> >>>   It seems the way above will end as something like an internal
> >>>layer, but without clear APIs tips what it is doing. Please reconsider
> >>>the advantages about a clear internal API layer.
> >>
> >>I'm not convinced by the internal API approach.  It took me a while when
> >>reviewing the code before I understood what was actually going on
> >>because of the qmp_transaction() and BlockdevAction duplication code.
> >>
> >>I see the internal API approach as an unnecessary layer of indirection.
> >>It makes the code more complicated to understand and maintain.  Next
> >>time we add something to qmp_transaction() it would also be necessary to
> >>duplicate that change for the internal API.  It creates unnecessary
> >>work.
> >>
> >   Basic process is almost the same in two approaches, I'd like to
> >adjust the code to avoid data duplication as much as possible, and
> >see if some function can be removed when code keeps clear, in next
> >version.
> >
> >>Just embrace QAPI, the point of it was to eliminate these external <->
> >>internal translations we were doing all the time.
> >>
> >>Stefan
> >>
> >
> >
> Hi, Stefan
>   I redesigned the structure, Following is the fake code:
> 
> typedef struct BdrvActionOps {
>     /* check the request's validation, allocate p_opaque if needed */
>     int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
>     /* take the action */
>     int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
>     /* update emulator */
>     int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
>     /* cancel the action */
>     int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
> } BdrvActionOps;
> 
> typedef struct BlkTransactionStates {
>     BlockdevAction *action;
>     void *opaque;
>     BdrvActionOps *ops;
>     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
> } BlkTransactionStates;
> 
> /* call ops->check and return state* to be enqueued */
> static BlkTransactionStates *transaction_create(BlockdevAction *action,
>                                                 Error **errp);
> 
> void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> {
>     BlockdevActionList *dev_entry = dev_list;
>     BlkTransactionStates *state;
> 
>     while (NULL != dev_entry) {
>         state = transaction_create(dev_entry->value, errp);
>         /* enqueue */
>         dev_entry = dev_entry->next;
>     }
> 
>     /* use queue with submit, commit, rollback callback */
> }
> 
> 
>   In this way, parameter duplication is saved, but one problem remains:
> parameter can't be hidden to user such as vm_state_size, but this would
> not be a problem if hmp "savevm" use his own code about block snapshot
> later, I mean not use qmp_transaction(). What do you think about the
> design? Do you have a better way to solve this problem?

Can you explain the vm_state_size problem again?  Sorry I forgot - I
think it had something to do with having an internal parameter in the
action that should not be exposed via QMP/HMP?

Stefan

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-3-12 23:43, Stefan Hajnoczi 写道:
> On Tue, Mar 12, 2013 at 04:30:41PM +0800, Wenchao Xia wrote:
>> 于 2013-1-15 15:03, Wenchao Xia 写道:
>>> 于 2013-1-14 18:06, Stefan Hajnoczi 写道:
>>>> On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
>>>>> 于 2013-1-11 17:12, Stefan Hajnoczi 写道:
>>>>>> On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
>>>>>>> 于 2013-1-10 20:41, Stefan Hajnoczi 写道:
>>>>>>>> On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
>>>>>>>>> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
>>>>>>>>>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
>>>>>>>>>>>    This patch switch to internal common API to take group external
>>>>>>>>>>> snapshots from qmp_transaction interface. qmp layer simply does
>>>>>>>>>>> a translation from user input.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>>>>>>>> ---
>>>>>>>>>>>   blockdev.c |  215
>>>>>>>>>>> ++++++++++++++++++++++++------------------------------------
>>>>>>>>>>>   1 files changed, 87 insertions(+), 128 deletions(-)
>>>>>>>>>>
>>>>>>>>>> An internal API for snapshots is not necessary.
>>>>>>>>>> qmp_transaction() is
>>>>>>>>>> already usable both from the monitor and C code.
>>>>>>>>>>
>>>>>>>>>> The QAPI code generator creates structs that can be accessed
>>>>>>>>>> directly
>>>>>>>>> >from C.  qmp_transaction(), BlockdevAction, and
>>>>>>>>> BlockdevActionList *is*
>>>>>>>>>> the snapshot API.  It just doesn't support internal snapshots
>>>>>>>>>> yet, which
>>>>>>>>>> is what you are trying to add.
>>>>>>>>>>
>>>>>>>>>> To add internal snapshot support, define a
>>>>>>>>>> BlockdevInternalSnapshot type
>>>>>>>>>> in qapi-schema.json and add internal snapshot support in
>>>>>>>>>> qmp_transaction().
>>>>>>>>>>
>>>>>>>>>> qmp_transaction() was designed with this in mind from the
>>>>>>>>>> beginning and
>>>>>>>>>> dispatches based on BlockdevAction->kind.
>>>>>>>>>>
>>>>>>>>>> The patch series will become much smaller while still adding
>>>>>>>>>> internal
>>>>>>>>>> snapshot support.
>>>>>>>>>>
>>>>>>>>>> Stefan
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>    As API, qmp_transaction have following disadvantages:
>>>>>>>>> 1) interface is based on string not data type inside qemu, that
>>>>>>>>> means
>>>>>>>>> other function calling it result in: bdrv->string->bdrv
>>>>>>>>
>>>>>>>> Use bdrv_get_device_name().  You already need to fill in filename or
>>>>>>>> snapshot name strings.  This is not a big disadvantage.
>>>>>>>>
>>>>>>>    Yes, not a big disadvantage, but why not save string operation but
>>>>>>> use (bdrv*) as much as possible?
>>>>>>>
>>>>>>> what happens will be:
>>>>>>>
>>>>>>> hmp-snapshot
>>>>>>>      |
>>>>>>> qmp-snapshot
>>>>>>>      |---------
>>>>>>>               |
>>>>>>>          qmp-transaction            savevm(may be other..)
>>>>>>>               |----------------------|
>>>>>>>                              |
>>>>>>>                internal transaction layer
>>>>>>
>>>>>> Saving the string operation is not worth duplicating the API.
>>>>>>
>>>>>    I agree with you for this line:), but,  it is a weight on the balance
>>>>> of choice, pls consider it together with issues below.
>>>>>
>>>>>>>>> 2) all capability are forced to be exposed.
>>>>>>>>
>>>>>>>> Is there something you cannot expose?
>>>>>>>>
>>>>>>>    As other component in qemu can use it, some option may
>>>>>>> be used only in qemu not to user. For eg, vm-state-size.
>>>>>>
>>>>>> When we hit a limitation of QAPI then it needs to be extended.  I'm
>>>>>> sure
>>>>>> there's a solution for splitting or hiding parts of the QAPI generated
>>>>>> API.
>>>>>>
>>>>>    I can't think it out now, it seems to be a bit tricky.
>>>>>
>>>>>>>>> 3) need structure to record each transaction state, such as
>>>>>>>>> BlkTransactionStates. Extending it is equal to add an internal
>>>>>>>>> layer.
>>>>>>>>
>>>>>>>> I agree that extending it is equal coding effort to adding an
>>>>>>>> internal
>>>>>>>> layer because you'll need to refactor qmp_transaction() a bit to
>>>>>>>> really
>>>>>>>> support additional action types.
>>>>>>>>
>>>>>>>> But it's the right thing to do.  Don't add unnecessary layers just
>>>>>>>> because writing new code is more fun than extending existing code.
>>>>>>>>
>>>>>>>   If this layer is not added but depending only qmp_transaction, there
>>>>>>> will be many "if else" fragment. I have tried that and the code
>>>>>>> is awkful, this layer did not bring extra burden only make what
>>>>>>> happens inside qmp_transaction clearer, I did not add this layer just
>>>>>>> for fun.
>>>>>>>
>>>>>>>
>>>>>>>>>    Actually I started up by use qmp_transaction as API, but soon
>>>>>>>>> found that work is almost done around BlkTransactionStates, so
>>>>>>>>> added a layer around it clearly.
>>>>>>
>>>>>> The qmp_transaction() implementation can be changed, I'm not saying you
>>>>>> have to hack in more if statements.  It's cleanest to introduce a
>>>>>> BdrvActionOps abstraction:
>>>>>>
>>>>>> typedef struct BdrvActionOps BdrvActionOps;
>>>>>> typedef struct BdrvTransactionState {
>>>>>>      const BdrvActionOps *ops;
>>>>>>      QLIST_ENTRY(BdrvTransactionState);
>>>>>> } BdrvTransactionState;
>>>>>>
>>>>>> struct BdrvActionOps {
>>>>>>      int (*prepare)(BdrvTransactionState *s, ...);
>>>>>>      int (*commit)(BdrvTransactionState *s, ...);
>>>>>>      int (*rollback)(BdrvTransactionState *s, ...);
>>>>>> };
>>>>>>
>>>>>> BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
>>>>>>
>>>>>> Then qmp_transaction() can be generic code that steps through the
>>>>>> transactions.
>>>>>    With internal API, qmp_transaction can still be generic code with
>>>>> a translate from bdrv* to char* at caller level.
>>>>>
>>>>>    This is similar to what your series does and I think it's
>>>>>> the right direction.
>>>>>>
>>>>>> But please don't duplicate the qmp_transaction() and
>>>>>> BlockdevAction/BlockdevActionList APIs.  In other words, change the
>>>>>> engine, not the whole car.
>>>>>>
>>>>>> Stefan
>>>>>>
>>>>>
>>>>>    If my understanding is correct, the BdrvActionOps need to be extended
>>>>> as following:
>>>>> struct BdrvActionOps {
>>>>>       /* need following for callback functions */
>>>>>       const char *sn_name;
>>>>>       BlockDriverState *bs;
>>>>>       ...
>>>>>       int (*prepare)(BdrvTransactionState *s, ...);
>>>>>       int (*commit)(BdrvTransactionState *s, ...);
>>>>>       int (*rollback)(BdrvTransactionState *s, ...);
>>>>> };
>>>>> Or an opaque* should used for every BdrvActionOps.
>>>>
>>>> It is nice to keep *Ops structs read-only so they can be static const.
>>>> This way the ops are shared between all instances of the same action
>>>> type.  Also the function pointers can be in read-only memory pages,
>>>> which is a slight security win since it prevents memory corruption
>>>> exploits from taking advantage of function pointers to execute arbitrary
>>>> code.
>>>>
>>>    Seems good, I will package callback functions into *Ops, thanks.
>>>
>>>> In the pseudo-code I posted the sn_name or bs fields go into an
>>>> action-specific state struct:
>>>>
>>>> typedef struct {
>>>>      BdrvTransactionState common;
>>>>      char *backup_sn_name;
>>>> } InternalSnapshotTransactionState;
>>>>
>>>> typedef struct {
>>>>      BdrvTransactionState common;
>>>>      BlockDriverState *old_bs;
>>>>      BlockDriverState *new_bs;
>>>> } ExternalSnapshotTransactionState;
>>>>
>>>>> Comparation:
>>>>> The way above:
>>>>>   1) translate from BlockdevAction to BdrvTransactionState by
>>>>>      bdrv_transaction_create().
>>>>>   2) enqueue BdrvTransactionState by
>>>>>      some code.
>>>>>   3) execute them by
>>>>>      a new function, name it as BdrvActionOpsRun().
>>>>
>>>> If you include .prepare() in the transaction creation, then it becomes
>>>> simpler:
>>>>
>>>> states = []
>>>> for action in actions:
>>>>      result = bdrv_transaction_create(action)  # invokes .prepare()
>>>>      if result is error:
>>>>          for state in states:
>>>>         state.rollback()
>>>>     return
>>>>      states.append(result)
>>>> for state in states:
>>>>      state.commit()
>>>>
>>>> Because we don't wait until BdrvActionOpsRun() before processing the
>>>> transaction, there's no need to translate from BlockdevAction to
>>>> BdrvTransactionState.  The BdrvTransactionState struct really only has
>>>> state required to commit/rollback the transaction.
>>>>
>>>> (Even if it becomes necessary to keep information from BlockdevAction
>>>> after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
>>>> duplicate it.)
>>>>
>>>    OK, *BlockdevAction plus *BlockDriverState and some other
>>> data used internal will be added in states.
>>>
>>>>> Internal API way:
>>>>>   1) translate BlockdevAction to BlkTransStates by
>>>>>      fill_blk_trs().
>>>>>   2) enqueue BlkTransStates to BlkTransStates by
>>>>>      add_transaction().
>>>>>   3) execute them by
>>>>>      submit_transaction().
>>>>>
>>>>>    It seems the way above will end as something like an internal
>>>>> layer, but without clear APIs tips what it is doing. Please reconsider
>>>>> the advantages about a clear internal API layer.
>>>>
>>>> I'm not convinced by the internal API approach.  It took me a while when
>>>> reviewing the code before I understood what was actually going on
>>>> because of the qmp_transaction() and BlockdevAction duplication code.
>>>>
>>>> I see the internal API approach as an unnecessary layer of indirection.
>>>> It makes the code more complicated to understand and maintain.  Next
>>>> time we add something to qmp_transaction() it would also be necessary to
>>>> duplicate that change for the internal API.  It creates unnecessary
>>>> work.
>>>>
>>>    Basic process is almost the same in two approaches, I'd like to
>>> adjust the code to avoid data duplication as much as possible, and
>>> see if some function can be removed when code keeps clear, in next
>>> version.
>>>
>>>> Just embrace QAPI, the point of it was to eliminate these external <->
>>>> internal translations we were doing all the time.
>>>>
>>>> Stefan
>>>>
>>>
>>>
>> Hi, Stefan
>>    I redesigned the structure, Following is the fake code:
>>
>> typedef struct BdrvActionOps {
>>      /* check the request's validation, allocate p_opaque if needed */
>>      int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
>>      /* take the action */
>>      int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
>>      /* update emulator */
>>      int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
>>      /* cancel the action */
>>      int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
>> } BdrvActionOps;
>>
>> typedef struct BlkTransactionStates {
>>      BlockdevAction *action;
>>      void *opaque;
>>      BdrvActionOps *ops;
>>      QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>> } BlkTransactionStates;
>>
>> /* call ops->check and return state* to be enqueued */
>> static BlkTransactionStates *transaction_create(BlockdevAction *action,
>>                                                  Error **errp);
>>
>> void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>> {
>>      BlockdevActionList *dev_entry = dev_list;
>>      BlkTransactionStates *state;
>>
>>      while (NULL != dev_entry) {
>>          state = transaction_create(dev_entry->value, errp);
>>          /* enqueue */
>>          dev_entry = dev_entry->next;
>>      }
>>
>>      /* use queue with submit, commit, rollback callback */
>> }
>>
>>
>>    In this way, parameter duplication is saved, but one problem remains:
>> parameter can't be hidden to user such as vm_state_size, but this would
>> not be a problem if hmp "savevm" use his own code about block snapshot
>> later, I mean not use qmp_transaction(). What do you think about the
>> design? Do you have a better way to solve this problem?
> 
> Can you explain the vm_state_size problem again?  Sorry I forgot - I
> think it had something to do with having an internal parameter in the
> action that should not be exposed via QMP/HMP?
> 
  Yep, this parameter will be used by qemu "live savevm" code later,
but should not expose it to user in qmp interface.

> Stefan
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Stefan Hajnoczi]

On Wed, Mar 13, 2013 at 09:36:06AM +0800, Wenchao Xia wrote:
> 于 2013-3-12 23:43, Stefan Hajnoczi 写道:
> > On Tue, Mar 12, 2013 at 04:30:41PM +0800, Wenchao Xia wrote:
> >> 于 2013-1-15 15:03, Wenchao Xia 写道:
> >>> 于 2013-1-14 18:06, Stefan Hajnoczi 写道:
> >>>> On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
> >>>>> 于 2013-1-11 17:12, Stefan Hajnoczi 写道:
> >>>>>> On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
> >>>>>>> 于 2013-1-10 20:41, Stefan Hajnoczi 写道:
> >>>>>>>> On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
> >>>>>>>>> 于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> >>>>>>>>>> On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
> >>>>>>>>>>>    This patch switch to internal common API to take group external
> >>>>>>>>>>> snapshots from qmp_transaction interface. qmp layer simply does
> >>>>>>>>>>> a translation from user input.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>   blockdev.c |  215
> >>>>>>>>>>> ++++++++++++++++++++++++------------------------------------
> >>>>>>>>>>>   1 files changed, 87 insertions(+), 128 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> An internal API for snapshots is not necessary.
> >>>>>>>>>> qmp_transaction() is
> >>>>>>>>>> already usable both from the monitor and C code.
> >>>>>>>>>>
> >>>>>>>>>> The QAPI code generator creates structs that can be accessed
> >>>>>>>>>> directly
> >>>>>>>>> >from C.  qmp_transaction(), BlockdevAction, and
> >>>>>>>>> BlockdevActionList *is*
> >>>>>>>>>> the snapshot API.  It just doesn't support internal snapshots
> >>>>>>>>>> yet, which
> >>>>>>>>>> is what you are trying to add.
> >>>>>>>>>>
> >>>>>>>>>> To add internal snapshot support, define a
> >>>>>>>>>> BlockdevInternalSnapshot type
> >>>>>>>>>> in qapi-schema.json and add internal snapshot support in
> >>>>>>>>>> qmp_transaction().
> >>>>>>>>>>
> >>>>>>>>>> qmp_transaction() was designed with this in mind from the
> >>>>>>>>>> beginning and
> >>>>>>>>>> dispatches based on BlockdevAction->kind.
> >>>>>>>>>>
> >>>>>>>>>> The patch series will become much smaller while still adding
> >>>>>>>>>> internal
> >>>>>>>>>> snapshot support.
> >>>>>>>>>>
> >>>>>>>>>> Stefan
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>    As API, qmp_transaction have following disadvantages:
> >>>>>>>>> 1) interface is based on string not data type inside qemu, that
> >>>>>>>>> means
> >>>>>>>>> other function calling it result in: bdrv->string->bdrv
> >>>>>>>>
> >>>>>>>> Use bdrv_get_device_name().  You already need to fill in filename or
> >>>>>>>> snapshot name strings.  This is not a big disadvantage.
> >>>>>>>>
> >>>>>>>    Yes, not a big disadvantage, but why not save string operation but
> >>>>>>> use (bdrv*) as much as possible?
> >>>>>>>
> >>>>>>> what happens will be:
> >>>>>>>
> >>>>>>> hmp-snapshot
> >>>>>>>      |
> >>>>>>> qmp-snapshot
> >>>>>>>      |---------
> >>>>>>>               |
> >>>>>>>          qmp-transaction            savevm(may be other..)
> >>>>>>>               |----------------------|
> >>>>>>>                              |
> >>>>>>>                internal transaction layer
> >>>>>>
> >>>>>> Saving the string operation is not worth duplicating the API.
> >>>>>>
> >>>>>    I agree with you for this line:), but,  it is a weight on the balance
> >>>>> of choice, pls consider it together with issues below.
> >>>>>
> >>>>>>>>> 2) all capability are forced to be exposed.
> >>>>>>>>
> >>>>>>>> Is there something you cannot expose?
> >>>>>>>>
> >>>>>>>    As other component in qemu can use it, some option may
> >>>>>>> be used only in qemu not to user. For eg, vm-state-size.
> >>>>>>
> >>>>>> When we hit a limitation of QAPI then it needs to be extended.  I'm
> >>>>>> sure
> >>>>>> there's a solution for splitting or hiding parts of the QAPI generated
> >>>>>> API.
> >>>>>>
> >>>>>    I can't think it out now, it seems to be a bit tricky.
> >>>>>
> >>>>>>>>> 3) need structure to record each transaction state, such as
> >>>>>>>>> BlkTransactionStates. Extending it is equal to add an internal
> >>>>>>>>> layer.
> >>>>>>>>
> >>>>>>>> I agree that extending it is equal coding effort to adding an
> >>>>>>>> internal
> >>>>>>>> layer because you'll need to refactor qmp_transaction() a bit to
> >>>>>>>> really
> >>>>>>>> support additional action types.
> >>>>>>>>
> >>>>>>>> But it's the right thing to do.  Don't add unnecessary layers just
> >>>>>>>> because writing new code is more fun than extending existing code.
> >>>>>>>>
> >>>>>>>   If this layer is not added but depending only qmp_transaction, there
> >>>>>>> will be many "if else" fragment. I have tried that and the code
> >>>>>>> is awkful, this layer did not bring extra burden only make what
> >>>>>>> happens inside qmp_transaction clearer, I did not add this layer just
> >>>>>>> for fun.
> >>>>>>>
> >>>>>>>
> >>>>>>>>>    Actually I started up by use qmp_transaction as API, but soon
> >>>>>>>>> found that work is almost done around BlkTransactionStates, so
> >>>>>>>>> added a layer around it clearly.
> >>>>>>
> >>>>>> The qmp_transaction() implementation can be changed, I'm not saying you
> >>>>>> have to hack in more if statements.  It's cleanest to introduce a
> >>>>>> BdrvActionOps abstraction:
> >>>>>>
> >>>>>> typedef struct BdrvActionOps BdrvActionOps;
> >>>>>> typedef struct BdrvTransactionState {
> >>>>>>      const BdrvActionOps *ops;
> >>>>>>      QLIST_ENTRY(BdrvTransactionState);
> >>>>>> } BdrvTransactionState;
> >>>>>>
> >>>>>> struct BdrvActionOps {
> >>>>>>      int (*prepare)(BdrvTransactionState *s, ...);
> >>>>>>      int (*commit)(BdrvTransactionState *s, ...);
> >>>>>>      int (*rollback)(BdrvTransactionState *s, ...);
> >>>>>> };
> >>>>>>
> >>>>>> BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
> >>>>>>
> >>>>>> Then qmp_transaction() can be generic code that steps through the
> >>>>>> transactions.
> >>>>>    With internal API, qmp_transaction can still be generic code with
> >>>>> a translate from bdrv* to char* at caller level.
> >>>>>
> >>>>>    This is similar to what your series does and I think it's
> >>>>>> the right direction.
> >>>>>>
> >>>>>> But please don't duplicate the qmp_transaction() and
> >>>>>> BlockdevAction/BlockdevActionList APIs.  In other words, change the
> >>>>>> engine, not the whole car.
> >>>>>>
> >>>>>> Stefan
> >>>>>>
> >>>>>
> >>>>>    If my understanding is correct, the BdrvActionOps need to be extended
> >>>>> as following:
> >>>>> struct BdrvActionOps {
> >>>>>       /* need following for callback functions */
> >>>>>       const char *sn_name;
> >>>>>       BlockDriverState *bs;
> >>>>>       ...
> >>>>>       int (*prepare)(BdrvTransactionState *s, ...);
> >>>>>       int (*commit)(BdrvTransactionState *s, ...);
> >>>>>       int (*rollback)(BdrvTransactionState *s, ...);
> >>>>> };
> >>>>> Or an opaque* should used for every BdrvActionOps.
> >>>>
> >>>> It is nice to keep *Ops structs read-only so they can be static const.
> >>>> This way the ops are shared between all instances of the same action
> >>>> type.  Also the function pointers can be in read-only memory pages,
> >>>> which is a slight security win since it prevents memory corruption
> >>>> exploits from taking advantage of function pointers to execute arbitrary
> >>>> code.
> >>>>
> >>>    Seems good, I will package callback functions into *Ops, thanks.
> >>>
> >>>> In the pseudo-code I posted the sn_name or bs fields go into an
> >>>> action-specific state struct:
> >>>>
> >>>> typedef struct {
> >>>>      BdrvTransactionState common;
> >>>>      char *backup_sn_name;
> >>>> } InternalSnapshotTransactionState;
> >>>>
> >>>> typedef struct {
> >>>>      BdrvTransactionState common;
> >>>>      BlockDriverState *old_bs;
> >>>>      BlockDriverState *new_bs;
> >>>> } ExternalSnapshotTransactionState;
> >>>>
> >>>>> Comparation:
> >>>>> The way above:
> >>>>>   1) translate from BlockdevAction to BdrvTransactionState by
> >>>>>      bdrv_transaction_create().
> >>>>>   2) enqueue BdrvTransactionState by
> >>>>>      some code.
> >>>>>   3) execute them by
> >>>>>      a new function, name it as BdrvActionOpsRun().
> >>>>
> >>>> If you include .prepare() in the transaction creation, then it becomes
> >>>> simpler:
> >>>>
> >>>> states = []
> >>>> for action in actions:
> >>>>      result = bdrv_transaction_create(action)  # invokes .prepare()
> >>>>      if result is error:
> >>>>          for state in states:
> >>>>         state.rollback()
> >>>>     return
> >>>>      states.append(result)
> >>>> for state in states:
> >>>>      state.commit()
> >>>>
> >>>> Because we don't wait until BdrvActionOpsRun() before processing the
> >>>> transaction, there's no need to translate from BlockdevAction to
> >>>> BdrvTransactionState.  The BdrvTransactionState struct really only has
> >>>> state required to commit/rollback the transaction.
> >>>>
> >>>> (Even if it becomes necessary to keep information from BlockdevAction
> >>>> after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
> >>>> duplicate it.)
> >>>>
> >>>    OK, *BlockdevAction plus *BlockDriverState and some other
> >>> data used internal will be added in states.
> >>>
> >>>>> Internal API way:
> >>>>>   1) translate BlockdevAction to BlkTransStates by
> >>>>>      fill_blk_trs().
> >>>>>   2) enqueue BlkTransStates to BlkTransStates by
> >>>>>      add_transaction().
> >>>>>   3) execute them by
> >>>>>      submit_transaction().
> >>>>>
> >>>>>    It seems the way above will end as something like an internal
> >>>>> layer, but without clear APIs tips what it is doing. Please reconsider
> >>>>> the advantages about a clear internal API layer.
> >>>>
> >>>> I'm not convinced by the internal API approach.  It took me a while when
> >>>> reviewing the code before I understood what was actually going on
> >>>> because of the qmp_transaction() and BlockdevAction duplication code.
> >>>>
> >>>> I see the internal API approach as an unnecessary layer of indirection.
> >>>> It makes the code more complicated to understand and maintain.  Next
> >>>> time we add something to qmp_transaction() it would also be necessary to
> >>>> duplicate that change for the internal API.  It creates unnecessary
> >>>> work.
> >>>>
> >>>    Basic process is almost the same in two approaches, I'd like to
> >>> adjust the code to avoid data duplication as much as possible, and
> >>> see if some function can be removed when code keeps clear, in next
> >>> version.
> >>>
> >>>> Just embrace QAPI, the point of it was to eliminate these external <->
> >>>> internal translations we were doing all the time.
> >>>>
> >>>> Stefan
> >>>>
> >>>
> >>>
> >> Hi, Stefan
> >>    I redesigned the structure, Following is the fake code:
> >>
> >> typedef struct BdrvActionOps {
> >>      /* check the request's validation, allocate p_opaque if needed */
> >>      int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
> >>      /* take the action */
> >>      int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
> >>      /* update emulator */
> >>      int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
> >>      /* cancel the action */
> >>      int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
> >> } BdrvActionOps;
> >>
> >> typedef struct BlkTransactionStates {
> >>      BlockdevAction *action;
> >>      void *opaque;
> >>      BdrvActionOps *ops;
> >>      QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
> >> } BlkTransactionStates;
> >>
> >> /* call ops->check and return state* to be enqueued */
> >> static BlkTransactionStates *transaction_create(BlockdevAction *action,
> >>                                                  Error **errp);
> >>
> >> void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> >> {
> >>      BlockdevActionList *dev_entry = dev_list;
> >>      BlkTransactionStates *state;
> >>
> >>      while (NULL != dev_entry) {
> >>          state = transaction_create(dev_entry->value, errp);
> >>          /* enqueue */
> >>          dev_entry = dev_entry->next;
> >>      }
> >>
> >>      /* use queue with submit, commit, rollback callback */
> >> }
> >>
> >>
> >>    In this way, parameter duplication is saved, but one problem remains:
> >> parameter can't be hidden to user such as vm_state_size, but this would
> >> not be a problem if hmp "savevm" use his own code about block snapshot
> >> later, I mean not use qmp_transaction(). What do you think about the
> >> design? Do you have a better way to solve this problem?
> > 
> > Can you explain the vm_state_size problem again?  Sorry I forgot - I
> > think it had something to do with having an internal parameter in the
> > action that should not be exposed via QMP/HMP?
> > 
>   Yep, this parameter will be used by qemu "live savevm" code later,
> but should not expose it to user in qmp interface.

The simplest solution is:

void bdrv_transaction(...)
{
    /* Common code here */
}

void qmp_transaction(...)
{
    /* Reject optional internal fields here */
    if (opts->has_vm_state_size) {
        error_setg(errp, "internal field vm_state_size must not be set");
	return;
    }

    bdrv_transaction(...);
}

If transaction is used from inside QEMU with vm_state_size, then call
bdrv_transaction() instead of qmp_transaction() to skip the public field
checks.

My second idea is to add QAPI syntax to make a field as "private" or
"internal".  The marshalling code will skip or return an error if the
field is set.

This is a little nicer since all callers use qmp_transaction() but we're
guaranteed that external JSON cannot make use of the field.

Stefan

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Kevin Wolf]

Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>   I redesigned the structure, Following is the fake code:
> 
> typedef struct BdrvActionOps {
>     /* check the request's validation, allocate p_opaque if needed */
>     int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
>     /* take the action */
>     int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
>     /* update emulator */
>     int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
>     /* cancel the action */
>     int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
> } BdrvActionOps;

Why do you need the split of prepare into check/submit?

If you have prepare/commit/abort, everybody will recognise this as the
standard transaction pattern because this is just how it's done.
Deviating from it needs a good justification in my opinion.

Kevin

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-3-13 18:18, Kevin Wolf 写道:
> Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>>    I redesigned the structure, Following is the fake code:
>>
>> typedef struct BdrvActionOps {
>>      /* check the request's validation, allocate p_opaque if needed */
>>      int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
>>      /* take the action */
>>      int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
>>      /* update emulator */
>>      int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
>>      /* cancel the action */
>>      int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
>> } BdrvActionOps;
>
> Why do you need the split of prepare into check/submit?
>
> If you have prepare/commit/abort, everybody will recognise this as the
> standard transaction pattern because this is just how it's done.
> Deviating from it needs a good justification in my opinion.
>
> Kevin
>

   My thought is rejecting the request in *check if parameter invalid
before take any action, while submit do the real action, to reduce
the chance to of rolling back when some request not valid in the batch.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Kevin Wolf]

Am 14.03.2013 um 06:08 hat Wenchao Xia geschrieben:
> 于 2013-3-13 18:18, Kevin Wolf 写道:
> >Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
> >>   I redesigned the structure, Following is the fake code:
> >>
> >>typedef struct BdrvActionOps {
> >>     /* check the request's validation, allocate p_opaque if needed */
> >>     int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
> >>     /* take the action */
> >>     int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
> >>     /* update emulator */
> >>     int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
> >>     /* cancel the action */
> >>     int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
> >>} BdrvActionOps;
> >
> >Why do you need the split of prepare into check/submit?
> >
> >If you have prepare/commit/abort, everybody will recognise this as the
> >standard transaction pattern because this is just how it's done.
> >Deviating from it needs a good justification in my opinion.
> >
> >Kevin
> >
> 
>   My thought is rejecting the request in *check if parameter invalid
> before take any action, while submit do the real action, to reduce
> the chance to of rolling back when some request not valid in the batch.

Okay, so it's not strictly needed, but an optimisation of the error
case?

Does it work well when the transaction includes an operation that
depends on the previous one, like create a snapshot and then do
something with this snapshot?

Anyway, even if we think it works and is worth the effort to optimise
such error cases, please use names that are consistent with the
transactions used for reopening: (check/)prepare/commit/abort.

Kevin

Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia]

于 2013-3-14 16:22, Kevin Wolf 写道:
> Am 14.03.2013 um 06:08 hat Wenchao Xia geschrieben:
>> 于 2013-3-13 18:18, Kevin Wolf 写道:
>>> Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>>>>    I redesigned the structure, Following is the fake code:
>>>>
>>>> typedef struct BdrvActionOps {
>>>>      /* check the request's validation, allocate p_opaque if needed */
>>>>      int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
>>>>      /* take the action */
>>>>      int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
>>>>      /* update emulator */
>>>>      int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
>>>>      /* cancel the action */
>>>>      int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
>>>> } BdrvActionOps;
>>>
>>> Why do you need the split of prepare into check/submit?
>>>
>>> If you have prepare/commit/abort, everybody will recognise this as the
>>> standard transaction pattern because this is just how it's done.
>>> Deviating from it needs a good justification in my opinion.
>>>
>>> Kevin
>>>
>>
>>    My thought is rejecting the request in *check if parameter invalid
>> before take any action, while submit do the real action, to reduce
>> the chance to of rolling back when some request not valid in the batch.
>
> Okay, so it's not strictly needed, but an optimisation of the error
> case?
>
> Does it work well when the transaction includes an operation that
> depends on the previous one, like create a snapshot and then do
> something with this snapshot?
>
   This seems to complex, since prepare of all actions are executed
in first round, they may interrupt each other later. So I am
thinking make it more clear as complete one job one time, which
may change the old qmp_transcation() logic a little.

> Anyway, even if we think it works and is worth the effort to optimise
> such error cases, please use names that are consistent with the
> transactions used for reopening: (check/)prepare/commit/abort.
>
   In above way check/prepare can be merged, how do you think of it?

> Kevin
>


-- 
Best Regards

Wenchao Xia