[Qemu-devel] [PATCH v3 0/9] block: generic image streaming

[Qemu-devel] [PATCH v3 0/9] block: generic image streaming

[Stefan Hajnoczi]

This series adds the 'block_stream' command which copies the contents of a
backing file into the image file while the VM is running.  The series builds on
copy-on-read and zero detection features which I sent out recently and I
suggest grabbing my git tree to try it out without merging these dependencies
yourself:

http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/image-streaming-api

The image streaming HMP/QMP commands are documented in the patch and also
described here:

http://wiki.qemu.org/Features/LiveBlockMigration/ImageStreamingAPI

The basic idea is to execute 'block_stream virtio0' while the guest is running.
Progress can be monitored using 'info block-jobs'.  When the streaming
operation completes it raises a QMP event.

Note: The last patch includes includes a Python test script called
test-stream.py, I do not propose to merge it.  When run in a QEMU source tree
it performs basic image streaming QMP tests.

Stefan Hajnoczi (9):
  coroutine: add co_sleep_ns() coroutine sleep function
  block: add BlockJob interface for long-running operations
  block: add image streaming block job
  block: rate-limit streaming operations
  qmp: add block_stream command
  qmp: add block_job_set_speed command
  qmp: add block_job_cancel command
  qmp: add query-block-jobs
  test: add image streaming test cases

 Makefile.objs          |    3 +-
 block/stream.c         |  174 ++++++++++++++++++++++++++++++++++++++++
 block_int.h            |   86 ++++++++++++++++++++
 blockdev.c             |  143 +++++++++++++++++++++++++++++++++
 hmp-commands.hx        |   41 ++++++++++
 hmp.c                  |   81 +++++++++++++++++++
 hmp.h                  |    4 +
 monitor.c              |   13 +++
 monitor.h              |    2 +
 qapi-schema.json       |  144 +++++++++++++++++++++++++++++++++
 qemu-coroutine-sleep.c |   38 +++++++++
 qemu-coroutine.h       |    6 ++
 qerror.c               |    4 +
 qerror.h               |    3 +
 qmp-commands.hx        |   93 +++++++++++++++++++++
 test-stream.py         |  208 ++++++++++++++++++++++++++++++++++++++++++++++++
 trace-events           |    9 ++
 17 files changed, 1051 insertions(+), 1 deletions(-)
 create mode 100644 block/stream.c
 create mode 100644 qemu-coroutine-sleep.c
 create mode 100644 test-stream.py

-- 
1.7.7.3

[Qemu-devel] [PATCH v3 1/9] coroutine: add co_sleep_ns() coroutine sleep function

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 Makefile.objs          |    2 +-
 qemu-coroutine-sleep.c |   38 ++++++++++++++++++++++++++++++++++++++
 qemu-coroutine.h       |    6 ++++++
 3 files changed, 45 insertions(+), 1 deletions(-)
 create mode 100644 qemu-coroutine-sleep.c

diff --git a/Makefile.objs b/Makefile.objs
index d7a6539..d737d81 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -12,7 +12,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o
 
 #######################################################################
 # coroutines
-coroutine-obj-y = qemu-coroutine.o qemu-coroutine-lock.o
+coroutine-obj-y = qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-sleep.o
 ifeq ($(CONFIG_UCONTEXT_COROUTINE),y)
 coroutine-obj-$(CONFIG_POSIX) += coroutine-ucontext.o
 else
diff --git a/qemu-coroutine-sleep.c b/qemu-coroutine-sleep.c
new file mode 100644
index 0000000..fd65274
--- /dev/null
+++ b/qemu-coroutine-sleep.c
@@ -0,0 +1,38 @@
+/*
+ * QEMU coroutine sleep
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Stefan Hajnoczi    <stefanha@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu-coroutine.h"
+#include "qemu-timer.h"
+
+typedef struct CoSleepCB {
+    QEMUTimer *ts;
+    Coroutine *co;
+} CoSleepCB;
+
+static void co_sleep_cb(void *opaque)
+{
+    CoSleepCB *sleep_cb = opaque;
+
+    qemu_free_timer(sleep_cb->ts);
+    qemu_coroutine_enter(sleep_cb->co, NULL);
+}
+
+void coroutine_fn co_sleep_ns(QEMUClock *clock, int64_t ns)
+{
+    CoSleepCB sleep_cb = {
+        .co = qemu_coroutine_self(),
+    };
+    sleep_cb.ts = qemu_new_timer(clock, SCALE_NS, co_sleep_cb, &sleep_cb);
+    qemu_mod_timer(sleep_cb.ts, qemu_get_clock_ns(clock) + ns);
+    qemu_coroutine_yield();
+}
diff --git a/qemu-coroutine.h b/qemu-coroutine.h
index 8a55fe1..bae1ffe 100644
--- a/qemu-coroutine.h
+++ b/qemu-coroutine.h
@@ -17,6 +17,7 @@
 
 #include <stdbool.h>
 #include "qemu-queue.h"
+#include "qemu-timer.h"
 
 /**
  * Coroutines are a mechanism for stack switching and can be used for
@@ -199,4 +200,9 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock);
  */
 void qemu_co_rwlock_unlock(CoRwlock *lock);
 
+/**
+ * Yield the coroutine for a given duration
+ */
+void coroutine_fn co_sleep_ns(QEMUClock *clock, int64_t ns);
+
 #endif /* QEMU_COROUTINE_H */
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 2/9] block: add BlockJob interface for long-running operations

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block_int.h |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/block_int.h b/block_int.h
index 89a860c..bc397c4 100644
--- a/block_int.h
+++ b/block_int.h
@@ -69,6 +69,36 @@ typedef struct BlockIOBaseValue {
     uint64_t ios[2];
 } BlockIOBaseValue;
 
+typedef void BlockJobCancelFunc(void *opaque);
+typedef struct BlockJob BlockJob;
+typedef struct BlockJobType {
+    /** Derived BlockJob struct size */
+    size_t instance_size;
+
+    /** String describing the operation, part of query-block-jobs QMP API */
+    const char *job_type;
+
+    /** Optional callback for job types that support setting a speed limit */
+    int (*set_speed)(BlockJob *job, int64_t value);
+} BlockJobType;
+
+/**
+ * Long-running operation on a BlockDriverState
+ */
+struct BlockJob {
+    const BlockJobType *job_type;
+    BlockDriverState *bs;
+    bool cancelled;
+
+    /* These fields are published by the query-block-jobs QMP API */
+    int64_t offset;
+    int64_t len;
+    int64_t speed;
+
+    BlockDriverCompletionFunc *cb;
+    void *opaque;
+};
+
 struct BlockDriver {
     const char *format_name;
     int instance_size;
@@ -270,6 +300,9 @@ struct BlockDriverState {
     void *private;
 
     QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
+
+    /* long-running background operation */
+    BlockJob *job;
 };
 
 struct BlockDriverAIOCB {
@@ -293,4 +326,54 @@ void bdrv_set_io_limits(BlockDriverState *bs,
 int is_windows_drive(const char *filename);
 #endif
 
+static inline void *block_job_create(const BlockJobType *job_type,
+                                     BlockDriverState *bs,
+                                     BlockDriverCompletionFunc *cb,
+                                     void *opaque)
+{
+    BlockJob *job;
+
+    if (bdrv_in_use(bs)) {
+        return NULL;
+    }
+    bdrv_set_in_use(bs, 1);
+
+    job = g_malloc0(job_type->instance_size);
+    job->job_type      = job_type;
+    job->bs            = bs;
+    job->cb            = cb;
+    job->opaque        = opaque;
+    bs->job = job;
+    return job;
+}
+
+static inline void block_job_complete(BlockJob *job, int ret)
+{
+    BlockDriverState *bs = job->bs;
+
+    assert(bs->job == job);
+    job->cb(job->opaque, ret);
+    bs->job = NULL;
+    g_free(job);
+    bdrv_set_in_use(bs, 0);
+}
+
+static inline int block_job_set_speed(BlockJob *job, int64_t value)
+{
+    if (!job->job_type->set_speed) {
+        return -ENOTSUP;
+    }
+    return job->job_type->set_speed(job, value);
+}
+
+static inline void block_job_cancel(BlockJob *job)
+{
+    job->cancelled = true;
+}
+
+static inline bool block_job_is_cancelled(BlockJob *job)
+{
+    return job->cancelled;
+}
+
 #endif /* BLOCK_INT_H */
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 3/9] block: add image streaming block job

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 Makefile.objs  |    1 +
 block/stream.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block_int.h    |    3 +
 trace-events   |    4 ++
 4 files changed, 129 insertions(+), 0 deletions(-)
 create mode 100644 block/stream.c

diff --git a/Makefile.objs b/Makefile.objs
index d737d81..025cc08 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -34,6 +34,7 @@ block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow
 block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-nested-y += qed-check.o
 block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
+block-nested-y += stream.o
 block-nested-$(CONFIG_WIN32) += raw-win32.o
 block-nested-$(CONFIG_POSIX) += raw-posix.o
 block-nested-$(CONFIG_LIBISCSI) += iscsi.o
diff --git a/block/stream.c b/block/stream.c
new file mode 100644
index 0000000..7d362ab
--- /dev/null
+++ b/block/stream.c
@@ -0,0 +1,121 @@
+/*
+ * Image streaming
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "trace.h"
+#include "block_int.h"
+
+enum {
+    /*
+     * Size of data buffer for populating the image file.  This should be large
+     * enough to process multiple clusters in a single call, so that populating
+     * contiguous regions of the image is efficient.
+     */
+    STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
+};
+
+typedef struct StreamBlockJob {
+    BlockJob common;
+    BlockDriverState *base;
+} StreamBlockJob;
+
+static int coroutine_fn stream_populate(BlockDriverState *bs,
+                                        int64_t sector_num, int nb_sectors,
+                                        void *buf)
+{
+    struct iovec iov = {
+        .iov_base = buf,
+        .iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
+    };
+    QEMUIOVector qiov;
+
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    /* Copy-on-read the unallocated clusters */
+    return bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
+}
+
+static void coroutine_fn stream_run(void *opaque)
+{
+    StreamBlockJob *s = opaque;
+    BlockDriverState *bs = s->common.bs;
+    int64_t sector_num, end;
+    int ret = 0;
+    int n;
+    void *buf;
+
+    buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
+    s->common.len = bdrv_getlength(bs);
+    bdrv_get_geometry(bs, (uint64_t *)&end);
+
+    bdrv_enable_zero_detection(bs);
+    bdrv_enable_copy_on_read(bs);
+
+    for (sector_num = 0; sector_num < end; sector_num += n) {
+        if (block_job_is_cancelled(&s->common)) {
+            break;
+        }
+
+        /* TODO rate-limit */
+        /* Note that even when no rate limit is applied we need to yield with
+         * no pending I/O here so that qemu_aio_flush() is able to return.
+         */
+        co_sleep_ns(rt_clock, 0);
+
+        ret = bdrv_co_is_allocated(bs, sector_num,
+                                   STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
+        trace_stream_one_iteration(s, sector_num, n, ret);
+        if (ret == 0) {
+            ret = stream_populate(bs, sector_num, n, buf);
+        }
+        if (ret < 0) {
+            break;
+        }
+
+        /* Publish progress */
+        s->common.offset += n * BDRV_SECTOR_SIZE;
+    }
+
+    bdrv_disable_copy_on_read(bs);
+    bdrv_disable_zero_detection(bs);
+
+    if (sector_num == end && ret == 0) {
+        bdrv_change_backing_file(bs, NULL, NULL);
+    }
+
+    qemu_vfree(buf);
+    block_job_complete(&s->common, ret);
+}
+
+static BlockJobType stream_job_type = {
+    .instance_size = sizeof(StreamBlockJob),
+    .job_type      = "stream",
+};
+
+int stream_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverCompletionFunc *cb, void *opaque)
+{
+    StreamBlockJob *s;
+    Coroutine *co;
+
+    if (bs->job) {
+        return -EBUSY;
+    }
+
+    s = block_job_create(&stream_job_type, bs, cb, opaque);
+    s->base = base;
+
+    co = qemu_coroutine_create(stream_run);
+    trace_stream_start(bs, base, s, co, opaque);
+    qemu_coroutine_enter(co, s);
+    return 0;
+}
diff --git a/block_int.h b/block_int.h
index bc397c4..614e4e2 100644
--- a/block_int.h
+++ b/block_int.h
@@ -376,4 +376,7 @@ static inline bool block_job_is_cancelled(BlockJob *job)
     return job->cancelled;
 }
 
+int stream_start(BlockDriverState *bs, BlockDriverState *base,
+                 BlockDriverCompletionFunc *cb, void *opaque);
+
 #endif /* BLOCK_INT_H */
diff --git a/trace-events b/trace-events
index 514849a..4efcd95 100644
--- a/trace-events
+++ b/trace-events
@@ -69,6 +69,10 @@ bdrv_co_writev(void *bs, int64_t sector_num, int nb_sector) "bs %p sector_num %"
 bdrv_co_io_em(void *bs, int64_t sector_num, int nb_sectors, int is_write, void *acb) "bs %p sector_num %"PRId64" nb_sectors %d is_write %d acb %p"
 bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t cluster_sector_num, int cluster_nb_sectors) "bs %p sector_num %"PRId64" nb_sectors %d cluster_sector_num %"PRId64" cluster_nb_sectors %d"
 
+# block/stream.c
+stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
+stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
+
 # hw/virtio-blk.c
 virtio_blk_req_complete(void *req, int status) "req %p status %d"
 virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 4/9] block: rate-limit streaming operations

[Stefan Hajnoczi]

This patch implements rate-limiting for image streaming.  If we've
exceeded the bandwidth quota for a 100 ms time slice we sleep the
coroutine until the next slice begins.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/stream.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 7d362ab..7f5b983 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -23,8 +23,39 @@ enum {
     STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
 };
 
+#define SLICE_TIME 100000000ULL /* ns */
+
+typedef struct {
+    int64_t next_slice_time;
+    uint64_t slice_quota;
+    uint64_t dispatched;
+} RateLimit;
+
+static int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n)
+{
+    int64_t delay_ns = 0;
+    int64_t now = qemu_get_clock_ns(rt_clock);
+
+    if (limit->next_slice_time < now) {
+        limit->next_slice_time = now + SLICE_TIME;
+        limit->dispatched = 0;
+    }
+    if (limit->dispatched + n > limit->slice_quota) {
+        delay_ns = limit->next_slice_time - now;
+    } else {
+        limit->dispatched += n;
+    }
+    return delay_ns;
+}
+
+static void ratelimit_set_speed(RateLimit *limit, uint64_t speed)
+{
+    limit->slice_quota = speed / (1000000000ULL / SLICE_TIME);
+}
+
 typedef struct StreamBlockJob {
     BlockJob common;
+    RateLimit limit;
     BlockDriverState *base;
 } StreamBlockJob;
 
@@ -61,20 +92,24 @@ static void coroutine_fn stream_run(void *opaque)
     bdrv_enable_copy_on_read(bs);
 
     for (sector_num = 0; sector_num < end; sector_num += n) {
+retry:
         if (block_job_is_cancelled(&s->common)) {
             break;
         }
 
-        /* TODO rate-limit */
-        /* Note that even when no rate limit is applied we need to yield with
-         * no pending I/O here so that qemu_aio_flush() is able to return.
-         */
-        co_sleep_ns(rt_clock, 0);
-
         ret = bdrv_co_is_allocated(bs, sector_num,
                                    STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
         trace_stream_one_iteration(s, sector_num, n, ret);
         if (ret == 0) {
+            if (s->common.speed) {
+                uint64_t delay_ns = ratelimit_calculate_delay(&s->limit, n);
+                if (delay_ns > 0) {
+                    co_sleep_ns(rt_clock, delay_ns);
+
+                    /* Recheck cancellation and that sectors are unallocated */
+                    goto retry;
+                }
+            }
             ret = stream_populate(bs, sector_num, n, buf);
         }
         if (ret < 0) {
@@ -83,6 +118,11 @@ static void coroutine_fn stream_run(void *opaque)
 
         /* Publish progress */
         s->common.offset += n * BDRV_SECTOR_SIZE;
+
+        /* Note that even when no rate limit is applied we need to yield
+         * with no pending I/O here so that qemu_aio_flush() returns.
+         */
+        co_sleep_ns(rt_clock, 0);
     }
 
     bdrv_disable_copy_on_read(bs);
@@ -96,9 +136,22 @@ static void coroutine_fn stream_run(void *opaque)
     block_job_complete(&s->common, ret);
 }
 
+static int stream_set_speed(BlockJob *job, int64_t value)
+{
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common);
+
+    if (value < 0) {
+        return -EINVAL;
+    }
+    job->speed = value;
+    ratelimit_set_speed(&s->limit, value / BDRV_SECTOR_SIZE);
+    return 0;
+}
+
 static BlockJobType stream_job_type = {
     .instance_size = sizeof(StreamBlockJob),
     .job_type      = "stream",
+    .set_speed     = stream_set_speed,
 };
 
 int stream_start(BlockDriverState *bs, BlockDriverState *base,
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 5/9] qmp: add block_stream command

[Stefan Hajnoczi]

Add the block_stream command, which starts copy backing file contents
into the image file.  Later patches add control over the background copy
speed, cancelation, and querying running streaming operations.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx  |   13 ++++++++++
 hmp.c            |   14 +++++++++++
 hmp.h            |    1 +
 monitor.c        |    3 ++
 monitor.h        |    1 +
 qapi-schema.json |   53 ++++++++++++++++++++++++++++++++++++++++++
 qerror.c         |    4 +++
 qerror.h         |    3 ++
 qmp-commands.hx  |   18 ++++++++++++++
 trace-events     |    4 +++
 11 files changed, 182 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index dbf0251..08355cf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -13,8 +13,11 @@
 #include "qerror.h"
 #include "qemu-option.h"
 #include "qemu-config.h"
+#include "qemu-objects.h"
 #include "sysemu.h"
 #include "block_int.h"
+#include "qmp-commands.h"
+#include "trace.h"
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
@@ -887,3 +890,68 @@ int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
     return 0;
 }
+
+static QObject *qobject_from_block_job(BlockJob *job)
+{
+    return qobject_from_jsonf("{ 'type': %s,"
+                              "'device': %s,"
+                              "'len': %" PRId64 ","
+                              "'offset': %" PRId64 ","
+                              "'speed': %" PRId64 " }",
+                              job->job_type->job_type,
+                              bdrv_get_device_name(job->bs),
+                              job->len,
+                              job->offset,
+                              job->speed);
+}
+
+static void block_stream_cb(void *opaque, int ret)
+{
+    BlockDriverState *bs = opaque;
+    QObject *obj;
+
+    trace_block_stream_cb(bs, bs->job, ret);
+
+    assert(bs->job);
+    obj = qobject_from_block_job(bs->job);
+    if (ret < 0) {
+        QDict *dict = qobject_to_qdict(obj);
+        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
+    }
+
+    monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj);
+    qobject_decref(obj);
+}
+
+void qmp_block_stream(const char *device, bool has_base,
+                      const char *base, Error **errp)
+{
+    BlockDriverState *bs;
+    int ret;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    /* Base device not supported */
+    if (base) {
+        error_set(errp, QERR_NOT_SUPPORTED);
+        return;
+    }
+
+    ret = stream_start(bs, NULL, block_stream_cb, bs);
+    if (ret < 0) {
+        switch (ret) {
+        case -EBUSY:
+            error_set(errp, QERR_DEVICE_IN_USE, device);
+            return;
+        default:
+            error_set(errp, QERR_NOT_SUPPORTED);
+            return;
+        }
+    }
+
+    trace_qmp_block_stream(bs, bs->job);
+}
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 79a9195..c0de41c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -70,6 +70,19 @@ but should be used with extreme caution.  Note that this command only
 resizes image files, it can not resize block devices like LVM volumes.
 ETEXI
 
