[Qemu-block] [PATCH v7 01/10] block: Introduce API for copy offloading

[Fam Zheng]

Introduce the bdrv_co_copy_range() API for copy offloading.  Block
drivers implementing this API support efficient copy operations that
avoid reading each block from the source device and writing it to the
destination devices.  Examples of copy offload primitives are SCSI
EXTENDED COPY and Linux copy_file_range(2).

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/io.c| 97 +++
 include/block/block.h | 32 
 include/block/block_int.h | 38 +++
 3 files changed, 167 insertions(+)

diff --git a/block/io.c b/block/io.c
index ca96b487eb..b7beaeeb9f 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2835,3 +2835,100 @@ void bdrv_unregister_buf(BlockDriverState *bs, void 
*host)
 bdrv_unregister_buf(child->bs, host);
 }
 }
+
+static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src,
+uint64_t src_offset,
+BdrvChild *dst,
+uint64_t dst_offset,
+uint64_t bytes,
+BdrvRequestFlags flags,
+bool recurse_src)
+{
+int ret;
+
+if (!src || !dst || !src->bs || !dst->bs) {
+return -ENOMEDIUM;
+}
+ret = bdrv_check_byte_request(src->bs, src_offset, bytes);
+if (ret) {
+return ret;
+}
+
+ret = bdrv_check_byte_request(dst->bs, dst_offset, bytes);
+if (ret) {
+return ret;
+}
+if (flags & BDRV_REQ_ZERO_WRITE) {
+return bdrv_co_pwrite_zeroes(dst, dst_offset, bytes, flags);
+}
+
+if (!src->bs->drv->bdrv_co_copy_range_from
+|| !dst->bs->drv->bdrv_co_copy_range_to
+|| src->bs->encrypted || dst->bs->encrypted) {
+return -ENOTSUP;
+}
+if (recurse_src) {
+return src->bs->drv->bdrv_co_copy_range_from(src->bs,
+ src, src_offset,
+ dst, dst_offset,
+ bytes, flags);
+} else {
+return dst->bs->drv->bdrv_co_copy_range_to(dst->bs,
+   src, src_offset,
+   dst, dst_offset,
+   bytes, flags);
+}
+}
+
+/* Copy range from @src to @dst.
+ *
+ * See the comment of bdrv_co_copy_range for the parameter and return value
+ * semantics. */
+int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
+ BdrvChild *dst, uint64_t dst_offset,
+ uint64_t bytes, BdrvRequestFlags 
flags)
+{
+return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
+   bytes, flags, true);
+}
+
+/* Copy range from @src to @dst.
+ *
+ * See the comment of bdrv_co_copy_range for the parameter and return value
+ * semantics. */
+int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
+   BdrvChild *dst, uint64_t dst_offset,
+   uint64_t bytes, BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
+   bytes, flags, false);
+}
+
+int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
+BdrvChild *dst, uint64_t dst_offset,
+uint64_t bytes, BdrvRequestFlags flags)
+{
+BdrvTrackedRequest src_req, dst_req;
+BlockDriverState *src_bs = src->bs;
+BlockDriverState *dst_bs = dst->bs;
+int ret;
+
+bdrv_inc_in_flight(src_bs);
+bdrv_inc_in_flight(dst_bs);
+tracked_request_begin(_req, src_bs, src_offset,
+  bytes, BDRV_TRACKED_READ);
+tracked_request_begin(_req, dst_bs, dst_offset,
+  bytes, BDRV_TRACKED_WRITE);
+
+wait_serialising_requests(_req);
+wait_serialising_requests(_req);
+ret = bdrv_co_copy_range_from(src, src_offset,
+  dst, dst_offset,
+  bytes, flags);
+
+tracked_request_end(_req);
+tracked_request_end(_req);
+bdrv_dec_in_flight(src_bs);
+bdrv_dec_in_flight(dst_bs);
+return ret;
+}
diff --git a/include/block/block.h b/include/block/block.h
index 3894edda9d..6cc6c7e699 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -611,4 +611,36 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, 
const char *name,
  */
 void bdrv_register_buf(BlockDriverState *bs, void *host, size_t size);
 void bdrv_unregister_buf(BlockDriverState 

[Qemu-block] [PATCH v7 02/10] raw: Check byte range uniformly

[Fam Zheng]

We don't verify the request range against s->size in the I/O callbacks
except for raw_co_pwritev. This is inconsistent (especially for
raw_co_pwrite_zeroes and raw_co_pdiscard), so fix them, in the meanwhile
make the helper reusable by the coming new callbacks.

Note that in most cases the block layer already verifies the request
byte range against our reported image length, before invoking the driver
callbacks.  The exception is during image creating, after
blk_set_allow_write_beyond_eof(blk, true) is called. But in that case,
the requests are not directly from the user or guest. So there is no
visible behavior change in adding the check code.

The int64_t -> uint64_t inconsistency, as shown by the type casting, is
pre-existing due to the interface.

Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 block/raw-format.c | 64 +-
 1 file changed, 39 insertions(+), 25 deletions(-)

diff --git a/block/raw-format.c b/block/raw-format.c
index fe33693a2d..b69a0674b3 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -167,16 +167,37 @@ static void raw_reopen_abort(BDRVReopenState *state)
 state->opaque = NULL;
 }
 
+/* Check and adjust the offset, against 'offset' and 'size' options. */
+static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
+uint64_t bytes, bool is_write)
+{
+BDRVRawState *s = bs->opaque;
+
+if (s->has_size && (*offset > s->size || bytes > (s->size - *offset))) {
+/* There's not enough space for the write, or the read request is
+ * out-of-range. Don't read/write anything to prevent leaking out of
+ * the size specified in options. */
+return is_write ? -ENOSPC : -EINVAL;;
+}
+
+if (*offset > INT64_MAX - s->offset) {
+return -EINVAL;
+}
+*offset += s->offset;
+
+return 0;
+}
+
 static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
   uint64_t bytes, QEMUIOVector *qiov,
   int flags)
 {
-BDRVRawState *s = bs->opaque;
+int ret;
 
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+ret = raw_adjust_offset(bs, , bytes, false);
+if (ret) {
+return ret;
 }
-offset += s->offset;
 
 BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
 return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
@@ -186,23 +207,11 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
 {
-BDRVRawState *s = bs->opaque;
 void *buf = NULL;
 BlockDriver *drv;
 QEMUIOVector local_qiov;
 int ret;
 
-if (s->has_size && (offset > s->size || bytes > (s->size - offset))) {
-/* There's not enough space for the data. Don't write anything and just
- * fail to prevent leaking out of the size specified in options. */
-return -ENOSPC;
-}
-
-if (offset > UINT64_MAX - s->offset) {
-ret = -EINVAL;
-goto fail;
-}
-
 if (bs->probed && offset < BLOCK_PROBE_BUF_SIZE && bytes) {
 /* Handling partial writes would be a pain - so we just
  * require that guests have 512-byte request alignment if
@@ -237,7 +246,10 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 qiov = _qiov;
 }
 
-offset += s->offset;
+ret = raw_adjust_offset(bs, , bytes, true);
+if (ret) {
+goto fail;
+}
 
 BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
 ret = bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
@@ -267,22 +279,24 @@ static int coroutine_fn 
raw_co_pwrite_zeroes(BlockDriverState *bs,
  int64_t offset, int bytes,
  BdrvRequestFlags flags)
 {
-BDRVRawState *s = bs->opaque;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+int ret;
+
+ret = raw_adjust_offset(bs, (uint64_t *), bytes, true);
+if (ret) {
+return ret;
 }
-offset += s->offset;
 return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
 }
 
 static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
 int64_t offset, int bytes)
 {
-BDRVRawState *s = bs->opaque;
-if (offset > UINT64_MAX - s->offset) {
-return -EINVAL;
+int ret;
+
+ret = raw_adjust_offset(bs, (uint64_t *), bytes, true);
+if (ret) {
+return ret;
 }
-offset += s->offset;
 return bdrv_co_pdiscard(bs->file->bs, offset, bytes);
 }
 
-- 
2.14.3

[Qemu-block] [PATCH v7 00/10] qemu-img convert with copy offloading

[Fam Zheng]

v7: Fix qcow2.

v6: Pick up rev-by from Stefan and Eric.
Tweak patch 2 commit message.

v5: - Fix raw offset/bytes check for read. [Eric]
- Fix qcow2_handle_l2meta. [Stefan]
- Add coroutine_fn whereever appropriate. [Stefan]

v4: - Fix raw offset and size. [Eric]
- iscsi: Drop unnecessary return values and variables in favor of
  constants. [Stefan]
- qcow2: Handle small backing case. [Stefan]
- file-posix: Translate ENOSYS to ENOTSUP. [Stefan]
- API documentation and commit message. [Stefan]
- Add rev-by to patches 3, 5 - 10. [Stefan, Eric]

This series introduces block layer API for copy offloading and makes use of it
in qemu-img convert.

For now we implemented the operation in local file protocol with
copy_file_range(2).  Besides that it's possible to add similar to iscsi, nfs
and potentially more.

As far as its usage goes, in addition to qemu-img convert, we can emulate
offloading in scsi-disk (handle EXTENDED COPY command), and use the API in
block jobs too.

Fam Zheng (10):
  block: Introduce API for copy offloading
  raw: Check byte range uniformly
  raw: Implement copy offloading
  qcow2: Implement copy offloading
  file-posix: Implement bdrv_co_copy_range
  iscsi: Query and save device designator when opening
  iscsi: Create and use iscsi_co_wait_for_task
  iscsi: Implement copy offloading
  block-backend: Add blk_co_copy_range
  qemu-img: Convert with copy offloading

 block/block-backend.c  |  18 +++
 block/file-posix.c |  96 -
 block/io.c |  97 +
 block/iscsi.c  | 314 -
 block/qcow2.c  | 229 ++
 block/raw-format.c |  96 +
 include/block/block.h  |  32 +
 include/block/block_int.h  |  38 +
 include/block/raw-aio.h|  10 +-
 include/scsi/constants.h   |   5 +
 include/sysemu/block-backend.h |   4 +
 qemu-img.c |  50 ++-
 12 files changed, 890 insertions(+), 99 deletions(-)

-- 
2.14.3

[Qemu-block] [PATCH v7 06/10] iscsi: Query and save device designator when opening

[Fam Zheng]

The device designator data returned in INQUIRY command will be useful to
fill in source/target fields during copy offloading. Do this when
connecting to the target and save the data for later use.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/iscsi.c| 41 +
 include/scsi/constants.h |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 3fd7203916..6d0035d4b9 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -68,6 +68,7 @@ typedef struct IscsiLun {
 QemuMutex mutex;
 struct scsi_inquiry_logical_block_provisioning lbp;
 struct scsi_inquiry_block_limits bl;
+struct scsi_inquiry_device_designator *dd;
 unsigned char *zeroblock;
 /* The allocmap tracks which clusters (pages) on the iSCSI target are
  * allocated and which are not. In case a target returns zeros for
@@ -1740,6 +1741,30 @@ static QemuOptsList runtime_opts = {
 },
 };
 
+static void iscsi_save_designator(IscsiLun *lun,
+  struct scsi_inquiry_device_identification 
*inq_di)
+{
+struct scsi_inquiry_device_designator *desig, *copy = NULL;
+
+for (desig = inq_di->designators; desig; desig = desig->next) {
+if (desig->association ||
+desig->designator_type > SCSI_DESIGNATOR_TYPE_NAA) {
+continue;
+}
+/* NAA works better than T10 vendor ID based designator. */
+if (!copy || copy->designator_type < desig->designator_type) {
+copy = desig;
+}
+}
+if (copy) {
+lun->dd = g_new(struct scsi_inquiry_device_designator, 1);
+*lun->dd = *copy;
+lun->dd->next = NULL;
+lun->dd->designator = g_malloc(copy->designator_length);
+memcpy(lun->dd->designator, copy->designator, copy->designator_length);
+}
+}
+
 static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -1922,6 +1947,7 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 struct scsi_task *inq_task;
 struct scsi_inquiry_logical_block_provisioning *inq_lbp;
 struct scsi_inquiry_block_limits *inq_bl;
+struct scsi_inquiry_device_identification *inq_di;
 switch (inq_vpd->pages[i]) {
 case SCSI_INQUIRY_PAGECODE_LOGICAL_BLOCK_PROVISIONING:
 inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
@@ -1947,6 +1973,17 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
sizeof(struct scsi_inquiry_block_limits));
 scsi_free_scsi_task(inq_task);
 break;
+case SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION:
+inq_task = iscsi_do_inquiry(iscsilun->iscsi, iscsilun->lun, 1,
+
SCSI_INQUIRY_PAGECODE_DEVICE_IDENTIFICATION,
+(void **) _di, errp);
+if (inq_task == NULL) {
+ret = -EINVAL;
+goto out;
+}
+iscsi_save_designator(iscsilun, inq_di);
+scsi_free_scsi_task(inq_task);
+break;
 default:
 break;
 }
@@ -2003,6 +2040,10 @@ static void iscsi_close(BlockDriverState *bs)
 iscsi_logout_sync(iscsi);
 }
 iscsi_destroy_context(iscsi);
+if (iscsilun->dd) {
+g_free(iscsilun->dd->designator);
+g_free(iscsilun->dd);
+}
 g_free(iscsilun->zeroblock);
 iscsi_allocmap_free(iscsilun);
 qemu_mutex_destroy(>mutex);
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index a141dd71f8..54733b7110 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -311,4 +311,6 @@
 #define MMC_PROFILE_HDDVD_RW_DL 0x005A
 #define MMC_PROFILE_INVALID 0x
 
+#define IDENT_DESCR_TGT_DESCR 0xE4
+
 #endif
-- 
2.14.3

[Qemu-block] [PATCH v7 07/10] iscsi: Create and use iscsi_co_wait_for_task

[Fam Zheng]

This loop is repeated a growing number times. Make a helper.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Eric Blake 
---
 block/iscsi.c | 54 +-
 1 file changed, 17 insertions(+), 37 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6d0035d4b9..6a365cb07b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -556,6 +556,17 @@ static inline bool iscsi_allocmap_is_valid(IscsiLun 
*iscsilun,
offset / iscsilun->cluster_size) == size);
 }
 
+static void coroutine_fn iscsi_co_wait_for_task(IscsiTask *iTask,
+IscsiLun *iscsilun)
+{
+while (!iTask->complete) {
+iscsi_set_events(iscsilun);
+qemu_mutex_unlock(>mutex);
+qemu_coroutine_yield();
+qemu_mutex_lock(>mutex);
+}
+}
+
 static int coroutine_fn
 iscsi_co_writev(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 QEMUIOVector *iov, int flags)
@@ -617,12 +628,7 @@ retry:
 scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
   iov->niov);
 #endif
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(>mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(>mutex);
-}
+iscsi_co_wait_for_task(, iscsilun);
 
 if (iTask.task != NULL) {
 scsi_free_scsi_task(iTask.task);
@@ -693,13 +699,7 @@ retry:
 ret = -ENOMEM;
 goto out_unlock;
 }
-
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(>mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(>mutex);
-}
+iscsi_co_wait_for_task(, iscsilun);
 
 if (iTask.do_retry) {
 if (iTask.task != NULL) {
@@ -863,13 +863,8 @@ retry:
 #if LIBISCSI_API_VERSION < (20160603)
 scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov, 
iov->niov);
 #endif
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(>mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(>mutex);
-}
 
+iscsi_co_wait_for_task(, iscsilun);
 if (iTask.task != NULL) {
 scsi_free_scsi_task(iTask.task);
 iTask.task = NULL;
@@ -906,12 +901,7 @@ retry:
 return -ENOMEM;
 }
 
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(>mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(>mutex);
-}
+iscsi_co_wait_for_task(, iscsilun);
 
 if (iTask.task != NULL) {
 scsi_free_scsi_task(iTask.task);
@@ -1143,12 +1133,7 @@ retry:
 goto out_unlock;
 }
 
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(>mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(>mutex);
-}
+iscsi_co_wait_for_task(, iscsilun);
 
 if (iTask.task != NULL) {
 scsi_free_scsi_task(iTask.task);
@@ -1244,12 +1229,7 @@ retry:
 return -ENOMEM;
 }
 
-while (!iTask.complete) {
-iscsi_set_events(iscsilun);
-qemu_mutex_unlock(>mutex);
-qemu_coroutine_yield();
-qemu_mutex_lock(>mutex);
-}
+iscsi_co_wait_for_task(, iscsilun);
 
 if (iTask.status == SCSI_STATUS_CHECK_CONDITION &&
 iTask.task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
-- 
2.14.3

[Qemu-block] [PATCH v7 05/10] file-posix: Implement bdrv_co_copy_range

[Fam Zheng]

With copy_file_range(2), we can implement the bdrv_co_copy_range
semantics.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/file-posix.c  | 96 +++--
 include/block/raw-aio.h | 10 --
 2 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 5a602cfe37..550a201750 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -59,6 +59,7 @@
 #ifdef __linux__
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -187,6 +188,8 @@ typedef struct RawPosixAIOData {
 #define aio_ioctl_cmd   aio_nbytes /* for QEMU_AIO_IOCTL */
 off_t aio_offset;
 int aio_type;
+int aio_fd2;
+off_t aio_offset2;
 } RawPosixAIOData;
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
@@ -1446,6 +1449,47 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData 
*aiocb)
 return -ENOTSUP;
 }
 
+static off_t copy_file_range(int in_fd, off_t *in_off, int out_fd,
+ off_t *out_off, size_t len, unsigned int flags)
+{
+#ifdef __NR_copy_file_range
+return syscall(__NR_copy_file_range, in_fd, in_off, out_fd,
+   out_off, len, flags);
+#else
+errno = ENOSYS;
+return -1;
+#endif
+}
+
+static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
+{
+uint64_t bytes = aiocb->aio_nbytes;
+off_t in_off = aiocb->aio_offset;
+off_t out_off = aiocb->aio_offset2;
+
+while (bytes) {
+ssize_t ret = copy_file_range(aiocb->aio_fildes, _off,
+  aiocb->aio_fd2, _off,
+  bytes, 0);
+if (ret == -EINTR) {
+continue;
+}
+if (ret < 0) {
+if (errno == ENOSYS) {
+return -ENOTSUP;
+} else {
+return -errno;
+}
+}
+if (!ret) {
+/* No progress (e.g. when beyond EOF), fall back to buffer I/O. */
+return -ENOTSUP;
+}
+bytes -= ret;
+}
+return 0;
+}
+
 static ssize_t handle_aiocb_discard(RawPosixAIOData *aiocb)
 {
 int ret = -EOPNOTSUPP;
@@ -1526,6 +1570,9 @@ static int aio_worker(void *arg)
 case QEMU_AIO_WRITE_ZEROES:
 ret = handle_aiocb_write_zeroes(aiocb);
 break;
+case QEMU_AIO_COPY_RANGE:
+ret = handle_aiocb_copy_range(aiocb);
+break;
 default:
 fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
 ret = -EINVAL;
@@ -1536,9 +1583,10 @@ static int aio_worker(void *arg)
 return ret;
 }
 
-static int paio_submit_co(BlockDriverState *bs, int fd,
-  int64_t offset, QEMUIOVector *qiov,
-  int bytes, int type)
+static int paio_submit_co_full(BlockDriverState *bs, int fd,
+   int64_t offset, int fd2, int64_t offset2,
+   QEMUIOVector *qiov,
+   int bytes, int type)
 {
 RawPosixAIOData *acb = g_new(RawPosixAIOData, 1);
 ThreadPool *pool;
@@ -1546,6 +1594,8 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
 acb->bs = bs;
 acb->aio_type = type;
 acb->aio_fildes = fd;
+acb->aio_fd2 = fd2;
+acb->aio_offset2 = offset2;
 
 acb->aio_nbytes = bytes;
 acb->aio_offset = offset;
@@ -1561,6 +1611,13 @@ static int paio_submit_co(BlockDriverState *bs, int fd,
 return thread_pool_submit_co(pool, aio_worker, acb);
 }
 
+static inline int paio_submit_co(BlockDriverState *bs, int fd,
+ int64_t offset, QEMUIOVector *qiov,
+ int bytes, int type)
+{
+return paio_submit_co_full(bs, fd, offset, -1, 0, qiov, bytes, type);
+}
+
 static BlockAIOCB *paio_submit(BlockDriverState *bs, int fd,
 int64_t offset, QEMUIOVector *qiov, int bytes,
 BlockCompletionFunc *cb, void *opaque, int type)
@@ -2451,6 +2508,35 @@ static void raw_abort_perm_update(BlockDriverState *bs)
 raw_handle_perm_lock(bs, RAW_PL_ABORT, 0, 0, NULL);
 }
 
+static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs,
+   BdrvChild *src, uint64_t 
src_offset,
+   BdrvChild *dst, uint64_t 
dst_offset,
+   uint64_t bytes, 
BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes, 
flags);
+}
+
+static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
+ BdrvChild *src, uint64_t 
src_offset,
+ BdrvChild *dst, uint64_t 
dst_offset,
+ uint64_t bytes, BdrvRequestFlags 
flags)
+{
+BDRVRawState *s = bs->opaque;
+BDRVRawState *src_s;
+
+assert(dst->bs == bs);
+if 

[Qemu-block] [PATCH v7 04/10] qcow2: Implement copy offloading

[Fam Zheng]

The two callbacks are implemented quite similarly to the read/write
functions: bdrv_co_copy_range_from maps for read and calls into bs->file
or bs->backing depending on the allocation status; bdrv_co_copy_range_to
maps for write and calls into bs->file.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Fam Zheng 
---
 block/qcow2.c | 229 ++
 1 file changed, 199 insertions(+), 30 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6d532470a8..8f89c4fe72 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1762,6 +1762,39 @@ static int coroutine_fn 
qcow2_co_block_status(BlockDriverState *bs,
 return status;
 }
 
+static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
+QCowL2Meta **pl2meta,
+bool link_l2)
+{
+int ret = 0;
+QCowL2Meta *l2meta = *pl2meta;
+
+while (l2meta != NULL) {
+QCowL2Meta *next;
+
+if (!ret && link_l2) {
+ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
+if (ret) {
+goto out;
+}
+}
+
+/* Take the request off the list of running requests */
+if (l2meta->nb_clusters != 0) {
+QLIST_REMOVE(l2meta, next_in_flight);
+}
+
+qemu_co_queue_restart_all(>dependent_requests);
+
+next = l2meta->next;
+g_free(l2meta);
+l2meta = next;
+}
+out:
+*pl2meta = l2meta;
+return ret;
+}
+
 static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 uint64_t bytes, QEMUIOVector *qiov,
 int flags)
@@ -2048,24 +2081,9 @@ static coroutine_fn int 
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
 }
 }
 
