[Qemu-block] Assertion failure on qcow2 disk with cluster_size != 64k

[Ed Swierk]

Shortly after I start qemu 2.7.0 with a qcow2 disk image created with
-o cluster_size=1048576, it prints the following and dies:

block/qcow2.c:2451: qcow2_co_pwrite_zeroes: Assertion `head + count <=
s->cluster_size' failed.

I narrowed the problem to bdrv_co_do_pwrite_zeroes(), called by
bdrv_aligned_pwritev() with flags & BDRV_REQ_ZERO_WRITE set.

On the first loop iteration, offset=8003584, count=2093056,
head=663552, tail=659456 and num=2093056. qcow2_co_pwrite_zeroes() is
called with offset=8003584 and count=385024 and finds that the head
portion is not already zero, so it returns -ENOTSUP.
bdrv_co_do_pwrite_zeroes() falls back to a normal write, with
max_transfer=65536.

The next iteration starts with offset incremented by 65536, count and
num decremented by 65536, and head=0, violating the assumption that
the entire 385024 bytes of the head remainder had been zeroed the
first time around. Then it calls qcow2_co_pwrite_zeroes() with an
unaligned offset=8069120 and a count=1368064 greater than the cluster
size, triggering the assertion failure.

Changing max_transfer in the normal write case to
MIN_NON_ZERO(alignment, MAX_WRITE_ZEROES_BOUNCE_BUFFER) appears to fix
the problem, but I don't pretend to understand all the subtleties
here.

--Ed

[Qemu-block] [PATCH 4/5] blockjob: add block_job_start

[John Snow]

Instead of automatically starting jobs at creation time via backup_start
et al, we'd like to return a job object pointer that can be started
manually at later point in time.

For now, add the block_job_start mechanism and start the jobs
automatically as we have been doing, with conversions job-by-job coming
in later patches.

Of note: cancellation of unstarted jobs will perform all the normal
cleanup as if the job had started, particularly abort and clean. The
only difference is that we will not emit any events, because the job
never actually started.

Signed-off-by: John Snow 
---
 block/backup.c|  3 +--
 block/commit.c|  3 +--
 block/mirror.c|  3 +--
 block/stream.c|  3 +--
 blockjob.c| 51 ---
 include/block/blockjob.h  |  9 +
 tests/test-blockjob-txn.c | 12 +--
 7 files changed, 58 insertions(+), 26 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 622f64e..2ce5115 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -638,9 +638,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 
 bdrv_op_block_all(target, job->common.blocker);
 job->common.len = len;
-job->common.co = qemu_coroutine_create(job->common.driver->start, job);
 block_job_txn_add_job(txn, >common);
-qemu_coroutine_enter(job->common.co);
+block_job_start(>common);
 return;
 
  error:
diff --git a/block/commit.c b/block/commit.c
index cc2030d..89820d7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -275,10 +275,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 
 trace_commit_start(bs, base, top, s, s->common.co);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
 
 
diff --git a/block/mirror.c b/block/mirror.c
index 3a29b94..8130474 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -970,9 +970,8 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 
 bdrv_op_block_all(target, s->common.blocker);
 
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_mirror_start(bs, s, s->common.co, opaque);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
diff --git a/block/stream.c b/block/stream.c
index 8ffed9c..3e3a7d3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -231,7 +231,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_stream_start(bs, base, s, s->common.co);
-qemu_coroutine_enter(s->common.co);
+block_job_start(>common);
 }
diff --git a/blockjob.c b/blockjob.c
index 150b87e..f574bc8 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -171,7 +171,8 @@ void *block_job_create(const char *job_id, const 
BlockJobDriver *driver,
 job->blk   = blk;
 job->cb= cb;
 job->opaque= opaque;
-job->busy  = true;
+job->busy  = false;
+job->paused= true;
 job->refcnt= 1;
 bs->job = job;
 
@@ -199,6 +200,21 @@ bool block_job_is_internal(BlockJob *job)
 return (job->id == NULL);
 }
 
+static bool block_job_started(BlockJob *job)
+{
+return job->co;
+}
+
+void block_job_start(BlockJob *job)
+{
+assert(job && !block_job_started(job) && job->paused &&
+   !job->busy && job->driver->start);
+job->paused = false;
+job->busy = true;
+job->co = qemu_coroutine_create(job->driver->start, job);
+qemu_coroutine_enter(job->co);
+}
+
 void block_job_ref(BlockJob *job)
 {
 ++job->refcnt;
@@ -239,14 +255,18 @@ static void block_job_completed_single(BlockJob *job)
 if (job->cb) {
 job->cb(job->opaque, job->ret);
 }
-if (block_job_is_cancelled(job)) {
-block_job_event_cancelled(job);
-} else {
-const char *msg = NULL;
-if (job->ret < 0) {
-msg = strerror(-job->ret);
+
+/* Emit events only if we actually started */
+if (block_job_started(job)) {
+if (block_job_is_cancelled(job)) {
+block_job_event_cancelled(job);
+} else {
+const char *msg = NULL;
+if (job->ret < 0) {
+msg = strerror(-job->ret);
+}
+block_job_event_completed(job, msg);
 }
-block_job_event_completed(job, msg);
 }
 
 if (job->txn) {
@@ -354,7 +374,8 @@ void block_job_complete(BlockJob *job, Error **errp)
 {
 /* Should not be reachable via external interface for internal jobs */
 assert(job->id);
-if (job->pause_count || job->cancelled || !job->driver->complete) {
+if 

[Qemu-block] [PATCH 5/5] blockjob: refactor backup_start as backup_job_create

[John Snow]

Refactor backup_start as backup_job_create, which only creates the job,
but does not automatically start it. The old interface, 'backup_start',
is not kept in favor of limiting the number of nearly-identical interfaces
that would have to be edited to keep up with QAPI changes in the future.

Callers that wish to synchronously start the backup_block_job can
instead just call block_job_start immediately after calling
backup_job_create.

Transactions are updated to use the new interface, calling block_job_start
only during the .commit phase, which helps prevent race conditions where
jobs may finish before we even finish building the transaction. This may
happen, for instance, during empty block backup jobs.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 block/backup.c| 26 ---
 block/replication.c   | 12 ---
 blockdev.c| 83 ++-
 include/block/block_int.h | 23 ++---
 4 files changed, 87 insertions(+), 57 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 2ce5115..d7e5c48 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -527,7 +527,7 @@ static const BlockJobDriver backup_job_driver = {
 .attached_aio_context   = backup_attached_aio_context,
 };
 
-void backup_start(const char *job_id, BlockDriverState *bs,
+BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, int64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
   bool compress,
@@ -547,52 +547,52 @@ void backup_start(const char *job_id, BlockDriverState 
*bs,
 
 if (bs == target) {
 error_setg(errp, "Source and target cannot be the same");
-return;
+return NULL;
 }
 
 if (!bdrv_is_inserted(bs)) {
 error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(bs));
-return;
+return NULL;
 }
 
 if (!bdrv_is_inserted(target)) {
 error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(target));
-return;
+return NULL;
 }
 
 if (compress && target->drv->bdrv_co_pwritev_compressed == NULL) {
 error_setg(errp, "Compression is not supported for this drive %s",
bdrv_get_device_name(target));
-return;
+return NULL;
 }
 
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
-return;
+return NULL;
 }
 
 if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
-return;
+return NULL;
 }
 
 if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
 if (!sync_bitmap) {
 error_setg(errp, "must provide a valid bitmap name for "
  "\"incremental\" sync mode");
-return;
+return NULL;
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
 if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
-return;
+return NULL;
 }
 } else if (sync_bitmap) {
 error_setg(errp,
"a sync_bitmap was provided to backup_run, "
"but received an incompatible sync_mode (%s)",
MirrorSyncMode_lookup[sync_mode]);
-return;
+return NULL;
 }
 
 len = bdrv_getlength(bs);
@@ -639,8 +639,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 bdrv_op_block_all(target, job->common.blocker);
 job->common.len = len;
 block_job_txn_add_job(txn, >common);
-block_job_start(>common);
-return;
+
+return >common;
 
  error:
 if (sync_bitmap) {
@@ -650,4 +650,6 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 backup_clean(>common);
 block_job_unref(>common);
 }
+
+return NULL;
 }
diff --git a/block/replication.c b/block/replication.c
index d4f4a7b..ca4a381 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -409,6 +409,7 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 int64_t active_length, hidden_length, disk_length;
 AioContext *aio_context;
 Error *local_err = NULL;
+BlockJob *job;
 
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
@@ -496,17 +497,18 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 bdrv_op_block_all(top_bs, s->blocker);
 bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-backup_start(NULL, s->secondary_disk->bs, s->hidden_disk->bs, 0,
- MIRROR_SYNC_MODE_NONE, NULL, false,
- BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
- BLOCK_JOB_INTERNAL, backup_job_completed, s,
- NULL, 

[Qemu-block] [PATCH 2/5] blockjob: add .clean property

[John Snow]

Cleaning up after we have deferred to the main thread but before the
transaction has converged can be dangerous and result in deadlocks
if the job cleanup invokes any BH polling loops.

A job may attempt to begin cleaning up, but may induce another job to
enter its cleanup routine. The second job, part of our same transaction,
will block waiting for the first job to finish, so neither job may now
make progress.

To rectify this, allow jobs to register a cleanup operation that will
always run regardless of if the job was in a transaction or not, and
if the transaction job group completed successfully or not.

Move sensitive cleanup to this callback instead which is guaranteed to
be run only after the transaction has converged, which removes sensitive
timing constraints from said cleanup.

Furthermore, in future patches these cleanup operations will be performed
regardless of whether or not we actually started the job. Therefore,
cleanup callbacks should essentially confine themselves to undoing create
operations, e.g. setup actions taken in what is now backup_start.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
 block/backup.c   | 13 +
 blockjob.c   |  3 +++
 include/block/blockjob_int.h |  8 
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 6d12100..ed6d74a 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -242,6 +242,13 @@ static void backup_abort(BlockJob *job)
 }
 }
 
+static void backup_clean(BlockJob *job)
+{
+BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+assert(s->target);
+blk_unref(s->target);
+}
+
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common);
@@ -306,6 +313,7 @@ static const BlockJobDriver backup_job_driver = {
 .set_speed  = backup_set_speed,
 .commit = backup_commit,
 .abort  = backup_abort,
+.clean  = backup_clean,
 .attached_aio_context   = backup_attached_aio_context,
 };
 
@@ -327,11 +335,8 @@ typedef struct {
 
 static void backup_complete(BlockJob *job, void *opaque)
 {
-BackupBlockJob *s = container_of(job, BackupBlockJob, common);
 BackupCompleteData *data = opaque;
 
-blk_unref(s->target);
-
 block_job_completed(job, data->ret);
 g_free(data);
 }
@@ -642,7 +647,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
 }
 if (job) {
-blk_unref(job->target);
+backup_clean(>common);
 block_job_unref(>common);
 }
 }
diff --git a/blockjob.c b/blockjob.c
index f55bfec..150b87e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -232,6 +232,9 @@ static void block_job_completed_single(BlockJob *job)
 job->driver->abort(job);
 }
 }
+if (job->driver->clean) {
+job->driver->clean(job);
+}
 
 if (job->cb) {
 job->cb(job->opaque, job->ret);
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 10ebb38..1c4bc90 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -74,6 +74,14 @@ struct BlockJobDriver {
 void (*abort)(BlockJob *job);
 
 /**
+ * If the callback is not NULL, it will be invoked after a call to either
+ * .commit() or .abort(). Regardless of which callback is invoked after
+ * completion, .clean() will always be called, even if the job does not
+ * belong to a transaction group.
+ */
+void (*clean)(BlockJob *job);
+
+/**
  * If the callback is not NULL, it will be invoked when the job transitions
  * into the paused state.  Paused jobs must not perform any asynchronous
  * I/O or event loop activity.  This callback is used to quiesce jobs.
-- 
2.7.4

[Qemu-block] [PATCH 1/5] blockjob: fix dead pointer in txn list

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Though it is not intended to be reached through normal circumstances,
if we do not gracefully deconstruct the transaction QLIST, we may wind
up with stale pointers in the list.

The rest of this series attempts to address the underlying issues,
but this should fix list inconsistencies.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Tested-by: John Snow 
Reviewed-by: John Snow 
[Rewrote commit message. --js]
Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 

Signed-off-by: John Snow 
---
 blockjob.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockjob.c b/blockjob.c
index e1d0382..f55bfec 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -247,6 +247,7 @@ static void block_job_completed_single(BlockJob *job)
 }
 
 if (job->txn) {
+QLIST_REMOVE(job, txn_list);
 block_job_txn_unref(job->txn);
 }
 block_job_unref(job);
-- 
2.7.4

[Qemu-block] [PATCH 3/5] blockjob: add .start field

[John Snow]

Add an explicit start field to specify the entrypoint. We already have
ownership of the coroutine itself AND managing the lifetime of the
coroutine, let's take control of creation of the coroutine, too.

This will allow us to delay creation of the actual coroutine until we
know we'll actually start a BlockJob in block_job_start. This avoids
the sticky question of how to "un-create" a Coroutine that hasn't been
started yet.

Signed-off-by: John Snow 
---
 block/backup.c   | 23 ---
 block/commit.c   |  3 ++-
 block/mirror.c   |  4 +++-
 block/stream.c   |  3 ++-
 include/block/blockjob_int.h |  3 +++
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index ed6d74a..622f64e 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -307,16 +307,6 @@ void backup_cow_request_end(CowRequest *req)
 cow_request_end(req);
 }
 
-static const BlockJobDriver backup_job_driver = {
-.instance_size  = sizeof(BackupBlockJob),
-.job_type   = BLOCK_JOB_TYPE_BACKUP,
-.set_speed  = backup_set_speed,
-.commit = backup_commit,
-.abort  = backup_abort,
-.clean  = backup_clean,
-.attached_aio_context   = backup_attached_aio_context,
-};
-
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
 bool read, int error)
 {
@@ -526,6 +516,17 @@ static void coroutine_fn backup_run(void *opaque)
 block_job_defer_to_main_loop(>common, backup_complete, data);
 }
 
+static const BlockJobDriver backup_job_driver = {
+.instance_size  = sizeof(BackupBlockJob),
+.job_type   = BLOCK_JOB_TYPE_BACKUP,
+.start  = backup_run,
+.set_speed  = backup_set_speed,
+.commit = backup_commit,
+.abort  = backup_abort,
+.clean  = backup_clean,
+.attached_aio_context   = backup_attached_aio_context,
+};
+
 void backup_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *target, int64_t speed,
   MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
@@ -637,7 +638,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
 
 bdrv_op_block_all(target, job->common.blocker);
 job->common.len = len;
-job->common.co = qemu_coroutine_create(backup_run, job);
+job->common.co = qemu_coroutine_create(job->common.driver->start, job);
 block_job_txn_add_job(txn, >common);
 qemu_coroutine_enter(job->common.co);
 return;
diff --git a/block/commit.c b/block/commit.c
index d555600..cc2030d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -205,6 +205,7 @@ static const BlockJobDriver commit_job_driver = {
 .instance_size = sizeof(CommitBlockJob),
 .job_type  = BLOCK_JOB_TYPE_COMMIT,
 .set_speed = commit_set_speed,
+.start = commit_run,
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
@@ -274,7 +275,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 s->backing_file_str = g_strdup(backing_file_str);
 
 s->on_error = on_error;
-s->common.co = qemu_coroutine_create(commit_run, s);
+s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 
 trace_commit_start(bs, base, top, s, s->common.co);
 qemu_coroutine_enter(s->common.co);
diff --git a/block/mirror.c b/block/mirror.c
index c81b5e0..3a29b94 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -891,6 +891,7 @@ static const BlockJobDriver mirror_job_driver = {
 .instance_size  = sizeof(MirrorBlockJob),
 .job_type   = BLOCK_JOB_TYPE_MIRROR,
 .set_speed  = mirror_set_speed,
+.start  = mirror_run,
 .complete   = mirror_complete,
 .pause  = mirror_pause,
 .attached_aio_context   = mirror_attached_aio_context,
@@ -900,6 +901,7 @@ static const BlockJobDriver commit_active_job_driver = {
 .instance_size  = sizeof(MirrorBlockJob),
 .job_type   = BLOCK_JOB_TYPE_COMMIT,
 .set_speed  = mirror_set_speed,
+.start  = mirror_run,
 .complete   = mirror_complete,
 .pause  = mirror_pause,
 .attached_aio_context   = mirror_attached_aio_context,
@@ -968,7 +970,7 @@ static void mirror_start_job(const char *job_id, 
BlockDriverState *bs,
 
 bdrv_op_block_all(target, s->common.blocker);
 
-s->common.co = qemu_coroutine_create(mirror_run, s);
+s->common.co = qemu_coroutine_create(s->common.driver->start, s);
 trace_mirror_start(bs, s, s->common.co, opaque);
 qemu_coroutine_enter(s->common.co);
 }
diff --git a/block/stream.c b/block/stream.c
index 906f7f3..8ffed9c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -212,6 +212,7 @@ 

[Qemu-block] [PATCH 0/5] jobs: fix transactional race condition

[John Snow]

Requires: [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1

There are a few problems with transactional job completion right now.

First, if jobs complete so quickly they complete before remaining jobs
get a chance to join the transaction, the completion mode can leave well
known state and the QLIST can get corrupted and the transactional jobs
can complete in batches or phases instead of all together.

Second, if two or more jobs defer to the main loop at roughly the same
time, it's possible for one job's cleanup to directly invoke the other
job's cleanup from within the same thread, leading to a situation that
will deadlock the entire transaction.

Thanks to Vladimir for pointing out these modes of failure.

I have omitted the test for right now, but wanted to air the patches on-list.
It makes no attempt to change the locking mechanisms around qmp_transaction
right now, asserting instead that things are no more broken than they were,
especially in the case of dataplane. I will make further attempts to clarify
the locking mechanisms around qmp_transaction after Paolo's changes go in.



For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch job-fix-race-condition
https://github.com/jnsnow/qemu/tree/job-fix-race-condition

This version is tagged job-fix-race-condition-v1:
https://github.com/jnsnow/qemu/releases/tag/job-fix-race-condition-v1

John Snow (4):
  blockjob: add .clean property
  blockjob: add .start field
  blockjob: add block_job_start
  blockjob: refactor backup_start as backup_job_create

Vladimir Sementsov-Ogievskiy (1):
  blockjob: fix dead pointer in txn list

 block/backup.c   | 59 +--
 block/commit.c   |  4 +--
 block/mirror.c   |  5 +--
 block/replication.c  | 12 ---
 block/stream.c   |  4 +--
 blockdev.c   | 83 
 blockjob.c   | 55 ++---
 include/block/block_int.h| 23 ++--
 include/block/blockjob.h |  9 +
 include/block/blockjob_int.h | 11 ++
 tests/test-blockjob-txn.c| 12 +++
 11 files changed, 182 insertions(+), 95 deletions(-)

-- 
2.7.4

Re: [Qemu-block] [PATCH v4 0/4] fdc: Use separate qdev device for drives

[John Snow]

On 10/20/2016 03:55 AM, Kevin Wolf wrote:

We have been complaining for a long time about how the floppy controller and
floppy drives are combined in a single qdev device and how this makes the
device awkward to work with because it behaves different from all other block
devices.

The latest reason to complain was when I noticed that using qdev device names
in QMP commands (e.g. for media change) doesn't really work when only the
controller is a qdev device, but the drives aren't.

So I decided to have a go at it, and this is the result.

It doesn't actually change any of the inner workings of the floppy controller,
but it wires things up differently on the qdev layer so that a floppy
controller now exposes a bus on which the floppy drives sit. This results in a
structure that is similar to IDE where the actual drive state is still in the
controller and the qdev device basically just contains the qdev properties -
not pretty, but quite workable.

The commit message of patch 3 explains how to use it. In short, there is a
'-device floppy' now and it does what you would expect if you ever used ide-cd.

The other problem is old command lines, especially those using things like
'-global isa-fdc,driveA=...'. In order to keep them working, we need to forward
the property to an internally created floppy drive device. This is a bit like
usb-storage, which we know is ugly, but works well enough in practice. The good
news here is that in contrast to usb-storage, the floppy controller only does
the forwarding for legacy configurations; as soon as you start using '-device
floppy', it doesn't happen any more.

So as you may have expected, this conversion doesn't result in a perfect
device, but I think it's definitely an improvement over the old state. I hope
you like it despite the warts. :-)

v4:
- John says that his grep is broken and hangs at 100% CPU with my attempt to
  extract the floppy controller from info qtree. Use a simpler sed command
  instead (which, unlike the grep command, doesn't handle arbitrary indentation
  level of the next item, but we know what comes next, so just hardcoding 10
  spaces works, too).

v3:
- Fixed omissons in the conversion sysbus-fdc and the Sun one. Nice, combining
  floppy controllers and weird platforms in a single series. [John]

v2:
- Added patch 4 (qemu-iotests case for floppy config on the command line)
- Patch 2: Create a floppy device only if a BlockBackend exists instead of
  always creating two of them
- Patch 2: Initialise drive->fdctrl even if no drive is attached, it is
  accessed anyway during migration
- Patch 3: Keep 'type' qdev property and FDrive->drive in sync
- Patch 3: Removed if with condition that is always true


Kevin Wolf (4):
  fdc: Add a floppy qbus
  fdc: Add a floppy drive qdev
  fdc: Move qdev properties to FloppyDrive
  qemu-iotests: Test creating floppy drives

 hw/block/fdc.c |  271 --
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1170 
 tests/qemu-iotests/group   |1 +
 vl.c   |1 +
 5 files changed, 1637 insertions(+), 48 deletions(-)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out



You're going to kill me, but...

the qemu-name scrubbing isn't working quite right:

-qemu: -device floppy,drive=none0,physical_block_size=1024: Physical and 
logical block size must be 512 for floppy
-qemu: -device floppy,drive=none0,physical_block_size=1024: Device 
initialization failed.
+qemu-system-x86_64: -device 
floppy,drive=none0,physical_block_size=1024: Physical and logical block 
size must be 512 for floppy
+qemu-system-x86_64: -device 
floppy,drive=none0,physical_block_size=1024: Device initialization failed.


I'm building from e.g. ~/src/qemu/bin/git and doing ../../configure 
--etc; and then running tests from ~/s/q/b/g/tests/qemu-iotests.


and to Konrad's point, is it okay to have both a "Floppy" device and a 
"floppy" device? One is a bus and the other is not, but maybe "floppy" 
and "floppy-drive" or "floppy-device" or something less accidentally 
ambiguous.


As to the proper casing of these objects... IDE as you've stated uses 
"IDE" for the bus and "ide-device" for the abstract device, "ide-drive," 
"ide-hd" and "ide-cd" for instances,  "pci-ide" and "ich9-ahci" for the 
HBAs.


Lowercase is probably okay for both the bus and the device -- looks like 
very few type names have caps. (German grammar be damned.)


Happy Friday? :(

Re: [Qemu-block] [Qemu-devel] [PATCH 7/9] block: Introduce .bdrv_co_ioctl() driver callback

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> This allows drivers to implement ioctls in a coroutine-based way.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 16 ++--
>  include/block/block_int.h |  2 ++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 8/9] raw: Implement .bdrv_co_ioctl instead of .bdrv_aio_ioctl

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> It's the simpler interface to use for the raw format driver.
> 
> Apart from that, this removes the last user of the AIO emulation
> implemented by bdrv_aio_ioctl().
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/raw_bsd.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 9/9] block: Remove bdrv_aio_ioctl()

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> It is unused now.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 27 ---
>  include/block/block.h |  3 ---
>  2 files changed, 30 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 6/9] block: Remove bdrv_ioctl()

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> It is unused now.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 37 -
>  include/block/block.h |  1 -
>  2 files changed, 38 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 5/9] raw-posix: Don't use bdrv_ioctl()

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> Instead of letting raw-posix use the bdrv_ioctl() abstraction to issue
> an ioctl to itself, just call ioctl() directly.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/raw-posix.c | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] block: Use blk_co_ioctl() for all BB level ioctls

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> All read/write functions already have a single coroutine-based function
> on the BlockBackend level through which all requests go (no matter what
> API style the external caller used) and which passes the requests down
> to the block node level.
> 
> This patch exports a bdrv_co_ioctl() function and uses it to extend this
> mode of operation to ioctls.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c  | 39 +--
>  block/io.c |  8 
>  include/block/block.h  |  1 +
>  include/sysemu/block-backend.h |  1 +
>  4 files changed, 39 insertions(+), 10 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] block: Remove bdrv_aio_pdiscard()

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> It is unused now.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/io.c| 29 -
>  block/trace-events|  1 -
>  include/block/block.h |  3 ---
>  3 files changed, 33 deletions(-)

Might be nice to research which commit id removed the last use, and
mention it in the commit message as an idea for how long we've had dead
code, but that's not a necessity.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] ssh: switch from libssh2 to libssh

[Pino Toscano]

On Thursday, 20 October 2016 18:04:34 CEST Stefan Weil wrote:
> Am 20.10.2016 um 17:15 schrieb Pino Toscano:
> > Rewrite the implementation of the ssh block driver to use libssh instead
> > of libssh.  The libssh library has various advantages over libssh:
> 
> libssh instead of libssh? In both sentences a "2" is missing.

Right, they should be "... instead of libssh2" and "advantages over
libssh2" -- fixed locally.

> Cygwin does not provide libssh for Mingw-w64, see
> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=libssh=x86_64

I guess I could ask them for these versions once this patch is approved
(so there's an existing use case).

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

Re: [Qemu-block] [Qemu-devel] [PATCH 2/9] block: Use blk_co_pdiscard() for all BB level discard

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> All read/write functions already have a single coroutine-based function
> on the BlockBackend level through which all requests go (no matter what
> API style the external caller used) and which passes the requests down
> to the block node level.
> 
> This patch extends this mode of operation to discards.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c | 30 ++
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] block: Use blk_co_flush() for all BB level flushes

[Eric Blake]

On 10/20/2016 08:46 AM, Kevin Wolf wrote:
> All read/write functions already have a single coroutine-based function
> on the BlockBackend level through which all requests go (no matter what
> API style the external caller used) and which passes the requests down
> to the block node level.
> 
> This patch extends this mode of operation to flushes.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c | 27 +--
>  1 file changed, 17 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] ssh: switch from libssh2 to libssh

[Richard W.M. Jones]

On Thu, Oct 20, 2016 at 05:44:41PM +0200, Pino Toscano wrote:
> On Thursday, 20 October 2016 16:32:50 CEST Richard W.M. Jones wrote:
> > On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote:
> > > Rewrite the implementation of the ssh block driver to use libssh instead
> > > of libssh.  The libssh library has various advantages over libssh:
> > > - easier API for authentication (for example for using ssh-agent)
> > > - easier API for known_hosts handling
> > > - supports newer types of keys in known_hosts
> > > 
> > > Kerberos authentication can be enabled once the libssh bug for it [1] is
> > > fixed.
> > > 
> > > The development version of libssh (i.e. the future 0.8.x) supports
> > > fsync, so reuse the build time check for this.
> > > 
> > > [1] https://red.libssh.org/issues/242
> > > 
> > > Signed-off-by: Pino Toscano 
> > 
> > 
> > When I applied this patch and compiled it with warnings enabled:
> > 
> > block/ssh.c: In function ‘connect_to_ssh’:
> > block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this 
> > function [-Wmaybe-uninitialized]
> >  return ret;
> > ^~~
> 
> Interesting, there was no warning for me.  Anyway, I think this:
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 7c316db..7ff376e 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -519,6 +519,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>  /* Create SSH session. */
>  s->session = ssh_new();
>  if (!s->session) {
> +ret = -EINVAL;
>  goto err;
>  }
>  
> should fix it (added already locally).

Yes, this fixes the warning.

> > To test the patch, I used virt-builder to create a virtual machine
> > disk image on another machine (accessible over ssh).  Then from my
> > laptop I ran:
> > 
> >   ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \
> >   -M accel=kvm -cpu host -m 2048 \
> >   -drive 
> > file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio
> >  \
> >   -serial stdio
> > 
> > Unfortunately this failed with a large number of sftp errors:
> > 
> >   read failed: (sftp error code: 0)
> > 
> > and subsequently hung.  So I'm afraid I couldn't test the patch at all :-(
> 
> Can you please enable the logging of the ssh driver, and libssh own
> logging too?  Basically (see lines 45-46) set:
> 
> #define DEBUG_SSH 1
> #define TRACE_LIBSSH  4

I'll send you the full trace privately since it's enormous.

Rich.

> > Also fsync was not supported for me, but I'm using 0.7.3 and the code
> > says I need 0.8.0.
> 
> Yes, this is correct.
> 
> Thanks,
> -- 
> Pino Toscano



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-block] [Qemu-devel] [PATCH] ssh: switch from libssh2 to libssh

[Stefan Weil]

Am 20.10.2016 um 17:15 schrieb Pino Toscano:
> Rewrite the implementation of the ssh block driver to use libssh instead
> of libssh.  The libssh library has various advantages over libssh:

libssh instead of libssh? In both sentences a "2" is missing.

Cygwin does not provide libssh for Mingw-w64, see
https://cygwin.com/cgi-bin2/package-grep.cgi?grep=libssh=x86_64

Stefan

Re: [Qemu-block] [Qemu-devel] [PATCH RFC 7/7] nbd/replication: implement .bdrv_get_info() for nbd and replication driver

[Eric Blake]

On 10/20/2016 08:57 AM, zhanghailiang wrote:
> Without this callback, there will be an error reports in the primary side:
> "qemu-system-x86_64: Couldn't determine the cluster size of the target image,
> which has no backing file: Operation not supported
> Aborting, since this may create an unusable destination image"
> 
> For nbd driver, it doesn't have cluster size, so here we return
> a fake value for it.
> 
> Signed-off-by: zhanghailiang 
> Signed-off-by: Wen Congyang 
> ---
>  block/nbd.c | 12 
>  block/replication.c |  6 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 6bc06d6..96d7023 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -40,6 +40,8 @@
>  
>  #define EN_OPTSTR ":exportname="
>  
> +#define NBD_FAKE_CLUSTER_SIZE 512

Why 512?  NBD allows byte-addressable operations (even if it is more
efficient on aligned I/O); and I've been working hard to convert things
to the point that NBD does not enforce alignment on other layers.
Wouldn't 1 be better?

> +static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +bdi->cluster_size  = NBD_FAKE_CLUSTER_SIZE;

I also have patches written (but waiting for NBD write zeroes support to
be reviewed first) that add support for the experimental NBD block info,
that lets a server advertise actual sizes to the client rather than
having to guess.  Here's the last time I posted a preview of it:

https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03567.html

It would be nice to use that instead of just faking things.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] ssh: switch from libssh2 to libssh

[Richard W.M. Jones]

On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote:
> Rewrite the implementation of the ssh block driver to use libssh instead
> of libssh.  The libssh library has various advantages over libssh:
> - easier API for authentication (for example for using ssh-agent)
> - easier API for known_hosts handling
> - supports newer types of keys in known_hosts
> 
> Kerberos authentication can be enabled once the libssh bug for it [1] is
> fixed.
> 
> The development version of libssh (i.e. the future 0.8.x) supports
> fsync, so reuse the build time check for this.
> 
> [1] https://red.libssh.org/issues/242
> 
> Signed-off-by: Pino Toscano 


When I applied this patch and compiled it with warnings enabled:

block/ssh.c: In function ‘connect_to_ssh’:
block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this function 
[-Wmaybe-uninitialized]
 return ret;
^~~

To test the patch, I used virt-builder to create a virtual machine
disk image on another machine (accessible over ssh).  Then from my
laptop I ran:

  ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \
  -M accel=kvm -cpu host -m 2048 \
  -drive 
file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio
 \
  -serial stdio

Unfortunately this failed with a large number of sftp errors:

  read failed: (sftp error code: 0)

and subsequently hung.  So I'm afraid I couldn't test the patch at all :-(

One slightly peculiar thing is that qemu ends up being linked to both
libssh and libssh2.  I believe the libssh2 dependency comes indirectly
from libcurl.  It's a slightly surprising situation but I suppose
nothing to worry about.

Also fsync was not supported for me, but I'm using 0.7.3 and the code
says I need 0.8.0.

I'll do a more detailed review when the above are fixed.

Rich.

>  block/Makefile.objs |   6 +-
>  block/ssh.c | 543 
> +---
>  configure   |  65 ---
>  3 files changed, 249 insertions(+), 365 deletions(-)
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 67a036a..602a182 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -19,7 +19,7 @@ block-obj-$(CONFIG_CURL) += curl.o
>  block-obj-$(CONFIG_RBD) += rbd.o
>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
> -block-obj-$(CONFIG_LIBSSH2) += ssh.o
> +block-obj-$(CONFIG_LIBSSH) += ssh.o
>  block-obj-y += accounting.o dirty-bitmap.o
>  block-obj-y += write-threshold.o
>  block-obj-y += backup.o
> @@ -38,8 +38,8 @@ rbd.o-cflags   := $(RBD_CFLAGS)
>  rbd.o-libs := $(RBD_LIBS)
>  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
>  gluster.o-libs := $(GLUSTERFS_LIBS)
> -ssh.o-cflags   := $(LIBSSH2_CFLAGS)
> -ssh.o-libs := $(LIBSSH2_LIBS)
> +ssh.o-cflags   := $(LIBSSH_CFLAGS)
> +ssh.o-libs := $(LIBSSH_LIBS)
>  archipelago.o-libs := $(ARCHIPELAGO_LIBS)
>  block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
>  dmg-bz2.o-libs := $(BZIP2_LIBS)
> diff --git a/block/ssh.c b/block/ssh.c
> index 5ce12b6..7c316db 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -24,8 +24,8 @@
>  
>  #include "qemu/osdep.h"
>  
> -#include 
> -#include 
> +#include 
> +#include 
>  
>  #include "block/block_int.h"
>  #include "qapi/error.h"
> @@ -38,14 +38,12 @@
>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>   * this block driver code.
>   *
> - * TRACE_LIBSSH2= enables tracing in libssh2 itself.  Note
> - * that this requires that libssh2 was specially compiled with the
> - * `./configure --enable-debug' option, so most likely you will have
> - * to compile it yourself.  The meaning of  is described
> - * here: http://www.libssh2.org/libssh2_trace.html
> + * TRACE_LIBSSH= enables tracing in libssh itself.
> + * The meaning of  is described here:
> + * http://api.libssh.org/master/group__libssh__log.html
>   */
>  #define DEBUG_SSH 0
> -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
> +#define TRACE_LIBSSH  0 /* see: SSH_LOG_* */
>  
>  #define DPRINTF(fmt, ...)   \
>  do {\
> @@ -60,50 +58,39 @@ typedef struct BDRVSSHState {
>  CoMutex lock;
>  
>  /* SSH connection. */
> -int sock; /* socket */
> -LIBSSH2_SESSION *session; /* ssh session */
> -LIBSSH2_SFTP *sftp;   /* sftp session */
> -LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */
> +ssh_session session;  /* ssh session */
> +sftp_session sftp;/* sftp session */
> +sftp_file sftp_handle;/* sftp remote file handle */
>  
> -/* See ssh_seek() function below. */
> -int64_t offset;
> -bool offset_op_read;
> -
> -/* File attributes at open.  We try to keep the .filesize field
> +/* File attributes at open.  We try to keep the .size 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/9] block-backend: Use coroutine for flush/discard/ioctl

[no-reply]

Hi,

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

Subject: [Qemu-devel] [PATCH 0/9] block-backend: Use coroutine for 
flush/discard/ioctl
Type: series
Message-id: 1476971169-31604-1-git-send-email-kw...@redhat.com

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

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

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

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

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f1e92c7 block: Remove bdrv_aio_ioctl()
6f353df raw: Implement .bdrv_co_ioctl instead of .bdrv_aio_ioctl
1e5fd07 block: Introduce .bdrv_co_ioctl() driver callback
04b823d block: Remove bdrv_ioctl()
4fed6c5 raw-posix: Don't use bdrv_ioctl()
b972aa0 block: Use blk_co_ioctl() for all BB level ioctls
53ef487 block: Remove bdrv_aio_pdiscard()
d777b24 block: Use blk_co_pdiscard() for all BB level discard
2470c9c block: Use blk_co_flush() for all BB level flushes

=== OUTPUT BEGIN ===
Checking PATCH 1/9: block: Use blk_co_flush() for all BB level flushes...
Checking PATCH 2/9: block: Use blk_co_pdiscard() for all BB level discard...
Checking PATCH 3/9: block: Remove bdrv_aio_pdiscard()...
Checking PATCH 4/9: block: Use blk_co_ioctl() for all BB level ioctls...
Checking PATCH 5/9: raw-posix: Don't use bdrv_ioctl()...
Checking PATCH 6/9: block: Remove bdrv_ioctl()...
Checking PATCH 7/9: block: Introduce .bdrv_co_ioctl() driver callback...
ERROR: space prohibited between function name and open parenthesis '('
#51: FILE: include/block/block_int.h:247:
+int coroutine_fn (*bdrv_co_ioctl)(BlockDriverState *bs,

total: 1 errors, 0 warnings, 35 lines checked

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

Checking PATCH 8/9: raw: Implement .bdrv_co_ioctl instead of .bdrv_aio_ioctl...
Checking PATCH 9/9: block: Remove bdrv_aio_ioctl()...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org

Re: [Qemu-block] [PATCH] ssh: switch from libssh2 to libssh

[Pino Toscano]

On Thursday, 20 October 2016 16:32:50 CEST Richard W.M. Jones wrote:
> On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote:
> > Rewrite the implementation of the ssh block driver to use libssh instead
> > of libssh.  The libssh library has various advantages over libssh:
> > - easier API for authentication (for example for using ssh-agent)
> > - easier API for known_hosts handling
> > - supports newer types of keys in known_hosts
> > 
> > Kerberos authentication can be enabled once the libssh bug for it [1] is
> > fixed.
> > 
> > The development version of libssh (i.e. the future 0.8.x) supports
> > fsync, so reuse the build time check for this.
> > 
> > [1] https://red.libssh.org/issues/242
> > 
> > Signed-off-by: Pino Toscano 
> 
> 
> When I applied this patch and compiled it with warnings enabled:
> 
> block/ssh.c: In function ‘connect_to_ssh’:
> block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this function 
> [-Wmaybe-uninitialized]
>  return ret;
> ^~~

Interesting, there was no warning for me.  Anyway, I think this:

diff --git a/block/ssh.c b/block/ssh.c
index 7c316db..7ff376e 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -519,6 +519,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 /* Create SSH session. */
 s->session = ssh_new();
 if (!s->session) {
+ret = -EINVAL;
 goto err;
 }
 
should fix it (added already locally).

> To test the patch, I used virt-builder to create a virtual machine
> disk image on another machine (accessible over ssh).  Then from my
> laptop I ran:
> 
>   ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \
>   -M accel=kvm -cpu host -m 2048 \
>   -drive 
> file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio
>  \
>   -serial stdio
> 
> Unfortunately this failed with a large number of sftp errors:
> 
>   read failed: (sftp error code: 0)
> 
> and subsequently hung.  So I'm afraid I couldn't test the patch at all :-(

Can you please enable the logging of the ssh driver, and libssh own
logging too?  Basically (see lines 45-46) set:

#define DEBUG_SSH 1
#define TRACE_LIBSSH  4

> Also fsync was not supported for me, but I'm using 0.7.3 and the code
> says I need 0.8.0.

Yes, this is correct.

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

[Qemu-block] [PATCH] ssh: switch from libssh2 to libssh

[Pino Toscano]

Rewrite the implementation of the ssh block driver to use libssh instead
of libssh.  The libssh library has various advantages over libssh:
- easier API for authentication (for example for using ssh-agent)
- easier API for known_hosts handling
- supports newer types of keys in known_hosts

Kerberos authentication can be enabled once the libssh bug for it [1] is
fixed.

The development version of libssh (i.e. the future 0.8.x) supports
fsync, so reuse the build time check for this.

[1] https://red.libssh.org/issues/242

Signed-off-by: Pino Toscano 
---
 block/Makefile.objs |   6 +-
 block/ssh.c | 543 +---
 configure   |  65 ---
 3 files changed, 249 insertions(+), 365 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 67a036a..602a182 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -19,7 +19,7 @@ block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
 block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
-block-obj-$(CONFIG_LIBSSH2) += ssh.o
+block-obj-$(CONFIG_LIBSSH) += ssh.o
 block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 block-obj-y += backup.o
@@ -38,8 +38,8 @@ rbd.o-cflags   := $(RBD_CFLAGS)
 rbd.o-libs := $(RBD_LIBS)
 gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
 gluster.o-libs := $(GLUSTERFS_LIBS)
-ssh.o-cflags   := $(LIBSSH2_CFLAGS)
-ssh.o-libs := $(LIBSSH2_LIBS)
+ssh.o-cflags   := $(LIBSSH_CFLAGS)
+ssh.o-libs := $(LIBSSH_LIBS)
 archipelago.o-libs := $(ARCHIPELAGO_LIBS)
 block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
 dmg-bz2.o-libs := $(BZIP2_LIBS)
diff --git a/block/ssh.c b/block/ssh.c
index 5ce12b6..7c316db 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
 
 #include "qemu/osdep.h"
 
-#include 
-#include 
+#include 
+#include 
 
 #include "block/block_int.h"
 #include "qapi/error.h"
@@ -38,14 +38,12 @@
 /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
  * this block driver code.
  *
- * TRACE_LIBSSH2= enables tracing in libssh2 itself.  Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself.  The meaning of  is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of  is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
  */
 #define DEBUG_SSH 0
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH  0 /* see: SSH_LOG_* */
 
 #define DPRINTF(fmt, ...)   \
 do {\
@@ -60,50 +58,39 @@ typedef struct BDRVSSHState {
 CoMutex lock;
 
 /* SSH connection. */
-int sock; /* socket */
-LIBSSH2_SESSION *session; /* ssh session */
-LIBSSH2_SFTP *sftp;   /* sftp session */
-LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */
+ssh_session session;  /* ssh session */
+sftp_session sftp;/* sftp session */
+sftp_file sftp_handle;/* sftp remote file handle */
 
-/* See ssh_seek() function below. */
-int64_t offset;
-bool offset_op_read;
-
-/* File attributes at open.  We try to keep the .filesize field
+/* File attributes at open.  We try to keep the .size field
  * updated if it changes (eg by writing at the end of the file).
  */
-LIBSSH2_SFTP_ATTRIBUTES attrs;
+sftp_attributes attrs;
 
 /* Used to warn if 'flush' is not supported. */
-char *hostport;
 bool unsafe_flush_warning;
 } BDRVSSHState;
 
 static void ssh_state_init(BDRVSSHState *s)
 {
 memset(s, 0, sizeof *s);
-s->sock = -1;
-s->offset = -1;
 qemu_co_mutex_init(>lock);
 }
 
 static void ssh_state_free(BDRVSSHState *s)
 {
-g_free(s->hostport);
+if (s->attrs) {
+sftp_attributes_free(s->attrs);
+}
 if (s->sftp_handle) {
-libssh2_sftp_close(s->sftp_handle);
+sftp_close(s->sftp_handle);
 }
 if (s->sftp) {
-libssh2_sftp_shutdown(s->sftp);
+sftp_free(s->sftp);
 }
 if (s->session) {
-libssh2_session_disconnect(s->session,
-   "from qemu ssh client: "
-   "user closed the connection");
-libssh2_session_free(s->session);
-}
-if (s->sock >= 0) {
-close(s->sock);
+ssh_disconnect(s->session);
+ssh_free(s->session);
 }
 }
 
@@ -118,13 +105,13 @@ session_error_setg(Error **errp, BDRVSSHState *s, const 
char *fs, ...)
 va_end(args);
 
 if (s->session) {
-char *ssh_err;
+const char *ssh_err;
 int ssh_err_code;
 
-/* This is not an errno.  See . */
-

Re: [Qemu-block] [Qemu-devel] [PATCH v14 00/19] QAPI/QOM work for non-scalar object properties

[Markus Armbruster]

I've warmed quite a bit to this series while reviewing it.  In
particular, I've come around to like structuring the command line ->
QAPI pipeline exactly like the JSON -> QAPI pipeline, namely 1. parse
into QObject, 2. convert to QAPI with the QObject input visitor.
QObject serves as abstract syntax here.  The differences between JSON
and command line result in different abstract syntax, which in turn
necessitates two cases in the input visitor.  The series adds more than
two, to cater for option visitor funnies.  Perhaps we can do without
some of them.

The other way to skin this cat would be an improved options visitor.
Has its advantages and disadvantages, but the big one is that you
already did the work for QObject input visitor solution.

The one major issue I have with the series is that it adds to the
growing body of QemuOpts hacks/workarounds.

We've pushed QemuOpts beyond well its design limits.  What started as a
simple store for scalar configuration parameters structured as key=value
lists, plus command line and configuration file syntax, has grown three
ways to specify lists (repeated keys, basically an implementation
accident that got pressed into service; special integer list syntax in
the options visitor, later adopted by the string input visitor,
hopefully compatibly; and the block layer's dotted key convention) and a
way to specify arbitrary complex values (block layer's dotted key
convention again).  Of these, only "repeated keys" is in QemuOpts
proper, all the others are bolted on and used only when needed.  How
they interact is not obvious.

This series marries all the bolted-on stuff and puts it in the QObject
visitor.  That's actually an improvement of sorts; at least it's in just
two places now.  But it's still a smorgasbord of syntactical/semantic
options.

I feel it's time to stop working around the design limits of QemuOpts
and start replacing them by something that serves our current needs.  We
basically need the expressive power of JSON on the command line.  Syntax
is debatable, but it should be *one* concrete command-line syntax,
parsed by *one* parser into *one* kind of abstract syntax tree, where
the only optional variations are the ones forced upon us by backward
compatibility.

We can't do this for 2.8, obviously.  We can try for 2.9.  I have pretty
specific ideas on how it should be done, so I guess it's only fair I
give it a try myself.

I know the block layer wants to use bits of this series to save some
coding work for certain features targeting 2.8.  I have no objections as
long as it doesn't create new ABI.  Exception: I'm okay with applying
dotted key convention to more of the same, e.g. new block drivers.

Sounds sane?

Re: [Qemu-block] [Qemu-devel] [PATCH v14 00/19] QAPI/QOM work for non-scalar object properties

[Daniel P. Berrange]

On Thu, Oct 20, 2016 at 05:06:33PM +0200, Markus Armbruster wrote:
> I've warmed quite a bit to this series while reviewing it.  In
> particular, I've come around to like structuring the command line ->
> QAPI pipeline exactly like the JSON -> QAPI pipeline, namely 1. parse
> into QObject, 2. convert to QAPI with the QObject input visitor.
> QObject serves as abstract syntax here.  The differences between JSON
> and command line result in different abstract syntax, which in turn
> necessitates two cases in the input visitor.  The series adds more than
> two, to cater for option visitor funnies.  Perhaps we can do without
> some of them.
> 
> The other way to skin this cat would be an improved options visitor.
> Has its advantages and disadvantages, but the big one is that you
> already did the work for QObject input visitor solution.
> 
> The one major issue I have with the series is that it adds to the
> growing body of QemuOpts hacks/workarounds.
> 
> We've pushed QemuOpts beyond well its design limits.  What started as a
> simple store for scalar configuration parameters structured as key=value
> lists, plus command line and configuration file syntax, has grown three
> ways to specify lists (repeated keys, basically an implementation
> accident that got pressed into service; special integer list syntax in
> the options visitor, later adopted by the string input visitor,
> hopefully compatibly; and the block layer's dotted key convention) and a
> way to specify arbitrary complex values (block layer's dotted key
> convention again).  Of these, only "repeated keys" is in QemuOpts
> proper, all the others are bolted on and used only when needed.  How
> they interact is not obvious.
> 
> This series marries all the bolted-on stuff and puts it in the QObject
> visitor.  That's actually an improvement of sorts; at least it's in just
> two places now.  But it's still a smorgasbord of syntactical/semantic
> options.
> 
> I feel it's time to stop working around the design limits of QemuOpts
> and start replacing them by something that serves our current needs.  We
> basically need the expressive power of JSON on the command line.  Syntax
> is debatable, but it should be *one* concrete command-line syntax,
> parsed by *one* parser into *one* kind of abstract syntax tree, where
> the only optional variations are the ones forced upon us by backward
> compatibility.
> 
> We can't do this for 2.8, obviously.  We can try for 2.9.  I have pretty
> specific ideas on how it should be done, so I guess it's only fair I
> give it a try myself.
> 
> I know the block layer wants to use bits of this series to save some
> coding work for certain features targeting 2.8.  I have no objections as
> long as it doesn't create new ABI.  Exception: I'm okay with applying
> dotted key convention to more of the same, e.g. new block drivers.
> 
> Sounds sane?

It is all fine with me.

Of the patch series proposed, I think patches 1->6 ought to be safe to
take for 2.8, without exposing new ABI:

 1. option: make parse_option_bool/number non-static
 2. qdict: implement a qdict_crumple method for un-flattening a dict
 3. qapi: add trace events for visitor
 4. qapi: rename QmpInputVisitor to QObjectInputVisitor
 5. qapi: rename QmpOutputVisitor to QObjectOutputVisitor
 6. qapi: don't pass two copies of TestInputVisitorData to tests

The rest are clearly not an option for 2.8 since freeze is barely
1 week away, and I'm travelling all next week

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-block] [PATCH 0/9] block-backend: Use coroutine for flush/discard/ioctl

[Paolo Bonzini]

- Original Message -
> From: "Kevin Wolf" 
> To: qemu-block@nongnu.org
> Cc: kw...@redhat.com, mre...@redhat.com, pbonz...@redhat.com, 
> qemu-de...@nongnu.org
> Sent: Thursday, October 20, 2016 3:46:00 PM
> Subject: [PATCH 0/9] block-backend: Use coroutine for flush/discard/ioctl
> 
> Paolo, this is my attempt at implementing what you were asking for last
> Friday.
> I converted blk_(co_)flush/pdiscard/ioctl so that all interfaces (coroutine,
> AIO, sync) go through the same coroutine-based function already on the
> BlockBackend level. Where it was reasonably easy, I also removed the
> corresponding emulations from block/io.c IIUC, this should cover your
> immediate
> needs.
> 
> 
> Function to remove this series leaves for another day:
> 
> * bdrv_aio_flush (used by blkdebug, blkverify, qed)
> * bdrv_flush (even more users)
> * bdrv_pdiscard (used by qcow2)
> 
> 
> BlockDriver callbacks to remove left for another day:
> 
> * bdrv_aio_pdiscard (implemented by raw-posix and rbd)
> * bdrv_aio_ioctl (implemented by raw-posix and iscsi)
> 
> In both cases, raw-posix is trivial to covert, but iscsi and rbd feel rather
> scary without a proper test setup.

Thanks!

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options

[Daniel P. Berrange]

On Thu, Oct 13, 2016 at 10:31:37AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > If given an option string such as
> >
> >   size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind
> >
> > the qemu_opts_to_qdict() method will currently overwrite
> > the values for repeated option keys, so only the last
> > value is in the returned dict:
> >
> > size=QString("1024")
> > nodes=QString("1-2")
> > policy=QString("bind")
> >
> > With this change the caller can optionally ask for all
> > the repeated values to be stored in a QList. In the
> > above example that would result in 'nodes' being a
> > QList, so the returned dict would contain
> >
> > size=QString("1024")
> > nodes=QList([QString("10"),
> >  QString("4-5"),
> >  QString("1-2")])
> > policy=QString("bind")
> >
> > Note that the conversion has no way of knowing whether
> > any given key is expected to be a list upfront - it can
> > only figure that out when seeing the first duplicated
> > key. Thus the caller has to be prepared to deal with the
> > fact that if a key 'foo' is a list, then the returned
> > qdict may contain either a QString or a QList for the
> > key 'foo'.
> 
> Actually three cases, not two:
> 
> 0. qdict does not contain the key means empty list.
> 
> 1. qdict contains the key with a QString value means list of one
> element.
> 
> 2. qdict contains the key with a QList value means list of more than one
> element.
> 
> I consider this weird.  However, it's usefully weird with at least your
> QObject input visitor.
> 
> > In a third mode, it is possible to ask for repeated
> > options to be reported as an error, rather than silently
> > dropping all but the last one.
> 
> Got users for this policy in the pipeline?

I in fact used it in the QObjectInputVisitor, when the
autocreate_list is not set.

I guess strictly speaking this is not back-compatible
if someone is passing repeated keys, but I judged that
rather than silently ignoring this incorrect usage, it
was acceptable to report an error.




> >  QTAILQ_FOREACH(opt, >head, next) {
> >  val = QOBJECT(qstring_from_str(opt->str));
> > -qdict_put_obj(qdict, opt->name, val);
> > +
> > +if (qdict_haskey(ret, opt->name)) {
> > +switch (repeatPolicy) {
> > +case QEMU_OPTS_REPEAT_POLICY_ERROR:
> > +error_setg(errp, "Option '%s' appears more than once",
> > +   opt->name);
> > +qobject_decref(val);
> > +if (!qdict) {
> > +QDECREF(ret);
> > +}
> > +return NULL;
> > +
> > +case QEMU_OPTS_REPEAT_POLICY_ALL:
> > +prevval = qdict_get(ret, opt->name);
> > +if (qobject_type(prevval) == QTYPE_QLIST) {
> > +/* Already identified this key as a list */
> > +list = qobject_to_qlist(prevval);
> > +} else {
> > +/* Replace current scalar with a list */
> > +list = qlist_new();
> > +qobject_incref(prevval);
> 
> Where is this reference decremented?

'prevval' is a borrowed reference from 'ret', against the
key opt->name.

qdict_put_obj decrements the reference we borrowed
from ret against the key opt->name. 

qlist_append_obj() takes ownership of the reference
it is passed, so we must qobject_incref() to avoid
qdict_put_obj free'ing prevval.

When we call qdict_put_obj() we're replacing the value
currently associted

> 
> > +qlist_append_obj(list, prevval);
> > +qdict_put_obj(ret, opt->name, QOBJECT(list));
> > +}
> > +qlist_append_obj(list, val);
> > +break;
> > +
> > +case QEMU_OPTS_REPEAT_POLICY_LAST:
> > +/* Just discard previously set value */
> > +qdict_put_obj(ret, opt->name, val);
> > +break;
> > +}
> > +} else {
> > +qdict_put_obj(ret, opt->name, val);
> > +}
> 
> A possible alternative:
> 
>val = QOBJECT(qstring_from_str(opt->str));
> 
>if (qdict_haskey(ret, opt->name)) {
>switch (repeatPolicy) {
>case QEMU_OPTS_REPEAT_POLICY_ERROR:
>error_setg(errp, "Option '%s' appears more than once",
>   opt->name);
>qobject_decref(val);
>if (!qdict) {
>QDECREF(ret);
>}
>return NULL;
> 
>case QEMU_OPTS_REPEAT_POLICY_ALL:
>prevval = qdict_get(ret, opt->name);
>if (qobject_type(prevval) == QTYPE_QLIST) {
>/* Already identified this key as a list */
>list = qobject_to_qlist(prevval);
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v14 13/21] qdict: allow qdict_crumple to accept compound types as values

[Daniel P. Berrange]

On Thu, Oct 13, 2016 at 02:35:38PM +0200, Markus Armbruster wrote:
> Cc: Kevin for discussion of QemuOpts dotted key convention
> 
> "Daniel P. Berrange"  writes:
> 
> > Currently qdict_crumple requires a totally flat QDict as its
> > input. i.e. all values in the QDict must be scalar types.
> >
> > In order to have backwards compatibility with the OptsVisitor,
> > qemu_opt_to_qdict() has a new mode where it may return a QList
> > for values in the QDict, if there was a repeated key. We thus
> > need to allow compound types to appear as values in the input
> > dict given to qdict_crumple().
> >
> > To avoid confusion, we sanity check that the user has not mixed
> > the old and new syntax at the same time. e.g. these are allowed
> >
> >foo=hello,foo=world,foo=wibble
> >foo.0=hello,foo.1=world,foo.2=wibble
> >
> > but this is forbidden
> >
> >foo=hello,foo=world,foo.2=wibble
> 
> I understand the need for foo.bar=val.  It makes it possible to specify
> nested dictionaries with QemuOpts.
> 
> The case for foo.0=val is less clear.  QemuOpts already supports lists,
> by repeating keys.  Why do we need a second, wordier way to specify
> them?

Two reasons I did this. First blockdev already uses this foo.0=val
syntax, and I wanted to be compatible with blockdev, so it could be
converted to use this new code.

Second, using foo.0 syntax means that you can unambigously determine
whether a key is going to be a scalar or a list. This lets the
qdict_crumple() method convert the QemuOpts to a QDict without
needing to know anything about the QAPI schema.

Of course I later had to add hacks to the visitor to cope with
the bare repeated key syntax, so I lost some of that benefit.

Personally I still prefer the unambiguous syntax as it lets us
give clear error messages when users do unexpected things, instead
of say, silently ignoring all but the last key.

> Note that this second way creates entirely new failure modes and
> restrictions.  Let me show using an example derived from one in
> qdict_crumple()'s contract:
> 
> foo.0.bar=bla,foo.eek.bar=blubb
> 
> Without the dotted key convention, this is perfectly fine: key
> "foo.0.bar" has the single value "bla", and key "foo.eek.bar" has
> the single value "blubb".  Equivalent JSON would be
> 
>   { "foo.0.bar": "bla", "foo.eek.bar": "blubb" }
> 
> With just the struct convention, it's still fine: it obviously means
> the same as JSON
> 
>   { "foo": { "0": { "bar": "bla" }, "eek": { "bar": "blubb" } } }
> 
> Adding the list convention makes it invalid.  It also outlaws a
> bunch of keys that would be just fine in JSON, namely any that get
> recognized as list index.  Raise your hand if you're willing to bet
> real money on your predictions of what will be recognized as list
> index, without looking at the code.  I'm not.
> 
> I'm afraid I have growing doubts regarding the QemuOpts dotted key
> convention in general.
> 
> The convention makes '.' a special character in keys, but only
> sometimes.  If the key gets consumed by something that uses dotted key
> convention, '.' is special, and to get a non-special '.', you need to
> escape it by doubling.  Else, it's not.
> 
> Since the same key can be used differently by different code, the same
> '.' could in theory be both special and non-special.  In practice, this
> would be madness.
> 
> Adopting the dotted key convention for an existing QemuOpts option, say
> -object [PATCH 15], *breaks* existing command line usage of keys
> containing '.', because you now have to escape the '.'.  Dan, I'm afraid
> you need to show that no such keys exist, or if they exist they don't
> matter.

I checked the things that I converted (eg -net, -object, -numa, etc),
but I didn't check -device since that's processed using different code.

> 
> I know we have keys containing '.' elsewhere, e.g. device "macio-ide"
> property "ide.0".  Our chronic inability to consistently restrict names
> in ABI to something sane is beyond foolish.
> 
> It's probably too late to back out the dotted key convention completely.
> Kevin?
> 
> Can we still back out the list part of the convention, and use repeated
> keys instead?
> 
> If we're stuck with some form of the dotted key convention, can we at
> least make it a more integral part of QemuOpts rather than something
> bolted on as an afterthought?  Here's my thinking on how that might be
> done:

The only issue with dropping the dotted list convention is compat
with the block layer code - we couldn't easily use this new visitor
logic to turn -drive into a QAPI BlockOptions object.  Kevin's new
-blockdev arg would potentially be ok with it since its a new arg,
but IIUC, we would have to do some cleanup inside various block
driver impls, since block layer doesn't use the QAPI objects
internally - they all get converted back into QemuOpts :-(

> 
> * Have a QemuOptsList flag @flat.
> 
> * If @flat, QemuOpts behaves as it always 

Re: [Qemu-block] [Qemu-devel] [PATCH v4 2/4] fdc: Add a floppy drive qdev

[KONRAD Frederic]

Le 20/10/2016 à 09:55, Kevin Wolf a écrit :

Floppy controllers automatically create two floppy drive devices in qdev
now. (They always created two drives, but managed them only internally.)

Signed-off-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 hw/block/fdc.c | 151 +
 1 file changed, 120 insertions(+), 31 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a3afb62..5aa8e52 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,8 @@
 #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)

 typedef struct FDCtrl FDCtrl;
+typedef struct FDrive FDrive;
+static FDrive *get_drv(FDCtrl *fdctrl, int unit);

 typedef struct FloppyBus {
 BusState bus;
@@ -180,7 +182,7 @@ typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
 } FDiskFlags;

-typedef struct FDrive {
+struct FDrive {
 FDCtrl *fdctrl;
 BlockBackend *blk;
 /* Drive status */
@@ -201,7 +203,7 @@ typedef struct FDrive {
 uint8_t media_rate;   /* Data rate of medium*/

 bool media_validated; /* Have we validated the media? */
-} FDrive;
+};


 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
@@ -466,6 +468,100 @@ static void fd_revalidate(FDrive *drv)
 }
 }

+static void fd_change_cb(void *opaque, bool load)
+{
+FDrive *drive = opaque;
+
+drive->media_changed = 1;
+drive->media_validated = false;
+fd_revalidate(drive);
+}
+
+static const BlockDevOps fd_block_ops = {
+.change_media_cb = fd_change_cb,
+};
+
+
+#define TYPE_FLOPPY_DRIVE "floppy"
+#define FLOPPY_DRIVE(obj) \
+ OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
+
+typedef struct FloppyDrive {
+DeviceState qdev;
+uint32_tunit;
+} FloppyDrive;
+
+static Property floppy_drive_properties[] = {
+DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static int floppy_drive_init(DeviceState *qdev)
+{
+FloppyDrive *dev = FLOPPY_DRIVE(qdev);
+FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);


Can't you use FLOPPY_BUS() introduced in the previous patch instead?
or I missed something?

Fred


+FDrive *drive;
+
+if (dev->unit == -1) {
+for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
+drive = get_drv(bus->fdc, dev->unit);
+if (!drive->blk) {
+break;
+}
+}
+}
+
+if (dev->unit >= MAX_FD) {
+error_report("Can't create floppy unit %d, bus supports only %d units",
+ dev->unit, MAX_FD);
+return -1;
+}
+
+/* TODO Check whether unit is in use */
+
+drive = get_drv(bus->fdc, dev->unit);
+
+if (drive->blk) {
+if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
+error_report("fdc doesn't support drive option werror");
+return -1;
+}
+if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+} else {
+/* Anonymous BlockBackend for an empty drive */
+drive->blk = blk_new();
+}
+
+fd_init(drive);
+if (drive->blk) {
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+pick_drive_type(drive);
+}
+fd_revalidate(drive);
+
+return 0;
+}
+
+static void floppy_drive_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *k = DEVICE_CLASS(klass);
+k->init = floppy_drive_init;
+set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
+k->bus_type = TYPE_FLOPPY_BUS;
+k->props = floppy_drive_properties;
+k->desc = "virtual floppy drive";
+}
+
+static const TypeInfo floppy_drive_info = {
+.name = TYPE_FLOPPY_DRIVE,
+.parent = TYPE_DEVICE,
+.instance_size = sizeof(FloppyDrive),
+.class_init = floppy_drive_class_init,
+};
+
 //
 /* Intel 82078 floppy disk controller emulation  */

@@ -1185,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
 }
 #endif

-static FDrive *get_cur_drv(FDCtrl *fdctrl)
+static FDrive *get_drv(FDCtrl *fdctrl, int unit)
 {
-switch (fdctrl->cur_drv) {
+switch (unit) {
 case 0: return drv0(fdctrl);
 case 1: return drv1(fdctrl);
 #if MAX_FD == 4
@@ -1198,6 +1294,11 @@ static FDrive *get_cur_drv(FDCtrl *fdctrl)
 }
 }

+static FDrive *get_cur_drv(FDCtrl *fdctrl)
+{
+return get_drv(fdctrl, fdctrl->cur_drv);
+}
+
 /* Status A register : 0x00 (read-only) */
 static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl)
 {
@@ -2357,46 +2458,33 @@ static void fdctrl_result_timer(void *opaque)
 }
 }

-static void fdctrl_change_cb(void *opaque, bool load)
-{
-FDrive *drive = opaque;
-
-drive->media_changed = 1;
-drive->media_validated = false;
-fd_revalidate(drive);
-}
-
-static const BlockDevOps fdctrl_block_ops = {
-

Re: [Qemu-block] [Qemu-devel] [PATCH v14 11/21] qapi: add integer range support for QObjectInputVisitor

[Daniel P. Berrange]

On Wed, Oct 12, 2016 at 05:50:41PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > The traditional CLI arg syntax allows two ways to specify
> > integer lists, either one value per key, or a range of
> > values per key. eg the following are identical:
> >
> >   -arg foo=5,foo=6,foo=7
> >   -arg foo=5-7
> >
> > This extends the QObjectInputVisitor so that it is able
> > to parse ranges and turn them into distinct list entries.
> >
> > This means that
> >
> >   -arg foo=5-7
> >
> > is treated as equivalent to
> >
> >   -arg foo.0=5,foo.1=6,foo.2=7
> >
> > Edge case tests are copied from test-opts-visitor to
> > ensure identical behaviour when parsing.
> >
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  include/qapi/qobject-input-visitor.h |  23 -
> >  qapi/qobject-input-visitor.c | 158 ++--
> >  tests/test-qobject-input-visitor.c   | 195 
> > +--
> >  3 files changed, 360 insertions(+), 16 deletions(-)
> >

> >  static void qobject_input_type_uint64(Visitor *v, const char *name,
> > @@ -366,21 +438,85 @@ static void 
> > qobject_input_type_uint64_autocast(Visitor *v, const char *name,
> > uint64_t *obj, Error **errp)
> >  {
> >  QObjectInputVisitor *qiv = to_qiv(v);
> > -QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
> > -true));
> > +QString *qstr;
> >  unsigned long long ret;
> > +char *end = NULL;
> > +StackObject *tos;
> > +bool inlist = false;
> > +
> > +/* Preferentially generate values from a range, before
> > + * trying to consume another QList element */
> > +tos = QSLIST_FIRST(>stack);
> > +if (tos) {
> > +if (tos->range_val < tos->range_limit) {
> > +*obj = tos->range_val + 1;
> > +tos->range_val++;
> > +return;
> > +} else {
> > +inlist = tos->entry != NULL;
> > +}
> > +}
> >  
> > +qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
> > +   true));
> >  if (!qstr || !qstr->string) {
> >  error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> > "string");
> >  return;
> >  }
> >  
> > -if (parse_uint_full(qstr->string, , 0) < 0) {
> > +if (parse_uint(qstr->string, , , 0) < 0) {
> >  error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
> >  return;
> >  }
> >  *obj = ret;
> > +
> > +/*
> > + * If we have string that represents an integer range (5-24),
> > + * parse the end of the range and set things up so we'll process
> > + * the rest of the range before consuming another element
> > + * from the QList.
> > + */
> > +if (end && *end) {
> > +if (!qiv->permit_int_ranges) {
> > +error_setg(errp,
> > +   "Integer ranges are not permitted here");
> > +return;
> > +}
> > +if (!inlist) {
> > +error_setg(errp,
> > +   "Integer ranges are only permitted when "
> > +   "visiting list parameters");
> > +return;
> > +}
> > +if (*end != '-') {
> > +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> > +   "a number range");
> > +return;
> > +}
> > +end++;
> > +if (parse_uint_full(end, , 0) < 0) {
> > +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a 
> > number");
> > +return;
> > +}
> > +if (*obj > ret) {
> > +error_setg(errp,
> > +   "Parameter '%s' range start %" PRIu64
> > +   " must be less than (or equal to) %llu",
> > +   name, *obj, ret);
> > +return;
> > +}
> > +if ((ret - *obj) > (QIV_RANGE_MAX - 1)) {
> > +error_setg(errp,
> > +   "Parameter '%s' range must be less than %d",
> > +   name, QIV_RANGE_MAX);
> > +return;
> > +}
> > +if (*obj != ret) {
> > +tos->range_val = *obj;
> > +tos->range_limit = ret;
> > +}
> > +}
> >  }
> 
> Duplicates the signed code, which is sad, but I don't have better ideas.
> 
> Except this one: are we actually using both the signed and the unsigned
> case now?  If not, can we get rid of the one we don't use?

Out of the args that I converted in this series, I only see uint16List
used. eg for -numa and -object hostmem

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: 

Re: [Qemu-block] [PATCH] m25p80: add support for the mx66l1g45g

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Reviewed-by: Marcin Krzeminski 

> -Original Message-
> From: Cédric Le Goater [mailto:c...@kaod.org]
> Sent: Thursday, October 20, 2016 9:18 AM
> To: Peter Crosthwaite 
> Cc: Kevin Wolf ; qemu-block@nongnu.org; Peter
> Maydell ; qemu-de...@nongnu.org; Krzeminski,
> Marcin (Nokia - PL/Wroclaw) ; Cédric Le
> Goater 
> Subject: [PATCH] m25p80: add support for the mx66l1g45g
> 
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/block/m25p80.c |1 +
>  1 file changed, 1 insertion(+)
> 
> Index: qemu-aspeed.git/hw/block/m25p80.c
> ==
> =
> --- qemu-aspeed.git.orig/hw/block/m25p80.c
> +++ qemu-aspeed.git/hw/block/m25p80.c
> @@ -203,6 +203,7 @@ static const FlashPartInfo known_devices
>  { INFO("mx25l25655e", 0xc22619,  0,  64 << 10, 512, 0) },
>  { INFO("mx66u51235f", 0xc2253a,  0,  64 << 10, 1024, ER_4K | ER_32K) 
> },
>  { INFO("mx66u1g45g",  0xc2253b,  0,  64 << 10, 2048, ER_4K | ER_32K) 
> },
> +{ INFO("mx66l1g45g",  0xc2201b,  0,  64 << 10, 2048, ER_4K | ER_32K) 
> },
> 
>  /* Micron */
>  { INFO("n25q032a11",  0x20bb16,  0,  64 << 10,  64, ER_4K) },

Re: [Qemu-block] [Qemu-devel] [PATCH v14 12/21] option: allow qemu_opts_to_qdict to merge repeated options

[Daniel P. Berrange]

On Wed, Oct 12, 2016 at 07:46:00PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > If given an option string such as
> >
> >   size=1024,nodes=10,nodes=4-5,nodes=1-2,policy=bind
> >
> > the qemu_opts_to_qdict() method will currently overwrite
> > the values for repeated option keys, so only the last
> > value is in the returned dict:
> >
> > size=QString("1024")
> > nodes=QString("1-2")
> > policy=QString("bind")
> >
> > With this change the caller can optionally ask for all
> > the repeated values to be stored in a QList. In the
> > above example that would result in 'nodes' being a
> > QList, so the returned dict would contain
> >
> > size=QString("1024")
> > nodes=QList([QString("10"),
> >  QString("4-5"),
> >  QString("1-2")])
> > policy=QString("bind")
> >
> > Note that the conversion has no way of knowing whether
> > any given key is expected to be a list upfront - it can
> > only figure that out when seeing the first duplicated
> > key. Thus the caller has to be prepared to deal with the
> > fact that if a key 'foo' is a list, then the returned
> > qdict may contain either a QString or a QList for the
> > key 'foo'.
> >
> > In a third mode, it is possible to ask for repeated
> > options to be reported as an error, rather than silently
> > dropping all but the last one.
> 
> To serve as a replacement for the options visitor, this needs to be able
> to behave exactly the same together with a suitably hacked up QObject
> input visitor.  Before I dive into the actual patch, let me summarize
> QemuOpts and options visitor behavior.
> 
> Warning, this is going to get ugly.
> 
> QemuOpts faithfully represents a key=value,... string as a list of
> QemuOpt.  Each QemuOpt represents one key=value.  They are in the same
> order.  If key occurs multiple times in the string, it occurs just the
> same in the list.
> 
> *Except* key "id" is special: it's stored outside the list, and all but
> the first one are silently ignored.
> 
> Most users only ever get the last value of a key.  Any non-last
> key=value are silently ignored.
> 
> We actually exploit this behavior to do defaults, by *prepending* them
> to the list.  See the use of qemu_opts_set_defaults() in main().
> 
> A few users get all values of keys (other than key "id"):
> 
> * -device, in qdev_device_add() with callback set_property().
> 
>   We first get "driver" and "bus" normally (silently ignoring non-last
>   values, as usual).  All other keys are device properties.  To set
>   them, we get all (key, value), ignore keys "driver" and "bus", and set
>   the rest.  If a key occurs multiple times, it gets set multiple times.
>   This effectively ignores all but the last one, silently.
> 
> * -semihosting-config, in main() with callback add_semihosting_arg().
> 
>   We first get a bunch of keys normally.  Key "arg" is special: it may
>   be repeated to build a list.  To implement that, we get all (key,
>   value), ignore keys other than "arg", and accumulate the values.
> 
> * -machine & friends, in main() with callback machine_set_property()
> 
>   Similar to -device, only for machines, with "type" instead of "driver"
>   and "bus".
> 
> * -spice, in qemu_spice_init() with callback add_channel()
> 
>   Keys "tls-channel" and "plaintext-channel" may be used repeated to
>   specify multiple channels.  To implement that, we get all (key,
>   value), ignore keys other than "tls-channel" and "plaintext-channel",
>   and set up a channel for each of the others.
> 
> * -writeconfig, in config_write_opts() with callback config_write_opt()
> 
>   We write out all keys in order.
> 
> * The options visitor, in opts_start_struct()
> 
>   We convert the list of (key, value) to a hash table of (key, list of
>   values).  Most of the time, the list of values has exactly one
>   element.
> 
>   When the visitor's user asks for a scalar, we return the last element
>   of the list of values, in lookup_scalar().
> 
>   When the user asks for list elements, we return the elements of the
>   list of values in order, in opts_next_list(), or if there are none,
>   the empty list in opts_start_list().
> 
> Unlike the options visitor, this patch (judging from your description)
> makes a list only when keys are repeated.  The QObject visitor will have
> to cope with finding both scalars and lists.  When it finds a scalar,
> but needs a list, it'll have to wrap it in a list (PATCH 09, I think).
> When it finds a list, but needs a scalar, it'll have to fish it out of
> the list (where is that?).

If my code finds a list but wants a scalar, it is reporting an
error.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict

[Daniel P. Berrange]

On Tue, Oct 18, 2016 at 04:32:13PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > The qdict_flatten() method will take a dict whose elements are
> > further nested dicts/lists and flatten them by concatenating
> > keys.
> >
> > The qdict_crumple() method aims to do the reverse, taking a flat
> > qdict, and turning it into a set of nested dicts/lists. It will
> > apply nesting based on the key name, with a '.' indicating a
> > new level in the hierarchy. If the keys in the nested structure
> > are all numeric, it will create a list, otherwise it will create
> > a dict.
> >
> > If the keys are a mixture of numeric and non-numeric, or the
> > numeric keys are not in strictly ascending order, an error will
> > be reported.
> >
> > As an example, a flat dict containing
> >
> >  {
> >'foo.0.bar': 'one',
> >'foo.0.wizz': '1',
> >'foo.1.bar': 'two',
> >'foo.1.wizz': '2'
> >  }
> >
> > will get turned into a dict with one element 'foo' whose
> > value is a list. The list elements will each in turn be
> > dicts.
> >
> >  {
> >'foo': [
> >  { 'bar': 'one', 'wizz': '1' },
> >  { 'bar': 'two', 'wizz': '2' }
> >],
> >  }
> >
> > If the key is intended to contain a literal '.', then it must
> > be escaped as '..'. ie a flat dict
> >
> >   {
> >  'foo..bar': 'wizz',
> >  'bar.foo..bar': 'eek',
> >  'bar.hello': 'world'
> >   }
> >
> > Will end up as
> >
> >   {
> >  'foo.bar': 'wizz',
> >  'bar': {
> > 'foo.bar': 'eek',
> > 'hello': 'world'
> >  }
> >   }
> >
> > The intent of this function is that it allows a set of QemuOpts
> > to be turned into a nested data structure that mirrors the nesting
> > used when the same object is defined over QMP.
> >
> > Reviewed-by: Eric Blake 
> > Reviewed-by: Kevin Wolf 
> > Reviewed-by: Marc-André Lureau 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  include/qapi/qmp/qdict.h |   1 +
> >  qobject/qdict.c  | 289 
> > +++
> >  tests/check-qdict.c  | 261 ++
> >  3 files changed, 551 insertions(+)
> >
> > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> > index 71b8eb0..e0d24e1 100644
> > --- a/include/qapi/qmp/qdict.h
> > +++ b/include/qapi/qmp/qdict.h
> > @@ -73,6 +73,7 @@ void qdict_flatten(QDict *qdict);
> >  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
> >  void qdict_array_split(QDict *src, QList **dst);
> >  int qdict_array_entries(QDict *src, const char *subqdict);
> > +QObject *qdict_crumple(const QDict *src, bool recursive, Error **errp);
> >  
> >  void qdict_join(QDict *dest, QDict *src, bool overwrite);
> >  
> > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > index 60f158c..c38e90e 100644
> > --- a/qobject/qdict.c
> > +++ b/qobject/qdict.c
> [...]
> > +/**
> > + * qdict_crumple:
> > + * @src: the original flat dictionary (only scalar values) to crumple
> > + * @recursive: true to recursively crumple nested dictionaries
> 
> Is recursive=false used outside tests in this series?

No, its not used.

It was suggested in a way earlier version by Max, but not sure if his
code uses it or not.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] fdc: Add a floppy qbus

[KONRAD Frederic]

Hi,

Le 20/10/2016 à 09:55, Kevin Wolf a écrit :

This adds a qbus to the floppy controller that should contain the floppy
drives eventually. At the moment it just exists and is empty.

Signed-off-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 hw/block/fdc.c | 40 +++-
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index b79873a..a3afb62 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -52,6 +52,33 @@
 }   \
 } while (0)

+
+//
+/* qdev floppy bus  */
+
+#define TYPE_FLOPPY_BUS "Floppy"


I don't remember of any TYPE_* starting with a uppercase. Maybe better 
using floppy? or floppy-bus?


Fred


+#define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
+
+typedef struct FDCtrl FDCtrl;
+
+typedef struct FloppyBus {
+BusState bus;
+FDCtrl *fdc;
+} FloppyBus;
+
+static const TypeInfo floppy_bus_info = {
+.name = TYPE_FLOPPY_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(FloppyBus),
+};
+
+static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
+{
+qbus_create_inplace(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL);
+bus->fdc = fdc;
+}
+
+
 //
 /* Floppy drive emulation   */

@@ -148,8 +175,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
 #define FD_SECTOR_SC   2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */

-typedef struct FDCtrl FDCtrl;
-
 /* Floppy disk drive emulation */
 typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
@@ -684,6 +709,7 @@ struct FDCtrl {
 /* Power down config (also with status regB access mode */
 uint8_t pwrd;
 /* Floppy drives */
+FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
 int reset_sensei;
@@ -2442,7 +2468,8 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
 *fdc_tc = qdev_get_gpio_in(dev, 0);
 }

-static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
+static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
+  Error **errp)
 {
 int i, j;
 static int command_tables_inited = 0;
@@ -2480,6 +2507,8 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error 
**errp)
 k->register_channel(fdctrl->dma, fdctrl->dma_chann,
 _transfer_handler, fdctrl);
 }