+    {
+        .name       = "block_stream",
+        .args_type  = "device:B,base:s?",
+        .params     = "device [base]",
+        .help       = "copy data from a backing file into a block device",
+        .mhandler.cmd = hmp_block_stream,
+    },
+
+STEXI
+@item block_stream
+@findex block_stream
+Copy data from a backing file into a block device.
+ETEXI
 
     {
         .name       = "eject",
diff --git a/hmp.c b/hmp.c
index dfab7ad..8b7d7d4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -531,3 +531,17 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "invalid CPU index\n");
     }
 }
+
+void hmp_block_stream(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+    const char *base = qdict_get_try_str(qdict, "base");
+
+    qmp_block_stream(device, base != NULL, base, &error);
+
+    if (error) {
+        monitor_printf(mon, "%s\n", error_get_pretty(error));
+        error_free(error);
+    }
+}
diff --git a/hmp.h b/hmp.h
index 4422578..3cb6fe5 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,5 +37,6 @@ void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
 void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
+void hmp_block_stream(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index 1be222e..a33218c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_SPICE_DISCONNECTED:
             event_name = "SPICE_DISCONNECTED";
             break;
+        case QEVENT_BLOCK_JOB_COMPLETED:
+            event_name = "BLOCK_JOB_COMPLETED";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index e76795f..d9b07dc 100644
--- a/monitor.h
+++ b/monitor.h
@@ -35,6 +35,7 @@ typedef enum MonitorEvent {
     QEVENT_SPICE_CONNECTED,
     QEVENT_SPICE_INITIALIZED,
     QEVENT_SPICE_DISCONNECTED,
+    QEVENT_BLOCK_JOB_COMPLETED,
     QEVENT_MAX,
 } MonitorEvent;
 
diff --git a/qapi-schema.json b/qapi-schema.json
index fbbdbe0..65308d2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -901,3 +901,56 @@
 # Notes: Do not use this command.
 ##
 { 'command': 'cpu', 'data': {'index': 'int'} }
+
+##
+# @block_stream:
+#
+# Copy data from a backing file into a block device.
+#
+# The block streaming operation is performed in the background until the entire
+# backing file has been copied.  This command returns immediately once streaming
+# has started.  The status of ongoing block streaming operations can be checked
+# with query-block-jobs.  The operation can be stopped before it has completed
+# using the block_job_cancel command.
+#
+# If a base file is specified then sectors are not copied from that base file and
+# its backing chain.  When streaming completes the image file will have the base
+# file as its backing file.  This can be used to stream a subset of the backing
+# file chain instead of flattening the entire image.
+#
+# On successful completion the image file is updated to drop the backing file.
+#
+# Arguments:
+#
+# @device: device name
+# @base:   common backing file
+#
+# Errors:
+#
+# DeviceInUse:    streaming is already active on this device
+# DeviceNotFound: device name is invalid
+# NotSupported:   image streaming is not supported by this device
+#
+# Events:
+#
+# On completion the BLOCK_JOB_COMPLETED event is raised with the following
+# fields:
+#
+# - type:     job type ("stream" for image streaming, json-string)
+# - device:   device name (json-string)
+# - len:      maximum progress value (json-int)
+# - offset:   current progress value (json-int)
+# - speed:    rate limit, bytes per second (json-int)
+# - error:    error message (json-string, only on error)
+#
+# The completion event is raised both on success and on failure.  On
+# success offset is equal to len.  On failure offset and len can be
+# used to indicate at which point the operation failed.
+#
+# On failure the error field contains a human-readable error message.  There are
+# no semantics other than that streaming has failed and clients should not try
+# to interpret the error string.
+#
+# Since: 1.1
+##
+{ 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
diff --git a/qerror.c b/qerror.c
index 14a1d59..605381a 100644
--- a/qerror.c
+++ b/qerror.c
@@ -174,6 +174,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "No '%(bus)' bus found for device '%(device)'",
     },
     {
+        .error_fmt = QERR_NOT_SUPPORTED,
+        .desc      = "Not supported",
+    },
+    {
         .error_fmt = QERR_OPEN_FILE_FAILED,
         .desc      = "Could not open '%(filename)'",
     },
diff --git a/qerror.h b/qerror.h
index 560d458..b5c0cfe 100644
--- a/qerror.h
+++ b/qerror.h
@@ -147,6 +147,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_NO_BUS_FOR_DEVICE \
     "{ 'class': 'NoBusForDevice', 'data': { 'device': %s, 'bus': %s } }"
 
+#define QERR_NOT_SUPPORTED \
+    "{ 'class': 'NotSupported', 'data': {} }"
+
 #define QERR_OPEN_FILE_FAILED \
     "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 94da2a8..8c1c934 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -683,6 +683,24 @@ Example:
 EQMP
 
     {
+        .name       = "block_stream",
+        .args_type  = "device:B,base:s?",
+        .mhandler.cmd_new = qmp_marshal_input_block_stream,
+    },
+
+SQMP
+block_stream
+------------
+See qapi-schema.json for documentation.
+
+Examples:
+
+-> { "execute": "block_stream", "arguments": { "device": "virtio0" } }
+<- { "return":  {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-sync",
         .args_type  = "device:B,snapshot-file:s?,format:s?",
         .params     = "device [new-image-file] [format]",
diff --git a/trace-events b/trace-events
index 4efcd95..6c1eec2 100644
--- a/trace-events
+++ b/trace-events
@@ -73,6 +73,10 @@ bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t clus
 stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
 stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
 
+# blockdev.c
+block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
+qmp_block_stream(void *bs, void *job) "bs %p job %p"
+
 # hw/virtio-blk.c
 virtio_blk_req_complete(void *req, int status) "req %p status %d"
 virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 6/9] qmp: add block_job_set_speed command

[Stefan Hajnoczi]

Add block_job_set_speed, which sets the maximum speed for a background
block operation.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c       |   25 +++++++++++++++++++++++++
 hmp-commands.hx  |   14 ++++++++++++++
 hmp.c            |   14 ++++++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   23 +++++++++++++++++++++++
 qmp-commands.hx  |   18 ++++++++++++++++++
 6 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 08355cf..b101ba1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -955,3 +955,28 @@ void qmp_block_stream(const char *device, bool has_base,
 
     trace_qmp_block_stream(bs, bs->job);
 }
+
+static BlockJob *find_block_job(const char *device)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_find(device);
+    if (!bs || !bs->job) {
+        return NULL;
+    }
+    return bs->job;
+}
+
+void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp)
+{
+    BlockJob *job = find_block_job(device);
+
+    if (!job) {
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, device);
+        return;
+    }
+
+    if (block_job_set_speed(job, value) < 0) {
+        error_set(errp, QERR_NOT_SUPPORTED);
+    }
+}
diff --git a/hmp-commands.hx b/hmp-commands.hx
index c0de41c..788ec97 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -85,6 +85,20 @@ Copy data from a backing file into a block device.
 ETEXI
 
     {
+        .name       = "block_job_set_speed",
+        .args_type  = "device:B,value:o",
+        .params     = "device value",
+        .help       = "set maximum speed for a background block operation",
+        .mhandler.cmd = hmp_block_job_set_speed,
+    },
+
+STEXI
+@item block_job_set_stream
+@findex block_job_set_stream
+Set maximum speed for a background block operation.
+ETEXI
+
+    {
         .name       = "eject",
         .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
diff --git a/hmp.c b/hmp.c
index 8b7d7d4..f3edf7c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -545,3 +545,17 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
         error_free(error);
     }
 }
+
+void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+    int64_t value = qdict_get_int(qdict, "value");
+
+    qmp_block_job_set_speed(device, value, &error);
+
+    if (error) {
+        monitor_printf(mon, "%s\n", error_get_pretty(error));
+        error_free(error);
+    }
+}
diff --git a/hmp.h b/hmp.h
index 3cb6fe5..4c1458e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -38,5 +38,6 @@ void hmp_system_reset(Monitor *mon, const QDict *qdict);
 void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_block_stream(Monitor *mon, const QDict *qdict);
+void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 65308d2..f6a8252 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -954,3 +954,26 @@
 # Since: 1.1
 ##
 { 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
+
+##
+# @block_job_set_speed:
+#
+# Set maximum speed for a background block operation.
+#
+# This command can only be issued when there is an active block job.
+#
+# Throttling can be disabled by setting the speed to 0.
+#
+# Arguments:
+#
+# - device: device name
+# - value:  maximum speed, in bytes per second
+#
+# Errors:
+# NotSupported:    job type does not support speed setting
+# DeviceNotActive: streaming is not active on this device
+#
+# Since: 1.1
+##
+{ 'command': 'block_job_set_speed',
+  'data': { 'device': 'str', 'value': 'int' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8c1c934..5541ca0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -701,6 +701,24 @@ Examples:
 EQMP
 
     {
+        .name       = "block_job_set_speed",
+        .args_type  = "device:B,value:o",
+        .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
+    },
+
+SQMP
+block_job_set_speed
+-------------------
+See qapi-schema.json for documentation.
+
+Example:
+
+-> { "execute": "block_job_set_speed",
+    "arguments": { "device": "virtio0", "value": 1024 } }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-sync",
         .args_type  = "device:B,snapshot-file:s?,format:s?",
         .params     = "device [new-image-file] [format]",
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 7/9] qmp: add block_job_cancel command

[Stefan Hajnoczi]

Add block_job_cancel, which stops an active block streaming operation.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c       |   19 ++++++++++++++++++-
 hmp-commands.hx  |   14 ++++++++++++++
 hmp.c            |   13 +++++++++++++
 hmp.h            |    1 +
 monitor.c        |    3 +++
 monitor.h        |    1 +
 qapi-schema.json |   36 ++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   18 ++++++++++++++++++
 trace-events     |    1 +
 9 files changed, 105 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b101ba1..b276b2f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -919,7 +919,11 @@ static void block_stream_cb(void *opaque, int ret)
         qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
     }
 
-    monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj);
+    if (block_job_is_cancelled(bs->job)) {
+        monitor_protocol_event(QEVENT_BLOCK_JOB_CANCELLED, obj);
+    } else {
+        monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj);
+    }
     qobject_decref(obj);
 }
 
@@ -980,3 +984,16 @@ void qmp_block_job_set_speed(const char *device, int64_t value, Error **errp)
         error_set(errp, QERR_NOT_SUPPORTED);
     }
 }
+
+void qmp_block_job_cancel(const char *device, Error **errp)
+{
+    BlockJob *job = find_block_job(device);
+
+    if (!job) {
+        error_set(errp, QERR_DEVICE_NOT_ACTIVE, device);
+        return;
+    }
+
+    trace_qmp_block_job_cancel(job);
+    block_job_cancel(job);
+}
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 788ec97..cc91055 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -99,6 +99,20 @@ Set maximum speed for a background block operation.
 ETEXI
 
     {
+        .name       = "block_job_cancel",
+        .args_type  = "device:B",
+        .params     = "device",
+        .help       = "stop an active block streaming operation",
+        .mhandler.cmd = hmp_block_job_cancel,
+    },
+
+STEXI
+@item block_job_cancel
+@findex block_job_cancel
+Stop an active block streaming operation.
+ETEXI
+
+    {
         .name       = "eject",
         .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
diff --git a/hmp.c b/hmp.c
index f3edf7c..66d9d0f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -559,3 +559,16 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
         error_free(error);
     }
 }
+
+void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *device = qdict_get_str(qdict, "device");
+
+    qmp_block_job_cancel(device, &error);
+
+    if (error) {
+        monitor_printf(mon, "%s\n", error_get_pretty(error));
+        error_free(error);
+    }
+}
diff --git a/hmp.h b/hmp.h
index 4c1458e..30d9051 100644
--- a/hmp.h
+++ b/hmp.h
@@ -39,5 +39,6 @@ void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_block_stream(Monitor *mon, const QDict *qdict);
 void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
+void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index a33218c..5bf29f2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -482,6 +482,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_BLOCK_JOB_COMPLETED:
             event_name = "BLOCK_JOB_COMPLETED";
             break;
+        case QEVENT_BLOCK_JOB_CANCELLED:
+            event_name = "BLOCK_JOB_CANCELLED";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index d9b07dc..2e7f38a 100644
--- a/monitor.h
+++ b/monitor.h
@@ -36,6 +36,7 @@ typedef enum MonitorEvent {
     QEVENT_SPICE_INITIALIZED,
     QEVENT_SPICE_DISCONNECTED,
     QEVENT_BLOCK_JOB_COMPLETED,
+    QEVENT_BLOCK_JOB_CANCELLED,
     QEVENT_MAX,
 } MonitorEvent;
 
diff --git a/qapi-schema.json b/qapi-schema.json
index f6a8252..270d05e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -943,6 +943,10 @@
 # - speed:    rate limit, bytes per second (json-int)
 # - error:    error message (json-string, only on error)
 #
+# If the operation was cancelled using block_job_cancel then the
+# BLOCK_JOB_CANCELLED event is raised with the same fields as
+# BLOCK_JOB_COMPLETED.
+#
 # The completion event is raised both on success and on failure.  On
 # success offset is equal to len.  On failure offset and len can be
 # used to indicate at which point the operation failed.
@@ -977,3 +981,35 @@
 ##
 { 'command': 'block_job_set_speed',
   'data': { 'device': 'str', 'value': 'int' } }