-while (l2meta != NULL) {
-QCowL2Meta *next;
-
-ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
-if (ret < 0) {
-goto fail;
-}
-
-/* Take the request off the list of running requests */
-if (l2meta->nb_clusters != 0) {
-QLIST_REMOVE(l2meta, next_in_flight);
-}
-
-qemu_co_queue_restart_all(>dependent_requests);
-
-next = l2meta->next;
-g_free(l2meta);
-l2meta = next;
+ret = qcow2_handle_l2meta(bs, , true);
+if (ret) {
+goto fail;
 }
 
 bytes -= cur_bytes;
@@ -2076,18 +2094,7 @@ static coroutine_fn int 
qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
 ret = 0;
 
 fail:
-while (l2meta != NULL) {
-QCowL2Meta *next;
-
-if (l2meta->nb_clusters != 0) {
-QLIST_REMOVE(l2meta, next_in_flight);
-}
-qemu_co_queue_restart_all(>dependent_requests);
-
-next = l2meta->next;
-g_free(l2meta);
-l2meta = next;
-}
+qcow2_handle_l2meta(bs, , false);
 
 qemu_co_mutex_unlock(>lock);
 
@@ -3274,6 +3281,166 @@ static coroutine_fn int 
qcow2_co_pdiscard(BlockDriverState *bs,
 return ret;
 }
 
+static int coroutine_fn
+qcow2_co_copy_range_from(BlockDriverState *bs,
+ BdrvChild *src, uint64_t src_offset,
+ BdrvChild *dst, uint64_t dst_offset,
+ uint64_t bytes, BdrvRequestFlags flags)
+{
+BDRVQcow2State *s = bs->opaque;
+int ret;
+unsigned int cur_bytes; /* number of bytes in current iteration */
+BdrvChild *child = NULL;
+BdrvRequestFlags cur_flags;
+
+assert(!bs->encrypted);
+qemu_co_mutex_lock(>lock);
+
+while (bytes != 0) {
+uint64_t copy_offset = 0;
+/* prepare next request */
+cur_bytes = MIN(bytes, INT_MAX);
+cur_flags = flags;
+
+ret = qcow2_get_cluster_offset(bs, src_offset, _bytes, 
_offset);
+if (ret < 0) {
+goto out;
+}
+
+switch (ret) {
+case QCOW2_CLUSTER_UNALLOCATED:
+if (bs->backing && bs->backing->bs) {
+int64_t backing_length = bdrv_getlength(bs->backing->bs);
+if (src_offset >= backing_length) {
+cur_flags |= BDRV_REQ_ZERO_WRITE;
+} else {
+child = bs->backing;
+cur_bytes = MIN(cur_bytes, backing_length - src_offset);
+copy_offset = src_offset;
+}
+} else {
+cur_flags |= BDRV_REQ_ZERO_WRITE;
+}
+break;
+
+case QCOW2_CLUSTER_ZERO_PLAIN:
+case QCOW2_CLUSTER_ZERO_ALLOC:
+cur_flags |= BDRV_REQ_ZERO_WRITE;
+break;
+
+case QCOW2_CLUSTER_COMPRESSED:
+ret = -ENOTSUP;
+goto out;
+break;
+
+case QCOW2_CLUSTER_NORMAL:
+child = bs->file;
+copy_offset += offset_into_cluster(s, 

[Qemu-block] [PATCH v7 03/10] raw: Implement copy offloading

[Fam Zheng]

Just pass down to ->file.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/raw-format.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/block/raw-format.c b/block/raw-format.c
index b69a0674b3..f2e468df6f 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -497,6 +497,36 @@ static int raw_probe_geometry(BlockDriverState *bs, 
HDGeometry *geo)
 return bdrv_probe_geometry(bs->file->bs, geo);
 }
 
+static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs,
+   BdrvChild *src, uint64_t 
src_offset,
+   BdrvChild *dst, uint64_t 
dst_offset,
+   uint64_t bytes, 
BdrvRequestFlags flags)
+{
+int ret;
+
+ret = raw_adjust_offset(bs, _offset, bytes, false);
+if (ret) {
+return ret;
+}
+return bdrv_co_copy_range_from(bs->file, src_offset, dst, dst_offset,
+   bytes, flags);
+}
+
+static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
+ BdrvChild *src, uint64_t 
src_offset,
+ BdrvChild *dst, uint64_t 
dst_offset,
+ uint64_t bytes, BdrvRequestFlags 
flags)
+{
+int ret;
+
+ret = raw_adjust_offset(bs, _offset, bytes, true);
+if (ret) {
+return ret;
+}
+return bdrv_co_copy_range_to(src, src_offset, bs->file, dst_offset, bytes,
+ flags);
+}
+
 BlockDriver bdrv_raw = {
 .format_name  = "raw",
 .instance_size= sizeof(BDRVRawState),
@@ -513,6 +543,8 @@ BlockDriver bdrv_raw = {
 .bdrv_co_pwrite_zeroes = _co_pwrite_zeroes,
 .bdrv_co_pdiscard = _co_pdiscard,
 .bdrv_co_block_status = _co_block_status,
+.bdrv_co_copy_range_from = _co_copy_range_from,
+.bdrv_co_copy_range_to  = _co_copy_range_to,
 .bdrv_truncate= _truncate,
 .bdrv_getlength   = _getlength,
 .has_variable_length  = true,
-- 
2.14.3

[Qemu-block] [PATCH v7 10/10] qemu-img: Convert with copy offloading

[Fam Zheng]

The new blk_co_copy_range interface offers a more efficient way in the
case of network based storage. Make use of it to allow faster convert
operation.

Since copy offloading cannot do zero detection ('-S') and compression
(-c), only try it when these options are not used.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 qemu-img.c | 50 --
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 976b437da0..75f1610aa0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1547,6 +1547,7 @@ typedef struct ImgConvertState {
 bool compressed;
 bool target_has_backing;
 bool wr_in_order;
+bool copy_range;
 int min_sparse;
 size_t cluster_sectors;
 size_t buf_sectors;
@@ -1740,6 +1741,37 @@ static int coroutine_fn convert_co_write(ImgConvertState 
*s, int64_t sector_num,
 return 0;
 }
 
+static int coroutine_fn convert_co_copy_range(ImgConvertState *s, int64_t 
sector_num,
+  int nb_sectors)
+{
+int n, ret;
+
+while (nb_sectors > 0) {
+BlockBackend *blk;
+int src_cur;
+int64_t bs_sectors, src_cur_offset;
+int64_t offset;
+
+convert_select_part(s, sector_num, _cur, _cur_offset);
+offset = (sector_num - src_cur_offset) << BDRV_SECTOR_BITS;
+blk = s->src[src_cur];
+bs_sectors = s->src_sectors[src_cur];
+
+n = MIN(nb_sectors, bs_sectors - (sector_num - src_cur_offset));
+
+ret = blk_co_copy_range(blk, offset, s->target,
+sector_num << BDRV_SECTOR_BITS,
+n << BDRV_SECTOR_BITS, 0);
+if (ret < 0) {
+return ret;
+}
+
+sector_num += n;
+nb_sectors -= n;
+}
+return 0;
+}
+
 static void coroutine_fn convert_co_do_copy(void *opaque)
 {
 ImgConvertState *s = opaque;
@@ -1762,6 +1794,7 @@ static void coroutine_fn convert_co_do_copy(void *opaque)
 int n;
 int64_t sector_num;
 enum ImgConvertBlockStatus status;
+bool copy_range;
 
 qemu_co_mutex_lock(>lock);
 if (s->ret != -EINPROGRESS || s->sector_num >= s->total_sectors) {
@@ -1791,7 +1824,9 @@ static void coroutine_fn convert_co_do_copy(void *opaque)
 s->allocated_sectors, 0);
 }
 
-if (status == BLK_DATA) {
+retry:
+copy_range = s->copy_range && s->status == BLK_DATA;
+if (status == BLK_DATA && !copy_range) {
 ret = convert_co_read(s, sector_num, n, buf);
 if (ret < 0) {
 error_report("error while reading sector %" PRId64
@@ -1813,7 +1848,15 @@ static void coroutine_fn convert_co_do_copy(void *opaque)
 }
 
 if (s->ret == -EINPROGRESS) {
-ret = convert_co_write(s, sector_num, n, buf, status);
+if (copy_range) {
+ret = convert_co_copy_range(s, sector_num, n);
+if (ret) {
+s->copy_range = false;
+goto retry;
+}
+} else {
+ret = convert_co_write(s, sector_num, n, buf, status);
+}
 if (ret < 0) {
 error_report("error while writing sector %" PRId64
  ": %s", sector_num, strerror(-ret));
@@ -1936,6 +1979,7 @@ static int img_convert(int argc, char **argv)
 ImgConvertState s = (ImgConvertState) {
 /* Need at least 4k of zeros for sparse detection */
 .min_sparse = 8,
+.copy_range = true,
 .buf_sectors= IO_BUF_SIZE / BDRV_SECTOR_SIZE,
 .wr_in_order= true,
 .num_coroutines = 8,
@@ -1976,6 +2020,7 @@ static int img_convert(int argc, char **argv)
 break;
 case 'c':
 s.compressed = true;
+s.copy_range = false;
 break;
 case 'o':
 if (!is_valid_option_list(optarg)) {
@@ -2017,6 +2062,7 @@ static int img_convert(int argc, char **argv)
 }
 
 s.min_sparse = sval / BDRV_SECTOR_SIZE;
+s.copy_range = false;
 break;
 }
 case 'p':
-- 
2.14.3

[Qemu-block] [PATCH v7 09/10] block-backend: Add blk_co_copy_range

[Fam Zheng]

It's a BlockBackend wrapper of the BDS interface.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/block-backend.c  | 18 ++
 include/sysemu/block-backend.h |  4 
 2 files changed, 22 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 89f47b00ea..d55c328736 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2211,3 +2211,21 @@ void blk_unregister_buf(BlockBackend *blk, void *host)
 {
 bdrv_unregister_buf(blk_bs(blk), host);
 }
+
+int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
+   BlockBackend *blk_out, int64_t off_out,
+   int bytes, BdrvRequestFlags flags)
+{
+int r;
+r = blk_check_byte_request(blk_in, off_in, bytes);
+if (r) {
+return r;
+}
+r = blk_check_byte_request(blk_out, off_out, bytes);
+if (r) {
+return r;
+}
+return bdrv_co_copy_range(blk_in->root, off_in,
+  blk_out->root, off_out,
+  bytes, flags);
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 92ab624fac..8d03d493c2 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -232,4 +232,8 @@ void blk_set_force_allow_inactivate(BlockBackend *blk);
 void blk_register_buf(BlockBackend *blk, void *host, size_t size);
 void blk_unregister_buf(BlockBackend *blk, void *host);
 
+int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
+   BlockBackend *blk_out, int64_t off_out,
+   int bytes, BdrvRequestFlags flags);
+
 #endif
-- 
2.14.3

[Qemu-block] [PATCH v7 08/10] iscsi: Implement copy offloading

[Fam Zheng]

Issue EXTENDED COPY (LID1) command to implement the copy_range API.

The parameter data construction code is modified from libiscsi's
iscsi-dd.c.

Signed-off-by: Fam Zheng 
Reviewed-by: Stefan Hajnoczi 
---
 block/iscsi.c| 219 +++
 include/scsi/constants.h |   3 +
 2 files changed, 222 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a365cb07b..5ea75646d9 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2205,6 +2205,221 @@ static void coroutine_fn 
iscsi_co_invalidate_cache(BlockDriverState *bs,
 iscsi_allocmap_invalidate(iscsilun);
 }
 
+static int coroutine_fn iscsi_co_copy_range_from(BlockDriverState *bs,
+ BdrvChild *src,
+ uint64_t src_offset,
+ BdrvChild *dst,
+ uint64_t dst_offset,
+ uint64_t bytes,
+ BdrvRequestFlags flags)
+{
+return bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes, 
flags);
+}
+
+static struct scsi_task *iscsi_xcopy_task(int param_len)
+{
+struct scsi_task *task;
+
+task = g_new0(struct scsi_task, 1);
+
+task->cdb[0] = EXTENDED_COPY;
+task->cdb[10]= (param_len >> 24) & 0xFF;
+task->cdb[11]= (param_len >> 16) & 0xFF;
+task->cdb[12]= (param_len >> 8) & 0xFF;
+task->cdb[13]= param_len & 0xFF;
+task->cdb_size   = 16;
+task->xfer_dir   = SCSI_XFER_WRITE;
+task->expxferlen = param_len;
+
+return task;
+}
+
+static void iscsi_populate_target_desc(unsigned char *desc, IscsiLun *lun)
+{
+struct scsi_inquiry_device_designator *dd = lun->dd;
+
+memset(desc, 0, 32);
+desc[0] = IDENT_DESCR_TGT_DESCR;
+desc[4] = dd->code_set;
+desc[5] = (dd->designator_type & 0xF)
+| ((dd->association & 3) << 4);
+desc[7] = dd->designator_length;
+memcpy(desc + 8, dd->designator, dd->designator_length);
+
+desc[28] = 0;
+desc[29] = (lun->block_size >> 16) & 0xFF;
+desc[30] = (lun->block_size >> 8) & 0xFF;
+desc[31] = lun->block_size & 0xFF;
+}
+
+static void iscsi_xcopy_desc_hdr(uint8_t *hdr, int dc, int cat, int src_index,
+ int dst_index)
+{
+hdr[0] = 0x02; /* BLK_TO_BLK_SEG_DESCR */
+hdr[1] = ((dc << 1) | cat) & 0xFF;
+hdr[2] = (XCOPY_BLK2BLK_SEG_DESC_SIZE >> 8) & 0xFF;
+/* don't account for the first 4 bytes in descriptor header*/
+hdr[3] = (XCOPY_BLK2BLK_SEG_DESC_SIZE - 4 /* SEG_DESC_SRC_INDEX_OFFSET */) 
& 0xFF;
+hdr[4] = (src_index >> 8) & 0xFF;
+hdr[5] = src_index & 0xFF;
+hdr[6] = (dst_index >> 8) & 0xFF;
+hdr[7] = dst_index & 0xFF;
+}
+
+static void iscsi_xcopy_populate_desc(uint8_t *desc, int dc, int cat,
+  int src_index, int dst_index, int 
num_blks,
+  uint64_t src_lba, uint64_t dst_lba)
+{
+iscsi_xcopy_desc_hdr(desc, dc, cat, src_index, dst_index);
+
+/* The caller should verify the request size */
+assert(num_blks < 65536);
+desc[10] = (num_blks >> 8) & 0xFF;
+desc[11] = num_blks & 0xFF;
+desc[12] = (src_lba >> 56) & 0xFF;
+desc[13] = (src_lba >> 48) & 0xFF;
+desc[14] = (src_lba >> 40) & 0xFF;
+desc[15] = (src_lba >> 32) & 0xFF;
+desc[16] = (src_lba >> 24) & 0xFF;
+desc[17] = (src_lba >> 16) & 0xFF;
+desc[18] = (src_lba >> 8) & 0xFF;
+desc[19] = src_lba & 0xFF;
+desc[20] = (dst_lba >> 56) & 0xFF;
+desc[21] = (dst_lba >> 48) & 0xFF;
+desc[22] = (dst_lba >> 40) & 0xFF;
+desc[23] = (dst_lba >> 32) & 0xFF;
+desc[24] = (dst_lba >> 24) & 0xFF;
+desc[25] = (dst_lba >> 16) & 0xFF;
+desc[26] = (dst_lba >> 8) & 0xFF;
+desc[27] = dst_lba & 0xFF;
+}
+
+static void iscsi_xcopy_populate_header(unsigned char *buf, int list_id, int 
str,
+int list_id_usage, int prio,
+int tgt_desc_len,
+int seg_desc_len, int inline_data_len)
+{
+buf[0] = list_id;
+buf[1] = ((str & 1) << 5) | ((list_id_usage & 3) << 3) | (prio & 7);
+buf[2] = (tgt_desc_len >> 8) & 0xFF;
+buf[3] = tgt_desc_len & 0xFF;
+buf[8] = (seg_desc_len >> 24) & 0xFF;
+buf[9] = (seg_desc_len >> 16) & 0xFF;
+buf[10] = (seg_desc_len >> 8) & 0xFF;
+buf[11] = seg_desc_len & 0xFF;
+buf[12] = (inline_data_len >> 24) & 0xFF;
+buf[13] = (inline_data_len >> 16) & 0xFF;
+buf[14] = (inline_data_len >> 8) & 0xFF;
+buf[15] = inline_data_len & 0xFF;
+}
+
+static void iscsi_xcopy_data(struct iscsi_data *data,
+ IscsiLun *src, int64_t src_lba,
+ IscsiLun *dst, int64_t dst_lba,
+ uint16_t num_blocks)
+{
+uint8_t 

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Kevin Wolf]

Am 28.05.2018 um 23:25 hat Richard W.M. Jones geschrieben:
> On Mon, May 28, 2018 at 10:20:54PM +0100, Richard W.M. Jones wrote:
> > On Mon, May 28, 2018 at 08:38:33PM +0200, Kevin Wolf wrote:
> > > Just accessing the image file within a tar archive is possible and we
> > > could write a block driver for that (I actually think we should do
> > > this), but it restricts you because certain operations like resizing
> > > aren't really possible in tar. Unfortunately, resizing is a really
> > > common operation for non-raw image formats.
> > 
> > We do this already in virt-v2v (using file.offset and file.size
> > parameters in the raw driver).
> > 
> > For virt-v2v we only need to read the source so resizing isn't an
> > issue.  For most of the cases we're talking about the downloaded image
> > would also be a template / base image, so I suppose only reading would
> > be required too.
> > 
> > I also wrote an nbdkit tar file driver (supports writes, but not
> > resizing).
> > https://manpages.debian.org/testing/nbdkit-plugin-perl/nbdkit-tar-plugin.1.en.html
> 
> I should add the other thorny issue with OVA files is that the
> metadata contains a checksum (SHA1 or SHA256) of the disk images.  If
> you modify the disk images in-place in the tar file then you need to
> recalculate those.

All of this means that OVA isn't really well suited to be used as a
native format for VM configuration + images. It's just for sharing
read-only images that are converted into another native format before
they are used.

Which is probably fair for the use case it was made for, but means that
we need something else to solve our problem.

Kevin

Re: [Qemu-block] [PATCH 09/14] qemu-iotests: Rewrite 207 for blockdev-create job

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:22PM +0200, Kevin Wolf wrote:
> This rewrites the test case 207 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> Most of the test cases stay the same as before (the exception being some
> improved 'size' options that allow distinguishing which command created
> the image), but in order to be able to implement proper job handling,
> the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/207| 435 
> +++---
>  tests/qemu-iotests/207.out|  89 +
>  tests/qemu-iotests/group  |   6 +-
>  tests/qemu-iotests/iotests.py |  23 ++-
>  4 files changed, 264 insertions(+), 289 deletions(-)
> 
> diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
> index f5c77852d1..91c1f7e811 100755
> --- a/tests/qemu-iotests/207
> +++ b/tests/qemu-iotests/207
> @@ -1,9 +1,11 @@
> -#!/bin/bash
> +#!/usr/bin/env python
>  #
>  # Test ssh image creation
>  #
>  # Copyright (C) 2018 Red Hat, Inc.
>  #
> +# Creator/Owner: Kevin Wolf 
> +#
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
>  # the Free Software Foundation; either version 2 of the License, or
> @@ -18,244 +20,193 @@
>  # along with this program.  If not, see .
>  #
>  
> -# creator
> -owner=kw...@redhat.com
> -
> -seq=`basename $0`
> -echo "QA output created by $seq"
> -
> -here=`pwd`
> -status=1 # failure is the default!
> -
> -# get standard environment, filters and checks
> -. ./common.rc
> -. ./common.filter
> -
> -_supported_fmt raw
> -_supported_proto ssh
> -_supported_os Linux
> -
> -function do_run_qemu()
> -{
> -echo Testing: "$@"
> -$QEMU -nographic -qmp stdio -serial none "$@"
> -echo
> -}
> -
> -function run_qemu()
> -{
> -do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
> -  | _filter_qemu | _filter_imgfmt \
> -  | _filter_actual_image_size
> -}
> -
> -echo
> -echo "=== Successful image creation (defaults) ==="
> -echo
> -
> -run_qemu < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "ssh",
> -  "location": {
> -  "path": "$TEST_IMG_FILE",
> -  "server": {
> -  "host": "127.0.0.1",
> -  "port": "22"
> -  }
> -  },
> -  "size": 4194304
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info | _filter_img_info
> -echo
> -TEST_IMG=$TEST_IMG_FILE _img_info | _filter_img_info
> -
> -echo
> -echo "=== Test host-key-check options ==="
> -echo
> -
> -run_qemu < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "ssh",
> -  "location": {
> -  "path": "$TEST_IMG_FILE",
> -  "server": {
> -  "host": "127.0.0.1",
> -  "port": "22"
> -  },
> -  "host-key-check": {
> -  "mode": "none"
> -  }
> -  },
> -  "size": 8388608
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info | _filter_img_info
> -
> -run_qemu < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "ssh",
> -  "location": {
> -  "path": "$TEST_IMG_FILE",
> -  "server": {
> -  "host": "127.0.0.1",
> -  "port": "22"
> -  },
> -  "host-key-check": {
> -  "mode": "known_hosts"
> -  }
> -  },
> -  "size": 4194304
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info | _filter_img_info
> -
> -
> -key=$(ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" |
> -  cut -d" " -f3 | base64 -d | md5sum -b | cut -d" " -f1)
> -
> -run_qemu < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "ssh",
> -  "location": {
> -  "path": "$TEST_IMG_FILE",
> -  "server": {
> -  "host": "127.0.0.1",
> -  "port": "22"
> -  },
> -  "host-key-check": {
> -  "mode": "hash",
> -  "type": "md5",
> -  "hash": "wrong"
> -  }
> -  },
> -  "size": 8388608
> -  }
> -}
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "ssh",
> -  "location": {
> -  "path": "$TEST_IMG_FILE",
> -  "server": {
> -  "host": "127.0.0.1",
> -  "port": "22"
> -  },
> -  "host-key-check": {
> -  "mode": "hash",
> -  "type": "md5",
> -  "hash": "$key"
> -  }
> -  },
> -  "size": 8388608
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info | _filter_img_info
> -
> -
> -key=$(ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" |

[Qemu-block] [PATCH v2 10/20] block: Drain recursively with a single BDRV_POLL_WHILE()

[Kevin Wolf]

Anything can happen inside BDRV_POLL_WHILE(), including graph
changes that may interfere with its callers (e.g. child list iteration
in recursive callers of bdrv_do_drained_begin).

Switch to a single BDRV_POLL_WHILE() call for the whole subtree at the
end of bdrv_do_drained_begin() to avoid such effects. The recursion
happens now inside the loop condition. As the graph can only change
between bdrv_drain_poll() calls, but not inside of it, doing the
recursion here is safe.

Signed-off-by: Kevin Wolf 
---
 include/block/block.h |  9 +---
 block.c   |  2 +-
 block/io.c| 63 ---
 3 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 52f07da3d3..56962e7ee7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -568,10 +568,13 @@ void bdrv_parent_drained_end(BlockDriverState *bs, 
BdrvChild *ignore);
 /**
  * bdrv_drain_poll:
  *
- * Poll for pending requests in @bs and its parents (except for
- * @ignore_parent). This is part of bdrv_drained_begin.
+ * Poll for pending requests in @bs, its parents (except for @ignore_parent),
+ * and if @recursive is true its children as well.
+ *
+ * This is part of bdrv_drained_begin.
  */
-bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent);
+bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
+ BdrvChild *ignore_parent);
 
 /**
  * bdrv_drained_begin:
diff --git a/block.c b/block.c
index 3c654a8a63..8ca2cf27b4 100644
--- a/block.c
+++ b/block.c
@@ -823,7 +823,7 @@ static void bdrv_child_cb_drained_begin(BdrvChild *child)
 static bool bdrv_child_cb_drained_poll(BdrvChild *child)
 {
 BlockDriverState *bs = child->opaque;
-return bdrv_drain_poll(bs, NULL);
+return bdrv_drain_poll(bs, false, NULL);
 }
 
 static void bdrv_child_cb_drained_end(BdrvChild *child)
diff --git a/block/io.c b/block/io.c
index 8303aebb58..01784d1115 100644
--- a/block/io.c
+++ b/block/io.c
@@ -165,6 +165,7 @@ typedef struct {
 bool done;
 bool begin;
 bool recursive;
+bool poll;
 BdrvChild *parent;
 } BdrvCoDrainData;
 
@@ -200,27 +201,42 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool 
begin)
 }
 
 /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
-bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent)
+bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
+ BdrvChild *ignore_parent)
 {
+BdrvChild *child, *next;
+
 if (bdrv_parent_drained_poll(bs, ignore_parent)) {
 return true;
 }
 
-return atomic_read(>in_flight);
+if (atomic_read(>in_flight)) {
+return true;
+}
+
+if (recursive) {
+QLIST_FOREACH_SAFE(child, >children, next, next) {
+if (bdrv_drain_poll(child->bs, recursive, child)) {
+return true;
+}
+}
+}
+
+return false;
 }
 
-static bool bdrv_drain_poll_top_level(BlockDriverState *bs,
+static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
   BdrvChild *ignore_parent)
 {
 /* Execute pending BHs first and check everything else only after the BHs
  * have executed. */
 while (aio_poll(bs->aio_context, false));
 
-return bdrv_drain_poll(bs, ignore_parent);
+return bdrv_drain_poll(bs, recursive, ignore_parent);
 }
 
 static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
-  BdrvChild *parent);
+  BdrvChild *parent, bool poll);
 static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
 BdrvChild *parent);
 
@@ -232,7 +248,7 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 
 bdrv_dec_in_flight(bs);
 if (data->begin) {
-bdrv_do_drained_begin(bs, data->recursive, data->parent);
+bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
 } else {
 bdrv_do_drained_end(bs, data->recursive, data->parent);
 }
@@ -243,7 +259,7 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 
 static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
 bool begin, bool recursive,
-BdrvChild *parent)
+BdrvChild *parent, bool poll)
 {
 BdrvCoDrainData data;
 
@@ -258,6 +274,7 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs,
 .begin = begin,
 .recursive = recursive,
 .parent = parent,
+.poll = poll,
 };
 bdrv_inc_in_flight(bs);
 aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -270,12 +287,12 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs,
 }
 
 void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
-  

[Qemu-block] [PATCH 1/4] hw/block/fdc: Replace error_setg(_abort) by error_report() + abort()

[Philippe Mathieu-Daudé]

Use error_report() + abort() instead of error_setg(_abort),
as suggested by the "qapi/error.h" documentation:

Please don't error_setg(_fatal, ...), use error_report() and
exit(), because that's more obvious.
Likewise, don't error_setg(_abort, ...), use assert().

Use abort() instead of the suggested assert() because the error message
already got displayed.

Suggested-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/block/fdc.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cd29e27d8f..048467c00b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -401,9 +401,10 @@ static int pick_geometry(FDrive *drv)
 
 /* No match of any kind found -- fd_format is misconfigured, abort. */
 if (match == -1) {
-error_setg(_abort, "No candidate geometries present in table "
-   " for floppy drive type '%s'",
-   FloppyDriveType_str(drv->drive));
+error_report("No candidate geometries present in table "
+ " for floppy drive type '%s'",
+ FloppyDriveType_str(drv->drive));
+abort();
 }
 
 parse = &(fd_formats[match]);
-- 
2.17.0

[Qemu-block] [PATCH 0/4] qapi/error: converts error_setg(_fatal) to error_report() + exit()

[Philippe Mathieu-Daudé]

Hi,

This series converts error_setg(_fatal) to error_report() + exit() as
suggested by the "qapi/error.h" documentation.

This reduce Coverity and Clang static analyzer positive falses.

See http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07585.html:

On 07/24/2017 04:52 PM, Eric Blake wrote:
That's a shame.  Rather, we should patch this file (and others) to avoid
all the inconsistent uses of error_setg(_*), to comply with the
error.h documentation.

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/block/fdc: Replace error_setg(_abort) by error_report() + abort()
  hw/ppc/spapr_drc: Replace error_setg(_abort) by error_report() +
abort()
  hw/arm/sysbus-fdt: Replace error_setg(_fatal) by error_report()
+ exit()
  device_tree: Replace error_setg(_fatal) by error_report() + exit()

 device_tree.c   | 23 +--
 hw/arm/sysbus-fdt.c | 45 ++---
 hw/block/fdc.c  |  7 ---
 hw/ppc/spapr_drc.c  |  3 ++-
 4 files changed, 45 insertions(+), 33 deletions(-)

-- 
2.17.0

Re: [Qemu-block] [PATCH 14/14] block/create: Mark blockdev-create stable

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:27PM +0200, Kevin Wolf wrote:
> We're ready to declare the blockdev-create job stable. This renames the
> corresponding QMP command from x-blockdev-create to blockdev-create.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  qapi/block-core.json   |  4 ++--
>  qapi/job.json  |  2 +-
>  block/create.c |  4 ++--
>  tests/qemu-iotests/206 |  2 +-
>  tests/qemu-iotests/206.out | 54 
> +++---
>  tests/qemu-iotests/207 |  2 +-
>  tests/qemu-iotests/207.out | 18 
>  tests/qemu-iotests/210 |  2 +-
>  tests/qemu-iotests/210.out | 18 
>  tests/qemu-iotests/211 |  2 +-
>  tests/qemu-iotests/211.out | 24 ++---
>  tests/qemu-iotests/212 |  2 +-
>  tests/qemu-iotests/212.out | 42 ++--
>  tests/qemu-iotests/213 |  2 +-
>  tests/qemu-iotests/213.out | 44 ++---
>  15 files changed, 111 insertions(+), 111 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1ed3a82373..015e5ac12b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4011,7 +4011,7 @@
>} }
>  
>  ##
> -# @x-blockdev-create:
> +# @blockdev-create:
>  #
>  # Starts a job to create an image format on a given node. The job is
>  # automatically finalized, but a manual job-dismiss is required.
> @@ -4022,7 +4022,7 @@
>  #
>  # Since: 3.0
>  ##
> -{ 'command': 'x-blockdev-create',
> +{ 'command': 'blockdev-create',
>'data': { 'job-id': 'str',
>  'options': 'BlockdevCreateOptions' } }
>  
> diff --git a/qapi/job.json b/qapi/job.json
> index 69c1970a58..17d10037c4 100644
> --- a/qapi/job.json
> +++ b/qapi/job.json
> @@ -17,7 +17,7 @@
>  #
>  # @backup: drive backup job type, see "drive-backup"
>  #
> -# @create: image creation job type, see "x-blockdev-create" (since 3.0)
> +# @create: image creation job type, see "blockdev-create" (since 3.0)
>  #
>  # Since: 1.7
>  ##
> diff --git a/block/create.c b/block/create.c
> index 87fdab3b72..21728acca0 100644
> --- a/block/create.c
> +++ b/block/create.c
> @@ -61,8 +61,8 @@ static const JobDriver blockdev_create_job_driver = {
>  .start = blockdev_create_run,
>  };
>  
> -void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions 
> *options,
> -   Error **errp)
> +void qmp_blockdev_create(const char *job_id, BlockdevCreateOptions *options,
> + Error **errp)
>  {
>  BlockdevCreateJob *s;
>  const char *fmt = BlockdevDriver_str(options->driver);
> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> index 9a305302d1..334410d6a7 100755
> --- a/tests/qemu-iotests/206
> +++ b/tests/qemu-iotests/206
> @@ -26,7 +26,7 @@ from iotests import imgfmt
>  iotests.verify_image_format(supported_fmts=['qcow2'])
>  
>  def blockdev_create(vm, options):
> -result = vm.qmp_log('x-blockdev-create', job_id='job0', options=options)
> +result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
>  
>  if 'return' in result:
>  assert result['return'] == {}
> diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out
> index 45367270e8..8b403ea08f 100644
> --- a/tests/qemu-iotests/206.out
> +++ b/tests/qemu-iotests/206.out
> @@ -1,13 +1,13 @@
>  === Successful image creation (defaults) ===
>  
> -{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'size': 0, 'driver': 'file', 'filename': 'TEST_DIR/t.qcow2'}}}
> +{'execute': 'blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'size': 0, 'driver': 'file', 'filename': 'TEST_DIR/t.qcow2'}}}
>  {u'return': {}}
>  {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
>  {u'return': {}}
>  
>  {'execute': 'blockdev-add', 'arguments': {'node_name': 'imgfile', 'driver': 
> 'file', 'filename': 'TEST_DIR/t.qcow2'}}
>  {u'return': {}}
> -{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'driver': 'qcow2', 'file': 'imgfile', 'size': 134217728}}}
> +{'execute': 'blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'driver': 'qcow2', 'file': 'imgfile', 'size': 134217728}}}
>  {u'return': {}}
>  {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
>  {u'return': {}}
> @@ -24,12 +24,12 @@ Format specific information:
>  
>  === Successful image creation (inline blockdev-add, explicit defaults) ===
>  
> -{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'nocow': False, 'preallocation': 'off', 'size': 0, 'driver': 'file', 
> 'filename': 'TEST_DIR/t.qcow2'}}}
> +{'execute': 'blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'nocow': False, 'preallocation': 'off', 'size': 0, 'driver': 'file', 
> 'filename': 'TEST_DIR/t.qcow2'}}}
>  {u'return': {}}
>  {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
>  {u'return': {}}
>  
> -{'execute': 

Re: [Qemu-block] [PATCH 08/14] qemu-iotests: Rewrite 206 for blockdev-create job

[Kevin Wolf]

Am 29.05.2018 um 14:27 hat Max Reitz geschrieben:
> On 2018-05-25 18:33, Kevin Wolf wrote:
> > This rewrites the test case 206 to work with the new x-blockdev-create
> > job rather than the old synchronous version of the command.
> > 
> > All of the test cases stay the same as before, but in order to be able
> > to implement proper job handling, the test case is rewritten in Python.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  tests/qemu-iotests/206| 705 
> > +-
> >  tests/qemu-iotests/206.out| 241 +--
> >  tests/qemu-iotests/group  |   2 +-
> >  tests/qemu-iotests/iotests.py |  15 +
> >  4 files changed, 448 insertions(+), 515 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> > index 0a18b2b19a..9a305302d1 100755
> > --- a/tests/qemu-iotests/206
> > +++ b/tests/qemu-iotests/206
> 
> [...]
> 
> > +import iotests
> > +from iotests import imgfmt
> > +
> > +iotests.verify_image_format(supported_fmts=['qcow2'])
> > +
> > +def blockdev_create(vm, options):
> > +result = vm.qmp_log('x-blockdev-create', job_id='job0', 
> > options=options)
> > +
> > +if 'return' in result:
> > +assert result['return'] == {}
> > +vm.run_job('job0')
> > +iotests.log("")
> > +
> > +with iotests.FilePath('t.qcow2') as disk_path, \
> > + iotests.FilePath('t.qcow2.base') as backing_path, \
> > + iotests.VM() as vm:
> > +
> > +vm.add_object('secret,id=keysec0,data="foo"')
> 
> I don't know how subprocess.Popen() works, but are you sure you aren't
> encrypting with '"foo"' now?  (i.e. literally that key, including quotes)

That's correct. Anything wrong with it?

(Okay, you're right. I did fix it in 210, but forgot it here...)

> > +
> > +#
> > +# Successful image creation (defaults)
> > +#
> > +iotests.log("=== Successful image creation (defaults) ===")
> 
> OK, the comment makes sense for visual separation.  But so would leaving
> three empty lines.  *cough*

I tried vertical spacing first, but it didn't work well for me. What
made the difference is the syntax highlighting of comments vs. code.
YMMV.

> > +iotests.log("")
> > +
> > +size = 128 * 1024 * 1024
> > +
> > +vm.launch()
> > +blockdev_create(vm, { 'driver': 'file',
> > +  'filename': disk_path,
> > +  'size': 0 })
> > +
> > +vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> > +   node_name='imgfile')
> > +
> > +blockdev_create(vm, { 'driver': imgfmt,
> > +  'file': 'imgfile',
> > +  'size': size })
> > +vm.shutdown()
> > +
> > +iotests.img_info_log(disk_path)
> > +
> 
> [...]
> 
> > +#
> > +# Successful image creation (v2 non-default options)
> > +#
> > +iotests.log("=== Successful image creation (v2 non-default options) 
> > ===")
> > +iotests.log("")
> > +
> > +vm.launch()
> > +blockdev_create(vm, { 'driver': 'file',
> > +  'filename': disk_path,
> > +  'size': 0 })
> > +
> > +blockdev_create(vm, { 'driver': imgfmt,
> > +  'file': {
> > +  'driver': 'file',
> > +  'filename': disk_path,
> > +  },
> > +  'size': size,
> > +  'backing-file': backing_path,
> 
> Change from the bash version: backing_path does not exist here.  Not
> sure if that was intentional.

No, it was not. It's actually a good test case, though. Creating an
image at backing_path would be easy enough, but maybe we should just
leave it?

> > +  'backing-fmt': 'qcow2',
> > +  'version': 'v2',
> > +  'cluster-size': 512 })
> > +vm.shutdown()
> > +
> > +iotests.img_info_log(disk_path)
> 
> [...]
> 
> > +#
> > +# Invalid sizes
> > +#
> > +iotests.log("=== Invalid sizes ===")
> > +
> > +# TODO Negative image sizes aren't handled correctly, but this is a 
> > problem
> > +# with QAPI's implementation of the 'size' type and affects other 
> > commands
> > +# as well. Once this is fixed, we may want to add a test case here.
> > +#
> > +# 1. Misaligned image size
> > +# 2. 2^64 - 512
> > +# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
> > +# 4. 2^63 - 512 (generally valid, but qcow2 can't handle images this 
> > size)
> > +
> > +vm.add_blockdev('driver=file,filename=%s,node-name=node0' % 
> > (disk_path))
> > +
> > +vm.launch()
> > +blockdev_create(vm, { 'driver': imgfmt,
> > +  'file': 'node0',
> > +  'size': 1234  })
> > +blockdev_create(vm, { 'driver': imgfmt,
> > +  'file': 'node0',
> > +  'size': 18446744073709551104 })
> 

Re: [Qemu-block] [libvirt-users] VM I/O performance drops dramatically during storage migration with drive-mirror

[Eric Blake]

On 05/28/2018 07:05 AM, Kashyap Chamarthy wrote:

Cc the QEMU Block Layer mailing list (qemu-block@nongnu.org), who might
have more insights here; and wrap long lines.


...

170 to less than 10. I also show the figure of this experiment in the
attachment of this email.


[The attachment should arrive on the 'libvirt-users' list archives; but
it's not there yet --
https://www.redhat.com/archives/libvirt-users/2018-May/thread.html]


Actually, the attachment was probably rejected by list moderation for 
being oversized.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-block] [PATCH v2 14/20] block: Defer .bdrv_drain_begin callback to polling phase

[Kevin Wolf]

We cannot allow aio_poll() in bdrv_drain_invoke(begin=true) until we're
done with propagating the drain through the graph and are doing the
single final BDRV_POLL_WHILE().

Just schedule the coroutine with the callback and increase bs->in_flight
to make sure that the polling phase will wait for it.

Signed-off-by: Kevin Wolf 
---
 block/io.c | 28 +++-
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index af14227af8..a5b899723a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -182,22 +182,40 @@ static void coroutine_fn bdrv_drain_invoke_entry(void 
*opaque)
 
 /* Set data->done before reading bs->wakeup.  */
 atomic_mb_set(>done, true);
-bdrv_wakeup(bs);
+bdrv_dec_in_flight(bs);
+
+if (data->begin) {
+g_free(data);
+}
 }
 
 /* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */
 static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
-BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
+BdrvCoDrainData *data;
 
 if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
 (!begin && !bs->drv->bdrv_co_drain_end)) {
 return;
 }
 