+
+floppy_bus_create(fdctrl, >bus, dev);
 fdctrl_connect_drives(fdctrl, errp);
 }

@@ -2508,7 +2537,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error 
**errp)
 }

 qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-fdctrl_realize_common(fdctrl, );
+fdctrl_realize_common(dev, fdctrl, );
 if (err != NULL) {
 error_propagate(errp, err);
 return;
@@ -2559,7 +2588,7 @@ static void sysbus_fdc_common_realize(DeviceState *dev, 
Error **errp)
 FDCtrlSysBus *sys = SYSBUS_FDC(dev);
 FDCtrl *fdctrl = >state;

-fdctrl_realize_common(fdctrl, errp);
+fdctrl_realize_common(dev, fdctrl, errp);
 }

 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
@@ -2744,6 +2773,7 @@ static void fdc_register_types(void)
 type_register_static(_fdc_type_info);
 type_register_static(_fdc_info);
 type_register_static(_fdc_info);
+type_register_static(_bus_info);
 }

 type_init(fdc_register_types)

[Qemu-block] [PATCH RFC 7/7] nbd/replication: implement .bdrv_get_info() for nbd and replication driver

[zhanghailiang]

Without this callback, there will be an error reports in the primary side:
"qemu-system-x86_64: Couldn't determine the cluster size of the target image,
which has no backing file: Operation not supported
Aborting, since this may create an unusable destination image"

