date:20190222

Even though the status field is deprecated, we still have to support
it for a few more releases. Since this is a very new kind of bitmap
state, it makes sense for it to have its own status field.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 7 ++-
 qapi/block-core.json | 7 ++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9042c04788..4e8f931659 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,10 +209,15 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
  *   or it can be Disabled and not recording writes.
  * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
  *   in any way from the monitor.
+ * (5) Inconsistent: This is a persistent bitmap whose "in use" bit is set, and
+ *   is unusable by QEMU. It can be deleted to remove it from
+ *   the qcow2.
  */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
-if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+return DIRTY_BITMAP_STATUS_INCONSISTENT;
+} else if (bdrv_dirty_bitmap_has_successor(bitmap)) {
 return DIRTY_BITMAP_STATUS_FROZEN;
 } else if (bdrv_dirty_bitmap_busy(bitmap)) {
 return DIRTY_BITMAP_STATUS_LOCKED;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a7209fce22..b25ca6a6c1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -442,10 +442,15 @@
 #  recording new writes. If the bitmap was @disabled, it is not
 #  recording new writes. (Since 2.12)
 #
+# @inconsistent: This is a persistent dirty bitmap that was marked in-use on
+#disk, and is unusable by QEMU. It can only be deleted.
+#Please rely on the inconsistent field in @BlockDirtyInfo
+#instead, as the status field is deprecated. (Since 4.0)
+#
 # Since: 2.4
 ##
 { 'enum': 'DirtyBitmapStatus',
-  'data': ['active', 'disabled', 'frozen', 'locked'] }
+  'data': ['active', 'disabled', 'frozen', 'locked', 'inconsistent'] }
 
 ##
 # @BlockDirtyInfo:
-- 
2.17.2

[Qemu-block] [PATCH v2 4/4] block/dirty-bitmaps: implement inconsistent bit

Set the inconsistent bit on load instead of rejecting such bitmaps.
There is no way to un-set it; the only option is to delete it.

Obvervations:
- bitmap loading does not need to update the header for in_use bitmaps.
- inconsistent bitmaps don't need to have their data loaded; they're
  glorified corruption sentinels.
- bitmap saving does not need to save inconsistent bitmaps back to disk.
- bitmap reopening DOES need to drop the readonly flag from inconsistent
  bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
  being eventually flushed back to disk.

Signed-off-by: John Snow 
---
 block/qcow2-bitmap.c | 77 +++-
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3ee524da4b..d1cc11da88 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
 uint32_t granularity;
 BdrvDirtyBitmap *bitmap = NULL;
 
+granularity = 1U << bm->granularity_bits;
+bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+if (bitmap == NULL) {
+goto fail;
+}
+
 if (bm->flags & BME_FLAG_IN_USE) {
-error_setg(errp, "Bitmap '%s' is in use", bm->name);
-goto fail;
+/* Data is unusable, skip loading it */
+return bitmap;
 }
 
 ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
@@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
 goto fail;
 }
 
-granularity = 1U << bm->granularity_bits;
-bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
-if (bitmap == NULL) {
-goto fail;
-}
-
 ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
@@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error 
**errp)
 Qcow2Bitmap *bm;
 GSList *created_dirty_bitmaps = NULL;
 bool header_updated = false;
+bool needs_update = false;
 
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
@@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error 
**errp)
 }
 
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-if (!(bm->flags & BME_FLAG_IN_USE)) {
-BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-if (bitmap == NULL) {
-goto fail;
-}
+BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+if (bitmap == NULL) {
+goto fail;
+}
 
-if (!(bm->flags & BME_FLAG_AUTO)) {
-bdrv_disable_dirty_bitmap(bitmap);
-}
-bdrv_dirty_bitmap_set_persistance(bitmap, true);
+if (bm->flags & BME_FLAG_IN_USE) {
+bdrv_dirty_bitmap_set_inconsistent(bitmap);
+} else {
+/* NB: updated flags only get written if can_write(bs) is true. */
 bm->flags |= BME_FLAG_IN_USE;
-created_dirty_bitmaps =
-g_slist_append(created_dirty_bitmaps, bitmap);
+needs_update = true;
 }
+if (!(bm->flags & BME_FLAG_AUTO)) {
+bdrv_disable_dirty_bitmap(bitmap);
+}
+bdrv_dirty_bitmap_set_persistance(bitmap, true);
+created_dirty_bitmaps =
+g_slist_append(created_dirty_bitmaps, bitmap);
 }
 
-if (created_dirty_bitmaps != NULL) {
+if (needs_update) {
 if (can_write(bs)) {
 /* in_use flags must be updated */
 int ret = update_ext_header_and_dir_in_place(bs, bm_list);
@@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 }
 
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-if (!(bm->flags & BME_FLAG_IN_USE)) {
-BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-if (bitmap == NULL) {
-continue;
-}
-
-if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-error_setg(errp, "Bitmap %s is not readonly but not marked"
- "'IN_USE' in the image. Something went wrong,"
- "all the bitmaps may be corrupted", bm->name);
-ret = -EINVAL;
-goto out;
-}
+BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+if (bitmap == NULL) {
+continue;
+}
 
-bm->flags |= BME_FLAG_IN_USE;
-ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was 
"
+   "not marked as readonly. This is a bug, something went "
+   "wrong. All of the bitmaps may be corrupted",

[Qemu-block] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
that have been marked as "in use".

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 19 +++
 include/block/dirty-bitmap.h |  2 ++
 qapi/block-core.json |  8 ++--
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 86c3b87ab9..9042c04788 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
and this bitmap must remain unchanged while
this flag is set. */
 bool persistent;/* bitmap must be saved to owner disk image */
+bool inconsistent;  /* bitmap is persistent, but not owned by QEMU.
+ * It cannot be used at all in any way, except
+ * a QMP user can remove it. */
 bool migration; /* Bitmap is selected for migration, it should
not be stored on the next inactivation
(persistent flag doesn't matter until next
@@ -462,6 +465,8 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 info->recording = bdrv_dirty_bitmap_recording(bm);
 info->busy = bdrv_dirty_bitmap_busy(bm);
 info->persistent = bm->persistent;
+info->has_inconsistent = bm->inconsistent;
+info->inconsistent = bm->inconsistent;
 entry->value = info;
 *plist = entry;
 plist = &entry->next;
@@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap 
*bitmap, bool persistent)
 qemu_mutex_unlock(bitmap->mutex);
 }
 
+/* Called with BQL taken. */
+void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
+{
+qemu_mutex_lock(bitmap->mutex);
+bitmap->inconsistent = true;
+bitmap->disabled = true;
+qemu_mutex_unlock(bitmap->mutex);
+}
+
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
 {
@@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap 
*bitmap)
 return bitmap->persistent && !bitmap->migration;
 }
 
+bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
+{
+return bitmap->inconsistent;
+}
+
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
 {
 BdrvDirtyBitmap *bm;
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ba8477b73f..b270773f7e 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap 
*bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
bool persistent);
+void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
  HBitmap **backup, Error **errp);
@@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap 
*bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6e543594b3..a7209fce22 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -470,12 +470,16 @@
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #  storage (since 4.0)
 #
+# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
+#Implies @recording and @busy to be false. To reclaim
+#ownership, use @block-dirty-bitmap-remove. (since 4.0)
+#
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-   'recording': 'bool', 'busy': 'bool',
-   'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
+   'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
+   'persistent': 'bool', '*inconsistent': 'bool' } }
 
 ##
 # @Qcow2BitmapInfoFlags:
-- 
2.17.2

[Qemu-block] [PATCH v2 0/4] bitmaps: add inconsistent bit

Allow QEMU to read in bitmaps that have the in-use bit set, for the
purposes of allowing users to delete those bitmaps.

This is chosen in preference to a hard error on load to minimize
impact for a non-critical error, but to force the user or management
utility to acknowledge that the bitmap is no longer viable.

Requires: [PATCH v3 00/10] dirty-bitmaps: deprecate @status field

John Snow (4):
  block/dirty-bitmaps: add inconsistent bit
  block/dirty-bitmap: add inconsistent status
  block/dirty-bitmaps: add block_dirty_bitmap_check function
  block/dirty-bitmaps: implement inconsistent bit

 block/dirty-bitmap.c   | 65 +++-
 block/qcow2-bitmap.c   | 77 ++
 blockdev.c | 49 --
 include/block/dirty-bitmap.h   | 15 ++-
 migration/block-dirty-bitmap.c | 12 ++
 nbd/server.c   |  3 +-
 qapi/block-core.json   | 15 +--
 7 files changed, 133 insertions(+), 103 deletions(-)

-- 
2.17.2

[Qemu-block] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

Instead of checking against busy, inconsistent, or read only directly,
use a check function with permissions bits that let us streamline the
checks without reproducing them in many places.

As a side effect, this adds consistency checks to all the QMP
interfaces for bitmap manipulation.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c   | 39 ---
 blockdev.c | 49 +++---
 include/block/dirty-bitmap.h   | 13 -
 migration/block-dirty-bitmap.c | 12 +++--
 nbd/server.c   |  3 +--
 5 files changed, 54 insertions(+), 62 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4e8f931659..2e7fd81866 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap 
*bitmap)
 return bitmap->successor;
 }
 
-bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+static bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
 return bitmap->busy;
 }
 
@@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap 
*bitmap)
  !bitmap->successor->disabled);
 }
 
+int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
+Error **errp)
+{
+if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
+error_setg(errp, "Bitmap '%s' is currently in use by another"
+   " operation and cannot be used", bitmap->name);
+return -1;
+}
+
+if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+   bitmap->name);
+return -1;
+}
+
+if ((flags & BDRV_BITMAP_INCONSISTENT) &&
+bdrv_dirty_bitmap_inconsistent(bitmap)) {
+error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
+   bitmap->name);
+error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
+  "this bitmap from disk");
+return -1;
+}
+
+return 0;
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an 
operation.
  * Requires that the bitmap is not user_locked and has no successor.
@@ -792,15 +819,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const 
BdrvDirtyBitmap *src,
 
 qemu_mutex_lock(dest->mutex);
 
-if (bdrv_dirty_bitmap_busy(dest)) {
-error_setg(errp, "Bitmap '%s' is currently in use by another"
-" operation and cannot be modified", dest->name);
-goto out;
-}
-
-if (bdrv_dirty_bitmap_readonly(dest)) {
-error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
-   dest->name);
+if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
 goto out;
 }
 
diff --git a/blockdev.c b/blockdev.c
index cbce44877d..5d74479ba7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2007,11 +2007,7 @@ static void 
block_dirty_bitmap_clear_prepare(BlkActionState *common,
 return;
 }
 
-if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-error_setg(errp, "Cannot modify a bitmap in use by another operation");
-return;
-} else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
-error_setg(errp, "Cannot clear a readonly bitmap");
+if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
 return;
 }
 
@@ -2056,10 +2052,7 @@ static void 
block_dirty_bitmap_enable_prepare(BlkActionState *common,
 return;
 }
 
-if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-error_setg(errp,
-   "Bitmap '%s' is currently in use by another operation"
-   " and cannot be enabled", action->name);
+if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
 return;
 }
 
@@ -2097,10 +2090,7 @@ static void 
block_dirty_bitmap_disable_prepare(BlkActionState *common,
 return;
 }
 
-if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-error_setg(errp,
-   "Bitmap '%s' is currently in use by another operation"
-   " and cannot be disabled", action->name);
+if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
 return;
 }
 
@@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, 
const char *name,
 return;
 }
 
-if (bdrv_dirty_bitmap_busy(bitmap)) {
-error_setg(errp,
-   "Bitmap '%s' is currently in use by another operation and"
-   " cannot be removed", name);
+if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
 return;
 }
 
@@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, 
const char *name,
 return;
 }
 
-if (bdrv_dirty_bitmap_busy(bitmap)) {
-error_setg(err

[Qemu-block] [PATCH v3 07/10] block/dirty-bitmaps: unify qmp_locked and user_locked calls

These mean the same thing now. Unify them and rename the merged call
bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
as well as help disambiguate from the various _locked and _unlocked
versions of bitmap helpers that refer to mutex locks.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c   | 40 +++---
 blockdev.c | 18 +++
 include/block/dirty-bitmap.h   |  5 ++---
 migration/block-dirty-bitmap.c |  6 ++---
 nbd/server.c   |  6 ++---
 5 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d92a269753..b3627b0d8c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,8 +48,7 @@ struct BdrvDirtyBitmap {
 QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty bitmap implementation */
 HBitmap *meta;  /* Meta dirty bitmap */
-bool qmp_locked;/* Bitmap is locked, it can't be modified
-   through QMP */
+bool busy;  /* Bitmap is busy, it can't be used via QMP */
 BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
 char *name; /* Optional non-empty unique ID */
 int64_t size;   /* Size of the bitmap, in bytes */
@@ -188,22 +187,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap 
*bitmap)
 return bitmap->successor;
 }
 
-bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_qmp_locked(bitmap);
+bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+return bitmap->busy;
 }
 
-void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
+void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
 {
 qemu_mutex_lock(bitmap->mutex);
-bitmap->qmp_locked = qmp_locked;
+bitmap->busy = busy;
 qemu_mutex_unlock(bitmap->mutex);
 }
 
-bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
-{
-return bitmap->qmp_locked;
-}
-
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
@@ -215,7 +209,7 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap 
*bitmap)
 {
 if (bdrv_dirty_bitmap_has_successor(bitmap)) {
 return DIRTY_BITMAP_STATUS_FROZEN;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+} else if (bdrv_dirty_bitmap_busy(bitmap)) {
 return DIRTY_BITMAP_STATUS_LOCKED;
 } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
 return DIRTY_BITMAP_STATUS_DISABLED;
@@ -243,7 +237,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 uint64_t granularity;
 BdrvDirtyBitmap *child;
 
-if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+if (bdrv_dirty_bitmap_busy(bitmap)) {
 error_setg(errp, "Cannot create a successor for a bitmap that is 
in-use "
"by an operation");
 return -1;
@@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 child->disabled = bitmap->disabled;
 bitmap->disabled = true;
 
-/* Install the successor and lock the parent */
+/* Install the successor and mark the parent as busy */
 bitmap->successor = child;
-bitmap->qmp_locked = true;
+bitmap->busy = true;
 return 0;
 }
 
@@ -289,7 +283,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap 
*bitmap)
 static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
 assert(!bitmap->active_iterators);
-assert(!bdrv_dirty_bitmap_user_locked(bitmap));
+assert(!bdrv_dirty_bitmap_busy(bitmap));
 assert(!bitmap->meta);
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
@@ -321,7 +315,7 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->successor = NULL;
 successor->persistent = bitmap->persistent;
 bitmap->persistent = false;
-bitmap->qmp_locked = false;
+bitmap->busy = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
 
 return successor;
@@ -330,7 +324,7 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 /**
  * In cases of failure where we can no longer safely delete the parent,
  * we may wish to re-join the parent and child/successor.
- * The merged parent will not be user_locked.
+ * The merged parent will be marked as not busy.
  * The marged parent will be enabled if and only if the successor was enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
@@ -351,7 +345,7 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
 }
 
 parent->disabled = successor->disabled;
-parent->qmp_locked = false;
+parent->busy = false;
 bdrv_release_dirty_bitmap_locked(successor);
 parent->successor = NULL;
 
@@ -382,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, 
int64_t bytes)
 
 bdrv_dirty_bitmaps_lock(bs);
 QLIST_FOREACH(bitmap, &bs->dirty_

[Qemu-block] [PATCH v3 03/10] block/dirty-bitmap: remove set/reset assertions against enabled bit

bdrv_set_dirty_bitmap and bdrv_reset_dirty_bitmap are only used as an
internal API by the mirror and migration areas of our code. These
calls modify the bitmap, but do so at the behest of QEMU and not the
guest.

Presently, these bitmaps are always "enabled" anyway, but there's no
reason they have to be.

Modify these internal APIs to drop this assertion.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index aa3f86bb73..9ea5738420 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -542,7 +542,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
   int64_t offset, int64_t bytes)
 {
-assert(bdrv_dirty_bitmap_enabled(bitmap));
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_set(bitmap->bitmap, offset, bytes);
 }
@@ -559,7 +558,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t offset, int64_t bytes)
 {
-assert(bdrv_dirty_bitmap_enabled(bitmap));
 assert(!bdrv_dirty_bitmap_readonly(bitmap));
 hbitmap_reset(bitmap->bitmap, offset, bytes);
 }
-- 
2.17.2

[Qemu-block] [PATCH v3 08/10] block/dirty-bitmaps: move comment block

Simply move the big status enum comment block to above the status
function, and document it as being deprecated. The whole confusing
block can get deleted in three releases time.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index b3627b0d8c..86c3b87ab9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -28,22 +28,6 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 
-/**
- * A BdrvDirtyBitmap can be in four possible user-visible states:
- * (1) Active:   successor is NULL, and disabled is false: full r/w mode
- * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
- *   guest writes are dropped, but monitor writes are possible,
- *   through commands like merge and clear.
- * (3) Frozen:   successor is not NULL.
- *   A frozen bitmap cannot be renamed, deleted, cleared, set,
- *   enabled, merged to, etc. A frozen bitmap can only abdicate()
- *   or reclaim().
- *   In this state, the anonymous successor bitmap may be either
- *   Active and recording writes from the guest (e.g. backup jobs),
- *   but it can be Disabled and not recording writes.
- * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
- *   in any way from the monitor.
- */
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty bitmap implementation */
@@ -204,7 +188,25 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 return !bitmap->disabled;
 }
 
-/* Called with BQL taken.  */
+/**
+ * bdrv_dirty_bitmap_status: This API is now deprecated.
+ * Called with BQL taken.
+ *
+ * A BdrvDirtyBitmap can be in four possible user-visible states:
+ * (1) Active:   successor is NULL, and disabled is false: full r/w mode
+ * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
+ *   guest writes are dropped, but monitor writes are possible,
+ *   through commands like merge and clear.
+ * (3) Frozen:   successor is not NULL.
+ *   A frozen bitmap cannot be renamed, deleted, cleared, set,
+ *   enabled, merged to, etc. A frozen bitmap can only abdicate()
+ *   or reclaim().
+ *   In this state, the anonymous successor bitmap may be either
+ *   Active and recording writes from the guest (e.g. backup jobs),
+ *   or it can be Disabled and not recording writes.
+ * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
+ *   in any way from the monitor.
+ */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
 if (bdrv_dirty_bitmap_has_successor(bitmap)) {
-- 
2.17.2

[Qemu-block] [PATCH v3 09/10] blockdev: remove unused paio parameter documentation

This field isn't present anymore.

Signed-off-by: John Snow 
---
 blockdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 1aaadb6128..cbce44877d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1255,7 +1255,6 @@ out_aio_context:
  * @node: The name of the BDS node to search for bitmaps
  * @name: The name of the bitmap to search for
  * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
- * @paio: Output pointer for aio_context acquisition, if desired. Can be NULL.
  * @errp: Output pointer for error information. Can be NULL.
  *
  * @return: A bitmap object on success, or NULL on failure.
-- 
2.17.2

[Qemu-block] [PATCH v3 05/10] nbd: change error checking order for bitmaps

Check that the bitmap is not in use prior to it checking if it is
not enabled/recording guest writes. The bitmap being busy was likely
at the behest of the user, so this error has a greater chance of being
understood by the user.

Signed-off-by: John Snow 
---
 nbd/server.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 0910d09a6d..de21c64ce3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1510,6 +1510,11 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t 
dev_offset,
 goto fail;
 }
 
+if (bdrv_dirty_bitmap_user_locked(bm)) {
+error_setg(errp, "Bitmap '%s' is in use", bitmap);
+goto fail;
+}
+
 if ((nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
 bdrv_dirty_bitmap_enabled(bm)) {
 error_setg(errp,
@@ -1518,11 +1523,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t 
dev_offset,
 goto fail;
 }
 
-if (bdrv_dirty_bitmap_user_locked(bm)) {
-error_setg(errp, "Bitmap '%s' is in use", bitmap);
-goto fail;
-}
-
 bdrv_dirty_bitmap_set_qmp_locked(bm, true);
 exp->export_bitmap = bm;
 exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
-- 
2.17.2

[Qemu-block] [PATCH v3 00/10] dirty-bitmaps: deprecate @status field

The current internal meanings of "locked", "user_locked",
"qmp_locked", "frozen", "enabled", and "disabled" are all
a little muddled.

Deprecate the @status field in favor of two new booleans
that carry very specific meanings. Then, rename and rework
some of the internal semantics to help make the API a bit
more clear and easier to read.

Well, in my opinion.

Based on my current bitmaps branch.

V3:

[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/10:[0002] [FC] 'block/dirty-bitmap: add recording and busy properties'
002/10:[0002] [FC] 'block/dirty-bitmaps: rename frozen predicate helper'
003/10:[down] 'block/dirty-bitmap: remove set/reset assertions against enabled 
bit'
004/10:[0006] [FC] 'block/dirty-bitmap: change semantics of enabled predicate'
005/10:[down] 'nbd: change error checking order for bitmaps'
006/10:[0002] [FC] 'block/dirty-bitmap: explicitly lock bitmaps with successors'
007/10:[0011] [FC] 'block/dirty-bitmaps: unify qmp_locked and user_locked calls'
008/10:[0002] [FC] 'block/dirty-bitmaps: move comment block'
009/10:[down] 'blockdev: remove unused paio parameter documentation'
010/10:[down] 'iotests: add busy/recording bit test to 124'

V3: (Following Vladimir's feedback)

001: Changed texi phrasing, parameter --> field
002: Commit message adjustment
 comment edit: "not locked" --> "not user_locked"
003: pulled forward out of patch 004
004: Commit message rewrite
 create_successor and abdicate doc adjustments
005: New.
006: Change successor doc comment
 Commit message adjustment
007: BdrvDirtyBitmap struct comment adjustments
 Comment change to create_successor (lock --> busy)
 Incidental changes from 004's changes
008: Grammar fix (Eric)
009: New, trivial, unrelated fix tagging along.
010: iotest 124 added.

John Snow (10):
  block/dirty-bitmap: add recording and busy properties
  block/dirty-bitmaps: rename frozen predicate helper
  block/dirty-bitmap: remove set/reset assertions against enabled bit
  block/dirty-bitmap: change semantics of enabled predicate
  nbd: change error checking order for bitmaps
  block/dirty-bitmap: explicitly lock bitmaps with successors
  block/dirty-bitmaps: unify qmp_locked and user_locked calls
  block/dirty-bitmaps: move comment block
  blockdev: remove unused paio parameter documentation
  iotests: add busy/recording bit test to 124

 block/dirty-bitmap.c   | 111 ++---
 blockdev.c |  19 +++---
 include/block/dirty-bitmap.h   |   7 +--
 migration/block-dirty-bitmap.c |   8 +--
 nbd/server.c   |  14 ++---
 qapi/block-core.json   |  10 ++-
 qemu-deprecated.texi   |   6 ++
 tests/qemu-iotests/124 | 110 
 tests/qemu-iotests/124.out |   4 +-
 tests/qemu-iotests/236.out |  28 +
 10 files changed, 239 insertions(+), 78 deletions(-)

-- 
2.17.2

[Qemu-block] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen predicate helper

"Frozen" was a good description a long time ago, but it isn't adequate now.
Rename the frozen predicate to has_successor to make the semantics of the
predicate more clear to outside callers.

In the process, remove some calls to frozen() that no longer semantically
make sense. For bdrv_enable_dirty_bitmap_locked and
bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
internals from performing this action when we only wished to prohibit QMP
users from issuing these commands. All of the QMP API commands for bitmap
manipulation already check against user_locked() to prohibit these actions.

Several other assertions really want to check that the bitmap isn't in-use
by another operation -- use the bitmap_user_locked function for this instead,
which presently also checks for has_successor. This leaves some redundant
checks of has_sucessor through different helpers that are addressed in
forthcoming patches.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c   | 32 +---
 include/block/dirty-bitmap.h   |  2 +-
 migration/block-dirty-bitmap.c |  2 +-
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 101383b3af..aa3f86bb73 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -50,7 +50,7 @@ struct BdrvDirtyBitmap {
 HBitmap *meta;  /* Meta dirty bitmap */
 bool qmp_locked;/* Bitmap is locked, it can't be modified
through QMP */
-BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state 
*/
 char *name; /* Optional non-empty unique ID */
 int64_t size;   /* Size of the bitmap, in bytes */
 bool disabled;  /* Bitmap is disabled. It ignores all writes to
@@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap 
*bitmap)
 }
 
 /* Called with BQL taken.  */
-bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
 {
 return bitmap->successor;
 }
 
 /* Both conditions disallow user-modification via QMP. */
 bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_frozen(bitmap) ||
+return bdrv_dirty_bitmap_has_successor(bitmap) ||
bdrv_dirty_bitmap_qmp_locked(bitmap);
 }
 
@@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 /* Called with BQL taken.  */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (bdrv_dirty_bitmap_has_successor(bitmap)) {
 return DIRTY_BITMAP_STATUS_FROZEN;
 } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
 return DIRTY_BITMAP_STATUS_LOCKED;
@@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap 
*bitmap)
 
 /**
  * Create a successor bitmap destined to replace this bitmap after an 
operation.
- * Requires that the bitmap is not frozen and has no successor.
+ * Requires that the bitmap is not user_locked and has no successor.
  * Called with BQL taken.
  */
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
@@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
*bs,
 uint64_t granularity;
 BdrvDirtyBitmap *child;
 
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
-error_setg(errp, "Cannot create a successor for a bitmap that is "
-   "currently frozen");
+if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+error_setg(errp, "Cannot create a successor for a bitmap that is 
in-use "
+   "by an operation");
+return -1;
+}
+if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+error_setg(errp, "Cannot create a successor for a bitmap that already "
+   "has one");
 return -1;
 }
-assert(!bitmap->successor);
 
 /* Create an anonymous successor */
 granularity = bdrv_dirty_bitmap_granularity(bitmap);
@@ -268,7 +272,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
-assert(!bdrv_dirty_bitmap_frozen(bitmap));
 bitmap->disabled = false;
 }
 