+
+##
+# @block_job_cancel:
+#
+# Stop an active block streaming operation.
+#
+# This command returns immediately after marking the active block streaming
+# operation for cancellation.  It is an error to call this command if no
+# operation is in progress.
+#
+# The operation will cancel as soon as possible and emit the
+# BLOCK_JOB_CANCELLED event.  Before that happens the job is still visible when
+# enumerated using query-block-jobs.
+#
+# The image file retains its backing file unless the streaming operation happens
+# to complete just as it is being cancelled.
+#
+# A new block streaming operation can be started at a later time to finish
+# copying all data from the backing file.
+#
+# Arguments:
+#
+# - device: device name
+#
+# Errors:
+#
+# DeviceNotActive: streaming is not active on this device
+# DeviceInUse:     cancellation already in progress
+#
+# Since: 1.1
+##
+{ 'command': 'block_job_cancel', 'data': { 'device': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5541ca0..a57d079 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -719,6 +719,24 @@ Example:
 EQMP
 
     {
+        .name       = "block_job_cancel",
+        .args_type  = "device:B",
+        .mhandler.cmd_new = qmp_marshal_input_block_job_cancel,
+    },
+
+SQMP
+block_job_cancel
+----------------
+See qapi-schema.json for documentation.
+
+Examples:
+
+-> { "execute": "block_job_cancel", "arguments": { "device": "virtio0" } }
+<- { "return":  {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-sync",
         .args_type  = "device:B,snapshot-file:s?,format:s?",
         .params     = "device [new-image-file] [format]",
diff --git a/trace-events b/trace-events
index 6c1eec2..cbca6d5 100644
--- a/trace-events
+++ b/trace-events
@@ -74,6 +74,7 @@ stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocat
 stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
 
 # blockdev.c
+qmp_block_job_cancel(void *job) "job %p"
 block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
 qmp_block_stream(void *bs, void *job) "bs %p job %p"
 
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Stefan Hajnoczi]

Add query-block-jobs, which shows the progress of ongoing block device
operations.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 blockdev.c       |   33 +++++++++++++++++++++++++++++++++
 hmp.c            |   40 ++++++++++++++++++++++++++++++++++++++++
 hmp.h            |    1 +
 monitor.c        |    7 +++++++
 qapi-schema.json |   32 ++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   39 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b276b2f..5b2b128 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -997,3 +997,36 @@ void qmp_block_job_cancel(const char *device, Error **errp)
     trace_qmp_block_job_cancel(job);
     block_job_cancel(job);
 }
+
+static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
+{
+    BlockJobInfoList **prev = opaque;
+    BlockJob *job = bs->job;
+
+    if (job) {
+        BlockJobInfoList *elem;
+        BlockJobInfo *info = g_new(BlockJobInfo, 1);
+        *info = (BlockJobInfo){
+            .type = g_strdup(job->job_type->job_type),
+            .device = g_strdup(bdrv_get_device_name(bs)),
+            .len = job->len,
+            .offset = job->offset,
+            .speed = job->speed,
+        };
+
+        elem = g_new0(BlockJobInfoList, 1);
+        elem->value = info;
+
+        (*prev)->next = elem;
+        *prev = elem;
+    }
+}
+
+BlockJobInfoList *qmp_query_block_jobs(Error **errp)
+{
+    /* Dummy is a fake list element for holding the head pointer */
+    BlockJobInfoList dummy = {};
+    BlockJobInfoList *prev = &dummy;
+    bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
+    return dummy.next;
+}
diff --git a/hmp.c b/hmp.c
index 66d9d0f..c16d6a1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
     qapi_free_PciInfoList(info);
 }
 
+void hmp_info_block_jobs(Monitor *mon)
+{
+    BlockJobInfoList *list;
+    Error *err = NULL;
+
+    list = qmp_query_block_jobs(&err);
+    assert(!err);
+
+    if (!list) {
+        monitor_printf(mon, "No active jobs\n");
+        return;
+    }
+
+    while (list) {
+        /* The HMP output for streaming jobs is special because historically it
+         * was different from other job types so applications may depend on the
+         * exact string.
+         */
+        if (strcmp(list->value->type, "stream") == 0) {
+            monitor_printf(mon, "Streaming device %s: Completed %" PRId64
+                           " of %" PRId64 " bytes, speed limit %" PRId64
+                           " bytes/s\n",
+                           list->value->device,
+                           list->value->offset,
+                           list->value->len,
+                           list->value->speed);
+        } else {
+            monitor_printf(mon, "Type %s, device %s: Completed %" PRId64
+                           " of %" PRId64 " bytes, speed limit %" PRId64
+                           " bytes/s\n",
+                           list->value->type,
+                           list->value->device,
+                           list->value->offset,
+                           list->value->len,
+                           list->value->speed);
+        }
+        list = list->next;
+    }
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 30d9051..9178b09 100644
--- a/hmp.h
+++ b/hmp.h
@@ -32,6 +32,7 @@ void hmp_info_vnc(Monitor *mon);
 void hmp_info_spice(Monitor *mon);
 void hmp_info_balloon(Monitor *mon);
 void hmp_info_pci(Monitor *mon);
+void hmp_info_block_jobs(Monitor *mon);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 5bf29f2..ee22e58 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2641,6 +2641,13 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.info = hmp_info_blockstats,
     },
     {
+        .name       = "block-jobs",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show progress of ongoing block device operations",
+        .mhandler.info = hmp_info_block_jobs,
+    },
+    {
         .name       = "registers",
         .args_type  = "",
         .params     = "",
diff --git a/qapi-schema.json b/qapi-schema.json
index 270d05e..f1d56c0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -845,6 +845,38 @@
 { 'command': 'query-pci', 'returns': ['PciInfo'] }
 
 ##
+# @BlockJobInfo:
+#
+# Information about a long-running block device operation.
+#
+# @type: the job type ('stream' for image streaming)
+#
+# @device: the block device name
+#
+# @len: the maximum progress value
+#
+# @offset: the current progress value
+#
+# @speed: the rate limit, bytes per second
+#
+# Since: 1.1
+##
+{ 'type': 'BlockJobInfo',
+  'data': {'type': 'str', 'device': 'str', 'len': 'int',
+           'offset': 'int', 'speed': 'int'} }
+
+##
+# @query-block-jobs:
+#
+# Return information about long-running block device operations.
+#
+# Returns: a list of @BlockJobInfo for each active block job
+#
+# Since: 1.1
+##
+{ 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
+
+##
 # @quit:
 #
 # This command will cause the QEMU process to exit gracefully.  While every
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a57d079..7fd968d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2106,3 +2106,42 @@ EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_balloon,
     },
+
+SQMP
+
+query-block-jobs
+----------------
+
+Show progress of ongoing block device operations.
+
+Return a json-array of all block device operations.  If no operation is active
+then return an empty array.  Each operation is a json-object with the following
+data:
+
+- type:     job type ("stream" for image streaming, json-string)
+- device:   device name (json-string)
+- len:      maximum progress value (json-int)
+- offset:   current progress value (json-int)
+- speed:    rate limit, bytes per second (json-int)
+
+Progress can be observed as offset increases and it reaches len when the
+operation completes.  Offset and len have undefined units but can be used to
+calculate a percentage indicating the progress that has been made.
+
+Example:
+
+-> { "execute": "query-block-jobs" }
+<- { "return":[
+      { "type": "stream", "device": "virtio0",
+        "len": 10737418240, "offset": 709632,
+        "speed": 0 }
+   ]
+ }
+
+EQMP
+
+    {
+        .name       = "query-block-jobs",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_block_jobs,
+    },
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 9/9] test: add image streaming test cases

[Stefan Hajnoczi]

python test-stream.py

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 test-stream.py |  208 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 test-stream.py

diff --git a/test-stream.py b/test-stream.py
new file mode 100644
index 0000000..16cbe5d
--- /dev/null
+++ b/test-stream.py
@@ -0,0 +1,208 @@
+#!/usr/bin/env python
+import unittest
+import subprocess
+import re
+import os
+import sys; sys.path.append('QMP/')
+import qmp
+
+def qemu_img(*args):
+    devnull = open('/dev/null', 'r+')
+    return subprocess.call(['./qemu-img'] + list(args), stdin=devnull, stdout=devnull)
+
+def qemu_io(*args):
+    args = ['./qemu-io'] + list(args)
+    return subprocess.Popen(args, stdout=subprocess.PIPE).communicate()[0]
+
+class VM(object):
+    def __init__(self):
+        self._monitor_path = '/tmp/qemu-mon.%d' % os.getpid()
+        self._qemu_log_path = '/tmp/qemu-log.%d' % os.getpid()
+        self._args = ['x86_64-softmmu/qemu-system-x86_64',
+                      '-chardev', 'socket,id=mon,path=' + self._monitor_path,
+                      '-mon', 'chardev=mon,mode=control', '-nographic']
+        self._num_drives = 0
+
+    def add_drive(self, path, opts=''):
+        options = ['if=virtio',
+                   'cache=none',
+                   'file=%s' % path,
+                   'id=drive%d' % self._num_drives]
+        if opts:
+            options.append(opts)
+
+        self._args.append('-drive')
+        self._args.append(','.join(options))
+        self._num_drives += 1
+        return self
+
+    def launch(self):
+        devnull = open('/dev/null', 'rb')
+        qemulog = open(self._qemu_log_path, 'wb')
+        self._qmp = qmp.QEMUMonitorProtocol(self._monitor_path, server=True)
+        self._popen = subprocess.Popen(self._args, stdin=devnull, stdout=qemulog,
+                                       stderr=subprocess.STDOUT)
+        self._qmp.accept()
+
+    def shutdown(self):
+        self._qmp.cmd('quit')
+        self._popen.wait()
+        os.remove(self._monitor_path)
+        os.remove(self._qemu_log_path)
+
+    def qmp(self, cmd, **args):
+        return self._qmp.cmd(cmd, args=args)
+
+    def get_qmp_events(self, wait=False):
+        events = self._qmp.get_events(wait=wait)
+        self._qmp.clear_events()
+        return events
+
+index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
+
+class QMPTestCase(unittest.TestCase):
+    def dictpath(self, d, path):
+        """Traverse a path in a nested dict"""
+        for component in path.split('/'):
+            m = index_re.match(component)
+            if m:
+                component, idx = m.groups()
+                idx = int(idx)
+
+            if not isinstance(d, dict) or component not in d:
+                self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+            d = d[component]
+
+            if m:
+                if not isinstance(d, list):
+                    self.fail('path component "%s" in "%s" is not a list in "%s"' % (component, path, str(d)))
+                try:
+                    d = d[idx]
+                except IndexError:
+                    self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
+        return d
+
+    def assert_qmp(self, d, path, value):
+        result = self.dictpath(d, path)
+        self.assertEqual(result, value, 'values not equal "%s" and "%s"' % (str(result), str(value)))
+
+    def assert_no_active_streams(self):
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return', [])
+
+class TestSingleDrive(QMPTestCase):
+    image_len = 1 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', 'backing.img', str(TestSingleDrive.image_len))
+        qemu_img('create', '-f', 'qed', '-o', 'backing_file=backing.img', 'test.qed')
+        self.vm = VM().add_drive('test.qed')
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove('test.qed')
+        os.remove('backing.img')
+
+    def test_stream(self):
+        self.assert_no_active_streams()
+
+        result = self.vm.qmp('block_stream', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        completed = False
+        while not completed:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp(event, 'data/type', 'stream')
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp(event, 'data/offset', self.image_len)
+                    self.assert_qmp(event, 'data/len', self.image_len)
+                    completed = True
+
+        self.assert_no_active_streams()
+
+        self.assertFalse('sectors not allocated' in qemu_io('-c', 'map', 'test.qed'),
+                         'image file not fully populated after streaming')
+
+    def test_device_not_found(self):
+        result = self.vm.qmp('block_stream', device='nonexistent')
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+class TestStreamStop(QMPTestCase):
+    image_len = 8 * 1024 * 1024 * 1024 # GB
+
+    def setUp(self):
+        qemu_img('create', 'backing.img', str(TestStreamStop.image_len))
+        qemu_img('create', '-f', 'qed', '-o', 'backing_file=backing.img', 'test.qed')
+        self.vm = VM().add_drive('test.qed')
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove('test.qed')
+        os.remove('backing.img')
+
+    def test_stream_stop(self):
+        import time
+
+        self.assert_no_active_streams()
+
+        result = self.vm.qmp('block_stream', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        time.sleep(1)
+        events = self.vm.get_qmp_events(wait=False)
+        self.assertEqual(events, [], 'unexpected QMP event: %s' % events)
+
+        self.vm.qmp('block_job_cancel', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        cancelled = False
+        while not cancelled:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_CANCELLED':
+                    self.assert_qmp(event, 'data/type', 'stream')
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    cancelled = True
+
+        self.assert_no_active_streams()
+
+class TestSetSpeed(QMPTestCase):
+    image_len = 80 * 1024 * 1024 # MB
+
+    def setUp(self):
+        qemu_img('create', 'backing.img', str(TestSetSpeed.image_len))
+        qemu_img('create', '-f', 'qed', '-o', 'backing_file=backing.img', 'test.qed')
+        self.vm = VM().add_drive('test.qed')
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove('test.qed')
+        os.remove('backing.img')
+
+    # This doesn't print or verify anything, only use it via "test-stream.py TestSetSpeed"
+    def test_stream_set_speed(self):
+        self.assert_no_active_streams()
+
+        result = self.vm.qmp('block_stream', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block_job_set_speed', device='drive0', value=8 * 1024 * 1024)
+        self.assert_qmp(result, 'return', {})
+
+        completed = False
+        while not completed:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp(event, 'data/type', 'stream')
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp(event, 'data/offset', self.image_len)
+                    self.assert_qmp(event, 'data/len', self.image_len)
+                    completed = True
+
+        self.assert_no_active_streams()
+
+if __name__ == '__main__':
+    unittest.main()
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH v3 0/9] block: generic image streaming

[Yibin Shen]

hi stefan:

all these patches looks good to me except one thing,
when I run a "qemu-img commit" command,
seems entire image(from start to end sector) will be write to the backing file,
I think what we really need is to commit only dirty sectors.
also maybe we can use a writeback mechanism alternaively.

Yibin Shen


On Tue, Dec 13, 2011 at 9:52 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> This series adds the 'block_stream' command which copies the contents of a
> backing file into the image file while the VM is running.  The series builds on
> copy-on-read and zero detection features which I sent out recently and I
> suggest grabbing my git tree to try it out without merging these dependencies
> yourself:
>
> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/image-streaming-api
>
> The image streaming HMP/QMP commands are documented in the patch and also
> described here:
>
> http://wiki.qemu.org/Features/LiveBlockMigration/ImageStreamingAPI
>
> The basic idea is to execute 'block_stream virtio0' while the guest is running.
> Progress can be monitored using 'info block-jobs'.  When the streaming
> operation completes it raises a QMP event.
>
> Note: The last patch includes includes a Python test script called
> test-stream.py, I do not propose to merge it.  When run in a QEMU source tree
> it performs basic image streaming QMP tests.
>
> Stefan Hajnoczi (9):
>  coroutine: add co_sleep_ns() coroutine sleep function
>  block: add BlockJob interface for long-running operations
>  block: add image streaming block job
>  block: rate-limit streaming operations
>  qmp: add block_stream command
>  qmp: add block_job_set_speed command
>  qmp: add block_job_cancel command
>  qmp: add query-block-jobs
>  test: add image streaming test cases
>
>  Makefile.objs          |    3 +-
>  block/stream.c         |  174 ++++++++++++++++++++++++++++++++++++++++
>  block_int.h            |   86 ++++++++++++++++++++
>  blockdev.c             |  143 +++++++++++++++++++++++++++++++++
>  hmp-commands.hx        |   41 ++++++++++
>  hmp.c                  |   81 +++++++++++++++++++
>  hmp.h                  |    4 +
>  monitor.c              |   13 +++
>  monitor.h              |    2 +
>  qapi-schema.json       |  144 +++++++++++++++++++++++++++++++++
>  qemu-coroutine-sleep.c |   38 +++++++++
>  qemu-coroutine.h       |    6 ++
>  qerror.c               |    4 +
>  qerror.h               |    3 +
>  qmp-commands.hx        |   93 +++++++++++++++++++++
>  test-stream.py         |  208 ++++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events           |    9 ++
>  17 files changed, 1051 insertions(+), 1 deletions(-)
>  create mode 100644 block/stream.c
>  create mode 100644 qemu-coroutine-sleep.c
>  create mode 100644 test-stream.py
>
> --
> 1.7.7.3
>
>

________________________________

This email (including any attachments) is confidential and may be legally privileged. If you received this email in error, please delete it immediately and do not copy it or use it for any purpose or disclose its contents to any other person. Thank you.

本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正确的收件人，请您立即删除本邮件。请不要将本电邮进行复制并用作任何其他用途、或透露本邮件之内容。谢谢。

Re: [Qemu-devel] [PATCH v3 3/9] block: add image streaming block job

[Marcelo Tosatti]

On Tue, Dec 13, 2011 at 01:52:25PM +0000, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  Makefile.objs  |    1 +
>  block/stream.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block_int.h    |    3 +
>  trace-events   |    4 ++
>  4 files changed, 129 insertions(+), 0 deletions(-)
>  create mode 100644 block/stream.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index d737d81..025cc08 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -34,6 +34,7 @@ block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow
>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-nested-y += qed-check.o
>  block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> +block-nested-y += stream.o
>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>  block-nested-$(CONFIG_POSIX) += raw-posix.o
>  block-nested-$(CONFIG_LIBISCSI) += iscsi.o
> diff --git a/block/stream.c b/block/stream.c
> new file mode 100644
> index 0000000..7d362ab
> --- /dev/null
> +++ b/block/stream.c
> @@ -0,0 +1,121 @@
> +/*
> + * Image streaming
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "trace.h"
> +#include "block_int.h"
> +
> +enum {
> +    /*
> +     * Size of data buffer for populating the image file.  This should be large
> +     * enough to process multiple clusters in a single call, so that populating
> +     * contiguous regions of the image is efficient.
> +     */
> +    STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
> +};
> +
> +typedef struct StreamBlockJob {
> +    BlockJob common;
> +    BlockDriverState *base;
> +} StreamBlockJob;
> +
> +static int coroutine_fn stream_populate(BlockDriverState *bs,
> +                                        int64_t sector_num, int nb_sectors,
> +                                        void *buf)
> +{
> +    struct iovec iov = {
> +        .iov_base = buf,
> +        .iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
> +    };
> +    QEMUIOVector qiov;
> +
> +    qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +    /* Copy-on-read the unallocated clusters */
> +    return bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
> +}
> +
> +static void coroutine_fn stream_run(void *opaque)
> +{
> +    StreamBlockJob *s = opaque;
> +    BlockDriverState *bs = s->common.bs;
> +    int64_t sector_num, end;
> +    int ret = 0;
> +    int n;
> +    void *buf;
> +
> +    buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
> +    s->common.len = bdrv_getlength(bs);
> +    bdrv_get_geometry(bs, (uint64_t *)&end);
> +
> +    bdrv_enable_zero_detection(bs);
> +    bdrv_enable_copy_on_read(bs);
> +
> +    for (sector_num = 0; sector_num < end; sector_num += n) {
> +        if (block_job_is_cancelled(&s->common)) {
> +            break;
> +        }
> +
> +        /* TODO rate-limit */
> +        /* Note that even when no rate limit is applied we need to yield with
> +         * no pending I/O here so that qemu_aio_flush() is able to return.
> +         */
> +        co_sleep_ns(rt_clock, 0);
> +
> +        ret = bdrv_co_is_allocated(bs, sector_num,
> +                                   STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
> +        trace_stream_one_iteration(s, sector_num, n, ret);
> +        if (ret == 0) {
> +            ret = stream_populate(bs, sector_num, n, buf);
> +        }
> +        if (ret < 0) {
> +            break;
> +        }
> +
> +        /* Publish progress */
> +        s->common.offset += n * BDRV_SECTOR_SIZE;
> +    }
> +
> +    bdrv_disable_copy_on_read(bs);
> +    bdrv_disable_zero_detection(bs);
> +
> +    if (sector_num == end && ret == 0) {
> +        bdrv_change_backing_file(bs, NULL, NULL);
> +    }

In fact there is no need for a new command to switch backing file,
it can be done here for the shared base case also.

Re: [Qemu-devel] [PATCH v3 0/9] block: generic image streaming

[Stefan Hajnoczi]

On Tue, Dec 13, 2011 at 2:12 PM, Yibin Shen <zituan@taobao.com> wrote:
> all these patches looks good to me except one thing,
> when I run a "qemu-img commit" command,
> seems entire image(from start to end sector) will be write to the backing file,
> I think what we really need is to commit only dirty sectors.
> also maybe we can use a writeback mechanism alternaively.

This series does not affect the behavior of "qemu-img commit".

Are you suggesting a new feature?  Please explain a little more.

Stefan

Re: [Qemu-devel] [PATCH v3 3/9] block: add image streaming block job

[Stefan Hajnoczi]

On Tue, Dec 13, 2011 at 2:14 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 01:52:25PM +0000, Stefan Hajnoczi wrote:
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> ---
>>  Makefile.objs  |    1 +
>>  block/stream.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  block_int.h    |    3 +
>>  trace-events   |    4 ++
>>  4 files changed, 129 insertions(+), 0 deletions(-)
>>  create mode 100644 block/stream.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index d737d81..025cc08 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -34,6 +34,7 @@ block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow
>>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>>  block-nested-y += qed-check.o
>>  block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> +block-nested-y += stream.o
>>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>>  block-nested-$(CONFIG_POSIX) += raw-posix.o
>>  block-nested-$(CONFIG_LIBISCSI) += iscsi.o
>> diff --git a/block/stream.c b/block/stream.c
>> new file mode 100644
>> index 0000000..7d362ab
>> --- /dev/null
>> +++ b/block/stream.c
>> @@ -0,0 +1,121 @@
>> +/*
>> + * Image streaming
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "trace.h"
>> +#include "block_int.h"
>> +
>> +enum {
>> +    /*
>> +     * Size of data buffer for populating the image file.  This should be large
>> +     * enough to process multiple clusters in a single call, so that populating
>> +     * contiguous regions of the image is efficient.
>> +     */
>> +    STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
>> +};
>> +
>> +typedef struct StreamBlockJob {
>> +    BlockJob common;
>> +    BlockDriverState *base;
>> +} StreamBlockJob;
>> +
>> +static int coroutine_fn stream_populate(BlockDriverState *bs,
>> +                                        int64_t sector_num, int nb_sectors,
>> +                                        void *buf)
>> +{
>> +    struct iovec iov = {
>> +        .iov_base = buf,
>> +        .iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
>> +    };
>> +    QEMUIOVector qiov;
>> +
>> +    qemu_iovec_init_external(&qiov, &iov, 1);
>> +
>> +    /* Copy-on-read the unallocated clusters */
>> +    return bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
>> +}
>> +
>> +static void coroutine_fn stream_run(void *opaque)
>> +{
>> +    StreamBlockJob *s = opaque;
>> +    BlockDriverState *bs = s->common.bs;
>> +    int64_t sector_num, end;
>> +    int ret = 0;
>> +    int n;
>> +    void *buf;
>> +
>> +    buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
>> +    s->common.len = bdrv_getlength(bs);
>> +    bdrv_get_geometry(bs, (uint64_t *)&end);
>> +
>> +    bdrv_enable_zero_detection(bs);
>> +    bdrv_enable_copy_on_read(bs);
>> +
>> +    for (sector_num = 0; sector_num < end; sector_num += n) {
>> +        if (block_job_is_cancelled(&s->common)) {
>> +            break;
>> +        }
>> +
>> +        /* TODO rate-limit */
>> +        /* Note that even when no rate limit is applied we need to yield with
>> +         * no pending I/O here so that qemu_aio_flush() is able to return.
>> +         */
>> +        co_sleep_ns(rt_clock, 0);
>> +
>> +        ret = bdrv_co_is_allocated(bs, sector_num,
>> +                                   STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
>> +        trace_stream_one_iteration(s, sector_num, n, ret);
>> +        if (ret == 0) {
>> +            ret = stream_populate(bs, sector_num, n, buf);
>> +        }
>> +        if (ret < 0) {
>> +            break;
>> +        }
>> +
>> +        /* Publish progress */
>> +        s->common.offset += n * BDRV_SECTOR_SIZE;
>> +    }
>> +
>> +    bdrv_disable_copy_on_read(bs);
>> +    bdrv_disable_zero_detection(bs);
>> +
>> +    if (sector_num == end && ret == 0) {
>> +        bdrv_change_backing_file(bs, NULL, NULL);
>> +    }
>
> In fact there is no need for a new command to switch backing file,
> it can be done here for the shared base case also.

Agreed.

Stefan

Re: [Qemu-devel] [PATCH v3 0/9] block: generic image streaming

[Yibin Shen]

On Tue, Dec 13, 2011 at 11:18 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Dec 13, 2011 at 2:12 PM, Yibin Shen <zituan@taobao.com> wrote:
>> all these patches looks good to me except one thing,
>> when I run a "qemu-img commit" command,
>> seems entire image(from start to end sector) will be write to the backing file,
>> I think what we really need is to commit only dirty sectors.
>> also maybe we can use a writeback mechanism alternaively.
>
> This series does not affect the behavior of "qemu-img commit".
>
> Are you suggesting a new feature?  Please explain a little more.
yes , I suggest a new feature,

in some situation, we need to commit data in qed image to backing file.
in old days ,people without copy-on-read, they only have copy-on-write,
so 'qemu-img commit' commit all exsits data to backing file is rational.

but with copy-on-read or image streaming,
which means most data in the image may be identical with backing file,
so commit entire image is a redundant operation.

can we record the dirty bitmap for data differ from backing file,
then in a commit operation, we only need to handle minimum data.

thanks
>
> Stefan
>

________________________________

This email (including any attachments) is confidential and may be legally privileged. If you received this email in error, please delete it immediately and do not copy it or use it for any purpose or disclose its contents to any other person. Thank you.

本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正确的收件人，请您立即删除本邮件。请不要将本电邮进行复制并用作任何其他用途、或透露本邮件之内容。谢谢。

Re: [Qemu-devel] [PATCH v3 0/9] block: generic image streaming

[Stefan Hajnoczi]

On Wed, Dec 14, 2011 at 1:42 AM, Yibin Shen <zituan@taobao.com> wrote:
> On Tue, Dec 13, 2011 at 11:18 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Dec 13, 2011 at 2:12 PM, Yibin Shen <zituan@taobao.com> wrote:
>>> all these patches looks good to me except one thing,
>>> when I run a "qemu-img commit" command,
>>> seems entire image(from start to end sector) will be write to the backing file,
>>> I think what we really need is to commit only dirty sectors.
>>> also maybe we can use a writeback mechanism alternaively.
>>
>> This series does not affect the behavior of "qemu-img commit".
>>
>> Are you suggesting a new feature?  Please explain a little more.
> yes , I suggest a new feature,
>
> in some situation, we need to commit data in qed image to backing file.
> in old days ,people without copy-on-read, they only have copy-on-write,
> so 'qemu-img commit' commit all exsits data to backing file is rational.
>
> but with copy-on-read or image streaming,
> which means most data in the image may be identical with backing file,
> so commit entire image is a redundant operation.
>
> can we record the dirty bitmap for data differ from backing file,
> then in a commit operation, we only need to handle minimum data.

This operation is needed as a dual to the blockdev-snapshot-sync QMP
command.  It merges changes back into the base image while the VM is
running (it's basically an online version of qemu-img commit and only
copies changed data).

I agree that it's a useful operation and perhaps someone will work on
it in the future.

Stefan

Re: [Qemu-devel] [PATCH v3 2/9] block: add BlockJob interface for long-running operations

[Kevin Wolf]

Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  block_int.h |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 83 insertions(+), 0 deletions(-)

Why are these functions static inline in the header instead of being
implemented in block.c?

The other thing I was going to ask, but don't really know to which patch
I should reply, is whether we need to take care of bdrv_close/delete
while a block job is still running. What happens if you unplug a device
that is being streamed?

Kevin

Re: [Qemu-devel] [PATCH v3 3/9] block: add image streaming block job

[Kevin Wolf]

Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  Makefile.objs  |    1 +
>  block/stream.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block_int.h    |    3 +
>  trace-events   |    4 ++
>  4 files changed, 129 insertions(+), 0 deletions(-)
>  create mode 100644 block/stream.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index d737d81..025cc08 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -34,6 +34,7 @@ block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow
>  block-nested-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-nested-y += qed-check.o
>  block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
> +block-nested-y += stream.o
>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>  block-nested-$(CONFIG_POSIX) += raw-posix.o
>  block-nested-$(CONFIG_LIBISCSI) += iscsi.o
> diff --git a/block/stream.c b/block/stream.c
> new file mode 100644
> index 0000000..7d362ab
> --- /dev/null
> +++ b/block/stream.c
> @@ -0,0 +1,121 @@
> +/*
> + * Image streaming
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "trace.h"
> +#include "block_int.h"
> +
> +enum {
> +    /*
> +     * Size of data buffer for populating the image file.  This should be large
> +     * enough to process multiple clusters in a single call, so that populating
> +     * contiguous regions of the image is efficient.
> +     */
> +    STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
> +};
> +
> +typedef struct StreamBlockJob {
> +    BlockJob common;
> +    BlockDriverState *base;
> +} StreamBlockJob;
> +
> +static int coroutine_fn stream_populate(BlockDriverState *bs,
> +                                        int64_t sector_num, int nb_sectors,
> +                                        void *buf)
> +{
> +    struct iovec iov = {
> +        .iov_base = buf,
> +        .iov_len  = nb_sectors * BDRV_SECTOR_SIZE,
> +    };
> +    QEMUIOVector qiov;
> +
> +    qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +    /* Copy-on-read the unallocated clusters */
> +    return bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
> +}
> +
> +static void coroutine_fn stream_run(void *opaque)
> +{
> +    StreamBlockJob *s = opaque;
> +    BlockDriverState *bs = s->common.bs;
> +    int64_t sector_num, end;
> +    int ret = 0;
> +    int n;
> +    void *buf;
> +
> +    buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
> +    s->common.len = bdrv_getlength(bs);
> +    bdrv_get_geometry(bs, (uint64_t *)&end);
> +
> +    bdrv_enable_zero_detection(bs);

I still don't agree with this all-or-nothing switch. The default that
you want to have is zero detection on COR, but no additional overhead
for guest-initiated writes.

> +    bdrv_enable_copy_on_read(bs);
> +
> +    for (sector_num = 0; sector_num < end; sector_num += n) {
> +        if (block_job_is_cancelled(&s->common)) {
> +            break;
> +        }
> +
> +        /* TODO rate-limit */
> +        /* Note that even when no rate limit is applied we need to yield with
> +         * no pending I/O here so that qemu_aio_flush() is able to return.
> +         */
> +        co_sleep_ns(rt_clock, 0);
> +
> +        ret = bdrv_co_is_allocated(bs, sector_num,
> +                                   STREAM_BUFFER_SIZE / BDRV_SECTOR_SIZE, &n);
> +        trace_stream_one_iteration(s, sector_num, n, ret);
> +        if (ret == 0) {
> +            ret = stream_populate(bs, sector_num, n, buf);
> +        }
> +        if (ret < 0) {
> +            break;
> +        }
> +
> +        /* Publish progress */
> +        s->common.offset += n * BDRV_SECTOR_SIZE;
> +    }
> +
> +    bdrv_disable_copy_on_read(bs);
> +    bdrv_disable_zero_detection(bs);
> +
> +    if (sector_num == end && ret == 0) {
> +        bdrv_change_backing_file(bs, NULL, NULL);

Need to check the return value.

Kevin

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Kevin Wolf]

Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> Add query-block-jobs, which shows the progress of ongoing block device
> operations.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  blockdev.c       |   33 +++++++++++++++++++++++++++++++++
>  hmp.c            |   40 ++++++++++++++++++++++++++++++++++++++++
>  hmp.h            |    1 +
>  monitor.c        |    7 +++++++
>  qapi-schema.json |   32 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   39 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 152 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index b276b2f..5b2b128 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -997,3 +997,36 @@ void qmp_block_job_cancel(const char *device, Error **errp)
>      trace_qmp_block_job_cancel(job);
>      block_job_cancel(job);
>  }
> +
> +static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
> +{
> +    BlockJobInfoList **prev = opaque;
> +    BlockJob *job = bs->job;
> +
> +    if (job) {
> +        BlockJobInfoList *elem;
> +        BlockJobInfo *info = g_new(BlockJobInfo, 1);
> +        *info = (BlockJobInfo){
> +            .type = g_strdup(job->job_type->job_type),
> +            .device = g_strdup(bdrv_get_device_name(bs)),
> +            .len = job->len,
> +            .offset = job->offset,
> +            .speed = job->speed,
> +        };

Some spaces to align it nicely?

> +
> +        elem = g_new0(BlockJobInfoList, 1);
> +        elem->value = info;
> +
> +        (*prev)->next = elem;
> +        *prev = elem;

I hate these open-coded lists. But not your fault...

> +    }
> +}
> +
> +BlockJobInfoList *qmp_query_block_jobs(Error **errp)
> +{
> +    /* Dummy is a fake list element for holding the head pointer */
> +    BlockJobInfoList dummy = {};
> +    BlockJobInfoList *prev = &dummy;
> +    bdrv_iterate(do_qmp_query_block_jobs_one, &prev);
> +    return dummy.next;
> +}
> diff --git a/hmp.c b/hmp.c
> index 66d9d0f..c16d6a1 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
>      qapi_free_PciInfoList(info);
>  }
>  
> +void hmp_info_block_jobs(Monitor *mon)
> +{
> +    BlockJobInfoList *list;
> +    Error *err = NULL;
> +
> +    list = qmp_query_block_jobs(&err);
> +    assert(!err);
> +
> +    if (!list) {
> +        monitor_printf(mon, "No active jobs\n");
> +        return;
> +    }
> +
> +    while (list) {
> +        /* The HMP output for streaming jobs is special because historically it
> +         * was different from other job types so applications may depend on the
> +         * exact string.
> +         */

Er, what? This is new code. What HMP clients use this string? I know
that libvirt already got support for this before we implemented it, but
shouldn't that be QMP only?

Kevin

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Stefan Hajnoczi]

On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> > diff --git a/hmp.c b/hmp.c
> > index 66d9d0f..c16d6a1 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
> >      qapi_free_PciInfoList(info);
> >  }
> >  
> > +void hmp_info_block_jobs(Monitor *mon)
> > +{
> > +    BlockJobInfoList *list;
> > +    Error *err = NULL;
> > +
> > +    list = qmp_query_block_jobs(&err);
> > +    assert(!err);
> > +
> > +    if (!list) {
> > +        monitor_printf(mon, "No active jobs\n");
> > +        return;
> > +    }
> > +
> > +    while (list) {
> > +        /* The HMP output for streaming jobs is special because historically it
> > +         * was different from other job types so applications may depend on the
> > +         * exact string.
> > +         */
> 
> Er, what? This is new code. What HMP clients use this string? I know
> that libvirt already got support for this before we implemented it, but
> shouldn't that be QMP only?

Libvirt HMP uses this particular string, which turned out to be
sub-optimal once I realized we might support other types of block jobs
in the future.

You can still build libvirt HMP-only by disabling the yajl library
dependency.  The approach I've taken is to make the interfaces available
over both HMP and QMP (and so has the libvirt-side code).

In any case, we have defined both HMP and QMP.  Libvirt implements both
and I don't think there's a reason to provide only QMP.

Luiz: For future features, are we supposed to provide only QMP
interfaces, not HMP?

Stefan

Re: [Qemu-devel] [PATCH v3 2/9] block: add BlockJob interface for long-running operations

[Stefan Hajnoczi]

On Wed, Dec 14, 2011 at 02:51:47PM +0100, Kevin Wolf wrote:
> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > ---
> >  block_int.h |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 83 insertions(+), 0 deletions(-)
> 
> Why are these functions static inline in the header instead of being
> implemented in block.c?

You are right, they should be moved.

> The other thing I was going to ask, but don't really know to which patch
> I should reply, is whether we need to take care of bdrv_close/delete
> while a block job is still running. What happens if you unplug a device
> that is being streamed?

Hotplug is protected by the bdrv_set_in_use() we issue during block job
creation/completion.

The 'change', 'resize', and 'snapshot_blkdev' operations are not
protected AFAICT.  I'll take a close look at them and send patches.

Stefan

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Kevin Wolf]

Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
> On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
>> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
>>> diff --git a/hmp.c b/hmp.c
>>> index 66d9d0f..c16d6a1 100644
>>> --- a/hmp.c
>>> +++ b/hmp.c
>>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
>>>      qapi_free_PciInfoList(info);
>>>  }
>>>  
>>> +void hmp_info_block_jobs(Monitor *mon)
>>> +{
>>> +    BlockJobInfoList *list;
>>> +    Error *err = NULL;
>>> +
>>> +    list = qmp_query_block_jobs(&err);
>>> +    assert(!err);
>>> +
>>> +    if (!list) {
>>> +        monitor_printf(mon, "No active jobs\n");
>>> +        return;
>>> +    }
>>> +
>>> +    while (list) {
>>> +        /* The HMP output for streaming jobs is special because historically it
>>> +         * was different from other job types so applications may depend on the
>>> +         * exact string.
>>> +         */
>>
>> Er, what? This is new code. What HMP clients use this string? I know
>> that libvirt already got support for this before we implemented it, but
>> shouldn't that be QMP only?
> 
> Libvirt HMP uses this particular string, which turned out to be
> sub-optimal once I realized we might support other types of block jobs
> in the future.
> 
> You can still build libvirt HMP-only by disabling the yajl library
> dependency.  The approach I've taken is to make the interfaces available
> over both HMP and QMP (and so has the libvirt-side code).
> 
> In any case, we have defined both HMP and QMP.  Libvirt implements both
> and I don't think there's a reason to provide only QMP.
> 
> Luiz: For future features, are we supposed to provide only QMP
> interfaces, not HMP?

Of course, qemu should provide them as HMP command. But libvirt
shouldn't use HMP commands. HMP is intended for human users, not as an
API for management.

And I was pretty sure that we all agreed that HMP should be considered
unstable after a transition period. For new commands there's certainly
no reason to have a transition period, so I would consider them unstable
from the very beginning. After all, there are no qemu versions that
support the feature in question, but don't support QMP.

Kevin

Re: [Qemu-devel] [PATCH v3 2/9] block: add BlockJob interface for long-running operations

[Marcelo Tosatti]

On Wed, Dec 14, 2011 at 02:51:47PM +0100, Kevin Wolf wrote:
> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > ---
> >  block_int.h |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 83 insertions(+), 0 deletions(-)
> 
> Why are these functions static inline in the header instead of being
> implemented in block.c?
> 
> The other thing I was going to ask, but don't really know to which patch
> I should reply, is whether we need to take care of bdrv_close/delete
> while a block job is still running. What happens if you unplug a device
> that is being streamed?

There is an additional reference, its not deleted until the operation
completes. Or at least it should be like that, Stefan?

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Luiz Capitulino]

On Thu, 15 Dec 2011 11:34:07 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
> > On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
> >> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> >>> diff --git a/hmp.c b/hmp.c
> >>> index 66d9d0f..c16d6a1 100644
> >>> --- a/hmp.c
> >>> +++ b/hmp.c
> >>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
> >>>      qapi_free_PciInfoList(info);
> >>>  }
> >>>  
> >>> +void hmp_info_block_jobs(Monitor *mon)
> >>> +{
> >>> +    BlockJobInfoList *list;
> >>> +    Error *err = NULL;
> >>> +
> >>> +    list = qmp_query_block_jobs(&err);
> >>> +    assert(!err);
> >>> +
> >>> +    if (!list) {
> >>> +        monitor_printf(mon, "No active jobs\n");
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    while (list) {
> >>> +        /* The HMP output for streaming jobs is special because historically it
> >>> +         * was different from other job types so applications may depend on the
> >>> +         * exact string.
> >>> +         */
> >>
> >> Er, what? This is new code. What HMP clients use this string? I know
> >> that libvirt already got support for this before we implemented it, but
> >> shouldn't that be QMP only?
> > 
> > Libvirt HMP uses this particular string, which turned out to be
> > sub-optimal once I realized we might support other types of block jobs
> > in the future.
> > 
> > You can still build libvirt HMP-only by disabling the yajl library
> > dependency.  The approach I've taken is to make the interfaces available
> > over both HMP and QMP (and so has the libvirt-side code).
> > 
> > In any case, we have defined both HMP and QMP.  Libvirt implements both
> > and I don't think there's a reason to provide only QMP.
> > 
> > Luiz: For future features, are we supposed to provide only QMP
> > interfaces, not HMP?
> 
> Of course, qemu should provide them as HMP command. But libvirt
> shouldn't use HMP commands. HMP is intended for human users, not as an
> API for management.

That's correct.

What defines if you're going to have a HMP version of a command is if
you expect it to be used by humans and if you do all its output and
arguments should be user friendly. You should never expect nor assume
it's going to be used by a management interface.

> 
> And I was pretty sure that we all agreed that HMP should be considered
> unstable after a transition period. For new commands there's certainly
> no reason to have a transition period, so I would consider them unstable
> from the very beginning. After all, there are no qemu versions that
> support the feature in question, but don't support QMP.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Stefan Hajnoczi]

On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
<lcapitulino@redhat.com> wrote:
> On Thu, 15 Dec 2011 11:34:07 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
>
>> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
>> > On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
>> >> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
>> >>> diff --git a/hmp.c b/hmp.c
>> >>> index 66d9d0f..c16d6a1 100644
>> >>> --- a/hmp.c
>> >>> +++ b/hmp.c
>> >>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
>> >>>      qapi_free_PciInfoList(info);
>> >>>  }
>> >>>
>> >>> +void hmp_info_block_jobs(Monitor *mon)
>> >>> +{
>> >>> +    BlockJobInfoList *list;
>> >>> +    Error *err = NULL;
>> >>> +
>> >>> +    list = qmp_query_block_jobs(&err);
>> >>> +    assert(!err);
>> >>> +
>> >>> +    if (!list) {
>> >>> +        monitor_printf(mon, "No active jobs\n");
>> >>> +        return;
>> >>> +    }
>> >>> +
>> >>> +    while (list) {
>> >>> +        /* The HMP output for streaming jobs is special because historically it
>> >>> +         * was different from other job types so applications may depend on the
>> >>> +         * exact string.
>> >>> +         */
>> >>
>> >> Er, what? This is new code. What HMP clients use this string? I know
>> >> that libvirt already got support for this before we implemented it, but
>> >> shouldn't that be QMP only?
>> >
>> > Libvirt HMP uses this particular string, which turned out to be
>> > sub-optimal once I realized we might support other types of block jobs
>> > in the future.
>> >
>> > You can still build libvirt HMP-only by disabling the yajl library
>> > dependency.  The approach I've taken is to make the interfaces available
>> > over both HMP and QMP (and so has the libvirt-side code).
>> >
>> > In any case, we have defined both HMP and QMP.  Libvirt implements both
>> > and I don't think there's a reason to provide only QMP.
>> >
>> > Luiz: For future features, are we supposed to provide only QMP
>> > interfaces, not HMP?
>>
>> Of course, qemu should provide them as HMP command. But libvirt
>> shouldn't use HMP commands. HMP is intended for human users, not as an
>> API for management.
>
> That's correct.
>
> What defines if you're going to have a HMP version of a command is if
> you expect it to be used by humans and if you do all its output and
> arguments should be user friendly. You should never expect nor assume
> it's going to be used by a management interface.

Okay, thanks Kevin and Luiz for explaining.  In this case I know it is
used by a management interface because libvirt has code to use it.

I was my mistake to include the HMP side as part of the "API" but it's
here now and I think we can live with this.

Stefan

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Luiz Capitulino]

On Thu, 15 Dec 2011 12:00:16 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> <lcapitulino@redhat.com> wrote:
> > On Thu, 15 Dec 2011 11:34:07 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> >
> >> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
> >> > On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
> >> >> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> >> >>> diff --git a/hmp.c b/hmp.c
> >> >>> index 66d9d0f..c16d6a1 100644
> >> >>> --- a/hmp.c
> >> >>> +++ b/hmp.c
> >> >>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
> >> >>>      qapi_free_PciInfoList(info);
> >> >>>  }
> >> >>>
> >> >>> +void hmp_info_block_jobs(Monitor *mon)
> >> >>> +{
> >> >>> +    BlockJobInfoList *list;
> >> >>> +    Error *err = NULL;
> >> >>> +
> >> >>> +    list = qmp_query_block_jobs(&err);
> >> >>> +    assert(!err);
> >> >>> +
> >> >>> +    if (!list) {
> >> >>> +        monitor_printf(mon, "No active jobs\n");
> >> >>> +        return;
> >> >>> +    }
> >> >>> +
> >> >>> +    while (list) {
> >> >>> +        /* The HMP output for streaming jobs is special because historically it
> >> >>> +         * was different from other job types so applications may depend on the
> >> >>> +         * exact string.
> >> >>> +         */
> >> >>
> >> >> Er, what? This is new code. What HMP clients use this string? I know
> >> >> that libvirt already got support for this before we implemented it, but
> >> >> shouldn't that be QMP only?
> >> >
> >> > Libvirt HMP uses this particular string, which turned out to be
> >> > sub-optimal once I realized we might support other types of block jobs
> >> > in the future.
> >> >
> >> > You can still build libvirt HMP-only by disabling the yajl library
> >> > dependency.  The approach I've taken is to make the interfaces available
> >> > over both HMP and QMP (and so has the libvirt-side code).
> >> >
> >> > In any case, we have defined both HMP and QMP.  Libvirt implements both
> >> > and I don't think there's a reason to provide only QMP.
> >> >
> >> > Luiz: For future features, are we supposed to provide only QMP
> >> > interfaces, not HMP?
> >>
> >> Of course, qemu should provide them as HMP command. But libvirt
> >> shouldn't use HMP commands. HMP is intended for human users, not as an
> >> API for management.
> >
> > That's correct.
> >
> > What defines if you're going to have a HMP version of a command is if
> > you expect it to be used by humans and if you do all its output and
> > arguments should be user friendly. You should never expect nor assume
> > it's going to be used by a management interface.
> 
> Okay, thanks Kevin and Luiz for explaining.  In this case I know it is
> used by a management interface because libvirt has code to use it.
> 
> I was my mistake to include the HMP side as part of the "API" but it's
> here now and I think we can live with this.

I need to fully review this series to ack or nack this, but unfortunately I've
had a busy week and will be on vacations for two weeks starting in a few hours.

I think it would be needed to have at least Kevin and Anthony or me acking this.

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Kevin Wolf]

Am 15.12.2011 13:00, schrieb Stefan Hajnoczi:
> On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> <lcapitulino@redhat.com> wrote:
>> On Thu, 15 Dec 2011 11:34:07 +0100
>> Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
>>>> On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
>>>>> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
>>>>>> diff --git a/hmp.c b/hmp.c
>>>>>> index 66d9d0f..c16d6a1 100644
>>>>>> --- a/hmp.c
>>>>>> +++ b/hmp.c
>>>>>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
>>>>>>      qapi_free_PciInfoList(info);
>>>>>>  }
>>>>>>
>>>>>> +void hmp_info_block_jobs(Monitor *mon)
>>>>>> +{
>>>>>> +    BlockJobInfoList *list;
>>>>>> +    Error *err = NULL;
>>>>>> +
>>>>>> +    list = qmp_query_block_jobs(&err);
>>>>>> +    assert(!err);
>>>>>> +
>>>>>> +    if (!list) {
>>>>>> +        monitor_printf(mon, "No active jobs\n");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    while (list) {
>>>>>> +        /* The HMP output for streaming jobs is special because historically it
>>>>>> +         * was different from other job types so applications may depend on the
>>>>>> +         * exact string.
>>>>>> +         */
>>>>>
>>>>> Er, what? This is new code. What HMP clients use this string? I know
>>>>> that libvirt already got support for this before we implemented it, but
>>>>> shouldn't that be QMP only?
>>>>
>>>> Libvirt HMP uses this particular string, which turned out to be
>>>> sub-optimal once I realized we might support other types of block jobs
>>>> in the future.
>>>>
>>>> You can still build libvirt HMP-only by disabling the yajl library
>>>> dependency.  The approach I've taken is to make the interfaces available
>>>> over both HMP and QMP (and so has the libvirt-side code).
>>>>
>>>> In any case, we have defined both HMP and QMP.  Libvirt implements both
>>>> and I don't think there's a reason to provide only QMP.
>>>>
>>>> Luiz: For future features, are we supposed to provide only QMP
>>>> interfaces, not HMP?
>>>
>>> Of course, qemu should provide them as HMP command. But libvirt
>>> shouldn't use HMP commands. HMP is intended for human users, not as an
>>> API for management.
>>
>> That's correct.
>>
>> What defines if you're going to have a HMP version of a command is if
>> you expect it to be used by humans and if you do all its output and
>> arguments should be user friendly. You should never expect nor assume
>> it's going to be used by a management interface.
> 
> Okay, thanks Kevin and Luiz for explaining.  In this case I know it is
> used by a management interface because libvirt has code to use it.
> 
> I was my mistake to include the HMP side as part of the "API" but it's
> here now and I think we can live with this.

We probably can, but I would prefer fixing it in libvirt. Possibly the
right fix there would be to remove it entirely from the HMP code - if I
understand right, the HMP code is only meant to support older qemu
versions anyway.

I also don't quite understand why there even is an option to disable QMP
in libvirt, we have always told that HMP will become unstable.

Kevin

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Stefan Hajnoczi]

On Thu, Dec 15, 2011 at 01:37:51PM +0100, Kevin Wolf wrote:
> Am 15.12.2011 13:00, schrieb Stefan Hajnoczi:
> > On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> > <lcapitulino@redhat.com> wrote:
> >> On Thu, 15 Dec 2011 11:34:07 +0100
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
> >>>> On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
> >>>>> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> >>>>>> diff --git a/hmp.c b/hmp.c
> >>>>>> index 66d9d0f..c16d6a1 100644
> >>>>>> --- a/hmp.c
> >>>>>> +++ b/hmp.c
> >>>>>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
> >>>>>>      qapi_free_PciInfoList(info);
> >>>>>>  }
> >>>>>>
> >>>>>> +void hmp_info_block_jobs(Monitor *mon)
> >>>>>> +{
> >>>>>> +    BlockJobInfoList *list;
> >>>>>> +    Error *err = NULL;
> >>>>>> +
> >>>>>> +    list = qmp_query_block_jobs(&err);
> >>>>>> +    assert(!err);
> >>>>>> +
> >>>>>> +    if (!list) {
> >>>>>> +        monitor_printf(mon, "No active jobs\n");
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    while (list) {
> >>>>>> +        /* The HMP output for streaming jobs is special because historically it
> >>>>>> +         * was different from other job types so applications may depend on the
> >>>>>> +         * exact string.
> >>>>>> +         */
> >>>>>
> >>>>> Er, what? This is new code. What HMP clients use this string? I know
> >>>>> that libvirt already got support for this before we implemented it, but
> >>>>> shouldn't that be QMP only?
> >>>>
> >>>> Libvirt HMP uses this particular string, which turned out to be
> >>>> sub-optimal once I realized we might support other types of block jobs
> >>>> in the future.
> >>>>
> >>>> You can still build libvirt HMP-only by disabling the yajl library
> >>>> dependency.  The approach I've taken is to make the interfaces available
> >>>> over both HMP and QMP (and so has the libvirt-side code).
> >>>>
> >>>> In any case, we have defined both HMP and QMP.  Libvirt implements both
> >>>> and I don't think there's a reason to provide only QMP.
> >>>>
> >>>> Luiz: For future features, are we supposed to provide only QMP
> >>>> interfaces, not HMP?
> >>>
> >>> Of course, qemu should provide them as HMP command. But libvirt
> >>> shouldn't use HMP commands. HMP is intended for human users, not as an
> >>> API for management.
> >>
> >> That's correct.
> >>
> >> What defines if you're going to have a HMP version of a command is if
> >> you expect it to be used by humans and if you do all its output and
> >> arguments should be user friendly. You should never expect nor assume
> >> it's going to be used by a management interface.
> > 
> > Okay, thanks Kevin and Luiz for explaining.  In this case I know it is
> > used by a management interface because libvirt has code to use it.
> > 
> > I was my mistake to include the HMP side as part of the "API" but it's
> > here now and I think we can live with this.
> 
> We probably can, but I would prefer fixing it in libvirt. Possibly the
> right fix there would be to remove it entirely from the HMP code - if I
> understand right, the HMP code is only meant to support older qemu
> versions anyway.
> 
> I also don't quite understand why there even is an option to disable QMP
> in libvirt, we have always told that HMP will become unstable.

Yeah, that's a good discussion to have.  We need to get everyone on the
same page.  I am starting a new thread where we can discuss this with
libvir-list and qemu-devel.

Stefan

Re: [Qemu-devel] [PATCH v3 2/9] block: add BlockJob interface for long-running operations

[Stefan Hajnoczi]

On Thu, Dec 15, 2011 at 08:40:08AM -0200, Marcelo Tosatti wrote:
> On Wed, Dec 14, 2011 at 02:51:47PM +0100, Kevin Wolf wrote:
> > Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > > ---
> > >  block_int.h |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 83 insertions(+), 0 deletions(-)
> > 
> > Why are these functions static inline in the header instead of being
> > implemented in block.c?
> > 
> > The other thing I was going to ask, but don't really know to which patch
> > I should reply, is whether we need to take care of bdrv_close/delete
> > while a block job is still running. What happens if you unplug a device
> > that is being streamed?
> 
> There is an additional reference, its not deleted until the operation
> completes. Or at least it should be like that, Stefan?

Right, the patch uses bdrv_set_in_use().  In my other reply to this
sub-thread I mentioned that certain operations like 'change', 'resize',
and 'snapshot_blkdev' do not seem to check bdrv_in_use() yet.

So I think in order to make image streaming safe additional checks will
need to be added in blockdev.c.

Stefan

Re: [Qemu-devel] [PATCH v3 5/9] qmp: add block_stream command

[Luiz Capitulino]

On Tue, 13 Dec 2011 13:52:27 +0000
Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:

> Add the block_stream command, which starts copy backing file contents
> into the image file.  Later patches add control over the background copy
> speed, cancelation, and querying running streaming operations.

Please also mention that you're adding an event, otherwise it comes as a
surprise to the reviewer.

When we talked about this implementation (ie. having events, cancellation etc)
I thought you were going to do something very specific to the streaming api,
like migration. However, you ended up adding async QMP support to the block
layer.

I have the impression this could be generalized a bit more and be moved to
QMP instead (and I strongly feel that's what we should do).

There's only one problem: we are waiting for the new QMP server to work on
that. I hope to have it integrated by the end of this month, but it might
be possible to add async support to the current server if waiting is not
an option.

> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
>  blockdev.c       |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |   13 ++++++++++
>  hmp.c            |   14 +++++++++++
>  hmp.h            |    1 +
>  monitor.c        |    3 ++
>  monitor.h        |    1 +
>  qapi-schema.json |   53 ++++++++++++++++++++++++++++++++++++++++++
>  qerror.c         |    4 +++
>  qerror.h         |    3 ++
>  qmp-commands.hx  |   18 ++++++++++++++
>  trace-events     |    4 +++
>  11 files changed, 182 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index dbf0251..08355cf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -13,8 +13,11 @@
>  #include "qerror.h"
>  #include "qemu-option.h"
>  #include "qemu-config.h"
> +#include "qemu-objects.h"
>  #include "sysemu.h"
>  #include "block_int.h"
> +#include "qmp-commands.h"
> +#include "trace.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> @@ -887,3 +890,68 @@ int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  
>      return 0;
>  }
> +
> +static QObject *qobject_from_block_job(BlockJob *job)
> +{
> +    return qobject_from_jsonf("{ 'type': %s,"
> +                              "'device': %s,"
> +                              "'len': %" PRId64 ","
> +                              "'offset': %" PRId64 ","
> +                              "'speed': %" PRId64 " }",
> +                              job->job_type->job_type,
> +                              bdrv_get_device_name(job->bs),
> +                              job->len,
> +                              job->offset,
> +                              job->speed);
> +}
> +
> +static void block_stream_cb(void *opaque, int ret)
> +{
> +    BlockDriverState *bs = opaque;
> +    QObject *obj;
> +
> +    trace_block_stream_cb(bs, bs->job, ret);
> +
> +    assert(bs->job);
> +    obj = qobject_from_block_job(bs->job);
> +    if (ret < 0) {
> +        QDict *dict = qobject_to_qdict(obj);
> +        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
> +    }
> +
> +    monitor_protocol_event(QEVENT_BLOCK_JOB_COMPLETED, obj);
> +    qobject_decref(obj);
> +}
> +
> +void qmp_block_stream(const char *device, bool has_base,
> +                      const char *base, Error **errp)
> +{
> +    BlockDriverState *bs;
> +    int ret;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    /* Base device not supported */
> +    if (base) {
> +        error_set(errp, QERR_NOT_SUPPORTED);
> +        return;
> +    }

Is this a future feature? In this case I'd rather drop the argument for
now and add it later. The only detail is that we haven't reached consensus
if it will be required to add a new command for that or if we'll be able
to just extend existing commands.

> +
> +    ret = stream_start(bs, NULL, block_stream_cb, bs);
> +    if (ret < 0) {
> +        switch (ret) {
> +        case -EBUSY:
> +            error_set(errp, QERR_DEVICE_IN_USE, device);
> +            return;
> +        default:
> +            error_set(errp, QERR_NOT_SUPPORTED);
> +            return;
> +        }
> +    }
> +
> +    trace_qmp_block_stream(bs, bs->job);
> +}
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 79a9195..c0de41c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -70,6 +70,19 @@ but should be used with extreme caution.  Note that this command only
>  resizes image files, it can not resize block devices like LVM volumes.
>  ETEXI
>  
> +    {
> +        .name       = "block_stream",
> +        .args_type  = "device:B,base:s?",
> +        .params     = "device [base]",
> +        .help       = "copy data from a backing file into a block device",
> +        .mhandler.cmd = hmp_block_stream,
> +    },
> +
> +STEXI
> +@item block_stream
> +@findex block_stream
> +Copy data from a backing file into a block device.
> +ETEXI
>  
>      {
>          .name       = "eject",
> diff --git a/hmp.c b/hmp.c
> index dfab7ad..8b7d7d4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -531,3 +531,17 @@ void hmp_cpu(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "invalid CPU index\n");
>      }
>  }
> +
> +void hmp_block_stream(Monitor *mon, const QDict *qdict)
> +{
> +    Error *error = NULL;
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *base = qdict_get_try_str(qdict, "base");
> +
> +    qmp_block_stream(device, base != NULL, base, &error);
> +
> +    if (error) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(error));
> +        error_free(error);
> +    }

There's a hmp_handle_error() helper recently merged that does exactly that.

> +}
> diff --git a/hmp.h b/hmp.h
> index 4422578..3cb6fe5 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,5 +37,6 @@ void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
>  void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
> +void hmp_block_stream(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/monitor.c b/monitor.c
> index 1be222e..a33218c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>          case QEVENT_SPICE_DISCONNECTED:
>              event_name = "SPICE_DISCONNECTED";
>              break;
> +        case QEVENT_BLOCK_JOB_COMPLETED:
> +            event_name = "BLOCK_JOB_COMPLETED";
> +            break;
>          default:
>              abort();
>              break;
> diff --git a/monitor.h b/monitor.h
> index e76795f..d9b07dc 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -35,6 +35,7 @@ typedef enum MonitorEvent {
>      QEVENT_SPICE_CONNECTED,
>      QEVENT_SPICE_INITIALIZED,
>      QEVENT_SPICE_DISCONNECTED,
> +    QEVENT_BLOCK_JOB_COMPLETED,
>      QEVENT_MAX,
>  } MonitorEvent;
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fbbdbe0..65308d2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -901,3 +901,56 @@
>  # Notes: Do not use this command.
>  ##
>  { 'command': 'cpu', 'data': {'index': 'int'} }
> +
> +##
> +# @block_stream:

Command names should be verbs and please use an hyphen.

> +#
> +# Copy data from a backing file into a block device.
> +#
> +# The block streaming operation is performed in the background until the entire
> +# backing file has been copied.  This command returns immediately once streaming
> +# has started.  The status of ongoing block streaming operations can be checked
> +# with query-block-jobs.  The operation can be stopped before it has completed
> +# using the block_job_cancel command.
> +#
> +# If a base file is specified then sectors are not copied from that base file and
> +# its backing chain.  When streaming completes the image file will have the base
> +# file as its backing file.  This can be used to stream a subset of the backing
> +# file chain instead of flattening the entire image.
> +#
> +# On successful completion the image file is updated to drop the backing file.

Nice doc.

> +#
> +# Arguments:
> +#
> +# @device: device name
> +# @base:   common backing file
> +#
> +# Errors:
> +#
> +# DeviceInUse:    streaming is already active on this device
> +# DeviceNotFound: device name is invalid
> +# NotSupported:   image streaming is not supported by this device

The convention used in this file to document errors is different.

> +#
> +# Events:
> +#
> +# On completion the BLOCK_JOB_COMPLETED event is raised with the following
> +# fields:
> +#
> +# - type:     job type ("stream" for image streaming, json-string)
> +# - device:   device name (json-string)
> +# - len:      maximum progress value (json-int)
> +# - offset:   current progress value (json-int)
> +# - speed:    rate limit, bytes per second (json-int)
> +# - error:    error message (json-string, only on error)
> +#
> +# The completion event is raised both on success and on failure.  On
> +# success offset is equal to len.  On failure offset and len can be
> +# used to indicate at which point the operation failed.
> +#
> +# On failure the error field contains a human-readable error message.  There are
> +# no semantics other than that streaming has failed and clients should not try
> +# to interpret the error string.

Events should be documented in QMP/qmp-events.txt.

> +#
> +# Since: 1.1
> +##
> +{ 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
> diff --git a/qerror.c b/qerror.c
> index 14a1d59..605381a 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -174,6 +174,10 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "No '%(bus)' bus found for device '%(device)'",
>      },
>      {
> +        .error_fmt = QERR_NOT_SUPPORTED,
> +        .desc      = "Not supported",

We have QERR_UNSUPPORTED already.

> +    },
> +    {
>          .error_fmt = QERR_OPEN_FILE_FAILED,
>          .desc      = "Could not open '%(filename)'",
>      },
> diff --git a/qerror.h b/qerror.h
> index 560d458..b5c0cfe 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -147,6 +147,9 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_NO_BUS_FOR_DEVICE \
>      "{ 'class': 'NoBusForDevice', 'data': { 'device': %s, 'bus': %s } }"
>  
> +#define QERR_NOT_SUPPORTED \
> +    "{ 'class': 'NotSupported', 'data': {} }"
> +
>  #define QERR_OPEN_FILE_FAILED \
>      "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
>  
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 94da2a8..8c1c934 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -683,6 +683,24 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "block_stream",
> +        .args_type  = "device:B,base:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_block_stream,
> +    },

You can drop the docs below. The above entry is needed because the current
QMP server still uses it (which causes duplication with the schema file,
this will be fixed soon).

> +
> +SQMP
> +block_stream
> +------------
> +See qapi-schema.json for documentation.
> +
> +Examples:
> +
> +-> { "execute": "block_stream", "arguments": { "device": "virtio0" } }
> +<- { "return":  {} }
> +
> +EQMP
> +
> +    {
>          .name       = "blockdev-snapshot-sync",
>          .args_type  = "device:B,snapshot-file:s?,format:s?",
>          .params     = "device [new-image-file] [format]",
> diff --git a/trace-events b/trace-events
> index 4efcd95..6c1eec2 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -73,6 +73,10 @@ bdrv_co_copy_on_readv(void *bs, int64_t sector_num, int nb_sectors, int64_t clus
>  stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
>  stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
>  
> +# blockdev.c
> +block_stream_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
> +qmp_block_stream(void *bs, void *job) "bs %p job %p"
> +
>  # hw/virtio-blk.c
>  virtio_blk_req_complete(void *req, int status) "req %p status %d"
>  virtio_blk_rw_complete(void *req, int ret) "req %p ret %d"

Re: [Qemu-devel] [PATCH v3 5/9] qmp: add block_stream command

[Luiz Capitulino]

By the way, most of my review comments applies to the other commands being
added in this series:

- Use verbs and separate word with hyphens
- Follow the documentation syntax (see other commands for examples)
- Use the hmp_handle_error() helper
- Drop any SQMP/EQMP documentation

Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs

[Luiz Capitulino]

On Thu, 15 Dec 2011 12:00:16 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino
> <lcapitulino@redhat.com> wrote:
> > On Thu, 15 Dec 2011 11:34:07 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> >
> >> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi:
> >> > On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote:
> >> >> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi:
> >> >>> diff --git a/hmp.c b/hmp.c
> >> >>> index 66d9d0f..c16d6a1 100644
> >> >>> --- a/hmp.c
> >> >>> +++ b/hmp.c
> >> >>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon)
> >> >>>      qapi_free_PciInfoList(info);
> >> >>>  }
> >> >>>
> >> >>> +void hmp_info_block_jobs(Monitor *mon)
> >> >>> +{
> >> >>> +    BlockJobInfoList *list;
> >> >>> +    Error *err = NULL;
> >> >>> +
> >> >>> +    list = qmp_query_block_jobs(&err);
> >> >>> +    assert(!err);
> >> >>> +
> >> >>> +    if (!list) {
> >> >>> +        monitor_printf(mon, "No active jobs\n");
> >> >>> +        return;
> >> >>> +    }
> >> >>> +
> >> >>> +    while (list) {
> >> >>> +        /* The HMP output for streaming jobs is special because historically it
> >> >>> +         * was different from other job types so applications may depend on the
> >> >>> +         * exact string.
> >> >>> +         */
> >> >>
> >> >> Er, what? This is new code. What HMP clients use this string? I know
> >> >> that libvirt already got support for this before we implemented it, but
> >> >> shouldn't that be QMP only?
> >> >
> >> > Libvirt HMP uses this particular string, which turned out to be
> >> > sub-optimal once I realized we might support other types of block jobs
> >> > in the future.
> >> >
> >> > You can still build libvirt HMP-only by disabling the yajl library
> >> > dependency.  The approach I've taken is to make the interfaces available
> >> > over both HMP and QMP (and so has the libvirt-side code).
> >> >
> >> > In any case, we have defined both HMP and QMP.  Libvirt implements both
> >> > and I don't think there's a reason to provide only QMP.
> >> >
> >> > Luiz: For future features, are we supposed to provide only QMP
> >> > interfaces, not HMP?
> >>
> >> Of course, qemu should provide them as HMP command. But libvirt
> >> shouldn't use HMP commands. HMP is intended for human users, not as an
> >> API for management.
> >
> > That's correct.
> >
> > What defines if you're going to have a HMP version of a command is if
> > you expect it to be used by humans and if you do all its output and
> > arguments should be user friendly. You should never expect nor assume
> > it's going to be used by a management interface.
> 
> Okay, thanks Kevin and Luiz for explaining.  In this case I know it is
> used by a management interface because libvirt has code to use it.
> 
> I was my mistake to include the HMP side as part of the "API" but it's
> here now and I think we can live with this.

I'll have to nack this. Having stable guarantees with something that's
not in the tree is just impossible, not even mentioning it's HMP.

Having said that, the HMP string can be whatever you want. I will just ask
you to drop the comment and won't refuse patches that change/improve it.

Re: [Qemu-devel] [PATCH v3 5/9] qmp: add block_stream command

[Stefan Hajnoczi]

On Wed, Jan 4, 2012 at 12:59 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Tue, 13 Dec 2011 13:52:27 +0000
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
>
>> Add the block_stream command, which starts copy backing file contents
>> into the image file.  Later patches add control over the background copy
>> speed, cancelation, and querying running streaming operations.
>
> Please also mention that you're adding an event, otherwise it comes as a
> surprise to the reviewer.

Ok.

> When we talked about this implementation (ie. having events, cancellation etc)
> I thought you were going to do something very specific to the streaming api,
> like migration. However, you ended up adding async QMP support to the block
> layer.
>
> I have the impression this could be generalized a bit more and be moved to
> QMP instead (and I strongly feel that's what we should do).
>
> There's only one problem: we are waiting for the new QMP server to work on
> that. I hope to have it integrated by the end of this month, but it might
> be possible to add async support to the current server if waiting is not
> an option.

I'm not sure what you'd like here, could you be more specific?  I have
introduced the concept of a block job - a long-running operation on
block devices - and the completion event when the job finishes.  I
guess you're thinking of a generic QMP job concept with a completion
event?  Or something else?

What I'd like to avoid at this stage is changing the QMP API as seen
by clients because we already have at least one client which relies on
this API.  I know that sucks and that this is my fault because I've
been dragging out this patch series for too long.  But in a situation
like this I think it's better to live with an API that doesn't meet
the current philosophy on nice APIs but works flawlessly with both new
and existing clients.  But it depends on the specifics, what changes
do you suggest?

>> +    /* Base device not supported */
>> +    if (base) {
>> +        error_set(errp, QERR_NOT_SUPPORTED);
>> +        return;
>> +    }
>
> Is this a future feature? In this case I'd rather drop the argument for
> now and add it later. The only detail is that we haven't reached consensus
> if it will be required to add a new command for that or if we'll be able
> to just extend existing commands.

Marcelo sent out patches for this features.  I think this series and
Marcelo's series can/will be merged one after another so that this
resolves itself without us needing to do anything.

http://www.mail-archive.com/qemu-devel@nongnu.org/msg91742.html

>> +    qmp_block_stream(device, base != NULL, base, &error);
>> +
>> +    if (error) {
>> +        monitor_printf(mon, "%s\n", error_get_pretty(error));
>> +        error_free(error);
>> +    }
>
> There's a hmp_handle_error() helper recently merged that does exactly that.

Ok.

>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index fbbdbe0..65308d2 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -901,3 +901,56 @@
>>  # Notes: Do not use this command.
>>  ##
>>  { 'command': 'cpu', 'data': {'index': 'int'} }
>> +
>> +##
>> +# @block_stream:
>
> Command names should be verbs and please use an hyphen.

These commands have been in the Image Streaming API spec from the beginning:

http://wiki.qemu.org/Features/LiveBlockMigration/ImageStreamingAPI

We cannot prettify the API at this stage.  You, Anthony, and I
discussed QAPI command naming on IRC maybe two months ago and this
seemed to be the way to do it.  These commands fit the existing
block_* commands and are already in use by libvirt - if we change
names now we break libvirt.

>> +# Errors:
>> +#
>> +# DeviceInUse:    streaming is already active on this device
>> +# DeviceNotFound: device name is invalid
>> +# NotSupported:   image streaming is not supported by this device
>
> The convention used in this file to document errors is different.

Ok.

>> +#
>> +# Events:
>> +#
>> +# On completion the BLOCK_JOB_COMPLETED event is raised with the following
>> +# fields:
>> +#
>> +# - type:     job type ("stream" for image streaming, json-string)
>> +# - device:   device name (json-string)
>> +# - len:      maximum progress value (json-int)
>> +# - offset:   current progress value (json-int)
>> +# - speed:    rate limit, bytes per second (json-int)
>> +# - error:    error message (json-string, only on error)
>> +#
>> +# The completion event is raised both on success and on failure.  On
>> +# success offset is equal to len.  On failure offset and len can be
>> +# used to indicate at which point the operation failed.
>> +#
>> +# On failure the error field contains a human-readable error message.  There are
>> +# no semantics other than that streaming has failed and clients should not try
>> +# to interpret the error string.
>
> Events should be documented in QMP/qmp-events.txt.

Ok.

>> +#
>> +# Since: 1.1
>> +##
>> +{ 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
>> diff --git a/qerror.c b/qerror.c
>> index 14a1d59..605381a 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -174,6 +174,10 @@ static const QErrorStringTable qerror_table[] = {
>>          .desc      = "No '%(bus)' bus found for device '%(device)'",
>>      },
>>      {
>> +        .error_fmt = QERR_NOT_SUPPORTED,
>> +        .desc      = "Not supported",
>
> We have QERR_UNSUPPORTED already.

I know.  We're stuck in a hard place here again because NotSupported
has been in the Image Streaming API spec and hence implemented in
libvirt for a while now.  If we change this then an old client which
only understands NotSupported will not know what to do with the
Unsupported error.

(Unsupported was not in QEMU when the Image Streaming API was defined.)

>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 94da2a8..8c1c934 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -683,6 +683,24 @@ Example:
>>  EQMP
>>
>>      {
>> +        .name       = "block_stream",
>> +        .args_type  = "device:B,base:s?",
>> +        .mhandler.cmd_new = qmp_marshal_input_block_stream,
>> +    },
>
> You can drop the docs below. The above entry is needed because the current
> QMP server still uses it (which causes duplication with the schema file,
> this will be fixed soon).

Ok.

[Qemu-devel] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Thu, 5 Jan 2012 13:48:43 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Jan 4, 2012 at 12:59 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Tue, 13 Dec 2011 13:52:27 +0000
> > Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote:
> >
> >> Add the block_stream command, which starts copy backing file contents
> >> into the image file.  Later patches add control over the background copy
> >> speed, cancelation, and querying running streaming operations.
> >
> > Please also mention that you're adding an event, otherwise it comes as a
> > surprise to the reviewer.
> 
> Ok.
> 
> > When we talked about this implementation (ie. having events, cancellation etc)
> > I thought you were going to do something very specific to the streaming api,
> > like migration. However, you ended up adding async QMP support to the block
> > layer.
> >
> > I have the impression this could be generalized a bit more and be moved to
> > QMP instead (and I strongly feel that's what we should do).
> >
> > There's only one problem: we are waiting for the new QMP server to work on
> > that. I hope to have it integrated by the end of this month, but it might
> > be possible to add async support to the current server if waiting is not
> > an option.
> 
> I'm not sure what you'd like here, could you be more specific?  I have
> introduced the concept of a block job - a long-running operation on
> block devices - and the completion event when the job finishes.  I
> guess you're thinking of a generic QMP job concept with a completion
> event?

Exactly. We should have QMP_JOB_COMPLETED instead of BLOCK_JOB_COMPLETED,
for example.

>  Or something else?
> 
> What I'd like to avoid at this stage is changing the QMP API as seen
> by clients because we already have at least one client which relies on
> this API.  I know that sucks and that this is my fault because I've
> been dragging out this patch series for too long.  But in a situation
> like this I think it's better to live with an API that doesn't meet
> the current philosophy on nice APIs but works flawlessly with both new
> and existing clients.  But it depends on the specifics, what changes
> do you suggest?

[...]

> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index fbbdbe0..65308d2 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -901,3 +901,56 @@
> >>  # Notes: Do not use this command.
> >>  ##
> >>  { 'command': 'cpu', 'data': {'index': 'int'} }
> >> +
> >> +##
> >> +# @block_stream:
> >
> > Command names should be verbs and please use an hyphen.
> 
> These commands have been in the Image Streaming API spec from the beginning:
> 
> http://wiki.qemu.org/Features/LiveBlockMigration/ImageStreamingAPI
> 
> We cannot prettify the API at this stage.  You, Anthony, and I
> discussed QAPI command naming on IRC maybe two months ago and this
> seemed to be the way to do it.  These commands fit the existing
> block_* commands and are already in use by libvirt - if we change
> names now we break libvirt.

[...]

> >> +#
> >> +# Since: 1.1
> >> +##
> >> +{ 'command': 'block_stream', 'data': { 'device': 'str', '*base': 'str' } }
> >> diff --git a/qerror.c b/qerror.c
> >> index 14a1d59..605381a 100644
> >> --- a/qerror.c
> >> +++ b/qerror.c
> >> @@ -174,6 +174,10 @@ static const QErrorStringTable qerror_table[] = {
> >>          .desc      = "No '%(bus)' bus found for device '%(device)'",
> >>      },
> >>      {
> >> +        .error_fmt = QERR_NOT_SUPPORTED,
> >> +        .desc      = "Not supported",
> >
> > We have QERR_UNSUPPORTED already.
> 
> I know.  We're stuck in a hard place here again because NotSupported
> has been in the Image Streaming API spec and hence implemented in
> libvirt for a while now.  If we change this then an old client which
> only understands NotSupported will not know what to do with the
> Unsupported error.
> 
> (Unsupported was not in QEMU when the Image Streaming API was defined.)

Let me try to understand it: is libvirt relying on an off tree API and
we are now required to have stable guarantees to off tree APIs?

I can't even express how insane this looks to me, at least not without
being too harsh.

Re: [Qemu-devel] [PATCH v3 5/9] qmp: add block_stream command

[Stefan Hajnoczi]

On Thu, Jan 5, 2012 at 1:48 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> +    /* Base device not supported */
>>> +    if (base) {
>>> +        error_set(errp, QERR_NOT_SUPPORTED);
>>> +        return;
>>> +    }
>>
>> Is this a future feature? In this case I'd rather drop the argument for
>> now and add it later. The only detail is that we haven't reached consensus
>> if it will be required to add a new command for that or if we'll be able
>> to just extend existing commands.
>
> Marcelo sent out patches for this features.  I think this series and
> Marcelo's series can/will be merged one after another so that this
> resolves itself without us needing to do anything.
>
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg91742.html

Just talked to Marcelo.  We'll merge his series into this one so this
issue will be gone completely in v4.

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Eric Blake]

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

On 01/05/2012 07:16 AM, Luiz Capitulino wrote:
>> I know.  We're stuck in a hard place here again because NotSupported
>> has been in the Image Streaming API spec and hence implemented in
>> libvirt for a while now.  If we change this then an old client which
>> only understands NotSupported will not know what to do with the
>> Unsupported error.
>>
>> (Unsupported was not in QEMU when the Image Streaming API was defined.)
> 
> Let me try to understand it: is libvirt relying on an off tree API and
> we are now required to have stable guarantees to off tree APIs?

No.  Libvirt recognizes the off-tree spelling, but does not rely on it -
after all, the goal of libvirt is to provide the high level action,
using whatever underlying mechanism(s) necessary to get to that action,
even if it means using several different attempts until one actually works.

If a user has the older libvirt, which only expects the off-tree
spelling, then that user's setup will break if they upgrade qemu but not
libvirt.  But that's not a severe problem - they could have only been
relying on the situation if they were using an off-tree build in the
first place, so they should be aware that upgrading qemu is a
potentially risky scenario, and that they may have to deal with the pieces.

Newer libvirt can be easily taught to recognize both the off-tree and
stable spellings of the error (checking the stable first, of course,
since that will be more likely as the off-tree qemu builds filter out
over time).  At which point, using either the off-tree qemu or the
stable qemu should both work with the newer libvirt.

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

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Anthony Liguori]

On 01/05/2012 09:35 AM, Eric Blake wrote:
> On 01/05/2012 07:16 AM, Luiz Capitulino wrote:
>>> I know.  We're stuck in a hard place here again because NotSupported
>>> has been in the Image Streaming API spec and hence implemented in
>>> libvirt for a while now.  If we change this then an old client which
>>> only understands NotSupported will not know what to do with the
>>> Unsupported error.
>>>
>>> (Unsupported was not in QEMU when the Image Streaming API was defined.)
>>
>> Let me try to understand it: is libvirt relying on an off tree API and
>> we are now required to have stable guarantees to off tree APIs?
>
> No.  Libvirt recognizes the off-tree spelling, but does not rely on it -
> after all, the goal of libvirt is to provide the high level action,
> using whatever underlying mechanism(s) necessary to get to that action,
> even if it means using several different attempts until one actually works.
>
> If a user has the older libvirt, which only expects the off-tree
> spelling, then that user's setup will break if they upgrade qemu but not
> libvirt.  But that's not a severe problem - they could have only been
> relying on the situation if they were using an off-tree build in the
> first place, so they should be aware that upgrading qemu is a
> potentially risky scenario, and that they may have to deal with the pieces.

Right, this is the difference between ABI compatibility and strict backwards 
compatibility.

To achieve ABI compatibility, we need to not overload BLOCK_JOB_COMPLETED to 
mean something other than libvirt what expects it to mean.

We MUST provide ABI compatibility and SHOULD provide backwards compatibility 
whenever possible.

In this case, I'd suggest that in the very least, we should add 
BLOCK_JOB_COMPLETED to qapi-schema.json with gen=False set.  That way it's 
codified in the schema to ensure we maintain ABI compatibility.

That said, I'm inclined to say that we should just use the BLOCK_JOB_COMPLETED 
name because I don't think we gain a lot by using QMP_JOB_COMPLETED (not that we 
shouldn't introduce it, but using it here isn't going to make or break anything).

With respect to libvirt relying on interfaces before they exist in QEMU, we need 
to be a bit flexible here.  We want to get better at co-development to help make 
libvirt support QEMU features as the bleeding edge.

Forcing libvirt to wait until a feature is fully baked in QEMU will ensure 
there's always a feature gap in libvirt which is in none of our best interests.

Now that we have gen=False support in qapi-schema.json, we can agree to an API 
and add it to QEMU before we fully implement it.  This gives libvirt something 
to work off of that they can rely upon.

Regards,

Anthony Liguori

>
> Newer libvirt can be easily taught to recognize both the off-tree and
> stable spellings of the error (checking the stable first, of course,
> since that will be more likely as the off-tree qemu builds filter out
> over time).  At which point, using either the off-tree qemu or the
> stable qemu should both work with the newer libvirt.
>

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Thu, 05 Jan 2012 09:56:44 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 01/05/2012 09:35 AM, Eric Blake wrote:
> > On 01/05/2012 07:16 AM, Luiz Capitulino wrote:
> >>> I know.  We're stuck in a hard place here again because NotSupported
> >>> has been in the Image Streaming API spec and hence implemented in
> >>> libvirt for a while now.  If we change this then an old client which
> >>> only understands NotSupported will not know what to do with the
> >>> Unsupported error.
> >>>
> >>> (Unsupported was not in QEMU when the Image Streaming API was defined.)
> >>
> >> Let me try to understand it: is libvirt relying on an off tree API and
> >> we are now required to have stable guarantees to off tree APIs?
> >
> > No.  Libvirt recognizes the off-tree spelling, but does not rely on it -
> > after all, the goal of libvirt is to provide the high level action,
> > using whatever underlying mechanism(s) necessary to get to that action,
> > even if it means using several different attempts until one actually works.
> >
> > If a user has the older libvirt, which only expects the off-tree
> > spelling, then that user's setup will break if they upgrade qemu but not
> > libvirt.  But that's not a severe problem - they could have only been
> > relying on the situation if they were using an off-tree build in the
> > first place, so they should be aware that upgrading qemu is a
> > potentially risky scenario, and that they may have to deal with the pieces.
> 
> Right, this is the difference between ABI compatibility and strict backwards 
> compatibility.
> 
> To achieve ABI compatibility, we need to not overload BLOCK_JOB_COMPLETED to 
> mean something other than libvirt what expects it to mean.
> 
> We MUST provide ABI compatibility and SHOULD provide backwards compatibility 
> whenever possible.
> 
> In this case, I'd suggest that in the very least, we should add 
> BLOCK_JOB_COMPLETED to qapi-schema.json with gen=False set.  That way it's 
> codified in the schema to ensure we maintain ABI compatibility.
> 
> That said, I'm inclined to say that we should just use the BLOCK_JOB_COMPLETED 
> name because I don't think we gain a lot by using QMP_JOB_COMPLETED (not that we 
> shouldn't introduce it, but using it here isn't going to make or break anything).

What I'm proposing is not just a rename, but adding proper async support to QMP,
instead of adding something that is specific to the block layer.

> With respect to libvirt relying on interfaces before they exist in QEMU, we need 
> to be a bit flexible here.  We want to get better at co-development to help make 
> libvirt support QEMU features as the bleeding edge.
> 
> Forcing libvirt to wait until a feature is fully baked in QEMU will ensure 
> there's always a feature gap in libvirt which is in none of our best interests.

We can ask them to wait at least until the API is merged. Most good review
and potential problems will only come when the patches are worked on and
reviewed on the list.

> Now that we have gen=False support in qapi-schema.json, we can agree to an API 
> and add it to QEMU before we fully implement it.  This gives libvirt something 
> to work off of that they can rely upon.

Meaning that we can modify the API later? Okay, but in this case we have
reasons to modify it before it's merged and I don't see why we shouldn't
do it.

> 
> Regards,
> 
> Anthony Liguori
> 
> >
> > Newer libvirt can be easily taught to recognize both the off-tree and
> > stable spellings of the error (checking the stable first, of course,
> > since that will be more likely as the off-tree qemu builds filter out
> > over time).  At which point, using either the off-tree qemu or the
> > stable qemu should both work with the newer libvirt.
> >
> 

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Stefan Hajnoczi]

On Thu, Jan 5, 2012 at 8:26 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Thu, 05 Jan 2012 09:56:44 -0600
> Anthony Liguori <aliguori@us.ibm.com> wrote:
>
>> On 01/05/2012 09:35 AM, Eric Blake wrote:
>> > On 01/05/2012 07:16 AM, Luiz Capitulino wrote:
>> >>> I know.  We're stuck in a hard place here again because NotSupported
>> >>> has been in the Image Streaming API spec and hence implemented in
>> >>> libvirt for a while now.  If we change this then an old client which
>> >>> only understands NotSupported will not know what to do with the
>> >>> Unsupported error.
>> >>>
>> >>> (Unsupported was not in QEMU when the Image Streaming API was defined.)
>> >>
>> >> Let me try to understand it: is libvirt relying on an off tree API and
>> >> we are now required to have stable guarantees to off tree APIs?
>> >
>> > No.  Libvirt recognizes the off-tree spelling, but does not rely on it -
>> > after all, the goal of libvirt is to provide the high level action,
>> > using whatever underlying mechanism(s) necessary to get to that action,
>> > even if it means using several different attempts until one actually works.
>> >
>> > If a user has the older libvirt, which only expects the off-tree
>> > spelling, then that user's setup will break if they upgrade qemu but not
>> > libvirt.  But that's not a severe problem - they could have only been
>> > relying on the situation if they were using an off-tree build in the
>> > first place, so they should be aware that upgrading qemu is a
>> > potentially risky scenario, and that they may have to deal with the pieces.
>>
>> Right, this is the difference between ABI compatibility and strict backwards
>> compatibility.
>>
>> To achieve ABI compatibility, we need to not overload BLOCK_JOB_COMPLETED to
>> mean something other than libvirt what expects it to mean.
>>
>> We MUST provide ABI compatibility and SHOULD provide backwards compatibility
>> whenever possible.
>>
>> In this case, I'd suggest that in the very least, we should add
>> BLOCK_JOB_COMPLETED to qapi-schema.json with gen=False set.  That way it's
>> codified in the schema to ensure we maintain ABI compatibility.
>>
>> That said, I'm inclined to say that we should just use the BLOCK_JOB_COMPLETED
>> name because I don't think we gain a lot by using QMP_JOB_COMPLETED (not that we
>> shouldn't introduce it, but using it here isn't going to make or break anything).
>
> What I'm proposing is not just a rename, but adding proper async support to QMP,
> instead of adding something that is specific to the block layer.

Proper async support - if you mean the ability to have multiple QMP
commands pending at a time - is harder than just fixing QEMU.  Clients
also need to start taking advantage of it.  Clients that do not will
be unable to continue when a QMP command takes a long time to
complete.

I think avoiding long-running QMP commands is a good idea.  We have
events which can be used to signal completion.  It's easy to implement
and does not require clients to change the way they think about QMP
commands.

Today I doubt many QMP clients have implemented multiple pending
commands, although the wire protocol allows it.

>> With respect to libvirt relying on interfaces before they exist in QEMU, we need
>> to be a bit flexible here.  We want to get better at co-development to help make
>> libvirt support QEMU features as the bleeding edge.
>>
>> Forcing libvirt to wait until a feature is fully baked in QEMU will ensure
>> there's always a feature gap in libvirt which is in none of our best interests.
>
> We can ask them to wait at least until the API is merged. Most good review
> and potential problems will only come when the patches are worked on and
> reviewed on the list.

The API was agreed on between QEMU and libvirt developers on the mailing
lists - you were included in that process.  Back in August I sent
patches which you saw ("[0/4] Image Streaming API").

I know the API is not what we'd design today when it comes to the
cosmetics.  We'd want to name things differently, use the Unsupported
event which was introduced in the meantime, and maybe make the job
completion concept generic.

QMP and QAPI have evolved in the time that this feature has been
reimplemented.  I have tried to keep up with QMP but the API itself is
from August.  We can't keep redrawing the lines.

In summary:
 * The API was designed and agreed several months ago.
 * You saw it back then and I've kept you up-to-date along the way.
 * It predates current QAPI conventions.
 * Merging it poses no problem, changing the API breaks existing libvirt.

I do feel bad that the code has been out of qemu.git for so long and I
certainly won't attempt this again in the future.  But I really think
the pros and cons say we should accept it as an August 2011 API just
like many of the other HMP/QMP commands we carry.

Regarding being more flexible about working together with libvirt, I
do think it's important to work on APIs together.  This avoids use
developing something purely from the QEMU internal perspective which
turns out to be unconsumable by our biggest QMP user :).

Stefan

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Fri, 6 Jan 2012 11:06:12 +0000
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, Jan 5, 2012 at 8:26 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Thu, 05 Jan 2012 09:56:44 -0600
> > Anthony Liguori <aliguori@us.ibm.com> wrote:
> >
> >> On 01/05/2012 09:35 AM, Eric Blake wrote:
> >> > On 01/05/2012 07:16 AM, Luiz Capitulino wrote:
> >> >>> I know.  We're stuck in a hard place here again because NotSupported
> >> >>> has been in the Image Streaming API spec and hence implemented in
> >> >>> libvirt for a while now.  If we change this then an old client which
> >> >>> only understands NotSupported will not know what to do with the
> >> >>> Unsupported error.
> >> >>>
> >> >>> (Unsupported was not in QEMU when the Image Streaming API was defined.)
> >> >>
> >> >> Let me try to understand it: is libvirt relying on an off tree API and
> >> >> we are now required to have stable guarantees to off tree APIs?
> >> >
> >> > No.  Libvirt recognizes the off-tree spelling, but does not rely on it -
> >> > after all, the goal of libvirt is to provide the high level action,
> >> > using whatever underlying mechanism(s) necessary to get to that action,
> >> > even if it means using several different attempts until one actually works.
> >> >
> >> > If a user has the older libvirt, which only expects the off-tree
> >> > spelling, then that user's setup will break if they upgrade qemu but not
> >> > libvirt.  But that's not a severe problem - they could have only been
> >> > relying on the situation if they were using an off-tree build in the
> >> > first place, so they should be aware that upgrading qemu is a
> >> > potentially risky scenario, and that they may have to deal with the pieces.
> >>
> >> Right, this is the difference between ABI compatibility and strict backwards
> >> compatibility.
> >>
> >> To achieve ABI compatibility, we need to not overload BLOCK_JOB_COMPLETED to
> >> mean something other than libvirt what expects it to mean.
> >>
> >> We MUST provide ABI compatibility and SHOULD provide backwards compatibility
> >> whenever possible.
> >>
> >> In this case, I'd suggest that in the very least, we should add
> >> BLOCK_JOB_COMPLETED to qapi-schema.json with gen=False set.  That way it's
> >> codified in the schema to ensure we maintain ABI compatibility.
> >>
> >> That said, I'm inclined to say that we should just use the BLOCK_JOB_COMPLETED
> >> name because I don't think we gain a lot by using QMP_JOB_COMPLETED (not that we
> >> shouldn't introduce it, but using it here isn't going to make or break anything).
> >
> > What I'm proposing is not just a rename, but adding proper async support to QMP,
> > instead of adding something that is specific to the block layer.
> 
> Proper async support - if you mean the ability to have multiple QMP
> commands pending at a time - is harder than just fixing QEMU.  Clients
> also need to start taking advantage of it.  Clients that do not will
> be unable to continue when a QMP command takes a long time to
> complete.

They can be fixed if we offer proper async support. Today they can't.

> I think avoiding long-running QMP commands is a good idea.  We have
> events which can be used to signal completion.  It's easy to implement
> and does not require clients to change the way they think about QMP
> commands.

I agree in principle, but in practice we risk having different subsystems
and different commands introducing their own async support which is going
to make our API (which is already far from perfect) impossible to use,
not to mention the maintainability hell that will arise from it.

Note that I'm not exactly advocating for heavyweight async support, I just
want to avoid keeping messing with this area.

Maybe, we could go real simple by having a standard event for
asynchronous commands, say ASYNC_CMD_FINISHED or something and that event
would contain only the command id and if the command succeeded or
failed. The APIs for cancelling and querying would have to be provided
by the command itself.

I can start a new thread to discuss async support. I haven't done it yet
because I don't have a concrete proposal and I also suspect that people are
tired of discussing this over and over again.

> Today I doubt many QMP clients have implemented multiple pending
> commands, although the wire protocol allows it.

That's true, but adding the id field in the command dict was silly, as
we don't support multiple pending commands.

> >> With respect to libvirt relying on interfaces before they exist in QEMU, we need
> >> to be a bit flexible here.  We want to get better at co-development to help make
> >> libvirt support QEMU features as the bleeding edge.
> >>
> >> Forcing libvirt to wait until a feature is fully baked in QEMU will ensure
> >> there's always a feature gap in libvirt which is in none of our best interests.
> >
> > We can ask them to wait at least until the API is merged. Most good review
> > and potential problems will only come when the patches are worked on and
> > reviewed on the list.
> 
> The API was agreed on between QEMU and libvirt developers on the mailing
> lists - you were included in that process.  Back in August I sent
> patches which you saw ("[0/4] Image Streaming API").
> 
> I know the API is not what we'd design today when it comes to the
> cosmetics.  We'd want to name things differently, use the Unsupported
> event which was introduced in the meantime, and maybe make the job
> completion concept generic.
> 
> QMP and QAPI have evolved in the time that this feature has been
> reimplemented.  I have tried to keep up with QMP but the API itself is
> from August.  We can't keep redrawing the lines.
> 
> In summary:
>  * The API was designed and agreed several months ago.
>  * You saw it back then and I've kept you up-to-date along the way.
>  * It predates current QAPI conventions.
>  * Merging it poses no problem, changing the API breaks existing libvirt.

It does pose problems. The name changes I've proposed are not minor
things, it's about conforming to the protocol which is quite important.
Duplicating errors is something that just doesn't make sense either.

And most importantly, you're adding async support to the block layer. This
means that we'll have two different async APIs when we add one to QMP,
or worse other subsystems will be motivated to have their own async APIs
too.

> I do feel bad that the code has been out of qemu.git for so long and I
> certainly won't attempt this again in the future.  But I really think
> the pros and cons say we should accept it as an August 2011 API just
> like many of the other HMP/QMP commands we carry.

I disagree. This should be reviwed and changed as any other submission.

> Regarding being more flexible about working together with libvirt, I
> do think it's important to work on APIs together.  This avoids use
> developing something purely from the QEMU internal perspective which
> turns out to be unconsumable by our biggest QMP user :).

We do work together with them. I've never ignored their opinion and I'm
probably the strongest opinionated when it comes to compatibility.

I just can't see how accepting something that is now rotted is going
to help either of us.

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Anthony Liguori]

On 01/06/2012 06:45 AM, Luiz Capitulino wrote:
> On Fri, 6 Jan 2012 11:06:12 +0000
> Stefan Hajnoczi<stefanha@gmail.com>  wrote:
>
>> Proper async support - if you mean the ability to have multiple QMP
>> commands pending at a time - is harder than just fixing QEMU.  Clients
>> also need to start taking advantage of it.  Clients that do not will
>> be unable to continue when a QMP command takes a long time to
>> complete.
>
> They can be fixed if we offer proper async support. Today they can't.
>
>> I think avoiding long-running QMP commands is a good idea.  We have
>> events which can be used to signal completion.  It's easy to implement
>> and does not require clients to change the way they think about QMP
>> commands.
>
> I agree in principle, but in practice we risk having different subsystems
> and different commands introducing their own async support which is going
> to make our API (which is already far from perfect) impossible to use,
> not to mention the maintainability hell that will arise from it.

I absolutely agree with you but practically speaking, we don't have generic 
async support today.

It's been my experience that holding up patch series for generic infrastructure 
that does exist 1) causes unnecessary angst in contributors 2) puts pressure on 
the infrastructure to get something in fast vs. get something in that's good.

And honestly, it's (2) that I worry the most about.  I don't want us to rush 
async support because we're eager to get block streaming merged.  This is why 
I'm not holding any new devices back while we get QOM merged even if it creates 
more work for me and introduces new compatibility problems to solve.

We also need to look at this interface as a public interface whether we 
technically committed it to or not.  The fact is, an important user is relying 
upon so that makes it a supported interface.  Even though I absolutely hate it, 
this is why we haven't changed the help output even after all of these years. 
Not breaking users should be one of our highest priorities.

Now we could change this command to make it a better QMP interface and we could 
do that in a compatible fashion.

However, since I think we'll get proper async support really soon and that will 
involve fundamentally changing this command (along with a bunch of other 
commands), I don't think there's a lot of value in making cosmetic changes right 
now.  If we're going to break backwards compatibility, I'd rather do it once 
than twice.

What I'd suggest is that we take the command in as-is and we mark it:

Since: 1.1
Deprecated: 1.2
See Also: TBD

The idea being that we'll introduce new generic async commands in 1.2 and 
deprecate this command.  We can figure out the removal schedule then too.  Since 
this command hasn't been around all that long, we can probably have a short 
removal schedule.

We should also mark the other psuedo-async commands this way too FWIW.

Regards,

Anthony Liguori

> Note that I'm not exactly advocating for heavyweight async support, I just
> want to avoid keeping messing with this area.
>
> Maybe, we could go real simple by having a standard event for
> asynchronous commands, say ASYNC_CMD_FINISHED or something and that event
> would contain only the command id and if the command succeeded or
> failed. The APIs for cancelling and querying would have to be provided
> by the command itself.
>
> I can start a new thread to discuss async support. I haven't done it yet
> because I don't have a concrete proposal and I also suspect that people are
> tired of discussing this over and over again.
>
>> Today I doubt many QMP clients have implemented multiple pending
>> commands, although the wire protocol allows it.
>
> That's true, but adding the id field in the command dict was silly, as
> we don't support multiple pending commands.
>
>>>> With respect to libvirt relying on interfaces before they exist in QEMU, we need
>>>> to be a bit flexible here.  We want to get better at co-development to help make
>>>> libvirt support QEMU features as the bleeding edge.
>>>>
>>>> Forcing libvirt to wait until a feature is fully baked in QEMU will ensure
>>>> there's always a feature gap in libvirt which is in none of our best interests.
>>>
>>> We can ask them to wait at least until the API is merged. Most good review
>>> and potential problems will only come when the patches are worked on and
>>> reviewed on the list.
>>
>> The API was agreed on between QEMU and libvirt developers on the mailing
>> lists - you were included in that process.  Back in August I sent
>> patches which you saw ("[0/4] Image Streaming API").
>>
>> I know the API is not what we'd design today when it comes to the
>> cosmetics.  We'd want to name things differently, use the Unsupported
>> event which was introduced in the meantime, and maybe make the job
>> completion concept generic.
>>
>> QMP and QAPI have evolved in the time that this feature has been
>> reimplemented.  I have tried to keep up with QMP but the API itself is
>> from August.  We can't keep redrawing the lines.
>>
>> In summary:
>>   * The API was designed and agreed several months ago.
>>   * You saw it back then and I've kept you up-to-date along the way.
>>   * It predates current QAPI conventions.
>>   * Merging it poses no problem, changing the API breaks existing libvirt.
>
> It does pose problems. The name changes I've proposed are not minor
> things, it's about conforming to the protocol which is quite important.
> Duplicating errors is something that just doesn't make sense either.
>
> And most importantly, you're adding async support to the block layer. This
> means that we'll have two different async APIs when we add one to QMP,
> or worse other subsystems will be motivated to have their own async APIs
> too.
>
>> I do feel bad that the code has been out of qemu.git for so long and I
>> certainly won't attempt this again in the future.  But I really think
>> the pros and cons say we should accept it as an August 2011 API just
>> like many of the other HMP/QMP commands we carry.
>
> I disagree. This should be reviwed and changed as any other submission.
>
>> Regarding being more flexible about working together with libvirt, I
>> do think it's important to work on APIs together.  This avoids use
>> developing something purely from the QEMU internal perspective which
>> turns out to be unconsumable by our biggest QMP user :).
>
> We do work together with them. I've never ignored their opinion and I'm
> probably the strongest opinionated when it comes to compatibility.
>
> I just can't see how accepting something that is now rotted is going
> to help either of us.
>

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Fri, 06 Jan 2012 09:08:19 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 01/06/2012 06:45 AM, Luiz Capitulino wrote:
> > On Fri, 6 Jan 2012 11:06:12 +0000
> > Stefan Hajnoczi<stefanha@gmail.com>  wrote:
> >
> >> Proper async support - if you mean the ability to have multiple QMP
> >> commands pending at a time - is harder than just fixing QEMU.  Clients
> >> also need to start taking advantage of it.  Clients that do not will
> >> be unable to continue when a QMP command takes a long time to
> >> complete.
> >
> > They can be fixed if we offer proper async support. Today they can't.
> >
> >> I think avoiding long-running QMP commands is a good idea.  We have
> >> events which can be used to signal completion.  It's easy to implement
> >> and does not require clients to change the way they think about QMP
> >> commands.
> >
> > I agree in principle, but in practice we risk having different subsystems
> > and different commands introducing their own async support which is going
> > to make our API (which is already far from perfect) impossible to use,
> > not to mention the maintainability hell that will arise from it.
> 
> I absolutely agree with you but practically speaking, we don't have generic 
> async support today.
> 
> It's been my experience that holding up patch series for generic infrastructure 
> that does exist 1) causes unnecessary angst in contributors 2) puts pressure on 
> the infrastructure to get something in fast vs. get something in that's good.
> 
> And honestly, it's (2) that I worry the most about.  I don't want us to rush 
> async support because we're eager to get block streaming merged.  This is why 
> I'm not holding any new devices back while we get QOM merged even if it creates 
> more work for me and introduces new compatibility problems to solve.