-data.co = qemu_coroutine_create(bdrv_drain_invoke_entry, );
-bdrv_coroutine_enter(bs, data.co);
-BDRV_POLL_WHILE(bs, !data.done);
+data = g_new(BdrvCoDrainData, 1);
+*data = (BdrvCoDrainData) {
+.bs = bs,
+.done = false,
+.begin = begin
+};
+
+/* Make sure the driver callback completes during the polling phase for
+ * drain_begin. */
+bdrv_inc_in_flight(bs);
+data->co = qemu_coroutine_create(bdrv_drain_invoke_entry, data);
+aio_co_schedule(bdrv_get_aio_context(bs), data->co);
+
+if (!begin) {
+BDRV_POLL_WHILE(bs, !data->done);
+g_free(data);
+}
 }
 
 /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
-- 
2.13.6

Re: [Qemu-block] [PATCH v4] block: fix QEMU crash with scsi-hd and drive_del

[Kevin Wolf]

Am 28.05.2018 um 14:03 hat Greg Kurz geschrieben:
> Removing a drive with drive_del while it is being used to run an I/O
> intensive workload can cause QEMU to crash.
> 
> An AIO flush can yield at some point:
> 
> blk_aio_flush_entry()
>  blk_co_flush(blk)
>   bdrv_co_flush(blk->root->bs)
>...
> qemu_coroutine_yield()
> 
> and let the HMP command to run, free blk->root and give control
> back to the AIO flush:
> 
> hmp_drive_del()
>  blk_remove_bs()
>   bdrv_root_unref_child(blk->root)
>child_bs = blk->root->bs
>bdrv_detach_child(blk->root)
> bdrv_replace_child(blk->root, NULL)
>  blk->root->bs = NULL
> g_free(blk->root) <== blk->root becomes stale
>bdrv_unref(child_bs)
> bdrv_delete(child_bs)
>  bdrv_close()
>   bdrv_drained_begin()
>bdrv_do_drained_begin()
> bdrv_drain_recurse()
>  aio_poll()
>   ...
>   qemu_coroutine_switch()
> 
> and the AIO flush completion ends up dereferencing blk->root:
> 
>   blk_aio_complete()
>scsi_aio_complete()
> blk_get_aio_context(blk)
>  bs = blk_bs(blk)
>  ie, bs = blk->root ? blk->root->bs : NULL
> ^
> stale
> 
> The problem is that we should avoid making block driver graph
> changes while we have in-flight requests. Let's drain all I/O
> for this BB before calling bdrv_root_unref_child().
> 
> Signed-off-by: Greg Kurz 

Hmm... It sounded convincing, but 'make check-tests/test-replication'
fails now. The good news is that with the drain fixes, for which I sent
v2 today, it passes, so instead of staging it in my block branch, I'll
put it at the end of my branch for the drain fixes.

Might take a bit longer than planned until it's in master, sorry.

Kevin

[Qemu-block] [PATCH v2 17/20] block: Move bdrv_drain_all_begin() out of coroutine context

[Kevin Wolf]

Before we can introduce a single polling loop for all nodes in
bdrv_drain_all_begin(), we must make sure to run it outside of coroutine
context like we already do for bdrv_do_drained_begin().

Signed-off-by: Kevin Wolf 
---
 block/io.c | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/block/io.c b/block/io.c
index a5b899723a..2c06aaf6d8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -264,11 +264,16 @@ static void bdrv_co_drain_bh_cb(void *opaque)
 Coroutine *co = data->co;
 BlockDriverState *bs = data->bs;
 
-bdrv_dec_in_flight(bs);
-if (data->begin) {
-bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
+if (bs) {
+bdrv_dec_in_flight(bs);
+if (data->begin) {
+bdrv_do_drained_begin(bs, data->recursive, data->parent, 
data->poll);
+} else {
+bdrv_do_drained_end(bs, data->recursive, data->parent);
+}
 } else {
-bdrv_do_drained_end(bs, data->recursive, data->parent);
+assert(data->begin);
+bdrv_drain_all_begin();
 }
 
 data->done = true;
@@ -294,7 +299,9 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs,
 .parent = parent,
 .poll = poll,
 };
-bdrv_inc_in_flight(bs);
+if (bs) {
+bdrv_inc_in_flight(bs);
+}
 aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
 bdrv_co_drain_bh_cb, );
 
@@ -464,6 +471,11 @@ void bdrv_drain_all_begin(void)
 BlockDriverState *bs;
 BdrvNextIterator it;
 
+if (qemu_in_coroutine()) {
+bdrv_co_yield_to_drain(NULL, true, false, NULL, true);
+return;
+}
+
 /* BDRV_POLL_WHILE() for a node can only be called from its own I/O thread
  * or the main loop AioContext. We potentially use BDRV_POLL_WHILE() on
  * nodes in several different AioContexts, so make sure we're in the main
-- 
2.13.6

[Qemu-block] [PATCH v2 19/20] block: Allow graph changes in bdrv_drain_all_begin/end sections

[Kevin Wolf]

bdrv_drain_all_*() used bdrv_next() to iterate over all root nodes and
did a subtree drain for each of them. This works fine as long as the
graph is static, but sadly, reality looks different.

If the graph changes so that root nodes are added or removed, we would
have to compensate for this. bdrv_next() returns each root node only
once even if it's the root node for multiple BlockBackends or for a
monitor-owned block driver tree, which would only complicate things.

The much easier and more obviously correct way is to fundamentally
change the way the functions work: Iterate over all BlockDriverStates,
no matter who owns them, and drain them individually. Compensation is
only necessary when a new BDS is created inside a drain_all section.
Removal of a BDS doesn't require any action because it's gone afterwards
anyway.

Signed-off-by: Kevin Wolf 
---
 include/block/block.h |  1 +
 include/block/block_int.h |  1 +
 block.c   | 34 ---
 block/io.c| 60 ---
 4 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index a678556608..1ff13854d8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -419,6 +419,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
  Error **errp);
 bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next_node(BlockDriverState *bs);
+BlockDriverState *bdrv_next_all_states(BlockDriverState *bs);
 
 typedef struct BdrvNextIterator {
 enum {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index edb16df015..212694a0c2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -822,6 +822,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
 int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
 BdrvRequestFlags flags);
 
+extern unsigned int bdrv_drain_all_count;
 void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent);
 void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState 
*old_parent);
 
diff --git a/block.c b/block.c
index ce6eb53ad9..5c58a19646 100644
--- a/block.c
+++ b/block.c
@@ -332,6 +332,10 @@ BlockDriverState *bdrv_new(void)
 
 qemu_co_queue_init(>flush_queue);
 
+for (i = 0; i < bdrv_drain_all_count; i++) {
+bdrv_drained_begin(bs);
+}
+
 QTAILQ_INSERT_TAIL(_bdrv_states, bs, bs_list);
 
 return bs;
@@ -1163,7 +1167,7 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 int open_flags, Error **errp)
 {
 Error *local_err = NULL;
-int ret;
+int i, ret;
 
 bdrv_assign_node_name(bs, node_name, _err);
 if (local_err) {
@@ -1211,6 +1215,12 @@ static int bdrv_open_driver(BlockDriverState *bs, 
BlockDriver *drv,
 assert(bdrv_min_mem_align(bs) != 0);
 assert(is_power_of_2(bs->bl.request_alignment));
 
+for (i = 0; i < bs->quiesce_counter; i++) {
+if (drv->bdrv_co_drain_begin) {
+drv->bdrv_co_drain_begin(bs);
+}
+}
+
 return 0;
 open_failed:
 bs->drv = NULL;
@@ -2021,7 +2031,12 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
 child->role->detach(child);
 }
 if (old_bs->quiesce_counter && child->role->drained_end) {
-for (i = 0; i < old_bs->quiesce_counter; i++) {
+int num = old_bs->quiesce_counter;
+if (child->role->parent_is_bds) {
+num -= bdrv_drain_all_count;
+}
+assert(num >= 0);
+for (i = 0; i < num; i++) {
 child->role->drained_end(child);
 }
 }
@@ -2033,7 +2048,12 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
 if (new_bs) {
 QLIST_INSERT_HEAD(_bs->parents, child, next_parent);
 if (new_bs->quiesce_counter && child->role->drained_begin) {
-for (i = 0; i < new_bs->quiesce_counter; i++) {
+int num = new_bs->quiesce_counter;
+if (child->role->parent_is_bds) {
+num -= bdrv_drain_all_count;
+}
+assert(num >= 0);
+for (i = 0; i < num; i++) {
 child->role->drained_begin(child);
 }
 }
@@ -4037,6 +4057,14 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
 return QTAILQ_NEXT(bs, node_list);
 }
 
+BlockDriverState *bdrv_next_all_states(BlockDriverState *bs)
+{
+if (!bs) {
+return QTAILQ_FIRST(_bdrv_states);
+}
+return QTAILQ_NEXT(bs, bs_list);
+}
+
 const char *bdrv_get_node_name(const BlockDriverState *bs)
 {
 return bs->node_name;
diff --git a/block/io.c b/block/io.c
index d71e278289..08298f815b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -38,6 +38,8 @@
 /* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
 #define MAX_BOUNCE_BUFFER (32768 << 

Re: [Qemu-block] [PATCH] qcow2: Fix Coverity warning when calculating the refcount cache size

[Kevin Wolf]

Am 28.05.2018 um 17:01 hat Alberto Garcia geschrieben:
> MIN_REFCOUNT_CACHE_SIZE is 4 and the cluster size is guaranteed to be
> at most 2MB, so the minimum refcount cache size (in bytes) is always
> going to fit in a 32-bit integer.
> 
> Coverity doesn't know that, and since we're storing the result in a
> uint64_t (*refcount_cache_size) it thinks that we need the 64 bits and
> that we probably want to do a 64-bit multiplication to prevent the
> result from being truncated.
> 
> This is a false positive in this case, but it's a fair warning.
> We could do a 64-bit multiplication to get rid of it, but since we
> know that a 32-bit variable is enough to store this value let's simply
> reuse min_refcount_cache, make it a normal int and stop doing casts.
> 
> Signed-off-by: Alberto Garcia 
> Reported-by: Peter Maydell 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/20] Drain fixes and cleanups, part 3

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180529172156.29311-1-kw...@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/20] Drain fixes and cleanups, part 3

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   patchew/20180529172156.29311-1-kw...@redhat.com -> 
patchew/20180529172156.29311-1-kw...@redhat.com
Switched to a new branch 'test'
eab5e8287f test-bdrv-drain: Test graph changes in drain_all section
3300d94a9b block: Allow graph changes in bdrv_drain_all_begin/end sections
3a37012ee9 block: ignore_bds_parents parameter for drain functions
a0cd9923b2 block: Move bdrv_drain_all_begin() out of coroutine context
7df9cb285f block: Allow AIO_WAIT_WHILE with NULL ctx
fa7d0ac25e test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll
3c98182ba5 block: Defer .bdrv_drain_begin callback to polling phase
928f6f85a4 test-bdrv-drain: Graph change through parent callback
75e58be3aa block: Don't poll in parent drain callbacks
84fea14482 test-bdrv-drain: Test node deletion in subtree recursion
6e4d9cda70 block: Drain recursively with a single BDRV_POLL_WHILE()
9531ba32b7 test-bdrv-drain: Add test for node deletion
f6fe14abad block: Remove bdrv_drain_recurse()
aa7ade75cf block: Really pause block jobs on drain
00d52a27b6 block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE()
c96450896a tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now
955c52ae29 block: Don't manually poll in bdrv_drain_all()
09713c9ac7 block: Remove 'recursive' parameter from bdrv_drain_invoke()
e0ccd9c8f2 block: Use bdrv_do_drain_begin/end in bdrv_drain_all()
9e0ea8ed00 test-bdrv-drain: bdrv_drain() works with cross-AioContext events

=== OUTPUT BEGIN ===
Checking PATCH 1/20: test-bdrv-drain: bdrv_drain() works with cross-AioContext 
events...
Checking PATCH 2/20: block: Use bdrv_do_drain_begin/end in bdrv_drain_all()...
Checking PATCH 3/20: block: Remove 'recursive' parameter from 
bdrv_drain_invoke()...
Checking PATCH 4/20: block: Don't manually poll in bdrv_drain_all()...
Checking PATCH 5/20: tests/test-bdrv-drain: bdrv_drain_all() works in 
coroutines now...
Checking PATCH 6/20: block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE()...
ERROR: trailing statements should be on next line
#38: FILE: block/io.c:190:
+while (aio_poll(bs->aio_context, false));

ERROR: braces {} are necessary for all arms of this statement
#38: FILE: block/io.c:190:
+while (aio_poll(bs->aio_context, false));
[...]

total: 2 errors, 0 warnings, 60 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 7/20: block: Really pause block jobs on drain...
Checking PATCH 8/20: block: Remove bdrv_drain_recurse()...
Checking PATCH 9/20: test-bdrv-drain: Add test for node deletion...
Checking PATCH 10/20: block: Drain recursively with a single 
BDRV_POLL_WHILE()...
Checking PATCH 11/20: test-bdrv-drain: Test node deletion in subtree 
recursion...
WARNING: line over 80 characters
#85: FILE: tests/test-bdrv-drain.c:1034:
+g_test_add_func("/bdrv-drain/detach/drain_subtree", 
test_detach_by_drain_subtree);

total: 0 errors, 1 warnings, 68 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 12/20: block: Don't poll in parent drain callbacks...
Checking PATCH 13/20: test-bdrv-drain: Graph change through parent callback...
WARNING: line over 80 characters
#81: FILE: tests/test-bdrv-drain.c:1049:
+child_a = bdrv_attach_child(parent_b, a, "PB-A", _backing, 
_abort);

total: 0 errors, 1 warnings, 142 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 14/20: block: Defer .bdrv_drain_begin callback to polling 
phase...
Checking PATCH 15/20: test-bdrv-drain: Test that bdrv_drain_invoke() doesn't 
poll...
Checking PATCH 16/20: block: Allow AIO_WAIT_WHILE with NULL ctx...
Checking PATCH 17/20: block: Move bdrv_drain_all_begin() out of coroutine 
context...
WARNING: line over 80 characters
#27: FILE: block/io.c:270:
+bdrv_do_drained_begin(bs, data->recursive, data->parent, 

Re: [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:19PM +0200, Kevin Wolf wrote:
> This adds a helper function that logs both the QMP request and the
> received response before returning it.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/iotests.py | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 17aa7c88dc..319d898172 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -206,6 +206,10 @@ def filter_qmp_event(event):
>  event['timestamp']['microseconds'] = 'USECS'
>  return event
>  
> +def filter_testfiles(msg):
> +prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> +return msg.replace(prefix, 'TEST_DIR/')
> +

Either as-is, or with the suggestion by Max to add the PID to the output:

Reviewed-by: Jeff Cody 


>  def log(msg, filters=[]):
>  for flt in filters:
>  msg = flt(msg)
> @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
>  result.append(filter_qmp_event(ev))
>  return result
>  
> +def qmp_log(self, cmd, **kwargs):
> +logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
> +log(filter_testfiles(logmsg))
> +result = self.qmp(cmd, **kwargs)
> +log(result)
> +return result
> +
>  
>  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>  
> -- 
> 2.13.6
> 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] qapi/error: converts error_setg(_fatal) to error_report() + exit()

[Auger Eric]

Hi,

On 05/29/2018 07:48 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series converts error_setg(_fatal) to error_report() + exit() as
> suggested by the "qapi/error.h" documentation.
> 
> This reduce Coverity and Clang static analyzer positive falses.
> 
> See http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07585.html:
> 
> On 07/24/2017 04:52 PM, Eric Blake wrote:
> That's a shame.  Rather, we should patch this file (and others) to avoid
> all the inconsistent uses of error_setg(_*), to comply with the
> error.h documentation.

Thanks for the fix.

For the series:
Reviewed-by: Eric Auger 

Thanks

Eric

> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (4):
>   hw/block/fdc: Replace error_setg(_abort) by error_report() + abort()
>   hw/ppc/spapr_drc: Replace error_setg(_abort) by error_report() +
> abort()
>   hw/arm/sysbus-fdt: Replace error_setg(_fatal) by error_report()
> + exit()
>   device_tree: Replace error_setg(_fatal) by error_report() + exit()
> 
>  device_tree.c   | 23 +--
>  hw/arm/sysbus-fdt.c | 45 ++---
>  hw/block/fdc.c  |  7 ---
>  hw/ppc/spapr_drc.c  |  3 ++-
>  4 files changed, 45 insertions(+), 33 deletions(-)
> 

Re: [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs

[Kevin Wolf]

Am 29.05.2018 um 16:43 hat Jeff Cody geschrieben:
> On Fri, May 25, 2018 at 06:33:16PM +0200, Kevin Wolf wrote:
> > So far we relied on job->ret and strerror() to produce an error message
> > for failed jobs. Not surprisingly, this tends to result in completely
> > useless messages.
> > 
> > This adds a Job.error field that can contain an error string for a
> > failing job, and a parameter to job_completed() that sets the field. As
> > a default, if NULL is passed, we continue to use strerror(job->ret).
> > 
> > All existing callers are changed to pass NULL. They can be improved in
> > separate patches.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  include/qemu/job.h |  7 ++-
> >  block/backup.c |  2 +-
> >  block/commit.c |  2 +-
> >  block/mirror.c |  2 +-
> >  block/stream.c |  2 +-
> >  job-qmp.c  |  9 ++---
> >  job.c  | 15 +--
> >  7 files changed, 25 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/qemu/job.h b/include/qemu/job.h
> > index 8c8badf75e..b2e1dd00b9 100644
> > --- a/include/qemu/job.h
> > +++ b/include/qemu/job.h
> > @@ -124,6 +124,9 @@ typedef struct Job {
> >  /** Estimated progress_current value at the completion of the job */
> >  int64_t progress_total;
> >  
> > +/** Error string for a failed job (NULL if job->ret == 0) */
> > +char *error;
> > +
> >  /** ret code passed to job_completed. */
> >  int ret;
> >  
> > @@ -466,13 +469,15 @@ void job_transition_to_ready(Job *job);
> >  /**
> >   * @job: The job being completed.
> >   * @ret: The status code.
> > + * @error: The error message for a failing job (only with @ret < 0). If 
> > @ret is
> > + * negative, but NULL is given for @error, strerror() is used.
> >   *
> >   * Marks @job as completed. If @ret is non-zero, the job transaction it is 
> > part
> >   * of is aborted. If @ret is zero, the job moves into the WAITING state. 
> > If it
> >   * is the last job to complete in its transaction, all jobs in the 
> > transaction
> >   * move from WAITING to PENDING.
> >   */
> > -void job_completed(Job *job, int ret);
> > +void job_completed(Job *job, int ret, Error *error);
> >  
> >  /** Asynchronously complete the specified @job. */
> >  void job_complete(Job *job, Error **errp);
> > diff --git a/block/backup.c b/block/backup.c
> > index 4e228e959b..5661435675 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -321,7 +321,7 @@ static void backup_complete(Job *job, void *opaque)
> >  {
> >  BackupCompleteData *data = opaque;
> >  
> > -job_completed(job, data->ret);
> > +job_completed(job, data->ret, NULL);
> >  g_free(data);
> >  }
> >  
> > diff --git a/block/commit.c b/block/commit.c
> > index 620666161b..e1814d9693 100644
> > --- a/block/commit.c
> > +++ b/block/commit.c
> > @@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque)
> >   * bdrv_set_backing_hd() to fail. */
> >  block_job_remove_all_bdrv(bjob);
> >  
> > -job_completed(job, ret);
> > +job_completed(job, ret, NULL);
> >  g_free(data);
> >  
> >  /* If bdrv_drop_intermediate() didn't already do that, remove the 
> > commit
> > diff --git a/block/mirror.c b/block/mirror.c
> > index dcb66ec3be..435268bbbf 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -581,7 +581,7 @@ static void mirror_exit(Job *job, void *opaque)
> >  blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
> >  blk_insert_bs(bjob->blk, mirror_top_bs, _abort);
> >  
> > -job_completed(job, data->ret);
> > +job_completed(job, data->ret, NULL);
> >  
> >  g_free(data);
> >  bdrv_drained_end(src);
> > diff --git a/block/stream.c b/block/stream.c
> > index a5d6e0cf8a..9264b68a1e 100644
> > --- a/block/stream.c
> > +++ b/block/stream.c
> > @@ -93,7 +93,7 @@ out:
> >  }
> >  
> >  g_free(s->backing_file_str);
> > -job_completed(job, data->ret);
> > +job_completed(job, data->ret, NULL);
> >  g_free(data);
> >  }
> >  
> > diff --git a/job-qmp.c b/job-qmp.c
> > index 7f38f63336..410775df61 100644
> > --- a/job-qmp.c
> > +++ b/job-qmp.c
> > @@ -136,14 +136,9 @@ void qmp_job_dismiss(const char *id, Error **errp)
> >  static JobInfo *job_query_single(Job *job, Error **errp)
> >  {
> >  JobInfo *info;
> > -const char *errmsg = NULL;
> >  
> >  assert(!job_is_internal(job));
> >  
> > -if (job->ret < 0) {
> > -errmsg = strerror(-job->ret);
> > -}
> > -
> >  info = g_new(JobInfo, 1);
> >  *info = (JobInfo) {
> >  .id = g_strdup(job->id),
> > @@ -151,8 +146,8 @@ static JobInfo *job_query_single(Job *job, Error **errp)
> >  .status = job->status,
> >  .current_progress   = job->progress_current,
> >  .total_progress = job->progress_total,
> > -.has_error  = !!errmsg,
> > -.error  = g_strdup(errmsg),
> > +.has_error  = !!job->error,
> > +

[Qemu-block] [PATCH v2 18/20] block: ignore_bds_parents parameter for drain functions

[Kevin Wolf]

In the future, bdrv_drained_all_begin/end() will drain all invidiual
nodes separately rather than whole subtrees. This means that we don't
want to propagate the drain to all parents any more: If the parent is a
BDS, it will already be drained separately. Recursing to all parents is
unnecessary work and would make it an O(n²) operation.

Prepare the drain function for the changed drain_all by adding an
ignore_bds_parents parameter to the internal implementation that
prevents the propagation of the drain to BDS parents. We still (have to)
propagate it to non-BDS parents like BlockBackends or Jobs because those
are not drained separately.

Signed-off-by: Kevin Wolf 
---
 include/block/block.h | 16 ++---
 include/block/block_int.h |  6 
 block.c   | 11 +++---
 block/io.c| 88 ---
 block/vvfat.c |  1 +
 5 files changed, 78 insertions(+), 44 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 6026c1385e..a678556608 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -555,7 +555,8 @@ void bdrv_io_unplug(BlockDriverState *bs);
  * Begin a quiesced section of all users of @bs. This is part of
  * bdrv_drained_begin.
  */
-void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore);
+void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
+   bool ignore_bds_parents);
 
 /**
  * bdrv_parent_drained_end:
@@ -563,18 +564,23 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, 
BdrvChild *ignore);
  * End a quiesced section of all users of @bs. This is part of
  * bdrv_drained_end.
  */
-void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);
+void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
+ bool ignore_bds_parents);
 
 /**
  * bdrv_drain_poll:
  *
  * Poll for pending requests in @bs, its parents (except for @ignore_parent),
- * and if @recursive is true its children as well.
+ * and if @recursive is true its children as well (used for subtree drain).
+ *
+ * If @ignore_bds_parents is true, parents that are BlockDriverStates must
+ * ignore the drain request because they will be drained separately (used for
+ * drain_all).
  *
  * This is part of bdrv_drained_begin.
  */
 bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
- BdrvChild *ignore_parent);
+ BdrvChild *ignore_parent, bool ignore_bds_parents);
 
 /**
  * bdrv_drained_begin:
@@ -595,7 +601,7 @@ void bdrv_drained_begin(BlockDriverState *bs);
  * running requests to complete.
  */
 void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
-   BdrvChild *parent);
+   BdrvChild *parent, bool ignore_bds_parents);
 
 /**
  * Like bdrv_drained_begin, but recursively begins a quiesced section for
diff --git a/include/block/block_int.h b/include/block/block_int.h
index feba86888e..edb16df015 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -545,6 +545,12 @@ struct BdrvChildRole {
  * points to. */
 bool stay_at_node;
 
+/* If true, the parent is a BlockDriverState and bdrv_next_all_states()
+ * will return it. This information is used for drain_all, where every node
+ * will be drained separately, so the drain only needs to be propagated to
+ * non-BDS parents. */
+bool parent_is_bds;
+
 void (*inherit_options)(int *child_flags, QDict *child_options,
 int parent_flags, QDict *parent_options);
 
diff --git a/block.c b/block.c
index 1405d1180b..ce6eb53ad9 100644
--- a/block.c
+++ b/block.c
@@ -817,13 +817,13 @@ static char *bdrv_child_get_parent_desc(BdrvChild *c)
 static void bdrv_child_cb_drained_begin(BdrvChild *child)
 {
 BlockDriverState *bs = child->opaque;
-bdrv_do_drained_begin_quiesce(bs, NULL);
+bdrv_do_drained_begin_quiesce(bs, NULL, false);
 }
 
 static bool bdrv_child_cb_drained_poll(BdrvChild *child)
 {
 BlockDriverState *bs = child->opaque;
-return bdrv_drain_poll(bs, false, NULL);
+return bdrv_drain_poll(bs, false, NULL, false);
 }
 
 static void bdrv_child_cb_drained_end(BdrvChild *child)