@@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap 
*bitmap)
 static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
 assert(!bitmap->active_iterators);
-assert(!bdrv_dirty_bitmap_frozen(bitmap));
+assert(!bdrv_dirty_bitmap_user_locked(bitmap));
 assert(!bitmap->meta);
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
@@ -325,7 +328,7 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 /**
  * In cases of failure where we can no longer safely delete the parent,
  * we may wish to re-join the parent and child/successor.
- * Th

[Qemu-block] [PATCH v3 10/10] iotests: add busy/recording bit test to 124

This adds a simple test that ensures the busy bit works for push backups,
as well as doubling as bonus test for incremental backups that get interrupted
by EIO errors.

Recording bit tests are already handled sufficiently by 236.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/124 | 110 +
 tests/qemu-iotests/124.out |   4 +-
 2 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 5aa1bf1bd6..30f12a2202 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -634,6 +634,116 @@ class 
TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
 self.vm.shutdown()
 self.check_backups()
 
+def test_incremental_pause(self):
+"""
+Test an incremental backup that errors into a pause and is resumed.
+"""
+
+drive0 = self.drives[0]
+result = self.vm.qmp('blockdev-add',
+ node_name=drive0['id'],
+ driver=drive0['fmt'],
+ file={
+ 'driver': 'blkdebug',
+ 'image': {
+ 'driver': 'file',
+ 'filename': drive0['file']
+ },
+ 'set-state': [{
+ 'event': 'flush_to_disk',
+ 'state': 1,
+ 'new_state': 2
+ },{
+ 'event': 'read_aio',
+ 'state': 2,
+ 'new_state': 3
+ }],
+ 'inject-error': [{
+ 'event': 'read_aio',
+ 'errno': 5,
+ 'state': 3,
+ 'immediately': False,
+ 'once': True
+ }],
+ })
+self.assert_qmp(result, 'return', {})
+self.create_anchor_backup(drive0)
+bitmap = self.add_bitmap('bitmap0', drive0)
+
+# Emulate guest activity
+self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+  ('0xfe', '16M', '256k'),
+  ('0x64', '32736k', '64k')))
+
+# For the purposes of query-block visibility of bitmaps, add a drive
+# frontend after we've written data; otherwise we can't use hmp-io
+result = self.vm.qmp("device_add",
+ id="device0",
+ drive=drive0['id'],
+ driver="virtio-blk")
+self.assert_qmp(result, 'return', {})
+
+# Bitmap Status Check
+query = self.vm.qmp('query-block')
+ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
+   if bmap.get('name') == bitmap.name][0]
+self.assert_qmp(ret, 'count', 458752)
+self.assert_qmp(ret, 'granularity', 65536)
+self.assert_qmp(ret, 'status', 'active')
+self.assert_qmp(ret, 'busy', False)
+self.assert_qmp(ret, 'recording', True)
+
+# Start backup
+parent, _ = bitmap.last_target()
+target = self.prepare_backup(bitmap, parent)
+res = self.vm.qmp('drive-backup',
+  job_id=bitmap.drive['id'],
+  device=bitmap.drive['id'],
+  sync='incremental',
+  bitmap=bitmap.name,
+  format=bitmap.drive['fmt'],
+  target=target,
+  mode='existing',
+  on_source_error='stop')
+self.assert_qmp(res, 'return', {})
+
+# Wait for the error
+event = self.vm.event_wait(name="BLOCK_JOB_ERROR",
+   
match={"data":{"device":bitmap.drive['id']}})
+self.assert_qmp(event, 'data', {'device': bitmap.drive['id'],
+'action': 'stop',
+'operation': 'read'})
+
+# Bitmap Status Check
+query = self.vm.qmp('query-block')
+ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
+   if bmap.get('name') == bitmap.name][0]
+self.assert_qmp(ret, 'count', 458752)
+self.assert_qmp(ret, 'granularity', 65536)
+self.assert_qmp(ret, 'status', 'frozen')
+self.assert_qmp(ret, 'busy', True)
+self.assert_qmp(ret, 'recording', True)
+
+# Resume and check incremental backup for consistency
+res = self.vm.qmp('block-job-resume', device=bitmap.drive['id'])
+self.assert_qmp(res, 'r

[Qemu-block] [PATCH v3 06/10] block/dirty-bitmap: explicitly lock bitmaps with successors

Instead of implying a user_locked/busy status, make it explicit.
Now, bitmaps in use by migration, NBD or backup operations
are all treated the same way with the same code paths.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
---
 block/dirty-bitmap.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 976831e765..d92a269753 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -50,7 +50,7 @@ struct BdrvDirtyBitmap {
 HBitmap *meta;  /* Meta dirty bitmap */
 bool qmp_locked;/* Bitmap is locked, it can't be modified
through QMP */
-BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state 
*/
+BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
 char *name; /* Optional non-empty unique ID */
 int64_t size;   /* Size of the bitmap, in bytes */
 bool disabled;  /* Bitmap is disabled. It ignores all writes to
@@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap 
*bitmap)
 return bitmap->successor;
 }
 
-/* Both conditions disallow user-modification via QMP. */
 bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-return bdrv_dirty_bitmap_has_successor(bitmap) ||
-   bdrv_dirty_bitmap_qmp_locked(bitmap);
+return bdrv_dirty_bitmap_qmp_locked(bitmap);
 }
 
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
@@ -267,8 +265,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 child->disabled = bitmap->disabled;
 bitmap->disabled = true;
 
-/* Install the successor and freeze the parent */
+/* Install the successor and lock the parent */
 bitmap->successor = child;
+bitmap->qmp_locked = true;
 return 0;
 }
 
@@ -322,6 +321,7 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 bitmap->successor = NULL;
 successor->persistent = bitmap->persistent;
 bitmap->persistent = false;
+bitmap->qmp_locked = false;
 bdrv_release_dirty_bitmap(bs, bitmap);
 
 return successor;
@@ -351,6 +351,7 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
 }
 
 parent->disabled = successor->disabled;
+parent->qmp_locked = false;
 bdrv_release_dirty_bitmap_locked(successor);
 parent->successor = NULL;
 
-- 
2.17.2

[Qemu-block] [PATCH v3 04/10] block/dirty-bitmap: change semantics of enabled predicate

Currently, the enabled predicate means something like:
"the QAPI status of the bitmap is ACTIVE."
After this patch, it should mean exclusively:
"This bitmap is recording guest writes, and is allowed to do so."

In many places, this is how this predicate was already used.
Internal usages of the bitmap QPI can call user_locked to find out if
the bitmap is in use by an operation.

To accommodate this, modify the create_successor routine to now
explicitly disable the parent bitmap at creation time.


Justifications:

1. bdrv_dirty_bitmap_status suffers no change from the lack of
   1:1 parity with the new predicates because of the order in which
   the predicates are checked. This is now only for compatibility.

2. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that were
   disabled or had a successor, while post-patch it is only skipping bitmaps
   that are disabled. To accommodate this, create_successor now ensures that
   any bitmap with a successor is explicitly disabled.

3. qcow2_store_persistent_dirty_bitmaps: No functional change. This function
   cares only about the literal enabled bit, and makes no effort to check if
   the bitmap is in-use or not. After this patch there are still no ways to
   produce an enabled bitmap with a successor.

4. block_dirty_bitmap_enable_prepare
   block_dirty_bitmap_disable_prepare
   init_dirty_bitmap_migration
   nbd_export_new

   These functions care about the literal enabled bit,
   and already check user_locked separately.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9ea5738420..976831e765 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
-return !(bitmap->disabled || bitmap->successor);
+return !bitmap->disabled;
 }
 
 /* Called with BQL taken.  */
@@ -236,6 +236,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap 
*bitmap)
 /**
  * Create a successor bitmap destined to replace this bitmap after an 
operation.
  * Requires that the bitmap is not user_locked and has no successor.
+ * The successor will be enabled if the parent bitmap was.
  * Called with BQL taken.
  */
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
@@ -264,6 +265,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
 /* Successor will be on or off based on our current state. */
 child->disabled = bitmap->disabled;
+bitmap->disabled = true;
 
 /* Install the successor and freeze the parent */
 bitmap->successor = child;
@@ -328,7 +330,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 /**
  * In cases of failure where we can no longer safely delete the parent,
  * we may wish to re-join the parent and child/successor.
- * The merged parent will not be user_locked, nor explicitly re-enabled.
+ * The merged parent will not be user_locked.
+ * The marged parent will be enabled if and only if the successor was enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
 BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
@@ -346,6 +349,8 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
 error_setg(errp, "Merging of parent and successor bitmap failed");
 return NULL;
 }
+
+parent->disabled = successor->disabled;
 bdrv_release_dirty_bitmap_locked(successor);
 parent->successor = NULL;
 
-- 
2.17.2

[Qemu-block] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties

The current API allows us to report a single status, which we've defined as:

Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
Locked: no successor, qmp_locked. may or may not be enabled.
Disabled: Not frozen or locked, disabled.
Active: Not frozen, locked, or disabled.

The problem is that both "Frozen" and "Locked" mean nearly the same thing,
and that both of them do not intuit whether they are recording guest writes
or not.

This patch deprecates that status field and introduces two orthogonal
properties instead to replace it.

Signed-off-by: John Snow 
---
 block/dirty-bitmap.c   |  9 +
 qapi/block-core.json   | 10 +-
 qemu-deprecated.texi   |  6 ++
 tests/qemu-iotests/236.out | 28 
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c6d4acebfa..101383b3af 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -226,6 +226,13 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap 
*bitmap)
 }
 }
 
+/* Called with BQL taken.  */
+static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
+{
+return !bitmap->disabled || (bitmap->successor &&
+ !bitmap->successor->disabled);
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an 
operation.
  * Requires that the bitmap is not frozen and has no successor.
@@ -448,6 +455,8 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 info->has_name = !!bm->name;
 info->name = g_strdup(bm->name);
 info->status = bdrv_dirty_bitmap_status(bm);
+info->recording = bdrv_dirty_bitmap_recording(bm);
+info->busy = bdrv_dirty_bitmap_user_locked(bm);
 info->persistent = bm->persistent;
 entry->value = info;
 *plist = entry;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 2b8afbb924..6e543594b3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -458,7 +458,14 @@
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
 #
-# @status: current status of the dirty bitmap (since 2.4)
+# @status: Deprecated in favor of @recording and @locked. (since 2.4)
+#
+# @recording: true if the bitmap is recording new writes from the guest.
+# Replaces `active` and `disabled` statuses. (since 4.0)
+#
+# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
+#and cannot be modified via QMP or used by another operation.
+#Replaces `locked` and `frozen` statuses. (since 4.0)
 #
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #  storage (since 4.0)
@@ -467,6 +474,7 @@
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+   'recording': 'bool', 'busy': 'bool',
'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
 
 ##
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 45c57952da..4c7ae8180f 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus, 
i.e.
 "autoload" parameter is now ignored. All bitmaps are automatically loaded
 from qcow2 images.
 
+@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0)
+
+The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
+the query-block command is deprecated. Two new boolean fields,
+``recording'' and ``busy'' effectively replace it.
+
 @subsection query-cpus (since 2.12.0)
 
 The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