For nbd driver, it doesn't have cluster size, so here we return
a fake value for it.

Signed-off-by: zhanghailiang 
Signed-off-by: Wen Congyang 
---
 block/nbd.c | 12 
 block/replication.c |  6 ++
 2 files changed, 18 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 6bc06d6..96d7023 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -40,6 +40,8 @@
 
 #define EN_OPTSTR ":exportname="
 
+#define NBD_FAKE_CLUSTER_SIZE 512
+
 typedef struct BDRVNBDState {
 NbdClientSession client;
 
@@ -483,6 +485,13 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
QDict *options)
 bs->full_open_options = opts;
 }
 
+static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+bdi->cluster_size  = NBD_FAKE_CLUSTER_SIZE;
+
+return 0;
+}
+
 static BlockDriver bdrv_nbd = {
 .format_name= "nbd",
 .protocol_name  = "nbd",
@@ -499,6 +508,7 @@ static BlockDriver bdrv_nbd = {
 .bdrv_detach_aio_context= nbd_detach_aio_context,
 .bdrv_attach_aio_context= nbd_attach_aio_context,
 .bdrv_refresh_filename  = nbd_refresh_filename,
+.bdrv_get_info  = nbd_get_info,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -517,6 +527,7 @@ static BlockDriver bdrv_nbd_tcp = {
 .bdrv_detach_aio_context= nbd_detach_aio_context,
 .bdrv_attach_aio_context= nbd_attach_aio_context,
 .bdrv_refresh_filename  = nbd_refresh_filename,
+.bdrv_get_info  = nbd_get_info,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -535,6 +546,7 @@ static BlockDriver bdrv_nbd_unix = {
 .bdrv_detach_aio_context= nbd_detach_aio_context,
 .bdrv_attach_aio_context= nbd_attach_aio_context,
 .bdrv_refresh_filename  = nbd_refresh_filename,
+.bdrv_get_info  = nbd_get_info,
 };
 
 static void bdrv_nbd_init(void)
diff --git a/block/replication.c b/block/replication.c
index e66b1ca..14c718e 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -707,6 +707,11 @@ static void replication_stop(ReplicationState *rs, bool 
failover, Error **errp)
 aio_context_release(aio_context);
 }
 
+static int replication_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
+{
+return bdrv_get_info(bs->file->bs, bdi);
+}
+
 BlockDriver bdrv_replication = {
 .format_name= "replication",
 .protocol_name  = "replication",
@@ -719,6 +724,7 @@ BlockDriver bdrv_replication = {
 .bdrv_co_readv  = replication_co_readv,
 .bdrv_co_writev = replication_co_writev,
 
+.bdrv_get_info  = replication_get_info,
 .is_filter  = true,
 .bdrv_recurse_is_first_non_filter = 
replication_recurse_is_first_non_filter,
 
-- 
1.8.3.1

[Qemu-block] [PATCH RFC 1/7] docs/block-replication: Add description for shared-disk case

[zhanghailiang]

Introuduce the scenario of shared-disk block replication
and how to use it.

Signed-off-by: zhanghailiang 
Signed-off-by: Wen Congyang 
Signed-off-by: Zhang Chen 
---
 docs/block-replication.txt | 131 +++--
 1 file changed, 127 insertions(+), 4 deletions(-)

diff --git a/docs/block-replication.txt b/docs/block-replication.txt
index 6bde673..97fcfc1 100644
--- a/docs/block-replication.txt
+++ b/docs/block-replication.txt
@@ -24,7 +24,7 @@ only dropped at next checkpoint time. To reduce the network 
transportation
 effort during a vmstate checkpoint, the disk modification operations of
 the Primary disk are asynchronously forwarded to the Secondary node.
 
-== Workflow ==
+== Non-shared disk workflow ==
 The following is the image of block replication workflow:
 
 +--+++
@@ -57,7 +57,7 @@ The following is the image of block replication workflow:
 4) Secondary write requests will be buffered in the Disk buffer and it
will overwrite the existing sector content in the buffer.
 
-== Architecture ==
+== None-shared disk architecture ==
 We are going to implement block replication from many basic
 blocks that are already in QEMU.
 
@@ -106,6 +106,74 @@ any state that would otherwise be lost by the speculative 
write-through
 of the NBD server into the secondary disk. So before block replication,
 the primary disk and secondary disk should contain the same data.
 
+== Shared Disk Mode Workflow ==
+The following is the image of block replication workflow:
+
++--+++
+|Primary Write Requests||Secondary Write Requests|
++--+++
+  |   |
+  |  (4)
+  |   V
+  |  /-\
+  | (2)Forward and write through | |
+  | +--> | Disk Buffer |
+  | || |
+  | |\-/
+  | |(1)read   |
+  | |  |
+   (3)write   | |  | backing file
+  V |  |
+ +-+   |
+ | Shared Disk | <-+
+ +-+
+
+1) Primary writes will read original data and forward it to Secondary
+   QEMU.
+2) Before Primary write requests are written to Shared disk, the
+   original sector content will be read from Shared disk and
+   forwarded and buffered in the Disk buffer on the secondary site,
+   but it will not overwrite the existing
+   sector content(it could be from either "Secondary Write Requests" or
+   previous COW of "Primary Write Requests") in the Disk buffer.
+3) Primary write requests will be written to Shared disk.
+4) Secondary write requests will be buffered in the Disk buffer and it
+   will overwrite the existing sector content in the buffer.
+
+== Shared Disk Mode Architecture ==
+We are going to implement block replication from many basic
+blocks that are already in QEMU.
+ virtio-blk ||   
.--
+ /  ||   | 
Secondary
+/   ||   
'--
+   /|| 
virtio-blk
+  / || 
 |