I agree.

> We also need to look at this interface as a public interface whether we 
> technically committed it to or not.  The fact is, an important user is relying 
> upon so that makes it a supported interface.  Even though I absolutely hate it, 
> this is why we haven't changed the help output even after all of these years. 
> Not breaking users should be one of our highest priorities.

One thing I don't understand: how is libvirt relying on it if it doesn't
exist in qemu.git yet?

> Now we could change this command to make it a better QMP interface and we could 
> do that in a compatible fashion.
> 
> However, since I think we'll get proper async support really soon and that will 
> involve fundamentally changing this command (along with a bunch of other 
> commands), I don't think there's a lot of value in making cosmetic changes right 
> now.  If we're going to break backwards compatibility, I'd rather do it once 
> than twice.

It goes beyond cosmetic changes. For example, will we allow other async
block commands to use this interface? And if we're doing this for block,
why not accept something similar for other subsystems if someone happen to
submit it?

Let me take a non-cosmetic change request example. The BLOCK_JOB_COMPLETED
event has a 'error' field. However, it's impossible to know which error
happened because the 'error' field contains only the human error description.

Another problem: the event is called BLOCK_JOB_COMPLETED, but it's tied
to the streaming API. If we allow other commands to use it, they will likely
have to add fields there, making the event worse than it already is.