@@ -907,6 +907,7 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
 }
 
 const BdrvChildRole child_file = {
+.parent_is_bds   = true,
 .get_parent_desc = bdrv_child_get_parent_desc,
 .inherit_options = bdrv_inherited_options,
 .drained_begin   = bdrv_child_cb_drained_begin,
@@ -932,6 +933,7 @@ static void bdrv_inherited_fmt_options(int *child_flags, 
QDict *child_options,
 }
 
 const BdrvChildRole child_format = {
+.parent_is_bds   = true,
 .get_parent_desc = bdrv_child_get_parent_desc,
 .inherit_options = bdrv_inherited_fmt_options,
 .drained_begin   = bdrv_child_cb_drained_begin,
@@ -1050,6 +1052,7 @@ static int 

Re: [Qemu-block] [PATCH 10/14] qemu-iotests: Rewrite 210 for blockdev-create job

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:23PM +0200, Kevin Wolf wrote:
> This rewrites the test case 210 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  tests/qemu-iotests/210| 393 
> ++
>  tests/qemu-iotests/210.out| 189 ++--
>  tests/qemu-iotests/group  |   2 +-
>  tests/qemu-iotests/iotests.py |  12 +-
>  4 files changed, 310 insertions(+), 286 deletions(-)
> 
> diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
> index e607c0d296..ff4fddea56 100755
> --- a/tests/qemu-iotests/210
> +++ b/tests/qemu-iotests/210
> @@ -1,9 +1,11 @@
> -#!/bin/bash
> +#!/usr/bin/env python
>  #
>  # Test luks and file image creation
>  #
>  # Copyright (C) 2018 Red Hat, Inc.
>  #
> +# Creator/Owner: Kevin Wolf 
> +#
>  # This program is free software; you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
>  # the Free Software Foundation; either version 2 of the License, or
> @@ -18,230 +20,165 @@
>  # along with this program.  If not, see .
>  #
>  
> -# creator
> -owner=kw...@redhat.com
> -
> -seq=`basename $0`
> -echo "QA output created by $seq"
> -
> -here=`pwd`
> -status=1 # failure is the default!
> -
> -# get standard environment, filters and checks
> -. ./common.rc
> -. ./common.filter
> -
> -_supported_fmt luks
> -_supported_proto file
> -_supported_os Linux
> -
> -function do_run_qemu()
> -{
> -echo Testing: "$@"
> -$QEMU -nographic -qmp stdio -serial none "$@"
> -echo
> -}
> -
> -function run_qemu()
> -{
> -do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
> -  | _filter_qemu | _filter_imgfmt \
> -  | _filter_actual_image_size
> -}
> -
> -echo
> -echo "=== Successful image creation (defaults) ==="
> -echo
> -
> -size=$((128 * 1024 * 1024))
> -
> -run_qemu -object secret,id=keysec0,data="foo" < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "file",
> -  "filename": "$TEST_IMG_FILE",
> -  "size": 0
> -  }
> -}
> -{ "execute": "blockdev-add",
> -  "arguments": {
> -  "driver": "file",
> -  "node-name": "imgfile",
> -  "filename": "$TEST_IMG_FILE"
> -  }
> -}
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "$IMGFMT",
> -  "file": "imgfile",
> -  "key-secret": "keysec0",
> -  "size": $size,
> -  "iter-time": 10
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info --format-specific | _filter_img_info --format-specific
> -
> -echo
> -echo "=== Successful image creation (with non-default options) ==="
> -echo
> -
> -# Choose a different size to show that we got a new image
> -size=$((64 * 1024 * 1024))
> -
> -run_qemu -object secret,id=keysec0,data="foo" < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "file",
> -  "filename": "$TEST_IMG_FILE",
> -  "size": 0
> -  }
> -}
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "$IMGFMT",
> -  "file": {
> -  "driver": "file",
> -  "filename": "$TEST_IMG_FILE"
> -  },
> -  "size": $size,
> -  "key-secret": "keysec0",
> -  "cipher-alg": "twofish-128",
> -  "cipher-mode": "ctr",
> -  "ivgen-alg": "plain64",
> -  "ivgen-hash-alg": "md5",
> -  "hash-alg": "sha1",
> -  "iter-time": 10
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info --format-specific | _filter_img_info --format-specific
> -
> -echo
> -echo "=== Invalid BlockdevRef ==="
> -echo
> -
> -run_qemu < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "$IMGFMT",
> -  "file": "this doesn't exist",
> -  "size": $size
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -echo
> -echo "=== Zero size ==="
> -echo
> -
> -run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \
> - -object secret,id=keysec0,data="foo" < -{ "execute": "qmp_capabilities" }
> -{ "execute": "x-blockdev-create",
> -  "arguments": {
> -  "driver": "$IMGFMT",
> -  "file": "node0",
> -  "key-secret": "keysec0",
> -  "size": 0,
> -  "iter-time": 10
> -  }
> -}
> -{ "execute": "quit" }
> -EOF
> -
> -_img_info | _filter_img_info
> -
> -
> -echo
> -echo "=== Invalid sizes ==="
> -echo
> -
> -# TODO Negative image sizes aren't handled correctly, but this is a problem
> -# with QAPI's implementation of the 'size' type and affects other commands as
> -# well. Once this is fixed, we may want to add a test case here.
> -
> -# 1. 2^64 - 512
> -# 2. 2^63 = 8 EB 

Re: [Qemu-block] [ovirt-users] Libvirt ERROR cannot access backing file after importing VM from OpenStack

[Eric Blake]

On 05/28/2018 05:27 AM, Arik Hadas wrote:

[Answering before reading the entire thread; apologies if I'm repeating 
things, or if I have to chime in again at other spots]



Let me demonstrate briefly the flow for OVA:
Let's say that we have a VM that is based on a template and has one disk
and one snapshot, so its volume-chain would be:
T -> S -> V
(V is the volume the VM writes to, S is the backing file of V and T is the
backing file of S).


I tend to write backing relationships as a left arrow, as in:

T <- S <- V

(can be read as: S depends on T, and V depends on S)


When exporting that VM to an OVA file we want the produced tar file to be
comprised of:
(1) OVF configuration
(2) single disk volume (preferably qcow).

So we need to collapse T, S, V into a single volume.
Sure, we can do 'qemu-img convert'. That's what we do now in oVirt 4.2:
(a) qemu-img convert produces a 'temporary' collapsed volume
(b) make a tar file of the OVf configuration and that 'temporary' volume
(c) delete the temporary volume

But the fact that we produce that 'temporary' volume obviously slows down
the entire operation.
It would be much better if we could "open" a stream that we can read from
the 'collapsed' form of that chain and stream it directly into the
appropriate tar file entry, without extra writes to the storage device.

Few months ago people from the oVirt-storage team checked the qemu toolset
and replied that this capability is not yet provided, therefore we
implemented the workaround described above. Apparently, the desired ability
can also be useful for the flow discussed in this thread so it worth asking
for it again :)


You CAN get a logically collapsed view of storage (that is, what the 
guest would see), by using an NBD export of volume V.  Reading from that 
volume will then pull sectors from whichever portion of the chain you 
need.  You can use either qemu-nbd (if no guest is writing to the 
chain), or within a running qemu, you can use nbd-server-start and 
nbd-server-add (over QMP) to get such an NBD server running.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-block] [PATCH v2 15/16] qemu-iotests: Rewrite 213 for blockdev-create job

[Kevin Wolf]

This rewrites the test case 213 to work with the new x-blockdev-create
job rather than the old synchronous version of the command.

All of the test cases stay the same as before, but in order to be able
to implement proper job handling, the test case is rewritten in Python.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/213 | 520 +
 tests/qemu-iotests/213.out | 208 +++---
 tests/qemu-iotests/group   |   4 +-
 3 files changed, 319 insertions(+), 413 deletions(-)

diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
index 3a00a0f6d6..29d25bcee1 100755
--- a/tests/qemu-iotests/213
+++ b/tests/qemu-iotests/213
@@ -1,9 +1,11 @@
-#!/bin/bash
+#!/usr/bin/env python
 #
 # Test vhdx and file image creation
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
+# Creator/Owner: Kevin Wolf 
+#
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 2 of the License, or
@@ -18,332 +20,190 @@
 # along with this program.  If not, see .
 #
 
-# creator
-owner=kw...@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-status=1   # failure is the default!
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt vhdx
-_supported_proto file
-_supported_os Linux
-
-function do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-function run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
-  | _filter_qemu | _filter_imgfmt \
-  | _filter_actual_image_size
-}
-
-echo
-echo "=== Successful image creation (defaults) ==="
-echo
-
-size=$((128 * 1024 * 1024))
-
-run_qemu <

[Qemu-block] [PATCH v2 16/16] block/create: Mark blockdev-create stable

[Kevin Wolf]

We're ready to declare the blockdev-create job stable. This renames the
corresponding QMP command from x-blockdev-create to blockdev-create.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Jeff Cody 
---
 qapi/block-core.json   |  4 ++--
 qapi/job.json  |  2 +-
 block/create.c |  4 ++--
 tests/qemu-iotests/206 |  2 +-
 tests/qemu-iotests/206.out | 54 +++---
 tests/qemu-iotests/207 |  2 +-
 tests/qemu-iotests/207.out | 18 
 tests/qemu-iotests/210 |  2 +-
 tests/qemu-iotests/210.out | 18 
 tests/qemu-iotests/211 |  2 +-
 tests/qemu-iotests/211.out | 24 ++---
 tests/qemu-iotests/212 |  2 +-
 tests/qemu-iotests/212.out | 42 ++--
 tests/qemu-iotests/213 |  2 +-
 tests/qemu-iotests/213.out | 44 ++---
 15 files changed, 111 insertions(+), 111 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index eb98596614..4b1de474a9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4011,7 +4011,7 @@
   } }
 
 ##
-# @x-blockdev-create:
+# @blockdev-create:
 #
 # Starts a job to create an image format on a given node. The job is
 # automatically finalized, but a manual job-dismiss is required.
@@ -4022,7 +4022,7 @@
 #
 # Since: 3.0
 ##
-{ 'command': 'x-blockdev-create',
+{ 'command': 'blockdev-create',
   'data': { 'job-id': 'str',
 'options': 'BlockdevCreateOptions' } }
 
diff --git a/qapi/job.json b/qapi/job.json
index 69c1970a58..17d10037c4 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -17,7 +17,7 @@
 #
 # @backup: drive backup job type, see "drive-backup"
 #
-# @create: image creation job type, see "x-blockdev-create" (since 3.0)
+# @create: image creation job type, see "blockdev-create" (since 3.0)
 #
 # Since: 1.7
 ##
diff --git a/block/create.c b/block/create.c
index 1a263e4b13..915cd41bcc 100644
--- a/block/create.c
+++ b/block/create.c
@@ -63,8 +63,8 @@ static const JobDriver blockdev_create_job_driver = {
 .start = blockdev_create_run,
 };
 
-void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions *options,
-   Error **errp)
+void qmp_blockdev_create(const char *job_id, BlockdevCreateOptions *options,
+ Error **errp)
 {
 BlockdevCreateJob *s;
 const char *fmt = BlockdevDriver_str(options->driver);
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index b8cf2e7dca..128c334c7c 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -26,7 +26,7 @@ from iotests import imgfmt
 iotests.verify_image_format(supported_fmts=['qcow2'])
 
 def blockdev_create(vm, options):
-result = vm.qmp_log('x-blockdev-create', job_id='job0', options=options)
+result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
 
 if 'return' in result:
 assert result['return'] == {}
diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out
index 34451a3fc6..789eebe57b 100644
--- a/tests/qemu-iotests/206.out
+++ b/tests/qemu-iotests/206.out
@@ -1,13 +1,13 @@
 === Successful image creation (defaults) ===
 
-{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'size': 0, 'driver': 'file', 'filename': 'TEST_DIR/PID-t.qcow2'}}}
+{'execute': 'blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'size': 0, 'driver': 'file', 'filename': 'TEST_DIR/PID-t.qcow2'}}}
 {u'return': {}}
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
 {'execute': 'blockdev-add', 'arguments': {'node_name': 'imgfile', 'driver': 
'file', 'filename': 'TEST_DIR/PID-t.qcow2'}}
 {u'return': {}}
-{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'driver': 'qcow2', 'file': 'imgfile', 'size': 134217728}}}
+{'execute': 'blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'driver': 'qcow2', 'file': 'imgfile', 'size': 134217728}}}
 {u'return': {}}
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
@@ -24,12 +24,12 @@ Format specific information:
 
 === Successful image creation (inline blockdev-add, explicit defaults) ===
 
-{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'nocow': False, 'preallocation': 'off', 'size': 0, 'driver': 'file', 
'filename': 'TEST_DIR/PID-t.qcow2'}}}
+{'execute': 'blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'nocow': False, 'preallocation': 'off', 'size': 0, 'driver': 'file', 
'filename': 'TEST_DIR/PID-t.qcow2'}}}
 {u'return': {}}
 {'execute': 'job-dismiss', 'arguments': {'id': 'job0'}}
 {u'return': {}}
 
-{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
{'cluster-size': 65536, 'refcount-bits': 16, 'version': 'v3', 'preallocation': 
'off', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-t.qcow2'}, 
'lazy-refcounts': False, 'driver': 'qcow2', 'size': 

[Qemu-block] [PATCH v2 09/16] qemu-iotests: iotests.py helper for non-file protocols

[Kevin Wolf]

This adds two helper functions that are useful for test cases that make
use of a non-file protocol (specifically ssh).

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 17 +
 1 file changed, 17 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8b612cb891..bc8f404ac2 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -303,6 +303,13 @@ def file_path(*names):
 
 return paths[0] if len(paths) == 1 else paths
 
+def remote_filename(path):
+if imgproto == 'file':
+return path
+elif imgproto == 'ssh':
+return "ssh://127.0.0.1%s" % (path)
+else:
+raise Exception("Protocol %s not supported" % (imgproto))
 
 class VM(qtest.QEMUQtestMachine):
 '''A QEMU VM'''
@@ -601,6 +608,16 @@ def verify_image_format(supported_fmts=[], 
unsupported_fmts=[]):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
+def verify_protocol(supported=[], unsupported=[]):
+assert not (supported and unsupported)
+
+if 'generic' in supported:
+return
+
+not_sup = supported and (imgproto not in supported)
+if not_sup or (imgproto in unsupported):
+notrun('not suitable for this protocol: %s' % imgproto)
+
 def verify_platform(supported_oses=['linux']):
 if True not in [sys.platform.startswith(x) for x in supported_oses]:
 notrun('not suitable for this OS: %s' % sys.platform)
-- 
2.13.6

[Qemu-block] [PATCH v2 06/16] qemu-iotests: Add VM.qmp_log()

[Kevin Wolf]

This adds a helper function that logs both the QMP request and the
received response before returning it.

Signed-off-by: Kevin Wolf 
Reviewed-by: Jeff Cody 
---
 tests/qemu-iotests/iotests.py | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 17aa7c88dc..2f54823db6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -206,6 +206,10 @@ def filter_qmp_event(event):
 event['timestamp']['microseconds'] = 'USECS'
 return event
 
+def filter_testfiles(msg):
+prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
+return msg.replace(prefix, 'TEST_DIR/PID-')
+
 def log(msg, filters=[]):
 for flt in filters:
 msg = flt(msg)
@@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
 result.append(filter_qmp_event(ev))
 return result
 
+def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
+logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
+log(logmsg, filters)
+result = self.qmp(cmd, **kwargs)
+log(str(result), filters)
+return result
+
 
 index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
 
-- 
2.13.6

[Qemu-block] [PATCH v2 14/16] qemu-iotests: Rewrite 212 for blockdev-create job

[Kevin Wolf]

This rewrites the test case 212 to work with the new x-blockdev-create
job rather than the old synchronous version of the command.

All of the test cases stay the same as before, but in order to be able
to implement proper job handling, the test case is rewritten in Python.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/212 | 483 +
 tests/qemu-iotests/212.out | 191 +++---
 tests/qemu-iotests/group   |   2 +-
 3 files changed, 295 insertions(+), 381 deletions(-)

diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
index e5a1ba77ce..03cf41d133 100755
--- a/tests/qemu-iotests/212
+++ b/tests/qemu-iotests/212
@@ -1,9 +1,11 @@
-#!/bin/bash
+#!/usr/bin/env python
 #
 # Test parallels and file image creation
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
+# Creator/Owner: Kevin Wolf 
+#
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 2 of the License, or
@@ -18,309 +20,176 @@
 # along with this program.  If not, see .
 #
 
-# creator
-owner=kw...@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-status=1   # failure is the default!
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt parallels
-_supported_proto file
-_supported_os Linux
-
-function do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-function run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
-  | _filter_qemu | _filter_imgfmt \
-  | _filter_actual_image_size
-}
-
-echo
-echo "=== Successful image creation (defaults) ==="
-echo
-
-size=$((128 * 1024 * 1024))
-
-run_qemu <

Re: [Qemu-block] [PATCH] nbd/client: Relax handling of large NBD_CMD_BLOCK_STATUS reply

[Eric Blake]

On 05/29/2018 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:

04.05.2018 01:26, Eric Blake wrote:

The NBD spec is proposing a relaxation of NBD_CMD_BLOCK_STATUS
where a server may have the final extent per context give a
length beyond the original request, if it can easily prove that
subsequent bytes have the same status, on the grounds that a
client can take advantage of this information for fewer block
status requests.  Since qemu 2.12 as a client always sends
NBD_CMD_FLAG_REQ_ONE, and rejects a server that sends extra
length, the upstream NBD spec will probably limit this behavior
to clients that don't request REQ_ONE semantics; but it doesn't
hurt to relax qemu to always be permissive of this server
behavior, even if it continues to use REQ_ONE.


Hi!

Patch is applied, are you going to make a patch for NBD spec?


Yes, thanks for the reminder (lots going on lately).

https://lists.debian.org/nbd/2018/05/msg00042.html

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-block] [PATCH v2 12/16] qemu-iotests: Rewrite 210 for blockdev-create job

[Kevin Wolf]

This rewrites the test case 210 to work with the new x-blockdev-create
job rather than the old synchronous version of the command.

All of the test cases stay the same as before, but in order to be able
to implement proper job handling, the test case is rewritten in Python.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Jeff Cody 
---
 tests/qemu-iotests/210| 393 ++
 tests/qemu-iotests/210.out| 197 ++---
 tests/qemu-iotests/group  |   2 +-
 tests/qemu-iotests/iotests.py |  12 +-
 4 files changed, 314 insertions(+), 290 deletions(-)

diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index e607c0d296..ff4fddea56 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -1,9 +1,11 @@
-#!/bin/bash
+#!/usr/bin/env python
 #
 # Test luks and file image creation
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
+# Creator/Owner: Kevin Wolf 
+#
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 2 of the License, or
@@ -18,230 +20,165 @@
 # along with this program.  If not, see .
 #
 