+  | ||   
replication(5)
+  |NBD  >   NBD   (2)  
 |
+  |  client ||server ---> hidden disk <-- 
active disk(4)
+  | ^   ||  |
+  |  replication(1) ||  |
+  | |   ||  |
+  |   +-'   ||  |
+ (3)  |drive-backup sync=none   ||  |
+. |   +-+   ||  |
+Primary | | |   ||   backing|
+' | |   ||  |
+  V |   |
+   

[Qemu-block] [PATCH RFC 5/7] replication: fix code logic with the new shared_disk option

[zhanghailiang]

Some code logic only be needed in non-shared disk, here
we adjust these codes to prepare for shared disk scenario.

Signed-off-by: zhanghailiang 
---
 block/replication.c | 44 ++--
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index d687ffc..39c616d 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -517,15 +517,21 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 bdrv_op_block_all(top_bs, s->blocker);
 bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-backup_start("replication-backup", s->secondary_disk->bs,
- s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL, false,
- BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
- backup_job_completed, s, NULL, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-backup_job_cleanup(s);
-aio_context_release(aio_context);
-return;
+/*
+ * Only in the case of non-shared disk,
+ * the backup job is in the Slave side
+ */
+if (!s->is_shared_disk) {
+backup_start("replication-backup", s->secondary_disk->bs,
+s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL, false,
+BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
+backup_job_completed, s, NULL, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+backup_job_cleanup(s);
+aio_context_release(aio_context);
+return;
+}
 }
 
 secondary_do_checkpoint(s, errp);
@@ -556,14 +562,16 @@ static void replication_do_checkpoint(ReplicationState 
*rs, Error **errp)
 case REPLICATION_MODE_PRIMARY:
 break;
 case REPLICATION_MODE_SECONDARY:
-if (!s->secondary_disk->bs->job) {
-error_setg(errp, "Backup job was cancelled unexpectedly");
-break;
-}
-backup_do_checkpoint(s->secondary_disk->bs->job, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-break;
+if (!s->is_shared_disk) {
+if (!s->secondary_disk->bs->job) {
+error_setg(errp, "Backup job was cancelled unexpectedly");
+break;
+}
+backup_do_checkpoint(s->secondary_disk->bs->job, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+break;
+}
 }
 secondary_do_checkpoint(s, errp);
 break;
@@ -644,7 +652,7 @@ static void replication_stop(ReplicationState *rs, bool 
failover, Error **errp)
  * before the BDS is closed, because we will access hidden
  * disk, secondary disk in backup_job_completed().
  */
-if (s->secondary_disk->bs->job) {
+if (!s->is_shared_disk && s->secondary_disk->bs->job) {
 block_job_cancel_sync(s->secondary_disk->bs->job);
 }
 
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v14 09/21] qapi: permit auto-creating single element lists

[Daniel P. Berrange]

On Wed, Oct 12, 2016 at 11:18:21AM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange"  writes:
> 
> > When converting QemuOpts to a QObject, there is no information
> > about compound types available,
> 
> Yes, that's a drawback of splitting the conversion into a QemuOpts ->
> QObject part that is oblivious of types, and a QObject -> QAPI object
> part that knows the types.
> 
> > so when visiting a list, the
> > corresponding QObject is not guaranteed to be a QList. We
> > therefore need to be able to auto-create a single element QList
> > from whatever type we find.
> >
> > This mode should only be enabled if you have compatibility
> > requirements for
> >
> >-arg foo=hello,foo=world
> >
> > to be treated as equivalent to the preferred syntax:
> >
> >-arg foo.0=hello,foo.1=world
> 
> Not sure this is "preferred".  "More powerfully warty" is probably
> closer to the truth ;)

Well, I call it "preferred" in the sense that that option syntax
directly maps to the QAPI syntax in an unambigous manner. ie
given the arg value alone "foo.0=hello,foo.1=world" you can clearly
determine that "foo" is a list. With the compat syntax you cannot
distinguish list from scalar, without knowing the QAPI schema.

> How is "-arg foo=hello,foo=world" treated if this mode isn't enabled?

The default behaviour would be that only the last key is present in
the dict, eg foo=world, and then if you tried to visit a list, the
visitor would complain that its got a QString instead of QList for
the key 'foo'.

This is related to patch 14

> What would be the drawbacks of doing this always instead of only when we
> "have compatibility requirements"?

Essentially we'd be permanently allowing 2 distinct syntaxes for
dealing with lists, for all options. I felt it desirable that we
have only a single syntax and only allow this alt syntax in the
backcompat cases.




> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index d9269c9..d88e9f9 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -48,6 +48,10 @@ struct QObjectInputVisitor
> >  
> >  /* True to reject parse in visit_end_struct() if unvisited keys 
> > remain. */
> >  bool strict;
> > +
> > +/* Whether we can auto-create single element lists when
> > + * encountering a non-QList type */
> > +bool autocreate_list;
> >  };
> >  
> >  static QObjectInputVisitor *to_qiv(Visitor *v)
> > @@ -108,6 +112,7 @@ static const QListEntry 
> > *qobject_input_push(QObjectInputVisitor *qiv,
> >  assert(obj);
> >  tos->obj = obj;
> >  tos->qapi = qapi;
> > +qobject_incref(obj);
> >  
> >  if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
> >  h = g_hash_table_new(g_str_hash, g_str_equal);
> > @@ -147,6 +152,7 @@ static void qobject_input_stack_object_free(StackObject 
> > *tos)
> >  if (tos->h) {
> >  g_hash_table_unref(tos->h);
> >  }
> > +qobject_decref(tos->obj);
> >  
> >  g_free(tos);
> >  }
> 
> Can you explain the reference counting change?