There's more, because I skipped this review in v3 as I jumped to the
"proper async support" discussion...

> What I'd suggest is that we take the command in as-is and we mark it:
> 
> Since: 1.1
> Deprecated: 1.2
> See Also: TBD
> 
> The idea being that we'll introduce new generic async commands in 1.2 and 
> deprecate this command.  We can figure out the removal schedule then too.  Since 
> this command hasn't been around all that long, we can probably have a short 
> removal schedule.

That makes its inclusion even discussable :) A few (very honest) questions:

 1. Is it really worth it to have the command for one or two releases?

 2. Will we allow other block commands to use this async API?

 3. Are we going to accept other ad-hoc async APIs until we have a
    proper one?

> 
> We should also mark the other psuedo-async commands this way too FWIW.
> 
> Regards,
> 
> Anthony Liguori
> 
> > Note that I'm not exactly advocating for heavyweight async support, I just
> > want to avoid keeping messing with this area.
> >
> > Maybe, we could go real simple by having a standard event for
> > asynchronous commands, say ASYNC_CMD_FINISHED or something and that event
> > would contain only the command id and if the command succeeded or
> > failed. The APIs for cancelling and querying would have to be provided
> > by the command itself.
> >
> > I can start a new thread to discuss async support. I haven't done it yet
> > because I don't have a concrete proposal and I also suspect that people are
> > tired of discussing this over and over again.
> >
> >> Today I doubt many QMP clients have implemented multiple pending
> >> commands, although the wire protocol allows it.
> >
> > That's true, but adding the id field in the command dict was silly, as
> > we don't support multiple pending commands.
> >
> >>>> With respect to libvirt relying on interfaces before they exist in QEMU, we need
> >>>> to be a bit flexible here.  We want to get better at co-development to help make
> >>>> libvirt support QEMU features as the bleeding edge.
> >>>>
> >>>> Forcing libvirt to wait until a feature is fully baked in QEMU will ensure
> >>>> there's always a feature gap in libvirt which is in none of our best interests.
> >>>
> >>> We can ask them to wait at least until the API is merged. Most good review
> >>> and potential problems will only come when the patches are worked on and
> >>> reviewed on the list.
> >>
> >> The API was agreed on between QEMU and libvirt developers on the mailing
> >> lists - you were included in that process.  Back in August I sent
> >> patches which you saw ("[0/4] Image Streaming API").
> >>
> >> I know the API is not what we'd design today when it comes to the
> >> cosmetics.  We'd want to name things differently, use the Unsupported
> >> event which was introduced in the meantime, and maybe make the job
> >> completion concept generic.
> >>
> >> QMP and QAPI have evolved in the time that this feature has been
> >> reimplemented.  I have tried to keep up with QMP but the API itself is
> >> from August.  We can't keep redrawing the lines.
> >>
> >> In summary:
> >>   * The API was designed and agreed several months ago.
> >>   * You saw it back then and I've kept you up-to-date along the way.
> >>   * It predates current QAPI conventions.
> >>   * Merging it poses no problem, changing the API breaks existing libvirt.
> >
> > It does pose problems. The name changes I've proposed are not minor
> > things, it's about conforming to the protocol which is quite important.
> > Duplicating errors is something that just doesn't make sense either.
> >
> > And most importantly, you're adding async support to the block layer. This
> > means that we'll have two different async APIs when we add one to QMP,
> > or worse other subsystems will be motivated to have their own async APIs
> > too.
> >
> >> I do feel bad that the code has been out of qemu.git for so long and I
> >> certainly won't attempt this again in the future.  But I really think
> >> the pros and cons say we should accept it as an August 2011 API just
> >> like many of the other HMP/QMP commands we carry.
> >
> > I disagree. This should be reviwed and changed as any other submission.
> >
> >> Regarding being more flexible about working together with libvirt, I
> >> do think it's important to work on APIs together.  This avoids use
> >> developing something purely from the QEMU internal perspective which
> >> turns out to be unconsumable by our biggest QMP user :).
> >
> > We do work together with them. I've never ignored their opinion and I'm
> > probably the strongest opinionated when it comes to compatibility.
> >
> > I just can't see how accepting something that is now rotted is going
> > to help either of us.
> >
> 

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Anthony Liguori]