index 5006f7bca1..815cd053f0 100644
--- a/tests/qemu-iotests/236.out
+++ b/tests/qemu-iotests/236.out
@@ -22,17 +22,21 @@ write -P0xcd 0x3ff 64k
   "bitmaps": {
 "drive0": [
   {
+"busy": false,
 "count": 262144,
 "granularity": 65536,
 "name": "bitmapB",
 "persistent": false,
+"recording": true,
 "status": "active"
   },
   {
+"busy": false,
 "count": 262144,
 "granularity": 65536,
 "name": "bitmapA",
 "persistent": false,
+"recording": true,
 "status": "active"
   }
 ]
@@ -84,17 +88,21 @@ write -P0xcd 0x3ff 64k
   "bitmaps": {
 "drive0": [
   {
+"busy": false,
 "count": 262144,
 "granularity": 65536,
 "name": "bitmapB",
 "persistent": false,
+"recording": true,
 "status": "active"
   },
   {
+"busy": false,
 "count": 262144,
 "granularity": 65536,
 "name": "bitmapA",
 "persistent": false,
+"recording": true,
 "status": "active"
   }
 ]
@@ -184,24 +192,30 @@ write -P0xea 0x3fe 64k
   "bitmaps": {
 "drive0": [
   {
+

Re: [Qemu-block] [ovirt-users] qemu-img info showed iscsi/FC lun size 0

2019-02-22 Thread Jingjie Jiang

Hi Nir,

Thanks for your reply.

What about qcow2 format?

Is there a choice to create vm disk with format qcow2 instead of raw?

Thanks,

Jingjie

On 2/21/19 5:46 PM, Nir Soffer wrote:

On Thu, Feb 21, 2019, 21:48 wrote:

Hi,
Based on oVirt 4.3.0, I have data domain from FC lun, then I
create new vm on the disk from FC data domain.
After VM was created. According to qemu-img info, the disk size is 0.
# qemu-img info

/rhev/data-center/mnt/blockSD/eaa6f641-6b36-4c1d-bf99-6ba77df3156f/images/8d3b455b-1da4-49f3-ba57-8cda64aa9dc9/949fa315-3934-4038-85f2-08aec52c1e2b

image:

/rhev/data-center/mnt/blockSD/eaa6f641-6b36-4c1d-bf99-6ba77df3156f/images/8d3b455b-1da4-49f3-ba57-8cda64aa9dc9/949fa315-3934-4038-85f2-08aec52c1e2b
file format: raw
virtual size: 10G (10737418240 bytes)
disk size: 0

I tried on iscsi and same result.

Is the behaviour expected?

It is expected in a way. Disk size is the amount of storage actually
used, and block devices has no way to tell that.

oVirt report the size of the block device in this case, which is more
accurate than zero.

However the real size allocated on the undrelying storage is somewhere
between zero an device size, and depends on the imlementation of the
storage. Nither qemu-img nor oVirt can tell the real size.

Nir

Thanks,
Jingjie

___
Users mailing list -- us...@ovirt.org
To unsubscribe send an email to users-le...@ovirt.org

Privacy Statement: https://www.ovirt.org/site/privacy-policy/
oVirt Code of Conduct:
https://www.ovirt.org/community/about/community-guidelines/
List Archives:

https://lists.ovirt.org/archives/list/us...@ovirt.org/message/TSXP7RENWIFMHIJWIAF6AGQPI3NOVNIZ/

[Qemu-block] [PULL 4/7] tests.acceptance: adds multi vm capability for acceptance tests

From: Caio Carrara 

This change adds the possibility to write acceptance tests with multi
virtual machine support. It's done keeping the virtual machines objects
stored in a test attribute (dictionary). This dictionary shouldn't be
accessed directly but through the new method added `get_vm`. This new
method accept a list of args (that will be added as virtual machine
arguments) and an optional name argument. The name is the key that
identify a single virtual machine along the test machines available. If
a name without a machine is informed a new machine will be instantiated.

The current usage of vm in tests will not be broken by this change since
it keeps a property called vm in the base test class. This property only
calls the new method `get_vm` with default parameters (no args and
'default' as machine name).

Signed-off-by: Caio Carrara 
Reviewed-by: Cleber Rosa 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20190212193855.13223-2-ccarr...@redhat.com>
Signed-off-by: Cleber Rosa 
---
 docs/devel/testing.rst| 41 ++-
 tests/acceptance/avocado_qemu/__init__.py | 25 +++---
 2 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 3ce171829d..60f897d915 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -633,7 +633,46 @@ instance, available at ``self.vm``.  Because many tests 
will tweak the
 QEMU command line, launching the QEMUMachine (by using ``self.vm.launch()``)
 is left to the test writer.
 
-At test "tear down", ``avocado_qemu.Test`` handles the QEMUMachine
+The base test class has also support for tests with more than one
+QEMUMachine. The way to get machines is through the ``self.get_vm()``
+method which will return a QEMUMachine instance. The ``self.get_vm()``
+method accepts arguments that will be passed to the QEMUMachine creation
+and also an optional `name` attribute so you can identify a specific
+machine and get it more than once through the tests methods. A simple
+and hypothetical example follows:
+
+.. code::
+
+  from avocado_qemu import Test
+
+
+  class MultipleMachines(Test):
+  """
+  :avocado: enable
+  """
+  def test_multiple_machines(self):
+  first_machine = self.get_vm()
+  second_machine = self.get_vm()
+  self.get_vm(name='third_machine').launch()
+
+  first_machine.launch()
+  second_machine.launch()
+
+  first_res = first_machine.command(
+  'human-monitor-command',
+  command_line='info version')
+
+  second_res = second_machine.command(
+  'human-monitor-command',
+  command_line='info version')
+
+  third_res = self.get_vm(name='third_machine').command(
+  'human-monitor-command',
+  command_line='info version')
+
+  self.assertEquals(first_res, second_res, third_res)
+
+At test "tear down", ``avocado_qemu.Test`` handles all the QEMUMachines
 shutdown.
 
 QEMUMachine
diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 28bfb8e9d3..a66ec72daa 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -10,6 +10,7 @@
 
 import os
 import sys
+import uuid
 
 import avocado
 
@@ -41,13 +42,29 @@ def pick_default_qemu_bin():
 
 class Test(avocado.Test):
 def setUp(self):
-self.vm = None
+self._vms = {}
 self.qemu_bin = self.params.get('qemu_bin',
 default=pick_default_qemu_bin())
 if self.qemu_bin is None:
 self.cancel("No QEMU binary defined or found in the source tree")
-self.vm = QEMUMachine(self.qemu_bin)
+
+def _new_vm(self, *args):
+vm = QEMUMachine(self.qemu_bin)
+if args:
+vm.add_args(*args)
+return vm
+
+@property
+def vm(self):
+return self.get_vm(name='default')
+
+def get_vm(self, *args, name=None):
+if not name:
+name = str(uuid.uuid4())
+if self._vms.get(name) is None:
+self._vms[name] = self._new_vm(*args)
+return self._vms[name]
 
 def tearDown(self):
-if self.vm is not None:
-self.vm.shutdown()
+for vm in self._vms.values():
+vm.shutdown()
-- 
2.20.1

[Qemu-block] [PULL 6/7] Acceptance tests: use linux-3.6 and set vm memory to 4GiB

From: Li Zhijian 

QEMU have already supported to load up to 4G initrd if the sepcified memory is
enough and XLF_CAN_BE_LOADED_ABOVE_4G is set by guest kernel

linux-3.6 kernel shipped by Fedora-18 cannot support xldflags so that it
cannot support loading more than 2GiB initrd

CC: Wainer dos Santos Moschetta 
CC: Caio Carrara 
CC: Cleber Rosa 
CC: Eduardo Habkost 
CC: Philippe Mathieu-Daudé 
Signed-off-by: Li Zhijian 
Reviewed-by: Wainer dos Santos Moschetta 
Reviewed-by: Cleber Rosa 
Message-Id: <1548638112-31101-1-git-send-email-lizhij...@cn.fujitsu.com>
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/linux_initrd.py | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
index 5a15fc4347..e33b5dcec0 100644
--- a/tests/acceptance/linux_initrd.py
+++ b/tests/acceptance/linux_initrd.py
@@ -23,14 +23,16 @@ class LinuxInitrd(Test):
 
 timeout = 60
 
-def test_with_2gib_file_should_exit_error_msg(self):
+def test_with_2gib_file_should_exit_error_msg_with_linux_v3_6(self):
 """
 Pretends to boot QEMU with an initrd file with size of 2GiB
 and expect it exits with error message.
+Fedora-18 shipped with linux-3.6 which have not supported xloadflags
+cannot support more than 2GiB initrd.
 """
-kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
-  'Everything/x86_64/os/images/pxeboot/vmlinuz')
-kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
+kernel_url = 
('https://archives.fedoraproject.org/pub/archive/fedora/li'
+  
'nux/releases/18/Fedora/x86_64/os/images/pxeboot/vmlinuz')
+kernel_hash = '41464f68efe42b9991250bed86c7081d2ccdbb21'
 kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
 max_size = 2 * (1024 ** 3) - 1
 
@@ -38,8 +40,8 @@ class LinuxInitrd(Test):
 initrd.seek(max_size)
 initrd.write(b'\0')
 initrd.flush()
-cmd = "%s -kernel %s -initrd %s" % (self.qemu_bin, kernel_path,
-initrd.name)
+cmd = "%s -kernel %s -initrd %s -m 4096" % (
+  self.qemu_bin, kernel_path, initrd.name)
 res = run(cmd, ignore_status=True)
 self.assertEqual(res.exit_status, 1)
 expected_msg = r'.*initrd is too large.*max: \d+, need %s.*' % (
-- 
2.20.1

[Qemu-block] [PULL 7/7] Acceptance tests: expect boot to extract 2GiB+ initrd with linux-v4.16

From: Li Zhijian 

XLF_CAN_BE_LOADED_ABOVE_4G is set on vmlinuz shipped by Fedora-28 so that
it's allowed to be loaded below 4 GB address.

timeout is updated to 5 minutes as well since we need more time to load a
large initrd to the guest

CC: Wainer dos Santos Moschetta 
CC: Caio Carrara 
CC: Cleber Rosa 
CC: Eduardo Habkost 
CC: Philippe Mathieu-Daudé 
Signed-off-by: Li Zhijian 
Reviewed-by: Wainer dos Santos Moschetta 
Reviewed-by: Cleber Rosa 
Message-Id: <1548638112-31101-2-git-send-email-lizhij...@cn.fujitsu.com>
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/linux_initrd.py | 37 +++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
index e33b5dcec0..fbdb48e43f 100644
--- a/tests/acceptance/linux_initrd.py
+++ b/tests/acceptance/linux_initrd.py
@@ -8,6 +8,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or
 # later.  See the COPYING file in the top-level directory.
 
+import logging
 import tempfile
 from avocado.utils.process import run
 
@@ -21,7 +22,7 @@ class LinuxInitrd(Test):
 :avocado: tags=x86_64
 """
 
-timeout = 60
+timeout = 300
 
 def test_with_2gib_file_should_exit_error_msg_with_linux_v3_6(self):
 """
@@ -47,3 +48,37 @@ class LinuxInitrd(Test):
 expected_msg = r'.*initrd is too large.*max: \d+, need %s.*' % (
 max_size + 1)
 self.assertRegex(res.stderr_text, expected_msg)
+
+def test_with_2gib_file_should_work_with_linux_v4_16(self):
+"""
+QEMU has supported up to 4 GiB initrd for recent kernel
+Expect guest can reach 'Unpacking initramfs...'
+"""
+kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
+  'Everything/x86_64/os/images/pxeboot/vmlinuz')
+kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
+kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+max_size = 2 * (1024 ** 3) + 1
+
+with tempfile.NamedTemporaryFile() as initrd:
+initrd.seek(max_size)
+initrd.write(b'\0')
+initrd.flush()
+
+self.vm.set_machine('pc')
+self.vm.set_console()
+kernel_command_line = 'console=ttyS0'
+self.vm.add_args('-kernel', kernel_path,
+ '-append', kernel_command_line,
+ '-initrd', initrd.name,
+ '-m', '5120')
+self.vm.launch()
+console = self.vm.console_socket.makefile()
+console_logger = logging.getLogger('console')
+while True:
+msg = console.readline()
+console_logger.debug(msg.strip())
+if 'Unpacking initramfs...' in msg:
+break
+if 'Kernel panic - not syncing' in msg:
+self.fail("Kernel panic reached")
-- 
2.20.1

[Qemu-block] [PULL 3/7] scripts/qemu.py: log QEMU launch command line

Even when the launch of QEMU succeeds, it's useful to have the command
line recorded.

Reviewed-by: Caio Carrara 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Alex Bennée 
Signed-off-by: Cleber Rosa 
Message-Id: <20190202005610.24048-2-cr...@redhat.com>
Signed-off-by: Cleber Rosa 
---
 python/qemu/__init__.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 38de3e9177..585cd2a1a3 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -321,6 +321,7 @@ class QEMUMachine(object):
 self._pre_launch()
 self._qemu_full_args = (self._wrapper + [self._binary] +
 self._base_args() + self._args)
+LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
 self._popen = subprocess.Popen(self._qemu_full_args,
stdin=devnull,
stdout=self._qemu_log_file,
-- 
2.20.1

[Qemu-block] [PULL 5/7] tests.acceptance: adds simple migration test

From: Caio Carrara 

This change adds the simplest possible migration test. Beyond the test
purpose itself it's also useful to exercise the multi virtual machines
capabilities from base avocado qemu test class.

Signed-off-by: Cleber Rosa 
Signed-off-by: Caio Carrara 
Reviewed-by: Cleber Rosa 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20190212193855.13223-3-ccarr...@redhat.com>
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/migration.py | 53 +++
 1 file changed, 53 insertions(+)
 create mode 100644 tests/acceptance/migration.py

diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
new file mode 100644
index 00..6115cf6c24
--- /dev/null
+++ b/tests/acceptance/migration.py
@@ -0,0 +1,53 @@
+# Migration test
+#
+# Copyright (c) 2019 Red Hat, Inc.
+#
+# Authors:
+#  Cleber Rosa 
+#  Caio Carrara 
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.  See the COPYING file in the top-level directory.
+
+
+from avocado_qemu import Test
+
+from avocado.utils import network
+from avocado.utils import wait
+
+
+class Migration(Test):
+"""
+:avocado: enable
+"""
+
+timeout = 10
+
+@staticmethod
+def migration_finished(vm):
+return vm.command('query-migrate')['status'] in ('completed', 'failed')
+
+def _get_free_port(self):
+port = network.find_free_port()
+if port is None:
+self.cancel('Failed to find a free port')
+return port
+
+
+def test_migration_with_tcp_localhost(self):
+source_vm = self.get_vm()
+dest_uri = 'tcp:localhost:%u' % self._get_free_port()
+dest_vm = self.get_vm('-incoming', dest_uri)
+dest_vm.launch()
+source_vm.launch()
+source_vm.qmp('migrate', uri=dest_uri)
+wait.wait_for(
+self.migration_finished,
+timeout=self.timeout,
+step=0.1,
+args=(source_vm,)
+)
+self.assertEqual(dest_vm.command('query-migrate')['status'], 
'completed')
+self.assertEqual(source_vm.command('query-migrate')['status'], 
'completed')
+self.assertEqual(dest_vm.command('query-status')['status'], 'running')
+self.assertEqual(source_vm.command('query-status')['status'], 
'postmigrate')
-- 
2.20.1

[Qemu-block] [PULL 2/7] Introduce a Python module structure

This is a simple move of Python code that wraps common QEMU
functionality, and are used by a number of different tests
and scripts.

By treating that code as a real Python module, we can more easily:
 * reuse code
 * have a proper place for the module's own unittests
 * apply a more consistent style
 * generate documentation

Signed-off-by: Cleber Rosa 
Reviewed-by: Caio Carrara 
Reviewed-by: Stefan Hajnoczi 
Message-Id: <20190206162901.19082-2-cr...@redhat.com>
Signed-off-by: Cleber Rosa 
---
 configure  |  1 +
 scripts/qemu.py => python/qemu/__init__.py | 11 ++-
 {scripts/qmp => python/qemu}/qmp.py|  0
 {scripts => python/qemu}/qtest.py  |  5 +++--
 scripts/device-crash-test  |  2 ++
 scripts/qmp/__init__.py|  0
 scripts/qmp/qemu-ga-client |  5 -
 scripts/qmp/qmp-shell  |  4 +++-
 scripts/render_block_graph.py  |  2 ++
 tests/acceptance/avocado_qemu/__init__.py  |  5 ++---
 tests/acceptance/virtio_version.py |  2 +-
 tests/migration/guestperf/engine.py|  7 ---
 tests/qemu-iotests/235 |  2 +-
 tests/qemu-iotests/238 |  2 +-
 tests/qemu-iotests/iotests.py  |  4 ++--
 tests/vm/basevm.py |  2 +-
 16 files changed, 33 insertions(+), 21 deletions(-)
 rename scripts/qemu.py => python/qemu/__init__.py (98%)
 rename {scripts/qmp => python/qemu}/qmp.py (100%)
 rename {scripts => python/qemu}/qtest.py (98%)
 delete mode 100644 scripts/qmp/__init__.py

diff --git a/configure b/configure
index a61682c3c7..343dd004e9 100755
--- a/configure
+++ b/configure
@@ -7604,6 +7604,7 @@ LINKS="$LINKS pc-bios/qemu-icon.bmp"
 LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
 LINKS="$LINKS tests/acceptance tests/data"
 LINKS="$LINKS tests/qemu-iotests/check"
+LINKS="$LINKS python"
 for bios_file in \
 $source_path/pc-bios/*.bin \
 $source_path/pc-bios/*.lid \
diff --git a/scripts/qemu.py b/python/qemu/__init__.py
similarity index 98%
rename from scripts/qemu.py
rename to python/qemu/__init__.py
index 32b00af5cc..38de3e9177 100644
--- a/scripts/qemu.py
+++ b/python/qemu/__init__.py
@@ -16,12 +16,13 @@ import errno
 import logging
 import os
 import subprocess
-import qmp.qmp
 import re
 import shutil
 import socket
 import tempfile
 
+from . import qmp
+
 
 LOG = logging.getLogger(__name__)
 
@@ -66,7 +67,7 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
 failures reported by the QEMU binary itself.
 """
 
-class MonitorResponseError(qmp.qmp.QMPError):
+class MonitorResponseError(qmp.QMPError):
 """
 Represents erroneous QMP monitor reply
 """
@@ -267,8 +268,8 @@ class QEMUMachine(object):
 self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
-self._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
-server=True)
+self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor,
+server=True)
 
 def _post_launch(self):
 self._qmp.accept()
@@ -384,7 +385,7 @@ class QEMUMachine(object):
 """
 reply = self.qmp(cmd, conv_keys, **args)
 if reply is None:
-raise qmp.qmp.QMPError("Monitor is closed")
+raise qmp.QMPError("Monitor is closed")
 if "error" in reply:
 raise MonitorResponseError(reply)
 return reply["return"]
diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
diff --git a/scripts/qtest.py b/python/qemu/qtest.py
similarity index 98%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index afac3fe900..eb45824dd0 100644
--- a/scripts/qtest.py
+++ b/python/qemu/qtest.py
@@ -13,7 +13,8 @@
 
 import socket
 import os
-import qemu
+
+from . import QEMUMachine
 
 
 class QEMUQtestProtocol(object):
@@ -79,7 +80,7 @@ class QEMUQtestProtocol(object):
 self._sock.settimeout(timeout)
 
 
-class QEMUQtestMachine(qemu.QEMUMachine):
+class QEMUQtestMachine(QEMUMachine):
 '''A QEMU VM'''
 
 def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 2a13fa4f84..a6748910ad 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -25,6 +25,7 @@ check for crashes and unexpected errors.
 """
 from __future__ import print_function
 
+import os
 import sys
 import glob
 import logging
@@ -34,6 +35,7 @@ import random
 import argparse
 from itertools import chain
 
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
diff --git a/scripts/qmp/__init__.py b/scripts/qmp/__init__.py

[Qemu-block] [PULL 0/7] Python queue, 2019-02-22

The following changes since commit 8eb29f1bf5a974dc4c11d2d1f5e7c7f7a62be116:

  Merge remote-tracking branch 'remotes/awilliam/tags/vfio-updates-20190221.0' 
into staging (2019-02-22 15:48:04 +)

are available in the Git repository at:

  git://github.com/clebergnu/qemu.git tags/python-next-pull-request

for you to fetch changes up to 8f1c89ec7443e4fa2cf106d8fa1c1c97b6ddeffb:

  Acceptance tests: expect boot to extract 2GiB+ initrd with linux-v4.16 
(2019-02-22 14:07:01 -0500)


Python queue, 2019-02-22

Python:
* introduce "python" directory with module namespace
* log QEMU launch command line on qemu.QEMUMachine

Acceptance Tests:
* initrd 4GiB+ test
* migration test
* multi vm support in test class
* bump Avocado version and drop ":avocado: enable"



Caio Carrara (2):
  tests.acceptance: adds multi vm capability for acceptance tests
  tests.acceptance: adds simple migration test

Cleber Rosa (3):
  Acceptance tests: drop usage of ":avocado: enable"
  Introduce a Python module structure
  scripts/qemu.py: log QEMU launch command line

Li Zhijian (2):
  Acceptance tests: use linux-3.6 and set vm memory to 4GiB
  Acceptance tests: expect boot to extract 2GiB+ initrd with linux-v4.16

 configure  |  1 +
 docs/devel/testing.rst | 42 -
 scripts/qemu.py => python/qemu/__init__.py | 12 +++--
 {scripts/qmp => python/qemu}/qmp.py|  0
 {scripts => python/qemu}/qtest.py  |  5 +-
 scripts/device-crash-test  |  2 +
 scripts/qmp/__init__.py|  0
 scripts/qmp/qemu-ga-client |  5 +-
 scripts/qmp/qmp-shell  |  4 +-
 scripts/render_block_graph.py  |  2 +
 tests/acceptance/avocado_qemu/__init__.py  | 30 +---
 tests/acceptance/boot_linux_console.py |  1 -
 tests/acceptance/linux_initrd.py   | 52 +
 tests/acceptance/migration.py  | 53 ++
 tests/acceptance/version.py|  1 -
 tests/acceptance/virtio_version.py |  3 +-
 tests/acceptance/vnc.py|  1 -
 tests/migration/guestperf/engine.py|  7 +--
 tests/qemu-iotests/235 |  2 +-
 tests/qemu-iotests/238 |  2 +-
 tests/qemu-iotests/iotests.py  |  4 +-
 tests/requirements.txt |  2 +-
 tests/vm/basevm.py |  2 +-
 23 files changed, 193 insertions(+), 40 deletions(-)
 rename scripts/qemu.py => python/qemu/__init__.py (98%)
 rename {scripts/qmp => python/qemu}/qmp.py (100%)
 rename {scripts => python/qemu}/qtest.py (98%)
 delete mode 100644 scripts/qmp/__init__.py
 create mode 100644 tests/acceptance/migration.py

-- 
2.20.1

[Qemu-block] [PULL 1/7] Acceptance tests: drop usage of ":avocado: enable"

The Avocado test runner attemps to find its INSTRUMENTED (that is,
Python based tests) in a manner that is as safe as possible to the
user.  Different from plain Python unittest, it won't load or
execute test code on an operation such as:

 $ avocado list tests/acceptance/

Before version 68.0, the logic implemented to identify INSTRUMENTED
tests would require either the ":avocado: enable" or ":avocado:
recursive" statement as a flag for tests that would not inherit
directly from "avocado.Test".  This is not necessary anymore,
and because of that the boiler plate statements can now be removed.

Reference: 
https://avocado-framework.readthedocs.io/en/68.0/release_notes/68_0.html#users-test-writers
Signed-off-by: Cleber Rosa 
Reviewed-by: Caio Carrara 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Wainer dos Santos Moschetta 
Message-Id: <20190218173723.26120-1-cr...@redhat.com>
Signed-off-by: Cleber Rosa 
---
 docs/devel/testing.rst | 1 -
 tests/acceptance/boot_linux_console.py | 1 -
 tests/acceptance/linux_initrd.py   | 1 -
 tests/acceptance/version.py| 1 -
 tests/acceptance/virtio_version.py | 1 -
 tests/acceptance/vnc.py| 1 -
 tests/requirements.txt | 2 +-
 7 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 135743a2bf..3ce171829d 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -600,7 +600,6 @@ the ``avocado_qemu.Test`` class.  Here's a simple usage 
example:
 
   class Version(Test):
   """
-  :avocado: enable
   :avocado: tags=quick
   """
   def test_qmp_human_info_version(self):
diff --git a/tests/acceptance/boot_linux_console.py 
b/tests/acceptance/boot_linux_console.py
index 98324f7591..beeb1e59e8 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -18,7 +18,6 @@ class BootLinuxConsole(Test):
 Boots a x86_64 Linux kernel and checks that the console is operational
 and the kernel command line is properly passed from QEMU to the kernel
 
-:avocado: enable
 :avocado: tags=x86_64
 """
 
diff --git a/tests/acceptance/linux_initrd.py b/tests/acceptance/linux_initrd.py
index 737355c2ef..5a15fc4347 100644
--- a/tests/acceptance/linux_initrd.py
+++ b/tests/acceptance/linux_initrd.py
@@ -18,7 +18,6 @@ class LinuxInitrd(Test):
 """
 Checks QEMU evaluates correctly the initrd file passed as -initrd option.
 
-:avocado: enable
 :avocado: tags=x86_64
 """
 
diff --git a/tests/acceptance/version.py b/tests/acceptance/version.py
index 13b0a7440d..67c2192c93 100644
--- a/tests/acceptance/version.py
+++ b/tests/acceptance/version.py
@@ -14,7 +14,6 @@ from avocado_qemu import Test
 
 class Version(Test):
 """
-:avocado: enable
 :avocado: tags=quick
 """
 def test_qmp_human_info_version(self):
diff --git a/tests/acceptance/virtio_version.py 
b/tests/acceptance/virtio_version.py
index ce990250d8..464d75aa4e 100644
--- a/tests/acceptance/virtio_version.py
+++ b/tests/acceptance/virtio_version.py
@@ -61,7 +61,6 @@ class VirtioVersionCheck(Test):
 same device tree created by `disable-modern` and
 `disable-legacy`.
 
-:avocado: enable
 :avocado: tags=x86_64
 """
 
diff --git a/tests/acceptance/vnc.py b/tests/acceptance/vnc.py
index b1ef9d71b1..064ceabcc1 100644
--- a/tests/acceptance/vnc.py
+++ b/tests/acceptance/vnc.py
@@ -13,7 +13,6 @@ from avocado_qemu import Test
 
 class Vnc(Test):
 """
-:avocado: enable
 :avocado: tags=vnc,quick
 """
 def test_no_vnc(self):
diff --git a/tests/requirements.txt b/tests/requirements.txt
index 64c6e27a94..002ded6a22 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1,4 +1,4 @@
 # Add Python module requirements, one per line, to be installed
 # in the tests/venv Python virtual environment. For more info,
 # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
-avocado-framework==65.0
+avocado-framework==68.0
-- 
2.20.1

Re: [Qemu-block] [PATCH 2/2] qcow2: mark image as corrupt if failing during create

On 19.02.19 13:50, Daniel P. Berrangé wrote:
> During creation we write a minimal qcow2 header and then update it with
> extra features. If the updating fails for some reason we might still be
> left with a valid qcow2 image that will be mistakenly used for I/O. We
> cannot delete the image, since we don't know if we created the
> underlying storage or not. Thus we mark the header as corrupt to
> prevents its later usage.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  block/qcow2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ecc577175f..338513e652 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
> Error **errp)
>  
>  ret = 0;
>  out:
> +if (ret < 0) {
> +qcow2_mark_corrupt(blk_bs(blk));

First, blk_bs(blk) may be the qcow2 BDS, or it may the protocol BDS here
(it is the latter before the first blk_new_open() call).  Calling
qcow2_mark_corrupt() unconditionally may mean an invalid access to
bs->opaque (which is not going to be of type BDRVQcow2State if the BDS
is not a qcow2 BDS).

Second, blk may be NULL (at various points, e.g. after blk_new_open()
failed).  Then this would yield a segfault in in blk_bs().

Third, blk_bs(blk) may be NULL (if blk_insert_bs() failed).  Then this
would yield a segfault in qcow2_mark_corrupt().

On a minor note, it is rather probably that blk_new_open() fails.  In
that case, there is currently no way to mark the image corrupt.  Would
it be useful and possible to have a function to mark a qcow2 image
corrupt without relying on qcow2 features, i.e. by writing directly to
the protocol layer (which is always @bs)?  This would be unsafe to use
as long as the protocol layer is opened by the qcow2 driver in some
other node, but we could invoke this function safely after @blk has been
freed.

Or maybe Eric's suggestion really is for the best, i.e. mark the image
corrupt from the start and then clean that after we're all done.  You
don't need a new flag for that, we already have BDRV_O_CHECK.

Max

> +}
>  blk_unref(blk);
>  bdrv_unref(bs);
>  return ret;
> 

signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/2] qcow2: fail if encryption opts are provided to non-encrypted image

On 19.02.19 13:50, Daniel P. Berrangé wrote:
> If the qcow2 image does not have any encryption method specified in its
> header, the user should not be providing any encryption options when
> opening it. We already detect this if the user had set "encrypt.format"
> but this field is optional so must consider any "encrypt.*" option to be
> an error.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  block/qcow2.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 65a54c9ac6..ecc577175f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1045,6 +1045,12 @@ static int 
> qcow2_update_options_prepare(BlockDriverState *bs,
>  ret = -EINVAL;
>  goto fail;
>  }
> +if (encryptopts && qdict_size(encryptopts)) {
> +error_setg(errp, "No encryption in image header, but encryption "
> +   "options provided");
> +ret = -EINVAL;
> +goto fail;
> +}

Doesn't this make the block right before this one a bit superfluous?

Max

>  break;
>  
>  case QCOW_CRYPT_AES:
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [ovirt-users] qemu-img info showed iscsi/FC lun size 0

2019-02-22 Thread Nir Soffer

On Fri, Feb 22, 2019 at 7:14 PM Nir Soffer  wrote:

> On Fri, Feb 22, 2019 at 5:00 PM Jingjie Jiang 
> wrote:
>
>> What about qcow2 format?
>>
> qcow2 reports the real size regardless of the underlying storage, since
qcow2 manages
the allocations. However the size is reported in qemu-img check in the
image-end-offset.

$ dd if=/dev/zero bs=1M count=10 | tr "\0" "\1" > test.raw

$ truncate -s 200m test.raw

$ truncate -s 1g backing

$ sudo losetup -f backing --show
/dev/loop2

$ sudo qemu-img convert -f raw -O qcow2 test.raw /dev/loop2

$ sudo qemu-img info --output json /dev/loop2
{
"virtual-size": 209715200,
"filename": "/dev/loop2",
"cluster-size": 65536,
"format": "qcow2",
"actual-size": 0,
"format-specific": {
"type": "qcow2",
"data": {
"compat": "1.1",
"lazy-refcounts": false,
"refcount-bits": 16,
"corrupt": false
}
},
"dirty-flag": false
}

$ sudo qemu-img check --output json /dev/loop2
{
"image-end-offset": 10813440,
"total-clusters": 3200,
"check-errors": 0,
"allocated-clusters": 160,
"filename": "/dev/loop2",
"format": "qcow2"
}

We use this for reducing volumes to optimal size after merging snapshots,
but
we don't report this value to engine.

Is there a choice  to create vm disk with format qcow2 instead of raw?
>>
> Not for LUNs, only for images.
>
> The available formats in 4.3 are documented here:
>
> https://ovirt.org/develop/release-management/features/storage/incremental-backup.html#disk-format
>
> incremental means you checked the checkbox "Enable incremental backup"
> when creating a disk.
> But note that the fact that we will create qcow2 image is implementation
> detail and the behavior
> may change in the future. For example, qemu is expected to provide a way
> to do incremental
> backup with raw volumes, and in this case we may create a raw volume
> instead of qcow2 volume.
> (actually raw data volume and qcow2 metadata volume).
>
> If you want to control the disk format, the only way is via the REST API
> or SDK, where you can
> specify the format instead of allocation policy. However even if you
> specify the format in the SDK
> the system may chose to change the format when copying the disk to another
> storage type. For
> example if you copy qcow2/sparse image from block storage to file storage
> the system will create
> a raw/sparse image.
>
> If you desire to control the format both from the UI and REST API/SDK and
> ensure that the system
> will never change the selected format please file a bug explaining the use
> case.
>
> On 2/21/19 5:46 PM, Nir Soffer wrote:
>>
>>
>>
>> On Thu, Feb 21, 2019, 21:48 >
>>> Hi,
>>> Based on oVirt 4.3.0, I have data domain from FC lun, then I create new
>>> vm on the disk from FC data domain.
>>> After VM was created. According to qemu-img info, the disk size is 0.
>>> # qemu-img info
>>> /rhev/data-center/mnt/blockSD/eaa6f641-6b36-4c1d-bf99-6ba77df3156f/images/8d3b455b-1da4-49f3-ba57-8cda64aa9dc9/949fa315-3934-4038-85f2-08aec52c1e2b
>>>
>>> image:
>>> /rhev/data-center/mnt/blockSD/eaa6f641-6b36-4c1d-bf99-6ba77df3156f/images/8d3b455b-1da4-49f3-ba57-8cda64aa9dc9/949fa315-3934-4038-85f2-08aec52c1e2b
>>> file format: raw
>>> virtual size: 10G (10737418240 bytes)
>>> disk size: 0
>>>
>>> I tried on iscsi and same result.
>>>
>>> Is the behaviour expected?
>>>
>>
>> It is expected in a way. Disk size is the amount of storage actually
>> used, and block devices has no way to tell that.
>>
>> oVirt report the size of the block device in this case, which is more
>> accurate than zero.
>>
>> However the real size allocated on the undrelying storage is somewhere
>> between zero an device size, and depends on the imlementation of the
>> storage. Nither qemu-img nor oVirt can tell the real size.
>>
>> Nir
>>
>>
>>> Thanks,
>>> Jingjie
>>>
>>> ___
>>> Users mailing list -- us...@ovirt.org
>>> To unsubscribe send an email to users-le...@ovirt.org
>>> Privacy Statement: https://www.ovirt.org/site/privacy-policy/
>>> oVirt Code of Conduct:
>>> https://www.ovirt.org/community/about/community-guidelines/
>>> List Archives:
>>> https://lists.ovirt.org/archives/list/us...@ovirt.org/message/TSXP7RENWIFMHIJWIAF6AGQPI3NOVNIZ/
>>>
>>

Re: [Qemu-block] [ovirt-users] qemu-img info showed iscsi/FC lun size 0

2019-02-22 Thread Nir Soffer

On Fri, Feb 22, 2019 at 5:00 PM Jingjie Jiang 
wrote:

> What about qcow2 format?
>
> Is there a choice  to create vm disk with format qcow2 instead of raw?
>
Not for LUNs, only for images.

The available formats in 4.3 are documented here:
https://ovirt.org/develop/release-management/features/storage/incremental-backup.html#disk-format

incremental means you checked the checkbox "Enable incremental backup" when
creating a disk.
But note that the fact that we will create qcow2 image is implementation
detail and the behavior
may change in the future. For example, qemu is expected to provide a way to
do incremental
backup with raw volumes, and in this case we may create a raw volume
instead of qcow2 volume.
(actually raw data volume and qcow2 metadata volume).

If you want to control the disk format, the only way is via the REST API or
SDK, where you can
specify the format instead of allocation policy. However even if you
specify the format in the SDK
the system may chose to change the format when copying the disk to another
storage type. For
example if you copy qcow2/sparse image from block storage to file storage
the system will create
a raw/sparse image.

If you desire to control the format both from the UI and REST API/SDK and
ensure that the system
will never change the selected format please file a bug explaining the use
case.

On 2/21/19 5:46 PM, Nir Soffer wrote:
>
>
>
> On Thu, Feb 21, 2019, 21:48 
>> Hi,
>> Based on oVirt 4.3.0, I have data domain from FC lun, then I create new
>> vm on the disk from FC data domain.
>> After VM was created. According to qemu-img info, the disk size is 0.
>> # qemu-img info
>> /rhev/data-center/mnt/blockSD/eaa6f641-6b36-4c1d-bf99-6ba77df3156f/images/8d3b455b-1da4-49f3-ba57-8cda64aa9dc9/949fa315-3934-4038-85f2-08aec52c1e2b
>>
>> image:
>> /rhev/data-center/mnt/blockSD/eaa6f641-6b36-4c1d-bf99-6ba77df3156f/images/8d3b455b-1da4-49f3-ba57-8cda64aa9dc9/949fa315-3934-4038-85f2-08aec52c1e2b
>> file format: raw
>> virtual size: 10G (10737418240 bytes)
>> disk size: 0
>>
>> I tried on iscsi and same result.
>>
>> Is the behaviour expected?
>>
>
> It is expected in a way. Disk size is the amount of storage actually used,
> and block devices has no way to tell that.
>
> oVirt report the size of the block device in this case, which is more
> accurate than zero.
>
> However the real size allocated on the undrelying storage is somewhere
> between zero an device size, and depends on the imlementation of the
> storage. Nither qemu-img nor oVirt can tell the real size.
>
> Nir
>
>
>> Thanks,
>> Jingjie
>>
>> ___
>> Users mailing list -- us...@ovirt.org
>> To unsubscribe send an email to users-le...@ovirt.org
>> Privacy Statement: https://www.ovirt.org/site/privacy-policy/
>> oVirt Code of Conduct:
>> https://www.ovirt.org/community/about/community-guidelines/
>> List Archives:
>> https://lists.ovirt.org/archives/list/us...@ovirt.org/message/TSXP7RENWIFMHIJWIAF6AGQPI3NOVNIZ/
>>
>

Re: [Qemu-block] [PATCH v2 1/2] iotests: ensure we print nbd server log on error

2019-02-22 Thread Daniel P . Berrangé

On Fri, Feb 22, 2019 at 09:54:25AM -0600, Eric Blake wrote:
> On 2/22/19 9:16 AM, Daniel P. Berrangé wrote:
> > On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
> >> On 20.02.19 15:58, Daniel P. Berrangé wrote:
> >>> If we abort the iotest early the server.log file might contain useful
> >>> information for diagnosing the problem. Ensure its contents are
> >>> displayed in this case.
> >>>
> >>> Reviewed-by: Eric Blake 
> >>> Signed-off-by: Daniel P. Berrangé 
> 
> >>> @@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' 
> >>> "$TEST_IMG" | _filter_qemu_io
> >>>  echo
> >>>  echo "== final server log =="
> >>>  cat "$TEST_DIR/server.log"
> >>> +rm -f $TEST_DIR/server.log
> >>
> >> I'm not sure how well the iotests currently cope with spaced dir names
> >> anyway, but it looks weird to not use quotes here right after a line
> >> that does.
> > 
> > Yes, that is a mistake since we tried quoting throughout the file
> 
> Can fix that while staging through my NBD queue. I'll probably send a PR
> on Monday.

Great, thanks.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2

Am 22.02.2019 um 17:13 hat Max Reitz geschrieben:
> On 22.02.19 16:57, Kevin Wolf wrote:
> > Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
> >> On 19.02.19 10:17, Kevin Wolf wrote:
> >>> Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
>  On 31.01.19 18:55, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf 
> > ---
> >  qapi/block-core.json | 1 +
> >  block/qcow2.c| 6 +-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> 
>  [...]
> 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 4959bf16a4..e3427f9fcd 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1459,7 +1459,9 @@ static int coroutine_fn 
> > qcow2_do_open(BlockDriverState *bs, QDict *options,
> >  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> > &child_file, false, &local_err);
> > -if (!s->data_file) {
> > +if (s->data_file) {
> > +s->image_data_file = g_strdup(s->data_file->bs->filename);
> > +} else {
> >  if (s->image_data_file) {
> >  error_free(local_err);
> >  local_err = NULL;
> 
>  Ah, this is what I looked for in the last patch. :-)
> 
>  (i.e. it should be in the last patch, not here)
> >>>
> >>> [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
> >>>
> >>> This is the last patch. :-P
> >>
> >> Sorry, I meant the previous one.
> >>
>  I think as it is it is just wrong, though.  If I pass enough options at
>  runtime, this will overwrite the image header:
> 
>  $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
>  $ ./qemu-img create -f raw bar.raw 64M
>  $ ./qemu-img info foo.qcow2
>  [...]
>  data file: foo.raw
>  [...]
>  $ ./qemu-io --image-opts \
>  file.filename=foo.qcow2,data-file.driver=file,\
>  data-file.filename=bar.raw,lazy-refcounts=on \
>  -c 'write 0 64k'
>  # (The lazy-refcounts is so the image header is updated)
>  $ ./qemu-img info foo.qcow2
>  [...]
>  data file: bar.raw
>  [...]
> 
>  The right thing would probably to check whether the header extension
>  exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
>  is NULL), s->image_data_file gets set; because there are no valid images
>  with the external data file flag set where there is no such header
>  extension.  So we must be in the process of creating the image right now.
> 
>  But even then, I don't quite like setting it here and not creating the
>  header extension as part of qcow2_co_create().  I can see why you've
>  done it this way, but creating a "bad" image on purpose (one with the
>  external data file bit set, but no such header extension present yet) in
>  order to detect and rectify this case when it is first opened (and the
>  opening code assuming that any such broken image must be one that is
>  opened the first time) is a bit weird.
> >>>
> >>> It's not really a bad image, just one that's a bit cumbersome to use
> >>> because you need to specify the 'data-file' option manually.
> >>
> >> Of course it's bad because it doesn't adhere to the specification (which
> >> you could amend, of course, since you add it with this series).  The
> >> spec says "If this bit is set, an external data file name header
> >> extension must be present as well."  Which it isn't until the image is
> >> opened with the data-file option.
> > 
> > Hm, I wonder whether that's a good requirement to make or whether we
> > should indeed change the spec. It wouldn't be so bad to have images that
> > require the data-file runtime option.
> > 
> > I guess we could lift the restriction later if we want to make use of
> > it. But the QEMU code is already written in a way that this works, so
> > maybe just allow it.
> 
> OK for me.
> 
>  I suppose doing it right (if you agree with the paragraph before the
>  last one) and adding a comment would make it less weird
>  ("s->image_data_file must be non-NULL for any valid image, so this image
>  must be one we are creating right now" or something like that).
> 
>  But still, the issue you point out in your cover letter remains; which
>  is that the node's filename and the filename given by the user may be
>  two different things.
> >>>
> >>> I think what I was planning to do was leaving the path from the image
> >>> header in s->image_data_file even when a runtime option overrides it.
> >>> After all, ImageInfo is about the image, not about the runtime state.
> >>
> >> I'm not talking about ImageInfo here, though, I'm talking about the
> >> image creation process.  The hunk I've quoted should be in the previous
> >> patch, not in this one.
> >>
> >> Which doe

Re: [Qemu-block] [RFC PATCH 10/11] qcow2: Store data file name in the image

On 22.02.19 17:06, Kevin Wolf wrote:
> Am 22.02.2019 um 16:43 hat Max Reitz geschrieben:
>> On 22.02.19 16:35, Kevin Wolf wrote:
>>> Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
 On 19.02.19 10:04, Kevin Wolf wrote:
> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> Rather than requiring that the external data file node is passed
>>> explicitly when creating the qcow2 node, store the filename in the
>>> designated header extension during .bdrv_create and read it from there
>>> as a default during .bdrv_open.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  block/qcow2.h  |  1 +
>>>  block/qcow2.c  | 69 +-
>>>  tests/qemu-iotests/082.out | 27 +++
>>>  3 files changed, 96 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 6cf862e8b9..4959bf16a4 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState 
>>> *bs, uint64_t start_offset,
>>>  #endif
>>>  break;
>>>  
>>> +case QCOW2_EXT_MAGIC_DATA_FILE:
>>> +{
>>> +s->image_data_file = g_malloc0(ext.len + 1);
>>> +ret = bdrv_pread(bs->file, offset, s->image_data_file, 
>>> ext.len);
>>> +if (ret < 0) {
>>> +error_setg_errno(errp, -ret, "ERROR: Could not data 
>>> file name");
>>
>> I think you accidentally a word.
>>
>>> +return ret;
>>> +}
>>> +#ifdef DEBUG_EXT
>>> +printf("Qcow2: Got external data file %s\n", 
>>> s->image_data_file);
>>> +#endif
>>> +break;
>>> +}
>>> +
>>>  default:
>>>  /* unknown magic - save it in case we need to rewrite the 
>>> header */
>>>  /* If you add a new feature, make sure to also update the 
>>> fast
>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn 
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>  /* Open external data file */
>>>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>>>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>>> -   &child_file, false, errp);
>>> +   &child_file, false, &local_err);
>>> +if (!s->data_file) {
>>> +if (s->image_data_file) {
>>> +error_free(local_err);
>>> +local_err = NULL;
>>
>> This looked a bit weird to me at first because I was wondering why you
>> wouldn't just pass allow_none=true and then handle errors (by passing
>> them on).  But right, we want to retry with a filename set, maybe that
>> makes more sense of the options.
>
> I think we want the normal error message for the !s->image_data_file
> case. With allow_none=true, we would have to come up with a new one here
> (in the else branch below).
>
>> Hm.  But then again, do we really?  It matches what we do with backing
>> files, but that does give at least me headaches from time to time.  How
>> bad would it be to allow either passing all valid options through
>> @options (which would make qcow2 ignore the string in the header), or
>> use the filename given in the header alone?
>
> You mean offering only one of the two ways to configure the node?

 Either just the filename from the image header, or ignore that and take
 all options from the user (who'd have to give a syntactically complete
 QAPI BlockdevRef object).

> The 'data-file' runtime option is a must so that libvirt can build the
> graph node by node (and possibly use file descriptor passing one day).
> But having to specify the option every time is very unfriendly for human
> users, so I think allowing to store the file name in the header is a
> must, too.

 Sure.  But I don't know whether we have to support taking the filename
 from the image header, and the user overriding some of the node's
 options (e.g. caching).
>>>
>>> So essentially this would mean passing NULL instead of options to
>>> bdrv_open_child() when we retry with the filename from the header.
>>>
>>> But it's inconsistent with all other places, which comes with two
>>
>> "all other places"?  Really it's just backing files, as everywhere else
>> there is no filename that doesn't come from the command line.
>>
>> Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
>> that case a bit different.  Although maybe it really isn't. *shrug*
> 
> Why would it be different? At least as a user, I consider them the same.
> It's true that bs->file an

Re: [Qemu-block] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2

On 22.02.19 16:57, Kevin Wolf wrote:
> Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
>> On 19.02.19 10:17, Kevin Wolf wrote:
>>> Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
 On 31.01.19 18:55, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 1 +
>  block/qcow2.c| 6 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)

 [...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4959bf16a4..e3427f9fcd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1459,7 +1459,9 @@ static int coroutine_fn 
> qcow2_do_open(BlockDriverState *bs, QDict *options,
>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> &child_file, false, &local_err);
> -if (!s->data_file) {
> +if (s->data_file) {
> +s->image_data_file = g_strdup(s->data_file->bs->filename);
> +} else {
>  if (s->image_data_file) {
>  error_free(local_err);
>  local_err = NULL;

 Ah, this is what I looked for in the last patch. :-)

 (i.e. it should be in the last patch, not here)
>>>
>>> [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
>>>
>>> This is the last patch. :-P
>>
>> Sorry, I meant the previous one.
>>
 I think as it is it is just wrong, though.  If I pass enough options at
 runtime, this will overwrite the image header:

 $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
 $ ./qemu-img create -f raw bar.raw 64M
 $ ./qemu-img info foo.qcow2
 [...]
 data file: foo.raw
 [...]
 $ ./qemu-io --image-opts \
 file.filename=foo.qcow2,data-file.driver=file,\
 data-file.filename=bar.raw,lazy-refcounts=on \
 -c 'write 0 64k'
 # (The lazy-refcounts is so the image header is updated)
 $ ./qemu-img info foo.qcow2
 [...]
 data file: bar.raw
 [...]

 The right thing would probably to check whether the header extension
 exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
 is NULL), s->image_data_file gets set; because there are no valid images
 with the external data file flag set where there is no such header
 extension.  So we must be in the process of creating the image right now.

 But even then, I don't quite like setting it here and not creating the
 header extension as part of qcow2_co_create().  I can see why you've
 done it this way, but creating a "bad" image on purpose (one with the
 external data file bit set, but no such header extension present yet) in
 order to detect and rectify this case when it is first opened (and the
 opening code assuming that any such broken image must be one that is
 opened the first time) is a bit weird.
>>>
>>> It's not really a bad image, just one that's a bit cumbersome to use
>>> because you need to specify the 'data-file' option manually.
>>
>> Of course it's bad because it doesn't adhere to the specification (which
>> you could amend, of course, since you add it with this series).  The
>> spec says "If this bit is set, an external data file name header
>> extension must be present as well."  Which it isn't until the image is
>> opened with the data-file option.
> 
> Hm, I wonder whether that's a good requirement to make or whether we
> should indeed change the spec. It wouldn't be so bad to have images that
> require the data-file runtime option.
> 
> I guess we could lift the restriction later if we want to make use of
> it. But the QEMU code is already written in a way that this works, so
> maybe just allow it.

OK for me.

 I suppose doing it right (if you agree with the paragraph before the
 last one) and adding a comment would make it less weird
 ("s->image_data_file must be non-NULL for any valid image, so this image
 must be one we are creating right now" or something like that).

 But still, the issue you point out in your cover letter remains; which
 is that the node's filename and the filename given by the user may be
 two different things.
>>>
>>> I think what I was planning to do was leaving the path from the image
>>> header in s->image_data_file even when a runtime option overrides it.
>>> After all, ImageInfo is about the image, not about the runtime state.
>>
>> I'm not talking about ImageInfo here, though, I'm talking about the
>> image creation process.  The hunk I've quoted should be in the previous
>> patch, not in this one.
>>
>> Which doesn't make wrong what you're saying, though, the ImageInfo
>> should print what's in the header.
>>
>>> Image creation would just manually set s->image_data_file before
>>> updating the header.
>>
>> It should, but currently it does that rather indirectly (by setting the

Re: [Qemu-block] [PATCH v2 1/2] iotests: ensure we print nbd server log on error

On 22.02.19 16:54, Eric Blake wrote:
> On 2/22/19 9:16 AM, Daniel P. Berrangé wrote:
>> On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
>>> On 20.02.19 15:58, Daniel P. Berrangé wrote:
 If we abort the iotest early the server.log file might contain useful
 information for diagnosing the problem. Ensure its contents are
 displayed in this case.

 Reviewed-by: Eric Blake 
 Signed-off-by: Daniel P. Berrangé 
> 
 @@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' 
 "$TEST_IMG" | _filter_qemu_io
  echo
  echo "== final server log =="
  cat "$TEST_DIR/server.log"
 +rm -f $TEST_DIR/server.log
>>>
>>> I'm not sure how well the iotests currently cope with spaced dir names
>>> anyway, but it looks weird to not use quotes here right after a line
>>> that does.
>>
>> Yes, that is a mistake since we tried quoting throughout the file
> 
> Can fix that while staging through my NBD queue. I'll probably send a PR
> on Monday.

That's OK for me.

Max




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2

Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
> On 19.02.19 10:17, Kevin Wolf wrote:
> > Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
> >> On 31.01.19 18:55, Kevin Wolf wrote:
> >>> Signed-off-by: Kevin Wolf 
> >>> ---
> >>>  qapi/block-core.json | 1 +
> >>>  block/qcow2.c| 6 +-
> >>>  2 files changed, 6 insertions(+), 1 deletion(-)
> >>
> >> [...]
> >>
> >>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>> index 4959bf16a4..e3427f9fcd 100644
> >>> --- a/block/qcow2.c
> >>> +++ b/block/qcow2.c
> >>> @@ -1459,7 +1459,9 @@ static int coroutine_fn 
> >>> qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >>>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> >>> &child_file, false, &local_err);
> >>> -if (!s->data_file) {
> >>> +if (s->data_file) {
> >>> +s->image_data_file = g_strdup(s->data_file->bs->filename);
> >>> +} else {
> >>>  if (s->image_data_file) {
> >>>  error_free(local_err);
> >>>  local_err = NULL;
> >>
> >> Ah, this is what I looked for in the last patch. :-)
> >>
> >> (i.e. it should be in the last patch, not here)
> > 
> > [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
> > 
> > This is the last patch. :-P
> 
> Sorry, I meant the previous one.
> 
> >> I think as it is it is just wrong, though.  If I pass enough options at
> >> runtime, this will overwrite the image header:
> >>
> >> $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
> >> $ ./qemu-img create -f raw bar.raw 64M
> >> $ ./qemu-img info foo.qcow2
> >> [...]
> >> data file: foo.raw
> >> [...]
> >> $ ./qemu-io --image-opts \
> >> file.filename=foo.qcow2,data-file.driver=file,\
> >> data-file.filename=bar.raw,lazy-refcounts=on \
> >> -c 'write 0 64k'
> >> # (The lazy-refcounts is so the image header is updated)
> >> $ ./qemu-img info foo.qcow2
> >> [...]
> >> data file: bar.raw
> >> [...]
> >>
> >> The right thing would probably to check whether the header extension
> >> exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
> >> is NULL), s->image_data_file gets set; because there are no valid images
> >> with the external data file flag set where there is no such header
> >> extension.  So we must be in the process of creating the image right now.
> >>
> >> But even then, I don't quite like setting it here and not creating the
> >> header extension as part of qcow2_co_create().  I can see why you've
> >> done it this way, but creating a "bad" image on purpose (one with the
> >> external data file bit set, but no such header extension present yet) in
> >> order to detect and rectify this case when it is first opened (and the
> >> opening code assuming that any such broken image must be one that is
> >> opened the first time) is a bit weird.
> > 
> > It's not really a bad image, just one that's a bit cumbersome to use
> > because you need to specify the 'data-file' option manually.
> 
> Of course it's bad because it doesn't adhere to the specification (which
> you could amend, of course, since you add it with this series).  The
> spec says "If this bit is set, an external data file name header
> extension must be present as well."  Which it isn't until the image is
> opened with the data-file option.

Hm, I wonder whether that's a good requirement to make or whether we
should indeed change the spec. It wouldn't be so bad to have images that
require the data-file runtime option.

I guess we could lift the restriction later if we want to make use of
it. But the QEMU code is already written in a way that this works, so
maybe just allow it.

> >> I suppose doing it right (if you agree with the paragraph before the
> >> last one) and adding a comment would make it less weird
> >> ("s->image_data_file must be non-NULL for any valid image, so this image
> >> must be one we are creating right now" or something like that).
> >>
> >> But still, the issue you point out in your cover letter remains; which
> >> is that the node's filename and the filename given by the user may be
> >> two different things.
> > 
> > I think what I was planning to do was leaving the path from the image
> > header in s->image_data_file even when a runtime option overrides it.
> > After all, ImageInfo is about the image, not about the runtime state.
> 
> I'm not talking about ImageInfo here, though, I'm talking about the
> image creation process.  The hunk I've quoted should be in the previous
> patch, not in this one.
> 
> Which doesn't make wrong what you're saying, though, the ImageInfo
> should print what's in the header.
> 
> > Image creation would just manually set s->image_data_file before
> > updating the header.
> 
> It should, but currently it does that rather indirectly (by setting the
> data-file option which then makes qcow2_do_open() copy it into
> s->image

Re: [Qemu-block] [RFC PATCH 10/11] qcow2: Store data file name in the image

Am 22.02.2019 um 16:43 hat Max Reitz geschrieben:
> On 22.02.19 16:35, Kevin Wolf wrote:
> > Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
> >> On 19.02.19 10:04, Kevin Wolf wrote:
> >>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
>  On 31.01.19 18:55, Kevin Wolf wrote:
> > Rather than requiring that the external data file node is passed
> > explicitly when creating the qcow2 node, store the filename in the
> > designated header extension during .bdrv_create and read it from there
> > as a default during .bdrv_open.
> >
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block/qcow2.h  |  1 +
> >  block/qcow2.c  | 69 +-
> >  tests/qemu-iotests/082.out | 27 +++
> >  3 files changed, 96 insertions(+), 1 deletion(-)
> 
>  [...]
> 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 6cf862e8b9..4959bf16a4 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState 
> > *bs, uint64_t start_offset,
> >  #endif
> >  break;
> >  
> > +case QCOW2_EXT_MAGIC_DATA_FILE:
> > +{
> > +s->image_data_file = g_malloc0(ext.len + 1);
> > +ret = bdrv_pread(bs->file, offset, s->image_data_file, 
> > ext.len);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "ERROR: Could not data 
> > file name");
> 
>  I think you accidentally a word.
> 
> > +return ret;
> > +}
> > +#ifdef DEBUG_EXT
> > +printf("Qcow2: Got external data file %s\n", 
> > s->image_data_file);
> > +#endif
> > +break;
> > +}
> > +
> >  default:
> >  /* unknown magic - save it in case we need to rewrite the 
> > header */
> >  /* If you add a new feature, make sure to also update the 
> > fast
> > @@ -1444,7 +1458,18 @@ static int coroutine_fn 
> > qcow2_do_open(BlockDriverState *bs, QDict *options,
> >  /* Open external data file */
> >  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> > -   &child_file, false, errp);
> > +   &child_file, false, &local_err);
> > +if (!s->data_file) {
> > +if (s->image_data_file) {
> > +error_free(local_err);
> > +local_err = NULL;
> 
>  This looked a bit weird to me at first because I was wondering why you
>  wouldn't just pass allow_none=true and then handle errors (by passing
>  them on).  But right, we want to retry with a filename set, maybe that
>  makes more sense of the options.
> >>>
> >>> I think we want the normal error message for the !s->image_data_file
> >>> case. With allow_none=true, we would have to come up with a new one here
> >>> (in the else branch below).
> >>>
>  Hm.  But then again, do we really?  It matches what we do with backing
>  files, but that does give at least me headaches from time to time.  How
>  bad would it be to allow either passing all valid options through
>  @options (which would make qcow2 ignore the string in the header), or
>  use the filename given in the header alone?
> >>>
> >>> You mean offering only one of the two ways to configure the node?
> >>
> >> Either just the filename from the image header, or ignore that and take
> >> all options from the user (who'd have to give a syntactically complete
> >> QAPI BlockdevRef object).
> >>
> >>> The 'data-file' runtime option is a must so that libvirt can build the
> >>> graph node by node (and possibly use file descriptor passing one day).
> >>> But having to specify the option every time is very unfriendly for human
> >>> users, so I think allowing to store the file name in the header is a
> >>> must, too.
> >>
> >> Sure.  But I don't know whether we have to support taking the filename
> >> from the image header, and the user overriding some of the node's
> >> options (e.g. caching).
> > 
> > So essentially this would mean passing NULL instead of options to
> > bdrv_open_child() when we retry with the filename from the header.
> > 
> > But it's inconsistent with all other places, which comes with two
> 
> "all other places"?  Really it's just backing files, as everywhere else
> there is no filename that doesn't come from the command line.
> 
> Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
> that case a bit different.  Although maybe it really isn't. *shrug*

Why would it be different? At least as a user, I consider them the same.
It's true that bs->file and bs->backing come with some additional magic,
b

Re: [Qemu-block] [PATCH v2 1/2] iotests: ensure we print nbd server log on error

2019-02-22 Thread Eric Blake

On 2/22/19 9:16 AM, Daniel P. Berrangé wrote:
> On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
>> On 20.02.19 15:58, Daniel P. Berrangé wrote:
>>> If we abort the iotest early the server.log file might contain useful
>>> information for diagnosing the problem. Ensure its contents are
>>> displayed in this case.
>>>
>>> Reviewed-by: Eric Blake 
>>> Signed-off-by: Daniel P. Berrangé 

>>> @@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' 
>>> "$TEST_IMG" | _filter_qemu_io
>>>  echo
>>>  echo "== final server log =="
>>>  cat "$TEST_DIR/server.log"
>>> +rm -f $TEST_DIR/server.log
>>
>> I'm not sure how well the iotests currently cope with spaced dir names
>> anyway, but it looks weird to not use quotes here right after a line
>> that does.
> 
> Yes, that is a mistake since we tried quoting throughout the file

Can fix that while staging through my NBD queue. I'll probably send a PR
on Monday.

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

Re: [Qemu-block] qemu-img: add seek option to dd

2019-02-22 Thread Eric Blake

On 2/22/19 8:59 AM, Max Reitz wrote:

>>
>> Is there anything I'm missing?  What is the status of that patch?
> 
> Eric sent another version last year:
> 
> http://lists.nongnu.org/archive/html/qemu-block/2018-08/msg00513.html
> 
> My main issue is that dd should be a frontend to convert, but it still
> isn't.  On top, I don't see why we need feature parity with dd when it's
> also possible to use qemu-nbd and just the normal dd.
> 
> (Or qemu-blkfuse for which I should sent patches one of these days...)
> 
> I think it's wrong to re-implement existing tools in qemu.  Instead, we
> should work on making it simpler for those existing tools to access qemu
> images.
> 
> Note that while I did rant a bit about the concept of Eric's patch, I
> didn't object it in principle.  I pointed out two technical issues, Eric
> talked about a v2, but that never appeared, I think.

Yeah, it's still on my TODO list, stalled behind incremental backups. It
will resurface when I finish my work on making qemu NBD not so awkward
when it comes to non-sector-aligned images, but as 4.0 soft freeze is
rapidly approaching, that may slip into 4.1.

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

Re: [Qemu-block] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure

Am 22.02.2019 um 15:12 hat Max Reitz geschrieben:
> On 19.02.19 09:51, Kevin Wolf wrote:
> > Am 19.02.2019 um 00:57 hat Max Reitz geschrieben:
> >> On 31.01.19 18:55, Kevin Wolf wrote:
> >>> This adds a .bdrv_open option to specify the external data file node.
> >>>
> >>> Signed-off-by: Kevin Wolf 
> >>> ---
> >>>  qapi/block-core.json |  3 ++-
> >>>  block/qcow2.h|  4 +++-
> >>>  block/qcow2.c| 25 +++--
> >>>  3 files changed, 28 insertions(+), 4 deletions(-)
> >>
> >> [...]
> >>
> >>> diff --git a/block/qcow2.h b/block/qcow2.h
> >>> index c161970882..e2114900b4 100644
> >>> --- a/block/qcow2.h
> >>> +++ b/block/qcow2.h
> >>
> >> [...]
> >>
> >>> @@ -205,7 +206,8 @@ enum {
> >>>  QCOW2_INCOMPAT_DATA_FILE= 1 << 
> >>> QCOW2_INCOMPAT_DATA_FILE_BITNR,
> >>>  
> >>>  QCOW2_INCOMPAT_MASK  = QCOW2_INCOMPAT_DIRTY
> >>> - | QCOW2_INCOMPAT_CORRUPT,
> >>> + | QCOW2_INCOMPAT_CORRUPT
> >>> + | QCOW2_INCOMPAT_DATA_FILE,
> >>
> >> This hunk seems to belong somewhere else.
> > 
> > Isn't this the first patch that actually allows opening images that have
> > QCOW2_INCOMPAT_DATA_FILE set in their header?
> 
> Oh, sorry.  I thought the mask was the mask of all incompatible
> features, but it's actually the mask of supported incompatible features.
>  Yep, then it's correct here.
> 
> >>>  };
> >>>  
> >>>  /* Compatible feature bits */
> >>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>> index ac9934b3ed..376232d3f0 100644
> >>> --- a/block/qcow2.c
> >>> +++ b/block/qcow2.c
> >>> @@ -1441,8 +1441,22 @@ static int coroutine_fn 
> >>> qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>>  goto fail;
> >>>  }
> >>>  
> >>> -/* TODO Open external data file */
> >>> -s->data_file = bs->file;
> >>> +/* Open external data file */
> >>> +if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >>> +s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> >>> +   &child_file, false, errp);
> >>> +if (!s->data_file) {
> >>> +ret = -EINVAL;
> >>> +goto fail;
> >>> +}
> >>> +} else if (qdict_get(options, QCOW2_OPT_DATA_FILE)) {
> >>
> >> I get the idea, but this isn't crumpled so this key may not exist (but
> >> data-file.driver and data-file.filename may).  Of course the fact that
> >> these options remain unused will be caught by the block layer, but that
> >> makes the error message below a bit less useful.
> > 
> > Hmm, good point... So you'd just leave out the check and always let the
> > block layer complain (with a less than helpful message)? Or are you
> > suggesting I should try and catch all cases somehow, even if that makes
> > things quite a bit more complicated?
> 
> I don't mind either way.  Nice error messages are nice as long as
> they're not too much trouble.  It's just that this current state feels a
> bit half-baked.

I think catching everything is just not worth the effort. So I
considered dropping the check. But then I thought that this half baked
check will make sure that we'll get full coverage once qcow2 is
converted to a QAPI-based open interface instead of forgetting to bring
the check back. So I'm leaning towards just leaving it as it is now. If
you like, I can add a comment that we know this is half baked.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [RFC PATCH 10/11] qcow2: Store data file name in the image

Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
> On 19.02.19 10:04, Kevin Wolf wrote:
> > Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
> >> On 31.01.19 18:55, Kevin Wolf wrote:
> >>> Rather than requiring that the external data file node is passed
> >>> explicitly when creating the qcow2 node, store the filename in the
> >>> designated header extension during .bdrv_create and read it from there
> >>> as a default during .bdrv_open.
> >>>
> >>> Signed-off-by: Kevin Wolf 
> >>> ---
> >>>  block/qcow2.h  |  1 +
> >>>  block/qcow2.c  | 69 +-
> >>>  tests/qemu-iotests/082.out | 27 +++
> >>>  3 files changed, 96 insertions(+), 1 deletion(-)
> >>
> >> [...]
> >>
> >>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>> index 6cf862e8b9..4959bf16a4 100644
> >>> --- a/block/qcow2.c
> >>> +++ b/block/qcow2.c
> >>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState 
> >>> *bs, uint64_t start_offset,
> >>>  #endif
> >>>  break;
> >>>  
> >>> +case QCOW2_EXT_MAGIC_DATA_FILE:
> >>> +{
> >>> +s->image_data_file = g_malloc0(ext.len + 1);
> >>> +ret = bdrv_pread(bs->file, offset, s->image_data_file, 
> >>> ext.len);
> >>> +if (ret < 0) {
> >>> +error_setg_errno(errp, -ret, "ERROR: Could not data file 
> >>> name");
> >>
> >> I think you accidentally a word.
> >>
> >>> +return ret;
> >>> +}
> >>> +#ifdef DEBUG_EXT
> >>> +printf("Qcow2: Got external data file %s\n", 
> >>> s->image_data_file);
> >>> +#endif
> >>> +break;
> >>> +}
> >>> +
> >>>  default:
> >>>  /* unknown magic - save it in case we need to rewrite the 
> >>> header */
> >>>  /* If you add a new feature, make sure to also update the 
> >>> fast
> >>> @@ -1444,7 +1458,18 @@ static int coroutine_fn 
> >>> qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>>  /* Open external data file */
> >>>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >>>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> >>> -   &child_file, false, errp);
> >>> +   &child_file, false, &local_err);
> >>> +if (!s->data_file) {
> >>> +if (s->image_data_file) {
> >>> +error_free(local_err);
> >>> +local_err = NULL;
> >>
> >> This looked a bit weird to me at first because I was wondering why you
> >> wouldn't just pass allow_none=true and then handle errors (by passing
> >> them on).  But right, we want to retry with a filename set, maybe that
> >> makes more sense of the options.
> > 
> > I think we want the normal error message for the !s->image_data_file
> > case. With allow_none=true, we would have to come up with a new one here
> > (in the else branch below).
> > 
> >> Hm.  But then again, do we really?  It matches what we do with backing
> >> files, but that does give at least me headaches from time to time.  How
> >> bad would it be to allow either passing all valid options through
> >> @options (which would make qcow2 ignore the string in the header), or
> >> use the filename given in the header alone?
> > 
> > You mean offering only one of the two ways to configure the node?
> 
> Either just the filename from the image header, or ignore that and take
> all options from the user (who'd have to give a syntactically complete
> QAPI BlockdevRef object).
> 
> > The 'data-file' runtime option is a must so that libvirt can build the
> > graph node by node (and possibly use file descriptor passing one day).
> > But having to specify the option every time is very unfriendly for human
> > users, so I think allowing to store the file name in the header is a
> > must, too.
> 
> Sure.  But I don't know whether we have to support taking the filename
> from the image header, and the user overriding some of the node's
> options (e.g. caching).

So essentially this would mean passing NULL instead of options to
bdrv_open_child() when we retry with the filename from the header.

But it's inconsistent with all other places, which comes with two
problems. It's confusing for users who are used to overriding just that
one option of a child. And do we actually spare you any headaches or do
we create new ones because we have now two different behaviours of
bdrv_open_child() callers that we must preserve in the future?

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [RFC PATCH 10/11] qcow2: Store data file name in the image

On 22.02.19 16:35, Kevin Wolf wrote:
> Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
>> On 19.02.19 10:04, Kevin Wolf wrote:
>>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
 On 31.01.19 18:55, Kevin Wolf wrote:
> Rather than requiring that the external data file node is passed
> explicitly when creating the qcow2 node, store the filename in the
> designated header extension during .bdrv_create and read it from there
> as a default during .bdrv_open.
>
> Signed-off-by: Kevin Wolf 
> ---
>  block/qcow2.h  |  1 +
>  block/qcow2.c  | 69 +-
>  tests/qemu-iotests/082.out | 27 +++
>  3 files changed, 96 insertions(+), 1 deletion(-)

 [...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6cf862e8b9..4959bf16a4 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState 
> *bs, uint64_t start_offset,
>  #endif
>  break;
>  
> +case QCOW2_EXT_MAGIC_DATA_FILE:
> +{
> +s->image_data_file = g_malloc0(ext.len + 1);
> +ret = bdrv_pread(bs->file, offset, s->image_data_file, 
> ext.len);
> +if (ret < 0) {
> +error_setg_errno(errp, -ret, "ERROR: Could not data file 
> name");

 I think you accidentally a word.

> +return ret;
> +}
> +#ifdef DEBUG_EXT
> +printf("Qcow2: Got external data file %s\n", 
> s->image_data_file);
> +#endif
> +break;
> +}
> +
>  default:
>  /* unknown magic - save it in case we need to rewrite the 
> header */
>  /* If you add a new feature, make sure to also update the 
> fast
> @@ -1444,7 +1458,18 @@ static int coroutine_fn 
> qcow2_do_open(BlockDriverState *bs, QDict *options,
>  /* Open external data file */
>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> -   &child_file, false, errp);
> +   &child_file, false, &local_err);
> +if (!s->data_file) {
> +if (s->image_data_file) {
> +error_free(local_err);
> +local_err = NULL;

 This looked a bit weird to me at first because I was wondering why you
 wouldn't just pass allow_none=true and then handle errors (by passing
 them on).  But right, we want to retry with a filename set, maybe that
 makes more sense of the options.
>>>
>>> I think we want the normal error message for the !s->image_data_file
>>> case. With allow_none=true, we would have to come up with a new one here
>>> (in the else branch below).
>>>
 Hm.  But then again, do we really?  It matches what we do with backing
 files, but that does give at least me headaches from time to time.  How
 bad would it be to allow either passing all valid options through
 @options (which would make qcow2 ignore the string in the header), or
 use the filename given in the header alone?
>>>
>>> You mean offering only one of the two ways to configure the node?
>>
>> Either just the filename from the image header, or ignore that and take
>> all options from the user (who'd have to give a syntactically complete
>> QAPI BlockdevRef object).
>>
>>> The 'data-file' runtime option is a must so that libvirt can build the
>>> graph node by node (and possibly use file descriptor passing one day).
>>> But having to specify the option every time is very unfriendly for human
>>> users, so I think allowing to store the file name in the header is a
>>> must, too.
>>
>> Sure.  But I don't know whether we have to support taking the filename
>> from the image header, and the user overriding some of the node's
>> options (e.g. caching).
> 
> So essentially this would mean passing NULL instead of options to
> bdrv_open_child() when we retry with the filename from the header.
> 
> But it's inconsistent with all other places, which comes with two

"all other places"?  Really it's just backing files, as everywhere else
there is no filename that doesn't come from the command line.

Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
that case a bit different.  Although maybe it really isn't. *shrug*

> problems. It's confusing for users who are used to overriding just that
> one option of a child. And do we actually spare you any headaches or do
> we create new ones because we have now two different behaviours of
> bdrv_open_child() callers that we must preserve in the future?

It means I can act out on my pain by being angry on how .backing
behaves.  That's better for my health than having to keep it

Re: [Qemu-block] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure

On 22.02.19 16:38, Kevin Wolf wrote:
> Am 22.02.2019 um 15:12 hat Max Reitz geschrieben:
>> On 19.02.19 09:51, Kevin Wolf wrote:
>>> Am 19.02.2019 um 00:57 hat Max Reitz geschrieben:
 On 31.01.19 18:55, Kevin Wolf wrote:
> This adds a .bdrv_open option to specify the external data file node.
>
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json |  3 ++-
>  block/qcow2.h|  4 +++-
>  block/qcow2.c| 25 +++--
>  3 files changed, 28 insertions(+), 4 deletions(-)

 [...]

> diff --git a/block/qcow2.h b/block/qcow2.h
> index c161970882..e2114900b4 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h

 [...]

> @@ -205,7 +206,8 @@ enum {
>  QCOW2_INCOMPAT_DATA_FILE= 1 << 
> QCOW2_INCOMPAT_DATA_FILE_BITNR,
>  
>  QCOW2_INCOMPAT_MASK  = QCOW2_INCOMPAT_DIRTY
> - | QCOW2_INCOMPAT_CORRUPT,
> + | QCOW2_INCOMPAT_CORRUPT
> + | QCOW2_INCOMPAT_DATA_FILE,

 This hunk seems to belong somewhere else.
>>>
>>> Isn't this the first patch that actually allows opening images that have
>>> QCOW2_INCOMPAT_DATA_FILE set in their header?
>>
>> Oh, sorry.  I thought the mask was the mask of all incompatible
>> features, but it's actually the mask of supported incompatible features.
>>  Yep, then it's correct here.
>>
>  };
>  
>  /* Compatible feature bits */
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ac9934b3ed..376232d3f0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1441,8 +1441,22 @@ static int coroutine_fn 
> qcow2_do_open(BlockDriverState *bs, QDict *options,
>  goto fail;
>  }
>  
> -/* TODO Open external data file */
> -s->data_file = bs->file;
> +/* Open external data file */
> +if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> +s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> +   &child_file, false, errp);
> +if (!s->data_file) {
> +ret = -EINVAL;
> +goto fail;
> +}
> +} else if (qdict_get(options, QCOW2_OPT_DATA_FILE)) {

 I get the idea, but this isn't crumpled so this key may not exist (but
 data-file.driver and data-file.filename may).  Of course the fact that
 these options remain unused will be caught by the block layer, but that
 makes the error message below a bit less useful.
>>>
>>> Hmm, good point... So you'd just leave out the check and always let the
>>> block layer complain (with a less than helpful message)? Or are you
>>> suggesting I should try and catch all cases somehow, even if that makes
>>> things quite a bit more complicated?
>>
>> I don't mind either way.  Nice error messages are nice as long as
>> they're not too much trouble.  It's just that this current state feels a
>> bit half-baked.
> 
> I think catching everything is just not worth the effort. So I
> considered dropping the check. But then I thought that this half baked
> check will make sure that we'll get full coverage once qcow2 is
> converted to a QAPI-based open interface instead of forgetting to bring
> the check back. So I'm leaning towards just leaving it as it is now. If
> you like, I can add a comment that we know this is half baked.

"Once"... :-)

Seems reasonable to me.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2] iotests: handle TypeError for Python3 in test 242




On 2/22/19 6:26 AM, Andrey Shinkevich wrote:
> The data type for bytes in Python3 differs from the one in Python2.
> Those cases should be managed separately.
> 
> v1:
> In the first version, the TypeError in Python3 was handled as the
> exception.
> Discussed in the e-mail thread with the Message ID:
> <1550519997-253534-1-git-send-email-andrey.shinkev...@virtuozzo.com>
> 
> Signed-off-by: Andrey Shinkevich 
> Reported-by: Kevin Wolf 
> ---
>  tests/qemu-iotests/242 | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
> index 16c65ed..446fbf8 100755
> --- a/tests/qemu-iotests/242
> +++ b/tests/qemu-iotests/242
> @@ -20,6 +20,7 @@
>  
>  import iotests
>  import json
> +import sys
>  from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
>  file_path, img_info_log, log, filter_qemu_io
>  
> @@ -65,9 +66,12 @@ def toggle_flag(offset):
>  with open(disk, "r+b") as f:
>  f.seek(offset, 0)
>  c = f.read(1)
> -toggled = chr(ord(c) ^ bitmap_flag_unknown)
> +toggled = ord(c) ^ bitmap_flag_unknown
>  f.seek(-1, 1)
> -f.write(toggled)
> +if sys.version_info.major >= 3:
> +f.write(bytes([toggled]))
> +else:
> +f.write(chr(toggled))
>  

I originally suggested:

sys.version_info.major == 2:
 ...

Because this is already present on other tests, and IIRC Max mentioned
using this as an easy to find flag the moment Python 2 support is to be
dropped.  But, looking for "sys.version_info.major" is just as
effective, so:

Reviewed-by: Cleber Rosa 

>  
>  qemu_img_create('-f', iotests.imgfmt, disk, '1M')
>

Re: [Qemu-block] [PATCH v2 1/2] iotests: ensure we print nbd server log on error

2019-02-22 Thread Daniel P . Berrangé

On Fri, Feb 22, 2019 at 04:06:32PM +0100, Max Reitz wrote:
> On 20.02.19 15:58, Daniel P. Berrangé wrote:
> > If we abort the iotest early the server.log file might contain useful
> > information for diagnosing the problem. Ensure its contents are
> > displayed in this case.
> > 
> > Reviewed-by: Eric Blake 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  tests/qemu-iotests/233 | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
> > index fc345a1a46..27932df075 100755
> > --- a/tests/qemu-iotests/233
> > +++ b/tests/qemu-iotests/233
> > @@ -30,6 +30,8 @@ _cleanup()
> >  {
> >  nbd_server_stop
> >  _cleanup_test_img
> > +# If we aborted early we want to see this log for diagnosis
> > +test -f "$TEST_DIR/server.log" && cat "$TEST_DIR/server.log"
> >  rm -f "$TEST_DIR/server.log"
> >  tls_x509_cleanup
> >  }
> > @@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' 
> > "$TEST_IMG" | _filter_qemu_io
> >  echo
> >  echo "== final server log =="
> >  cat "$TEST_DIR/server.log"
> > +rm -f $TEST_DIR/server.log
> 
> I'm not sure how well the iotests currently cope with spaced dir names
> anyway, but it looks weird to not use quotes here right after a line
> that does.

Yes, that is a mistake since we tried quoting throughout the file


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH v2 1/2] iotests: ensure we print nbd server log on error

On 20.02.19 15:58, Daniel P. Berrangé wrote:
> If we abort the iotest early the server.log file might contain useful
> information for diagnosing the problem. Ensure its contents are
> displayed in this case.
> 
> Reviewed-by: Eric Blake 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  tests/qemu-iotests/233 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
> index fc345a1a46..27932df075 100755
> --- a/tests/qemu-iotests/233
> +++ b/tests/qemu-iotests/233
> @@ -30,6 +30,8 @@ _cleanup()
>  {
>  nbd_server_stop
>  _cleanup_test_img
> +# If we aborted early we want to see this log for diagnosis
> +test -f "$TEST_DIR/server.log" && cat "$TEST_DIR/server.log"
>  rm -f "$TEST_DIR/server.log"
>  tls_x509_cleanup
>  }
> @@ -120,6 +122,7 @@ $QEMU_IO -f $IMGFMT -r -U -c 'r -P 0x22 1m 1m' 
> "$TEST_IMG" | _filter_qemu_io
>  echo
>  echo "== final server log =="
>  cat "$TEST_DIR/server.log"
> +rm -f $TEST_DIR/server.log

I'm not sure how well the iotests currently cope with spaced dir names
anyway, but it looks weird to not use quotes here right after a line
that does.

Max

>  
>  # success, all done
>  echo "*** done"
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] qemu-img: add seek option to dd

On 20.02.19 06:54, Richard W.M. Jones wrote:
> It seems like it's not easy to write to a place in a qcow2 file using
> qemu-io or qemu-img.  For example suppose I want to overwrite blocks
> 100 and 101 with my own data:
> 
> $ qemu-img dd -f raw -O qcow2 bs=4096 skip=100 count=2 \
>   if=/tmp/input.raw of=/tmp/disk.qcow2
> qemu-img: /tmp/input.raw: cannot skip to specified offset
> 
> Skip is skipping the input, not the output.  Turning to qemu-io, it
> doesn't look like you can control the data written (only write
> patterns).
> 
> This patch looks good, but AFAICT it never went upstream:
> 
>   https://patchwork.kernel.org/patch/9273161/
>   "qemu-img: add seek option to dd"
> 
> Is there anything I'm missing?  What is the status of that patch?