Previously the stack stored a borrowed reference, since it didn't
ever need responsibility for free'ing the object when popping the
stack. This is no longer the case if you look a few lines later


> 
> > @@ -197,7 +203,7 @@ static void qobject_input_start_list(Visitor *v, const 
> > char *name,
> >  QObject *qobj = qobject_input_get_object(qiv, name, true);
> >  const QListEntry *entry;
> >  
> > -if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> > +if (!qobj || (!qiv->autocreate_list && qobject_type(qobj) != 
> > QTYPE_QLIST)) {
> 
> Long line, but I believe it'll go away when you rebase for commit
> 1382d4a.
> 
> >  if (list) {
> >  *list = NULL;
> >  }
> > @@ -206,7 +212,16 @@ static void qobject_input_start_list(Visitor *v, const 
> > char *name,
> >  return;
> >  }
> >  
> > -entry = qobject_input_push(qiv, qobj, list, errp);
> > +if (qobject_type(qobj) != QTYPE_QLIST) {
> > +QList *tmplist = qlist_new();
> > +qlist_append_obj(tmplist, qobj);
> > +qobject_incref(qobj);
> > +entry = qobject_input_push(qiv, QOBJECT(tmplist), list, errp);
> > +QDECREF(tmplist);

... here we are storing the 'qmplist' in the stack, and so when
popping the stack, we must free that object. We thus need
the stack to always hold its own reference, so when popping
it can decref and (potentially) release the last reference.

> > +} else {
> > +entry = qobject_input_push(qiv, qobj, list, errp);
> > +}
> > +
> >  if (list) {
> >  if (entry) {
> >  *list = g_malloc0(size);
> 
> Buries autolist behavior in the middle of things.  What about doing it
> first, so it's more separate?

I'm not sure I understand what you mean here ?

> 
>QObjectInputVisitor *qiv = to_qiv(v);
>QObject *qobj = 

[Qemu-block] [PATCH RFC 3/7] replication: add shared-disk and shared-disk-id options

[zhanghailiang]

We use these two options to identify which disk is
shared

Signed-off-by: zhanghailiang 
Signed-off-by: Wen Congyang 
Signed-off-by: Zhang Chen 
---
 block/replication.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/block/replication.c b/block/replication.c
index 3bd1cf1..2a2fdb2 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -25,9 +25,12 @@
 typedef struct BDRVReplicationState {
 ReplicationMode mode;
 int replication_state;
+bool is_shared_disk;
+char *shared_disk_id;
 BdrvChild *active_disk;
 BdrvChild *hidden_disk;
 BdrvChild *secondary_disk;
+BdrvChild *primary_disk;
 char *top_id;
 ReplicationState *rs;
 Error *blocker;
@@ -53,6 +56,9 @@ static void replication_stop(ReplicationState *rs, bool 
failover,
 
 #define REPLICATION_MODE"mode"
 #define REPLICATION_TOP_ID  "top-id"
+#define REPLICATION_SHARED_DISK "shared-disk"
+#define REPLICATION_SHARED_DISK_ID "shared-disk-id"
+
 static QemuOptsList replication_runtime_opts = {
 .name = "replication",
 .head = QTAILQ_HEAD_INITIALIZER(replication_runtime_opts.head),
@@ -65,6 +71,14 @@ static QemuOptsList replication_runtime_opts = {
 .name = REPLICATION_TOP_ID,
 .type = QEMU_OPT_STRING,
 },
+{
+.name = REPLICATION_SHARED_DISK_ID,
+.type = QEMU_OPT_STRING,
+},
+{
+.name = REPLICATION_SHARED_DISK,
+.type = QEMU_OPT_BOOL,
+},
 { /* end of list */ }
 },
 };
@@ -85,6 +99,8 @@ static int replication_open(BlockDriverState *bs, QDict 
*options,
 QemuOpts *opts = NULL;
 const char *mode;
 const char *top_id;
+const char *shared_disk_id;
+BlockBackend *blk;
 
 ret = -EINVAL;
 opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
@@ -114,6 +130,22 @@ static int replication_open(BlockDriverState *bs, QDict 
*options,
"The option mode's value should be primary or secondary");
 goto fail;
 }
+s->is_shared_disk = qemu_opt_get_bool(opts, REPLICATION_SHARED_DISK,
+false);
+if (s->is_shared_disk && (s->mode == REPLICATION_MODE_PRIMARY)) {
+shared_disk_id = qemu_opt_get(opts, REPLICATION_SHARED_DISK_ID);
+if (!shared_disk_id) {
+error_setg(_err, "Missing shared disk blk");
+goto fail;
+}
+s->shared_disk_id = g_strdup(shared_disk_id);
+blk = blk_by_name(s->shared_disk_id);
+if (!blk) {
+error_setg(_err, "There is no %s block", s->shared_disk_id);
+goto fail;
+}
+s->primary_disk = blk_root(blk);
+}
 
 s->rs = replication_new(bs, _ops);
 
@@ -130,6 +162,7 @@ static void replication_close(BlockDriverState *bs)
 {
 BDRVReplicationState *s = bs->opaque;
 
+g_free(s->shared_disk_id);
 if (s->replication_state == BLOCK_REPLICATION_RUNNING) {
 replication_stop(s->rs, false, NULL);
 }
-- 
1.8.3.1

[Qemu-block] [PATCH RFC 6/7] replication: Implement block replication for shared disk case

[zhanghailiang]

Just as the scenario of non-shared disk block replication,
we are going to implement block replication from many basic
blocks that are already in QEMU.
The architecture is:

 virtio-blk ||   
.--
 /  ||   | 
Secondary
/   ||   
'--
   /|| 
virtio-blk
  / ||  
|
  | ||   
replication(5)
  |NBD  >   NBD   (2)   
|
  |  client ||server ---> hidden disk <-- 
active disk(4)
  | ^   ||  |
  |  replication(1) ||  |
  | |   ||  |
  |   +-'   ||  |
 (3)  |drive-backup sync=none   ||  |
. |   +-+   ||  |
Primary | | |   ||   backing|
' | |   ||  |
  V |   |
   +---+|
   |   shared disk | <--+
   +---+

1) Primary writes will read original data and forward it to Secondary
   QEMU.
2) The hidden-disk is created automatically. It buffers the original content
   that is modified by the primary VM. It should also be an empty disk, and
   the driver supports bdrv_make_empty() and backing file.
3) Primary write requests will be written to Shared disk.
4) Secondary write requests will be buffered in the active disk and it
   will overwrite the existing sector content in the buffer.

Signed-off-by: zhanghailiang 
Signed-off-by: Wen Congyang 
Signed-off-by: Zhang Chen 
---
 block/replication.c | 45 ++---
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 39c616d..e66b1ca 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -221,7 +221,7 @@ static coroutine_fn int 
replication_co_readv(BlockDriverState *bs,
  QEMUIOVector *qiov)
 {
 BDRVReplicationState *s = bs->opaque;
-BdrvChild *child = s->secondary_disk;
+BdrvChild *child = s->is_shared_disk ? s->primary_disk : s->secondary_disk;
 BlockJob *job = NULL;
 CowRequest req;
 int ret;
@@ -398,8 +398,12 @@ static void backup_job_completed(void *opaque, int ret)
 /* The backup job is cancelled unexpectedly */
 s->error = -EIO;
 }
-
-backup_job_cleanup(s);
+if (s->mode == REPLICATION_MODE_PRIMARY) {
+s->replication_state = BLOCK_REPLICATION_DONE;
+s->error = 0;
+} else {
+backup_job_cleanup(s);
+}
 }
 
 static bool check_top_bs(BlockDriverState *top_bs, BlockDriverState *bs)
@@ -450,6 +454,15 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 
 switch (s->mode) {
 case REPLICATION_MODE_PRIMARY:
+if (s->is_shared_disk) {
+backup_start("replication-backup", s->primary_disk->bs, bs, 0,
+MIRROR_SYNC_MODE_NONE, NULL, false, BLOCKDEV_ON_ERROR_REPORT,
+BLOCKDEV_ON_ERROR_REPORT, backup_job_completed,
+s, NULL, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+}
+}
 break;
 case REPLICATION_MODE_SECONDARY:
 s->active_disk = bs->file;
@@ -468,7 +481,8 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 }
 
 s->secondary_disk = s->hidden_disk->bs->backing;
-if (!s->secondary_disk->bs || !bdrv_has_blk(s->secondary_disk->bs)) {
+if (!s->secondary_disk->bs ||
+(!s->is_shared_disk && !bdrv_has_blk(s->secondary_disk->bs))) {
 error_setg(errp, "The secondary disk doesn't have block backend");
 aio_context_release(aio_context);
 return;
@@ -560,11 +574,24 @@ static void replication_do_checkpoint(ReplicationState 
*rs, Error **errp)
 
 switch (s->mode) {
 case REPLICATION_MODE_PRIMARY:
+if (s->is_shared_disk) {
+if (!s->primary_disk->bs->job) {
+error_setg(errp, "Primary backup job was cancelled"
+   " unexpectedly");
+break;
+}
+
+

[Qemu-block] [PATCH RFC 2/7] block-backend: Introduce blk_root() helper

[zhanghailiang]

With this helper function, we can get the BdrvChild struct
from BlockBackend

Signed-off-by: zhanghailiang 
---
 block/block-backend.c  | 5 +
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1a724a8..66387f0 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -389,6 +389,11 @@ BlockDriverState *blk_bs(BlockBackend *blk)
 return blk->root ? blk->root->bs : NULL;
 }
 
+BdrvChild *blk_root(BlockBackend *blk)
+{
+return blk->root;
+}
+
 static BlockBackend *bdrv_first_blk(BlockDriverState *bs)
 {
 BdrvChild *child;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b07159b..867f9f5 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -99,6 +99,7 @@ void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 bool bdrv_has_blk(BlockDriverState *bs);
 bool bdrv_is_root_node(BlockDriverState *bs);
+BdrvChild *blk_root(BlockBackend *blk);
 
 void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow);
 void blk_iostatus_enable(BlockBackend *blk);
-- 
1.8.3.1

[Qemu-block] [PATCH RFC 4/7] replication: Split out backup_do_checkpoint() from secondary_do_checkpoint()

[zhanghailiang]

The helper backup_do_checkpoint() will be used for primary related
codes. Here we split it out from secondary_do_checkpoint().

Besides, it is unnecessary to call backup_do_checkpoint() in
replication starting and normally stop replication path.
We only need call it while do real checkpointing.

Signed-off-by: zhanghailiang 
---
 block/replication.c | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 2a2fdb2..d687ffc 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -320,20 +320,8 @@ static bool 
replication_recurse_is_first_non_filter(BlockDriverState *bs,
 
 static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
 {
-Error *local_err = NULL;
 int ret;
 
-if (!s->secondary_disk->bs->job) {
-error_setg(errp, "Backup job was cancelled unexpectedly");
-return;
-}
-
-backup_do_checkpoint(s->secondary_disk->bs->job, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return;
-}
-
 ret = s->active_disk->bs->drv->bdrv_make_empty(s->active_disk->bs);
 if (ret < 0) {
 error_setg(errp, "Cannot make active disk empty");
@@ -539,6 +527,8 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 aio_context_release(aio_context);
 return;
 }
+
+secondary_do_checkpoint(s, errp);
 break;
 default:
 aio_context_release(aio_context);
@@ -547,10 +537,6 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
 
 s->replication_state = BLOCK_REPLICATION_RUNNING;
 
-if (s->mode == REPLICATION_MODE_SECONDARY) {
-secondary_do_checkpoint(s, errp);
-}
-
 s->error = 0;
 aio_context_release(aio_context);
 }
@@ -560,13 +546,29 @@ static void replication_do_checkpoint(ReplicationState 
*rs, Error **errp)
 BlockDriverState *bs = rs->opaque;
 BDRVReplicationState *s;
 AioContext *aio_context;
+Error *local_err = NULL;
 
 aio_context = bdrv_get_aio_context(bs);
 aio_context_acquire(aio_context);
 s = bs->opaque;
 
-if (s->mode == REPLICATION_MODE_SECONDARY) {
+switch (s->mode) {
+case REPLICATION_MODE_PRIMARY:
+break;
+case REPLICATION_MODE_SECONDARY:
+if (!s->secondary_disk->bs->job) {
+error_setg(errp, "Backup job was cancelled unexpectedly");
+break;
+}
+backup_do_checkpoint(s->secondary_disk->bs->job, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+break;
+}
 secondary_do_checkpoint(s, errp);
+break;
+default:
+abort();
 }
 aio_context_release(aio_context);
 }
-- 
1.8.3.1

[Qemu-block] [PATCH RFC 0/7] COLO block replication supports shared disk case

[zhanghailiang]

COLO block replication doesn't support the shared disk case,
Here we try to implement it.

Just as the scenario of non-shared disk block replication,
we are going to implement block replication from many basic
blocks that are already in QEMU.
The architecture is:

 virtio-blk ||   
.--
 /  ||   | 
Secondary
/   ||   
'--
   /|| 
virtio-blk
  / ||  
|
  | ||   
replication(5)
  |NBD  >   NBD   (2)   
|
  |  client ||server ---> hidden disk <-- 
active disk(4)
  | ^   ||  |
  |  replication(1) ||  |
  | |   ||  |
  |   +-'   ||  |
 (3)  |drive-backup sync=none   ||  |
. |   +-+   ||  |
Primary | | |   ||   backing|
' | |   ||  |
  V |   |
   +---+|
   |   shared disk | <--+
   +---+
1) Primary writes will read original data and forward it to Secondary
   QEMU.
2) The hidden-disk will buffers the original content that is modified
   by the primary VM. It should also be an empty disk, and
   the driver supports bdrv_make_empty() and backing file.
3) Primary write requests will be written to Shared disk.
4) Secondary write requests will be buffered in the active disk and it
   will overwrite the existing sector content in the buffe

For more details, please refer to patch 1.

The complete codes can be found from the link:
https://github.com/coloft/qemu/tree/colo-v5.1-developing-COLO-frame-v21-with-shared-disk

Test steps:
1. Secondary:
# x86_64-softmmu/qemu-system-x86_64 -boot c -m 2048 -smp 2 -qmp stdio -vnc :9 
-name secondary -enable-kvm -cpu qemu64,+kvmclock -device piix3-usb-uhci -drive 
if=none,driver=qcow2,file.filename=/mnt/ramfs/hidden_disk.img,id=hidden_disk0,backing.driver=raw,backing.file.filename=/work/kvm/suse11_sp3_64
  -drive 
if=virtio,id=active-disk0,driver=replication,mode=secondary,file.driver=qcow2,top-id=active-disk0,file.file.filename=/mnt/ramfs/active_disk.img,file.backing=hidden_disk0,shared-disk=on
 -incoming tcp:0:

Issue qmp commands:
{'execute':'qmp_capabilities'}
{'execute': 'nbd-server-start', 'arguments': {'addr': {'type': 'inet', 'data': 
{'host': '0', 'port': '9998'} } } }
{'execute': 'nbd-server-add', 'arguments': {'device': 'hidden_disk0', 
'writable': true } }

2.Primary:
# x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 2048 -smp 2 -qmp stdio -vnc 
:9 -name primary -cpu qemu64,+kvmclock -device piix3-usb-uhci -drive 
if=virtio,id=primary_disk0,file.filename=/work/kvm/suse11_sp3_64,driver=raw -S

Issue qmp commands:
{'execute':'qmp_capabilities'}
{'execute': 'human-monitor-command', 'arguments': {'command-line': 'drive_add 
-n buddy 
driver=replication,mode=primary,file.driver=nbd,file.host=9.42.3.17,file.port=9998,file.export=hidden_disk0,shared-disk-id=primary_disk0,shared-disk=on,node-name=rep'}}
{'execute': 'migrate-set-capabilities', 'arguments': {'capabilities': [ 
{'capability': 'x-colo', 'state': true } ] } }
{'execute': 'migrate', 'arguments': {'uri': 'tcp:9.42.3.17:' } }

3. Failover
Secondary side:
Issue qmp commands:
{ 'execute': 'nbd-server-stop' }
{ "execute": "x-colo-lost-heartbeat" }

Please review and any commits are welcomed.

Cc: Juan Quintela 
Cc: Amit Shah  
Cc: Dr. David Alan Gilbert (git) 

zhanghailiang (7):
  docs/block-replication: Add description for shared-disk case
  block-backend: Introduce blk_root() helper
  replication: add shared-disk and shared-disk-id options
  replication: Split out backup_do_checkpoint() from
secondary_do_checkpoint()
  replication: fix code logic with the new shared_disk option
  replication: Implement block replication for shared disk case
  nbd/replication: implement .bdrv_get_info() for nbd and replication
driver

 block/block-backend.c  |   5 ++
 block/nbd.c|  12 
 block/replication.c| 146 +++--
 docs/block-replication.txt | 131 ++--
 include/sysemu/block-backend.h |   1 +
 5 files changed, 258 

[Qemu-block] [PATCH 8/9] raw: Implement .bdrv_co_ioctl instead of .bdrv_aio_ioctl

[Kevin Wolf]

It's the simpler interface to use for the raw format driver.

Apart from that, this removes the last user of the AIO emulation
implemented by bdrv_aio_ioctl().

Signed-off-by: Kevin Wolf 
---
 block/raw_bsd.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 588d408..fc16ec1 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -176,12 +176,9 @@ static void raw_lock_medium(BlockDriverState *bs, bool 
locked)
 bdrv_lock_medium(bs->file->bs, locked);
 }
 
-static BlockAIOCB *raw_aio_ioctl(BlockDriverState *bs,
- unsigned long int req, void *buf,
- BlockCompletionFunc *cb,
- void *opaque)
+static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
 {
-return bdrv_aio_ioctl(bs->file->bs, req, buf, cb, opaque);
+return bdrv_co_ioctl(bs->file->bs, req, buf);
 }
 
 static int raw_has_zero_init(BlockDriverState *bs)
@@ -261,7 +258,7 @@ BlockDriver bdrv_raw = {
 .bdrv_media_changed   = _media_changed,
 .bdrv_eject   = _eject,
 .bdrv_lock_medium = _lock_medium,
-.bdrv_aio_ioctl   = _aio_ioctl,
+.bdrv_co_ioctl= _co_ioctl,
 .create_opts  = _create_opts,
 .bdrv_has_zero_init   = _has_zero_init
 };
-- 
1.8.3.1

[Qemu-block] [PATCH 4/9] block: Use blk_co_ioctl() for all BB level ioctls

[Kevin Wolf]

All read/write functions already have a single coroutine-based function
on the BlockBackend level through which all requests go (no matter what
API style the external caller used) and which passes the requests down
to the block node level.

This patch exports a bdrv_co_ioctl() function and uses it to extend this
mode of operation to ioctls.

Signed-off-by: Kevin Wolf 
---
 block/block-backend.c  | 39 +--
 block/io.c |  8 
 include/block/block.h  |  1 +
 include/sysemu/block-backend.h |  1 +
 4 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 39336de..c53ca30 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1141,23 +1141,50 @@ void blk_aio_cancel_async(BlockAIOCB *acb)
 bdrv_aio_cancel_async(acb);
 }
 
-int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
+int blk_co_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
 {
 if (!blk_is_available(blk)) {
 return -ENOMEDIUM;
 }
 
-return bdrv_ioctl(blk_bs(blk), req, buf);
+return bdrv_co_ioctl(blk_bs(blk), req, buf);
+}
+
+static void blk_ioctl_entry(void *opaque)
+{
+BlkRwCo *rwco = opaque;
+rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset,
+ rwco->qiov->iov[0].iov_base);
+}
+
+int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
+{
+return blk_prw(blk, req, buf, 0, blk_ioctl_entry, 0);
+}
+
+static void blk_aio_ioctl_entry(void *opaque)
+{
+BlkAioEmAIOCB *acb = opaque;
+BlkRwCo *rwco = >rwco;
+
+rwco->ret = blk_co_ioctl(rwco->blk, rwco->offset,
+ rwco->qiov->iov[0].iov_base);
+blk_aio_complete(acb);
 }
 
 BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
   BlockCompletionFunc *cb, void *opaque)
 {
-if (!blk_is_available(blk)) {
-return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
-}
+QEMUIOVector qiov;
+struct iovec iov;
+
+iov = (struct iovec) {
+.iov_base = buf,
+.iov_len = 0,
+};
+qemu_iovec_init_external(, , 1);
 
-return bdrv_aio_ioctl(blk_bs(blk), req, buf, cb, opaque);
+return blk_aio_prwv(blk, req, 0, , blk_aio_ioctl_entry, 0, cb, 
opaque);
 }
 
 int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count)