On 01/06/2012 01:42 PM, Luiz Capitulino wrote:
> On Fri, 06 Jan 2012 09:08:19 -0600
>> We also need to look at this interface as a public interface whether we
>> technically committed it to or not.  The fact is, an important user is relying
>> upon so that makes it a supported interface.  Even though I absolutely hate it,
>> this is why we haven't changed the help output even after all of these years.
>> Not breaking users should be one of our highest priorities.
>
> One thing I don't understand: how is libvirt relying on it if it doesn't
> exist in qemu.git yet?

Because there was a discussion on qemu-devel and we agreed on an interface that 
both sides would implement to.

I expect this to happen more often in the future too.

>> Now we could change this command to make it a better QMP interface and we could
>> do that in a compatible fashion.
>>
>> However, since I think we'll get proper async support really soon and that will
>> involve fundamentally changing this command (along with a bunch of other
>> commands), I don't think there's a lot of value in making cosmetic changes right
>> now.  If we're going to break backwards compatibility, I'd rather do it once
>> than twice.
>
> It goes beyond cosmetic changes. For example, will we allow other async
> block commands to use this interface? And if we're doing this for block,
> why not accept something similar for other subsystems if someone happen to
> submit it?

Yes, I'm in agreement with you on direction.  But adding proper async support is 
not a simple task, and blocking this interface based on that is a bad idea for 
reasons previously mentioned.