Eric sent another version last year:

http://lists.nongnu.org/archive/html/qemu-block/2018-08/msg00513.html

My main issue is that dd should be a frontend to convert, but it still
isn't.  On top, I don't see why we need feature parity with dd when it's
also possible to use qemu-nbd and just the normal dd.

(Or qemu-blkfuse for which I should sent patches one of these days...)

I think it's wrong to re-implement existing tools in qemu.  Instead, we
should work on making it simpler for those existing tools to access qemu
images.

Note that while I did rant a bit about the concept of Eric's patch, I
didn't object it in principle.  I pointed out two technical issues, Eric
talked about a v2, but that never appeared, I think.

Max

signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2] iotests: handle TypeError for Python3 in test 242

2019-02-22 Thread Eric Blake

On 2/22/19 5:26 AM, Andrey Shinkevich wrote:
> The data type for bytes in Python3 differs from the one in Python2.
> Those cases should be managed separately.
> 
> v1:
> In the first version, the TypeError in Python3 was handled as the
> exception.
> Discussed in the e-mail thread with the Message ID:
> <1550519997-253534-1-git-send-email-andrey.shinkev...@virtuozzo.com>

This paragraph belongs...

> 
> Signed-off-by: Andrey Shinkevich 
> Reported-by: Kevin Wolf 
> ---