-# creator
-owner=kw...@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-status=1   # failure is the default!
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt luks
-_supported_proto file
-_supported_os Linux
-
-function do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-function run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
-  | _filter_qemu | _filter_imgfmt \
-  | _filter_actual_image_size
-}
-
-echo
-echo "=== Successful image creation (defaults) ==="
-echo
-
-size=$((128 * 1024 * 1024))
-
-run_qemu -object secret,id=keysec0,data="foo" <0 
size"}}
-{"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"SHUTDOWN", "data": {"guest": false}}
-
-image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": 
"TEST_DIR/t.IMGFMT"}, "key-secret": "keysec0"}
+{'execute': 'block_resize', 'arguments': {'size': 9223372036854775296, 
'node_name': 'node1'}}
+{u'error': {u'class': u'GenericError', u'desc': u'The requested file size is 
too large'}}
+{'execute': 'block_resize', 'arguments': {'size': 9223372036854775808L, 
'node_name': 'node1'}}
+{u'error': {u'class': u'GenericError', u'desc': u"Invalid parameter type for 
'size', expected: integer"}}
+{'execute': 'block_resize', 'arguments': {'size': 18446744073709551104L, 
'node_name': 'node1'}}
+{u'error': {u'class': u'GenericError', u'desc': u"Invalid parameter type for 
'size', expected: integer"}}
+{'execute': 'block_resize', 'arguments': {'size': -9223372036854775808, 
'node_name': 'node1'}}
+{u'error': {u'class': u'GenericError', u'desc': u"Parameter 'size' expects a 
>0 size"}}
+image: json:{"driver": "IMGFMT", "file": {"driver": "file", "filename": 
"TEST_IMG"}, "key-secret": "keysec0"}
 file format: IMGFMT
 virtual size: 0 (0 bytes)
-*** done
+encrypted: yes
+Format specific information:
+ivgen alg: plain64
+hash alg: sha256
+cipher alg: aes-256
+uuid: ----
+cipher mode: xts
+slots:
+[0]:
+active: true
+iters: XXX
+key offset: 4096
+stripes: 4000
+[1]:
+active: false
+key offset: 262144
+[2]:
+active: false
+key offset: 520192
+[3]:
+active: false
+key offset: 778240
+[4]:
+active: false
+key offset: 1036288
+[5]:
+active: false
+key offset: 1294336
+[6]:
+active: false
+key offset: 1552384
+[7]:
+active: false
+key offset: 1810432
+payload offset: 2068480
+master key iters: XXX
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 8a84bf057d..a1d04ce367 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -208,9 +208,9 @@
 207 rw auto
 208 rw auto quick
 209 rw auto quick
+210 rw auto
 # TODO The following commented out tests need to be reworked to work
 # with the x-blockdev-create job
-#210 rw auto
 #211 rw auto quick
 #212 rw auto quick
 #213 rw auto quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bc8f404ac2..fdbdd8b300 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -109,8 +109,16 @@ def qemu_img_pipe(*args):
 sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
 return subp.communicate()[0]
 
-def img_info_log(filename, 

[Qemu-block] [PATCH v2 13/16] qemu-iotests: Rewrite 211 for blockdev-create job

[Kevin Wolf]

This rewrites the test case 211 to work with the new x-blockdev-create
job rather than the old synchronous version of the command.

All of the test cases stay the same as before, but in order to be able
to implement proper job handling, the test case is rewritten in Python.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/211 | 381 ++---
 tests/qemu-iotests/211.out | 133 +---
 tests/qemu-iotests/group   |   2 +-
 3 files changed, 229 insertions(+), 287 deletions(-)

diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index 1edec26517..b45f886d23 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -1,9 +1,11 @@
-#!/bin/bash
+#!/usr/bin/env python
 #
 # Test VDI and file image creation
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
+# Creator/Owner: Kevin Wolf 
+#
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 2 of the License, or
@@ -18,229 +20,154 @@
 # along with this program.  If not, see .
 #
 
-# creator
-owner=kw...@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-status=1   # failure is the default!
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt vdi
-_supported_proto file
-_supported_os Linux
-
-function do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-function run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
-  | _filter_qemu | _filter_imgfmt \
-  | _filter_actual_image_size
-}
-
-echo
-echo "=== Successful image creation (defaults) ==="
-echo
-
-size=$((128 * 1024 * 1024))
-
-run_qemu <

[Qemu-block] [PATCH v2 03/16] job: Add error message for failing jobs

[Kevin Wolf]

So far we relied on job->ret and strerror() to produce an error message
for failed jobs. Not surprisingly, this tends to result in completely
useless messages.

This adds a Job.error field that can contain an error string for a
failing job, and a parameter to job_completed() that sets the field. As
a default, if NULL is passed, we continue to use strerror(job->ret).

All existing callers are changed to pass NULL. They can be improved in
separate patches.

Signed-off-by: Kevin Wolf 
---
 include/qemu/job.h|  7 ++-
 block/backup.c|  2 +-
 block/commit.c|  2 +-
 block/mirror.c|  2 +-
 block/stream.c|  2 +-
 job-qmp.c |  9 ++---
 job.c | 16 ++--
 tests/test-bdrv-drain.c   |  2 +-
 tests/test-blockjob-txn.c |  2 +-
 tests/test-blockjob.c |  2 +-
 10 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 8c8badf75e..1d820530fa 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -124,6 +124,9 @@ typedef struct Job {
 /** Estimated progress_current value at the completion of the job */
 int64_t progress_total;
 
+/** Error string for a failed job (NULL if, and only if, job->ret == 0) */
+char *error;
+
 /** ret code passed to job_completed. */
 int ret;
 
@@ -466,13 +469,15 @@ void job_transition_to_ready(Job *job);
 /**
  * @job: The job being completed.
  * @ret: The status code.
+ * @error: The error message for a failing job (only with @ret < 0). If @ret is
+ * negative, but NULL is given for @error, strerror() is used.
  *
  * Marks @job as completed. If @ret is non-zero, the job transaction it is part
  * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
  * is the last job to complete in its transaction, all jobs in the transaction
  * move from WAITING to PENDING.
  */
-void job_completed(Job *job, int ret);
+void job_completed(Job *job, int ret, Error *error);
 
 /** Asynchronously complete the specified @job. */
 void job_complete(Job *job, Error **errp);
diff --git a/block/backup.c b/block/backup.c
index 4e228e959b..5661435675 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -321,7 +321,7 @@ static void backup_complete(Job *job, void *opaque)
 {
 BackupCompleteData *data = opaque;
 
-job_completed(job, data->ret);
+job_completed(job, data->ret, NULL);
 g_free(data);
 }
 
diff --git a/block/commit.c b/block/commit.c
index 620666161b..e1814d9693 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque)
  * bdrv_set_backing_hd() to fail. */
 block_job_remove_all_bdrv(bjob);
 
-job_completed(job, ret);
+job_completed(job, ret, NULL);
 g_free(data);
 
 /* If bdrv_drop_intermediate() didn't already do that, remove the commit
diff --git a/block/mirror.c b/block/mirror.c
index dcb66ec3be..435268bbbf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -581,7 +581,7 @@ static void mirror_exit(Job *job, void *opaque)
 blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
 blk_insert_bs(bjob->blk, mirror_top_bs, _abort);
 
-job_completed(job, data->ret);
+job_completed(job, data->ret, NULL);
 
 g_free(data);
 bdrv_drained_end(src);
diff --git a/block/stream.c b/block/stream.c
index a5d6e0cf8a..9264b68a1e 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -93,7 +93,7 @@ out:
 }
 
 g_free(s->backing_file_str);
-job_completed(job, data->ret);
+job_completed(job, data->ret, NULL);
 g_free(data);
 }
 
diff --git a/job-qmp.c b/job-qmp.c
index 7f38f63336..410775df61 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -136,14 +136,9 @@ void qmp_job_dismiss(const char *id, Error **errp)
 static JobInfo *job_query_single(Job *job, Error **errp)
 {
 JobInfo *info;
-const char *errmsg = NULL;
 
 assert(!job_is_internal(job));
 
-if (job->ret < 0) {
-errmsg = strerror(-job->ret);
-}
-
 info = g_new(JobInfo, 1);
 *info = (JobInfo) {
 .id = g_strdup(job->id),
@@ -151,8 +146,8 @@ static JobInfo *job_query_single(Job *job, Error **errp)
 .status = job->status,
 .current_progress   = job->progress_current,
 .total_progress = job->progress_total,
-.has_error  = !!errmsg,
-.error  = g_strdup(errmsg),
+.has_error  = !!job->error,
+.error  = g_strdup(job->error),
 };
 
 return info;
diff --git a/job.c b/job.c
index f026661b0f..84e140238b 100644
--- a/job.c
+++ b/job.c
@@ -369,6 +369,7 @@ void job_unref(Job *job)
 
 QLIST_REMOVE(job, job_list);
 
+g_free(job->error);
 g_free(job->id);
 g_free(job);
 }
@@ -660,6 +661,9 @@ static void job_update_rc(Job *job)
 job->ret = -ECANCELED;
 }
 if (job->ret) {
+if (!job->error) {
+

[Qemu-block] [PATCH v2 08/16] qemu-iotests: Add VM.run_job()

[Kevin Wolf]

Add an iotests.py function that runs a job and only returns when it is
destroyed. An error is logged when the job failed and job-finalize and
job-dismiss commands are issued if necessary.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index edcd2bb701..8b612cb891 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -418,6 +418,25 @@ class VM(qtest.QEMUQtestMachine):
 log(str(result), filters)
 return result
 
+def run_job(self, job, auto_finalize=True, auto_dismiss=False):
+while True:
+for ev in self.get_qmp_events_filtered(wait=True):
+if ev['event'] == 'JOB_STATUS_CHANGE':
+status = ev['data']['status']
+if status == 'aborting':
+result = self.qmp('query-jobs')
+for j in result['return']:
+if j['id'] == job:
+log('Job failed: %s' % (j['error']))
+elif status == 'pending' and not auto_finalize:
+self.qmp_log('job-finalize', id=job)
+elif status == 'concluded' and not auto_dismiss:
+self.qmp_log('job-dismiss', id=job)
+elif status == 'null':
+return
+else:
+iotests.log(ev)
+
 
 index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
 
-- 
2.13.6

Re: [Qemu-block] [ovirt-users] Libvirt ERROR cannot access backing file after importing VM from OpenStack

[Nir Soffer]

On Mon, May 28, 2018 at 2:38 PM Kevin Wolf  wrote:

> Am 28.05.2018 um 12:27 hat Arik Hadas geschrieben:
> > On Mon, May 28, 2018 at 11:25 AM, Kevin Wolf  wrote:
> >
> > > [ Adding qemu-block ]
> > >
> > > Am 27.05.2018 um 10:36 hat Arik Hadas geschrieben:
> > > > On Thu, May 24, 2018 at 6:13 PM, Nir Soffer 
> wrote:
> > > >
> > > > > On Thu, May 24, 2018 at 6:06 PM Vrgotic, Marko <
> > > m.vrgo...@activevideo.com>
> > > > > wrote:
> > > > >
> > > > >> Dear Nir,
> > > > >>
> > > > >> Thank you for quick reply.
> > > > >>
> > > > >> Ok, why it will not work?
> > > > >>
> > > > >
> > > > > Because the image has a backing file which is not accessible to
> oVirt.
> > > > >
> > > > >
> > > > >> I used qemu+tcp connection, via import method through engine
> admin UI.
> > > > >>
> > > > >> Images was imported and converted according logs, still “backing
> file”
> > > > >> invalid entry remained.
> > > > >>
> > > > >> Also, I did use same method before, connecting to plain “libvirt
> kvm”
> > > > >> host, import and conversion went smooth, no backend file.
> > > > >>
> > > > >> Image format is qcow(2) which is supported by oVirt.
> > > > >>
> > > > >> What am I missing? Should I use different method?
> > > > >>
> > > > >
> > > > > I guess this is not a problem on your side, but a bug in our side.
> > > > >
> > > > > Either we should block the operation that cannot work, or fix the
> > > process
> > > > > so we don't refer to non-existing image.
> > > > >
> > > > > When importing we have 2 options:
> > > > >
> > > > > - import the entire chain,  importing all images in the chain,
> > > converting
> > > > >  each image to oVirt volume, and updating the backing file of each
> > > layer
> > > > > to point to the oVirt image.
> > > > >
> > > > > - import the current state of the image into a new image, using
> either
> > > raw
> > > > > or qcow2, but without any backing file.
> > > > >
> > > > > Arik, do you know why we create qcow2 file with invalid backing
> file?
> > > > >
> > > >
> > > > It seems to be a result of a bit naive behavior of the kvm2ovirt
> module
> > > > that tries to download only the top-level volume the VM uses,
> assuming
> > > each
> > > > of the disks to be imported is comprised of a single volume.
> > > >
> > > > Maybe it's time to finally asking QEMU guys to provide a way to
> consume
> > > the
> > > > 'collapsed' form of a chain of volumes as a stream if that's not
> > > available
> > > > yet? ;) It can also boost the recently added process of exporting
> VMs as
> > > > OVAs...
> > >
> > > Not sure which operation we're talking about on the QEMU level, but
> > > generally the "collapsed" view is the normal thing because that's what
> > > guests see.
> > >
> > > For example, if you use 'qemu-img convert', you have to pass options to
> > > specifically disable it and convert only a single layer if you want to
> > > keep using backing files instead of getting a standalone image that
> > > contains everything.
> > >
> >
> > Yeah, some context was missing. Sorry about that.
> >
> > Let me demonstrate briefly the flow for OVA:
> > Let's say that we have a VM that is based on a template and has one disk
> > and one snapshot, so its volume-chain would be:
> > T -> S -> V
> > (V is the volume the VM writes to, S is the backing file of V and T is
> the
> > backing file of S).
> > When exporting that VM to an OVA file we want the produced tar file to be
> > comprised of:
> > (1) OVF configuration
> > (2) single disk volume (preferably qcow).
> >
> > So we need to collapse T, S, V into a single volume.
> > Sure, we can do 'qemu-img convert'. That's what we do now in oVirt 4.2:
> > (a) qemu-img convert produces a 'temporary' collapsed volume
> > (b) make a tar file of the OVf configuration and that 'temporary' volume
> > (c) delete the temporary volume
> >
> > But the fact that we produce that 'temporary' volume obviously slows down
> > the entire operation.
> > It would be much better if we could "open" a stream that we can read from
> > the 'collapsed' form of that chain and stream it directly into the
> > appropriate tar file entry, without extra writes to the storage device.
> >
> > Few months ago people from the oVirt-storage team checked the qemu
> toolset
> > and replied that this capability is not yet provided, therefore we
> > implemented the workaround described above. Apparently, the desired
> ability
> > can also be useful for the flow discussed in this thread so it worth
> asking
> > for it again :)
>
> I think real streaming is unlikely to happen because most image formats
> that QEMU supports aren't made that way. If there is a compelling
> reason, we can consider it, but it would work only with very few target
> formats and as such would have to be separate from existing commands.
>

Real streaming is exactly what we want, and we need it only for qcow2
format,
because it is our preferred way to pack images in ova.

We have 2 possible use cases:

Exporting images or ova files:

image in 

Re: [Qemu-block] [PATCH v4] block: fix QEMU crash with scsi-hd and drive_del

[Greg Kurz]

On Tue, 29 May 2018 22:19:17 +0200
Kevin Wolf  wrote:

> Am 28.05.2018 um 14:03 hat Greg Kurz geschrieben:
> > Removing a drive with drive_del while it is being used to run an I/O
> > intensive workload can cause QEMU to crash.
> > 
> > An AIO flush can yield at some point:
> > 
> > blk_aio_flush_entry()
> >  blk_co_flush(blk)
> >   bdrv_co_flush(blk->root->bs)
> >...
> > qemu_coroutine_yield()
> > 
> > and let the HMP command to run, free blk->root and give control
> > back to the AIO flush:
> > 
> > hmp_drive_del()
> >  blk_remove_bs()
> >   bdrv_root_unref_child(blk->root)
> >child_bs = blk->root->bs
> >bdrv_detach_child(blk->root)
> > bdrv_replace_child(blk->root, NULL)
> >  blk->root->bs = NULL
> > g_free(blk->root) <== blk->root becomes stale
> >bdrv_unref(child_bs)
> > bdrv_delete(child_bs)
> >  bdrv_close()
> >   bdrv_drained_begin()
> >bdrv_do_drained_begin()
> > bdrv_drain_recurse()
> >  aio_poll()
> >   ...
> >   qemu_coroutine_switch()
> > 
> > and the AIO flush completion ends up dereferencing blk->root:
> > 
> >   blk_aio_complete()
> >scsi_aio_complete()
> > blk_get_aio_context(blk)
> >  bs = blk_bs(blk)
> >  ie, bs = blk->root ? blk->root->bs : NULL
> > ^
> > stale
> > 
> > The problem is that we should avoid making block driver graph
> > changes while we have in-flight requests. Let's drain all I/O
> > for this BB before calling bdrv_root_unref_child().
> > 
> > Signed-off-by: Greg Kurz   
> 
> Hmm... It sounded convincing, but 'make check-tests/test-replication'
> fails now. The good news is that with the drain fixes, for which I sent
> v2 today, it passes, so instead of staging it in my block branch, I'll
> put it at the end of my branch for the drain fixes.
> 
> Might take a bit longer than planned until it's in master, sorry.
> 
> Kevin

Works for me :)

Thanks !

--
Greg

Re: [Qemu-block] cmdline: How to connect a SD card to a specific SD controller?

[Philippe Mathieu-Daudé]

On 05/28/2018 01:30 AM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> I'd like to connect a specific SD card to a specific SDHCI from command
> line, and I'm getting a bit lost with command line options.
> 
> I'm using an updated version of this patch, but this is not relevant to
> this thread:
> http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg01914.html
> 
> In the following example, I'd like to connect an UHS-I enabled SD card
> to the first SHDCI of the Exynos4210 board (Nuri).
> 
> I tried:
> 
> $ ./arm-softmmu/qemu-system-arm -M nuri \
>   -device sd-card,id=sd0,uhs=1 \
>   -drive if=sd,id=sd0,driver=null-co,size=4G \
>   -monitor stdio -S

Peter suggested on IRC to use:

  -drive if=none,id=mydrive,... -device sd-card,...,drive=mydrive

which indeed works enough for my tests:

  $ ./arm-softmmu/qemu-system-arm -M nuri \
-drive if=none,id=mydrive,driver=null-co,size=4G \
-device sd-card,id=sd0,uhs=1,drive=mydrive \
-monitor stdio -S

The same issues are still here but don't bother:

(qemu) info qtree
bus: main-system-bus
  type System
  dev: generic-sdhci, id ""
gpio-out "sysbus-irq" 1
sd-spec-version = 2 (0x2)
uhs = 0 (0x0)
capareg = 99090560 (0x5e80080)
maxcurr = 0 (0x0)
pending-insert-quirk = false
dma = ""
mmio 1254/0100
bus: sd-bus
  type sdhci-bus
  dev: sd-card, id "sd0"
drive = "mydrive"
spi = false
uhs = 1 (0x1)
  dev: sd-card, id ""
drive = ""
spi = false
uhs = 0 (0x0)

^ 2 cards

> But then the QTree looks unexpected:
> 
> (qemu) info qtree
> bus: main-system-bus
>   type System
>   dev: generic-sdhci, id ""
> gpio-out "sysbus-irq" 1
> sd-spec-version = 2 (0x2)
> uhs = 0 (0x0)
> capareg = 99090560 (0x5e80080)
> maxcurr = 0 (0x0)
> pending-insert-quirk = false
> dma = ""
> mmio 1254/0100
> bus: sd-bus
>   type sdhci-bus
>   dev: sd-card, id "sd0"
> drive = ""
> spi = false
> uhs = 1 (0x1)
>   dev: sd-card, id ""
> drive = ""
> spi = false
> uhs = 0 (0x0)
> 
>   ^ 2 cards
> 
>   dev: generic-sdhci, id ""
> gpio-out "sysbus-irq" 1
> sd-spec-version = 2 (0x2)
> uhs = 0 (0x0)
> capareg = 99090560 (0x5e80080)
> maxcurr = 0 (0x0)
> pending-insert-quirk = false
> dma = ""
> mmio 1253/0100
> bus: sd-bus
>   type sdhci-bus
>   dev: sd-card, id ""
> drive = ""
> spi = false
> uhs = 0 (0x0)
>   dev: generic-sdhci, id ""
> gpio-out "sysbus-irq" 1
> sd-spec-version = 2 (0x2)
> uhs = 0 (0x0)
> capareg = 99090560 (0x5e80080)
> maxcurr = 0 (0x0)
> pending-insert-quirk = false
> dma = ""
> mmio 1252/0100
> bus: sd-bus
>   type sdhci-bus
>   dev: sd-card, id ""
> drive = ""
> spi = false
> uhs = 0 (0x0)
>   dev: generic-sdhci, id ""
> gpio-out "sysbus-irq" 1
> sd-spec-version = 2 (0x2)
> uhs = 0 (0x0)
> capareg = 99090560 (0x5e80080)
> maxcurr = 0 (0x0)
> pending-insert-quirk = false
> dma = ""
> mmio 1251/0100
> bus: sd-bus
>   type sdhci-bus
>   dev: sd-card, id ""
> drive = "sd0"
> 
> ^--- drive is here
> 
> spi = false
> uhs = 0 (0x0)
> 
> in hw/sd/core.c we have:
> 
> static SDState *get_card(SDBus *sdbus)
> {
> /* We only ever have one child on the bus so just return it */
> BusChild *kid = QTAILQ_FIRST(>qbus.children);
> 
> if (!kid) {
> return NULL;
> }
> return SD_CARD(kid->child);
> }
> 
> Spec v1 allow multiple cards per bus but Spec v2 (and up) restrict to 1
> per bus.
> 
> 
> in exynos4210_init() we have:
> 
> di = drive_get(IF_SD, 0, n);
> blk = di ? blk_by_legacy_dinfo(di) : NULL;
> carddev = qdev_create(qdev_get_child_bus(dev, "sd-bus"),
> TYPE_SD_CARD);
> 
> so there are no restriction in connecting various slaves to the same bus.
> 
> I also wonder about this drive_get(), is this an outdated API?
> 
> Only used in 3 boards:
> 
> $ git grep 'drive_get(IF_SD'
> hw/arm/exynos4210.c:397:di = drive_get(IF_SD, 0, n);
> hw/arm/omap1.c:3990:dinfo = drive_get(IF_SD, 0, 0);
> hw/arm/omap2.c:2489:dinfo = drive_get(IF_SD, 0, 0);
> hw/arm/pxa2xx.c:2098:dinfo = drive_get(IF_SD, 0, 0);
> hw/arm/pxa2xx.c::dinfo = drive_get(IF_SD, 0, 0);
> 
> versus drive_get_next() used in others:
> 
> hw/arm/mcimx7d-sabre.c:70:di = drive_get_next(IF_SD);
> hw/arm/raspi.c:200:di = drive_get_next(IF_SD);
> hw/arm/xilinx_zynq.c:269:di = drive_get_next(IF_SD);
> hw/arm/xlnx-zcu102.c:114:DriveInfo *di = drive_get_next(IF_SD);
> hw/sd/milkymist-memcard.c:279:dinfo = drive_get_next(IF_SD);
> hw/sd/pl181.c:508:dinfo = drive_get_next(IF_SD);
> 

[Qemu-block] [PATCH v2 05/16] qemu-iotests: Add VM.get_qmp_events_filtered()

[Kevin Wolf]

This adds a helper function that returns a list of QMP events that are
already filtered through filter_qmp_event().

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 28159d837a..17aa7c88dc 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -383,6 +383,11 @@ class VM(qtest.QEMUQtestMachine):
 output_list += [key + '=' + obj[key]]
 return ','.join(output_list)
 
+def get_qmp_events_filtered(self, wait=True):
+result = []
+for ev in self.get_qmp_events(wait=wait):
+result.append(filter_qmp_event(ev))
+return result
 
 
 index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
-- 
2.13.6

[Qemu-block] [PATCH v2 02/16] vhdx: Fix vhdx_co_create() return value

[Kevin Wolf]

.bdrv_co_create() is supposed to return 0 on success, but vhdx could
return a positive value instead. Fix this.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Jeff Cody 
---
 block/vhdx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 0b1e21c750..b1ba121bb6 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1951,7 +1951,7 @@ static int coroutine_fn 
vhdx_co_create(BlockdevCreateOptions *opts,
 goto delete_and_exit;
 }
 
-
+ret = 0;
 delete_and_exit:
 blk_unref(blk);
 bdrv_unref(bs);
-- 
2.13.6

[Qemu-block] [PATCH v2 11/16] qemu-iotests: Rewrite 207 for blockdev-create job

[Kevin Wolf]

This rewrites the test case 207 to work with the new x-blockdev-create
job rather than the old synchronous version of the command.

Most of the test cases stay the same as before (the exception being some
improved 'size' options that allow distinguishing which command created
the image), but in order to be able to implement proper job handling,
the test case is rewritten in Python.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/207 | 440 -
 tests/qemu-iotests/207.out | 107 +--
 tests/qemu-iotests/group   |   6 +-
 3 files changed, 257 insertions(+), 296 deletions(-)

diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index f5c77852d1..b595c925a5 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -1,9 +1,11 @@
-#!/bin/bash
+#!/usr/bin/env python
 #
 # Test ssh image creation
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
+# Creator/Owner: Kevin Wolf 
+#
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 2 of the License, or
@@ -18,244 +20,198 @@
 # along with this program.  If not, see .
 #
 
-# creator
-owner=kw...@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-status=1   # failure is the default!
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt raw
-_supported_proto ssh
-_supported_os Linux
-
-function do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-function run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
-  | _filter_qemu | _filter_imgfmt \
-  | _filter_actual_image_size
-}
-
-echo
-echo "=== Successful image creation (defaults) ==="
-echo
-
-run_qemu /dev/null | grep -v "\\^#" | ' +
+'cut -d" " -f3 | base64 -d | sha1sum -b | cut -d" " -f1',
+shell=True).rstrip()
+
+vm.launch()
+blockdev_create(vm, { 'driver': 'ssh',
+  'location': {
+  'path': disk_path,
+  'server': {
+  'host': '127.0.0.1',
+  'port': '22'
+  },
+  'host-key-check': {
+  'mode': 'hash',
+  'type': 'sha1',
+  'hash': 'wrong',
+  }
+  },
+  'size': 2097152 })
+blockdev_create(vm, { 'driver': 'ssh',
+  'location': {
+  'path': disk_path,
+  'server': {
+  'host': '127.0.0.1',
+  'port': '22'
+  },
+  'host-key-check': {
+  'mode': 'hash',
+  'type': 'sha1',
+ 

[Qemu-block] [PATCH v2 10/16] qemu-iotests: Rewrite 206 for blockdev-create job

[Kevin Wolf]

This rewrites the test case 206 to work with the new x-blockdev-create
job rather than the old synchronous version of the command.

All of the test cases stay the same as before, but in order to be able
to implement proper job handling, the test case is rewritten in Python.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/206 | 680 ++---
 tests/qemu-iotests/206.out | 253 ++---
 tests/qemu-iotests/group   |   2 +-
 3 files changed, 414 insertions(+), 521 deletions(-)

diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index 0a18b2b19a..b8cf2e7dca 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -1,9 +1,11 @@
-#!/bin/bash
+#!/usr/bin/env python
 #
 # Test qcow2 and file image creation
 #
 # Copyright (C) 2018 Red Hat, Inc.
 #
+# Creator/Owner: Kevin Wolf 
+#
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
 # the Free Software Foundation; either version 2 of the License, or
@@ -18,419 +20,263 @@
 # along with this program.  If not, see .
 #
 
-# creator
-owner=kw...@redhat.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-here=`pwd`
-status=1   # failure is the default!
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt qcow2
-_supported_proto file
-_supported_os Linux
-
-function do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-function run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \
-  | _filter_qemu | _filter_imgfmt \
-  | _filter_actual_image_size
-}
-
-echo
-echo "=== Successful image creation (defaults) ==="
-echo
-
-size=$((128 * 1024 * 1024))
-
-run_qemu <

[Qemu-block] [PATCH v2 00/16] block: Make blockdev-create a job and stable API

[Kevin Wolf]

This changes the x-blockdev-create QMP command so that it doesn't block
the monitor and the main loop any more, but starts a background job that
performs the image creation.

The basic job as implemented here is all that is necessary to make image
creation asynchronous and to provide a QMP interface that can be marked
stable, but it still lacks a few features that jobs usually provide: The
job will ignore pause commands and it doesn't publish progress yet (so
both current-progress and total-progress stay at 0). These features can
be added later without breaking compatibility.

At the end of the series, the interface is declared stable and the x-
prefix is removed.

v2:
- Made job->error == NULL iff job->ret == 0 [Max]
- Fixed a comment in qmp_blockdev_create() and added a TODO comment for
  AioContext locking at least [Max]
- Many small changes in the tests [Max, Jeff]

Kevin Wolf (16):
  vdi: Fix vdi_co_do_create() return value
  vhdx: Fix vhdx_co_create() return value
  job: Add error message for failing jobs
  block/create: Make x-blockdev-create a job
  qemu-iotests: Add VM.get_qmp_events_filtered()
  qemu-iotests: Add VM.qmp_log()
  qemu-iotests: Add iotests.img_info_log()
  qemu-iotests: Add VM.run_job()
  qemu-iotests: iotests.py helper for non-file protocols
  qemu-iotests: Rewrite 206 for blockdev-create job
  qemu-iotests: Rewrite 207 for blockdev-create job
  qemu-iotests: Rewrite 210 for blockdev-create job
  qemu-iotests: Rewrite 211 for blockdev-create job
  qemu-iotests: Rewrite 212 for blockdev-create job
  qemu-iotests: Rewrite 213 for blockdev-create job
  block/create: Mark blockdev-create stable

 qapi/block-core.json  |  18 +-
 qapi/job.json |   4 +-
 include/qemu/job.h|   7 +-
 block/backup.c|   2 +-
 block/commit.c|   2 +-
 block/create.c|  67 +++--
 block/mirror.c|   2 +-
 block/stream.c|   2 +-
 block/vdi.c   |   1 +
 block/vhdx.c  |   2 +-
 job-qmp.c |   9 +-
 job.c |  16 +-
 tests/test-bdrv-drain.c   |   2 +-
 tests/test-blockjob-txn.c |   2 +-
 tests/test-blockjob.c |   2 +-
 tests/qemu-iotests/206| 680 --
 tests/qemu-iotests/206.out| 253 +---
 tests/qemu-iotests/207| 440 ---
 tests/qemu-iotests/207.out| 107 +++
 tests/qemu-iotests/210| 393 ++--
 tests/qemu-iotests/210.out| 197 
 tests/qemu-iotests/211| 381 ++-
 tests/qemu-iotests/211.out| 133 +
 tests/qemu-iotests/212| 483 +++---
 tests/qemu-iotests/212.out| 191 +++-
 tests/qemu-iotests/213| 520 
 tests/qemu-iotests/213.out| 208 -
 tests/qemu-iotests/iotests.py |  78 +
 28 files changed, 1979 insertions(+), 2223 deletions(-)

-- 
2.13.6

[Qemu-block] [PATCH v2 01/16] vdi: Fix vdi_co_do_create() return value

[Kevin Wolf]

.bdrv_co_create() is supposed to return 0 on success, but vdi could
return a positive value instead. Fix this.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Jeff Cody 
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index 96a22b8e83..668af0a828 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -865,6 +865,7 @@ static int coroutine_fn 
vdi_co_do_create(BlockdevCreateOptions *create_options,
 }
 }
 
+ret = 0;
 exit:
 blk_unref(blk);
 bdrv_unref(bs_file);
-- 
2.13.6

[Qemu-block] [PATCH v2 07/16] qemu-iotests: Add iotests.img_info_log()

[Kevin Wolf]

This adds a filter function to postprocess 'qemu-img info' input
(similar to what _img_info does), and an img_info_log() function that
calls 'qemu-img info' and logs the filtered output.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/iotests.py | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2f54823db6..edcd2bb701 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -109,6 +109,12 @@ def qemu_img_pipe(*args):
 sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
 return subp.communicate()[0]
 
+def img_info_log(filename, filter_path=None):
+output = qemu_img_pipe('info', '-f', imgfmt, filename)
+if not filter_path:
+filter_path = filename
+log(filter_img_info(output, filter_path))
+
 def qemu_io(*args):
 '''Run qemu-io and return the stdout data'''
 args = qemu_io_args + list(args)
@@ -210,6 +216,18 @@ def filter_testfiles(msg):
 prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
 return msg.replace(prefix, 'TEST_DIR/PID-')
 
+def filter_img_info(output, filename):
+lines = []
+for line in output.split('\n'):
+if 'disk size' in line or 'actual-size' in line:
+continue
+line = line.replace(filename, 'TEST_IMG') \
+   .replace(imgfmt, 'IMGFMT')
+line = re.sub('iters: [0-9]+', 'iters: XXX', line)
+line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
+lines.append(line)
+return '\n'.join(lines)
+
 def log(msg, filters=[]):
 for flt in filters:
 msg = flt(msg)
-- 
2.13.6

[Qemu-block] [PATCH v2 04/16] block/create: Make x-blockdev-create a job

[Kevin Wolf]

This changes the x-blockdev-create QMP command so that it doesn't block
the monitor and the main loop any more, but starts a background job that
performs the image creation.

The basic job as implemented here is all that is necessary to make image
creation asynchronous and to provide a QMP interface that can be marked
stable, but it still lacks a few features that jobs usually provide: The
job will ignore pause commands and it doesn't publish more than very
basic progress yet (total-progress is 1 and current-progress advances
from 0 to 1 when the driver callbacks returns). These features can be
added later without breaking compatibility.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 qapi/block-core.json | 14 ++
 qapi/job.json|  4 ++-
 block/create.c   | 67 +---
 tests/qemu-iotests/group | 14 +-
 4 files changed, 66 insertions(+), 33 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ad66ad6f80..eb98596614 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4013,14 +4013,18 @@
 ##
 # @x-blockdev-create:
 #
-# Create an image format on a given node.
-# TODO Replace with something asynchronous (block job?)
+# Starts a job to create an image format on a given node. The job is
+# automatically finalized, but a manual job-dismiss is required.
 #
-# Since: 2.12
+# @job-id:  Identifier for the newly created job.
+#
+# @options: Options for the image creation.
+#
+# Since: 3.0
 ##
 { 'command': 'x-blockdev-create',
-  'data': 'BlockdevCreateOptions',
-  'boxed': true }
+  'data': { 'job-id': 'str',
+'options': 'BlockdevCreateOptions' } }
 
 ##
 # @blockdev-open-tray:
diff --git a/qapi/job.json b/qapi/job.json
index 970124de76..69c1970a58 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -17,10 +17,12 @@
 #
 # @backup: drive backup job type, see "drive-backup"
 #
+# @create: image creation job type, see "x-blockdev-create" (since 3.0)
+#
 # Since: 1.7
 ##
 { 'enum': 'JobType',
-  'data': ['commit', 'stream', 'mirror', 'backup'] }
+  'data': ['commit', 'stream', 'mirror', 'backup', 'create'] }
 
 ##
 # @JobStatus:
diff --git a/block/create.c b/block/create.c
index 8bd8a03719..1a263e4b13 100644
--- a/block/create.c
+++ b/block/create.c
@@ -24,28 +24,51 @@
 
 #include "qemu/osdep.h"
 #include "block/block_int.h"
+#include "qemu/job.h"
 #include "qapi/qapi-commands-block-core.h"
+#include "qapi/qapi-visit-block-core.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/error.h"
 
-typedef struct BlockdevCreateCo {
+typedef struct BlockdevCreateJob {
+Job common;
 BlockDriver *drv;
 BlockdevCreateOptions *opts;
 int ret;
-Error **errp;
-} BlockdevCreateCo;
+Error *err;
+} BlockdevCreateJob;
 
-static void coroutine_fn bdrv_co_create_co_entry(void *opaque)
+static void blockdev_create_complete(Job *job, void *opaque)
 {
-BlockdevCreateCo *cco = opaque;
-cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp);
+BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
+
+job_completed(job, s->ret, s->err);
 }
 
-void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
+static void coroutine_fn blockdev_create_run(void *opaque)
 {
+BlockdevCreateJob *s = opaque;
+
+job_progress_set_remaining(>common, 1);
+s->ret = s->drv->bdrv_co_create(s->opts, >err);
+job_progress_update(>common, 1);
+
+qapi_free_BlockdevCreateOptions(s->opts);
+job_defer_to_main_loop(>common, blockdev_create_complete, NULL);
+}
+
+static const JobDriver blockdev_create_job_driver = {
+.instance_size = sizeof(BlockdevCreateJob),
+.job_type  = JOB_TYPE_CREATE,
+.start = blockdev_create_run,
+};
+
+void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions *options,
+   Error **errp)
+{
+BlockdevCreateJob *s;
 const char *fmt = BlockdevDriver_str(options->driver);
 BlockDriver *drv = bdrv_find_format(fmt);
-Coroutine *co;
-BlockdevCreateCo cco;
 
 /* If the driver is in the schema, we know that it exists. But it may not
  * be whitelisted. */
@@ -55,22 +78,24 @@ void qmp_x_blockdev_create(BlockdevCreateOptions *options, 
Error **errp)
 return;
 }
 
-/* Call callback if it exists */
+/* Error out if the driver doesn't support .bdrv_co_create */
 if (!drv->bdrv_co_create) {
 error_setg(errp, "Driver does not support blockdev-create");
 return;
 }
 
-cco = (BlockdevCreateCo) {
-.drv = drv,
-.opts = options,
-.ret = -EINPROGRESS,
-.errp = errp,
-};
-
-co = qemu_coroutine_create(bdrv_co_create_co_entry, );
-qemu_coroutine_enter(co);
-while (cco.ret == -EINPROGRESS) {
-aio_poll(qemu_get_aio_context(), true);
+/* Create the block job */
+/* TODO Running in the main context. Block drivers need to error out or add
+ * 

Re: [Qemu-block] [ovirt-users] Libvirt ERROR cannot access backing file after importing VM from OpenStack

[Nir Soffer]

On Tue, May 29, 2018 at 10:43 PM Eric Blake  wrote:

> On 05/28/2018 05:27 AM, Arik Hadas wrote:
>
...

> > Few months ago people from the oVirt-storage team checked the qemu
> toolset
> > and replied that this capability is not yet provided, therefore we
> > implemented the workaround described above. Apparently, the desired
> ability
> > can also be useful for the flow discussed in this thread so it worth
> asking
> > for it again :)
>
> You CAN get a logically collapsed view of storage (that is, what the
> guest would see), by using an NBD export of volume V.  Reading from that
> volume will then pull sectors from whichever portion of the chain you
> need.  You can use either qemu-nbd (if no guest is writing to the
> chain), or within a running qemu, you can use nbd-server-start and
> nbd-server-add (over QMP) to get such an NBD server running.


NBD expose the guest data, but we want the qcow2 stream - without
creating a new image.

Nir

Re: [Qemu-block] [ovirt-users] Libvirt ERROR cannot access backing file after importing VM from OpenStack

[Eric Blake]

On 05/29/2018 04:11 PM, Nir Soffer wrote:


I think real streaming is unlikely to happen because most image formats
that QEMU supports aren't made that way. If there is a compelling
reason, we can consider it, but it would work only with very few target
formats and as such would have to be separate from existing commands.



Real streaming is exactly what we want, and we need it only for qcow2
format,
because it is our preferred way to pack images in ova.

We have 2 possible use cases:

Exporting images or ova files:

image in any format -> qemu-img -> [qcow2 byte stream] -> imageio http
server -> http client


image in any format -> qemu-img measure (to learn how large to size qcow2)
then create destination qcow2 file that large and serve it over NBD as 
raw (perhaps using an nbdkit plugin for this part)

then qemu-img convert to destination format qcow2 as NBD client

So, as long as your NBD server (via nbdkit plugin) can talk to imageio 
http server -> http client, and sized things properly according to 
qemu-img measure, then qemu-img can write qcow2 (rather than it's more 
usual raw) over the NBD connection, and when the process is complete, 
the http client will have a fully-populated qcow2 file with no temporary 
files created in the meantime.




Importing images or ova files:

http client -> imageio http server -> [qcow2 byte stream] -> qemu-img ->
image in any format


Same sort of thing - as long as the NBD server is serving a qcow2 file 
as raw data, then the NBD client is interpreting that data as qcow2, 
then qemu-img convert should be able to convert that qcow2 stream into 
any format.


Or, put another way, usually you do the conversion from qcow2 to raw at 
the server, then the client sees raw bytes:


qemu-nbd -f qcow2 file.qcow2 # expose only the guest-visible bytes...
qemu-img convert -f raw nbd://host output # and write those bytes

but in this case, you'd be serving raw bytes at the server, and letting 
the client do qcow2 conversion:


qemu-nbd -f raw file.qcow2 # expose the full qcow2 file...
qemu-img convert -f qcow2 nbd://host output # and extract the guest view

where using nbdkit instead of qemu-nbd as your point of contact with the 
imageio http server may make more sense.




If we will have this, we don't need to create temporary storage space and we
can avoid several image copies in the process.

This will also improve the user experience, avoiding the wait until
a ova is created before the user can start downloading it.

As for OVA files, I think it might be useful to have a tar block driver

instead which would allow you to open a file inside a tar archive (you
could then also directly run an OVA without extracting it first). We
probably wouldn't be able to support resizing images there, but that
should be okay.

If you can create a tar file that reserves space for the image file
without actually writing it, a possible workaround today would be using
the offset/size runtime options of the raw driver to convert directly
into a region inside the tar archive.



What are the offset/size runtime options? I cannot find anything about
them in man qemu-img.


##
# @BlockdevOptionsRaw:
#
# Driver specific block device options for the raw driver.
#
# @offset:  position where the block device starts
# @size:the assumed size of the device
#
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsRaw',
  'base': 'BlockdevOptionsGenericFormat',
  'data': { '*offset': 'int', '*size': 'int' } }


Yeah, it's a pity that "qemu-img create -o help -f raw" has forgotten to 
document them, the way "qemu-img create -o help -f qcow2" does for its 
options, so we should fix that.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-block] [PATCH v2 00/16] block: Make blockdev-create a job and stable API

[Jeff Cody]

On Tue, May 29, 2018 at 10:38:54PM +0200, Kevin Wolf wrote:
> This changes the x-blockdev-create QMP command so that it doesn't block
> the monitor and the main loop any more, but starts a background job that
> performs the image creation.
> 

I'm sure modifying the drivers is fodder for a future series, since this
series lays the groundwork.  But from testing, some image formats (e.g.
qcow2) still block the main loop and monitor when doing blockdev-create
(preallocation:full, and a larger image size is particularly noticeable).

> The basic job as implemented here is all that is necessary to make image
> creation asynchronous and to provide a QMP interface that can be marked
> stable, but it still lacks a few features that jobs usually provide: The
> job will ignore pause commands and it doesn't publish progress yet (so
> both current-progress and total-progress stay at 0). These features can
> be added later without breaking compatibility.
> 
> At the end of the series, the interface is declared stable and the x-
> prefix is removed.
> 
> v2:
> - Made job->error == NULL iff job->ret == 0 [Max]
> - Fixed a comment in qmp_blockdev_create() and added a TODO comment for
>   AioContext locking at least [Max]
> - Many small changes in the tests [Max, Jeff]
> 
> Kevin Wolf (16):
>   vdi: Fix vdi_co_do_create() return value
>   vhdx: Fix vhdx_co_create() return value
>   job: Add error message for failing jobs
>   block/create: Make x-blockdev-create a job
>   qemu-iotests: Add VM.get_qmp_events_filtered()
>   qemu-iotests: Add VM.qmp_log()
>   qemu-iotests: Add iotests.img_info_log()
>   qemu-iotests: Add VM.run_job()
>   qemu-iotests: iotests.py helper for non-file protocols
>   qemu-iotests: Rewrite 206 for blockdev-create job
>   qemu-iotests: Rewrite 207 for blockdev-create job
>   qemu-iotests: Rewrite 210 for blockdev-create job
>   qemu-iotests: Rewrite 211 for blockdev-create job
>   qemu-iotests: Rewrite 212 for blockdev-create job
>   qemu-iotests: Rewrite 213 for blockdev-create job
>   block/create: Mark blockdev-create stable
> 
>  qapi/block-core.json  |  18 +-
>  qapi/job.json |   4 +-
>  include/qemu/job.h|   7 +-
>  block/backup.c|   2 +-
>  block/commit.c|   2 +-
>  block/create.c|  67 +++--
>  block/mirror.c|   2 +-
>  block/stream.c|   2 +-
>  block/vdi.c   |   1 +
>  block/vhdx.c  |   2 +-
>  job-qmp.c |   9 +-
>  job.c |  16 +-
>  tests/test-bdrv-drain.c   |   2 +-
>  tests/test-blockjob-txn.c |   2 +-
>  tests/test-blockjob.c |   2 +-
>  tests/qemu-iotests/206| 680 
> --
>  tests/qemu-iotests/206.out| 253 +---
>  tests/qemu-iotests/207| 440 ---
>  tests/qemu-iotests/207.out| 107 +++
>  tests/qemu-iotests/210| 393 ++--
>  tests/qemu-iotests/210.out| 197 
>  tests/qemu-iotests/211| 381 ++-
>  tests/qemu-iotests/211.out| 133 +
>  tests/qemu-iotests/212| 483 +++---
>  tests/qemu-iotests/212.out| 191 +++-
>  tests/qemu-iotests/213| 520 
>  tests/qemu-iotests/213.out| 208 -
>  tests/qemu-iotests/iotests.py |  78 +
>  28 files changed, 1979 insertions(+), 2223 deletions(-)
> 
> -- 
> 2.13.6
> 

Re: [Qemu-block] cmdline: How to connect a SD card to a specific SD controller?

[Thomas Huth]

On 30.05.2018 01:08, Philippe Mathieu-Daudé wrote:
> On 05/28/2018 01:30 AM, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> I'd like to connect a specific SD card to a specific SDHCI from command
>> line, and I'm getting a bit lost with command line options.
>>
>> I'm using an updated version of this patch, but this is not relevant to
>> this thread:
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg01914.html
>>
>> In the following example, I'd like to connect an UHS-I enabled SD card
>> to the first SHDCI of the Exynos4210 board (Nuri).
>>
>> I tried:
>>
>> $ ./arm-softmmu/qemu-system-arm -M nuri \
>>   -device sd-card,id=sd0,uhs=1 \
>>   -drive if=sd,id=sd0,driver=null-co,size=4G \
>>   -monitor stdio -S
> 
> Peter suggested on IRC to use:
> 
>   -drive if=none,id=mydrive,... -device sd-card,...,drive=mydrive
> 
> which indeed works enough for my tests:
> 
>   $ ./arm-softmmu/qemu-system-arm -M nuri \
> -drive if=none,id=mydrive,driver=null-co,size=4G \
> -device sd-card,id=sd0,uhs=1,drive=mydrive \
> -monitor stdio -S
> 
> The same issues are still here but don't bother:
> 
> (qemu) info qtree
> bus: main-system-bus
>   type System
>   dev: generic-sdhci, id ""
> gpio-out "sysbus-irq" 1
> sd-spec-version = 2 (0x2)
> uhs = 0 (0x0)
> capareg = 99090560 (0x5e80080)
> maxcurr = 0 (0x0)
> pending-insert-quirk = false
> dma = ""
> mmio 1254/0100
> bus: sd-bus
>   type sdhci-bus
>   dev: sd-card, id "sd0"
> drive = "mydrive"
> spi = false
> uhs = 1 (0x1)
>   dev: sd-card, id ""
> drive = ""
> spi = false
> uhs = 0 (0x0)
> 
> ^ 2 cards

Looks like the nuri board always creates an sd-card by default? You
could try to change exynos4210_init() to only create it if
drive_get(IF_SD, ...) is not returning NULL - then it should be possible
to disable it with "-nodefaults".

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 03/16] job: Add error message for failing jobs

[Jeff Cody]

On Tue, May 29, 2018 at 10:38:57PM +0200, Kevin Wolf wrote:
> So far we relied on job->ret and strerror() to produce an error message
> for failed jobs. Not surprisingly, this tends to result in completely
> useless messages.
> 
> This adds a Job.error field that can contain an error string for a
> failing job, and a parameter to job_completed() that sets the field. As
> a default, if NULL is passed, we continue to use strerror(job->ret).
> 
> All existing callers are changed to pass NULL. They can be improved in
> separate patches.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/qemu/job.h|  7 ++-
>  block/backup.c|  2 +-
>  block/commit.c|  2 +-
>  block/mirror.c|  2 +-
>  block/stream.c|  2 +-
>  job-qmp.c |  9 ++---
>  job.c | 16 ++--
>  tests/test-bdrv-drain.c   |  2 +-
>  tests/test-blockjob-txn.c |  2 +-
>  tests/test-blockjob.c |  2 +-
>  10 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 8c8badf75e..1d820530fa 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,6 +124,9 @@ typedef struct Job {
>  /** Estimated progress_current value at the completion of the job */
>  int64_t progress_total;
>  
> +/** Error string for a failed job (NULL if, and only if, job->ret == 0) 
> */
> +char *error;
> +
>  /** ret code passed to job_completed. */
>  int ret;
>  
> @@ -466,13 +469,15 @@ void job_transition_to_ready(Job *job);
>  /**
>   * @job: The job being completed.
>   * @ret: The status code.
> + * @error: The error message for a failing job (only with @ret < 0). If @ret 
> is
> + * negative, but NULL is given for @error, strerror() is used.
>   *
>   * Marks @job as completed. If @ret is non-zero, the job transaction it is 
> part
>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If 
> it
>   * is the last job to complete in its transaction, all jobs in the 
> transaction
>   * move from WAITING to PENDING.
>   */
> -void job_completed(Job *job, int ret);
> +void job_completed(Job *job, int ret, Error *error);
>  
>  /** Asynchronously complete the specified @job. */
>  void job_complete(Job *job, Error **errp);
> diff --git a/block/backup.c b/block/backup.c
> index 4e228e959b..5661435675 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -321,7 +321,7 @@ static void backup_complete(Job *job, void *opaque)
>  {
>  BackupCompleteData *data = opaque;
>  
> -job_completed(job, data->ret);
> +job_completed(job, data->ret, NULL);
>  g_free(data);
>  }
>  
> diff --git a/block/commit.c b/block/commit.c
> index 620666161b..e1814d9693 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque)
>   * bdrv_set_backing_hd() to fail. */
>  block_job_remove_all_bdrv(bjob);
>  
> -job_completed(job, ret);
> +job_completed(job, ret, NULL);
>  g_free(data);
>  
>  /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> diff --git a/block/mirror.c b/block/mirror.c
> index dcb66ec3be..435268bbbf 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -581,7 +581,7 @@ static void mirror_exit(Job *job, void *opaque)
>  blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
>  blk_insert_bs(bjob->blk, mirror_top_bs, _abort);
>  
> -job_completed(job, data->ret);
> +job_completed(job, data->ret, NULL);
>  
>  g_free(data);
>  bdrv_drained_end(src);
> diff --git a/block/stream.c b/block/stream.c
> index a5d6e0cf8a..9264b68a1e 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -93,7 +93,7 @@ out:
>  }
>  
>  g_free(s->backing_file_str);
> -job_completed(job, data->ret);
> +job_completed(job, data->ret, NULL);
>  g_free(data);
>  }
>  
> diff --git a/job-qmp.c b/job-qmp.c
> index 7f38f63336..410775df61 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -136,14 +136,9 @@ void qmp_job_dismiss(const char *id, Error **errp)
>  static JobInfo *job_query_single(Job *job, Error **errp)
>  {
>  JobInfo *info;
> -const char *errmsg = NULL;
>  
>  assert(!job_is_internal(job));
>  
> -if (job->ret < 0) {
> -errmsg = strerror(-job->ret);
> -}
> -
>  info = g_new(JobInfo, 1);
>  *info = (JobInfo) {
>  .id = g_strdup(job->id),
> @@ -151,8 +146,8 @@ static JobInfo *job_query_single(Job *job, Error **errp)
>  .status = job->status,
>  .current_progress   = job->progress_current,
>  .total_progress = job->progress_total,
> -.has_error  = !!errmsg,
> -.error  = g_strdup(errmsg),
> +.has_error  = !!job->error,
> +.error  = g_strdup(job->error),
>  };
>  
>  return info;
> diff --git a/job.c b/job.c
> index f026661b0f..84e140238b 100644

Re: [Qemu-block] [PATCH v2 04/16] block/create: Make x-blockdev-create a job

[Jeff Cody]

On Tue, May 29, 2018 at 10:38:58PM +0200, Kevin Wolf wrote:
> This changes the x-blockdev-create QMP command so that it doesn't block
> the monitor and the main loop any more, but starts a background job that
> performs the image creation.
> 
> The basic job as implemented here is all that is necessary to make image
> creation asynchronous and to provide a QMP interface that can be marked
> stable, but it still lacks a few features that jobs usually provide: The
> job will ignore pause commands and it doesn't publish more than very
> basic progress yet (total-progress is 1 and current-progress advances
> from 0 to 1 when the driver callbacks returns). These features can be
> added later without breaking compatibility.
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Max Reitz 

Reviewed-by: Jeff Cody 

> ---
>  qapi/block-core.json | 14 ++
>  qapi/job.json|  4 ++-
>  block/create.c   | 67 
> +---
>  tests/qemu-iotests/group | 14 +-
>  4 files changed, 66 insertions(+), 33 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ad66ad6f80..eb98596614 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4013,14 +4013,18 @@
>  ##
>  # @x-blockdev-create:
>  #
> -# Create an image format on a given node.
> -# TODO Replace with something asynchronous (block job?)
> +# Starts a job to create an image format on a given node. The job is
> +# automatically finalized, but a manual job-dismiss is required.
>  #
> -# Since: 2.12
> +# @job-id:  Identifier for the newly created job.
> +#
> +# @options: Options for the image creation.
> +#
> +# Since: 3.0
>  ##
>  { 'command': 'x-blockdev-create',
> -  'data': 'BlockdevCreateOptions',
> -  'boxed': true }
> +  'data': { 'job-id': 'str',
> +'options': 'BlockdevCreateOptions' } }
>  
>  ##
>  # @blockdev-open-tray:
> diff --git a/qapi/job.json b/qapi/job.json
> index 970124de76..69c1970a58 100644
> --- a/qapi/job.json
> +++ b/qapi/job.json
> @@ -17,10 +17,12 @@
>  #
>  # @backup: drive backup job type, see "drive-backup"
>  #
> +# @create: image creation job type, see "x-blockdev-create" (since 3.0)
> +#
>  # Since: 1.7
>  ##
>  { 'enum': 'JobType',
> -  'data': ['commit', 'stream', 'mirror', 'backup'] }
> +  'data': ['commit', 'stream', 'mirror', 'backup', 'create'] }
>  
>  ##
>  # @JobStatus:
> diff --git a/block/create.c b/block/create.c
> index 8bd8a03719..1a263e4b13 100644
> --- a/block/create.c
> +++ b/block/create.c
> @@ -24,28 +24,51 @@
>  
>  #include "qemu/osdep.h"
>  #include "block/block_int.h"
> +#include "qemu/job.h"
>  #include "qapi/qapi-commands-block-core.h"
> +#include "qapi/qapi-visit-block-core.h"
> +#include "qapi/clone-visitor.h"
>  #include "qapi/error.h"
>  
> -typedef struct BlockdevCreateCo {
> +typedef struct BlockdevCreateJob {
> +Job common;
>  BlockDriver *drv;
>  BlockdevCreateOptions *opts;
>  int ret;
> -Error **errp;
> -} BlockdevCreateCo;
> +Error *err;
> +} BlockdevCreateJob;
>  
> -static void coroutine_fn bdrv_co_create_co_entry(void *opaque)
> +static void blockdev_create_complete(Job *job, void *opaque)
>  {
> -BlockdevCreateCo *cco = opaque;
> -cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp);
> +BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
> +
> +job_completed(job, s->ret, s->err);
>  }
>  
> -void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
> +static void coroutine_fn blockdev_create_run(void *opaque)
>  {
> +BlockdevCreateJob *s = opaque;
> +
> +job_progress_set_remaining(>common, 1);
> +s->ret = s->drv->bdrv_co_create(s->opts, >err);
> +job_progress_update(>common, 1);
> +
> +qapi_free_BlockdevCreateOptions(s->opts);
> +job_defer_to_main_loop(>common, blockdev_create_complete, NULL);
> +}
> +
> +static const JobDriver blockdev_create_job_driver = {
> +.instance_size = sizeof(BlockdevCreateJob),
> +.job_type  = JOB_TYPE_CREATE,
> +.start = blockdev_create_run,
> +};
> +
> +void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions 
> *options,
> +   Error **errp)
> +{
> +BlockdevCreateJob *s;
>  const char *fmt = BlockdevDriver_str(options->driver);
>  BlockDriver *drv = bdrv_find_format(fmt);
> -Coroutine *co;
> -BlockdevCreateCo cco;
>  
>  /* If the driver is in the schema, we know that it exists. But it may not
>   * be whitelisted. */
> @@ -55,22 +78,24 @@ void qmp_x_blockdev_create(BlockdevCreateOptions 
> *options, Error **errp)
>  return;
>  }
>  
> -/* Call callback if it exists */
> +/* Error out if the driver doesn't support .bdrv_co_create */
>  if (!drv->bdrv_co_create) {
>  error_setg(errp, "Driver does not support blockdev-create");
>  return;
>  }
>  
> -cco = (BlockdevCreateCo) {
> -.drv = drv,
> -   

Re: [Qemu-block] [PATCH v2 05/16] qemu-iotests: Add VM.get_qmp_events_filtered()

[Jeff Cody]

On Tue, May 29, 2018 at 10:38:59PM +0200, Kevin Wolf wrote:
> This adds a helper function that returns a list of QMP events that are
> already filtered through filter_qmp_event().
> 
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Max Reitz 

Reviewed-by: Jeff Cody 

> ---
>  tests/qemu-iotests/iotests.py | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 28159d837a..17aa7c88dc 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -383,6 +383,11 @@ class VM(qtest.QEMUQtestMachine):
>  output_list += [key + '=' + obj[key]]
>  return ','.join(output_list)
>  
> +def get_qmp_events_filtered(self, wait=True):
> +result = []
> +for ev in self.get_qmp_events(wait=wait):
> +result.append(filter_qmp_event(ev))
> +return result
>  
>  
>  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH v2 07/16] qemu-iotests: Add iotests.img_info_log()