> Let me take a non-cosmetic change request example. The BLOCK_JOB_COMPLETED
> event has a 'error' field. However, it's impossible to know which error
> happened because the 'error' field contains only the human error description.

Ok.  That's worth thinking about.

> Another problem: the event is called BLOCK_JOB_COMPLETED, but it's tied
> to the streaming API. If we allow other commands to use it, they will likely
> have to add fields there, making the event worse than it already is.

But aren't we going to introduce a proper async interface?  This is what has me 
confused.

> There's more, because I skipped this review in v3 as I jumped to the
> "proper async support" discussion...

Well let's do proper async support.  As I mentioned, I'd rather take this 
command in as-is, introduce proper async support, and then deprecate a bunch of 
stuff at once.

>> What I'd suggest is that we take the command in as-is and we mark it:
>>
>> Since: 1.1
>> Deprecated: 1.2
>> See Also: TBD
>>
>> The idea being that we'll introduce new generic async commands in 1.2 and
>> deprecate this command.  We can figure out the removal schedule then too.  Since
>> this command hasn't been around all that long, we can probably have a short
>> removal schedule.
>
> That makes its inclusion even discussable :) A few (very honest) questions:
>
>   1. Is it really worth it to have the command for one or two releases?

Yes.  The most important consideration IMHO is parallelizing development.  We 
want the block layer to evolve to the greatest extent possible independent of 
our other subsystems.  If we don't have the right infrastructure in QMP to 
support a block feature, that shouldn't hold up progress in the block layer to 
the greatest extent possible.