diff --git a/block/io.c b/block/io.c
index ff93ba1..7c119d5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2492,7 +2492,7 @@ int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, 
int count)
 return rwco.ret;
 }
 
-static int bdrv_co_do_ioctl(BlockDriverState *bs, int req, void *buf)
+int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
 {
 BlockDriver *drv = bs->drv;
 BdrvTrackedRequest tracked_req;
@@ -2528,7 +2528,7 @@ typedef struct {
 static void coroutine_fn bdrv_co_ioctl_entry(void *opaque)
 {
 BdrvIoctlCoData *data = opaque;
-data->ret = bdrv_co_do_ioctl(data->bs, data->req, data->buf);
+data->ret = bdrv_co_ioctl(data->bs, data->req, data->buf);
 }
 
 /* needed for generic scsi interface */
@@ -2558,8 +2558,8 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int 
req, void *buf)
 static void coroutine_fn bdrv_co_aio_ioctl_entry(void *opaque)
 {
 BlockAIOCBCoroutine *acb = opaque;
-acb->req.error = bdrv_co_do_ioctl(acb->common.bs,
-  acb->req.req, acb->req.buf);
+acb->req.error = bdrv_co_ioctl(acb->common.bs,
+   acb->req.req, acb->req.buf);
 bdrv_co_complete(acb);
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index 99a15a6..e06db62 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -318,6 +318,7 @@ void bdrv_aio_cancel(BlockAIOCB *acb);
 void bdrv_aio_cancel_async(BlockAIOCB *acb);
 
 /* sg packet commands */
+int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf);
 int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf);
 BlockAIOCB *bdrv_aio_ioctl(BlockDriverState *bs,
 unsigned long int req, void *buf,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b07159b..6444e41 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -146,6 +146,7 @@ BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t 
offset, int count,
  BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
+int blk_co_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
 BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
   BlockCompletionFunc *cb, void *opaque);
-- 
1.8.3.1

[Qemu-block] [PATCH 7/9] block: Introduce .bdrv_co_ioctl() driver callback

[Kevin Wolf]

This allows drivers to implement ioctls in a coroutine-based way.

Signed-off-by: Kevin Wolf 
---
 block/io.c| 16 ++--
 include/block/block_int.h |  2 ++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index 35fdcca..370c7d8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2502,17 +2502,21 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void 
*buf)
 BlockAIOCB *acb;
 
 tracked_request_begin(_req, bs, 0, 0, BDRV_TRACKED_IOCTL);
-if (!drv || !drv->bdrv_aio_ioctl) {
+if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
 co.ret = -ENOTSUP;
 goto out;
 }
 
-acb = drv->bdrv_aio_ioctl(bs, req, buf, bdrv_co_io_em_complete, );
-if (!acb) {
-co.ret = -ENOTSUP;
-goto out;
+if (drv->bdrv_co_ioctl) {
+co.ret = drv->bdrv_co_ioctl(bs, req, buf);
+} else {
+acb = drv->bdrv_aio_ioctl(bs, req, buf, bdrv_co_io_em_complete, );
+if (!acb) {
+co.ret = -ENOTSUP;
+goto out;
+}
+qemu_coroutine_yield();
 }
-qemu_coroutine_yield();
 out:
 tracked_request_end(_req);
 return co.ret;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3e79228..e96e9ad 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -244,6 +244,8 @@ struct BlockDriver {
 BlockAIOCB *(*bdrv_aio_ioctl)(BlockDriverState *bs,
 unsigned long int req, void *buf,
 BlockCompletionFunc *cb, void *opaque);
+int coroutine_fn (*bdrv_co_ioctl)(BlockDriverState *bs,
+  unsigned long int req, void *buf);
 
 /* List of options for creating images, terminated by name == NULL */
 QemuOptsList *create_opts;
-- 
1.8.3.1

[Qemu-block] [PATCH 1/9] block: Use blk_co_flush() for all BB level flushes

[Kevin Wolf]

All read/write functions already have a single coroutine-based function
on the BlockBackend level through which all requests go (no matter what
API style the external caller used) and which passes the requests down
to the block node level.

This patch extends this mode of operation to flushes.

Signed-off-by: Kevin Wolf 
---
 block/block-backend.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1a724a8..96bb634 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1099,14 +1099,19 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t 
offset,
 blk_aio_write_entry, flags, cb, opaque);
 }
 
+static void blk_aio_flush_entry(void *opaque)
+{
+BlkAioEmAIOCB *acb = opaque;
+BlkRwCo *rwco = >rwco;
+
+rwco->ret = blk_co_flush(rwco->blk);
+blk_aio_complete(acb);
+}
+
 BlockAIOCB *blk_aio_flush(BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque)
 {
-if (!blk_is_available(blk)) {
-return blk_abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
-}
-
-return bdrv_aio_flush(blk_bs(blk), cb, opaque);
+return blk_aio_prwv(blk, 0, 0, NULL, blk_aio_flush_entry, 0, cb, opaque);
 }
 
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
@@ -1169,13 +1174,15 @@ int blk_co_flush(BlockBackend *blk)
 return bdrv_co_flush(blk_bs(blk));
 }
 
-int blk_flush(BlockBackend *blk)
+static void blk_flush_entry(void *opaque)
 {
-if (!blk_is_available(blk)) {
-return -ENOMEDIUM;
-}
+BlkRwCo *rwco = opaque;
+rwco->ret = blk_co_flush(rwco->blk);
+}
 
-return bdrv_flush(blk_bs(blk));
+int blk_flush(BlockBackend *blk)
+{
+return blk_prw(blk, 0, NULL, 0, blk_flush_entry, 0);
 }
 
 void blk_drain(BlockBackend *blk)
-- 
1.8.3.1

[Qemu-block] [PATCH 5/9] raw-posix: Don't use bdrv_ioctl()

[Kevin Wolf]

Instead of letting raw-posix use the bdrv_ioctl() abstraction to issue
an ioctl to itself, just call ioctl() directly.

Signed-off-by: Kevin Wolf 
---
 block/raw-posix.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index f481e57..247e47b 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2069,13 +2069,23 @@ static bool hdev_is_sg(BlockDriverState *bs)
 
 #if defined(__linux__)
 
+BDRVRawState *s = bs->opaque;
 struct stat st;
 struct sg_scsi_id scsiid;
 int sg_version;
+int ret;
+
+if (stat(bs->filename, ) < 0 || !S_ISCHR(st.st_mode)) {
+return false;
+}
 