...here, after the --- marker. It is useful to reviewers, but does not
need to land in the long-term git history (a year from now, we won't
care if it was v2 or v10 that landed, nor how it changed from the
versions that didn't land).  I can clean it up on staging.

Reviewed-by: Eric Blake 

Will stage through my NBD tree.

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

Re: [Qemu-block] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure

On 19.02.19 09:51, Kevin Wolf wrote:
> Am 19.02.2019 um 00:57 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> This adds a .bdrv_open option to specify the external data file node.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  qapi/block-core.json |  3 ++-
>>>  block/qcow2.h|  4 +++-
>>>  block/qcow2.c| 25 +++--
>>>  3 files changed, 28 insertions(+), 4 deletions(-)
>>
>> [...]
>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index c161970882..e2114900b4 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>
>> [...]
>>
>>> @@ -205,7 +206,8 @@ enum {
>>>  QCOW2_INCOMPAT_DATA_FILE= 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
>>>  
>>>  QCOW2_INCOMPAT_MASK  = QCOW2_INCOMPAT_DIRTY
>>> - | QCOW2_INCOMPAT_CORRUPT,
>>> + | QCOW2_INCOMPAT_CORRUPT
>>> + | QCOW2_INCOMPAT_DATA_FILE,
>>
>> This hunk seems to belong somewhere else.
> 
> Isn't this the first patch that actually allows opening images that have
> QCOW2_INCOMPAT_DATA_FILE set in their header?

Oh, sorry.  I thought the mask was the mask of all incompatible
features, but it's actually the mask of supported incompatible features.
 Yep, then it's correct here.

>>>  };
>>>  
>>>  /* Compatible feature bits */
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index ac9934b3ed..376232d3f0 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1441,8 +1441,22 @@ static int coroutine_fn 
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>  goto fail;
>>>  }
>>>  
>>> -/* TODO Open external data file */
>>> -s->data_file = bs->file;
>>> +/* Open external data file */
>>> +if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>>> +s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>>> +   &child_file, false, errp);
>>> +if (!s->data_file) {
>>> +ret = -EINVAL;
>>> +goto fail;
>>> +}
>>> +} else if (qdict_get(options, QCOW2_OPT_DATA_FILE)) {
>>
>> I get the idea, but this isn't crumpled so this key may not exist (but
>> data-file.driver and data-file.filename may).  Of course the fact that
>> these options remain unused will be caught by the block layer, but that
>> makes the error message below a bit less useful.
> 
> Hmm, good point... So you'd just leave out the check and always let the
> block layer complain (with a less than helpful message)? Or are you
> suggesting I should try and catch all cases somehow, even if that makes
> things quite a bit more complicated?

I don't mind either way.  Nice error messages are nice as long as
they're not too much trouble.  It's just that this current state feels a
bit half-baked.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [RFC PATCH 10/11] qcow2: Store data file name in the image