>   2. Will we allow other block commands to use this async API?

Depends on how long it takes to do "proper async support".

>   3. Are we going to accept other ad-hoc async APIs until we have a
>      proper one?

Yes.  So let's get serious about getting a proper interface in :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Tue, 10 Jan 2012 13:18:41 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 01/06/2012 01:42 PM, Luiz Capitulino wrote:
> > On Fri, 06 Jan 2012 09:08:19 -0600
> >> We also need to look at this interface as a public interface whether we
> >> technically committed it to or not.  The fact is, an important user is relying
> >> upon so that makes it a supported interface.  Even though I absolutely hate it,
> >> this is why we haven't changed the help output even after all of these years.
> >> Not breaking users should be one of our highest priorities.
> >
> > One thing I don't understand: how is libvirt relying on it if it doesn't
> > exist in qemu.git yet?
> 
> Because there was a discussion on qemu-devel and we agreed on an interface that 
> both sides would implement to.
> 
> I expect this to happen more often in the future too.

For future commands we either, implement it right away or ask libvirt to
wait for the command to be merged, or at least pass initial review.

> 
> >> Now we could change this command to make it a better QMP interface and we could
> >> do that in a compatible fashion.
> >>
> >> However, since I think we'll get proper async support really soon and that will
> >> involve fundamentally changing this command (along with a bunch of other
> >> commands), I don't think there's a lot of value in making cosmetic changes right
> >> now.  If we're going to break backwards compatibility, I'd rather do it once
> >> than twice.
> >
> > It goes beyond cosmetic changes. For example, will we allow other async
> > block commands to use this interface? And if we're doing this for block,
> > why not accept something similar for other subsystems if someone happen to
> > submit it?
> 
> Yes, I'm in agreement with you on direction.  But adding proper async support is 
> not a simple task, and blocking this interface based on that is a bad idea for 
> reasons previously mentioned.
> 
> > Let me take a non-cosmetic change request example. The BLOCK_JOB_COMPLETED
> > event has a 'error' field. However, it's impossible to know which error
> > happened because the 'error' field contains only the human error description.
> 
> Ok.  That's worth thinking about.
> 
> > Another problem: the event is called BLOCK_JOB_COMPLETED, but it's tied
> > to the streaming API. If we allow other commands to use it, they will likely
> > have to add fields there, making the event worse than it already is.
> 
> But aren't we going to introduce a proper async interface?  This is what has me 
> confused.

Yes, I was thinking about new block commands using this API before we get
proper async support...

> > There's more, because I skipped this review in v3 as I jumped to the
> > "proper async support" discussion...
> 
> Well let's do proper async support.  As I mentioned, I'd rather take this 
> command in as-is, introduce proper async support, and then deprecate a bunch of 
> stuff at once.

Ok, I'm willing to do this as Stefan has agreed on deprecating this as soon as
we get proper async support.

> 
> >> What I'd suggest is that we take the command in as-is and we mark it:
> >>
> >> Since: 1.1
> >> Deprecated: 1.2
> >> See Also: TBD
> >>
> >> The idea being that we'll introduce new generic async commands in 1.2 and
> >> deprecate this command.  We can figure out the removal schedule then too.  Since
> >> this command hasn't been around all that long, we can probably have a short
> >> removal schedule.
> >
> > That makes its inclusion even discussable :) A few (very honest) questions:
> >
> >   1. Is it really worth it to have the command for one or two releases?
> 
> Yes.  The most important consideration IMHO is parallelizing development.  We 
> want the block layer to evolve to the greatest extent possible independent of 
> our other subsystems.  If we don't have the right infrastructure in QMP to 
> support a block feature, that shouldn't hold up progress in the block layer to 
> the greatest extent possible.
> 
> >   2. Will we allow other block commands to use this async API?
> 
> Depends on how long it takes to do "proper async support".
> 
> >   3. Are we going to accept other ad-hoc async APIs until we have a
> >      proper one?
> 
> Yes.  So let's get serious about getting a proper interface in :-)

ACK

> 
> Regards,
> 
> Anthony Liguori
> 

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Anthony Liguori]

On 01/10/2012 02:55 PM, Luiz Capitulino wrote:
> On Tue, 10 Jan 2012 13:18:41 -0600
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>> On 01/06/2012 01:42 PM, Luiz Capitulino wrote:
>>> On Fri, 06 Jan 2012 09:08:19 -0600
>>>> We also need to look at this interface as a public interface whether we
>>>> technically committed it to or not.  The fact is, an important user is relying
>>>> upon so that makes it a supported interface.  Even though I absolutely hate it,
>>>> this is why we haven't changed the help output even after all of these years.
>>>> Not breaking users should be one of our highest priorities.
>>>
>>> One thing I don't understand: how is libvirt relying on it if it doesn't
>>> exist in qemu.git yet?
>>
>> Because there was a discussion on qemu-devel and we agreed on an interface that
>> both sides would implement to.
>>
>> I expect this to happen more often in the future too.
>
> For future commands we either, implement it right away or ask libvirt to
> wait for the command to be merged, or at least pass initial review.

Or commit the schema entry to qapi-schema.json with gen=False.

But when this command was first proposed, qapi-schema.json didn't exist in the 
tree :-)

>> But aren't we going to introduce a proper async interface?  This is what has me
>> confused.
>
> Yes, I was thinking about new block commands using this API before we get
> proper async support...

Well let's avoid that problem by doing proper async support before we get new 
block commands ;-)

>>> There's more, because I skipped this review in v3 as I jumped to the
>>> "proper async support" discussion...
>>
>> Well let's do proper async support.  As I mentioned, I'd rather take this
>> command in as-is, introduce proper async support, and then deprecate a bunch of
>> stuff at once.
>
> Ok, I'm willing to do this as Stefan has agreed on deprecating this as soon as
> we get proper async support.

Excellent.

BTW, you mentioned that you were going to send an RFC for proper async support?

Regards,

Anthony Liguori

>>
>>>> What I'd suggest is that we take the command in as-is and we mark it:
>>>>
>>>> Since: 1.1
>>>> Deprecated: 1.2
>>>> See Also: TBD
>>>>
>>>> The idea being that we'll introduce new generic async commands in 1.2 and
>>>> deprecate this command.  We can figure out the removal schedule then too.  Since
>>>> this command hasn't been around all that long, we can probably have a short
>>>> removal schedule.
>>>
>>> That makes its inclusion even discussable :) A few (very honest) questions:
>>>
>>>    1. Is it really worth it to have the command for one or two releases?
>>
>> Yes.  The most important consideration IMHO is parallelizing development.  We
>> want the block layer to evolve to the greatest extent possible independent of
>> our other subsystems.  If we don't have the right infrastructure in QMP to
>> support a block feature, that shouldn't hold up progress in the block layer to
>> the greatest extent possible.
>>
>>>    2. Will we allow other block commands to use this async API?
>>
>> Depends on how long it takes to do "proper async support".
>>
>>>    3. Are we going to accept other ad-hoc async APIs until we have a
>>>       proper one?
>>
>> Yes.  So let's get serious about getting a proper interface in :-)
>
> ACK
>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>

Re: [Qemu-devel] [libvirt] QMP: Supporting off tree APIs

[Luiz Capitulino]

On Tue, 10 Jan 2012 15:02:48 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 01/10/2012 02:55 PM, Luiz Capitulino wrote:
> > On Tue, 10 Jan 2012 13:18:41 -0600
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >> On 01/06/2012 01:42 PM, Luiz Capitulino wrote:
> >>> On Fri, 06 Jan 2012 09:08:19 -0600
> >>>> We also need to look at this interface as a public interface whether we
> >>>> technically committed it to or not.  The fact is, an important user is relying
> >>>> upon so that makes it a supported interface.  Even though I absolutely hate it,
> >>>> this is why we haven't changed the help output even after all of these years.
> >>>> Not breaking users should be one of our highest priorities.
> >>>
> >>> One thing I don't understand: how is libvirt relying on it if it doesn't
> >>> exist in qemu.git yet?
> >>
> >> Because there was a discussion on qemu-devel and we agreed on an interface that
> >> both sides would implement to.
> >>
> >> I expect this to happen more often in the future too.
> >
> > For future commands we either, implement it right away or ask libvirt to
> > wait for the command to be merged, or at least pass initial review.
> 
> Or commit the schema entry to qapi-schema.json with gen=False.
> 
> But when this command was first proposed, qapi-schema.json didn't exist in the 
> tree :-)
> 
> >> But aren't we going to introduce a proper async interface?  This is what has me
> >> confused.
> >
> > Yes, I was thinking about new block commands using this API before we get
> > proper async support...
> 
> Well let's avoid that problem by doing proper async support before we get new 
> block commands ;-)
> 
> >>> There's more, because I skipped this review in v3 as I jumped to the
> >>> "proper async support" discussion...
> >>
> >> Well let's do proper async support.  As I mentioned, I'd rather take this
> >> command in as-is, introduce proper async support, and then deprecate a bunch of
> >> stuff at once.
> >
> > Ok, I'm willing to do this as Stefan has agreed on deprecating this as soon as
> > we get proper async support.
> 
> Excellent.
> 
> BTW, you mentioned that you were going to send an RFC for proper async support?

It's just a few proposals for the high-level API (ie. no patches), I can send it
tomorrow.

> 
> Regards,
> 
> Anthony Liguori
> 
> >>
> >>>> What I'd suggest is that we take the command in as-is and we mark it:
> >>>>
> >>>> Since: 1.1
> >>>> Deprecated: 1.2
> >>>> See Also: TBD
> >>>>
> >>>> The idea being that we'll introduce new generic async commands in 1.2 and
> >>>> deprecate this command.  We can figure out the removal schedule then too.  Since
> >>>> this command hasn't been around all that long, we can probably have a short
> >>>> removal schedule.
> >>>
> >>> That makes its inclusion even discussable :) A few (very honest) questions:
> >>>
> >>>    1. Is it really worth it to have the command for one or two releases?
> >>
> >> Yes.  The most important consideration IMHO is parallelizing development.  We
> >> want the block layer to evolve to the greatest extent possible independent of
> >> our other subsystems.  If we don't have the right infrastructure in QMP to
> >> support a block feature, that shouldn't hold up progress in the block layer to
> >> the greatest extent possible.
> >>
> >>>    2. Will we allow other block commands to use this async API?
> >>
> >> Depends on how long it takes to do "proper async support".
> >>
> >>>    3. Are we going to accept other ad-hoc async APIs until we have a
> >>>       proper one?
> >>
> >> Yes.  So let's get serious about getting a proper interface in :-)
> >
> > ACK
> >
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> >>
> >
>