-if (stat(bs->filename, ) >= 0 && S_ISCHR(st.st_mode) &&
-!bdrv_ioctl(bs, SG_GET_VERSION_NUM, _version) &&
-!bdrv_ioctl(bs, SG_GET_SCSI_ID, )) {
+ret = ioctl(s->fd, SG_GET_VERSION_NUM, _version);
+if (ret < 0) {
+return false;
+}
+
+ret = ioctl(s->fd, SG_GET_SCSI_ID, );
+if (ret >= 0) {
 DPRINTF("SG device found: type=%d, version=%d\n",
 scsiid.scsi_type, sg_version);
 return true;
-- 
1.8.3.1

[Qemu-block] [PATCH 3/9] block: Remove bdrv_aio_pdiscard()

[Kevin Wolf]

It is unused now.

Signed-off-by: Kevin Wolf 
---
 block/io.c| 29 -
 block/trace-events|  1 -
 include/block/block.h |  3 ---
 3 files changed, 33 deletions(-)

diff --git a/block/io.c b/block/io.c
index b136c89..ff93ba1 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2196,35 +2196,6 @@ BlockAIOCB *bdrv_aio_flush(BlockDriverState *bs,
 return >common;
 }
 
-static void coroutine_fn bdrv_aio_pdiscard_co_entry(void *opaque)
-{
-BlockAIOCBCoroutine *acb = opaque;
-BlockDriverState *bs = acb->common.bs;
-
-acb->req.error = bdrv_co_pdiscard(bs, acb->req.offset, acb->req.bytes);
-bdrv_co_complete(acb);
-}
-
-BlockAIOCB *bdrv_aio_pdiscard(BlockDriverState *bs, int64_t offset, int count,
-  BlockCompletionFunc *cb, void *opaque)
-{
-Coroutine *co;
-BlockAIOCBCoroutine *acb;
-
-trace_bdrv_aio_pdiscard(bs, offset, count, opaque);
-
-acb = qemu_aio_get(_em_co_aiocb_info, bs, cb, opaque);
-acb->need_bh = true;
-acb->req.error = -EINPROGRESS;
-acb->req.offset = offset;
-acb->req.bytes = count;
-co = qemu_coroutine_create(bdrv_aio_pdiscard_co_entry, acb);
-qemu_coroutine_enter(co);
-
-bdrv_co_maybe_schedule_bh(acb);
-return >common;
-}
-
 void *qemu_aio_get(const AIOCBInfo *aiocb_info, BlockDriverState *bs,
BlockCompletionFunc *cb, void *opaque)
 {
diff --git a/block/trace-events b/block/trace-events
index 05fa13c..aff8a96 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -9,7 +9,6 @@ blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int 
bytes, int flags
 blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int 
flags) "blk %p bs %p offset %"PRId64" bytes %u flags %x"
 
 # block/io.c
-bdrv_aio_pdiscard(void *bs, int64_t offset, int count, void *opaque) "bs %p 
offset %"PRId64" count %d opaque %p"
 bdrv_aio_flush(void *bs, void *opaque) "bs %p opaque %p"
 bdrv_aio_readv(void *bs, int64_t sector_num, int nb_sectors, void *opaque) "bs 
%p sector_num %"PRId64" nb_sectors %d opaque %p"
 bdrv_aio_writev(void *bs, int64_t sector_num, int nb_sectors, void *opaque) 
"bs %p sector_num %"PRId64" nb_sectors %d opaque %p"
diff --git a/include/block/block.h b/include/block/block.h
index 107c603..99a15a6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -314,9 +314,6 @@ BlockAIOCB *bdrv_aio_writev(BdrvChild *child, int64_t 
sector_num,
 BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *bdrv_aio_flush(BlockDriverState *bs,
BlockCompletionFunc *cb, void *opaque);
-BlockAIOCB *bdrv_aio_pdiscard(BlockDriverState *bs,
-  int64_t offset, int count,
-  BlockCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockAIOCB *acb);
 void bdrv_aio_cancel_async(BlockAIOCB *acb);
 
-- 
1.8.3.1

[Qemu-block] [PATCH 0/9] block-backend: Use coroutine for flush/discard/ioctl

[Kevin Wolf]

Paolo, this is my attempt at implementing what you were asking for last Friday.
I converted blk_(co_)flush/pdiscard/ioctl so that all interfaces (coroutine,
AIO, sync) go through the same coroutine-based function already on the
BlockBackend level. Where it was reasonably easy, I also removed the
corresponding emulations from block/io.c IIUC, this should cover your immediate
needs.


Function to remove this series leaves for another day:

* bdrv_aio_flush (used by blkdebug, blkverify, qed)
* bdrv_flush (even more users)
* bdrv_pdiscard (used by qcow2)


BlockDriver callbacks to remove left for another day:

* bdrv_aio_pdiscard (implemented by raw-posix and rbd)
* bdrv_aio_ioctl (implemented by raw-posix and iscsi)

In both cases, raw-posix is trivial to covert, but iscsi and rbd feel rather
scary without a proper test setup.


Kevin Wolf (9):
  block: Use blk_co_flush() for all BB level flushes
  block: Use blk_co_pdiscard() for all BB level discard
  block: Remove bdrv_aio_pdiscard()
  block: Use blk_co_ioctl() for all BB level ioctls
  raw-posix: Don't use bdrv_ioctl()
  block: Remove bdrv_ioctl()
  block: Introduce .bdrv_co_ioctl() driver callback
  raw: Implement .bdrv_co_ioctl instead of .bdrv_aio_ioctl
  block: Remove bdrv_aio_ioctl()

 block/block-backend.c  |  94 --
 block/io.c | 111 -
 block/raw-posix.c  |  16 --
 block/raw_bsd.c|   9 ++--
 block/trace-events |   1 -
 include/block/block.h  |   8 +--
 include/block/block_int.h  |   2 +
 include/sysemu/block-backend.h |   1 +
 8 files changed, 98 insertions(+), 144 deletions(-)

-- 
1.8.3.1

[Qemu-block] [PATCH 2/9] block: Use blk_co_pdiscard() for all BB level discard

[Kevin Wolf]

All read/write functions already have a single coroutine-based function
on the BlockBackend level through which all requests go (no matter what
API style the external caller used) and which passes the requests down
to the block node level.

This patch extends this mode of operation to discards.

Signed-off-by: Kevin Wolf 
---
 block/block-backend.c | 30 ++
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 96bb634..39336de 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1114,16 +1114,21 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
 return blk_aio_prwv(blk, 0, 0, NULL, blk_aio_flush_entry, 0, cb, opaque);
 }
 
+static void blk_aio_pdiscard_entry(void *opaque)
+{
+BlkAioEmAIOCB *acb = opaque;
+BlkRwCo *rwco = >rwco;
+
+rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, acb->bytes);
+blk_aio_complete(acb);
+}
+
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk,
  int64_t offset, int count,
  BlockCompletionFunc *cb, void *opaque)
 {
-int ret = blk_check_byte_request(blk, offset, count);
-if (ret < 0) {
-return blk_abort_aio_request(blk, cb, opaque, ret);
-}
-
-return bdrv_aio_pdiscard(blk_bs(blk), offset, count, cb, opaque);
+return blk_aio_prwv(blk, offset, count, NULL, blk_aio_pdiscard_entry, 0,
+cb, opaque);
 }
 
 void blk_aio_cancel(BlockAIOCB *acb)
@@ -1562,14 +1567,15 @@ int blk_truncate(BlockBackend *blk, int64_t offset)
 return bdrv_truncate(blk_bs(blk), offset);
 }
 
-int blk_pdiscard(BlockBackend *blk, int64_t offset, int count)
+static void blk_pdiscard_entry(void *opaque)
 {
-int ret = blk_check_byte_request(blk, offset, count);
-if (ret < 0) {
-return ret;
-}
+BlkRwCo *rwco = opaque;
+rwco->ret = blk_co_pdiscard(rwco->blk, rwco->offset, rwco->qiov->size);
+}
 
-return bdrv_pdiscard(blk_bs(blk), offset, count);
+int blk_pdiscard(BlockBackend *blk, int64_t offset, int count)
+{
+return blk_prw(blk, offset, NULL, count, blk_pdiscard_entry, 0);
 }
 
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v14 20/21] net: convert to QObjectInputVisitor for -net/-netdev parsing

[Eric Blake]

On 10/20/2016 02:38 AM, Markus Armbruster wrote:

>> @@ -1069,7 +1069,21 @@ int net_client_init(QemuOpts *opts, bool is_netdev, 
>> Error **errp)
>>  void *object = NULL;
>>  Error *err = NULL;
>>  int ret = -1;
>> -Visitor *v = opts_visitor_new(opts);
>> +/*
>> + * Needs autocreate_lists=true in order support existing
>> + * syntax for list options where the bare key is repeated
>> + *
>> + * Needs autocreate_struct_levels=3 in order to deal with
>> + * 3 level nesting in NetLegacy option args, which was
>> + * exposed as a flat namespace with OptVisitor
>> + */
>> +Visitor *v = qobject_input_visitor_new_opts(opts, true, 3, false, true,
>> +);
>> +
>> +if (err) {
>> +error_propagate(errp, err);
>> +return -1;
>> +}
>>  
>>  {
>>  /* Parse convenience option format ip6-net=fec0::0[/64] */
> 
> Neither NetLegacy nor Netdev are ABI, so if I understand the problem,
> perhaps I can find a way around it.  Let's figure out what exactly
> requires levels=3.

Netdev is not ABI only because we decided to NOT apply the last patch of
QAPI-fying it in 2.7 while deciding to handle the back-compat
(non-?)issues that existing netdev_add QMP command accepts both 1 and
"1", but using the Netdev type would accept only 1.

While working towards making netdev_add use QAPI, I intentionally left
NetLegacy unchanged; but since NetLegacy is solely used by the command
line, feel free to rearrange that type as long as CLI back-compat is kept.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

On 01.10.2016 19:26, Max Reitz wrote:

On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are


[...]


diff --git a/block/qcow2.c b/block/qcow2.c
index 08c4ef9..02ec224 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -213,6 +213,11 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
  s->bitmap_directory_size =
  bitmaps_ext.bitmap_directory_size;
  
+ret = qcow2_read_bitmaps(bs, errp);

+if (ret < 0) {
+return ret;
+}
+
I think I'd put this directly into qcow2_open(), just like
qcow2_read_snapshots(); but that's an optional suggestion.

Max




Snapshots are not header extension.. so it is not the case. Here 
qcow2_read_bitmaps looks like part of header extension loading, and 
header extension fields describe other parts of the extension.. I think 
this is a good point, isn't it?


--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/4] fdc: Add a floppy qbus

[Kevin Wolf]

Am 20.10.2016 um 10:06 hat KONRAD Frederic geschrieben:
> Hi,
> 
> Le 20/10/2016 à 09:55, Kevin Wolf a écrit :
> >This adds a qbus to the floppy controller that should contain the floppy
> >drives eventually. At the moment it just exists and is empty.
> >
> >Signed-off-by: Kevin Wolf 
> >Reviewed-by: John Snow 
> >---
> > hw/block/fdc.c | 40 +++-
> > 1 file changed, 35 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> >index b79873a..a3afb62 100644
> >--- a/hw/block/fdc.c
> >+++ b/hw/block/fdc.c
> >@@ -52,6 +52,33 @@
> > }   \
> > } while (0)
> >
> >+
> >+//
> >+/* qdev floppy bus  */
> >+
> >+#define TYPE_FLOPPY_BUS "Floppy"
> 
> I don't remember of any TYPE_* starting with a uppercase. Maybe
> better using floppy? or floppy-bus?

This patch is mostly modelled after IDE. The block related ones are:

include/hw/ide/internal.h:#define TYPE_IDE_BUS "IDE"
include/hw/scsi/scsi.h:#define TYPE_SCSI_BUS "SCSI"

There are more uppercase ones (even non-acronyms), but I admit that I
didn't check for other users but just did what IDE does.

Kevin

Re: [Qemu-block] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Kevin Wolf]

Am 20.10.2016 um 10:25 hat Alberto Garcia geschrieben:
> On Wed 19 Oct 2016 07:11:20 PM CEST, Kevin Wolf wrote:
> >> bdrv_drain_all() doesn't allow the caller to do anything after all
> >> pending requests have been completed but before block jobs are
> >> resumed.
> >> 
> >> This patch splits bdrv_drain_all() into _begin() and _end() for that
> >> purpose. It also adds aio_{disable,enable}_external() calls to
> >> disable external clients in the meantime.
> >> 
> >> Signed-off-by: Alberto Garcia 
> >
> > This looks okay as a first step, possibly enough for this series
> > (we'll have to review this carefully), but it leaves us with a rather
> > limited version of bdrv_drain_all_begin/end that excludes many useful
> > cases. One of them is that John wants to use it around QMP
> > transactions.
> >
> > Specifically, you can't add a new BDS or a new block job in a
> > drain_all section because then bdrv_drain_all_end() would try to
> > unpause the new thing even though it has never been paused. Depending
> > on what else we did with it, this will either corrupt the pause
> > counters or just directly fail an assertion.
> 
> The problem is: do you want to be able to create a new block job and let
> it run? Because then you can end up having the same problem that this
> patch is trying to prevent if the new job attempts to reopen a
> BlockDriverState.

No, as I wrote it would have to be automatically paused on creation if
it is created in a drained_all section. It would only actually start to
run after bdrv_drain_all_end().

Kevin

Re: [Qemu-block] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Alberto Garcia]

On Wed 19 Oct 2016 07:11:20 PM CEST, Kevin Wolf wrote:

>> bdrv_drain_all() doesn't allow the caller to do anything after all
>> pending requests have been completed but before block jobs are
>> resumed.
>> 
>> This patch splits bdrv_drain_all() into _begin() and _end() for that
>> purpose. It also adds aio_{disable,enable}_external() calls to
>> disable external clients in the meantime.
>> 
>> Signed-off-by: Alberto Garcia 
>
> This looks okay as a first step, possibly enough for this series
> (we'll have to review this carefully), but it leaves us with a rather
> limited version of bdrv_drain_all_begin/end that excludes many useful
> cases. One of them is that John wants to use it around QMP
> transactions.
>
> Specifically, you can't add a new BDS or a new block job in a
> drain_all section because then bdrv_drain_all_end() would try to
> unpause the new thing even though it has never been paused. Depending
> on what else we did with it, this will either corrupt the pause
> counters or just directly fail an assertion.

The problem is: do you want to be able to create a new block job and let
it run? Because then you can end up having the same problem that this
patch is trying to prevent if the new job attempts to reopen a
BlockDriverState.

Berto

[Qemu-block] [PATCH v4 4/4] qemu-iotests: Test creating floppy drives

[Kevin Wolf]

This tests the different supported methods to create floppy drives and
how they interact.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1170 
 tests/qemu-iotests/group   |1 +
 3 files changed, 1413 insertions(+)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
new file mode 100755
index 000..88619ab
--- /dev/null
+++ b/tests/qemu-iotests/172
@@ -0,0 +1,242 @@
+#!/bin/bash
+#
+# Test floppy configuration
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
+function do_run_qemu()
+{
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -serial none "$@"
+echo
+}
+
+function check_floppy_qtree()
+{
+echo
+echo Testing: "$@" | _filter_testdir
+
+# QEMU_OPTIONS contains -nodefaults, we don't want that here because the
+# defaults are part of what should be checked here
+echo "info qtree" |
+QEMU_OPTIONS="" do_run_qemu "$@" | _filter_win32 |
+sed -ne '/^  dev: isa-fdc/,/^  dev:/{x;p}'
+}
+
+function check_cache_mode()
+{
+echo "info block none0" |
+QEMU_OPTIONS="" do_run_qemu -drive if=none,file="$TEST_IMG" "$@" |
+_filter_win32 | grep "Cache mode"
+}
+
+
+size=720k
+
+_make_test_img $size
+
+# Default drive semantics:
+#
+# By default you get a single empty floppy drive. You can override it with
+# -drive and using the same index, but if you use -drive to add a floppy to a
+# different index, you get both of them. However, as soon as you use any
+# '-device floppy', even to a different slot, the default drive is disabled.
+
+echo
+echo
+echo === Default ===
+
+check_floppy_qtree
+
+echo
+echo
+echo === Using -fda/-fdb options ===
+
+check_floppy_qtree -fda "$TEST_IMG"
+check_floppy_qtree -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+
+
+echo
+echo
+echo === Using -drive options ===
+
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG",index=1
+
+echo
+echo
+echo === Using -drive if=none and -global ===
+
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+   -global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
+
+echo
+echo
+echo === Using -drive if=none and -device ===
+
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+   -device floppy,drive=none0 -device floppy,drive=none1,unit=1
+
+echo
+echo
+echo === Mixing -fdX and -global ===
+
+# Working
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+
+# Conflicting (-fdX wins)
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+
+echo
+echo
+echo === Mixing -fdX and -device ===
+
+# Working
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive 

[Qemu-block] [PATCH v4 0/4] fdc: Use separate qdev device for drives

[Kevin Wolf]

We have been complaining for a long time about how the floppy controller and
floppy drives are combined in a single qdev device and how this makes the
device awkward to work with because it behaves different from all other block
devices.

The latest reason to complain was when I noticed that using qdev device names
in QMP commands (e.g. for media change) doesn't really work when only the
controller is a qdev device, but the drives aren't.

So I decided to have a go at it, and this is the result.

It doesn't actually change any of the inner workings of the floppy controller,
but it wires things up differently on the qdev layer so that a floppy
controller now exposes a bus on which the floppy drives sit. This results in a
structure that is similar to IDE where the actual drive state is still in the
controller and the qdev device basically just contains the qdev properties -
not pretty, but quite workable.

The commit message of patch 3 explains how to use it. In short, there is a
'-device floppy' now and it does what you would expect if you ever used ide-cd.

The other problem is old command lines, especially those using things like
'-global isa-fdc,driveA=...'. In order to keep them working, we need to forward
the property to an internally created floppy drive device. This is a bit like
usb-storage, which we know is ugly, but works well enough in practice. The good
news here is that in contrast to usb-storage, the floppy controller only does
the forwarding for legacy configurations; as soon as you start using '-device
floppy', it doesn't happen any more.

So as you may have expected, this conversion doesn't result in a perfect
device, but I think it's definitely an improvement over the old state. I hope
you like it despite the warts. :-)

v4:
- John says that his grep is broken and hangs at 100% CPU with my attempt to
  extract the floppy controller from info qtree. Use a simpler sed command
  instead (which, unlike the grep command, doesn't handle arbitrary indentation
  level of the next item, but we know what comes next, so just hardcoding 10
  spaces works, too).

v3:
- Fixed omissons in the conversion sysbus-fdc and the Sun one. Nice, combining
  floppy controllers and weird platforms in a single series. [John]

v2:
- Added patch 4 (qemu-iotests case for floppy config on the command line)
- Patch 2: Create a floppy device only if a BlockBackend exists instead of
  always creating two of them
- Patch 2: Initialise drive->fdctrl even if no drive is attached, it is
  accessed anyway during migration
- Patch 3: Keep 'type' qdev property and FDrive->drive in sync
- Patch 3: Removed if with condition that is always true


Kevin Wolf (4):
  fdc: Add a floppy qbus
  fdc: Add a floppy drive qdev
  fdc: Move qdev properties to FloppyDrive
  qemu-iotests: Test creating floppy drives

 hw/block/fdc.c |  271 --
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1170 
 tests/qemu-iotests/group   |1 +
 vl.c   |1 +
 5 files changed, 1637 insertions(+), 48 deletions(-)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

-- 
1.8.3.1

[Qemu-block] [PATCH v4 3/4] fdc: Move qdev properties to FloppyDrive

[Kevin Wolf]

This makes the FloppyDrive qdev object actually useful: Now that it has
all properties that don't belong to the controller, you can actually
use '-device floppy' and get a working result.

Command line semantics is consistent with CD-ROM drives: By default you
get a single empty floppy drive. You can override it with -drive and
using the same index, but if you use -drive to add a floppy to a
different index, you get both of them. However, as soon as you use any
'-device floppy', even to a different slot, the default drive is
disabled.