[Jeff Cody]

On Tue, May 29, 2018 at 10:39:01PM +0200, Kevin Wolf wrote:
> This adds a filter function to postprocess 'qemu-img info' input
> (similar to what _img_info does), and an img_info_log() function that
> calls 'qemu-img info' and logs the filtered output.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  tests/qemu-iotests/iotests.py | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 2f54823db6..edcd2bb701 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -109,6 +109,12 @@ def qemu_img_pipe(*args):
>  sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
> '.join(qemu_img_args + list(args
>  return subp.communicate()[0]
>  
> +def img_info_log(filename, filter_path=None):
> +output = qemu_img_pipe('info', '-f', imgfmt, filename)
> +if not filter_path:
> +filter_path = filename
> +log(filter_img_info(output, filter_path))
> +
>  def qemu_io(*args):
>  '''Run qemu-io and return the stdout data'''
>  args = qemu_io_args + list(args)
> @@ -210,6 +216,18 @@ def filter_testfiles(msg):
>  prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>  return msg.replace(prefix, 'TEST_DIR/PID-')
>  
> +def filter_img_info(output, filename):
> +lines = []
> +for line in output.split('\n'):
> +if 'disk size' in line or 'actual-size' in line:
> +continue
> +line = line.replace(filename, 'TEST_IMG') \
> +   .replace(imgfmt, 'IMGFMT')
> +line = re.sub('iters: [0-9]+', 'iters: XXX', line)
> +line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
> ----', line)
> +lines.append(line)
> +return '\n'.join(lines)
> +
>  def log(msg, filters=[]):
>  for flt in filters:
>  msg = flt(msg)
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH v2 09/16] qemu-iotests: iotests.py helper for non-file protocols

[Jeff Cody]

On Tue, May 29, 2018 at 10:39:03PM +0200, Kevin Wolf wrote:
> This adds two helper functions that are useful for test cases that make
> use of a non-file protocol (specifically ssh).
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  tests/qemu-iotests/iotests.py | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 8b612cb891..bc8f404ac2 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -303,6 +303,13 @@ def file_path(*names):
>  
>  return paths[0] if len(paths) == 1 else paths
>  
> +def remote_filename(path):
> +if imgproto == 'file':
> +return path
> +elif imgproto == 'ssh':
> +return "ssh://127.0.0.1%s" % (path)
> +else:
> +raise Exception("Protocol %s not supported" % (imgproto))
>  
>  class VM(qtest.QEMUQtestMachine):
>  '''A QEMU VM'''
> @@ -601,6 +608,16 @@ def verify_image_format(supported_fmts=[], 
> unsupported_fmts=[]):
>  if not_sup or (imgfmt in unsupported_fmts):
>  notrun('not suitable for this image format: %s' % imgfmt)
>  
> +def verify_protocol(supported=[], unsupported=[]):
> +assert not (supported and unsupported)
> +
> +if 'generic' in supported:
> +return
> +
> +not_sup = supported and (imgproto not in supported)
> +if not_sup or (imgproto in unsupported):
> +notrun('not suitable for this protocol: %s' % imgproto)
> +
>  def verify_platform(supported_oses=['linux']):
>  if True not in [sys.platform.startswith(x) for x in supported_oses]:
>  notrun('not suitable for this OS: %s' % sys.platform)
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH v2 08/16] qemu-iotests: Add VM.run_job()

[Jeff Cody]

On Tue, May 29, 2018 at 10:39:02PM +0200, Kevin Wolf wrote:
> Add an iotests.py function that runs a job and only returns when it is
> destroyed. An error is logged when the job failed and job-finalize and
> job-dismiss commands are issued if necessary.
> 
> Signed-off-by: Kevin Wolf 

Reviewed-by: Jeff Cody 

> ---
>  tests/qemu-iotests/iotests.py | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index edcd2bb701..8b612cb891 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -418,6 +418,25 @@ class VM(qtest.QEMUQtestMachine):
>  log(str(result), filters)
>  return result
>  
> +def run_job(self, job, auto_finalize=True, auto_dismiss=False):
> +while True:
> +for ev in self.get_qmp_events_filtered(wait=True):
> +if ev['event'] == 'JOB_STATUS_CHANGE':
> +status = ev['data']['status']
> +if status == 'aborting':
> +result = self.qmp('query-jobs')
> +for j in result['return']:
> +if j['id'] == job:
> +log('Job failed: %s' % (j['error']))
> +elif status == 'pending' and not auto_finalize:
> +self.qmp_log('job-finalize', id=job)
> +elif status == 'concluded' and not auto_dismiss:
> +self.qmp_log('job-dismiss', id=job)
> +elif status == 'null':
> +return
> +else:
> +iotests.log(ev)
> +
>  
>  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>  
> -- 
> 2.13.6
> 

Re: [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> So far we relied on job->ret and strerror() to produce an error message
> for failed jobs. Not surprisingly, this tends to result in completely
> useless messages.
> 
> This adds a Job.error field that can contain an error string for a
> failing job, and a parameter to job_completed() that sets the field. As
> a default, if NULL is passed, we continue to use strerror(job->ret).
> 
> All existing callers are changed to pass NULL. They can be improved in
> separate patches.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/qemu/job.h |  7 ++-
>  block/backup.c |  2 +-
>  block/commit.c |  2 +-
>  block/mirror.c |  2 +-
>  block/stream.c |  2 +-
>  job-qmp.c  |  9 ++---
>  job.c  | 15 +--
>  7 files changed, 25 insertions(+), 14 deletions(-)

There are some tests that call job_completed().  Those should be updated
as well.

> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 8c8badf75e..b2e1dd00b9 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,6 +124,9 @@ typedef struct Job {
>  /** Estimated progress_current value at the completion of the job */
>  int64_t progress_total;
>  
> +/** Error string for a failed job (NULL if job->ret == 0) */
> +char *error;
> +

I think we should bind this directly to job->ret, i.e. this is NULL if
and only if job->ret == 0.  (Just because more constraints tend to make
things nicer.  And also because if you don't, then qmp_job_dismiss()
cannot assume that job->error is set on error, which would be a change
in behavior.)

>  /** ret code passed to job_completed. */
>  int ret;
> 
[...]

> diff --git a/job.c b/job.c
> index f026661b0f..fc39eaaa5e 100644
> --- a/job.c
> +++ b/job.c

job_prepare() (called by job_do_finalize() through job_txn_apply()) may
set job->ret.  If it does set it to a negative value, job_do_finalize()
will call job_completed_txn_abort(), which will eventually invoke
job_finalize_single(), which then runs job_update_rc() on the job.  This
is a bit too much code between setting job->ret and job->error for my
taste, I'd rather set job->error much sooner (e.g. by calling
job_update_rc() directly in job_prepare(), which I don't think would
change anything).

But if you think that this is just fine, then OK, because I don't think
the user can do QMP queries in between (it would still break the iff
relationship between job->ret and job->error, which I find desirable).

[...]

> @@ -661,6 +662,9 @@ static void job_update_rc(Job *job)
>  }
>  if (job->ret) {
>  job_state_transition(job, JOB_STATUS_ABORTING);
> +if (!job->error) {
> +job->error = g_strdup(strerror(-job->ret));
> +}

If we do use an iff relationship between job->ret and job->error, this
should probably be inserted before job_state_transition().

Max

>  }
>  }



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 14/21] hw: Do not include "sysemu/blockdev.h" if it is not necessary

[Cornelia Huck]

On Mon, 28 May 2018 20:27:12 -0300
Philippe Mathieu-Daudé  wrote:

> Remove those unneeded includes to speed up the compilation
> process a little bit.
> 
> Code change produced with:
> 
> $ git grep '#include "sysemu/blockdev.h"' | \
>   cut -d: -f-1 | \
>   xargs egrep -L 
> "(BlockInterfaceType|DriveInfo|drive_get|blk_legacy_dinfo|blockdev_mark_auto_del)"
>  | \
>   xargs sed -i.bak '/#include "sysemu\/blockdev.h"/d'
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/block/m25p80.c  | 1 -
>  hw/block/onenand.c | 1 -
>  hw/i386/xen/xen-mapcache.c | 1 -
>  hw/s390x/virtio-ccw.c  | 1 -
>  hw/scsi/scsi-generic.c | 1 -
>  hw/sd/sdhci.c  | 1 -
>  hw/usb/dev-storage.c   | 1 -
>  monitor.c  | 1 -
>  8 files changed, 8 deletions(-)

Acked-by: Cornelia Huck 

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Max Reitz]

On 2018-05-29 08:44, Kevin Wolf wrote:
> Am 28.05.2018 um 23:25 hat Richard W.M. Jones geschrieben:
>> On Mon, May 28, 2018 at 10:20:54PM +0100, Richard W.M. Jones wrote:
>>> On Mon, May 28, 2018 at 08:38:33PM +0200, Kevin Wolf wrote:
 Just accessing the image file within a tar archive is possible and we
 could write a block driver for that (I actually think we should do
 this), but it restricts you because certain operations like resizing
 aren't really possible in tar. Unfortunately, resizing is a really
 common operation for non-raw image formats.
>>>
>>> We do this already in virt-v2v (using file.offset and file.size
>>> parameters in the raw driver).
>>>
>>> For virt-v2v we only need to read the source so resizing isn't an
>>> issue.  For most of the cases we're talking about the downloaded image
>>> would also be a template / base image, so I suppose only reading would
>>> be required too.
>>>
>>> I also wrote an nbdkit tar file driver (supports writes, but not
>>> resizing).
>>> https://manpages.debian.org/testing/nbdkit-plugin-perl/nbdkit-tar-plugin.1.en.html
>>
>> I should add the other thorny issue with OVA files is that the
>> metadata contains a checksum (SHA1 or SHA256) of the disk images.  If
>> you modify the disk images in-place in the tar file then you need to
>> recalculate those.
> 
> All of this means that OVA isn't really well suited to be used as a
> native format for VM configuration + images. It's just for sharing
> read-only images that are converted into another native format before
> they are used.
> 
> Which is probably fair for the use case it was made for, but means that
> we need something else to solve our problem.

Maybe we should first narrow down our problem.  Maybe you have done that
already, but I'm quite in the dark still.

The original problem was that you need to supply a machine type to qemu,
and that multiple common architectures now have multiple machine types
and not necessarily all work with a single image.  So far so good, but I
have two issues here already:

(1) How is qemu supposed to interpret that information?  If it's stored
in the image file, I don't see a nice way of retrieving it before the
machine is initialized, at least not with qemu's current architecture.
Once we support configuring qemu solely through QMP, sure, you can do a
blockdev-add and then build the machine accordingly.  But that is not
here today, and I'm not sure this is a good idea either, because that
would mean automagic defaults for the machine-building QMP commands
derived from the blockdev-add earlier, which should get a plain "No".
Also, having to use QMP to build your machine wouldn't make anything
easier; at least not easier than just supplying a configuration file
along with the image.

(Building the magic into -blockdev might be less horrible, but such
magic (adding block devices influences machine defaults) to me still
doesn't seem worth not having to supply a config file along with the
disk image.)

(2) Again, I personally just really don't like saving such information
in a disk image.  One actual argument I can bring up for that distaste
is this: Suppose, you have multiple images attached to your VM.  Now the
VM wants to store the machine type.  Where does it go?  Into all of
them?  But some of those images may only contain data and might be
intended to be shared between multiple VMs.  So those shouldn't receive
the mark.  Only disks with binaries should receive them.
But what if those binaries are just cross-compiled binaries for some
other VM?  Oh no, so not even binaries are a sure indicator...  So I
have no idea where the information is supposed to be stored.  In any
case, "the first image" just gets an outright "no" from me, and "all
images" gets an "I don't think this is a good idea".

Loading is fun, too.  OK, so you attach multiple disk images to a VM.
Oops, they have varying machine type information...  Now what?  Use the
information from the first one?  Definitely no.  Just ignore all of the
information in such a case and have the user supply the machine type
again?  Possible, but it seems weird to me that qemu would usually guess
the machine type, but once you attach some random other image to it, it
suddenly fails to do that.  But maybe it's just me who thinks this is weird.


OK, so let's go a step further.  We have stored the machine type
information in order to not have to supply a config file with the qcow2
image -- because if we did, it could just contain the machine type and
that would be it.

So to me it follows naturally that just storing the machine type doesn't
make much sense if we cannot also store more VM configuration in a qcow2
file, because I don't see why you should be able to ship an image
without a config file only if all you need to supply is a machine type.
Often, you also need to supply how much memory the VM needs (which
depends on the OS on the image) or what storage controller to use (does
the OS have virtio drivers? 

Re: [Qemu-block] [PATCH v4 12/21] hw: Do not include "sysemu/block-backend.h" if it is not necessary

[Cornelia Huck]

On Mon, 28 May 2018 20:27:10 -0300
Philippe Mathieu-Daudé  wrote:

> Remove those unneeded includes to speed up the compilation
> process a little bit. (Continue 7eceff5b5a1fa cleanup)
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/arm/collie.c  | 1 -
>  hw/arm/gumstix.c | 1 -
>  hw/arm/mainstone.c   | 1 -
>  hw/arm/nseries.c | 1 -
>  hw/arm/omap1.c   | 1 -
>  hw/arm/omap2.c   | 1 -
>  hw/arm/omap_sx1.c| 1 -
>  hw/arm/pxa2xx.c  | 1 -
>  hw/arm/spitz.c   | 1 -
>  hw/arm/versatilepb.c | 1 -
>  hw/arm/vexpress.c| 1 -
>  hw/arm/virt.c| 1 -
>  hw/arm/xilinx_zynq.c | 1 -
>  hw/arm/z2.c  | 1 -
>  hw/block/dataplane/virtio-blk.c  | 1 -
>  hw/block/virtio-blk.c| 1 -
>  hw/core/qdev-properties.c| 1 -
>  hw/cris/axis_dev88.c | 1 -
>  hw/display/tc6393xb.c| 1 -
>  hw/ide/pci.c | 1 -
>  hw/ide/via.c | 1 -
>  hw/isa/isa-superio.c | 1 -
>  hw/lm32/lm32_boards.c| 1 -
>  hw/lm32/milkymist.c  | 1 -
>  hw/microblaze/petalogix_ml605_mmu.c  | 1 -
>  hw/microblaze/petalogix_s3adsp1800_mmu.c | 1 -
>  hw/mips/mips_r4k.c   | 1 -
>  hw/ppc/spapr.c   | 1 -
>  hw/ppc/virtex_ml507.c| 2 --
>  hw/s390x/virtio-ccw.c| 1 -
>  hw/scsi/mptsas.c | 1 -
>  hw/sd/pl181.c| 1 -
>  hw/sd/sdhci.c| 1 -
>  hw/sd/ssi-sd.c   | 1 -
>  hw/sh4/r2d.c | 1 -
>  hw/virtio/virtio-pci.c   | 1 -
>  hw/xen/xen_devconfig.c   | 1 -
>  hw/xtensa/xtfpga.c   | 1 -
>  38 files changed, 39 deletions(-)

Acked-by: Cornelia Huck 

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Kevin Wolf]