On 19.02.19 10:04, Kevin Wolf wrote:
> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> Rather than requiring that the external data file node is passed
>>> explicitly when creating the qcow2 node, store the filename in the
>>> designated header extension during .bdrv_create and read it from there
>>> as a default during .bdrv_open.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  block/qcow2.h  |  1 +
>>>  block/qcow2.c  | 69 +-
>>>  tests/qemu-iotests/082.out | 27 +++
>>>  3 files changed, 96 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 6cf862e8b9..4959bf16a4 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
>>> uint64_t start_offset,
>>>  #endif
>>>  break;
>>>  
>>> +case QCOW2_EXT_MAGIC_DATA_FILE:
>>> +{
>>> +s->image_data_file = g_malloc0(ext.len + 1);
>>> +ret = bdrv_pread(bs->file, offset, s->image_data_file, 
>>> ext.len);
>>> +if (ret < 0) {
>>> +error_setg_errno(errp, -ret, "ERROR: Could not data file 
>>> name");
>>
>> I think you accidentally a word.
>>
>>> +return ret;
>>> +}
>>> +#ifdef DEBUG_EXT
>>> +printf("Qcow2: Got external data file %s\n", 
>>> s->image_data_file);
>>> +#endif
>>> +break;
>>> +}
>>> +
>>>  default:
>>>  /* unknown magic - save it in case we need to rewrite the 
>>> header */
>>>  /* If you add a new feature, make sure to also update the fast
>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn 
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>  /* Open external data file */
>>>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>>>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>>> -   &child_file, false, errp);
>>> +   &child_file, false, &local_err);
>>> +if (!s->data_file) {
>>> +if (s->image_data_file) {
>>> +error_free(local_err);
>>> +local_err = NULL;
>>
>> This looked a bit weird to me at first because I was wondering why you
>> wouldn't just pass allow_none=true and then handle errors (by passing
>> them on).  But right, we want to retry with a filename set, maybe that
>> makes more sense of the options.
> 
> I think we want the normal error message for the !s->image_data_file
> case. With allow_none=true, we would have to come up with a new one here
> (in the else branch below).
> 
>> Hm.  But then again, do we really?  It matches what we do with backing
>> files, but that does give at least me headaches from time to time.  How
>> bad would it be to allow either passing all valid options through
>> @options (which would make qcow2 ignore the string in the header), or
>> use the filename given in the header alone?
> 
> You mean offering only one of the two ways to configure the node?

Either just the filename from the image header, or ignore that and take
all options from the user (who'd have to give a syntactically complete
QAPI BlockdevRef object).

> The 'data-file' runtime option is a must so that libvirt can build the
> graph node by node (and possibly use file descriptor passing one day).
> But having to specify the option every time is very unfriendly for human
> users, so I think allowing to store the file name in the header is a
> must, too.

Sure.  But I don't know whether we have to support taking the filename
from the image header, and the user overriding some of the node's
options (e.g. caching).

>>> +s->data_file = bdrv_open_child(s->image_data_file, options,
>>> +   "data-file", bs, 
>>> &child_file,
>>> +   false, errp);
>>> +} else {
>>> +error_propagate(errp, local_err);
>>> +}
>>> +}
>>>  if (!s->data_file) {
>>>  ret = -EINVAL;
>>>  goto fail;
>>
>> [...]
>>
>>> @@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const 
>>> char *filename, QemuOpts *opt
>>>  goto finish;
>>>  }
>>>  
>>> +/* Create and open an external data file (protocol layer) */
>>> +val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
>>> +if (val) {
>>> +ret = bdrv_create_file(val, opts, errp);
>>
>> I suppose taking an existing file is saved for later?
> 
> I think it's saved for the day that 'qemu-img create' is extended (or
> replaced with an alternative) to provide the same functionality as QMP
> blockdev-create.

Ah, yes. :-)

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PULL 24/27] tests/virtio-blk: change assert on data_size in virtio_blk_request()

From: Stefano Garzarella 

The size of data in the virtio_blk_request must be a multiple
of 512 bytes for IN and OUT requests, or a multiple of the size
of struct virtio_blk_discard_write_zeroes for DISCARD and
WRITE_ZEROES requests.

Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Thomas Huth 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-8-sgarz...@redhat.com
Message-Id: <20190221103314.58500-8-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/virtio-blk-test.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 04c608764b..0739498da7 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -144,7 +144,20 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, 
QVirtioDevice *d,
 uint64_t addr;
 uint8_t status = 0xFF;
 
-g_assert_cmpuint(data_size % 512, ==, 0);
+switch (req->type) {
+case VIRTIO_BLK_T_IN:
+case VIRTIO_BLK_T_OUT:
+g_assert_cmpuint(data_size % 512, ==, 0);
+break;
+case VIRTIO_BLK_T_DISCARD:
+case VIRTIO_BLK_T_WRITE_ZEROES:
+g_assert_cmpuint(data_size %
+ sizeof(struct virtio_blk_discard_write_zeroes), ==, 
0);
+break;
+default:
+g_assert_cmpuint(data_size, ==, 0);
+}
+
 addr = guest_alloc(alloc, sizeof(*req) + data_size);
 
 virtio_blk_fix_request(d, req);
-- 
2.20.1

Re: [Qemu-block] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset

On 19.02.19 09:45, Kevin Wolf wrote:
> Am 19.02.2019 um 00:13 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> The cluster allocation code uses 0 as an invalid offset that is used in
>>> case of errors or as "offset not yet determined". With external data
>>> files, a host cluster offset of 0 becomes valid, though.
>>>
>>> Define a constant INV_OFFSET (which is not cluster aligned and will
>>> therefore never be a valid offset) that can be used for such purposes.
>>>
>>> This removes the additional host_offset == 0 check that commit
>>> ff52aab2df5 introduced; the confusion between an invalid offset and
>>> (erroneous) allocation at offset 0 is removed with this change.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  block/qcow2.h |  2 ++
>>>  block/qcow2-cluster.c | 59 ---
>>>  2 files changed, 29 insertions(+), 32 deletions(-)
>>
>> qcow2_get_cluster_offset() still returns 0 for unallocated clusters.
>> (And qcow2_co_block_status() tests for that, so it would never report a
>> valid offset for the first cluster in an externally allocated qcow2 file.)
> 
> I think the bug here is in qcow2_get_cluster_offset().

You mean qcow2_co_block_status()?

>It shouldn't look
> at cluster_offset, but at ret if it wants to know the allocation status.
> So I think this needs to become something like:
> 
> if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC) &&
> !s->crypto) {
> ...
> }

I don't think that, because it doesn't want to know the allocation
status.  It wants to know whether it can return valid map information,
which it can if (1) qcow2_get_cluster_offset() returned a valid offset,
and (2) the data is represented in plain text (i.e. not compressed or
encrypted).

OTOH maybe having a whitelist instead of a blacklist would indeed be
more safe, in a sense.  But first, this isn't a pure whitelist, because
it still has to check "!s->crypto".  Second, it isn't like allocated
zero clusters store the data the same way it's seen in the guest.  So
even the whitelist part feels a bit weird to me.

All in all, the way it is right now makes more sense to me.

>> qcow2_alloc_compressed_cluster_offset() should return INV_OFFSET on
>> error (yeah, there are no compressed clusters in external files, but
>> this seems like the right thing to do still).
> 
> Ok, makes sense.
> 
>> (And there are cases like qcow2_co_preadv(), where cluster_offset is
>> initialized to 0 -- it doesn't make a difference what it's initialized
>> to (it's just to silence the compiler, I suppose), but it should still
>> use this new constant now.  I think.)
> 
> I don't think I would change places where cluster_offset is never used
> at all or never used alone without checking the cluster type, too.

OK.

> qcow2_get_cluster_offset() still returns 0 for unallocated and
> non-preallocated zero clusters, and I think that's fine because it also
> returns the cluster type, which is information about whether the offset
> is even valid.
> 
> In theory, it shouldn't matter at all if we return 0, INV_OFFSET or 42
> there, but in practice I'd bet neither money nor production images on
> this. If it ain't broke, don't fix it.
I don't see how an "organic growth" code base which sometimes uses 0 and
sometimes INV_OFFSET for invalid offsets is any more trustworthy, but
whatever.  I'm in a position where I don't have to learn the qcow2 code
from scratch but instead would have to review your changes, so I won't
complain further.

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PULL 27/27] tests/virtio-blk: add test for DISCARD command

From: Stefano Garzarella 

If the DISCARD feature is enabled, we try this command in the
test_basic(), checking only the status returned by the request.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-11-sgarz...@redhat.com
Message-Id: <20190221103314.58500-11-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/virtio-blk-test.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index f43d971099..8d2fc9c710 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -317,6 +317,33 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator 
*alloc,
 guest_free(alloc, req_addr);
 }
 
+if (features & (1u << VIRTIO_BLK_F_DISCARD)) {
+struct virtio_blk_discard_write_zeroes dwz_hdr;
+
+req.type = VIRTIO_BLK_T_DISCARD;
+req.data = (char *) &dwz_hdr;
+dwz_hdr.sector = 0;
+dwz_hdr.num_sectors = 1;
+dwz_hdr.flags = 0;
+
+virtio_blk_fix_dwz_hdr(dev, &dwz_hdr);
+
+req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr));
+
+free_head = qvirtqueue_add(vq, req_addr, 16, false, true);
+qvirtqueue_add(vq, req_addr + 16, sizeof(dwz_hdr), false, true);
+qvirtqueue_add(vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, false);
+
+qvirtqueue_kick(dev, vq, free_head);
+
+qvirtio_wait_used_elem(dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 16 + sizeof(dwz_hdr));
+g_assert_cmpint(status, ==, 0);
+
+guest_free(alloc, req_addr);
+}
+
 if (features & (1u << VIRTIO_F_ANY_LAYOUT)) {
 /* Write and read with 2 descriptor layout */
 /* Write request */
-- 
2.20.1

[Qemu-block] [PULL 23/27] virtio-blk: add DISCARD and WRITE_ZEROES features

From: Stefano Garzarella 

This patch adds the support of DISCARD and WRITE_ZEROES commands,
that have been introduced in the virtio-blk protocol to have
better performance when using SSD backend.

We support only one segment per request since multiple segments
are not widely used and there are no userspace APIs that allow
applications to submit multiple segments in a single call.

Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-7-sgarz...@redhat.com
Message-Id: <20190221103314.58500-7-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/virtio/virtio-blk.h |   2 +
 hw/block/virtio-blk.c  | 184 +
 2 files changed, 186 insertions(+)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 7877ae67ae..cddcfbebe9 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -38,6 +38,8 @@ struct VirtIOBlkConf
 uint32_t request_merging;
 uint16_t num_queues;
 uint16_t queue_size;
+uint32_t max_discard_sectors;
+uint32_t max_write_zeroes_sectors;
 };
 
 struct VirtIOBlockDataPlane;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8798d13bc4..c159a3d5f7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -169,6 +169,30 @@ out:
 aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
+static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
+{
+VirtIOBlockReq *req = opaque;
+VirtIOBlock *s = req->dev;
+bool is_write_zeroes = (virtio_ldl_p(VIRTIO_DEVICE(s), &req->out.type) &
+~VIRTIO_BLK_T_BARRIER) == 
VIRTIO_BLK_T_WRITE_ZEROES;
+
+aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
+if (ret) {
+if (virtio_blk_handle_rw_error(req, -ret, false, is_write_zeroes)) {
+goto out;
+}
+}
+
+virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
+if (is_write_zeroes) {
+block_acct_done(blk_get_stats(s->blk), &req->acct);
+}
+virtio_blk_free_request(req);
+
+out:
+aio_context_release(blk_get_aio_context(s->conf.conf.blk));
+}
+
 #ifdef __linux__
 
 typedef struct {
@@ -502,6 +526,84 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
 return true;
 }
 
+static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
+struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
+{
+VirtIOBlock *s = req->dev;
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+uint64_t sector;
+uint32_t num_sectors, flags, max_sectors;
+uint8_t err_status;
+int bytes;
+
+sector = virtio_ldq_p(vdev, &dwz_hdr->sector);
+num_sectors = virtio_ldl_p(vdev, &dwz_hdr->num_sectors);
+flags = virtio_ldl_p(vdev, &dwz_hdr->flags);
+max_sectors = is_write_zeroes ? s->conf.max_write_zeroes_sectors :
+  s->conf.max_discard_sectors;
+
+/*
+ * max_sectors is at most BDRV_REQUEST_MAX_SECTORS, this check
+ * make us sure that "num_sectors << BDRV_SECTOR_BITS" can fit in
+ * the integer variable.
+ */
+if (unlikely(num_sectors > max_sectors)) {
+err_status = VIRTIO_BLK_S_IOERR;
+goto err;
+}
+
+bytes = num_sectors << BDRV_SECTOR_BITS;
+
+if (unlikely(!virtio_blk_sect_range_ok(s, sector, bytes))) {
+err_status = VIRTIO_BLK_S_IOERR;
+goto err;
+}
+
+/*
+ * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard
+ * and write zeroes commands if any unknown flag is set.
+ */
+if (unlikely(flags & ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
+err_status = VIRTIO_BLK_S_UNSUPP;
+goto err;
+}
+
+if (is_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
+int blk_aio_flags = 0;
+
+if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
+blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
+}
+
+block_acct_start(blk_get_stats(s->blk), &req->acct, bytes,
+ BLOCK_ACCT_WRITE);
+
+blk_aio_pwrite_zeroes(s->blk, sector << BDRV_SECTOR_BITS,
+  bytes, blk_aio_flags,
+  virtio_blk_discard_write_zeroes_complete, req);
+} else { /* VIRTIO_BLK_T_DISCARD */
+/*
+ * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for
+ * discard commands if the unmap flag is set.
+ */
+if (unlikely(flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
+err_status = VIRTIO_BLK_S_UNSUPP;
+goto err;
+}
+
+blk_aio_pdiscard(s->blk, sector << BDRV_SECTOR_BITS, bytes,
+ virtio_blk_discard_write_zeroes_complete, req);
+}
+
+return VIRTIO_BLK_S_OK;
+
+err:
+if (is_write_zeroes) {
+block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
+}
+return err_status;
+}
+
 static int virtio_blk_handle

[Qemu-block] [PULL 19/27] virtio-blk: add host_features field in VirtIOBlock

From: Stefano Garzarella 

Since configurable features for virtio-blk are growing, this patch
adds host_features field in the struct VirtIOBlock. (as in virtio-net)
In this way, we can avoid to add new fields for new properties and
we can directly set VIRTIO_BLK_F* flags in the host_features.

We update "config-wce" and "scsi" property definition to use the new
host_features field without change the behaviour.

Suggested-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-3-sgarz...@redhat.com
Message-Id: <20190221103314.58500-3-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/virtio/virtio-blk.h |  3 +--
 hw/block/virtio-blk.c  | 16 +---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 5117431d96..f7345b0511 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -35,8 +35,6 @@ struct VirtIOBlkConf
 BlockConf conf;
 IOThread *iothread;
 char *serial;
-uint32_t scsi;
-uint32_t config_wce;
 uint32_t request_merging;
 uint16_t num_queues;
 uint16_t queue_size;
@@ -57,6 +55,7 @@ typedef struct VirtIOBlock {
 bool dataplane_disabled;
 bool dataplane_started;
 struct VirtIOBlockDataPlane *dataplane;
+uint64_t host_features;
 } VirtIOBlock;
 
 typedef struct VirtIOBlockReq {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index afedf48ca6..7813237756 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -245,7 +245,7 @@ static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
  */
 scsi = (void *)elem->in_sg[elem->in_num - 2].iov_base;
 
-if (!blk->conf.scsi) {
+if (!virtio_has_feature(blk->host_features, VIRTIO_BLK_F_SCSI)) {
 status = VIRTIO_BLK_S_UNSUPP;
 goto fail;
 }
@@ -785,12 +785,15 @@ static uint64_t virtio_blk_get_features(VirtIODevice 
*vdev, uint64_t features,
 {
 VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+/* Firstly sync all virtio-blk possible supported features */
+features |= s->host_features;
+
 virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX);
 virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
 virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
 virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
 if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
-if (s->conf.scsi) {
+if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_SCSI)) {
 error_setg(errp, "Please set scsi=off for virtio-blk devices in 
order to use virtio 1.0");
 return 0;
 }
@@ -799,9 +802,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, 
uint64_t features,
 virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
 }
 
-if (s->conf.config_wce) {
-virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-}
 if (blk_enable_write_cache(s->blk)) {
 virtio_add_feature(&features, VIRTIO_BLK_F_WCE);
 }
@@ -1015,9 +1015,11 @@ static Property virtio_blk_properties[] = {
 DEFINE_BLOCK_ERROR_PROPERTIES(VirtIOBlock, conf.conf),
 DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, conf.conf),
 DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial),
-DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true),
+DEFINE_PROP_BIT64("config-wce", VirtIOBlock, host_features,
+  VIRTIO_BLK_F_CONFIG_WCE, true),
 #ifdef __linux__
-DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false),
+DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features,
+  VIRTIO_BLK_F_SCSI, false),
 #endif
 DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
 true),
-- 
2.20.1

[Qemu-block] [PULL 21/27] virtio-net: make VirtIOFeature usable for other virtio devices

From: Stefano Garzarella 

In order to use VirtIOFeature also in other virtio devices, we move
its declaration and the endof() macro (renamed in virtio_endof())
in virtio.h.
We add virtio_feature_get_config_size() function to iterate the array
of VirtIOFeature and to return the config size depending on the
features enabled. (as virtio_net_set_config_size() did)

Suggested-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-5-sgarz...@redhat.com
Message-Id: <20190221103314.58500-5-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/virtio/virtio.h | 15 +++
 hw/net/virtio-net.c| 31 +++
 hw/virtio/virtio.c | 15 +++
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9c1fa07d6d..ce9516236a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -37,6 +37,21 @@ static inline hwaddr vring_align(hwaddr addr,
 return QEMU_ALIGN_UP(addr, align);
 }
 
+/*
+ * Calculate the number of bytes up to and including the given 'field' of
+ * 'container'.
+ */
+#define virtio_endof(container, field) \
+(offsetof(container, field) + sizeof_field(container, field))
+
+typedef struct VirtIOFeature {
+uint64_t flags;
+size_t end;
+} VirtIOFeature;
+
+size_t virtio_feature_get_config_size(VirtIOFeature *features,
+  uint64_t host_features);
+
 typedef struct VirtQueue VirtQueue;
 
 #define VIRTQUEUE_MAX_SIZE 1024
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3f319ef723..6e6b146022 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -82,29 +82,17 @@ static inline __virtio16 *virtio_net_rsc_ext_num_dupacks(
 
 #endif
 
-/*
- * Calculate the number of bytes up to and including the given 'field' of
- * 'container'.
- */
-#define endof(container, field) \
-(offsetof(container, field) + sizeof_field(container, field))
-
-typedef struct VirtIOFeature {
-uint64_t flags;
-size_t end;
-} VirtIOFeature;
-
 static VirtIOFeature feature_sizes[] = {
 {.flags = 1ULL << VIRTIO_NET_F_MAC,
- .end = endof(struct virtio_net_config, mac)},
+ .end = virtio_endof(struct virtio_net_config, mac)},
 {.flags = 1ULL << VIRTIO_NET_F_STATUS,
- .end = endof(struct virtio_net_config, status)},
+ .end = virtio_endof(struct virtio_net_config, status)},
 {.flags = 1ULL << VIRTIO_NET_F_MQ,
- .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
+ .end = virtio_endof(struct virtio_net_config, max_virtqueue_pairs)},
 {.flags = 1ULL << VIRTIO_NET_F_MTU,
- .end = endof(struct virtio_net_config, mtu)},
+ .end = virtio_endof(struct virtio_net_config, mtu)},
 {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
- .end = endof(struct virtio_net_config, duplex)},
+ .end = virtio_endof(struct virtio_net_config, duplex)},
 {}
 };
 
@@ -2580,15 +2568,10 @@ static void virtio_net_guest_notifier_mask(VirtIODevice 
*vdev, int idx,
 
 static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
 {
-int i, config_size = 0;
 virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
 
-for (i = 0; feature_sizes[i].flags != 0; i++) {
-if (host_features & feature_sizes[i].flags) {
-config_size = MAX(feature_sizes[i].end, config_size);
-}
-}
-n->config_size = config_size;
+n->config_size = virtio_feature_get_config_size(feature_sizes,
+host_features);
 }
 
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a1ff647a66..2626a895cb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2036,6 +2036,21 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
 return ret;
 }
 
+size_t virtio_feature_get_config_size(VirtIOFeature *feature_sizes,
+  uint64_t host_features)
+{
+size_t config_size = 0;
+int i;
+
+for (i = 0; feature_sizes[i].flags != 0; i++) {
+if (host_features & feature_sizes[i].flags) {
+config_size = MAX(feature_sizes[i].end, config_size);
+}
+}
+
+return config_size;
+}
+
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 {
 int i, ret;
-- 
2.20.1

[Qemu-block] [PULL 20/27] virtio-blk: add "discard" and "write-zeroes" properties

From: Stefano Garzarella 

In order to avoid migration issues, we enable DISCARD and
WRITE_ZEROES features only for machine type >= 4.0

As discussed with Michael S. Tsirkin and Stefan Hajnoczi on the
list [1], DISCARD operation should not have security implications
(eg. page cache attacks), so we can enable it by default.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg00504.html

Suggested-by: Dr. David Alan Gilbert 
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-4-sgarz...@redhat.com
Message-Id: <20190221103314.58500-4-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 hw/block/virtio-blk.c | 4 
 hw/core/machine.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 7813237756..f7cd322811 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1027,6 +1027,10 @@ static Property virtio_blk_properties[] = {
 DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
 DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
  IOThread *),
+DEFINE_PROP_BIT64("discard", VirtIOBlock, host_features,
+  VIRTIO_BLK_F_DISCARD, true),
+DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
+  VIRTIO_BLK_F_WRITE_ZEROES, true),
 DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 077fbd182a..766ca5899d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,6 +33,8 @@ GlobalProperty hw_compat_3_1[] = {
 { "usb-kbd", "serial", "42" },
 { "usb-mouse", "serial", "42" },
 { "usb-kbd", "serial", "42" },
+{ "virtio-blk-device", "discard", "false" },
+{ "virtio-blk-device", "write-zeroes", "false" },
 };
 const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
 
-- 
2.20.1

[Qemu-block] [PULL 16/27] hw/ide: drop iov field from IDEBufferedRequest

From: Vladimir Sementsov-Ogievskiy 