Using '-device floppy' without specifying the unit will choose the first
free slot on the controller.

Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 120 ++---
 vl.c   |   1 +
 2 files changed, 89 insertions(+), 32 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5aa8e52..537b996 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -35,6 +35,7 @@
 #include "qemu/timer.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/block/block.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -487,12 +488,18 @@ static const BlockDevOps fd_block_ops = {
  OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
 
 typedef struct FloppyDrive {
-DeviceState qdev;
-uint32_tunit;
+DeviceState qdev;
+uint32_tunit;
+BlockConf   conf;
+FloppyDriveType type;
 } FloppyDrive;
 
 static Property floppy_drive_properties[] = {
 DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_BLOCK_PROPERTIES(FloppyDrive, conf),
+DEFINE_PROP_DEFAULT("drive-type", FloppyDrive, type,
+FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+FloppyDriveType),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -501,6 +508,7 @@ static int floppy_drive_init(DeviceState *qdev)
 FloppyDrive *dev = FLOPPY_DRIVE(qdev);
 FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
 FDrive *drive;
+int ret;
 
 if (dev->unit == -1) {
 for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
@@ -517,29 +525,57 @@ static int floppy_drive_init(DeviceState *qdev)
 return -1;
 }
 
-/* TODO Check whether unit is in use */
-
 drive = get_drv(bus->fdc, dev->unit);
-
 if (drive->blk) {
-if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
-error_report("fdc doesn't support drive option werror");
-return -1;
-}
-if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-error_report("fdc doesn't support drive option rerror");
-return -1;
-}
-} else {
+error_report("Floppy unit %d is in use", dev->unit);
+return -1;
+}
+
+if (!dev->conf.blk) {
 /* Anonymous BlockBackend for an empty drive */
-drive->blk = blk_new();
+dev->conf.blk = blk_new();
+ret = blk_attach_dev(dev->conf.blk, qdev);
+assert(ret == 0);
 }
 
-fd_init(drive);
-if (drive->blk) {
-blk_set_dev_ops(drive->blk, _block_ops, drive);
-pick_drive_type(drive);
+blkconf_blocksizes(>conf);
+if (dev->conf.logical_block_size != 512 ||
+dev->conf.physical_block_size != 512)
+{
+error_report("Physical and logical block size must be 512 for floppy");
+return -1;
+}
+
+/* rerror/werror aren't supported by fdc and therefore not even registered
+ * with qdev. So set the defaults manually before they are used in
+ * blkconf_apply_backend_options(). */
+dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
+dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
+blkconf_apply_backend_options(>conf);
+
+/* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
+ * for empty drives. */
+if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
+blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option werror");
+return -1;
 }
+if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+
+drive->blk = dev->conf.blk;
+drive->fdctrl = bus->fdc;
+
+fd_init(drive);
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+
+/* Keep 'type' qdev property and FDrive->drive in sync */
+drive->drive = dev->type;
+pick_drive_type(drive);
+dev->type = drive->drive;
+
 fd_revalidate(drive);
 
 return 0;
@@ -808,6 +844,10 @@ struct FDCtrl {
 FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
+struct {
+BlockBackend *blk;
+FloppyDriveType type;
+} qdev_for_drives[MAX_FD];
 int reset_sensei;
 uint32_t check_media_rate;
 FloppyDriveType fallback; /* 

[Qemu-block] [PATCH v4 2/4] fdc: Add a floppy drive qdev

[Kevin Wolf]

Floppy controllers automatically create two floppy drive devices in qdev
now. (They always created two drives, but managed them only internally.)

Signed-off-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 hw/block/fdc.c | 151 +
 1 file changed, 120 insertions(+), 31 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a3afb62..5aa8e52 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,8 @@
 #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
 
 typedef struct FDCtrl FDCtrl;
+typedef struct FDrive FDrive;
+static FDrive *get_drv(FDCtrl *fdctrl, int unit);
 
 typedef struct FloppyBus {
 BusState bus;
@@ -180,7 +182,7 @@ typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
 } FDiskFlags;
 
-typedef struct FDrive {
+struct FDrive {
 FDCtrl *fdctrl;
 BlockBackend *blk;
 /* Drive status */
@@ -201,7 +203,7 @@ typedef struct FDrive {
 uint8_t media_rate;   /* Data rate of medium*/
 
 bool media_validated; /* Have we validated the media? */
-} FDrive;
+};
 
 
 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
@@ -466,6 +468,100 @@ static void fd_revalidate(FDrive *drv)
 }
 }
 
+static void fd_change_cb(void *opaque, bool load)
+{
+FDrive *drive = opaque;
+
+drive->media_changed = 1;
+drive->media_validated = false;
+fd_revalidate(drive);
+}
+
+static const BlockDevOps fd_block_ops = {
+.change_media_cb = fd_change_cb,
+};
+
+
+#define TYPE_FLOPPY_DRIVE "floppy"
+#define FLOPPY_DRIVE(obj) \
+ OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
+
+typedef struct FloppyDrive {
+DeviceState qdev;
+uint32_tunit;
+} FloppyDrive;
+
+static Property floppy_drive_properties[] = {
+DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static int floppy_drive_init(DeviceState *qdev)
+{
+FloppyDrive *dev = FLOPPY_DRIVE(qdev);
+FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
+FDrive *drive;
+
+if (dev->unit == -1) {
+for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
+drive = get_drv(bus->fdc, dev->unit);
+if (!drive->blk) {
+break;
+}
+}
+}
+
+if (dev->unit >= MAX_FD) {
+error_report("Can't create floppy unit %d, bus supports only %d units",
+ dev->unit, MAX_FD);
+return -1;
+}
+
+/* TODO Check whether unit is in use */
+
+drive = get_drv(bus->fdc, dev->unit);
+
+if (drive->blk) {
+if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
+error_report("fdc doesn't support drive option werror");
+return -1;
+}
+if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+} else {
+/* Anonymous BlockBackend for an empty drive */
+drive->blk = blk_new();
+}
+
+fd_init(drive);
+if (drive->blk) {
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+pick_drive_type(drive);
+}
+fd_revalidate(drive);
+
+return 0;
+}
+
+static void floppy_drive_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *k = DEVICE_CLASS(klass);
+k->init = floppy_drive_init;
+set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
+k->bus_type = TYPE_FLOPPY_BUS;
+k->props = floppy_drive_properties;
+k->desc = "virtual floppy drive";
+}
+
+static const TypeInfo floppy_drive_info = {
+.name = TYPE_FLOPPY_DRIVE,
+.parent = TYPE_DEVICE,
+.instance_size = sizeof(FloppyDrive),
+.class_init = floppy_drive_class_init,
+};
+
 //
 /* Intel 82078 floppy disk controller emulation  */
 
@@ -1185,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
 }
 #endif
 
-static FDrive *get_cur_drv(FDCtrl *fdctrl)
+static FDrive *get_drv(FDCtrl *fdctrl, int unit)
 {
-switch (fdctrl->cur_drv) {
+switch (unit) {
 case 0: return drv0(fdctrl);
 case 1: return drv1(fdctrl);
 #if MAX_FD == 4
@@ -1198,6 +1294,11 @@ static FDrive *get_cur_drv(FDCtrl *fdctrl)
 }
 }
 
+static FDrive *get_cur_drv(FDCtrl *fdctrl)
+{
+return get_drv(fdctrl, fdctrl->cur_drv);
+}
+
 /* Status A register : 0x00 (read-only) */
 static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl)
 {
@@ -2357,46 +2458,33 @@ static void fdctrl_result_timer(void *opaque)
 }
 }
 
-static void fdctrl_change_cb(void *opaque, bool load)
-{
-FDrive *drive = opaque;
-
-drive->media_changed = 1;
-drive->media_validated = false;
-fd_revalidate(drive);
-}
-
-static const BlockDevOps fdctrl_block_ops = {
-.change_media_cb = fdctrl_change_cb,
-};
-
 /* Init functions */
 static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
 {
 

[Qemu-block] [PATCH v4 1/4] fdc: Add a floppy qbus

[Kevin Wolf]

This adds a qbus to the floppy controller that should contain the floppy
drives eventually. At the moment it just exists and is empty.

Signed-off-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 hw/block/fdc.c | 40 +++-
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index b79873a..a3afb62 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -52,6 +52,33 @@
 }   \
 } while (0)
 
+
+//
+/* qdev floppy bus  */
+
+#define TYPE_FLOPPY_BUS "Floppy"
+#define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
+
+typedef struct FDCtrl FDCtrl;
+
+typedef struct FloppyBus {
+BusState bus;
+FDCtrl *fdc;
+} FloppyBus;
+
+static const TypeInfo floppy_bus_info = {
+.name = TYPE_FLOPPY_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(FloppyBus),
+};
+
+static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
+{
+qbus_create_inplace(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL);
+bus->fdc = fdc;
+}
+
+
 //
 /* Floppy drive emulation   */
 
@@ -148,8 +175,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
 #define FD_SECTOR_SC   2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
 
-typedef struct FDCtrl FDCtrl;
-
 /* Floppy disk drive emulation */
 typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
@@ -684,6 +709,7 @@ struct FDCtrl {
 /* Power down config (also with status regB access mode */
 uint8_t pwrd;
 /* Floppy drives */
+FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
 int reset_sensei;
@@ -2442,7 +2468,8 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
 *fdc_tc = qdev_get_gpio_in(dev, 0);
 }
 
-static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
+static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
+  Error **errp)
 {
 int i, j;
 static int command_tables_inited = 0;
@@ -2480,6 +2507,8 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error 
**errp)
 k->register_channel(fdctrl->dma, fdctrl->dma_chann,
 _transfer_handler, fdctrl);
 }
+
+floppy_bus_create(fdctrl, >bus, dev);
 fdctrl_connect_drives(fdctrl, errp);
 }
 
@@ -2508,7 +2537,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error 
**errp)
 }
 
 qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-fdctrl_realize_common(fdctrl, );
+fdctrl_realize_common(dev, fdctrl, );
 if (err != NULL) {
 error_propagate(errp, err);
 return;
@@ -2559,7 +2588,7 @@ static void sysbus_fdc_common_realize(DeviceState *dev, 
Error **errp)
 FDCtrlSysBus *sys = SYSBUS_FDC(dev);
 FDCtrl *fdctrl = >state;
 
-fdctrl_realize_common(fdctrl, errp);
+fdctrl_realize_common(dev, fdctrl, errp);
 }
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
@@ -2744,6 +2773,7 @@ static void fdc_register_types(void)
 type_register_static(_fdc_type_info);
 type_register_static(_fdc_info);
 type_register_static(_fdc_info);
+type_register_static(_bus_info);
 }
 
 type_init(fdc_register_types)
-- 
1.8.3.1

[Qemu-block] [PATCH] m25p80: add support for the mx66l1g45g

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater 
---
 hw/block/m25p80.c |1 +
 1 file changed, 1 insertion(+)

Index: qemu-aspeed.git/hw/block/m25p80.c
===
--- qemu-aspeed.git.orig/hw/block/m25p80.c
+++ qemu-aspeed.git/hw/block/m25p80.c
@@ -203,6 +203,7 @@ static const FlashPartInfo known_devices
 { INFO("mx25l25655e", 0xc22619,  0,  64 << 10, 512, 0) },
 { INFO("mx66u51235f", 0xc2253a,  0,  64 << 10, 1024, ER_4K | ER_32K) },
 { INFO("mx66u1g45g",  0xc2253b,  0,  64 << 10, 2048, ER_4K | ER_32K) },
+{ INFO("mx66l1g45g",  0xc2201b,  0,  64 << 10, 2048, ER_4K | ER_32K) },
 
 /* Micron */
 { INFO("n25q032a11",  0x20bb16,  0,  64 << 10,  64, ER_4K) },

Re: [Qemu-block] [Qemu-devel] [PATCH v14 17/21] numa: convert to use QObjectInputVisitor for -numa

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> Switch away from using OptsVisitor to parse the -numa
> argument processing. This enables use of the modern
> list syntax for specifying CPUs. e.g. the old syntax
>
>   -numa node,nodeid=0,cpus=0-3,cpus=8-11,mem=107
>
> is equivalent to
>
>   -numa node,nodeid=0,cpus.0=0,cpus.1=1,cpus.2=2,cpus.3=3,\
> cpus.4=8,cpus.5=9,cpus.6=10,cpus.7=11,mem=107
>
> Furthermore, the cli arg can now follow the QAPI schema
> nesting, so the above is equivalent to the canonical
> syntax:
>
>   -numa type=node,data.nodeid=0,data.cpus.0=0,data.cpus.1=1,\
> data.cpus.2=2,data.cpus.3=3,data.cpus.4=8,data.cpus.5=9,\
>   data.cpus.6=10,data.cpus.7=11,data.mem=107
>
> A test case is added to cover argument parsing to validate
> that both the old and new syntax is correctly handled.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/sysemu/numa_int.h |  11 +
>  numa.c|  36 +-
>  stubs/Makefile.objs   |   5 ++
>  stubs/exec.c  |   6 +++
>  stubs/hostmem.c   |  14 ++
>  stubs/memory.c|  41 
>  stubs/qdev.c  |   8 
>  stubs/vl.c|   8 
>  stubs/vmstate.c   |   4 ++
>  tests/Makefile.include|   2 +
>  tests/test-numa.c | 116 
> ++
>  11 files changed, 240 insertions(+), 11 deletions(-)
>  create mode 100644 include/sysemu/numa_int.h
>  create mode 100644 stubs/exec.c
>  create mode 100644 stubs/hostmem.c
>  create mode 100644 stubs/memory.c
>  create mode 100644 stubs/qdev.c
>  create mode 100644 stubs/vl.c
>  create mode 100644 tests/test-numa.c
>
> diff --git a/include/sysemu/numa_int.h b/include/sysemu/numa_int.h
> new file mode 100644
> index 000..93160da
> --- /dev/null
> +++ b/include/sysemu/numa_int.h
> @@ -0,0 +1,11 @@
> +#ifndef SYSEMU_NUMA_INT_H
> +#define SYSEMU_NUMA_INT_H
> +
> +#include "sysemu/numa.h"
> +
> +extern int have_memdevs;
> +extern int max_numa_nodeid;
> +
> +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> +
> +#endif
> diff --git a/numa.c b/numa.c
> index 6289f46..653ebf1 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -23,14 +23,14 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "sysemu/numa.h"
> +#include "sysemu/numa_int.h"
>  #include "exec/cpu-common.h"
>  #include "qemu/bitmap.h"
>  #include "qom/cpu.h"
>  #include "qemu/error-report.h"
>  #include "include/exec/cpu-common.h" /* for RAM_ADDR_FMT */
>  #include "qapi-visit.h"
> -#include "qapi/opts-visitor.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "hw/boards.h"
>  #include "sysemu/hostmem.h"
>  #include "qmp-commands.h"
> @@ -45,10 +45,10 @@ QemuOptsList qemu_numa_opts = {
>  .desc = { { 0 } } /* validated with OptsVisitor */
>  };
>  
> -static int have_memdevs = -1;
> -static int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
> - * For all nodes, nodeid < max_numa_nodeid
> - */
> +int have_memdevs = -1;
> +int max_numa_nodeid; /* Highest specified NUMA node ID, plus one.
> +  * For all nodes, nodeid < max_numa_nodeid
> +  */
>  int nb_numa_nodes;
>  NodeInfo numa_info[MAX_NODES];
>  
> @@ -189,6 +189,9 @@ static void numa_node_parse(NumaNodeOptions *node, 
> QemuOpts *opts, Error **errp)
>  if (node->has_mem) {
>  uint64_t mem_size = node->mem;
>  const char *mem_str = qemu_opt_get(opts, "mem");
> +if (!mem_str) {
> +mem_str = qemu_opt_get(opts, "data.mem");
> +}
>  /* Fix up legacy suffix-less format */
>  if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
>  mem_size <<= 20;
> @@ -211,16 +214,27 @@ static void numa_node_parse(NumaNodeOptions *node, 
> QemuOpts *opts, Error **errp)
>  max_numa_nodeid = MAX(max_numa_nodeid, nodenr + 1);
>  }
>  
> -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> +int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
>  {
>  NumaOptions *object = NULL;
>  Error *err = NULL;
> +Visitor *v;
>  
> -{
> -Visitor *v = opts_visitor_new(opts);
> -visit_type_NumaOptions(v, NULL, , );
> -visit_free(v);
> +/*
> + * Needs autocreate_list=true, permit_int_ranges=true and
> + * permit_repeated_opts=true in order to support existing
> + * syntax for 'cpus' parameter which is an int list.
> + *
> + * Needs autocreate_struct_levels=1, because existing syntax
> + * used a nested struct in the QMP schema with a flat namespace
> + * in the CLI args.

Which QAPI definition(s) do you allude to here?

If it's just union NumaOptions, that's not ABI.  I'd simply flatten it.
Sketch appended.

> + */
> +v = qobject_input_visitor_new_opts(opts, true, 1, true, true, );
> +if (err) {
> +goto end;
>  }
> +

Re: [Qemu-block] [Qemu-devel] [PATCH v14 16/21] hmp: support non-scalar properties with object_add

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> The current object_add HMP syntax only allows for
> creation of objects with scalar properties, or a list
> with a fixed scalar element type. Objects which have
> properties that are represented as structs in the QAPI
> schema cannot be created using -object.
>
> This is a design limitation of the way the OptsVisitor
> is written. It simply iterates over the QemuOpts values
> as a flat list. The support for lists is enabled by
> allowing the same key to be repeated in the opts string.
>
> The QObjectInputVisitor now has functionality that allows
> it to replace OptsVisitor, maintaining full backwards
> compatibility for previous CLI syntax, while also allowing
> use of new syntax for structs.
>
> Thus -object can now support non-scalar properties,
> for example the QMP object:
>
>   {
> "execute": "object-add",
> "arguments": {
>   "qom-type": "demo",
>   "id": "demo0",
>   "parameters": {
> "foo": [
>   { "bar": "one", "wizz": "1" },
>   { "bar": "two", "wizz": "2" }
> ]
>   }
> }
>   }
>
> Would be creatable via the HMP now using
>
>object_add demo,id=demo0,\
>   foo.0.bar=one,foo.0.wizz=1,\
>   foo.1.bar=two,foo.1.wizz=2
>
> Notice that this syntax is intentionally compatible
> with that currently used by block drivers. NB the
> indentation seen here after line continuations should
> not be used in reality, it is just for clarity in this
> commit message.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  hmp.c | 25 +
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 336e7bf..b32d8c8 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -25,7 +25,7 @@
>  #include "qemu/sockets.h"
>  #include "monitor/monitor.h"
>  #include "monitor/qdev.h"
> -#include "qapi/opts-visitor.h"
> +#include "qapi/qobject-input-visitor.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/string-output-visitor.h"
>  #include "qapi/util.h"
> @@ -1696,21 +1696,30 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>  void hmp_object_add(Monitor *mon, const QDict *qdict)
>  {
>  Error *err = NULL;
> -QemuOpts *opts;
>  Visitor *v;
>  Object *obj = NULL;
> +QObject *pobj;
>  
> -opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, );
> +pobj = qdict_crumple(qdict, true, );
>  if (err) {
> -hmp_handle_error(mon, );
> -return;
> +goto cleanup;
>  }
>  
> -v = opts_visitor_new(opts);
> -obj = user_creatable_add(qdict, v, );
> +/*
> + * We need autocreate_list=true + permit_int_ranges=true
> + * in order to maintain compat with OptsVisitor creation
> + * of the 'numa' object which had an int16List property.
> + *
> + * We need autocreate_structs=1, because at the CLI level

autocreate_struct_levels=1

Same in previous patch.

> + * we have the object type + properties in the same flat
> + * struct, even though at the QMP level they are nested.
> + */
> +v = qobject_input_visitor_new_autocast(pobj, true, 1, true);
> +obj = user_creatable_add(qobject_to_qdict(pobj), v, );
>  visit_free(v);
> -qemu_opts_del(opts);
>  
> + cleanup:
> +qobject_decref(pobj);
>  if (err) {
>  hmp_handle_error(mon, );
>  }