Am 29.05.2018 um 11:23 hat Max Reitz geschrieben:
> On 2018-05-28 21:09, Kevin Wolf wrote:
> > Am 28.05.2018 um 20:44 hat Max Reitz geschrieben:
> >> On 2018-05-28 20:38, Kevin Wolf wrote:
> >>> Am 28.05.2018 um 20:30 hat Richard W.M. Jones geschrieben:
>  On Mon, May 28, 2018 at 08:10:32PM +0200, Max Reitz wrote:
> > As someone who is just naive and doesn't see the big picture, I don't
> > see what's wrong with using a tar file that contains the image and
> > additional data.
> 
>  FWIW an OVA file is exactly this: an uncompressed tar file containing
>  disk image(s) and metadata.
> >>>
> >>> If we combine VM configuration and the disk image this way, I would
> >>> still want to directly use that combined thing without having to extract
> >>> its components first.
> >>>
> >>> Just accessing the image file within a tar archive is possible and we
> >>> could write a block driver for that (I actually think we should do
> >>> this), but it restricts you because certain operations like resizing
> >>> aren't really possible in tar. Unfortunately, resizing is a really
> >>> common operation for non-raw image formats.
> >>>
> >>> And if I think of a file format that can contain several different
> >>> things that can be individually resized etc., I end up with qcow2 in the
> >>> simple case or a full file system in the more complex case.
> >>
> >> Well, you end up with VMDK.
> > 
> > I don't think VMDK can save several different objects? It can have some
> > metadata in the descriptor, and it can spread the contents of a single
> > object across multiple files (with extents), but I don't think it has
> > something comparable to e.g. qcow2 snapshots, which are separate objects
> > with an individual size that can dynamically change.
> 
> Right, I tried to be funny and was over-simplifying in the process.
> 
> What I meant is: You end up with an image format that is spread on a
> filesystem, like VMDK is (usually).  Then you have some metadata
> descriptor file that describes the rest and multiple data object files.
> 
> (For completeness's sake: And you can use an external or an internal
> filesystem, that is, use multiple files (like VMDK) or have an internal
> filesystem (like tar, except tar doesn't allow fragmentation).)

Let's call the libvirt XML the image file and the qcow2 files its
data object files and we're done?

I'm afraid spreading things across multiple files doesn't meet the
requirements for the problem at hand, though...

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Max Reitz]

On 2018-05-28 21:09, Kevin Wolf wrote:
> Am 28.05.2018 um 20:44 hat Max Reitz geschrieben:
>> On 2018-05-28 20:38, Kevin Wolf wrote:
>>> Am 28.05.2018 um 20:30 hat Richard W.M. Jones geschrieben:
 On Mon, May 28, 2018 at 08:10:32PM +0200, Max Reitz wrote:
> As someone who is just naive and doesn't see the big picture, I don't
> see what's wrong with using a tar file that contains the image and
> additional data.

 FWIW an OVA file is exactly this: an uncompressed tar file containing
 disk image(s) and metadata.
>>>
>>> If we combine VM configuration and the disk image this way, I would
>>> still want to directly use that combined thing without having to extract
>>> its components first.
>>>
>>> Just accessing the image file within a tar archive is possible and we
>>> could write a block driver for that (I actually think we should do
>>> this), but it restricts you because certain operations like resizing
>>> aren't really possible in tar. Unfortunately, resizing is a really
>>> common operation for non-raw image formats.
>>>
>>> And if I think of a file format that can contain several different
>>> things that can be individually resized etc., I end up with qcow2 in the
>>> simple case or a full file system in the more complex case.
>>
>> Well, you end up with VMDK.
> 
> I don't think VMDK can save several different objects? It can have some
> metadata in the descriptor, and it can spread the contents of a single
> object across multiple files (with extents), but I don't think it has
> something comparable to e.g. qcow2 snapshots, which are separate objects
> with an individual size that can dynamically change.

Right, I tried to be funny and was over-simplifying in the process.

What I meant is: You end up with an image format that is spread on a
filesystem, like VMDK is (usually).  Then you have some metadata
descriptor file that describes the rest and multiple data object files.

(For completeness's sake: And you can use an external or an internal
filesystem, that is, use multiple files (like VMDK) or have an internal
filesystem (like tar, except tar doesn't allow fragmentation).)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [ovirt-users] Libvirt ERROR cannot access backing file after importing VM from OpenStack

[Kevin Wolf]

Am 29.05.2018 um 11:27 hat Richard W.M. Jones geschrieben:
> On Mon, May 28, 2018 at 01:27:21PM +0300, Arik Hadas wrote:
> > Let me demonstrate briefly the flow for OVA:
> > Let's say that we have a VM that is based on a template and has one disk
> > and one snapshot, so its volume-chain would be:
> > T -> S -> V
> > (V is the volume the VM writes to, S is the backing file of V and T is the
> > backing file of S).
> > When exporting that VM to an OVA file we want the produced tar file to be
> > comprised of:
> > (1) OVF configuration
> > (2) single disk volume (preferably qcow).
> > 
> > So we need to collapse T, S, V into a single volume.
> > Sure, we can do 'qemu-img convert'. That's what we do now in oVirt 4.2:
> > (a) qemu-img convert produces a 'temporary' collapsed volume
> > (b) make a tar file of the OVf configuration and that 'temporary' volume
> > (c) delete the temporary volume
> > 
> > But the fact that we produce that 'temporary' volume obviously slows down
> > the entire operation.
> > It would be much better if we could "open" a stream that we can read from
> > the 'collapsed' form of that chain and stream it directly into the
> > appropriate tar file entry, without extra writes to the storage device.
> 
> A custom nbdkit plugin is possible here.  In fact it's almost possible
> using the existing nbdkit-tar-plugin[1], except that it doesn't
> support resizing the tarball so you'd need a way to predict the size
> of the final qcow2 file.

I think you can predict the size with 'qemu-img measure'.

But how do you create a tar archive that contains an empty file of the
right size without actually processing and writing gigabytes of zero
bytes? Is there an existing tool that can do that or would you have to
write your own?

> The main difficulty for modifying nbdkit-tar-plugin is working out how
> to resize tar files.  If you can do that then it's likely just a few
> lines of code.

This sounds impossible to do when the tar archive needs to stay
consistent at all times.

Kevin

Re: [Qemu-block] [PATCH 02/14] vhdx: Fix vhdx_co_create() return value

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> .bdrv_co_create() is supposed to return 0 on success, but vhdx could
> return a positive value instead. Fix this.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/vhdx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [ovirt-users] Libvirt ERROR cannot access backing file after importing VM from OpenStack

[Richard W.M. Jones]

On Mon, May 28, 2018 at 01:27:21PM +0300, Arik Hadas wrote:
> Let me demonstrate briefly the flow for OVA:
> Let's say that we have a VM that is based on a template and has one disk
> and one snapshot, so its volume-chain would be:
> T -> S -> V
> (V is the volume the VM writes to, S is the backing file of V and T is the
> backing file of S).
> When exporting that VM to an OVA file we want the produced tar file to be
> comprised of:
> (1) OVF configuration
> (2) single disk volume (preferably qcow).
> 
> So we need to collapse T, S, V into a single volume.
> Sure, we can do 'qemu-img convert'. That's what we do now in oVirt 4.2:
> (a) qemu-img convert produces a 'temporary' collapsed volume
> (b) make a tar file of the OVf configuration and that 'temporary' volume
> (c) delete the temporary volume
> 
> But the fact that we produce that 'temporary' volume obviously slows down
> the entire operation.
> It would be much better if we could "open" a stream that we can read from
> the 'collapsed' form of that chain and stream it directly into the
> appropriate tar file entry, without extra writes to the storage device.

A custom nbdkit plugin is possible here.  In fact it's almost possible
using the existing nbdkit-tar-plugin[1], except that it doesn't
support resizing the tarball so you'd need a way to predict the size
of the final qcow2 file.

The main difficulty for modifying nbdkit-tar-plugin is working out how
to resize tar files.  If you can do that then it's likely just a few
lines of code.

Rich.

[1] 
https://manpages.debian.org/testing/nbdkit-plugin-perl/nbdkit-tar-plugin.1.en.html
https://github.com/libguestfs/nbdkit/blob/master/plugins/tar/tar.pl

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-block] [PATCH 01/14] vdi: Fix vdi_co_do_create() return value

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> .bdrv_co_create() is supposed to return 0 on success, but vdi could
> return a positive value instead. Fix this.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/vdi.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v4 20/21] hw/ide: Remove unused include

[Thomas Huth]

On 29.05.2018 01:27, Philippe Mathieu-Daudé wrote:
> There is no need to include pci.h in this file.
> (Continue f23c81073a cleanup).
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/ide/core.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 866c659498..cc9ca28c33 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -25,7 +25,6 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
> -#include "hw/pci/pci.h"
>  #include "hw/isa/isa.h"
>  #include "qemu/error-report.h"
>  #include "qemu/timer.h"
> 

Reviewed-by: Thomas Huth 

Re: [Qemu-block] [PATCH 04/14] block/create: Make x-blockdev-create a job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This changes the x-blockdev-create QMP command so that it doesn't block
> the monitor and the main loop any more, but starts a background job that
> performs the image creation.
> 
> The basic job as implemented here is all that is necessary to make image
> creation asynchronous and to provide a QMP interface that can be marked
> stable, but it still lacks a few features that jobs usually provide: The
> job will ignore pause commands and it doesn't publish progress yet (so
> both current-progress and total-progress stay at 0). These features can
> be added later without breaking compatibility.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 14 +++
>  qapi/job.json|  4 +++-
>  block/create.c   | 61 
> 
>  tests/qemu-iotests/group | 14 ++-
>  4 files changed, 61 insertions(+), 32 deletions(-)

[...]

> diff --git a/block/create.c b/block/create.c
> index 8bd8a03719..87fdab3b72 100644
> --- a/block/create.c
> +++ b/block/create.c
> @@ -24,28 +24,49 @@
>  
>  #include "qemu/osdep.h"
>  #include "block/block_int.h"
> +#include "qemu/job.h"
>  #include "qapi/qapi-commands-block-core.h"
> +#include "qapi/qapi-visit-block-core.h"
> +#include "qapi/clone-visitor.h"
>  #include "qapi/error.h"
>  
> -typedef struct BlockdevCreateCo {
> +typedef struct BlockdevCreateJob {
> +Job common;
>  BlockDriver *drv;
>  BlockdevCreateOptions *opts;
>  int ret;
> -Error **errp;
> -} BlockdevCreateCo;
> +Error *err;
> +} BlockdevCreateJob;
>  
> -static void coroutine_fn bdrv_co_create_co_entry(void *opaque)
> +static void blockdev_create_complete(Job *job, void *opaque)
>  {
> -BlockdevCreateCo *cco = opaque;
> -cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp);
> +BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
> +
> +job_completed(job, s->ret, s->err);
>  }
>  
> -void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp)
> +static void coroutine_fn blockdev_create_run(void *opaque)
>  {
> +BlockdevCreateJob *s = opaque;
> +
> +s->ret = s->drv->bdrv_co_create(s->opts, >err);
> +
> +qapi_free_BlockdevCreateOptions(s->opts);
> +job_defer_to_main_loop(>common, blockdev_create_complete, NULL);

Better be safe than sorry, but probably not strictly necessary
considering the job is always run in the main loop anyway (more on that
below, though).

> +}
> +
> +static const JobDriver blockdev_create_job_driver = {
> +.instance_size = sizeof(BlockdevCreateJob),
> +.job_type  = JOB_TYPE_CREATE,
> +.start = blockdev_create_run,
> +};
> +
> +void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions 
> *options,
> +   Error **errp)
> +{
> +BlockdevCreateJob *s;
>  const char *fmt = BlockdevDriver_str(options->driver);
>  BlockDriver *drv = bdrv_find_format(fmt);
> -Coroutine *co;
> -BlockdevCreateCo cco;
>  
>  /* If the driver is in the schema, we know that it exists. But it may not
>   * be whitelisted. */
> @@ -61,16 +82,16 @@ void qmp_x_blockdev_create(BlockdevCreateOptions 
> *options, Error **errp)

Minor note: The comment above this if () block reads:

/* Call callback if it exists */

Which is now no longer really what is happening.  It just checks whether
the block driver supports blockdev-create.

>  return;
>  }
>  
> -cco = (BlockdevCreateCo) {
> -.drv = drv,
> -.opts = options,
> -.ret = -EINPROGRESS,
> -.errp = errp,
> -};
> -
> -co = qemu_coroutine_create(bdrv_co_create_co_entry, );
> -qemu_coroutine_enter(co);
> -while (cco.ret == -EINPROGRESS) {
> -aio_poll(qemu_get_aio_context(), true);
> +/* Create the block job */
> +s = job_create(job_id, _create_job_driver, NULL,
> +   qemu_get_aio_context(), JOB_DEFAULT | JOB_MANUAL_DISMISS,

Hmmm...  The old code already used the main AIO context, but is that
correct?  When you create a qcow2 node on top of a file node, shouldn't
you use the AIO context of the file node?

I see that figuring out the correct context generically may be difficult
to impossible, so OK, maybe we should just run image creation in the
main context for now.  But then, qcow2 (and other formats) should at
least take a lock on their file's context, which they don't seem to do.

So I suppose I can give a

Reviewed-by: Max Reitz 

for this patch, but I think some block drivers need some locking.

Max

> +   NULL, NULL, errp);
> +if (!s) {
> +return;
>  }
> +
> +s->drv = drv,
> +s->opts = QAPI_CLONE(BlockdevCreateOptions, options),
> +
> +job_start(>common);
>  }



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This adds a helper function that logs both the QMP request and the
> received response before returning it.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/iotests.py | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 17aa7c88dc..319d898172 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -206,6 +206,10 @@ def filter_qmp_event(event):
>  event['timestamp']['microseconds'] = 'USECS'
>  return event
>  
> +def filter_testfiles(msg):
> +prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> +return msg.replace(prefix, 'TEST_DIR/')

I'd prefer 'TEST_DIR/PID-' (just because).

But if you really like just 'TEST_DIR/'...  Then OK.

> +
>  def log(msg, filters=[]):
>  for flt in filters:
>  msg = flt(msg)
> @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
>  result.append(filter_qmp_event(ev))
>  return result
>  
> +def qmp_log(self, cmd, **kwargs):
> +logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
> +log(filter_testfiles(logmsg))
> +result = self.qmp(cmd, **kwargs)
> +log(result)

I think we should apply the testfiles filter here, too (error messages
may contain file names, after all).

Max

> +return result
> +
>  
>  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 21/40] job: Convert block_job_cancel_async() to Job

[Kashyap Chamarthy]

On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:
> Am 24.05.2018 um 19:42 hat John Snow geschrieben:

[...]

(Randomly chiming in for a small clarification.)

> > >> or some other mechanism that accomplishes the same type of behavior. It
> > >> would be nice if it did not have to be determined at job creation time
> > >> but instead could be determined later.
> > > 
> > > I agree. We already made sure that job-cancel really means cancel on the
> > > QAPI level, so we're free to do that. We just need to keep supporting
> > > block-job-cancel with the old semantics, so what I have is the easy
> > > conversion. We can change the internal implementation when we actually
> > > implement the selection of a completion mode.
> > > 
> > > Kevin
> > > 
> > 
> > We need this before 3.0 though, yeah? unless we make job-cancel
> > x-job-cancel or some other warning that the way it works might change, yeah?
> > 
> > Or do I misunderstand our leeway to change this at a later point in time?
> > 
> > (can job-cancel apply to block jobs created with the legacy
> > infrastructure? My reading was "yes.")
> 
> It can, and it already has its final semantics, so nothing has to change
> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
> force=true. If you want the complete-by-cancel behaviour of mirror, you
> have to use block-job-cancel for now, because job-cancel doesn't provide
> that functionality.

I think by "complete-by-cancel" you are referring to this[*] magic
behaviour, mentioned in the last sentence here:

When you cancel an in-progress ‘mirror’ job before the source and
target are synchronized, block-job-cancel will emit the event
BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
after it has indicated (via the event BLOCK_JOB_READY) that the
source and target have reached synchronization, then the event
emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.


[*] 
https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515

> So what we can change later is adding a way to initiate this secondary
> completion mode with a job-* command (probably with a new option for
> job-complete). But we wouldn't change the semantics of exisiting
> commands.

Ah, so if I'm understanding it correctly, the existing subtle magic of
"complete-by-cancel" will be rectified by separting the two distinct
operations: 'job-cancel' and 'job-complete'.


-- 
/kashyap

Re: [Qemu-block] [PATCH 09/14] qemu-iotests: Rewrite 207 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 207 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> Most of the test cases stay the same as before (the exception being some
> improved 'size' options that allow distinguishing which command created
> the image), but in order to be able to implement proper job handling,
> the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/207| 435 
> +++---
>  tests/qemu-iotests/207.out|  89 +
>  tests/qemu-iotests/group  |   6 +-
>  tests/qemu-iotests/iotests.py |  23 ++-
>  4 files changed, 264 insertions(+), 289 deletions(-)
> 
> diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
> index f5c77852d1..91c1f7e811 100755
> --- a/tests/qemu-iotests/207
> +++ b/tests/qemu-iotests/207

[...]

> +#
> +# Test host-key-check options
> +#
> +iotests.log("=== Test host-key-check options ===")
> +iotests.log("")

[...]

> +md5_key = subprocess.check_output(
> +'ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" | ' +
> +'cut -d" " -f3 | base64 -d | md5sum -b | cut -d" " -f1',
> +shell=True).rstrip()
> +
> +vm.launch()
> +blockdev_create(vm, { 'driver': 'ssh',
> +  'location': {
> +  'path': disk_path,
> +  'server': {
> +  'host': '127.0.0.1',
> +  'port': '22'
> +  },
> +  'host-key-check': {
> +  'mode': 'hash',
> +  'type': 'md5',
> +  'hash': 'wrong',
> +  }
> +  },
> +  'size': 2097152 })

Technically a change from before where it was 8M, but not a change I'm
opposed to.

[...]

> +#
> +# Invalid path and user
> +#
> +iotests.log("=== Invalid path and user ===")
> +iotests.log("")
> +
> +vm.launch()
> +blockdev_create(vm, { 'driver': 'ssh',
> +  'location': {
> +  'path': '/this/is/not/an/existing/path',
> +  'server': {
> +  'host': '127.0.0.1',
> +  'port': '22'
> +  },
> +  'host-key-check': {
> +  'mode': 'none'
> +  }
> +  },
> +  'size': 4194304 })
> +blockdev_create(vm, { 'driver': 'ssh',
> +  'location': {
> +  'path': disk_path,
> +  'user': 'invalid user',
> +  'server': {
> +  'host': '127.0.0.1',
> +  'port': '22'
> +  },
> +  'host-key-check': {
> +  'mode': 'none'
> +  }
> +  },
> +  'size': 4194304 })

Technical changes again (the previous test didn't have host-key-check),
but these are good changes.

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index f0f4ef32f0..e945caa6bb 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -109,9 +109,11 @@ def qemu_img_pipe(*args):
>  sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
> '.join(qemu_img_args + list(args
>  return subp.communicate()[0]
>  
> -def img_info_log(filename):
> +def img_info_log(filename, filter_path=None):
>  output = qemu_img_pipe('info', '-f', imgfmt, filename)
> -log(filter_img_info(output, filename))
> +if not filter_path:
> +filter_path = filename
> +log(filter_img_info(output, filter_path))
>  
>  def qemu_io(*args):
>  '''Run qemu-io and return the stdout data'''
> @@ -301,6 +303,13 @@ def file_path(*names):
>  
>  return paths[0] if len(paths) == 1 else paths
>  
> +def remote_filename(path):

I don't really understand why you have two patches in this series to add
functions to iotests.py, but then you keep on adding more functions in
other patches on the side.

> +if imgproto == 'file':
> +return imgproto

Shouldn't this be path?

With that fixed:

Reviewed-by: Max Reitz 

> +elif imgproto == 'ssh':
> +return "ssh://127.0.0.1%s" % (path)
> +else:
> +raise Exception("Protocol %s not supported" % (imgproto))
>  
>  class VM(qtest.QEMUQtestMachine):
>  '''A QEMU VM'''
> @@ -595,6 +604,16 @@ def verify_image_format(supported_fmts=[], 

Re: [Qemu-block] [PATCH 11/14] qemu-iotests: Rewrite 211 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 211 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/211 | 384 
> ++---
>  tests/qemu-iotests/211.out | 123 ---
>  tests/qemu-iotests/group   |   2 +-
>  3 files changed, 227 insertions(+), 282 deletions(-)
> 
> diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
> index 1edec26517..2fd89cfb32 100755
> --- a/tests/qemu-iotests/211
> +++ b/tests/qemu-iotests/211

[...]
> +iotests.img_info_log(disk_path)
> +iotests.log(iotests.filter_testfiles(iotests.qemu_img_pipe(
> +'map', '--output=json', disk_path)))

The _filter_qemu_img_map wasn't there to filter the filename (it isn't
present in the JSON output anyway, from what I can see), but to filter
the offset.

I don't think we really need to filter the offset here, though, because
this test is for a single specific image format with very specific
options, so it should be OK.

(The only thing is that the filter_testfiles() call seems unnecessary.)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 05/14] qemu-iotests: Add VM.get_qmp_events_filtered()

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This adds a helper function that returns a list of QMP events that are
> already filtered through filter_qmp_event().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/iotests.py | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 21/40] job: Convert block_job_cancel_async() to Job

[Kashyap Chamarthy]

On Tue, May 29, 2018 at 02:30:47PM +0200, Max Reitz wrote:
> On 2018-05-29 13:59, Kashyap Chamarthy wrote:
> > On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:

[...]

> >> It can, and it already has its final semantics, so nothing has to change
> >> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
> >> force=true. If you want the complete-by-cancel behaviour of mirror, you
> >> have to use block-job-cancel for now, because job-cancel doesn't provide
> >> that functionality.
> > 
> > I think by "complete-by-cancel" you are referring to this[*] magic
> > behaviour, mentioned in the last sentence here:
> > 
> > When you cancel an in-progress ‘mirror’ job before the source and
> > target are synchronized, block-job-cancel will emit the event
> > BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
> > after it has indicated (via the event BLOCK_JOB_READY) that the
> > source and target have reached synchronization, then the event
> > emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.
> > 
> > 
> > [*] 
> > https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515
> > 
> >> So what we can change later is adding a way to initiate this secondary
> >> completion mode with a job-* command (probably with a new option for
> >> job-complete). But we wouldn't change the semantics of exisiting
> >> commands.
> > 
> > Ah, so if I'm understanding it correctly, the existing subtle magic of
> > "complete-by-cancel" will be rectified by separting the two distinct
> > operations: 'job-cancel' and 'job-complete'.
> 
> Not really, because we already had those two operations for block jobs.
> The thing is that block-job-complete (and thus job-complete) will pivot
> to the target disk, but block-job-cancel doesn't.

Indeed; from the doc linked earlier: "Issuing the command
``block-job-complete`` (after it emits the event
``BLOCK_JOB_COMPLETED``) will adjust the guest device (i.e. live QEMU)
to point to the target image, [E], causing all the new writes from this
point on to happen there."

> The special behavior is that you can use block-job-cancel after
> BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
> we have a real plan on how to represent that with the generic job
> commands, we just know that we don't want to use job-cancel.

Ah, thanks for clarifying.  Yes, what you say makes sense —  not using
'job-cancel' to represent completion.

> (Maybe we can add a flag to job-complete (which to me does not sound
> like a good idea), or you could set flags on jobs while they are
> running, so you can set a do-not-pivot flag on the mirror job before you
> complete it.)

Yeah, spelling that out, 'do-not-pivot' or something along those lines,
as a flag makes it clearer.  "Implicit is better than explicit".

-- 
/kashyap

Re: [Qemu-block] [PATCH 14/14] block/create: Mark blockdev-create stable

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> We're ready to declare the blockdev-create job stable. This renames the
> corresponding QMP command from x-blockdev-create to blockdev-create.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json   |  4 ++--
>  qapi/job.json  |  2 +-
>  block/create.c |  4 ++--
>  tests/qemu-iotests/206 |  2 +-
>  tests/qemu-iotests/206.out | 54 
> +++---
>  tests/qemu-iotests/207 |  2 +-
>  tests/qemu-iotests/207.out | 18 
>  tests/qemu-iotests/210 |  2 +-
>  tests/qemu-iotests/210.out | 18 
>  tests/qemu-iotests/211 |  2 +-
>  tests/qemu-iotests/211.out | 24 ++---
>  tests/qemu-iotests/212 |  2 +-
>  tests/qemu-iotests/212.out | 42 ++--
>  tests/qemu-iotests/213 |  2 +-
>  tests/qemu-iotests/213.out | 44 ++---
>  15 files changed, 111 insertions(+), 111 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Kevin Wolf]

Am 29.05.2018 um 13:48 hat Max Reitz geschrieben:
> On 2018-05-25 18:33, Kevin Wolf wrote:
> > This adds a helper function that logs both the QMP request and the
> > received response before returning it.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  tests/qemu-iotests/iotests.py | 11 +++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 17aa7c88dc..319d898172 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -206,6 +206,10 @@ def filter_qmp_event(event):
> >  event['timestamp']['microseconds'] = 'USECS'
> >  return event
> >  
> > +def filter_testfiles(msg):
> > +prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> > +return msg.replace(prefix, 'TEST_DIR/')
> 
> I'd prefer 'TEST_DIR/PID-' (just because).
> 
> But if you really like just 'TEST_DIR/'...  Then OK.

I preferred that because it leaves the output unchanged from the old
bash tests, which made reviewing the results easier. Maybe that's a too
temporary advantage to be of any use in the future, though, so we could
change it afterwards...

> > +
> >  def log(msg, filters=[]):
> >  for flt in filters:
> >  msg = flt(msg)
> > @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
> >  result.append(filter_qmp_event(ev))
> >  return result
> >  
> > +def qmp_log(self, cmd, **kwargs):
> > +logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
> > +log(filter_testfiles(logmsg))
> > +result = self.qmp(cmd, **kwargs)
> > +log(result)
> 
> I think we should apply the testfiles filter here, too (error messages
> may contain file names, after all).

Didn't happen in the test outputs of this series, and filter_testfiles()
processes strings whereas result is a dict, so it would be more
complicated than just adding a function call.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 08/14] qemu-iotests: Rewrite 206 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 206 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/206| 705 
> +-
>  tests/qemu-iotests/206.out| 241 +--
>  tests/qemu-iotests/group  |   2 +-
>  tests/qemu-iotests/iotests.py |  15 +
>  4 files changed, 448 insertions(+), 515 deletions(-)
> 
> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> index 0a18b2b19a..9a305302d1 100755
> --- a/tests/qemu-iotests/206
> +++ b/tests/qemu-iotests/206

[...]

> +import iotests
> +from iotests import imgfmt
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +def blockdev_create(vm, options):
> +result = vm.qmp_log('x-blockdev-create', job_id='job0', options=options)
> +
> +if 'return' in result:
> +assert result['return'] == {}
> +vm.run_job('job0')
> +iotests.log("")
> +
> +with iotests.FilePath('t.qcow2') as disk_path, \
> + iotests.FilePath('t.qcow2.base') as backing_path, \
> + iotests.VM() as vm:
> +
> +vm.add_object('secret,id=keysec0,data="foo"')

I don't know how subprocess.Popen() works, but are you sure you aren't
encrypting with '"foo"' now?  (i.e. literally that key, including quotes)

> +
> +#
> +# Successful image creation (defaults)
> +#
> +iotests.log("=== Successful image creation (defaults) ===")

OK, the comment makes sense for visual separation.  But so would leaving
three empty lines.  *cough*

> +iotests.log("")
> +
> +size = 128 * 1024 * 1024
> +
> +vm.launch()
> +blockdev_create(vm, { 'driver': 'file',
> +  'filename': disk_path,
> +  'size': 0 })
> +
> +vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> +   node_name='imgfile')
> +
> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': 'imgfile',
> +  'size': size })
> +vm.shutdown()
> +
> +iotests.img_info_log(disk_path)
> +

[...]

> +#
> +# Successful image creation (v2 non-default options)
> +#
> +iotests.log("=== Successful image creation (v2 non-default options) ===")
> +iotests.log("")
> +
> +vm.launch()
> +blockdev_create(vm, { 'driver': 'file',
> +  'filename': disk_path,
> +  'size': 0 })
> +
> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': {
> +  'driver': 'file',
> +  'filename': disk_path,
> +  },
> +  'size': size,
> +  'backing-file': backing_path,

Change from the bash version: backing_path does not exist here.  Not
sure if that was intentional.

> +  'backing-fmt': 'qcow2',
> +  'version': 'v2',
> +  'cluster-size': 512 })
> +vm.shutdown()
> +
> +iotests.img_info_log(disk_path)

[...]

> +#
> +# Invalid sizes
> +#
> +iotests.log("=== Invalid sizes ===")
> +
> +# TODO Negative image sizes aren't handled correctly, but this is a 
> problem
> +# with QAPI's implementation of the 'size' type and affects other 
> commands
> +# as well. Once this is fixed, we may want to add a test case here.
> +#
> +# 1. Misaligned image size
> +# 2. 2^64 - 512
> +# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
> +# 4. 2^63 - 512 (generally valid, but qcow2 can't handle images this 
> size)
> +
> +vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
> +
> +vm.launch()
> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': 'node0',
> +  'size': 1234  })
> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': 'node0',
> +  'size': 18446744073709551104 })
> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': 'node0',
> +  'size': 9223372036854775808 })
> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': 'node0',
> +  'size': 9223372036854775296})

Missing space before the closing fancy bracket, critical!

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 20ce5a0cf0..f0f4ef32f0 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -416,6 +416,21 @@ class VM(qtest.QEMUQtestMachine):
>  log(result)
>  return result
>  
> +def run_job(self, job):

Is there any reason 

Re: [Qemu-block] [Qemu-devel] [PATCH v2 21/40] job: Convert block_job_cancel_async() to Job

[Max Reitz]