@iov is used only to initialize @qiov. Let's use new
qemu_iovec_init_buf() instead, which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-17-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-17-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/ide/internal.h |  1 -
 hw/ide/core.c | 11 ++-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index fa99486d7a..1b02bb9151 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -346,7 +346,6 @@ extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
 
 typedef struct IDEBufferedRequest {
 QLIST_ENTRY(IDEBufferedRequest) list;
-struct iovec iov;
 QEMUIOVector qiov;
 QEMUIOVector *original_qiov;
 BlockCompletionFunc *original_cb;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 13f8f78ca6..6afadf894f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -629,13 +629,15 @@ static void ide_buffered_readv_cb(void *opaque, int ret)
 IDEBufferedRequest *req = opaque;
 if (!req->orphaned) {
 if (!ret) {
-qemu_iovec_from_buf(req->original_qiov, 0, req->iov.iov_base,
+assert(req->qiov.size == req->original_qiov->size);
+qemu_iovec_from_buf(req->original_qiov, 0,
+req->qiov.local_iov.iov_base,
 req->original_qiov->size);
 }
 req->original_cb(req->original_opaque, ret);
 }
 QLIST_REMOVE(req, list);
-qemu_vfree(req->iov.iov_base);
+qemu_vfree(qemu_iovec_buf(&req->qiov));
 g_free(req);
 }
 
@@ -660,9 +662,8 @@ BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t 
sector_num,
 req->original_qiov = iov;
 req->original_cb = cb;
 req->original_opaque = opaque;
-req->iov.iov_base = qemu_blockalign(blk_bs(s->blk), iov->size);
-req->iov.iov_len = iov->size;
-qemu_iovec_init_external(&req->qiov, &req->iov, 1);
+qemu_iovec_init_buf(&req->qiov, blk_blockalign(s->blk, iov->size),
+iov->size);
 
 aioreq = blk_aio_preadv(s->blk, sector_num << BDRV_SECTOR_BITS,
 &req->qiov, 0, ide_buffered_readv_cb, req);
-- 
2.20.1

[Qemu-block] [PULL 26/27] tests/virtio-blk: add test for WRITE_ZEROES command

From: Stefano Garzarella 

If the WRITE_ZEROES feature is enabled, we check this command
in the test_basic().

Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Acked-by: Thomas Huth 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-10-sgarz...@redhat.com
Message-Id: <20190221103314.58500-10-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/virtio-blk-test.c | 62 +
 1 file changed, 62 insertions(+)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 85b6b99adc..f43d971099 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -255,6 +255,68 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator 
*alloc,
 
 guest_free(alloc, req_addr);
 
+if (features & (1u << VIRTIO_BLK_F_WRITE_ZEROES)) {
+struct virtio_blk_discard_write_zeroes dwz_hdr;
+void *expected;
+
+/*
+ * WRITE_ZEROES request on the same sector of previous test where
+ * we wrote "TEST".
+ */
+req.type = VIRTIO_BLK_T_WRITE_ZEROES;
+req.data = (char *) &dwz_hdr;
+dwz_hdr.sector = 0;
+dwz_hdr.num_sectors = 1;
+dwz_hdr.flags = 0;
+
+virtio_blk_fix_dwz_hdr(dev, &dwz_hdr);
+
+req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr));
+
+free_head = qvirtqueue_add(vq, req_addr, 16, false, true);
+qvirtqueue_add(vq, req_addr + 16, sizeof(dwz_hdr), false, true);
+qvirtqueue_add(vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, false);
+
+qvirtqueue_kick(dev, vq, free_head);
+
+qvirtio_wait_used_elem(dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 16 + sizeof(dwz_hdr));
+g_assert_cmpint(status, ==, 0);
+
+guest_free(alloc, req_addr);
+
+/* Read request to check if the sector contains all zeroes */
+req.type = VIRTIO_BLK_T_IN;
+req.ioprio = 1;
+req.sector = 0;
+req.data = g_malloc0(512);
+
+req_addr = virtio_blk_request(alloc, dev, &req, 512);
+
+g_free(req.data);
+
+free_head = qvirtqueue_add(vq, req_addr, 16, false, true);
+qvirtqueue_add(vq, req_addr + 16, 512, true, true);
+qvirtqueue_add(vq, req_addr + 528, 1, true, false);
+
+qvirtqueue_kick(dev, vq, free_head);
+
+qvirtio_wait_used_elem(dev, vq, free_head, NULL,
+   QVIRTIO_BLK_TIMEOUT_US);
+status = readb(req_addr + 528);
+g_assert_cmpint(status, ==, 0);
+
+data = g_malloc(512);
+expected = g_malloc0(512);
+memread(req_addr + 16, data, 512);
+g_assert_cmpmem(data, 512, expected, 512);
+g_free(expected);
+g_free(data);
+
+guest_free(alloc, req_addr);
+}
+
 if (features & (1u << VIRTIO_F_ANY_LAYOUT)) {
 /* Write and read with 2 descriptor layout */
 /* Write request */
-- 
2.20.1

[Qemu-block] [PULL 17/27] hw/ide: drop iov field from IDEDMA

From: Vladimir Sementsov-Ogievskiy 

@iov is used only to initialize @qiov. Let's use new
qemu_iovec_init_buf() instead, which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-18-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-18-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/ide/internal.h | 1 -
 hw/ide/atapi.c| 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1b02bb9151..8efd03132b 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -455,7 +455,6 @@ struct IDEDMAOps {
 
 struct IDEDMA {
 const struct IDEDMAOps *ops;
-struct iovec iov;
 QEMUIOVector qiov;
 BlockAIOCB *aiocb;
 };
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 4de86555d9..1b0f66cc08 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -420,9 +420,8 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 data_offset = 0;
 }
 trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
-s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
-s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
-qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
+qemu_iovec_init_buf(&s->bus->dma->qiov, s->io_buffer + data_offset,
+n * ATAPI_SECTOR_SIZE);
 
 s->bus->dma->aiocb = ide_buffered_readv(s, (int64_t)s->lba << 2,
 &s->bus->dma->qiov, n * 4,
-- 
2.20.1

[Qemu-block] [PULL 12/27] qemu-img: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-13-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-13-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 qemu-img.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 25288c4d18..7853935049 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1670,7 +1670,6 @@ static int coroutine_fn convert_co_read(ImgConvertState 
*s, int64_t sector_num,
 {
 int n, ret;
 QEMUIOVector qiov;
-struct iovec iov;
 
 assert(nb_sectors <= s->buf_sectors);
 while (nb_sectors > 0) {
@@ -1686,9 +1685,7 @@ static int coroutine_fn convert_co_read(ImgConvertState 
*s, int64_t sector_num,
 bs_sectors = s->src_sectors[src_cur];
 
 n = MIN(nb_sectors, bs_sectors - (sector_num - src_cur_offset));
-iov.iov_base = buf;
-iov.iov_len = n << BDRV_SECTOR_BITS;
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, buf, n << BDRV_SECTOR_BITS);
 
 ret = blk_co_preadv(
 blk, (sector_num - src_cur_offset) << BDRV_SECTOR_BITS,
@@ -1712,7 +1709,6 @@ static int coroutine_fn convert_co_write(ImgConvertState 
*s, int64_t sector_num,
 {
 int ret;
 QEMUIOVector qiov;
-struct iovec iov;
 
 while (nb_sectors > 0) {
 int n = nb_sectors;
@@ -1740,9 +1736,7 @@ static int coroutine_fn convert_co_write(ImgConvertState 
*s, int64_t sector_num,
 (s->compressed &&
  !buffer_is_zero(buf, n * BDRV_SECTOR_SIZE)))
 {
-iov.iov_base = buf;
-iov.iov_len = n << BDRV_SECTOR_BITS;
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, buf, n << BDRV_SECTOR_BITS);
 
 ret = blk_co_pwritev(s->target, sector_num << BDRV_SECTOR_BITS,
  n << BDRV_SECTOR_BITS, &qiov, flags);
-- 
2.20.1

[Qemu-block] [PULL 25/27] tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function

From: Stefano Garzarella 

This function is useful to fix the endianness of struct
virtio_blk_discard_write_zeroes headers.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-9-sgarz...@redhat.com
Message-Id: <20190221103314.58500-9-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/virtio-blk-test.c | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0739498da7..85b6b99adc 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -46,6 +46,12 @@ typedef struct QVirtioBlkReq {
 uint8_t status;
 } QVirtioBlkReq;
 
+#ifdef HOST_WORDS_BIGENDIAN
+const bool host_is_big_endian = true;
+#else
+const bool host_is_big_endian; /* false */
+#endif
+
 static char *drive_create(void)
 {
 int fd, ret;
@@ -125,12 +131,6 @@ static QVirtioPCIDevice *virtio_blk_pci_init(QPCIBus *bus, 
int slot)
 
 static inline void virtio_blk_fix_request(QVirtioDevice *d, QVirtioBlkReq *req)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-const bool host_is_big_endian = true;
-#else
-const bool host_is_big_endian = false;
-#endif
-
 if (qvirtio_is_big_endian(d) != host_is_big_endian) {
 req->type = bswap32(req->type);
 req->ioprio = bswap32(req->ioprio);
@@ -138,6 +138,17 @@ static inline void virtio_blk_fix_request(QVirtioDevice 
*d, QVirtioBlkReq *req)
 }
 }
 
+
+static inline void virtio_blk_fix_dwz_hdr(QVirtioDevice *d,
+struct virtio_blk_discard_write_zeroes *dwz_hdr)
+{
+if (qvirtio_is_big_endian(d) != host_is_big_endian) {
+dwz_hdr->sector = bswap64(dwz_hdr->sector);
+dwz_hdr->num_sectors = bswap32(dwz_hdr->num_sectors);
+dwz_hdr->flags = bswap32(dwz_hdr->flags);
+}
+}
+
 static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d,
QVirtioBlkReq *req, uint64_t data_size)
 {
-- 
2.20.1

[Qemu-block] [PULL 18/27] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()

From: Stefano Garzarella 

We add acct_failed param in order to use virtio_blk_handle_rw_error()
also when is not required to call block_acct_failed(). (eg. a discard
operation is failed)

Reviewed-by: Michael S. Tsirkin 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-2-sgarz...@redhat.com
Message-Id: <20190221103314.58500-2-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 hw/block/virtio-blk.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cf7f47eaba..afedf48ca6 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -65,7 +65,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, 
unsigned char status)
 }
 
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
-bool is_read)
+bool is_read, bool acct_failed)
 {
 VirtIOBlock *s = req->dev;
 BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
@@ -78,7 +78,9 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, 
int error,
 s->rq = req;
 } else if (action == BLOCK_ERROR_ACTION_REPORT) {
 virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
-block_acct_failed(blk_get_stats(s->blk), &req->acct);
+if (acct_failed) {
+block_acct_failed(blk_get_stats(s->blk), &req->acct);
+}
 virtio_blk_free_request(req);
 }
 
@@ -116,7 +118,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
  * the memory until the request is completed (which will
  * happen on the other side of the migration).
  */
-if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
+if (virtio_blk_handle_rw_error(req, -ret, is_read, true)) {
 continue;
 }
 }
@@ -135,7 +137,7 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
 
 aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
 if (ret) {
-if (virtio_blk_handle_rw_error(req, -ret, 0)) {
+if (virtio_blk_handle_rw_error(req, -ret, 0, true)) {
 goto out;
 }
 }
-- 
2.20.1

[Qemu-block] [PULL 22/27] virtio-blk: set config size depending on the features enabled

From: Stefano Garzarella 

Starting from DISABLE and WRITE_ZEROES features, we use an array of
VirtIOFeature (as virtio-net) to properly set the config size
depending on the features enabled.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Stefano Garzarella 
Message-id: 20190221103314.58500-6-sgarz...@redhat.com
Message-Id: <20190221103314.58500-6-sgarz...@redhat.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/virtio/virtio-blk.h |  1 +
 hw/block/virtio-blk.c  | 31 +--
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index f7345b0511..7877ae67ae 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -56,6 +56,7 @@ typedef struct VirtIOBlock {
 bool dataplane_started;
 struct VirtIOBlockDataPlane *dataplane;
 uint64_t host_features;
+size_t config_size;
 } VirtIOBlock;
 
 typedef struct VirtIOBlockReq {
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f7cd322811..8798d13bc4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -28,9 +28,28 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 
-/* We don't support discard yet, hide associated config fields. */
+/* Config size before the discard support (hide associated config fields) */
 #define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
  max_discard_sectors)
+/*
+ * Starting from the discard feature, we can use this array to properly
+ * set the config size depending on the features enabled.
+ */
+static VirtIOFeature feature_sizes[] = {
+{.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
+ .end = virtio_endof(struct virtio_blk_config, discard_sector_alignment)},
+{.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
+ .end = virtio_endof(struct virtio_blk_config, write_zeroes_may_unmap)},
+{}
+};
+
+static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
+{
+s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
+virtio_feature_get_config_size(feature_sizes, host_features));
+
+assert(s->config_size <= sizeof(struct virtio_blk_config));
+}
 
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
 VirtIOBlockReq *req)
@@ -763,8 +782,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 blkcfg.alignment_offset = 0;
 blkcfg.wce = blk_enable_write_cache(s->blk);
 virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
-memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
-QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
+memcpy(config, &blkcfg, s->config_size);
 }
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -772,8 +790,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const 
uint8_t *config)
 VirtIOBlock *s = VIRTIO_BLK(vdev);
 struct virtio_blk_config blkcfg;
 
-memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
-QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
+memcpy(&blkcfg, config, s->config_size);
 
 aio_context_acquire(blk_get_aio_context(s->blk));
 blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
@@ -956,7 +973,9 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 
-virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
+virtio_blk_set_config_size(s, s->host_features);
+
+virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
 
 s->blk = conf->conf.blk;
 s->rq = NULL;
-- 
2.20.1

[Qemu-block] [PULL 14/27] tests/test-bdrv-drain: use QEMU_IOVEC_INIT_BUF

From: Vladimir Sementsov-Ogievskiy 

Use new QEMU_IOVEC_INIT_BUF() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-15-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-15-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 tests/test-bdrv-drain.c | 29 -
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index ee1740ff06..821be405f0 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -204,12 +204,7 @@ static void test_drv_cb_common(enum drain_type drain_type, 
bool recursive)
 BlockAIOCB *acb;
 int aio_ret;
 
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = NULL,
-.iov_len = 0,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
 
 blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
 bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
@@ -670,12 +665,7 @@ static void test_iothread_common(enum drain_type 
drain_type, int drain_thread)
 AioContext *ctx_a = iothread_get_aio_context(a);
 AioContext *ctx_b = iothread_get_aio_context(b);
 
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = NULL,
-.iov_len = 0,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
 
 /* bdrv_drain_all() may only be called from the main loop thread */
 if (drain_type == BDRV_DRAIN_ALL && drain_thread != 0) {
@@ -1148,13 +1138,7 @@ static void coroutine_fn test_co_delete_by_drain(void 
*opaque)
 BlockDriverState *bs = blk_bs(blk);
 BDRVTestTopState *tts = bs->opaque;
 void *buffer = g_malloc(65536);
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = buffer,
-.iov_len  = 65536,
-};
-
-qemu_iovec_init_external(&qiov, &iov, 1);
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buffer, 65536);
 
 /* Pretend some internal write operation from parent to child.
  * Important: We have to read from the child, not from the parent!
@@ -1365,12 +1349,7 @@ static void test_detach_indirect(bool by_parent_cb)
 BdrvChild *child_a, *child_b;
 BlockAIOCB *acb;
 
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = NULL,
-.iov_len = 0,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
 
 if (!by_parent_cb) {
 detach_by_driver_cb_role = child_file;
-- 
2.20.1

[Qemu-block] [PULL 15/27] hw/ide: drop iov field from IDEState

From: Vladimir Sementsov-Ogievskiy 

@iov is used only to initialize @qiov. Let's use new
qemu_iovec_init_buf() instead, which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-16-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-16-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/hw/ide/internal.h | 1 -
 hw/ide/atapi.c| 9 -
 hw/ide/core.c | 8 ++--
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 880413ddc7..fa99486d7a 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -405,7 +405,6 @@ struct IDEState {
 int atapi_dma; /* true if dma is requested for the packet cmd */
 BlockAcctCookie acct;
 BlockAIOCB *pio_aiocb;
-struct iovec iov;
 QEMUIOVector qiov;
 QLIST_HEAD(, IDEBufferedRequest) buffered_requests;
 /* ATA DMA state */
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 39e473f9c2..4de86555d9 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -174,16 +174,15 @@ static void cd_read_sector_cb(void *opaque, int ret)
 
 static int cd_read_sector(IDEState *s)
 {
+void *buf;
+
 if (s->cd_sector_size != 2048 && s->cd_sector_size != 2352) {
 block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_READ);
 return -EINVAL;
 }
 
-s->iov.iov_base = (s->cd_sector_size == 2352) ?
-  s->io_buffer + 16 : s->io_buffer;
-
-s->iov.iov_len = ATAPI_SECTOR_SIZE;
-qemu_iovec_init_external(&s->qiov, &s->iov, 1);
+buf = (s->cd_sector_size == 2352) ? s->io_buffer + 16 : s->io_buffer;
+qemu_iovec_init_buf(&s->qiov, buf, ATAPI_SECTOR_SIZE);
 
 trace_cd_read_sector(s->lba);
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 84832008b8..13f8f78ca6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -774,9 +774,7 @@ static void ide_sector_read(IDEState *s)
 return;
 }
 
-s->iov.iov_base = s->io_buffer;
-s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
-qemu_iovec_init_external(&s->qiov, &s->iov, 1);
+qemu_iovec_init_buf(&s->qiov, s->io_buffer, n * BDRV_SECTOR_SIZE);
 
 block_acct_start(blk_get_stats(s->blk), &s->acct,
  n * BDRV_SECTOR_SIZE, BLOCK_ACCT_READ);
@@ -1045,9 +1043,7 @@ static void ide_sector_write(IDEState *s)
 return;
 }
 
-s->iov.iov_base = s->io_buffer;
-s->iov.iov_len  = n * BDRV_SECTOR_SIZE;
-qemu_iovec_init_external(&s->qiov, &s->iov, 1);
+qemu_iovec_init_buf(&s->qiov, s->io_buffer, n * BDRV_SECTOR_SIZE);
 
 block_acct_start(blk_get_stats(s->blk), &s->acct,
  n * BDRV_SECTOR_SIZE, BLOCK_ACCT_WRITE);
-- 
2.20.1

[Qemu-block] [PULL 09/27] block/qcow2: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-10-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-10-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/qcow2.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 65a54c9ac6..b6d475229e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3894,7 +3894,6 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 {
 BDRVQcow2State *s = bs->opaque;
 QEMUIOVector hd_qiov;
-struct iovec iov;
 int ret;
 size_t out_len;
 uint8_t *buf, *out_buf;
@@ -3960,11 +3959,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 goto fail;
 }
 
-iov = (struct iovec) {
-.iov_base   = out_buf,
-.iov_len= out_len,
-};
-qemu_iovec_init_external(&hd_qiov, &iov, 1);
+qemu_iovec_init_buf(&hd_qiov, out_buf, out_len);
 
 BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
 ret = bdrv_co_pwritev(bs->file, cluster_offset, out_len, &hd_qiov, 0);
@@ -3990,7 +3985,6 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
 int ret = 0, csize, nb_csectors;
 uint64_t coffset;
 uint8_t *buf, *out_buf;
-struct iovec iov;
 QEMUIOVector local_qiov;
 int offset_in_cluster = offset_into_cluster(s, offset);
 
@@ -4002,9 +3996,7 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
 if (!buf) {
 return -ENOMEM;
 }
-iov.iov_base = buf;
-iov.iov_len = csize;
-qemu_iovec_init_external(&local_qiov, &iov, 1);
+qemu_iovec_init_buf(&local_qiov, buf, csize);
 
 out_buf = qemu_blockalign(bs, s->cluster_size);
 
-- 
2.20.1

[Qemu-block] [PULL 10/27] block/qed: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-11-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-11-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/qed-table.c | 16 +++-
 block/qed.c   | 31 +--
 2 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/block/qed-table.c b/block/qed-table.c
index 7df5680adb..c497bd4aec 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -21,16 +21,11 @@
 /* Called with table_lock held.  */
 static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table)
 {
-QEMUIOVector qiov;
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(
+qiov, table->offsets, s->header.cluster_size * s->header.table_size);
 int noffsets;
 int i, ret;
 
-struct iovec iov = {
-.iov_base = table->offsets,
-.iov_len = s->header.cluster_size * s->header.table_size,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
-
 trace_qed_read_table(s, offset, table);
 
 qemu_co_mutex_unlock(&s->table_lock);
@@ -71,7 +66,6 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, 
QEDTable *table,
 unsigned int sector_mask = BDRV_SECTOR_SIZE / sizeof(uint64_t) - 1;
 unsigned int start, end, i;
 QEDTable *new_table;
-struct iovec iov;
 QEMUIOVector qiov;
 size_t len_bytes;
 int ret;
@@ -85,11 +79,7 @@ static int qed_write_table(BDRVQEDState *s, uint64_t offset, 
QEDTable *table,
 len_bytes = (end - start) * sizeof(uint64_t);
 
 new_table = qemu_blockalign(s->bs, len_bytes);
-iov = (struct iovec) {
-.iov_base = new_table->offsets,
-.iov_len = len_bytes,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, new_table->offsets, len_bytes);
 
 /* Byteswap table */
 for (i = start; i < end; i++) {
diff --git a/block/qed.c b/block/qed.c
index 1280870024..c5e6d6ad41 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -113,18 +113,13 @@ static int coroutine_fn qed_write_header(BDRVQEDState *s)
 int nsectors = DIV_ROUND_UP(sizeof(QEDHeader), BDRV_SECTOR_SIZE);
 size_t len = nsectors * BDRV_SECTOR_SIZE;
 uint8_t *buf;
-struct iovec iov;
 QEMUIOVector qiov;
 int ret;
 
 assert(s->allocating_acb || s->allocating_write_reqs_plugged);
 
 buf = qemu_blockalign(s->bs, len);
-iov = (struct iovec) {
-.iov_base = buf,
-.iov_len = len,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, buf, len);
 
 ret = bdrv_co_preadv(s->bs->file, 0, qiov.size, &qiov, 0);
 if (ret < 0) {
@@ -913,7 +908,6 @@ static int coroutine_fn 
qed_copy_from_backing_file(BDRVQEDState *s,
 {
 QEMUIOVector qiov;
 QEMUIOVector *backing_qiov = NULL;
-struct iovec iov;
 int ret;
 
 /* Skip copy entirely if there is no work to do */
@@ -921,11 +915,7 @@ static int coroutine_fn 
qed_copy_from_backing_file(BDRVQEDState *s,
 return 0;
 }
 
-iov = (struct iovec) {
-.iov_base = qemu_blockalign(s->bs, len),
-.iov_len = len,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, qemu_blockalign(s->bs, len), len);
 
 ret = qed_read_backing_file(s, pos, &qiov, &backing_qiov);
 
@@ -946,7 +936,7 @@ static int coroutine_fn 
qed_copy_from_backing_file(BDRVQEDState *s,
 }
 ret = 0;
 out:
-qemu_vfree(iov.iov_base);
+qemu_vfree(qemu_iovec_buf(&qiov));
 return ret;
 }
 
@@ -1447,8 +1437,12 @@ static int coroutine_fn 
bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
   BdrvRequestFlags flags)
 {
 BDRVQEDState *s = bs->opaque;
-QEMUIOVector qiov;
-struct iovec iov;
+
+/*
+ * Zero writes start without an I/O buffer.  If a buffer becomes necessary
+ * then it will be allocated during request processing.
+ */
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
 
 /* Fall back if the request is not aligned */
 if (qed_offset_into_cluster(s, offset) ||
@@ -1456,13 +1450,6 @@ static int coroutine_fn 
bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
 return -ENOTSUP;
 }
 
-/* Zero writes start without an I/O buffer.  If a buffer becomes necessary
- * then it will be allocated during request processing.
- */
-iov.iov_base = NULL;
-iov.iov_len = bytes;
-
-qemu_iovec_init_external(&qiov, &iov, 1);
 return qed_co_request(bs, offset >> BDRV_SECTOR_BITS, &qiov,
   bytes >> BDRV_SECTOR_BITS,
   QED_AIOCB_WRITE | QED_AIOCB_ZERO);
-- 
2.20.1

[Qemu-block] [PULL 13/27] migration/block: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-14-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-14-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 migration/block.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 0e24e18d13..83c633fb3f 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -83,7 +83,6 @@ typedef struct BlkMigBlock {
 BlkMigDevState *bmds;
 int64_t sector;
 int nr_sectors;
-struct iovec iov;
 QEMUIOVector qiov;
 BlockAIOCB *aiocb;
 
@@ -314,9 +313,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState 
*bmds)
 blk->sector = cur_sector;
 blk->nr_sectors = nr_sectors;
 
-blk->iov.iov_base = blk->buf;
-blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE;
-qemu_iovec_init_external(&blk->qiov, &blk->iov, 1);
+qemu_iovec_init_buf(&blk->qiov, blk->buf, nr_sectors * BDRV_SECTOR_SIZE);
 
 blk_mig_lock();
 block_mig_state.submitted++;
@@ -556,9 +553,8 @@ static int mig_save_device_dirty(QEMUFile *f, 
BlkMigDevState *bmds,
 blk->nr_sectors = nr_sectors;
 
 if (is_async) {
-blk->iov.iov_base = blk->buf;
-blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE;
-qemu_iovec_init_external(&blk->qiov, &blk->iov, 1);
+qemu_iovec_init_buf(&blk->qiov, blk->buf,
+nr_sectors * BDRV_SECTOR_SIZE);
 
 blk->aiocb = blk_aio_preadv(bmds->blk,
 sector * BDRV_SECTOR_SIZE,
-- 
2.20.1

[Qemu-block] [PULL 08/27] block/qcow: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-9-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-9-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/qcow.c | 21 -
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 0a235bf393..409c700d33 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -628,7 +628,6 @@ static coroutine_fn int qcow_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 int offset_in_cluster;
 int ret = 0, n;
 uint64_t cluster_offset;
-struct iovec hd_iov;
 QEMUIOVector hd_qiov;
 uint8_t *buf;
 void *orig_buf;
@@ -661,9 +660,7 @@ static coroutine_fn int qcow_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 if (!cluster_offset) {
 if (bs->backing) {
 /* read from the base image */
-hd_iov.iov_base = (void *)buf;
-hd_iov.iov_len = n;
-qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+qemu_iovec_init_buf(&hd_qiov, buf, n);
 qemu_co_mutex_unlock(&s->lock);
 /* qcow2 emits this on bs->file instead of bs->backing */
 BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
@@ -688,9 +685,7 @@ static coroutine_fn int qcow_co_preadv(BlockDriverState 
*bs, uint64_t offset,
 ret = -EIO;
 break;
 }
-hd_iov.iov_base = (void *)buf;
-hd_iov.iov_len = n;
-qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+qemu_iovec_init_buf(&hd_qiov, buf, n);
 qemu_co_mutex_unlock(&s->lock);
 BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
 ret = bdrv_co_preadv(bs->file, cluster_offset + offset_in_cluster,
@@ -733,7 +728,6 @@ static coroutine_fn int qcow_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 int offset_in_cluster;
 uint64_t cluster_offset;
 int ret = 0, n;
-struct iovec hd_iov;
 QEMUIOVector hd_qiov;
 uint8_t *buf;
 void *orig_buf;
@@ -779,9 +773,7 @@ static coroutine_fn int qcow_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 }
 }
 
-hd_iov.iov_base = (void *)buf;
-hd_iov.iov_len = n;
-qemu_iovec_init_external(&hd_qiov, &hd_iov, 1);
+qemu_iovec_init_buf(&hd_qiov, buf, n);
 qemu_co_mutex_unlock(&s->lock);
 BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
 ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster,
@@ -1062,7 +1054,6 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t 
offset,
 {
 BDRVQcowState *s = bs->opaque;
 QEMUIOVector hd_qiov;
-struct iovec iov;
 z_stream strm;
 int ret, out_len;
 uint8_t *buf, *out_buf;
@@ -1128,11 +1119,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, 
uint64_t offset,
 }
 cluster_offset &= s->cluster_offset_mask;
 
-iov = (struct iovec) {
-.iov_base   = out_buf,
-.iov_len= out_len,
-};
-qemu_iovec_init_external(&hd_qiov, &iov, 1);
+qemu_iovec_init_buf(&hd_qiov, out_buf, out_len);
 BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
 ret = bdrv_co_pwritev(bs->file, cluster_offset, out_len, &hd_qiov, 0);
 if (ret < 0) {
-- 
2.20.1

[Qemu-block] [PULL 11/27] block/vmdk: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-12-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-12-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/vmdk.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 096e8eb662..41048741cd 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1371,7 +1371,6 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t 
cluster_offset,
 VmdkGrainMarker *data = NULL;
 uLongf buf_len;
 QEMUIOVector local_qiov;
-struct iovec iov;
 int64_t write_offset;
 int64_t write_end_sector;
 
@@ -1399,11 +1398,7 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t 
cluster_offset,
 data->size = cpu_to_le32(buf_len);
 
 n_bytes = buf_len + sizeof(VmdkGrainMarker);
-iov = (struct iovec) {
-.iov_base   = data,
-.iov_len= n_bytes,
-};
-qemu_iovec_init_external(&local_qiov, &iov, 1);
+qemu_iovec_init_buf(&local_qiov, data, n_bytes);
 
 BLKDBG_EVENT(extent->file, BLKDBG_WRITE_COMPRESSED);
 } else {
-- 
2.20.1

[Qemu-block] [PULL 00/27] Block patches

The following changes since commit fc3dbb90f2eb069801bfb4cfe9cbc83cf9c5f4a9:

  Merge remote-tracking branch 'remotes/jnsnow/tags/bitmaps-pull-request' into 
staging (2019-02-21 13:09:33 +)

are available in the Git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 9a9f4b74fa547b68edb38fa414999836770a4735:

  tests/virtio-blk: add test for DISCARD command (2019-02-22 09:42:17 +)


Pull request



Stefano Garzarella (10):
  virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()
  virtio-blk: add host_features field in VirtIOBlock
  virtio-blk: add "discard" and "write-zeroes" properties
  virtio-net: make VirtIOFeature usable for other virtio devices
  virtio-blk: set config size depending on the features enabled
  virtio-blk: add DISCARD and WRITE_ZEROES features
  tests/virtio-blk: change assert on data_size in virtio_blk_request()
  tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function
  tests/virtio-blk: add test for WRITE_ZEROES command
  tests/virtio-blk: add test for DISCARD command

Vladimir Sementsov-Ogievskiy (17):
  block: enhance QEMUIOVector structure
  block/io: use qemu_iovec_init_buf
  block/block-backend: use QEMU_IOVEC_INIT_BUF
  block/backup: use qemu_iovec_init_buf
  block/commit: use QEMU_IOVEC_INIT_BUF
  block/stream: use QEMU_IOVEC_INIT_BUF
  block/parallels: use QEMU_IOVEC_INIT_BUF
  block/qcow: use qemu_iovec_init_buf
  block/qcow2: use qemu_iovec_init_buf
  block/qed: use qemu_iovec_init_buf
  block/vmdk: use qemu_iovec_init_buf
  qemu-img: use qemu_iovec_init_buf
  migration/block: use qemu_iovec_init_buf
  tests/test-bdrv-drain: use QEMU_IOVEC_INIT_BUF
  hw/ide: drop iov field from IDEState
  hw/ide: drop iov field from IDEBufferedRequest
  hw/ide: drop iov field from IDEDMA

 include/hw/ide/internal.h  |   3 -
 include/hw/virtio/virtio-blk.h |   6 +-
 include/hw/virtio/virtio.h |  15 ++
 include/qemu/iov.h |  64 -
 block/backup.c |   5 +-
 block/block-backend.c  |  13 +-
 block/commit.c |   7 +-
 block/io.c |  89 +++-
 block/parallels.c  |  13 +-
 block/qcow.c   |  21 +--
 block/qcow2.c  |  12 +-
 block/qed-table.c  |  16 +--
 block/qed.c|  31 ++---
 block/stream.c |   7 +-
 block/vmdk.c   |   7 +-
 hw/block/virtio-blk.c  | 245 ++---
 hw/core/machine.c  |   2 +
 hw/ide/atapi.c |  14 +-
 hw/ide/core.c  |  19 ++-
 hw/net/virtio-net.c|  31 +
 hw/virtio/virtio.c |  15 ++
 migration/block.c  |  10 +-
 qemu-img.c |  10 +-
 tests/test-bdrv-drain.c|  29 +---
 tests/virtio-blk-test.c| 127 -
 25 files changed, 525 insertions(+), 286 deletions(-)

-- 
2.20.1

[Qemu-block] [PULL 03/27] block/block-backend: use QEMU_IOVEC_INIT_BUF

From: Vladimir Sementsov-Ogievskiy 

Use new QEMU_IOVEC_INIT_BUF() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-4-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-4-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/block-backend.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f6ea824308..6cc25569ef 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1204,17 +1204,8 @@ static int blk_prw(BlockBackend *blk, int64_t offset, 
uint8_t *buf,
int64_t bytes, CoroutineEntry co_entry,
BdrvRequestFlags flags)
 {
-QEMUIOVector qiov;
-struct iovec iov;
-BlkRwCo rwco;
-
-iov = (struct iovec) {
-.iov_base = buf,
-.iov_len = bytes,
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
-
-rwco = (BlkRwCo) {
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
+BlkRwCo rwco = {
 .blk= blk,
 .offset = offset,
 .iobuf  = &qiov,
-- 
2.20.1

[Qemu-block] [PULL 01/27] block: enhance QEMUIOVector structure

From: Vladimir Sementsov-Ogievskiy 

Add a possibility of embedded iovec, for cases when we need only one
local iov.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Message-id: 20190218140926.333779-2-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-2-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 include/qemu/iov.h | 64 --
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 5f433c7768..48b45987b7 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -133,10 +133,70 @@ size_t iov_discard_back(struct iovec *iov, unsigned int 
*iov_cnt,
 typedef struct QEMUIOVector {
 struct iovec *iov;
 int niov;
-int nalloc;
-size_t size;
+
+/*
+ * For external @iov (qemu_iovec_init_external()) or allocated @iov
+ * (qemu_iovec_init()), @size is the cumulative size of iovecs and
+ * @local_iov is invalid and unused.
+ *
+ * For embedded @iov (QEMU_IOVEC_INIT_BUF() or qemu_iovec_init_buf()),
+ * @iov is equal to &@local_iov, and @size is valid, as it has same
+ * offset and type as @local_iov.iov_len, which is guaranteed by
+ * static assertion below.
+ *
+ * @nalloc is always valid and is -1 both for embedded and external
+ * cases. It is included in the union only to ensure the padding prior
+ * to the @size field will not result in a 0-length array.
+ */
+union {
+struct {
+int nalloc;
+struct iovec local_iov;
+};
+struct {
+char __pad[sizeof(int) + offsetof(struct iovec, iov_len)];
+size_t size;
+};
+};
 } QEMUIOVector;
 
+QEMU_BUILD_BUG_ON(offsetof(QEMUIOVector, size) !=
+  offsetof(QEMUIOVector, local_iov.iov_len));
+
+#define QEMU_IOVEC_INIT_BUF(self, buf, len)  \
+{\
+.iov = &(self).local_iov,\
+.niov = 1,   \
+.nalloc = -1,\
+.local_iov = {   \
+.iov_base = (void *)(buf), /* cast away const */ \
+.iov_len = (len),\
+},   \
+}
+
+/*
+ * qemu_iovec_init_buf
+ *
+ * Initialize embedded QEMUIOVector.
+ *
+ * Note: "const" is used over @buf pointer to make it simple to pass
+ * const pointers, appearing in read functions. Then this "const" is
+ * cast away by QEMU_IOVEC_INIT_BUF().
+ */
+static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
+   const void *buf, size_t len)
+{
+*qiov = (QEMUIOVector) QEMU_IOVEC_INIT_BUF(*qiov, buf, len);
+}
+
+static inline void *qemu_iovec_buf(QEMUIOVector *qiov)
+{
+/* Only supports embedded iov */
+assert(qiov->nalloc == -1 && qiov->iov == &qiov->local_iov);
+
+return qiov->local_iov.iov_base;
+}
+
 void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
 void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
 void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
-- 
2.20.1

[Qemu-block] [PULL 07/27] block/parallels: use QEMU_IOVEC_INIT_BUF

From: Vladimir Sementsov-Ogievskiy 

Use new QEMU_IOVEC_INIT_BUF() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-8-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-8-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/parallels.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index cc9445879d..15bc97b759 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -220,23 +220,20 @@ static int64_t allocate_clusters(BlockDriverState *bs, 
int64_t sector_num,
 if (bs->backing) {
 int64_t nb_cow_sectors = to_allocate * s->tracks;
 int64_t nb_cow_bytes = nb_cow_sectors << BDRV_SECTOR_BITS;
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_len = nb_cow_bytes,
-.iov_base = qemu_blockalign(bs, nb_cow_bytes)
-};
-qemu_iovec_init_external(&qiov, &iov, 1);
+QEMUIOVector qiov =
+QEMU_IOVEC_INIT_BUF(qiov, qemu_blockalign(bs, nb_cow_bytes),
+nb_cow_bytes);
 
 ret = bdrv_co_preadv(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE,
  nb_cow_bytes, &qiov, 0);
 if (ret < 0) {
-qemu_vfree(iov.iov_base);
+qemu_vfree(qemu_iovec_buf(&qiov));
 return ret;
 }
 
 ret = bdrv_co_pwritev(bs->file, s->data_end * BDRV_SECTOR_SIZE,
   nb_cow_bytes, &qiov, 0);
-qemu_vfree(iov.iov_base);
+qemu_vfree(qemu_iovec_buf(&qiov));
 if (ret < 0) {
 return ret;
 }
-- 
2.20.1

[Qemu-block] [PULL 06/27] block/stream: use QEMU_IOVEC_INIT_BUF

From: Vladimir Sementsov-Ogievskiy 

Use new QEMU_IOVEC_INIT_BUF() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-7-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-7-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/stream.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 7a49ac0992..e14579ff80 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -41,14 +41,9 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
 int64_t offset, uint64_t bytes,
 void *buf)
 {
-struct iovec iov = {
-.iov_base = buf,
-.iov_len  = bytes,
-};
-QEMUIOVector qiov;
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
 assert(bytes < SIZE_MAX);
-qemu_iovec_init_external(&qiov, &iov, 1);
 
 /* Copy-on-read the unallocated clusters */
 return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
-- 
2.20.1

[Qemu-block] [PULL 05/27] block/commit: use QEMU_IOVEC_INIT_BUF

From: Vladimir Sementsov-Ogievskiy 

Use new QEMU_IOVEC_INIT_BUF() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-6-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-6-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/commit.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 53148e610b..d500a93068 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -47,14 +47,9 @@ static int coroutine_fn commit_populate(BlockBackend *bs, 
BlockBackend *base,
 void *buf)
 {
 int ret = 0;
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = buf,
-.iov_len = bytes,
-};
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
 assert(bytes < SIZE_MAX);
-qemu_iovec_init_external(&qiov, &iov, 1);
 
 ret = blk_co_preadv(bs, offset, qiov.size, &qiov, 0);
 if (ret < 0) {
-- 
2.20.1

[Qemu-block] [PULL 04/27] block/backup: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-5-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-5-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/backup.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 435414e964..9988753249 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -107,7 +107,6 @@ static int coroutine_fn 
backup_cow_with_bounce_buffer(BackupBlockJob *job,
   void **bounce_buffer)
 {
 int ret;
-struct iovec iov;
 QEMUIOVector qiov;
 BlockBackend *blk = job->common.blk;
 int nbytes;
@@ -119,9 +118,7 @@ static int coroutine_fn 
backup_cow_with_bounce_buffer(BackupBlockJob *job,
 if (!*bounce_buffer) {
 *bounce_buffer = blk_blockalign(blk, job->cluster_size);
 }
-iov.iov_base = *bounce_buffer;
-iov.iov_len = nbytes;
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, *bounce_buffer, nbytes);
 
 ret = blk_co_preadv(blk, start, qiov.size, &qiov, read_flags);
 if (ret < 0) {
-- 
2.20.1

[Qemu-block] [PULL 02/27] block/io: use qemu_iovec_init_buf

From: Vladimir Sementsov-Ogievskiy 

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

While being here, use qemu_try_blockalign0 as well.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Message-id: 20190218140926.333779-3-vsement...@virtuozzo.com
Message-Id: <20190218140926.333779-3-vsement...@virtuozzo.com>
Signed-off-by: Stefan Hajnoczi 
---
 block/io.c | 89 --
 1 file changed, 20 insertions(+), 69 deletions(-)

diff --git a/block/io.c b/block/io.c
index 213ca03d8d..2ba603c7bc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -843,17 +843,13 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
 static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
   int nb_sectors, bool is_write, BdrvRequestFlags flags)
 {
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = (void *)buf,
-.iov_len = nb_sectors * BDRV_SECTOR_SIZE,
-};
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
+nb_sectors * BDRV_SECTOR_SIZE);
 
 if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
 return -EINVAL;
 }
 
-qemu_iovec_init_external(&qiov, &iov, 1);
 return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
 &qiov, is_write, flags);
 }
@@ -880,13 +876,8 @@ int bdrv_write(BdrvChild *child, int64_t sector_num,
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
 {
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = NULL,
-.iov_len = bytes,
-};
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
 
-qemu_iovec_init_external(&qiov, &iov, 1);
 return bdrv_prwv_co(child, offset, &qiov, true,
 BDRV_REQ_ZERO_WRITE | flags);
 }
@@ -950,17 +941,12 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, 
QEMUIOVector *qiov)
 
 int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
 {
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base = (void *)buf,
-.iov_len = bytes,
-};
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
 if (bytes < 0) {
 return -EINVAL;
 }
 
-qemu_iovec_init_external(&qiov, &iov, 1);
 return bdrv_preadv(child, offset, &qiov);
 }
 
@@ -978,17 +964,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, 
QEMUIOVector *qiov)
 
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
 {
-QEMUIOVector qiov;
-struct iovec iov = {
-.iov_base   = (void *) buf,
-.iov_len= bytes,
-};
+QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
 
 if (bytes < 0) {
 return -EINVAL;
 }
 
-qemu_iovec_init_external(&qiov, &iov, 1);
 return bdrv_pwritev(child, offset, &qiov);
 }
 
@@ -1165,7 +1146,6 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
 void *bounce_buffer;
 
 BlockDriver *drv = bs->drv;
-struct iovec iov;
 QEMUIOVector local_qiov;
 int64_t cluster_offset;
 int64_t cluster_bytes;
@@ -1230,9 +1210,8 @@ static int coroutine_fn 
bdrv_co_do_copy_on_readv(BdrvChild *child,
 
 if (ret <= 0) {
 /* Must copy-on-read; use the bounce buffer */
-iov.iov_base = bounce_buffer;
-iov.iov_len = pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
-qemu_iovec_init_external(&local_qiov, &iov, 1);
+pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
+qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
 
 ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
  &local_qiov, 0);
@@ -1477,7 +1456,7 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
 {
 BlockDriver *drv = bs->drv;
 QEMUIOVector qiov;
-struct iovec iov = {0};
+void *buf = NULL;
 int ret = 0;
 bool need_flush = false;
 int head = 0;
@@ -1547,16 +1526,14 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
 need_flush = true;
 }
 num = MIN(num, max_transfer);
-iov.iov_len = num;
-if (iov.iov_base == NULL) {
-iov.iov_base = qemu_try_blockalign(bs, num);
-if (iov.iov_base == NULL) {
+if (buf == NULL) {
+buf = qemu_try_blockalign0(bs, num);
+if (buf == NULL) {
 ret = -ENOMEM;
 goto fail;
 }
-memset(iov.iov_base, 0, num);
 }
-qemu_iovec_init_external(&qiov, &iov, 1);
+qemu_iovec_init_buf(&qiov, buf, num);
 
 ret = bdrv_driver_pwritev(bs, offset, num, &qiov, write_flags);
 
@@ -1564,8 +1541,8 @@

Re: [Qemu-block] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2

On 19.02.19 10:17, Kevin Wolf wrote:
> Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  qapi/block-core.json | 1 +
>>>  block/qcow2.c| 6 +-
>>>  2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 4959bf16a4..e3427f9fcd 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1459,7 +1459,9 @@ static int coroutine_fn 
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>  if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>>>  s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>>> &child_file, false, &local_err);
>>> -if (!s->data_file) {
>>> +if (s->data_file) {
>>> +s->image_data_file = g_strdup(s->data_file->bs->filename);
>>> +} else {
>>>  if (s->image_data_file) {
>>>  error_free(local_err);
>>>  local_err = NULL;
>>
>> Ah, this is what I looked for in the last patch. :-)
>>
>> (i.e. it should be in the last patch, not here)
> 
> [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
> 
> This is the last patch. :-P

Sorry, I meant the previous one.

>> I think as it is it is just wrong, though.  If I pass enough options at
>> runtime, this will overwrite the image header:
>>
>> $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
>> $ ./qemu-img create -f raw bar.raw 64M
>> $ ./qemu-img info foo.qcow2
>> [...]
>> data file: foo.raw
>> [...]
>> $ ./qemu-io --image-opts \
>> file.filename=foo.qcow2,data-file.driver=file,\
>> data-file.filename=bar.raw,lazy-refcounts=on \
>> -c 'write 0 64k'
>> # (The lazy-refcounts is so the image header is updated)
>> $ ./qemu-img info foo.qcow2
>> [...]
>> data file: bar.raw
>> [...]
>>
>> The right thing would probably to check whether the header extension
>> exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
>> is NULL), s->image_data_file gets set; because there are no valid images
>> with the external data file flag set where there is no such header
>> extension.  So we must be in the process of creating the image right now.
>>
>> But even then, I don't quite like setting it here and not creating the
>> header extension as part of qcow2_co_create().  I can see why you've
>> done it this way, but creating a "bad" image on purpose (one with the
>> external data file bit set, but no such header extension present yet) in
>> order to detect and rectify this case when it is first opened (and the
>> opening code assuming that any such broken image must be one that is
>> opened the first time) is a bit weird.
> 
> It's not really a bad image, just one that's a bit cumbersome to use
> because you need to specify the 'data-file' option manually.

Of course it's bad because it doesn't adhere to the specification (which
you could amend, of course, since you add it with this series).  The
spec says "If this bit is set, an external data file name header
extension must be present as well."  Which it isn't until the image is
opened with the data-file option.

>> I suppose doing it right (if you agree with the paragraph before the
>> last one) and adding a comment would make it less weird
>> ("s->image_data_file must be non-NULL for any valid image, so this image
>> must be one we are creating right now" or something like that).
>>
>> But still, the issue you point out in your cover letter remains; which
>> is that the node's filename and the filename given by the user may be
>> two different things.
> 
> I think what I was planning to do was leaving the path from the image
> header in s->image_data_file even when a runtime option overrides it.
> After all, ImageInfo is about the image, not about the runtime state.

I'm not talking about ImageInfo here, though, I'm talking about the
image creation process.  The hunk I've quoted should be in the previous
patch, not in this one.

Which doesn't make wrong what you're saying, though, the ImageInfo
should print what's in the header.

> Image creation would just manually set s->image_data_file before
> updating the header.

It should, but currently it does that rather indirectly (by setting the
data-file option which then makes qcow2_do_open() copy it into
s->image_data_file).

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] Configuring pflash devices for OVMF firmware

2019-02-22 Thread Markus Armbruster

The other day, I described how my attempt to implement Paolo's
suggestion to add block properties to the machine ran into difficulties.
To recap briefly, creating devices within a machine's .instance_init()
crashes.  Turns out device_post_init() calls qdev_get_machine(), which
calls container_get() for QOM path "/machine".  Since "/machine" doesn't
exist, yet (we're busy creating it), container_get() "helpfully" creates
it as a new "container" object.  Oww.  Fortunately, we crash soon after,
when we try to create the real "/machine".

I managed to rejigger global property application code to work without
qdev_get_machine().  Good.

Not so good: QEMU still crashes the same way.

Turns out we *also* call qdev_get_machine() on the first
sysbus_get_default().  We create the main system bus then, and
immediately store it in "/machine/unattached/sysbus".  So
container_get() "helpfully" creates first "/machine" and then
"/machine/unattached" for us.

I managed to rejigger that, too.  Now my QEMU survives creating a pflash
device in pc_machine_initfn(), and I can add a machine property
"pflash0" with object_property_add_alias().  Good.

Not so good: attempting to set it with -machine pflash0=pflash0 fails
with "Property 'cfi.pflash01.drive' can't find value 'pflash0'".

Turns out we set the machine's properties around line 4250, some 140
lines before we create block backends.  Block backend "pflash0" doesn't
exist, yet.

This is madness, but at least it's familiar madness; we've rejiggered
the order of creating stuff in main() numerous times already.

To be continued...

[Qemu-block] [PATCH v2] iotests: handle TypeError for Python3 in test 242

2019-02-22 Thread Andrey Shinkevich

The data type for bytes in Python3 differs from the one in Python2.
Those cases should be managed separately.

v1:
In the first version, the TypeError in Python3 was handled as the
exception.
Discussed in the e-mail thread with the Message ID:
<1550519997-253534-1-git-send-email-andrey.shinkev...@virtuozzo.com>

Signed-off-by: Andrey Shinkevich 
Reported-by: Kevin Wolf 
---
 tests/qemu-iotests/242 | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 16c65ed..446fbf8 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -20,6 +20,7 @@
 
 import iotests
 import json
+import sys
 from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
 file_path, img_info_log, log, filter_qemu_io
 
@@ -65,9 +66,12 @@ def toggle_flag(offset):
 with open(disk, "r+b") as f:
 f.seek(offset, 0)
 c = f.read(1)
-toggled = chr(ord(c) ^ bitmap_flag_unknown)
+toggled = ord(c) ^ bitmap_flag_unknown
 f.seek(-1, 1)
-f.write(toggled)
+if sys.version_info.major >= 3:
+f.write(bytes([toggled]))
+else:
+f.write(chr(toggled))
 
 
 qemu_img_create('-f', iotests.imgfmt, disk, '1M')
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-img: implement copy offload (-C) for dd

Am 21.02.2019 um 20:32 hat Sergio Lopez geschrieben:
> On Thu, Feb 21, 2019 at 12:08:12PM -0600, Eric Blake wrote:
> > On 2/21/19 11:37 AM, Sergio Lopez wrote:
> > > This parameter is analogous to convert's "-C", making use of
> > > bdrv_co_copy_range().
> > 
> > The last time I tried to patch 'qemu-img dd', it was pointed out that it
> > already has several bugs (where it is not on feature-parity with real
> > dd), and that we REALLY want to make it a syntactic sugar wrapper around
> > 'qemu-img convert', rather than duplicating code (which means that
> > qemu-img convert needs to make it easier to do arbitrary offsets and
> > subsets - although to some extent you can already do that with
> > --image-opts and appropriate raw driver wrappers).
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg02618.html
> 
> Interesting, I wasn't aware of that conversation. It might a little
> late to go again through it, but while I don't a strong opinion about
> it, I do have some reservations about the idea of making 'dd' a
> frontend for 'convert'.
> 
> While I do see the functional similarity of both commands, to me they
> are quite different at a semantical level. For 'convert', I do expect
> it to do "the right thing" and use the optimal settings (i.e. choosing
> the best transfer size) by default, while 'dd' is more of "do whatever
> the user told you to do no matter how wrong it is".

I think that's what defaults are for. It would make sense to allow the
user to specify the buffer size even for 'qemu-img convert'. In fact, we
already have a variable for this, we'd just have to add an option to set
it explicitly instead of just relying on what the output block node
tells us.

> Due to this differences, I think turning 'convert' code into something
> able to deal with 'dd' semantics would imply adding a considerable
> number of conditionals, possibly making it harder to maintain than
> keeping it separate.

'qemu-img dd' currently supports five options:

* if and of. These are obviously supported for convert, too.
* count and skip. We don't have these in convert yet. We could either
  add the functionality there or add a raw layer in the 'dd'
  implementation before we call into the common code.
* bs. The buffer size is already configurable in ImgConvertState.

So getting this functionality into 'convert' should be easy.

There are more differences between 'convert' and 'dd' in how exactly the
copy is done. I'm not sure whether there is actually a good use for the
dumb 'dd' copying or whether it was just implemented this way because it
was easier.

Currently we have features like copying only a given range only in 'dd',
and most other features like zero detection, dealing with backing files,
compression, copy offloading or parallel out-of-order copy only in
'convert'.

Actually, we have more than just these two places that copy images: We
also have the mirror block job and for other special cases also the
other block jobs that copy data, each with its own list of features and
missing options.

What I really want to see eventually is a way to have all features
available in all of these instead of only a random selection where
you're out of luck if you want to copy part of an image with compressed
output while the VM is running because these three are features
supported in three different copy implementations and you can't get more
than one of the features at the same time.

Kevin

Re: [Qemu-block] [PATCH v6 00/10] virtio-blk: add DISCARD and WRITE_ZEROES features