On 2018-05-29 13:59, Kashyap Chamarthy wrote:
> On Fri, May 25, 2018 at 10:00:35AM +0200, Kevin Wolf wrote:
>> Am 24.05.2018 um 19:42 hat John Snow geschrieben:
> 
> [...]
> 
> (Randomly chiming in for a small clarification.)
> 
> or some other mechanism that accomplishes the same type of behavior. It
> would be nice if it did not have to be determined at job creation time
> but instead could be determined later.

 I agree. We already made sure that job-cancel really means cancel on the
 QAPI level, so we're free to do that. We just need to keep supporting
 block-job-cancel with the old semantics, so what I have is the easy
 conversion. We can change the internal implementation when we actually
 implement the selection of a completion mode.

 Kevin

>>>
>>> We need this before 3.0 though, yeah? unless we make job-cancel
>>> x-job-cancel or some other warning that the way it works might change, yeah?
>>>
>>> Or do I misunderstand our leeway to change this at a later point in time?
>>>
>>> (can job-cancel apply to block jobs created with the legacy
>>> infrastructure? My reading was "yes.")
>>
>> It can, and it already has its final semantics, so nothing has to change
>> before 3.0. job-cancel is equivalent to block-job-cancel with fixed
>> force=true. If you want the complete-by-cancel behaviour of mirror, you
>> have to use block-job-cancel for now, because job-cancel doesn't provide
>> that functionality.
> 
> I think by "complete-by-cancel" you are referring to this[*] magic
> behaviour, mentioned in the last sentence here:
> 
> When you cancel an in-progress ‘mirror’ job before the source and
> target are synchronized, block-job-cancel will emit the event
> BLOCK_JOB_CANCELLED. However, note that if you cancel a ‘mirror’ job
> after it has indicated (via the event BLOCK_JOB_READY) that the
> source and target have reached synchronization, then the event
> emitted by block-job-cancel changes to BLOCK_JOB_COMPLETED.
> 
> 
> [*] 
> https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst#l515
> 
>> So what we can change later is adding a way to initiate this secondary
>> completion mode with a job-* command (probably with a new option for
>> job-complete). But we wouldn't change the semantics of exisiting
>> commands.
> 
> Ah, so if I'm understanding it correctly, the existing subtle magic of
> "complete-by-cancel" will be rectified by separting the two distinct
> operations: 'job-cancel' and 'job-complete'.

Not really, because we already had those two operations for block jobs.
The thing is that block-job-complete (and thus job-complete) will pivot
to the target disk, but block-job-cancel doesn't.

The special behavior is that you can use block-job-cancel after
BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
we have a real plan on how to represent that with the generic job
commands, we just know that we don't want to use job-cancel.

(Maybe we can add a flag to job-complete (which to me does not sound
like a good idea), or you could set flags on jobs while they are
running, so you can set a do-not-pivot flag on the mirror job before you
complete it.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Kevin Wolf]

Am 29.05.2018 um 14:15 hat Max Reitz geschrieben:
> On 2018-05-29 14:12, Kevin Wolf wrote:
> > Am 29.05.2018 um 13:48 hat Max Reitz geschrieben:
> >> On 2018-05-25 18:33, Kevin Wolf wrote:
> >>> This adds a helper function that logs both the QMP request and the
> >>> received response before returning it.
> >>>
> >>> Signed-off-by: Kevin Wolf 
> >>> ---
> >>>  tests/qemu-iotests/iotests.py | 11 +++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >>> index 17aa7c88dc..319d898172 100644
> >>> --- a/tests/qemu-iotests/iotests.py
> >>> +++ b/tests/qemu-iotests/iotests.py
> >>> @@ -206,6 +206,10 @@ def filter_qmp_event(event):
> >>>  event['timestamp']['microseconds'] = 'USECS'
> >>>  return event
> >>>  
> >>> +def filter_testfiles(msg):
> >>> +prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> >>> +return msg.replace(prefix, 'TEST_DIR/')
> >>
> >> I'd prefer 'TEST_DIR/PID-' (just because).
> >>
> >> But if you really like just 'TEST_DIR/'...  Then OK.
> > 
> > I preferred that because it leaves the output unchanged from the old
> > bash tests, which made reviewing the results easier. Maybe that's a too
> > temporary advantage to be of any use in the future, though, so we could
> > change it afterwards...
> 
> It doesn't really make reviewing the patches easier, though, because the
> hardest part of course is the change of the test itself and not the
> change of the result. :-)
> 
> (And some file name changes really are on the easy side.)

If you think so... For me the main reason to convert the test files was
to see whether the job actually does the same thing as the old
synchronous command did.

> >>> +
> >>>  def log(msg, filters=[]):
> >>>  for flt in filters:
> >>>  msg = flt(msg)
> >>> @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
> >>>  result.append(filter_qmp_event(ev))
> >>>  return result
> >>>  
> >>> +def qmp_log(self, cmd, **kwargs):
> >>> +logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
> >>> +log(filter_testfiles(logmsg))
> >>> +result = self.qmp(cmd, **kwargs)
> >>> +log(result)
> >>
> >> I think we should apply the testfiles filter here, too (error messages
> >> may contain file names, after all).
> > 
> > Didn't happen in the test outputs of this series, and filter_testfiles()
> > processes strings whereas result is a dict, so it would be more
> > complicated than just adding a function call.
> 
> You mean it would be "log(filter_testfiles('%s' % result)))"?

Ah, you mean just for logging? Yes, we can do that. I thought you meant
returning a filtered result as well.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 09/14] qemu-iotests: Rewrite 207 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 207 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> Most of the test cases stay the same as before (the exception being some
> improved 'size' options that allow distinguishing which command created
> the image), but in order to be able to implement proper job handling,
> the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/207| 435 
> +++---
>  tests/qemu-iotests/207.out|  89 +
>  tests/qemu-iotests/group  |   6 +-
>  tests/qemu-iotests/iotests.py |  23 ++-
>  4 files changed, 264 insertions(+), 289 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
> index 417deee970..299650872c 100644
> --- a/tests/qemu-iotests/207.out
> +++ b/tests/qemu-iotests/207.out

[...]

> +{'execute': 'x-blockdev-create', 'arguments': {'job_id': 'job0', 'options': 
> {'driver': 'ssh', 'location': {'path': 'TEST_DIR/t.img', 'host-key-check': 
> {'hash': 'f3386a5742ddc4a04244118e59a1f92b', 'type': 'md5', 'mode': 'hash'}, 
> 'server': {'host': '127.0.0.1', 'port': '22'}}, 'size': 8388608}}}

On second thought...  That hash shouldn't be here.  Well, my R-b stands
if you change it to 314620f24e31d6074e573b80887ace53. ;-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 21/40] job: Convert block_job_cancel_async() to Job

[Kashyap Chamarthy]

On Tue, May 29, 2018 at 03:10:58PM +0200, Kashyap Chamarthy wrote:
> On Tue, May 29, 2018 at 02:30:47PM +0200, Max Reitz wrote:

[...]

> > The special behavior is that you can use block-job-cancel after
> > BLOCK_JOB_READY to complete the job, but not pivot to it.  I don't think
> > we have a real plan on how to represent that with the generic job
> > commands, we just know that we don't want to use job-cancel.
> 
> Ah, thanks for clarifying.  Yes, what you say makes sense —  not using
> 'job-cancel' to represent completion.
> 
> > (Maybe we can add a flag to job-complete (which to me does not sound
> > like a good idea), or you could set flags on jobs while they are
> > running, so you can set a do-not-pivot flag on the mirror job before you
> > complete it.)
> 
> Yeah, spelling that out, 'do-not-pivot' or something along those lines,
> as a flag makes it clearer.  "Implicit is better than explicit".

Oops, I got the quote reverse :P.  Correct: "Explicit is better than
implicit".  But you've auto-corrected in your head already...

-- 
/kashyap

Re: [Qemu-block] [PATCH 13/14] qemu-iotests: Rewrite 213 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 213 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/213 | 520 
> +
>  tests/qemu-iotests/213.out | 198 ++---
>  tests/qemu-iotests/group   |   4 +-
>  3 files changed, 314 insertions(+), 408 deletions(-)
> 
> diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
> index 3a00a0f6d6..fe4017edc7 100755
> --- a/tests/qemu-iotests/213
> +++ b/tests/qemu-iotests/213

[...]

> +import iotests
> +from iotests import imgfmt
> +
> +iotests.verify_image_format(supported_fmts=['vhdx'])
> +iotests.verify_protocol(supported=['file'])
> +
> +def blockdev_create(vm, options):
> +result = vm.qmp_log('x-blockdev-create', job_id='job0', options=options)
> +
> +if 'return' in result:
> +assert result['return'] == {}
> +vm.run_job('job0')
> +iotests.log("")
> +
> +with iotests.FilePath('t.vdi') as disk_path, \

Now you've just given up? :-)

Again, with that fixed:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/14] qemu-iotests: Add iotests.img_info_log()

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This adds a filter function to postprocess 'qemu-img info' input
> (similar to what _img_info does), and an img_info_log() function that
> calls 'qemu-img info' and logs the filtered output.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/iotests.py | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 319d898172..20ce5a0cf0 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -109,6 +109,10 @@ def qemu_img_pipe(*args):
>  sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
> '.join(qemu_img_args + list(args
>  return subp.communicate()[0]
>  
> +def img_info_log(filename):
> +output = qemu_img_pipe('info', '-f', imgfmt, filename)
> +log(filter_img_info(output, filename))
> +
>  def qemu_io(*args):
>  '''Run qemu-io and return the stdout data'''
>  args = qemu_io_args + list(args)
> @@ -210,6 +214,18 @@ def filter_testfiles(msg):
>  prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>  return msg.replace(prefix, 'TEST_DIR/')
>  
> +def filter_img_info(output, filename):
> +lines = []
> +for line in output.split('\n'):
> +if 'disk size' in line or 'actual size' in line:

Wouldn't that be 'actual-size' (because I presume this is for JSON output)?

> +continue
> +line = line.replace(filename, 'TEST_DIR/t.IMGFMT') \

Why not just 'TEST_IMG'?

Max

> +   .replace(imgfmt, 'IMGFMT')
> +line = re.sub('iters: [0-9]+', 'iters: XXX', line)
> +line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
> ----', line)
> +lines.append(line)
> +return '\n'.join(lines)
> +
>  def log(msg, filters=[]):
>  for flt in filters:
>  msg = flt(msg)
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Max Reitz]

On 2018-05-29 14:12, Kevin Wolf wrote:
> Am 29.05.2018 um 13:48 hat Max Reitz geschrieben:
>> On 2018-05-25 18:33, Kevin Wolf wrote:
>>> This adds a helper function that logs both the QMP request and the
>>> received response before returning it.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  tests/qemu-iotests/iotests.py | 11 +++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 17aa7c88dc..319d898172 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -206,6 +206,10 @@ def filter_qmp_event(event):
>>>  event['timestamp']['microseconds'] = 'USECS'
>>>  return event
>>>  
>>> +def filter_testfiles(msg):
>>> +prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>> +return msg.replace(prefix, 'TEST_DIR/')
>>
>> I'd prefer 'TEST_DIR/PID-' (just because).
>>
>> But if you really like just 'TEST_DIR/'...  Then OK.
> 
> I preferred that because it leaves the output unchanged from the old
> bash tests, which made reviewing the results easier. Maybe that's a too
> temporary advantage to be of any use in the future, though, so we could
> change it afterwards...

It doesn't really make reviewing the patches easier, though, because the
hardest part of course is the change of the test itself and not the
change of the result. :-)

(And some file name changes really are on the easy side.)

>>> +
>>>  def log(msg, filters=[]):
>>>  for flt in filters:
>>>  msg = flt(msg)
>>> @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
>>>  result.append(filter_qmp_event(ev))
>>>  return result
>>>  
>>> +def qmp_log(self, cmd, **kwargs):
>>> +logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
>>> +log(filter_testfiles(logmsg))
>>> +result = self.qmp(cmd, **kwargs)
>>> +log(result)
>>
>> I think we should apply the testfiles filter here, too (error messages
>> may contain file names, after all).
> 
> Didn't happen in the test outputs of this series, and filter_testfiles()
> processes strings whereas result is a dict, so it would be more
> complicated than just adding a function call.

You mean it would be "log(filter_testfiles('%s' % result)))"?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Max Reitz]

On 2018-05-29 14:39, Kevin Wolf wrote:
> Am 29.05.2018 um 14:15 hat Max Reitz geschrieben:
>> On 2018-05-29 14:12, Kevin Wolf wrote:
>>> Am 29.05.2018 um 13:48 hat Max Reitz geschrieben:
 On 2018-05-25 18:33, Kevin Wolf wrote:
> This adds a helper function that logs both the QMP request and the
> received response before returning it.
>
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/iotests.py | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 17aa7c88dc..319d898172 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -206,6 +206,10 @@ def filter_qmp_event(event):
>  event['timestamp']['microseconds'] = 'USECS'
>  return event
>  
> +def filter_testfiles(msg):
> +prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> +return msg.replace(prefix, 'TEST_DIR/')

 I'd prefer 'TEST_DIR/PID-' (just because).

 But if you really like just 'TEST_DIR/'...  Then OK.
>>>
>>> I preferred that because it leaves the output unchanged from the old
>>> bash tests, which made reviewing the results easier. Maybe that's a too
>>> temporary advantage to be of any use in the future, though, so we could
>>> change it afterwards...
>>
>> It doesn't really make reviewing the patches easier, though, because the
>> hardest part of course is the change of the test itself and not the
>> change of the result. :-)
>>
>> (And some file name changes really are on the easy side.)
> 
> If you think so... For me the main reason to convert the test files was
> to see whether the job actually does the same thing as the old
> synchronous command did.

Sure, but kompare is rather good at highlighting changes in a single
line, so I can quickly verify them as benign. :-)

> +
>  def log(msg, filters=[]):
>  for flt in filters:
>  msg = flt(msg)
> @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
>  result.append(filter_qmp_event(ev))
>  return result
>  
> +def qmp_log(self, cmd, **kwargs):
> +logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
> +log(filter_testfiles(logmsg))
> +result = self.qmp(cmd, **kwargs)
> +log(result)

 I think we should apply the testfiles filter here, too (error messages
 may contain file names, after all).
>>>
>>> Didn't happen in the test outputs of this series, and filter_testfiles()
>>> processes strings whereas result is a dict, so it would be more
>>> complicated than just adding a function call.
>>
>> You mean it would be "log(filter_testfiles('%s' % result)))"?
> 
> Ah, you mean just for logging? Yes, we can do that. I thought you meant
> returning a filtered result as well.

Oh, no.  At least I can't think of a reason why we'd want that.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 10/14] qemu-iotests: Rewrite 210 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 210 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/210| 393 
> ++
>  tests/qemu-iotests/210.out| 189 ++--
>  tests/qemu-iotests/group  |   2 +-
>  tests/qemu-iotests/iotests.py |  12 +-
>  4 files changed, 310 insertions(+), 286 deletions(-)

Reviewed-by: Max Reitz 

> diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
> index e607c0d296..ff4fddea56 100755
> --- a/tests/qemu-iotests/210
> +++ b/tests/qemu-iotests/210

[...]

> +#
> +# Invalid sizes
> +#
> +
> +# TODO Negative image sizes aren't handled correctly, but this is a 
> problem
> +# with QAPI's implementation of the 'size' type and affects other 
> commands as
> +# well. Once this is fixed, we may want to add a test case here.
> +
> +# 1. 2^64 - 512
> +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this)
> +# 3. 2^63 - 512 (generally valid, but with the crypto header the file 
> will
> +#exceed 63 bits)
> +iotests.log("=== Invalid sizes ===")
> +iotests.log("")
> +
> +vm.launch()
> +for size in [ 18446744073709551104, 9223372036854775808, 
> 9223372036854775296 ]:

Maybe this would be nice in patch 8 as well.

Max

> +blockdev_create(vm, { 'driver': imgfmt,
> +  'file': 'node0',
> +  'key-secret': 'keysec0',
> +  'size': size })
> +vm.shutdown(



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Tue, May 29, 2018 at 12:14:28PM +0200, Kevin Wolf wrote:
> Am 29.05.2018 um 11:23 hat Max Reitz geschrieben:
> > On 2018-05-28 21:09, Kevin Wolf wrote:
> > > Am 28.05.2018 um 20:44 hat Max Reitz geschrieben:
> > >> On 2018-05-28 20:38, Kevin Wolf wrote:
> > >>> Am 28.05.2018 um 20:30 hat Richard W.M. Jones geschrieben:
> >  On Mon, May 28, 2018 at 08:10:32PM +0200, Max Reitz wrote:
> > > As someone who is just naive and doesn't see the big picture, I don't
> > > see what's wrong with using a tar file that contains the image and
> > > additional data.
> > 
> >  FWIW an OVA file is exactly this: an uncompressed tar file containing
> >  disk image(s) and metadata.
> > >>>
> > >>> If we combine VM configuration and the disk image this way, I would
> > >>> still want to directly use that combined thing without having to extract
> > >>> its components first.
> > >>>
> > >>> Just accessing the image file within a tar archive is possible and we
> > >>> could write a block driver for that (I actually think we should do
> > >>> this), but it restricts you because certain operations like resizing
> > >>> aren't really possible in tar. Unfortunately, resizing is a really
> > >>> common operation for non-raw image formats.
> > >>>
> > >>> And if I think of a file format that can contain several different
> > >>> things that can be individually resized etc., I end up with qcow2 in the
> > >>> simple case or a full file system in the more complex case.
> > >>
> > >> Well, you end up with VMDK.
> > > 
> > > I don't think VMDK can save several different objects? It can have some
> > > metadata in the descriptor, and it can spread the contents of a single
> > > object across multiple files (with extents), but I don't think it has
> > > something comparable to e.g. qcow2 snapshots, which are separate objects
> > > with an individual size that can dynamically change.
> > 
> > Right, I tried to be funny and was over-simplifying in the process.
> > 
> > What I meant is: You end up with an image format that is spread on a
> > filesystem, like VMDK is (usually).  Then you have some metadata
> > descriptor file that describes the rest and multiple data object files.
> > 
> > (For completeness's sake: And you can use an external or an internal
> > filesystem, that is, use multiple files (like VMDK) or have an internal
> > filesystem (like tar, except tar doesn't allow fragmentation).)
> 
> Let's call the libvirt XML the image file and the qcow2 files its
> data object files and we're done?

libvirt XML doesn't seems to be a solution for the problem, but a
separate VM specification file could work anyway.


> 
> I'm afraid spreading things across multiple files doesn't meet the
> requirements for the problem at hand, though...

Yeah, I wanted to make this an extension of qcow2, so people
don't have Yet Another file format to choose from.  We could also
let libvirt automatically write hints to disk images so things
would Just Work if you copied a disk image from an existing VM.

But I'm not the one working on management layers or guest image
tools, so I will just trust Richard's and Daniel's opinions on
this.

-- 
Eduardo

Re: [Qemu-block] [PATCH 12/14] qemu-iotests: Rewrite 212 for blockdev-create job

[Max Reitz]

On 2018-05-25 18:33, Kevin Wolf wrote:
> This rewrites the test case 212 to work with the new x-blockdev-create
> job rather than the old synchronous version of the command.
> 
> All of the test cases stay the same as before, but in order to be able
> to implement proper job handling, the test case is rewritten in Python.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/212 | 483 
> +
>  tests/qemu-iotests/212.out | 181 ++---
>  tests/qemu-iotests/group   |   2 +-
>  3 files changed, 290 insertions(+), 376 deletions(-)
> 
> diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
> index e5a1ba77ce..5ac4947b82 100755
> --- a/tests/qemu-iotests/212
> +++ b/tests/qemu-iotests/212

[...]

> +import iotests
> +from iotests import imgfmt
> +
> +iotests.verify_image_format(supported_fmts=['parallels'])
> +iotests.verify_protocol(supported=['file'])
> +
> +def blockdev_create(vm, options):
> +result = vm.qmp_log('x-blockdev-create', job_id='job0', options=options)
> +
> +if 'return' in result:
> +assert result['return'] == {}
> +vm.run_job('job0')
> +iotests.log("")
> +
> +with iotests.FilePath('t.vdi') as disk_path, \

't.vdi', are you sure about that? ;-)

(You thought I wouldn't read the boiler plate anymore, didn't you?  Ha!)

With that fixed:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Eduardo Habkost]

On Tue, May 29, 2018 at 03:03:16PM +0100, Dr. David Alan Gilbert wrote:
> * Richard W.M. Jones (rjo...@redhat.com) wrote:
> > On Fri, May 18, 2018 at 06:09:56PM +0100, Daniel P. Berrangé wrote:
> > > The closest to a cross-hypervisor standard is OVF which can store
> > > metadata about required hardware for a VM. I'm pretty sure it does
> > > not have the concept of machine types, but maybe it has a way for
> > > people to define metadata extensions. Since it is just XML at the
> > > end of the day, even if there was nothing official in OVF, it would
> > > be possible to just define a custom XML namespace and declare a
> > > schema for that to follow.
> > 
> > I have a great deal of experience with the OVF "standard".
> > TL;DR: DO NOT USE IT.
> 
> In addition to the detail below, from reading DMTF's OVF spec (DSP0243 v
> 2.1.1) I see absolutely nothing specifying hardware type.
> Sure it can specify size of storage, number of ether cards, MAC
> addresses for them etc - but I don't see any where specify the type of 
> emualted system.

Maybe the VirtualHardwareSection/System/vssd:VirtualSystemType
element could be used for that.  (DSP0243 v2.1.1, line 650).

But based on Richard's feedback, I think we shouldn't even try to
use it.

-- 
Eduardo

Re: [Qemu-block] [PATCH 02/14] vhdx: Fix vhdx_co_create() return value

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:15PM +0200, Kevin Wolf wrote:
> .bdrv_co_create() is supposed to return 0 on success, but vhdx could
> return a positive value instead. Fix this.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/vhdx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 0b1e21c750..b1ba121bb6 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1951,7 +1951,7 @@ static int coroutine_fn 
> vhdx_co_create(BlockdevCreateOptions *opts,
>  goto delete_and_exit;
>  }
>  
> -
> +ret = 0;
>  delete_and_exit:
>  blk_unref(blk);
>  bdrv_unref(bs);
> -- 
> 2.13.6
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH 03/14] job: Add error message for failing jobs

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:16PM +0200, Kevin Wolf wrote:
> So far we relied on job->ret and strerror() to produce an error message
> for failed jobs. Not surprisingly, this tends to result in completely
> useless messages.
> 
> This adds a Job.error field that can contain an error string for a
> failing job, and a parameter to job_completed() that sets the field. As
> a default, if NULL is passed, we continue to use strerror(job->ret).
> 
> All existing callers are changed to pass NULL. They can be improved in
> separate patches.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  include/qemu/job.h |  7 ++-
>  block/backup.c |  2 +-
>  block/commit.c |  2 +-
>  block/mirror.c |  2 +-
>  block/stream.c |  2 +-
>  job-qmp.c  |  9 ++---
>  job.c  | 15 +--
>  7 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 8c8badf75e..b2e1dd00b9 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,6 +124,9 @@ typedef struct Job {
>  /** Estimated progress_current value at the completion of the job */
>  int64_t progress_total;
>  
> +/** Error string for a failed job (NULL if job->ret == 0) */
> +char *error;
> +
>  /** ret code passed to job_completed. */
>  int ret;
>  
> @@ -466,13 +469,15 @@ void job_transition_to_ready(Job *job);
>  /**
>   * @job: The job being completed.
>   * @ret: The status code.
> + * @error: The error message for a failing job (only with @ret < 0). If @ret 
> is
> + * negative, but NULL is given for @error, strerror() is used.
>   *
>   * Marks @job as completed. If @ret is non-zero, the job transaction it is 
> part
>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If 
> it
>   * is the last job to complete in its transaction, all jobs in the 
> transaction
>   * move from WAITING to PENDING.
>   */
> -void job_completed(Job *job, int ret);
> +void job_completed(Job *job, int ret, Error *error);
>  
>  /** Asynchronously complete the specified @job. */
>  void job_complete(Job *job, Error **errp);
> diff --git a/block/backup.c b/block/backup.c
> index 4e228e959b..5661435675 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -321,7 +321,7 @@ static void backup_complete(Job *job, void *opaque)
>  {
>  BackupCompleteData *data = opaque;
>  
> -job_completed(job, data->ret);
> +job_completed(job, data->ret, NULL);
>  g_free(data);
>  }
>  
> diff --git a/block/commit.c b/block/commit.c
> index 620666161b..e1814d9693 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque)
>   * bdrv_set_backing_hd() to fail. */
>  block_job_remove_all_bdrv(bjob);
>  
> -job_completed(job, ret);
> +job_completed(job, ret, NULL);
>  g_free(data);
>  
>  /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> diff --git a/block/mirror.c b/block/mirror.c
> index dcb66ec3be..435268bbbf 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -581,7 +581,7 @@ static void mirror_exit(Job *job, void *opaque)
>  blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
>  blk_insert_bs(bjob->blk, mirror_top_bs, _abort);
>  
> -job_completed(job, data->ret);
> +job_completed(job, data->ret, NULL);
>  
>  g_free(data);
>  bdrv_drained_end(src);
> diff --git a/block/stream.c b/block/stream.c
> index a5d6e0cf8a..9264b68a1e 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -93,7 +93,7 @@ out:
>  }
>  
>  g_free(s->backing_file_str);
> -job_completed(job, data->ret);
> +job_completed(job, data->ret, NULL);
>  g_free(data);
>  }
>  
> diff --git a/job-qmp.c b/job-qmp.c
> index 7f38f63336..410775df61 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -136,14 +136,9 @@ void qmp_job_dismiss(const char *id, Error **errp)
>  static JobInfo *job_query_single(Job *job, Error **errp)
>  {
>  JobInfo *info;
> -const char *errmsg = NULL;
>  
>  assert(!job_is_internal(job));
>  
> -if (job->ret < 0) {
> -errmsg = strerror(-job->ret);
> -}
> -
>  info = g_new(JobInfo, 1);
>  *info = (JobInfo) {
>  .id = g_strdup(job->id),
> @@ -151,8 +146,8 @@ static JobInfo *job_query_single(Job *job, Error **errp)
>  .status = job->status,
>  .current_progress   = job->progress_current,
>  .total_progress = job->progress_total,
> -.has_error  = !!errmsg,
> -.error  = g_strdup(errmsg),
> +.has_error  = !!job->error,
> +.error  = g_strdup(job->error),
>  };
>  
>  return info;
> diff --git a/job.c b/job.c
> index f026661b0f..fc39eaaa5e 100644
> --- a/job.c
> +++ b/job.c
> @@ -369,6 +369,7 @@ void job_unref(Job *job)
>  
>  QLIST_REMOVE(job, job_list);
>  
> +g_free(job->error);
>  

Re: [Qemu-block] [PATCH 01/14] vdi: Fix vdi_co_do_create() return value

[Jeff Cody]

On Fri, May 25, 2018 at 06:33:14PM +0200, Kevin Wolf wrote:
> .bdrv_co_create() is supposed to return 0 on success, but vdi could
> return a positive value instead. Fix this.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/vdi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 96a22b8e83..668af0a828 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -865,6 +865,7 @@ static int coroutine_fn 
> vdi_co_do_create(BlockdevCreateOptions *create_options,
>  }
>  }
>  
> +ret = 0;
>  exit:
>  blk_unref(blk);
>  bdrv_unref(bs_file);
> -- 
> 2.13.6
> 
> 

Reviewed-by: Jeff Cody 

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Dr. David Alan Gilbert]

* Richard W.M. Jones (rjo...@redhat.com) wrote:
> On Fri, May 18, 2018 at 06:09:56PM +0100, Daniel P. Berrangé wrote:
> > The closest to a cross-hypervisor standard is OVF which can store
> > metadata about required hardware for a VM. I'm pretty sure it does
> > not have the concept of machine types, but maybe it has a way for
> > people to define metadata extensions. Since it is just XML at the
> > end of the day, even if there was nothing official in OVF, it would
> > be possible to just define a custom XML namespace and declare a
> > schema for that to follow.
> 
> I have a great deal of experience with the OVF "standard".
> TL;DR: DO NOT USE IT.

In addition to the detail below, from reading DMTF's OVF spec (DSP0243 v
2.1.1) I see absolutely nothing specifying hardware type.
Sure it can specify size of storage, number of ether cards, MAC
addresses for them etc - but I don't see any where specify the type of 
emualted system.

Dave

> Long answer copied from a rant I wrote on an internal mailing list a
> while back:
> 
>   Don't make the mistake of confusing OVF for a format.  It's not,
>   there are at least 4 non-interoperable OVF "format"s around:
> 
>- 2 x oVirt OVF
>- VMware's OVF used in exported OVA files
>- VirtualBox's OVF used in their exported OVA files
> 
>   These are all different and do not interoperate *at all*.  So before
>   you decide "let's parse OVF", be precise about which format(s) you
>   actually want to parse.
> 
>   Also OVF is a hideous format.  Many fields are obviously internal data
>   dumps of VMware structures, complete with internal VMware IDs instead
>   of descriptive names.  Where there are descriptive names, they use
>   English strings instead of keywords, like: 
> MetaBytes
>   or my particular WTF favourite, a meaningful field which references
>   English (only) Wikipedia:
> 
> http://en.wikipedia.org/wiki/Byte;>
> 
>   File references are split over two places, and there are other
>   examples where data is needlessly duplicated or it's unclear what data
>   is supposed to be.
> 
>   Of course VMware Inc. are not stupid enough to use this format for
>   their own purposes.  They use a completely different format (VMX)
>   which is a lot like YAML.
> 
> Rich.
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> Fedora Windows cross-compiler. Compile Windows programs, test, and
> build Windows installers. Over 100 libraries supported.
> http://fedoraproject.org/wiki/MinGW
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-block] [Qemu-devel] storing machine data in qcow images?

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:

> Seriously, though: are disk images a sane way to package software?

They're not actually terrible.
You can just start a VM with them straight away, no unpacking required,
no installation time.  Especially when used with something like
-snapshot.
It's also easy for the person packaging them.

Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK