Re: [Qemu-block] [PATCH v4 0/3] iotests: Fix test 162

[Hao QingFeng]

在 2016-10-13 3:46, Max Reitz 写道:

On 12.10.2016 10:55, Hao QingFeng wrote:

Max,

Just a common question for this case, if sshx block driver wasn't built
into qemu-img, this case would fail as below:

Good point, and thanks for bringing it up, but it's not directly linked
to this series other than by its subject, of course, so I'd rather add a
fix on top.

Thanks and sorry for sending to the improper mail series.

exec /home/haoqf/KVMonz/qemu/tests/qemu-iotests/../../qemu-img info
--image-opts driver=ssh,host=localhost,port=0.42,path=/foo
qemu-img: Could not open
'driver=ssh,host=localhost,port=0.42,path=/foo': Unknown driver 'ssh'

Adding 162.notrun can bypass this case but it would skip it even if
qemu-img has sshx block driver, in which case I think it should be run.

So How about adding a script to dynamically check at runtime if the
current env qemu-img can meet the requirement to run the test or not?

Unfortunately, the list of block drivers listed by will not contain ssh
if ssh is built as a module, which is possible.
Actually I am not sure if I understood it. Do you mean 
"CONFIG_LIBSSH2=m" set

rather than "CONFIG_LIBSSH2=y" in config-host.mak? But in the configure it's
set to be "CONFIG_LIBSSH2=y":
if test "$libssh2" = "yes" ; then
  echo "CONFIG_LIBSSH2=y" >> $config_host_mak
  echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
  echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
fi
Meanwhile I changed it to be "CONFIG_LIBSSH2=m" and reconfig, make the qemu,
qemu-img --help can still prompt ssh.

This is a bug that should be fixed, but I'd rather do so in a separate
series from this one.

In any case, once it is fixed I'd rather just take the approach quorum
tests take already (e.g. test 081), which is something like:

test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
[ "$test_ssh" = "" ] && _notrun "ssh support required"

Cool. Agree with this like what was done in 081.  thanks

Max



--
QingFeng Hao

Re: [Qemu-block] [Qemu-devel] [PATCH v2 04/11] blockjobs: Always use block_job_get_aio_context

[John Snow]

As context to everyone else as to why I'm going down the rabbit hole of 
trying to remove external references to AioContext at all, see 
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00795.html


On 10/07/2016 03:49 AM, Paolo Bonzini wrote:



On 06/10/2016 22:22, John Snow wrote:

Calls .complete(), for which the only implementation is
mirror_complete. Uh, this actually seems messy. Looks like there's
nothing to prevent us from calling this after we've already told it to
complete once.


Yeah, it should have an

if (s->should_complete) {
return;
}

at the beginning.  I have other mirror.c patches in my queue so I can
take care of that.



Or something up the stack at block_job_complete so it's not up to job 
implementations? What if the next implementer "forgets."



block_job_cancel and block_job_complete are different.

block_job_cancel is called in many places, but we can just add a similar
block_job_user_cancel if we wanted a version which takes care to acquire
context and one that does not. (Or we could just acquire the context
regardless, but Paolo warned me ominously that recursive locks are EVIL.
He sounded serious.)


Not that many places:

- block_job_finish_sync calls it, and you can just release/reacquire
around the call to "finish(job, _err)".



This makes me a little nervous because we went through the trouble of 
creating this callback, but we're going to assume we know that it's a 
public interface that will take the lock for itself (or otherwise does 
not require a lock.)


In practice it works, but it seems needlessly mystifying in terms of 
proving correctness.



- there are two callers in blockdev.c, and you can just remove the
acquire/release from blockdev.c if you push it in block_job_cancel.



Makes sense; I don't like the association of (bs :: job) here anyway. 
Again we're grabbing context for a job where that job may not even be 
running.



As to block_job_cancel_sync:



Which I didn't audit, because no callers use job->blk to get the 
AioContext before calling this; they use bs if bs->job is present.



- replication_stop is not acquiring s->secondary_disk->bs's AioContext.



Seems like a bug on their part. Would be fixed by having cancel acquire 
context for itself.



- there is no need to hold the AioContext between ->prepare and ->clean.
 My suggestion is to ref the blockjob after storing it in state->job
(you probably should do that anyway) and unref'ing it in ->clean.  Then
you can call again let block_job_cancel_sync(bs->job) take the
AioContext, which it will do in block_job_finish_sync.


Yeah, I should be reffing it anyway.

The rest of this... What I think you mean is acquiring and releasing the 
context as needed for EACH of prepare, commit, abort, and clean as 
necessary, right?


And then in this case, it simply wouldn't be necessary for abort, as the 
sync cancel would do it for us.





block_job_complete has no direct callers outside of QMP, but it is also
used as a callback by block_job_complete_sync, used in qemu-img for
run_block_job. I can probably rewrite qemu_img to avoid this usage.


No need to: qemu-img is not acquiring the AioContext, so it's okay to
let block_job_complete do that (like block_job_cancel,
block_job_complete will be called by block_job_finish_sync without the
AioContext acquired).



Eh? Oh, you're right, it just gets it for the sake of aio_poll.


Paolo




Alright.


Say I *do* push the acquisitions down into blockjob.c. What benefit does 
that provide? Won't I still need the block_job_get_aio_context() 
function (At least internally) to know which context to acquire? This 
would preclude you from deleting it.


Plus... we remove some fairly simple locking mechanisms and then inflate 
it tenfold. I'm not convinced this is an improvement.


As context and a refresher (for me when I re-read this email in 12 
hours,) there are three places externally that are using an AioContext 
lock as acquired from *within* a BlockJob, excluding those that acquire 
a context separately from a Job and use that to reason that accesses to 
the job are safe (For example, blockdev_mark_auto_del.)


(1) QMP interface for job management
(2) bdrv_drain_all, in block/io.c


(1) AFAICT, the QMP interface is concerned with assuring itself it has 
unique access to the BlockJob structure itself, and it doesn't really 
authentically care about the AIOContext itself -- just race-free access 
to the Job.


This is not necessarily buggy today because, even though we grab the 
BlockBackend's context unconditionally, we already know the main/monitor 
thread is not accessing the blockjob. It's still silly, though.


(2) bdrv_drain_all appears to be worried about the same thing; we just 
need to safely deliver pause/resume messages.


I'm less sure about where this can run from, and suspect that if the job 
has deferred to main that this could be buggy. If bdrv_drain_all is 
called from context A and the job is running on context M having 

Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] iotests: Skip test 162 if there is no SSH support

[Eric Blake]

On 10/12/2016 03:49 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/162 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
> index f8eecb3..cad2bd7 100755
> --- a/tests/qemu-iotests/162
> +++ b/tests/qemu-iotests/162
> @@ -35,6 +35,9 @@ status=1# failure is the default!
>  _supported_fmt generic
>  _supported_os Linux
>  
> +test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
> +[ "$test_ssh" = "" ] && _notrun "ssh support required"
> +

Reviewed-by: Eric Blake 

>  echo
>  echo '=== NBD ==='
>  # NBD expects all of its arguments to be strings
> 

-- 
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 2/3] block: Emit modules in bdrv_iterate_format()

[Eric Blake]

On 10/12/2016 03:49 PM, Max Reitz wrote:
> Some block drivers may not be loaded yet, but qemu supports them
> nonetheless. bdrv_iterate_format() should report them, too.
> 
> Signed-off-by: Max Reitz 
> ---
>  block.c | 18 ++
>  1 file changed, 18 insertions(+)
> 

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 0/3] iotests: Skip 162 if there is no SSH support

[no-reply]

Hi,

Your series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Message-id: 20161012204907.25941-1-mre...@redhat.com
Subject: [Qemu-devel] [PATCH 0/3] iotests: Skip 162 if there is no SSH support
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=16
make docker-test-quick@centos6
make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20161012204907.25941-1-mre...@redhat.com -> 
patchew/20161012204907.25941-1-mre...@redhat.com
Switched to a new branch 'test'
edffc08 iotests: Skip test 162 if there is no SSH support
de2a49f block: Emit modules in bdrv_iterate_format()
63e6b44 block: Fix bdrv_iterate_format() sorting

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
  BUILD   centos6
=== OUTPUT END ===

Abort: command timeout (>3600 seconds)


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

Re: [Qemu-block] [Qemu-devel] [PATCH 1/3] block: Fix bdrv_iterate_format() sorting

[Eric Blake]

On 10/12/2016 03:49 PM, Max Reitz wrote:
> bdrv_iterate_format() did not actually sort the formats by name but by
> "pointer interpreted as string". That is probably not what we intended
> to do, so fix it (by changing qsort_strcmp() so it matches the example
> from qsort()'s manual page).
> 
> Signed-off-by: Max Reitz 
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'm a bit surprised that code sanitizers like Coverity or ASAN aren't
(yet?) able to flag this.

Reviewed-by: Eric Blake 

> 
> diff --git a/block.c b/block.c
> index bb1f1ec..e46e4b2 100644
> --- a/block.c
> +++ b/block.c
> @@ -2789,7 +2789,7 @@ const char *bdrv_get_format_name(BlockDriverState *bs)
>  
>  static int qsort_strcmp(const void *a, const void *b)
>  {
> -return strcmp(a, b);
> +return strcmp(*(char *const *)a, *(char *const *)b);
>  }
>  
>  void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
> 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 1/3] block: Fix bdrv_iterate_format() sorting

[Max Reitz]

bdrv_iterate_format() did not actually sort the formats by name but by
"pointer interpreted as string". That is probably not what we intended
to do, so fix it (by changing qsort_strcmp() so it matches the example
from qsort()'s manual page).

Signed-off-by: Max Reitz 
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index bb1f1ec..e46e4b2 100644
--- a/block.c
+++ b/block.c
@@ -2789,7 +2789,7 @@ const char *bdrv_get_format_name(BlockDriverState *bs)
 
 static int qsort_strcmp(const void *a, const void *b)
 {
-return strcmp(a, b);
+return strcmp(*(char *const *)a, *(char *const *)b);
 }
 
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
-- 
2.10.0

[Qemu-block] [PATCH 0/3] iotests: Skip 162 if there is no SSH support

[Max Reitz]

As reported by Hao QingFeng, iotest 162 is currently executed even if
qemu does not have any SSH support (which makes it fail, naturally).

Fixing that is not so trivial, because qemu-img currently does not
report modules, and SSH can be compiled as a module, so that needs to be
fixed first. While doing so, I noticed that bdrv_iterate_format() tries
to sort the list of formats, which is a bit contrary to my experience.
Turns out that needs to be fixed, too.


This series can either be applied on top of my series "iotests: Fix test
162" or just directly on master, both works (i.e. the patches in this
series do not interfere with those from that one). I still thought I'd
mention that series, if nothing else then only to get you to review that
other one. ;-)


Max Reitz (3):
  block: Fix bdrv_iterate_format() sorting
  block: Emit modules in bdrv_iterate_format()
  iotests: Skip test 162 if there is no SSH support

 block.c| 20 +++-
 tests/qemu-iotests/162 |  3 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.10.0

[Qemu-block] [PATCH 3/3] iotests: Skip test 162 if there is no SSH support

[Max Reitz]

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/162 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
index f8eecb3..cad2bd7 100755
--- a/tests/qemu-iotests/162
+++ b/tests/qemu-iotests/162
@@ -35,6 +35,9 @@ status=1  # failure is the default!
 _supported_fmt generic
 _supported_os Linux
 
+test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
+[ "$test_ssh" = "" ] && _notrun "ssh support required"
+
 echo
 echo '=== NBD ==='
 # NBD expects all of its arguments to be strings
-- 
2.10.0

Re: [Qemu-block] [PATCH v4 0/3] iotests: Fix test 162

[Max Reitz]

On 12.10.2016 10:55, Hao QingFeng wrote:
> Max,
> 
> Just a common question for this case, if sshx block driver wasn't built
> into qemu-img, this case would fail as below:

Good point, and thanks for bringing it up, but it's not directly linked
to this series other than by its subject, of course, so I'd rather add a
fix on top.

> exec /home/haoqf/KVMonz/qemu/tests/qemu-iotests/../../qemu-img info
> --image-opts driver=ssh,host=localhost,port=0.42,path=/foo
> qemu-img: Could not open
> 'driver=ssh,host=localhost,port=0.42,path=/foo': Unknown driver 'ssh'
> 
> Adding 162.notrun can bypass this case but it would skip it even if
> qemu-img has sshx block driver, in which case I think it should be run.
> 
> So How about adding a script to dynamically check at runtime if the
> current env qemu-img can meet the requirement to run the test or not?

Unfortunately, the list of block drivers listed by will not contain ssh
if ssh is built as a module, which is possible.

This is a bug that should be fixed, but I'd rather do so in a separate
series from this one.

In any case, once it is fixed I'd rather just take the approach quorum
tests take already (e.g. test 081), which is something like:

test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)')
[ "$test_ssh" = "" ] && _notrun "ssh support required"

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/22] qcow2: add dirty bitmaps extension

[Max Reitz]

On 11.10.2016 14:09, Vladimir Sementsov-Ogievskiy wrote:
> On 01.10.2016 17:46, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> Add dirty bitmap extension as specified in docs/specs/qcow2.txt.
>>> For now, just mirror extension header into Qcow2 state and check
>>> constraints.
>>>
>>> For now, disable image resize if it has bitmaps. It will be fixed later.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block/qcow2.c | 83
>>> +++
>>>   block/qcow2.h |  4 +++
>>>   2 files changed, 87 insertions(+)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index c079aa8..08c4ef9 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>> [...]
>>
>>> @@ -162,6 +164,62 @@ static int
>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>   }
>>>   break;
>>>   +case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>>> +ret = bdrv_pread(bs->file, offset, _ext, ext.len);
>> Overflows with ext.len > sizeof(bitmaps_ext).
>>
>> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)
>>
>>> +if (ret < 0) {
>>> +error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>>> + "Could not read ext header");
>>> +return ret;
>>> +}
>>> +
>>> +if (bitmaps_ext.reserved32 != 0) {
>>> +error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>>> + "Reserved field is not zero.");
>> Please drop the full stop at the end.
> 
> what do you mean? goto to fail: here? or not stop at all, just print error?

The "." at the end of the message. :-)

(https://en.wikipedia.org/wiki/Full_stop)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 09/22] block: introduce persistent dirty bitmaps

[Max Reitz]

On 11.10.2016 15:11, Vladimir Sementsov-Ogievskiy wrote:
> On 07.10.2016 20:54, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
>>> on bdrv_close, using format driver. Format driver should maintain bitmap
>>> storing.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   block.c  | 30 ++
>>>   block/dirty-bitmap.c | 27 +++
>>>   block/qcow2-bitmap.c |  1 +
>>>   include/block/block.h|  2 ++
>>>   include/block/block_int.h|  2 ++
>>>   include/block/dirty-bitmap.h |  6 ++
>>>   6 files changed, 68 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 804e3d4..1cde03a 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2196,6 +2196,7 @@ void bdrv_reopen_abort(BDRVReopenState
>>> *reopen_state)
>>>   static void bdrv_close(BlockDriverState *bs)
>>>   {
>>>   BdrvAioNotifier *ban, *ban_next;
>>> +Error *local_err = NULL;
>>> assert(!bs->job);
>>>   assert(!bs->refcnt);
>>> @@ -2204,6 +2205,10 @@ static void bdrv_close(BlockDriverState *bs)
>>>   bdrv_flush(bs);
>>>   bdrv_drain(bs); /* in case flush left pending I/O */
>>>   +bdrv_store_persistent_bitmaps(bs, _err);
>>> +if (local_err != NULL) {
>>> +error_report_err(local_err);
>>> +}
>> That seems pretty wrong to me. If the persistent bitmaps cannot be
>> stored, the node should not be closed to avoid loss of data.
>>
>>>   bdrv_release_named_dirty_bitmaps(bs);
>> Especially since the next function will just drop all the dirty bitmaps.
>>
>> I see the issue that bdrv_close() is only called by bdrv_delete() which
>> in turn is only called by bdrv_unref(); and how are you supposed to
>> react to bdrv_unref() failing?
>>
>> So I'm not sure how this issue should be addressed, but this is most
>> certainly not ideal. You should not just drop supposedly persistent
>> dirty bitmaps if they cannot be saved.
>>
>> We really should to have some way to keep the bitmap around if it cannot
>> be saved, but I don't know how to do that either.
>>
>> In any case, we should make sure that the node supports saving
>> persistent dirty bitmaps, because having persistent dirty bitmaps at a
>> node that does not support them is something we can and must prevent
>> beforehand.
>>
>> But I don't know how to handle failure if writing the dirty bitmap
>> fails. I guess one could argue that it's the same as bdrv_flush()
>> failing and thus can be handled in the same way, i.e. ignore it. I'm not
>> happy with that, but I'd accept it if there's no other way.
> 
> For now, the only usage of these bitmaps is incremental backup and
> bitmaps are not critical data. If we lost them we will just do full
> backup. If there will be some critical persistent bdrv dirty bitmaps in
> future, we can introduce a callback BdrvDirtyBitmap.store_failed for
> them, which will somehow handle that case.. Detach bitmap from bs and
> save it in memory, add qmp commands to raw-dump them, etc.. I

Yes, fine with me. Still, we should make an effort to detect the case
that some block driver will not be able to store a certain persistent
bitmap attached to one of its nodes as early as possible, ideally
already when the bitmap is created.

Max



signature.asc
Description: OpenPGP digital signature

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

[Markus Armbruster]

Kevin Wolf  writes:

> Am 12.10.2016 um 17:50 hat Markus Armbruster geschrieben:
>> "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 
>
>> > @@ -329,21 +335,87 @@ static void 
>> > qobject_input_type_int64_autocast(Visitor *v, const char *name,
>> >int64_t *obj, Error **errp)
>> >  {
>> >  QObjectInputVisitor *qiv = to_qiv(v);
>> > -QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> > -true));
>> > +QString *qstr;
>> >  int64_t ret;
>> > +const 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 ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
>> > +*obj = tos->range_val + 1;
>> > +tos->range_val++;
>> 
>> Roundabout way to write
>> 
>>*obj = tos->range_val++;
>
> *obj = ++tos->range_val, actually.

Of course, thanks.

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

[Markus Armbruster]

"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?).

> All existing callers are all converted to explicitly
> request the historical behaviour of only reporting the
> last key. Later patches will make use of the new modes.
>
> Signed-off-by: Daniel P. Berrange 

Out of steam for today.

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 10:10 PM, Kevin Wolf  wrote:
> Am 12.10.2016 um 18:20 hat Ashijeet Acharya geschrieben:
>> On Wed, Oct 12, 2016 at 9:31 PM, Kevin Wolf  wrote:
>> > Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> >> This series adds blockdev-add support for SSH block driver.
>> >>
>> >> Patch 1 prepares the code for the addition of a new option prefix,
>> >> which is "server.". This is accomplished by adding a
>> >> ssh_has_filename_options_conflict() function which helps to iterate
>> >> over the various options and check for conflict.
>> >>
>> >> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
>> >> and then makes it accept a InetSocketAddress under the "server" option.
>> >> The old options "host" and "port" are supported as legacy options and
>> >> then translated to the respective InetSocketAddress representation.
>> >>
>> >> Patch 3 drops the usage of "host" and "port" outside of
>> >> ssh_has_filename_options_conflict() and
>> >> ssh_process_legacy_socket_options() functions in order to make them
>> >> legacy options completely.
>> >>
>> >> Patch 4 helps to allow blockdev-add support for the SSH block driver
>> >> by making the SSH option available.

>> First thing I made sure if I wasn't breaking anything. Basically I
>> checked if blockdev-add worked with it and then if I am able to remove
>> it with x-blockdev-del.
>
> Did you try out all of the options that we support, including those in
> InetSocketAddress?

No, only 'host' and 'port' from InetSocketAddress which is why I think
the tests were successful in the first place. Using other options seem
to break it. Like you suggested, I think using
qobject_input_visitor_new_autocast() should fix this.

>
>> Also, there were no major changes which could
>> make the patches to break. I don't see tests available for SSH either
>> so there was nothing much I can do.
>> Do you have anything in mind?
>
> Actually, qemu-iotests has ssh support. It's probably not run very
> often, so I'm not sure whether it completely passes on master, but worth
> a try anyway. Check out ./check --help in tests/qemu-iotests, you'll
> probably want something like './check -T -ssh'.
>

Oh, I wasn't aware of that. I will look around now. Thanks!

> The commit message that added ssh support to the tests says:
>
> Note in order to run these tests on ssh, you must be running a local
> ssh daemon, and that daemon must accept loopback connections, and
> ssh-agent has to be set up to allow logins on the local daemon.  In
> other words, the following command should just work without demanding
> any passphrase:
>
>  ssh localhost
>
> Hope this is helpful.

Yes very helpful!!!

Thanks again!

Ashijeet
>
> Kevin

Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 9:21 PM, Kevin Wolf  wrote:
> Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> Add InetSocketAddress compatibility to SSH driver.
>>
>> Add a new option "server" to the SSH block driver which then accepts
>> a InetSocketAddress.
>>
>> "host" and "port" are supported as legacy options and are mapped to
>> their InetSocketAddress representation.
>>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>>  block/ssh.c | 87 
>> ++---
>>  1 file changed, 78 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 75cb7bc..702871a 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -32,8 +32,11 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/sockets.h"
>>  #include "qemu/uri.h"
>> +#include "qapi-visit.h"
>>  #include "qapi/qmp/qint.h"
>>  #include "qapi/qmp/qstring.h"
>> +#include "qapi/qmp-input-visitor.h"
>> +#include "qapi/qmp-output-visitor.h"
>>
>>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>>   * this block driver code.
>> @@ -74,6 +77,8 @@ typedef struct BDRVSSHState {
>>   */
>>  LIBSSH2_SFTP_ATTRIBUTES attrs;
>>
>> +InetSocketAddress *inet;
>> +
>>  /* Used to warn if 'flush' is not supported. */
>>  char *hostport;
>>  bool unsafe_flush_warning;
>> @@ -263,7 +268,9 @@ static bool ssh_has_filename_options_conflict(QDict 
>> *options, Error **errp)
>>  !strcmp(qe->key, "port") ||
>>  !strcmp(qe->key, "path") ||
>>  !strcmp(qe->key, "user") ||
>> -!strcmp(qe->key, "host_key_check"))
>> +!strcmp(qe->key, "host_key_check") ||
>> +!strcmp(qe->key, "server") ||
>> +!strncmp(qe->key, "server.", 7))
>
> There is strstart() from cutils.c which looks a bit nicer (you don't
> have to specify the string length then).

Okay, I will use that.

>
>>  {
>>  error_setg(errp, "Option '%s' cannot be used with a file name",
>> qe->key);
>> @@ -555,13 +562,71 @@ static QemuOptsList ssh_runtime_opts = {
>>  },
>>  };
>>
>> +static bool ssh_process_legacy_socket_options(QDict *output_opts,
>> +  QemuOpts *legacy_opts,
>> +  Error **errp)
>> +{
>> +const char *host = qemu_opt_get(legacy_opts, "host");
>> +const char *port = qemu_opt_get(legacy_opts, "port");
>> +
>> +if (!host && port) {
>> +error_setg(errp, "port may not be used without host");
>> +return false;
>> +}
>
> This check is rather pointless if !host causes an error anyway.

Hmm... I will remove it.

>
>> +if (!host) {
>> +error_setg(errp, "No hostname was specified");
>> +return false;
>> +} else {
>> +qdict_put(output_opts, "server.host", qstring_from_str(host));
>> +qdict_put(output_opts, "server.port",
>> +  qstring_from_str(port ?: stringify(22)));
>> +}
>> +
>> +return true;
>> +}
>> +
>> +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
>> + Error **errp)
>> +{
>> +InetSocketAddress *inet = NULL;
>> +QDict *addr = NULL;
>> +QObject *crumpled_addr = NULL;
>> +Visitor *iv = NULL;
>> +Error *local_error = NULL;
>> +
>> +qdict_extract_subqdict(options, , "server.");
>> +if (!qdict_size(addr)) {
>> +error_setg(errp, "SSH server address missing");
>> +goto out;
>> +}
>> +
>> +crumpled_addr = qdict_crumple(addr, true, errp);
>> +if (!crumpled_addr) {
>> +goto out;
>> +}
>> +
>> +iv = qmp_input_visitor_new(crumpled_addr, true);
>
> I believe you need qobject_input_visitor_new_autocast() here.
>
> Do integer properties like port work for you without it?

In InetSocketAddress 'port' is of the type 'char*' but now I think
using qobject_input_visitor_new_autocast() will be right since there
are other fields as well.

>
>> +visit_type_InetSocketAddress(iv, NULL, , _error);
>> +if (local_error) {
>> +error_propagate(errp, local_error);
>> +goto out;
>> +}
>> +
>> +out:
>> +QDECREF(addr);
>> +qobject_decref(crumpled_addr);
>> +visit_free(iv);
>> +return inet;
>> +}
>> +
>>  static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>>int ssh_flags, int creat_mode, Error **errp)
>>  {
>>  int r, ret;
>>  QemuOpts *opts = NULL;
>>  Error *local_err = NULL;
>> -const char *host, *user, *path, *host_key_check;
>> +const char *user, *path, *host_key_check;
>>  int port;
>>
>>  opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
>> @@ -572,15 +637,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
>> *options,
>>  goto err;
>>  }
>>
>> -host = qemu_opt_get(opts, "host");
>> -if (!host) {
>> +if 

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Kevin Wolf]

Am 12.10.2016 um 18:20 hat Ashijeet Acharya geschrieben:
> On Wed, Oct 12, 2016 at 9:31 PM, Kevin Wolf  wrote:
> > Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> >> This series adds blockdev-add support for SSH block driver.
> >>
> >> Patch 1 prepares the code for the addition of a new option prefix,
> >> which is "server.". This is accomplished by adding a
> >> ssh_has_filename_options_conflict() function which helps to iterate
> >> over the various options and check for conflict.
> >>
> >> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
> >> and then makes it accept a InetSocketAddress under the "server" option.
> >> The old options "host" and "port" are supported as legacy options and
> >> then translated to the respective InetSocketAddress representation.
> >>
> >> Patch 3 drops the usage of "host" and "port" outside of
> >> ssh_has_filename_options_conflict() and
> >> ssh_process_legacy_socket_options() functions in order to make them
> >> legacy options completely.
> >>
> >> Patch 4 helps to allow blockdev-add support for the SSH block driver
> >> by making the SSH option available.
> >
> > Commented on patch 2, the rest looks good to me at first sight.
> 
> Yes, I am going through that currently and will ask you if I have any queries.
> 
> >
> > Just curious, what kind of testing did you give the series?
> 
> First thing I made sure if I wasn't breaking anything. Basically I
> checked if blockdev-add worked with it and then if I am able to remove
> it with x-blockdev-del.

Did you try out all of the options that we support, including those in
InetSocketAddress?

> Also, there were no major changes which could
> make the patches to break. I don't see tests available for SSH either
> so there was nothing much I can do.
> Do you have anything in mind?

Actually, qemu-iotests has ssh support. It's probably not run very
often, so I'm not sure whether it completely passes on master, but worth
a try anyway. Check out ./check --help in tests/qemu-iotests, you'll
probably want something like './check -T -ssh'.

The commit message that added ssh support to the tests says:

Note in order to run these tests on ssh, you must be running a local
ssh daemon, and that daemon must accept loopback connections, and
ssh-agent has to be set up to allow logins on the local daemon.  In
other words, the following command should just work without demanding
any passphrase:

 ssh localhost

Hope this is helpful.

Kevin

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 9:31 PM, Kevin Wolf  wrote:
> Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
>> This series adds blockdev-add support for SSH block driver.
>>
>> Patch 1 prepares the code for the addition of a new option prefix,
>> which is "server.". This is accomplished by adding a
>> ssh_has_filename_options_conflict() function which helps to iterate
>> over the various options and check for conflict.
>>
>> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
>> and then makes it accept a InetSocketAddress under the "server" option.
>> The old options "host" and "port" are supported as legacy options and
>> then translated to the respective InetSocketAddress representation.
>>
>> Patch 3 drops the usage of "host" and "port" outside of
>> ssh_has_filename_options_conflict() and
>> ssh_process_legacy_socket_options() functions in order to make them
>> legacy options completely.
>>
>> Patch 4 helps to allow blockdev-add support for the SSH block driver
>> by making the SSH option available.
>
> Commented on patch 2, the rest looks good to me at first sight.

Yes, I am going through that currently and will ask you if I have any queries.

>
> Just curious, what kind of testing did you give the series?

First thing I made sure if I wasn't breaking anything. Basically I
checked if blockdev-add worked with it and then if I am able to remove
it with x-blockdev-del. Also, there were no major changes which could
make the patches to break. I don't see tests available for SSH either
so there was nothing much I can do.
Do you have anything in mind?

Ashijeet

>
> Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] iotests: add transactional failure race test

[John Snow]

On 10/12/2016 07:26 AM, Vladimir Sementsov-Ogievskiy wrote:

it is almost a duplication of test_transaction_failure, I think it would
be better to make separate do_test_transaction_failure with parameter
and two wrappers



Yes, sorry -- I missed that for this iteration, but I'll act on it for 
the next iteration.



On 01.10.2016 01:00, John Snow wrote:

Add a regression test for the case found by Vladimir.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 91
++
  tests/qemu-iotests/124.out |  4 +-
  2 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 2f0bc24..b8f7dad 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -512,6 +512,97 @@ class
TestIncrementalBackup(TestIncrementalBackupBase):
  self.check_backups()
+def test_transaction_failure_race(self):
+'''Test: Verify that transactions with jobs that have no data to
+transfer do not cause race conditions in the cancellation of
the entire
+transaction job group.
+'''
+
+# Create a second drive, with pattern:
+drive1 = self.add_node('drive1')
+self.img_create(drive1['file'], drive1['fmt'])
+io_write_patterns(drive1['file'], (('0x14', 0, 512),
+   ('0x5d', '1M', '32k'),
+   ('0xcd', '32M', '124k')))
+
+# Create a blkdebug interface to this img as 'drive1'
+result = self.vm.qmp('blockdev-add', options={
+'node-name': drive1['id'],
+'driver': drive1['fmt'],
+'file': {
+'driver': 'blkdebug',
+'image': {
+'driver': 'file',
+'filename': drive1['file']
+},
+'set-state': [{
+'event': 'flush_to_disk',
+'state': 1,
+'new_state': 2
+}],
+'inject-error': [{
+'event': 'read_aio',
+'errno': 5,
+'state': 2,
+'immediately': False,
+'once': True
+}],
+}
+})
+self.assert_qmp(result, 'return', {})
+
+# Create bitmaps and full backups for both drives
+drive0 = self.drives[0]
+dr0bm0 = self.add_bitmap('bitmap0', drive0)
+dr1bm0 = self.add_bitmap('bitmap0', drive1)
+self.create_anchor_backup(drive0)
+self.create_anchor_backup(drive1)
+self.assert_no_active_block_jobs()
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+
+# Emulate some writes
+self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
+  ('0xef', '16M', '256k'),
+  ('0x46', '32736k', '64k')))
+
+# Create incremental backup targets
+target0 = self.prepare_backup(dr0bm0)
+target1 = self.prepare_backup(dr1bm0)
+
+# Ask for a new incremental backup per-each drive, expecting
drive1's
+# backup to fail and attempt to cancel the empty drive0 job.
+transaction = [
+transaction_drive_backup(drive0['id'], target0,
sync='incremental',
+ format=drive0['fmt'],
mode='existing',
+ bitmap=dr0bm0.name),
+transaction_drive_backup(drive1['id'], target1,
sync='incremental',
+ format=drive1['fmt'],
mode='existing',
+ bitmap=dr1bm0.name)
+]
+result = self.vm.qmp('transaction', actions=transaction,
+ properties={'completion-mode': 'grouped'} )
+self.assert_qmp(result, 'return', {})
+
+# Observe that drive0's backup is cancelled and drive1
completes with
+# an error.
+self.wait_qmp_backup_cancelled(drive0['id'])
+self.assertFalse(self.wait_qmp_backup(drive1['id']))
+error = self.vm.event_wait('BLOCK_JOB_ERROR')
+self.assert_qmp(error, 'data', {'device': drive1['id'],
+'action': 'report',
+'operation': 'read'})
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+self.assert_no_active_block_jobs()
+
+# Delete drive0's successful target and eliminate our record
of the
+# unsuccessful drive1 target.
+dr0bm0.del_target()
+dr1bm0.del_target()
+
+self.vm.shutdown()
+
+
+
  def test_sync_dirty_bitmap_missing(self):
  self.assert_no_active_block_jobs()
  self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[John Snow]

On 10/12/2016 06:22 AM, Kevin Wolf wrote:

Am 11.10.2016 um 17:47 hat John Snow geschrieben:

On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote:

On 10/10/16 17:34, Eric Blake wrote:


On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:

The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
necessarily the case for all platforms. Use this as the default alignment for
all current callers.

Signed-off-by: Mark Cave-Ayland 
---



@@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
return;
}

-if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
-qemu_iovec_discard_back(>iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
+if (dbs->iov.size & (dbs->align - 1)) {
+qemu_iovec_discard_back(>iov, dbs->iov.size & (dbs->align - 1));


Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
Semantically it is the same, but the macros make it obvious what the
bit-twiddling is doing.

Unless you think that needs a tweak,
Reviewed-by: Eric Blake 


I can't say I feel too strongly about it since there are plenty of other
examples of this style in the codebase, so I'm happy to go with whatever
John/Paolo are most happy with.


ATB,

Mark.



I can't pretend I am consistent, but when in doubt use the macro.
Not worth a respin IMO, but I think this falls out of my
jurisdiction :)

Acked-by: John Snow 


dma-helpers.c is officially unmaintained, and as the other patch is
clearly IDE, I think the series should go through your tree.

Kevin



Oh! I was under the impression that Paolo had the dma-helpers. My 
mistake. I will test and stage this.


--js

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

[Kevin Wolf]

Am 12.10.2016 um 17:50 hat Markus Armbruster geschrieben:
> "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 

> > @@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor 
> > *v, const char *name,
> >int64_t *obj, Error **errp)
> >  {
> >  QObjectInputVisitor *qiv = to_qiv(v);
> > -QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
> > -true));
> > +QString *qstr;
> >  int64_t ret;
> > +const 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 ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
> > +*obj = tos->range_val + 1;
> > +tos->range_val++;
> 
> Roundabout way to write
> 
>*obj = tos->range_val++;

*obj = ++tos->range_val, actually.

Kevin

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Kevin Wolf]

Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> This series adds blockdev-add support for SSH block driver.
> 
> Patch 1 prepares the code for the addition of a new option prefix,
> which is "server.". This is accomplished by adding a
> ssh_has_filename_options_conflict() function which helps to iterate
> over the various options and check for conflict.
> 
> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
> and then makes it accept a InetSocketAddress under the "server" option.
> The old options "host" and "port" are supported as legacy options and
> then translated to the respective InetSocketAddress representation.
> 
> Patch 3 drops the usage of "host" and "port" outside of
> ssh_has_filename_options_conflict() and
> ssh_process_legacy_socket_options() functions in order to make them
> legacy options completely.
> 
> Patch 4 helps to allow blockdev-add support for the SSH block driver
> by making the SSH option available.

Commented on patch 2, the rest looks good to me at first sight.

Just curious, what kind of testing did you give the series?

Kevin

Re: [Qemu-block] [PATCH 2/4] block/ssh: Add InetSocketAddress and accept it

[Kevin Wolf]

Am 11.10.2016 um 09:37 hat Ashijeet Acharya geschrieben:
> Add InetSocketAddress compatibility to SSH driver.
> 
> Add a new option "server" to the SSH block driver which then accepts
> a InetSocketAddress.
> 
> "host" and "port" are supported as legacy options and are mapped to
> their InetSocketAddress representation.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
>  block/ssh.c | 87 
> ++---
>  1 file changed, 78 insertions(+), 9 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index 75cb7bc..702871a 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -32,8 +32,11 @@
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "qemu/uri.h"
> +#include "qapi-visit.h"
>  #include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp-input-visitor.h"
> +#include "qapi/qmp-output-visitor.h"
>  
>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>   * this block driver code.
> @@ -74,6 +77,8 @@ typedef struct BDRVSSHState {
>   */
>  LIBSSH2_SFTP_ATTRIBUTES attrs;
>  
> +InetSocketAddress *inet;
> +
>  /* Used to warn if 'flush' is not supported. */
>  char *hostport;
>  bool unsafe_flush_warning;
> @@ -263,7 +268,9 @@ static bool ssh_has_filename_options_conflict(QDict 
> *options, Error **errp)
>  !strcmp(qe->key, "port") ||
>  !strcmp(qe->key, "path") ||
>  !strcmp(qe->key, "user") ||
> -!strcmp(qe->key, "host_key_check"))
> +!strcmp(qe->key, "host_key_check") ||
> +!strcmp(qe->key, "server") ||
> +!strncmp(qe->key, "server.", 7))

There is strstart() from cutils.c which looks a bit nicer (you don't
have to specify the string length then).

>  {
>  error_setg(errp, "Option '%s' cannot be used with a file name",
> qe->key);
> @@ -555,13 +562,71 @@ static QemuOptsList ssh_runtime_opts = {
>  },
>  };
>  
> +static bool ssh_process_legacy_socket_options(QDict *output_opts,
> +  QemuOpts *legacy_opts,
> +  Error **errp)
> +{
> +const char *host = qemu_opt_get(legacy_opts, "host");
> +const char *port = qemu_opt_get(legacy_opts, "port");
> +
> +if (!host && port) {
> +error_setg(errp, "port may not be used without host");
> +return false;
> +}

This check is rather pointless if !host causes an error anyway.

> +if (!host) {
> +error_setg(errp, "No hostname was specified");
> +return false;
> +} else {
> +qdict_put(output_opts, "server.host", qstring_from_str(host));
> +qdict_put(output_opts, "server.port",
> +  qstring_from_str(port ?: stringify(22)));
> +}
> +
> +return true;
> +}
> +
> +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
> + Error **errp)
> +{
> +InetSocketAddress *inet = NULL;
> +QDict *addr = NULL;
> +QObject *crumpled_addr = NULL;
> +Visitor *iv = NULL;
> +Error *local_error = NULL;
> +
> +qdict_extract_subqdict(options, , "server.");
> +if (!qdict_size(addr)) {
> +error_setg(errp, "SSH server address missing");
> +goto out;
> +}
> +
> +crumpled_addr = qdict_crumple(addr, true, errp);
> +if (!crumpled_addr) {
> +goto out;
> +}
> +
> +iv = qmp_input_visitor_new(crumpled_addr, true);

I believe you need qobject_input_visitor_new_autocast() here.

Do integer properties like port work for you without it?

> +visit_type_InetSocketAddress(iv, NULL, , _error);
> +if (local_error) {
> +error_propagate(errp, local_error);
> +goto out;
> +}
> +
> +out:
> +QDECREF(addr);
> +qobject_decref(crumpled_addr);
> +visit_free(iv);
> +return inet;
> +}
> +
>  static int connect_to_ssh(BDRVSSHState *s, QDict *options,
>int ssh_flags, int creat_mode, Error **errp)
>  {
>  int r, ret;
>  QemuOpts *opts = NULL;
>  Error *local_err = NULL;
> -const char *host, *user, *path, *host_key_check;
> +const char *user, *path, *host_key_check;
>  int port;
>  
>  opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
> @@ -572,15 +637,11 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
> *options,
>  goto err;
>  }
>  
> -host = qemu_opt_get(opts, "host");
> -if (!host) {
> +if (!ssh_process_legacy_socket_options(options, opts, errp)) {
>  ret = -EINVAL;
> -error_setg(errp, "No hostname was specified");
>  goto err;
>  }
>  
> -port = qemu_opt_get_number(opts, "port", 22);
> -
>  path = qemu_opt_get(opts, "path");
>  if (!path) {
>  ret = -EINVAL;
> @@ -603,9 +664,16 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
> *options,
>  

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

[Markus Armbruster]

"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(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 94051f3..242b767 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -19,6 +19,12 @@
>  
>  typedef struct QObjectInputVisitor QObjectInputVisitor;
>  
> +/* Inclusive upper bound on the size of any flattened range. This is a safety
> + * (= anti-annoyance) measure; wrong ranges should not cause long startup
> + * delays nor exhaust virtual memory.
> + */
> +#define QIV_RANGE_MAX 65536
> +
>  /**
>   * Create a new input visitor that converts @obj to a QAPI object.
>   *
> @@ -71,6 +77,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * The value given determines how many levels of structs are allowed to
>   * be flattened in this way.
>   *
> + * If @permit_int_ranges is true, then when visiting a list of integers,
> + * the integer value strings may encode ranges eg a single element
> + * containing "5-7" is treated as if there were three elements "5", "6",
> + * "7". This should only be used if compatibility is required with the
> + * OptsVisitor which would allow integer ranges. e.g. set this to true
> + * if you have compatibility requirements for
> + *
> + *   -arg val=5-8
> + *
> + * to be treated as equivalent to the preferred syntax:
> + *
> + *   -arg val.0=5,val.1=6,val.2=7,val.3=8
> + *
>   * The visitor always operates in strict mode, requiring all dict keys
>   * to be consumed during visitation. An error will be reported if this
>   * does not happen.
> @@ -80,7 +99,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   */
>  Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>  bool autocreate_list,
> -size_t autocreate_struct_levels);
> +size_t autocreate_struct_levels,
> +bool permit_int_ranges);
>  
>  
>  /**
> @@ -98,6 +118,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>  Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
>  bool autocreate_list,
>  size_t autocreate_struct_levels,
> +bool permit_int_ranges,
>  Error **errp);
>  
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 1be4865..6b3d0f2 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -31,6 +31,8 @@ typedef struct StackObject
>  
>  GHashTable *h;   /* If obj is dict: unvisited keys */
>  const QListEntry *entry; /* If obj is list: unvisited tail */
> +uint64_t range_val;
> +uint64_t range_limit;

It's either ugly unions or ugly type casts.  The options visitor picked
ugly unions, you're picking ugly type casts.  Matter of taste, as long
as the type casts are all kosher.

>  
>  QSLIST_ENTRY(StackObject) node;
>  } StackObject;
> @@ -60,6 +62,10 @@ struct QObjectInputVisitor
>   * consider auto-creating a struct containing
>   * remaining unvisited items */
>  size_t autocreate_struct_levels;
> +
> +/* Whether int lists can have single values representing
> + * ranges of values */
> +bool permit_int_ranges;
>  };
>  
>  static QObjectInputVisitor *to_qiv(Visitor *v)
> @@ -282,7 +288,7 @@ static GenericList *qobject_input_next_list(Visitor *v, 
> GenericList *tail,
>  QObjectInputVisitor *qiv = to_qiv(v);
>  StackObject *so = QSLIST_FIRST(>stack);
>  
> -if (!so->entry) {
> +if ((so->range_val == so->range_limit) && !so->entry) {
>  return NULL;
>  }
>  tail->next = g_malloc0(size);
> @@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor 
> *v, const char *name,
>int64_t *obj, Error **errp)
>  {
>  QObjectInputVisitor *qiv = 

[Qemu-block] [PATCH] hw/block/nvme: Simplify if-statements a little bit

[Thomas Huth]

The condition  '!A || (A && B)' is equivalent to '!A || B'.

Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
Signed-off-by: Thomas Huth 
---
 hw/block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cef3bb4..53d9d2e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
 if (!cqid || nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
-if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
+if (!sqid || !nvme_check_sqid(n, sqid)) {
 return NVME_INVALID_QID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
@@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
 uint16_t qflags = le16_to_cpu(c->cq_flags);
 uint64_t prp1 = le64_to_cpu(c->prp1);
 
-if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
+if (!cqid || !nvme_check_cqid(n, cqid)) {
 return NVME_INVALID_CQID | NVME_DNR;
 }
 if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
-- 
1.8.3.1

Re: [Qemu-block] [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Markus Armbruster  writes:

> Markus Armbruster  writes:
>
>> Markus Armbruster  writes:
>>
>>> "Daniel P. Berrange"  writes:
>>>
 Currently the QObjectInputVisitor assumes that all scalar
 values are directly represented as the final types declared
 by the thing being visited. ie it assumes an 'int' is using
>>>
>>> i.e.
>>>
 QInt, and a 'bool' is using QBool, etc.  This is good when
 QObjectInputVisitor is fed a QObject that came from a JSON
 document on the QMP monitor, as it will strictly validate
 correctness.

 To allow QObjectInputVisitor to be reused for visiting
 a QObject originating from QemuOpts, an alternative mode
 is needed where all the scalars types are represented as
 QString and converted on the fly to the final desired
 type.

 Reviewed-by: Kevin Wolf 
 Reviewed-by: Marc-André Lureau 
 Signed-off-by: Daniel P. Berrange 
>> [...]
 diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
 index 5ff3db3..cf41df6 100644
 --- a/qapi/qobject-input-visitor.c
 +++ b/qapi/qobject-input-visitor.c
 @@ -20,6 +20,7 @@
  #include "qemu-common.h"
  #include "qapi/qmp/types.h"
  #include "qapi/qmp/qerror.h"
 +#include "qemu/cutils.h"
  
  #define QIV_STACK_SIZE 1024
  
 @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, 
 const char *name, int64_t *obj,
  *obj = qint_get_int(qint);
  }
  
 +
 +static void qobject_input_type_int64_autocast(Visitor *v, const char 
 *name,
 +  int64_t *obj, Error **errp)
 +{
 +QObjectInputVisitor *qiv = to_qiv(v);
 +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
 +true));
>>>
>>> Needs a rebase for commit 1382d4a.  Same elsewhere.
>>>
 +int64_t ret;
 +
 +if (!qstr || !qstr->string) {
>>>
>>> I don't think !qstr->string can happen.  Same elsewhere.
>>>
 +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : 
 "null",
 +   "string");
 +return;
 +}
>>>
>>> So far, this is basically qobject_input_type_str() less the g_strdup().
>>> Worth factoring out?
>>>
>>> Now we're entering out parsing swamp:
>>>
 +
 +if (qemu_strtoll(qstr->string, NULL, 0, ) < 0) {
 +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>>
>> "integer", actually.
>
> Wait!  You're using QERR_INVALID_PARAMETER_VALUE here, so it's "an
> integer".
>
> To be pedantically correct, we'd have to complain about the type when
> qemu_strtoll() fails to parse, and about the value when it parses okay,
> but the value is out of range.

Nevermind, I got confused.  The type is actually always string here, so
your use of QERR_INVALID_PARAMETER_VALUE is appropriate.

[...]

Re: [Qemu-block] [Qemu-devel] [PATCH] hw/block/nvme: Simplify if-statements a little bit

[Peter Maydell]

On 12 October 2016 at 16:18, Thomas Huth  wrote:
> The condition  '!A || (A && B)' is equivalent to '!A || B'.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1464611
> Signed-off-by: Thomas Huth 
> ---
>  hw/block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index cef3bb4..53d9d2e 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
>  if (!cqid || nvme_check_cqid(n, cqid)) {
>  return NVME_INVALID_CQID | NVME_DNR;
>  }
> -if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
> +if (!sqid || !nvme_check_sqid(n, sqid)) {
>  return NVME_INVALID_QID | NVME_DNR;
>  }
>  if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
> @@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
>  uint16_t qflags = le16_to_cpu(c->cq_flags);
>  uint64_t prp1 = le64_to_cpu(c->prp1);
>
> -if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
> +if (!cqid || !nvme_check_cqid(n, cqid)) {
>  return NVME_INVALID_CQID | NVME_DNR;
>  }
>  if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Markus Armbruster  writes:

> Markus Armbruster  writes:
>
>> "Daniel P. Berrange"  writes:
>>
>>> Currently the QObjectInputVisitor assumes that all scalar
>>> values are directly represented as the final types declared
>>> by the thing being visited. ie it assumes an 'int' is using
>>
>> i.e.
>>
>>> QInt, and a 'bool' is using QBool, etc.  This is good when
>>> QObjectInputVisitor is fed a QObject that came from a JSON
>>> document on the QMP monitor, as it will strictly validate
>>> correctness.
>>>
>>> To allow QObjectInputVisitor to be reused for visiting
>>> a QObject originating from QemuOpts, an alternative mode
>>> is needed where all the scalars types are represented as
>>> QString and converted on the fly to the final desired
>>> type.
>>>
>>> Reviewed-by: Kevin Wolf 
>>> Reviewed-by: Marc-André Lureau 
>>> Signed-off-by: Daniel P. Berrange 
> [...]
>>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>>> index 5ff3db3..cf41df6 100644
>>> --- a/qapi/qobject-input-visitor.c
>>> +++ b/qapi/qobject-input-visitor.c
>>> @@ -20,6 +20,7 @@
>>>  #include "qemu-common.h"
>>>  #include "qapi/qmp/types.h"
>>>  #include "qapi/qmp/qerror.h"
>>> +#include "qemu/cutils.h"
>>>  
>>>  #define QIV_STACK_SIZE 1024
>>>  
>>> @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, const 
>>> char *name, int64_t *obj,
>>>  *obj = qint_get_int(qint);
>>>  }
>>>  
>>> +
>>> +static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
>>> +  int64_t *obj, Error **errp)
>>> +{
>>> +QObjectInputVisitor *qiv = to_qiv(v);
>>> +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>>> +true));
>>
>> Needs a rebase for commit 1382d4a.  Same elsewhere.
>>
>>> +int64_t ret;
>>> +
>>> +if (!qstr || !qstr->string) {
>>
>> I don't think !qstr->string can happen.  Same elsewhere.
>>
>>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>> +   "string");
>>> +return;
>>> +}
>>
>> So far, this is basically qobject_input_type_str() less the g_strdup().
>> Worth factoring out?
>>
>> Now we're entering out parsing swamp:
>>
>>> +
>>> +if (qemu_strtoll(qstr->string, NULL, 0, ) < 0) {
>>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>
> "integer", actually.

Wait!  You're using QERR_INVALID_PARAMETER_VALUE here, so it's "an
integer".

To be pedantically correct, we'd have to complain about the type when
qemu_strtoll() fails to parse, and about the value when it parses okay,
but the value is out of range.

>>> +return;
>>> +}
>>
>> To serve as replacement for the options visitor, this needs to parse
>> exactly like opts_type_int64().
>>
>> It should also match the JSON parser as far as possible, to minimize
>> difference between the two QObject input visitor variants, and the
>> QemuOpts parser, for command line consistency.
>>
>> opts_type_int64() uses strtoll() directly.  It carefully checks for no
>> conversion (both ways, EINVAL and endptr == str), range, and string not
>> fully consumed.
>>
>> Your code uses qemu_strtoll().  Bug#1: qemu_strtoll() assumes long long
>> is exactly 64 bits.  If it's wider, we fail to diagnose overflow.  If
>> it's narrower, we can't parse all values.  Bug#2: your code fails to
>> check the string is fully consumed.
>>
>> The JSON parser also uses strtoll() directly, in parse_literal().  When
>> we get there, we know that the string consists only of decimal digits,
>> possibly prefixed by a minus sign.  Therefore, strtoll() can only fail
>> with ERANGE, and parse_literal() handles that correctly.
>>
>> QemuOpts doesn't do signed integers.
>>
>>> +*obj = ret;
>>> +}
>>> +
>>>  static void qobject_input_type_uint64(Visitor *v, const char *name,
>>>uint64_t *obj, Error **errp)
>>>  {
>>> @@ -279,6 +302,27 @@ static void qobject_input_type_uint64(Visitor *v, 
>>> const char *name,
>>>  *obj = qint_get_int(qint);
>>>  }
>>>  
>>> +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));
>>> +unsigned long long ret;
>>> +
>>> +if (!qstr || !qstr->string) {
>>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>>> +   "string");
>>> +return;
>>> +}
>>> +
>>> +if (parse_uint_full(qstr->string, , 0) < 0) {
>>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, 

Re: [Qemu-block] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 12.10.2016 um 16:33 hat Alberto Garcia geschrieben:
> On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf  wrote:
> >> +/* Block all intermediate nodes between bs and base, because they
> >> + * will disappear from the chain after this operation */
> >> +for (iter = backing_bs(bs); iter && iter != base; iter = 
> >> backing_bs(iter)) {
> >> +block_job_add_bdrv(>common, iter);
> >> +}
> >
> > In patch 6, you had a comment that the top node must be blocked as
> > well because its backing file string will be updated. Isn't it the
> > same for streaming?
> 
> In the block-stream case, 'device' is always the top node, and creating
> the block job there already blocks all operations in it.
> 
> In the block-commit case, 'device' and 'top' are different parameters,
> that's why 'top' must be explicitly blocked. I actually don't think that
> we need to block 'device' at all, and we could even make the parameter
> optional. That would be for a future patch, though. Also, the backing
> file string is not updated in 'top', but in its overlay (unlike in
> block-stream, 'top' disappears after a block-commit job).

I see. So the block job for commit is owned by a node that is
potentially completely unrelated to the operation except that it holds
op blockers and because we use it to finde the overlay to change the
backing file pointers. This is _so_ broken. :-)

With your series (for the op blockers) and BdrvChild (for the operations
on the overlay) we should actually be much closer to finally remove
this. Good news.

Kevin

Re: [Qemu-block] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:30:27 PM CEST, Kevin Wolf  wrote:
>>  if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>>  goto out;
>>  }
>
> Added a bit more context.
>
> This check is redundant now...
>
>>  if (has_base) {
>>  base_bs = bdrv_find_backing_image(bs, base);
>>  if (base_bs == NULL) {
>>  error_setg(errp, QERR_BASE_NOT_FOUND, base);
>>  goto out;
>>  }
>>  assert(bdrv_get_aio_context(base_bs) == aio_context);
>>  base_name = base;
>>  }
>>  
>> +/* Check for op blockers in the whole chain between bs and base */
>> +for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
>> +if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
>> +goto out;
>> +}
>> +}
>
> ...because you start with iter = bs here.

You're right, I'll fix it.

> Another thought I had while looking at the previous few patches is
> whether all the op blocker checks wouldn't be better moved to the
> actual block job code (i.e. stream_start in this case).

I thought about that too. In some cases I don't know if it's a good idea
because the qmp_foo_bar() functions do a bit more than simply checking
blockers (e.g. blockdev-mirror creates the target image), so you would
want to have the checks before that.

However doing it in the actual block job code could allow us to do other
things. For example: at the moment when we call block-stream we check
whether a number of BDSs are blocked (in qmp_block_stream()), and if
they're not we proceed to block them (in stream_start()). We could make
block_job_add_bdrv() do both things. On the other hand, since the plan
is to move to a new block job API maybe it's better not to overdo things
now.

It's worth considering for the future anyway.

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor

[Markus Armbruster]

Markus Armbruster  writes:

> "Daniel P. Berrange"  writes:
>
>> Currently the QObjectInputVisitor assumes that all scalar
>> values are directly represented as the final types declared
>> by the thing being visited. ie it assumes an 'int' is using
>
> i.e.
>
>> QInt, and a 'bool' is using QBool, etc.  This is good when
>> QObjectInputVisitor is fed a QObject that came from a JSON
>> document on the QMP monitor, as it will strictly validate
>> correctness.
>>
>> To allow QObjectInputVisitor to be reused for visiting
>> a QObject originating from QemuOpts, an alternative mode
>> is needed where all the scalars types are represented as
>> QString and converted on the fly to the final desired
>> type.
>>
>> Reviewed-by: Kevin Wolf 
>> Reviewed-by: Marc-André Lureau 
>> Signed-off-by: Daniel P. Berrange 
[...]
>> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> index 5ff3db3..cf41df6 100644
>> --- a/qapi/qobject-input-visitor.c
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -20,6 +20,7 @@
>>  #include "qemu-common.h"
>>  #include "qapi/qmp/types.h"
>>  #include "qapi/qmp/qerror.h"
>> +#include "qemu/cutils.h"
>>  
>>  #define QIV_STACK_SIZE 1024
>>  
>> @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, const 
>> char *name, int64_t *obj,
>>  *obj = qint_get_int(qint);
>>  }
>>  
>> +
>> +static void qobject_input_type_int64_autocast(Visitor *v, const char *name,
>> +  int64_t *obj, Error **errp)
>> +{
>> +QObjectInputVisitor *qiv = to_qiv(v);
>> +QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> +true));
>
> Needs a rebase for commit 1382d4a.  Same elsewhere.
>
>> +int64_t ret;
>> +
>> +if (!qstr || !qstr->string) {
>
> I don't think !qstr->string can happen.  Same elsewhere.
>
>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> +   "string");
>> +return;
>> +}
>
> So far, this is basically qobject_input_type_str() less the g_strdup().
> Worth factoring out?
>
> Now we're entering out parsing swamp:
>
>> +
>> +if (qemu_strtoll(qstr->string, NULL, 0, ) < 0) {
>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");

"integer", actually.

>> +return;
>> +}
>
> To serve as replacement for the options visitor, this needs to parse
> exactly like opts_type_int64().
>
> It should also match the JSON parser as far as possible, to minimize
> difference between the two QObject input visitor variants, and the
> QemuOpts parser, for command line consistency.
>
> opts_type_int64() uses strtoll() directly.  It carefully checks for no
> conversion (both ways, EINVAL and endptr == str), range, and string not
> fully consumed.
>
> Your code uses qemu_strtoll().  Bug#1: qemu_strtoll() assumes long long
> is exactly 64 bits.  If it's wider, we fail to diagnose overflow.  If
> it's narrower, we can't parse all values.  Bug#2: your code fails to
> check the string is fully consumed.
>
> The JSON parser also uses strtoll() directly, in parse_literal().  When
> we get there, we know that the string consists only of decimal digits,
> possibly prefixed by a minus sign.  Therefore, strtoll() can only fail
> with ERANGE, and parse_literal() handles that correctly.
>
> QemuOpts doesn't do signed integers.
>
>> +*obj = ret;
>> +}
>> +
>>  static void qobject_input_type_uint64(Visitor *v, const char *name,
>>uint64_t *obj, Error **errp)
>>  {
>> @@ -279,6 +302,27 @@ static void qobject_input_type_uint64(Visitor *v, const 
>> char *name,
>>  *obj = qint_get_int(qint);
>>  }
>>  
>> +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));
>> +unsigned long long ret;
>> +
>> +if (!qstr || !qstr->string) {
>> +error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> +   "string");
>> +return;
>> +}
>> +
>> +if (parse_uint_full(qstr->string, , 0) < 0) {
>> +error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");

"integer".

>> +return;
>> +}
>> +*obj = ret;
>
> Differently wrong :)
>
> To serve as replacement for the options visitor, this needs to parse
> exactly like opts_type_uint64().
>
> Again, this should also match the JSON parser and the QemuOpts parser as
> far as possible.
>
> opts_type_uint64() uses parse_uint().  It carefully checks for no
> conversion (EINVAL; parse_uint() normalizes), range, 

Re: [Qemu-block] [PATCH v10 10/16] docs: Document how to stream to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Signed-off-by: Alberto Garcia 
> ---
>  docs/live-block-ops.txt | 31 ---
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
> index a257087..014c8c9 100644
> --- a/docs/live-block-ops.txt
> +++ b/docs/live-block-ops.txt
> @@ -10,9 +10,9 @@ Snapshot live merge
>  Given a snapshot chain, described in this document in the following
>  format:
>  
> -[A] -> [B] -> [C] -> [D]
> +[A] <- [B] <- [C] <- [D] <- [E]
>  
> -Where the rightmost object ([D] in the example) described is the current
> +Where the rightmost object ([E] in the example) described is the current
>  image which the guest OS has write access to. To the left of it is its base
>  image, and so on accordingly until the leftmost image, which has no
>  base.
> @@ -21,11 +21,14 @@ The snapshot live merge operation transforms such a chain 
> into a
>  smaller one with fewer elements, such as this transformation relative
>  to the first example:
>  
> -[A] -> [D]
> +[A] <- [E]
>  
> -Currently only forward merge with target being the active image is
> -supported, that is, data copy is performed in the right direction with
> -destination being the rightmost image.
> +Data is copied in the right direction with destination being the
> +rightmost image, but any other intermediate image can be specified
> +instead. In this example data is copied from [C] into [D], so [D] can
> +be backed by [B]:
> +
> +[A] <- [B] <- [D] <- [E]
>  
>  The operation is implemented in QEMU through image streaming facilities.

This whole document is hopelessly outdated. At least, we need to clarify
here that streaming isn't the only operation that exists and that the
explanation refers to streaming only.

Ideally we would also add a section on commit. And likeweise, the next
section about "live block copy" should be extended to cover mirror and
backup.

Kevin

Re: [Qemu-block] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This patch makes the 'device' parameter of the 'block-stream' command
> accept a node name that is not a root node.
> 
> In addition to that, operation blockers will be checked in all
> intermediate nodes between the top and the base node.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  blockdev.c   | 11 +--
>  qapi/block-core.json | 10 +++---
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index f13fc69..57c8329 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2937,7 +2937,7 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>bool has_on_error, BlockdevOnError on_error,
>Error **errp)
>  {
> -BlockDriverState *bs;
> +BlockDriverState *bs, *iter;
>  BlockDriverState *base_bs = NULL;
>  AioContext *aio_context;
>  Error *local_err = NULL;
> @@ -2947,7 +2947,7 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>  on_error = BLOCKDEV_ON_ERROR_REPORT;
>  }
>  
> -bs = qmp_get_root_bs(device, errp);
> +bs = bdrv_lookup_bs(device, device, errp);
>  if (!bs) {
>  return;
>  }
>  aio_context = bdrv_get_aio_context(bs);
>  aio_context_acquire(aio_context);
>  
>  if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
>  goto out;
>  }

Added a bit more context.

This check is redundant now...

>  if (has_base) {
>  base_bs = bdrv_find_backing_image(bs, base);
>  if (base_bs == NULL) {
>  error_setg(errp, QERR_BASE_NOT_FOUND, base);
>  goto out;
>  }
>  assert(bdrv_get_aio_context(base_bs) == aio_context);
>  base_name = base;
>  }
>  
> +/* Check for op blockers in the whole chain between bs and base */
> +for (iter = bs; iter && iter != base_bs; iter = backing_bs(iter)) {
> +if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
> +goto out;
> +}
> +}

...because you start with iter = bs here.

Another thought I had while looking at the previous few patches is
whether all the op blocker checks wouldn't be better moved to the actual
block job code (i.e. stream_start in this case). In this case it doesn't
matter much because this is the only caller of stream_start(), but for
other jobs the situation is more complicated and it would be easy for a
caller to forget the checks.

Probably also matter for another series, but I wanted to mention it.

> +
>  /* if we are streaming the entire chain, the result will have no backing
>   * file, and specifying one is therefore an error */
>  if (base_bs == NULL && has_backing_file) {

Kevin

Re: [Qemu-block] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Alberto Garcia]

On Wed 12 Oct 2016 04:23:05 PM CEST, Kevin Wolf  wrote:
>> +/* Block all intermediate nodes between bs and base, because they
>> + * will disappear from the chain after this operation */
>> +for (iter = backing_bs(bs); iter && iter != base; iter = 
>> backing_bs(iter)) {
>> +block_job_add_bdrv(>common, iter);
>> +}
>
> In patch 6, you had a comment that the top node must be blocked as
> well because its backing file string will be updated. Isn't it the
> same for streaming?

In the block-stream case, 'device' is always the top node, and creating
the block job there already blocks all operations in it.

In the block-commit case, 'device' and 'top' are different parameters,
that's why 'top' must be explicitly blocked. I actually don't think that
we need to block 'device' at all, and we could even make the parameter
optional. That would be for a future patch, though. Also, the backing
file string is not updated in 'top', but in its overlay (unlike in
block-stream, 'top' disappears after a block-commit job).

Berto

Re: [Qemu-block] [PATCH v10 08/16] block: Support streaming to an intermediate layer

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> This makes sure that the image we are streaming into is open in
> read-write mode during the operation.
> 
> Operation blockers are also set in all intermediate nodes, since they
> will be removed from the chain afterwards.
> 
> Finally, this also unblocks the stream operation in backing files.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c|  4 +++-
>  block/stream.c | 24 
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index c80b528..8255d75 100644
> --- a/block.c
> +++ b/block.c
> @@ -1428,9 +1428,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>  backing_hd->drv ? backing_hd->drv->format_name : "");
>  
>  bdrv_op_block_all(backing_hd, bs->backing_blocker);
> -/* Otherwise we won't be able to commit due to check in bdrv_commit */
> +/* Otherwise we won't be able to commit or stream */
>  bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
>  bs->backing_blocker);
> +bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
> +bs->backing_blocker);
>  /*
>   * We do backup in 3 ways:
>   * 1. drive backup
> diff --git a/block/stream.c b/block/stream.c
> index 3187481..b8ab89a 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -37,6 +37,7 @@ typedef struct StreamBlockJob {
>  BlockDriverState *base;
>  BlockdevOnError on_error;
>  char *backing_file_str;
> +int bs_flags;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> @@ -81,6 +82,11 @@ static void stream_complete(BlockJob *job, void *opaque)
>  bdrv_set_backing_hd(bs, base);
>  }
>  
> +/* Reopen the image back in read-only mode if necessary */
> +if (s->bs_flags != bdrv_get_flags(bs)) {
> +bdrv_reopen(bs, s->bs_flags, NULL);
> +}
> +
>  g_free(s->backing_file_str);
>  block_job_completed(>common, data->ret);
>  g_free(data);
> @@ -220,6 +226,8 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>BlockCompletionFunc *cb, void *opaque, Error **errp)
>  {
>  StreamBlockJob *s;
> +BlockDriverState *iter;
> +int orig_bs_flags;
>  
>  s = block_job_create(job_id, _job_driver, bs, speed,
>   cb, opaque, errp);
> @@ -227,8 +235,24 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>  return;
>  }
>  
> +/* Make sure that the image is opened in read-write mode */
> +orig_bs_flags = bdrv_get_flags(bs);
> +if (!(orig_bs_flags & BDRV_O_RDWR)) {
> +if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
> +block_job_unref(>common);
> +return;
> +}
> +}
> +
> +/* Block all intermediate nodes between bs and base, because they
> + * will disappear from the chain after this operation */
> +for (iter = backing_bs(bs); iter && iter != base; iter = 
> backing_bs(iter)) {
> +block_job_add_bdrv(>common, iter);
> +}

In patch 6, you had a comment that the top node must be blocked as well
because its backing file string will be updated. Isn't it the same for
streaming?

>  s->base = base;
>  s->backing_file_str = g_strdup(backing_file_str);
> +s->bs_flags = orig_bs_flags;
>  
>  s->on_error = on_error;
>  s->common.co = qemu_coroutine_create(stream_run, s);

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v14 10/21] qapi: permit auto-creating nested structs

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> Some of the historical command line opts that had their
> keys in in a completely flat namespace are now represented
> by QAPI schemas that use a nested structs. When converting
> the QemuOpts to QObject, there is no information about
> compound types available, so the QObject will be completely
> flat, even after the qdict_crumple() call. So when starting
> a struct, we may not have a QDict available in the input
> data, so we auto-create a QDict containing all the currently
> unvisited input data keys. Not all historical command line
> opts require this, so the behaviour is opt-in, by specifying
> how many levels of structs are permitted to be auto-created.
>
> Note that this only works if the child struct is the last
> field to the visited in the parent struct. This is always
> the case for currently existing legacy command line options.
>
> The example is the NetLegacy type which has 3 levels of
> structs. The modern way to represent this in QemuOpts would
> be the dot-separated component approach
>
>   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>opts.data.fd=3,opts.data.script=ifup
>
> The legacy syntax will just be presenting
>
>   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup
>
> So we need to auto-create 3 levels of struct when visiting.
>
> The implementation here will enable visiting in both the
> modern and legacy syntax, compared to OptsVisitor which
> only allows the legacy syntax.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h |  22 +-
>  qapi/qobject-input-visitor.c |  59 ++--
>  tests/test-qobject-input-visitor.c   | 130 
> ---
>  3 files changed, 194 insertions(+), 17 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 1809f48..94051f3 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -45,7 +45,7 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * If @autocreate_list is true, then as an alternative to a normal QList,
>   * list values can be stored as a QString or QDict instead, which will
>   * be interpreted as representing single element lists. This should only
> - * by used if compatibility is required with the OptsVisitor which allowed
> + * be used if compatibility is required with the OptsVisitor which allowed
>   * repeated keys, without list indexes, to represent lists. e.g. set this
>   * to true if you have compatibility requirements for
>   *

Typo introduced in the previous patch, please fix it there.

Skipping the rest of this patch until it's clear we need it.

[...]

Re: [Qemu-block] [PATCH v10 07/16] block: Block all intermediate nodes in commit_active_start()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> When block-commit is launched without the top parameter, it uses
> internally a mirror block job. In that case all intermediate nodes
> between the active and base nodes must be blocked as well.
> 
> Signed-off-by: Alberto Garcia 

Same as the patch before, dataplane is okay because AioContexts are
switched together with the source image.

On second thought, however, maybe both places should set a blocker that
prevents attaching intermediate nodes to a new block device because you
can't read consistent data there. That's a preexisting problem, though,
and needs its own patch.

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [Qemu-devel] [PATCH v14 10/21] qapi: permit auto-creating nested structs

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> On Thu, Oct 06, 2016 at 05:51:57PM +0200, Kevin Wolf wrote:
>> Am 06.10.2016 um 17:39 hat Daniel P. Berrange geschrieben:
>> > On Thu, Oct 06, 2016 at 05:30:05PM +0200, Kevin Wolf wrote:
>> > > Am 06.10.2016 um 17:18 hat Daniel P. Berrange geschrieben:
>> > > > On Thu, Oct 06, 2016 at 05:10:42PM +0200, Kevin Wolf wrote:
>> > > > > Am 30.09.2016 um 16:45 hat Daniel P. Berrange geschrieben:
>> > > > > > The example is the NetLegacy type which has 3 levels of
>> > > > > > structs. The modern way to represent this in QemuOpts would
>> > > > > > be the dot-separated component approach
>> > > > > > 
>> > > > > >   -net vlan=1,id=foo,name=bar,opts.type=tap,\
>> > > > > >opts.data.fd=3,opts.data.script=ifup
>> > > > > > 
>> > > > > > The legacy syntax will just be presenting
>> > > > > > 
>> > > > > >   -net vlan=1,id=foo,name=bar,type=tap,fd=3,script=ifup
>> > > > > > 
>> > > > > > So we need to auto-create 3 levels of struct when visiting.
>> > > > > > 
>> > > > > > The implementation here will enable visiting in both the
>> > > > > > modern and legacy syntax, compared to OptsVisitor which
>> > > > > > only allows the legacy syntax.
>> > > > > 
>> > > > > So you're actually introducing the modern syntax only now?
>> > > > 
>> > > > No, the modern syntax is fully implemented by patch 8.
>> > > 
>> > > "now" in the sense of "in this series" (because this means that there is
>> > > no external API to preserve yet).
>> > 
>> > Well the syntax implemented in patch 8 is designed to
>> > 100% mirror the QAPI schema structure nesting. I don't
>> > think we want to change that behaviour.
>> 
>> I think patch 8 is fine as it is.
>> 
>> > If there are certain QAPI schemas structs can still
>> > have freedom to change though, its fine to reconsider
>> > them
>> > 
>> > > 
>> > > > This patch is about adding hacks for the legacy syntax used
>> > > > by the OptsVisitor. The OptsVisitor didn't interpret struct
>> > > > nesting at all, so everything looked flat - this only works
>> > > > as long as you don't have the same key used in multiple
>> > > > structs at different levels, so is not useful as a general
>> > > > approach - it only works by luck really.
>> > > > 
>> > > > > I don't think an "opts.data." prefix makes a lot of sense. I suspect 
>> > > > > the
>> > > > > schema looks this way only because we didn't have flat unions in 1.2.
>> > > > >
>> > > > > So, considering that it is a purely internally used type not visible 
>> > > > > in
>> > > > > QMP, would it make sense to change NetLegacy to be a flat union 
>> > > > > instead,
>> > > > > with NetLegacyOptions as the common base? Then you get the same flat
>> > > > > namespace that we always had and that makes much more sense as an 
>> > > > > API.
>> > > > 
>> > > > Changing that will impact on the QMP data structure, so I don't think
>> > > > we can do that.
>> > > 
>> > > I don't see this type used in QMP at all. It's only used for command
>> > > line parsing, and only with the OptsVisitor, so I think we're fine if we
>> > > flatten it now.
>> > 
>> > Ok, yes, it seems "NetLegacy" is only used in CLI arg parsing, so we
>> > do have some freedom there.
>> > 
>> > This patch was also needed for -numa handling too - again we might have
>> > some flexibility to flatten that.
>> 
>> NumaOptions is also unused in QMP, so it's the same thing.

For better or worse, the QAPI schema doesn't separate externally visible
stuff from purely internal stuff.  Useful trick to see what's external:

$ python -B scripts/qapi-introspect.py -cu -p scratch- qapi-schema.json

This generates scratch-qmp-introspect.c describing the external QMP
interface with unmasked type names (-u).  Anything not mentioned there
can be changed freely.

I'd like to see patches flattening simple unions that aren't externally
visible.  Flat carries a bit of notational overhead in the schema, but I
hope to address that once the QAPI queue is under control.

>> If these two are the only options that need the behaviour, I would
>> prefer if we changed the QAPI schema to flatten them, and then we could
>> save ourselves some complexity by dropping this patch.
>
> Agreed, the fewer hacks like this that we need the better.

Indeed.  Please find out which hacks we actually need.

I believe the best way to keep the warts in check is getting rid of the
options visitor (done in this series) and the string visitor (hopefully
enabled by this series).

Re: [Qemu-block] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Alberto Garcia]

On Wed 12 Oct 2016 03:47:34 PM CEST, Kevin Wolf  wrote:
> Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
>> Use block_job_add_bdrv() instead of blocking all operations in
>> backup_start() and unblocking them in backup_run().
>> 
>> Signed-off-by: Alberto Garcia 
>
> This has the same problem as mirror (dataplane must be blocked).

Yeah, I think I'll keep dataplane blocked in all cases except in
block_job_create(). BLOCK_OP_TYPE_DATAPLANE is only checked in root
nodes anyway.

Berto

Re: [Qemu-block] [PATCH v10 06/16] block: Block all nodes involved in the block-commit operation

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> After a successful block-commit operation all nodes between top and
> base are removed from the backing chain, and top's overlay needs to
> be updated to point to base. Because of that we should prevent other
> block jobs from messing with them.
> 
> This patch blocks all operations in these nodes in commit_start().

Again, except for dataplane. But this time, I think it's actually okay
because backing files of the source automatically stay in the same
AioContext as the source. Just mention it in the commit message.

> Signed-off-by: Alberto Garcia 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v10 05/16] block: Check blockers in all nodes involved in a block-commit job

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> qmp_block_commit() checks for op blockers in the active and
> destination (base) images. However all nodes between top_bs and base
> are also involved, and they are removed from the chain afterwards.
> 
> In addition to that, if top_bs is not the active layer then top_bs's
> overlay also needs to be checked because it's involved in the job (its
> backing image string needs to be updated to point to 'base').
> 
> This patch checks that none of those nodes are blocked.
> 
> Signed-off-by: Alberto Garcia 

Reviewed-by: Kevin Wolf 

Re: [Qemu-block] [PATCH v10 04/16] block: Use block_job_add_bdrv() in backup_start()

[Kevin Wolf]

Am 06.10.2016 um 15:02 hat Alberto Garcia geschrieben:
> Use block_job_add_bdrv() instead of blocking all operations in
> backup_start() and unblocking them in backup_run().
> 
> Signed-off-by: Alberto Garcia 

This has the same problem as mirror (dataplane must be blocked).

Kevin

Re: [Qemu-block] [PATCH 09/22] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

On 12.10.2016 14:38, Vladimir Sementsov-Ogievskiy wrote:

On 07.10.2016 22:28, Max Reitz wrote:

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

New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
on bdrv_close, using format driver. Format driver should maintain 
bitmap

storing.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c  | 30 ++
  block/dirty-bitmap.c | 27 +++
  block/qcow2-bitmap.c |  1 +
  include/block/block.h|  2 ++
  include/block/block_int.h|  2 ++
  include/block/dirty-bitmap.h |  6 ++
  6 files changed, 68 insertions(+)

[...]


diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 623e1d1..0314581 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c

[...]

@@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const 
BdrvDirtyBitmap *bitmap)

  {
  return bitmap->autoload;
  }
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+bool persistent)
+{
+bitmap->persistent = persistent;

After some thinking, I think this function should be more complex: It
should check whether the node the bitmap is attached to actually can
handle persistent bitmaps and whether it would actually support storing
*this* bitmap.

For instance, a qcow2 node would not support writing overly large
bitmaps (limited by BME_MAX_TABLE_SIZE and BME_MAX_PHYS_SIZE) or bitmaps
with overly large granularities (BME_MAX_GRANULARITY_BITS) or bitmaps
whose name is already occupied by some bitmap that is already stored in
the file but has not been loaded.

Checking this here will trivially prevent users from creating such
bitmaps and will also preempt detection of such failures during
bdrv_close() when they cannot be handled gracefully.

Max


Good point, but I can't do it exactly as you say, because I call this 
function from qcow2_read_bitmaps, for just created bitmap and it 
should not be checked and of course it's name is occupied..


So, I'll add an additional checking function, to call it from 
qmp_block_dirty_bitmap_add, if persistent parameter is set to true.







+}






--
Best regards,
Vladimir

Re: [Qemu-block] [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Markus Armbruster]

Alberto Garcia  writes:

> On Tue 11 Oct 2016 06:32:39 PM CEST, Markus Armbruster wrote:
>
 3) QEMU could advertise that feature to the client. This is probably
 simpler than trying to figure it out from the API. I guess that's
 the idea of 'qmp_capabilities'?
>>>
>>> I think that was the idea, though it was never used. If we had used
>>> it, I'm not sure how long the capabilities list would be today. :-)
>>
>> QMP capabilities are for changes in the QMP protocol, not for changes
>> in commands, events and types.  The protocol has been good enough so
>> far, thus no capabilities.
>
> I see. Wouldn't it make sense in general to have a way to ask QEMU
> whether some certain feature is supported?

Feature bits work fine for interfaces that see relatively little change.
Some QEMU interfaces are like that, for instance the QMP protocol
itself.  Other interfaces, however, change at a rapid pace.  If we tried
to provide a feature bit for every change there, we'd end up with a huge
number of them, almost certainly underdocumented, and most of them not
used by any client.  We'd probably fail to provide feature bits for some
of the interface changes, and have "feature X" bits followed some time
later by "feature X, except now it actually works" bits.

Instead, we opted for making external interfaces introspectable, so that
clients can detect stuff whether we thought of the need to detect it or
not.

The first interface with decent introspection is QMP: there's
query-qmp-schema.  Changes are easy to detect there as long as they're
*syntactic*: new commands or events, type extensions and so forth.  QMP
introspection is blind to purely *semantic* changes, say a string
argument that can now denote a node-name in addition to a filename.
Such changes should be avoided.

Re: [Qemu-block] [PATCH 09/22] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

On 07.10.2016 22:28, Max Reitz wrote:

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

New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
on bdrv_close, using format driver. Format driver should maintain bitmap
storing.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c  | 30 ++
  block/dirty-bitmap.c | 27 +++
  block/qcow2-bitmap.c |  1 +
  include/block/block.h|  2 ++
  include/block/block_int.h|  2 ++
  include/block/dirty-bitmap.h |  6 ++
  6 files changed, 68 insertions(+)

[...]


diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 623e1d1..0314581 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c

[...]


@@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap)
  {
  return bitmap->autoload;
  }
+
+void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
+bool persistent)
+{
+bitmap->persistent = persistent;

After some thinking, I think this function should be more complex: It
should check whether the node the bitmap is attached to actually can
handle persistent bitmaps and whether it would actually support storing
*this* bitmap.

For instance, a qcow2 node would not support writing overly large
bitmaps (limited by BME_MAX_TABLE_SIZE and BME_MAX_PHYS_SIZE) or bitmaps
with overly large granularities (BME_MAX_GRANULARITY_BITS) or bitmaps
whose name is already occupied by some bitmap that is already stored in
the file but has not been loaded.

Checking this here will trivially prevent users from creating such
bitmaps and will also preempt detection of such failures during
bdrv_close() when they cannot be handled gracefully.

Max


Good point, but I can't do it exactly as you say, because I call this 
function from qcow2_read_bitmaps, for just created bitmap and it should 
not be checked and of course it's name is occupied..





+}



--
Best regards,
Vladimir

Re: [Qemu-block] [PATCH 00/18] Dirty bitmaps postcopy migration

[Vladimir Sementsov-Ogievskiy]

ping

For now there are some notes mostly about accessory patches. What about 
migration itself? Is it ok?


On 16.08.2016 13:25, Vladimir Sementsov-Ogievskiy wrote:

v2:
some bugs fixed, iotests a bit changed and merged into one test.
based on block-next (https://github.com/XanClic/qemu/commits/block-next)
clone: tag postcopy-v2 from https://src.openvz.org/scm/~vsementsov/qemu.git
online: 
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fpostcopy-v2

v1:

These series are derived from my 'Dirty bitmaps migration' series. The
core idea is switch to postcopy migration and drop usage of meta
bitmaps.

These patches provide dirty bitmap postcopy migration feature. Only
named dirty bitmaps are to be migrated. Migration may be enabled using
migration capabilities.

The overall method (thanks to John Snow):

1. migrate bitmaps meta data in .save_live_setup
- create/find related bitmaps on target
- disable them
- create successors (anonimous children) only for enabled migrated
  bitmaps
2. do nothing in precopy stage
3. just before target vm start: enable successors, created in (1)
4. migrate bitmap data
5. reclaime bitmaps (merge successors to their parents)
6. enable bitmaps (only bitmaps, which was enabled in source)


Some patches are unchnaged from (v7) of 'Dirty bitmaps migration'
(DBMv7). I've left Reviewed-by's for them, if you don't like it, say me
and I'll drop them in the following version.

So, relatively to last DBMv7:

01-04: new patches, splitting common postcopy migration out of ram
postcopy migration
05: equal to DBMv7.05
06: new
07: equal to DBMv7.06
08: new
09: equal to DBMv7.07
10: new
11: derived from DBMv7.08, see below
12-15: equal to DBMv7.09-12
16: derived from DBMv7.13
- switch from fifo to socket, as postcopy don't work with fifo
  for now
- change parameters: size, granularity, regions
- add time.sleep, to wait for postcopy migration phase (bad
  temporary solution.
- drop Reviewed-by
17: new

11: the core patch of the series, it is derived from
[DBMv7.08: migration: add migration_block-dirty-bitmap.c]
There are a lot of changes related to switching from precopy to
postcopy, but functions related to migration stream itself
(structs, send/load sequences) are mostly unchnaged.

So, changes, to switch from precopy to postcopy:
- removed all staff related to meta bitmaps and dirty phase!!!
- add dirty_bitmap_mig_enable_successors, and call it before
  target vm start in loadvm_postcopy_handle_run
- add enabled_bitmaps list of bitmaps for
  dirty_bitmap_mig_enable_successors

- enabled flag is send with start bitmap chunk instead of
  completion chunk
- sectors_per_chunk is calculated directly from CHUNK_SIZE, not
  using meta bitmap granularity

- dirty_bitmap_save_iterate: remove dirty_phase, move bulk_phase
  to postcopy stage
- dirty_bitmap_save_pending: remove dirty phase related pending,
  switch pending to non-postcopyable
- dirty_bitmap_load_start: get enabled flag and prepare
  successors for enabled bitmaps, also add them to
  enabled_bitmaps list
- dirty_bitmap_load_complete: for enabled bitmaps: merge them
  with successors and enable

- savevm handlers:
  * remove separate savevm_dirty_bitmap_live_iterate_handlers state
(it was bad idea, any way), and move its save_live_iterate to
savevm_dirty_bitmap_handlers
  * add is_active_iterate savevm handler, which allows iterations
only in postcopy stage (after stopping source vm)
  * add has_postcopy savevm handler. (ofcourse, just returning true)
  * use save_live_complete_postcopy instead of
save_live_complete_precopy

Other changes:
- some debug output changed
- remove MIN_LIVE_SIZE, is_live_iterative and related staff (it
  was needed to omit iterations if bitmap data is small, possibly
  this should be reimplemented)

Vladimir Sementsov-Ogievskiy (18):
   migration: add has_postcopy savevm handler
   migration: fix ram_save_pending
   migration: split common postcopy out of ram postcopy
   migration: introduce postcopy-only pending
   block: add bdrv_next_dirty_bitmap()
   block: add bdrv_dirty_bitmap_enable_successor()
   qapi: add dirty-bitmaps migration capability
   block/dirty-bitmap: add bdrv_dirty_bitmap_release_successor
   migration: include migrate_dirty_bitmaps in migrate_postcopy
   migration/qemu-file: add qemu_put_counted_string()
   migration: add is_active_iterate handler
   migration: add postcopy migration of dirty bitmaps
   iotests: maintain several vms in test
   iotests: add add_incoming_migration to VM class
   qapi: add md5 checksum of last dirty bitmap level to 

Re: [Qemu-block] [PATCH v2 11/11] iotests: add transactional failure race test

[Vladimir Sementsov-Ogievskiy]

it is almost a duplication of test_transaction_failure, I think it would 
be better to make separate do_test_transaction_failure with parameter 
and two wrappers


On 01.10.2016 01:00, John Snow wrote:

Add a regression test for the case found by Vladimir.

Reported-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: John Snow 
---
  tests/qemu-iotests/124 | 91 ++
  tests/qemu-iotests/124.out |  4 +-
  2 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 2f0bc24..b8f7dad 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -512,6 +512,97 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
  self.check_backups()
  
  
+def test_transaction_failure_race(self):

+'''Test: Verify that transactions with jobs that have no data to
+transfer do not cause race conditions in the cancellation of the entire
+transaction job group.
+'''
+
+# Create a second drive, with pattern:
+drive1 = self.add_node('drive1')
+self.img_create(drive1['file'], drive1['fmt'])
+io_write_patterns(drive1['file'], (('0x14', 0, 512),
+   ('0x5d', '1M', '32k'),
+   ('0xcd', '32M', '124k')))
+
+# Create a blkdebug interface to this img as 'drive1'
+result = self.vm.qmp('blockdev-add', options={
+'node-name': drive1['id'],
+'driver': drive1['fmt'],
+'file': {
+'driver': 'blkdebug',
+'image': {
+'driver': 'file',
+'filename': drive1['file']
+},
+'set-state': [{
+'event': 'flush_to_disk',
+'state': 1,
+'new_state': 2
+}],
+'inject-error': [{
+'event': 'read_aio',
+'errno': 5,
+'state': 2,
+'immediately': False,
+'once': True
+}],
+}
+})
+self.assert_qmp(result, 'return', {})
+
+# Create bitmaps and full backups for both drives
+drive0 = self.drives[0]
+dr0bm0 = self.add_bitmap('bitmap0', drive0)
+dr1bm0 = self.add_bitmap('bitmap0', drive1)
+self.create_anchor_backup(drive0)
+self.create_anchor_backup(drive1)
+self.assert_no_active_block_jobs()
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+
+# Emulate some writes
+self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
+  ('0xef', '16M', '256k'),
+  ('0x46', '32736k', '64k')))
+
+# Create incremental backup targets
+target0 = self.prepare_backup(dr0bm0)
+target1 = self.prepare_backup(dr1bm0)
+
+# Ask for a new incremental backup per-each drive, expecting drive1's
+# backup to fail and attempt to cancel the empty drive0 job.
+transaction = [
+transaction_drive_backup(drive0['id'], target0, sync='incremental',
+ format=drive0['fmt'], mode='existing',
+ bitmap=dr0bm0.name),
+transaction_drive_backup(drive1['id'], target1, sync='incremental',
+ format=drive1['fmt'], mode='existing',
+ bitmap=dr1bm0.name)
+]
+result = self.vm.qmp('transaction', actions=transaction,
+ properties={'completion-mode': 'grouped'} )
+self.assert_qmp(result, 'return', {})
+
+# Observe that drive0's backup is cancelled and drive1 completes with
+# an error.
+self.wait_qmp_backup_cancelled(drive0['id'])
+self.assertFalse(self.wait_qmp_backup(drive1['id']))
+error = self.vm.event_wait('BLOCK_JOB_ERROR')
+self.assert_qmp(error, 'data', {'device': drive1['id'],
+'action': 'report',
+'operation': 'read'})
+self.assertFalse(self.vm.get_qmp_events(wait=False))
+self.assert_no_active_block_jobs()
+
+# Delete drive0's successful target and eliminate our record of the
+# unsuccessful drive1 target.
+dr0bm0.del_target()
+dr1bm0.del_target()
+
+self.vm.shutdown()
+
+
+
  def test_sync_dirty_bitmap_missing(self):
  self.assert_no_active_block_jobs()
  self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 36376be..e56cae0 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-..
+...
  

Re: [Qemu-block] [PATCH] nbd: Use CoQueue for free_sema instead of CoMutex

[Paolo Bonzini]

On 12/10/2016 12:18, Changlong Xie wrote:
> +if (s->in_flight == MAX_NBD_REQUESTS) {
> +qemu_co_queue_wait(>free_sema);
>  assert(s->in_flight < MAX_NBD_REQUESTS);
>  }

I was wondering if this should be a while loop instead, but the
assertion protects against that.  So:

Reviewed-by: Paolo Bonzini 

Re: [Qemu-block] [Qemu-devel] [PATCH] nbd: Use CoQueue for free_sema instead of CoMutex

[Changlong Xie]

On 10/12/2016 06:18 PM, Changlong Xie wrote:


time request   Actions
29   15(most case) in_flight=15, Coroutine=C15, free_sema->holder=C17, 
mutex->locked=false


Per Paolo's suggestion "The simplest fix is to change it to CoQueue, which is 
like a condition
variable", this patch replaces CoMutex with CoQueue.

Cc: Wen Congyang
Reported-by: zhanghailiang
Suggested-by: Paolo Bonzini
Signed-off-by: Changlong Xie
---


Please refer to:

http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg02172.html

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] dma-helpers: explicitly pass alignment into dma-helpers

[Kevin Wolf]

Am 11.10.2016 um 17:47 hat John Snow geschrieben:
> On 10/10/2016 03:23 PM, Mark Cave-Ayland wrote:
> >On 10/10/16 17:34, Eric Blake wrote:
> >
> >>On 10/09/2016 11:43 AM, Mark Cave-Ayland wrote:
> >>>The hard-coded default alignment is BDRV_SECTOR_SIZE, however this is not
> >>>necessarily the case for all platforms. Use this as the default alignment 
> >>>for
> >>>all current callers.
> >>>
> >>>Signed-off-by: Mark Cave-Ayland 
> >>>---
> >>
> >>>@@ -160,8 +161,8 @@ static void dma_blk_cb(void *opaque, int ret)
> >>> return;
> >>> }
> >>>
> >>>-if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
> >>>-qemu_iovec_discard_back(>iov, dbs->iov.size & 
> >>>~BDRV_SECTOR_MASK);
> >>>+if (dbs->iov.size & (dbs->align - 1)) {
> >>>+qemu_iovec_discard_back(>iov, dbs->iov.size & (dbs->align - 
> >>>1));
> >>
> >>Would it be any smarter to use osdep.h's QEMU_IS_ALIGNED(dbs->iov.size,
> >>dbs->align) and QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align)?
> >>Semantically it is the same, but the macros make it obvious what the
> >>bit-twiddling is doing.
> >>
> >>Unless you think that needs a tweak,
> >>Reviewed-by: Eric Blake 
> >
> >I can't say I feel too strongly about it since there are plenty of other
> >examples of this style in the codebase, so I'm happy to go with whatever
> >John/Paolo are most happy with.
> >
> >
> >ATB,
> >
> >Mark.
> >
> 
> I can't pretend I am consistent, but when in doubt use the macro.
> Not worth a respin IMO, but I think this falls out of my
> jurisdiction :)
> 
> Acked-by: John Snow 

dma-helpers.c is officially unmaintained, and as the other patch is
clearly IDE, I think the series should go through your tree.

Kevin

Re: [Qemu-block] [PATCH 1/3] block: add BDS field to count in-flight requests

[Paolo Bonzini]

On 12/10/2016 11:50, Kevin Wolf wrote:
> > (By the way, I need to repost this series anyway, but let's finish the
> > discussion first to understand what you'd like to have in 2.8).
>
> I'm still not completely sold on the order in which we should do things,
> but you've been insisting enough that I'll just trust you on this.

Thanks, I'll post v2 then.

Paolo

[Qemu-block] [PATCH] nbd: Use CoQueue for free_sema instead of CoMutex

[Changlong Xie]

NBD is using the CoMutex in a way that wasn't anticipated. For example, if 
there are
N(N=26, MAX_NBD_REQUESTS=16) nbd write requests, so we will invoke 
nbd_client_co_pwritev
N times.

time request Actions
11   in_flight=1, Coroutine=C1
22   in_flight=2, Coroutine=C2
...
15   15  in_flight=15, Coroutine=C15
16   16  in_flight=16, Coroutine=C16, free_sema->holder=C16, 
mutex->locked=true
17   17  in_flight=16, Coroutine=C17, queue C17 into free_sema->queue
18   18  in_flight=16, Coroutine=C18, queue C18 into free_sema->queue
...
26   N   in_flight=16, Coroutine=C26, queue C26 into free_sema->queue


Once nbd client recieves request No.16' reply, we will re-enter C16. It's ok, 
because
it's equal to 'free_sema->holder'.

time request Actions
27   16  in_flight=15, Coroutine=C16, free_sema->holder=C16, 
mutex->locked=false


Then nbd_coroutine_end invokes qemu_co_mutex_unlock what will pop coroutines 
from
free_sema->queue's head and enter C17. More free_sema->holder is C17 now.

time request Actions
28   17  in_flight=16, Coroutine=C17, free_sema->holder=C17, 
mutex->locked=true


In above scenario, we only recieves request No.16' reply. As time goes by, nbd 
client will
almostly recieves replies from requests 1 to 15 rather than request 17 who owns 
C17. In this
case, we will encounter assert "mutex->holder == self" failed since Kevin's 
commit 0e438cdc
"coroutine: Let CoMutex remember who holds it". For example, if nbd client 
recieves request
No.15' reply, qemu will stop unexpectedly:

time request   Actions
29   15(most case) in_flight=15, Coroutine=C15, free_sema->holder=C17, 
mutex->locked=false


Per Paolo's suggestion "The simplest fix is to change it to CoQueue, which is 
like a condition
variable", this patch replaces CoMutex with CoQueue.

Cc: Wen Congyang 
Reported-by: zhanghailiang 
Suggested-by: Paolo Bonzini 
Signed-off-by: Changlong Xie 
---
 block/nbd-client.c | 8 
 block/nbd-client.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 2cf3237..40b28ab 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -199,8 +199,8 @@ static void nbd_coroutine_start(NbdClientSession *s,
 {
 /* Poor man semaphore.  The free_sema is locked when no other request
  * can be accepted, and unlocked after receiving one reply.  */
-if (s->in_flight >= MAX_NBD_REQUESTS - 1) {
-qemu_co_mutex_lock(>free_sema);
+if (s->in_flight == MAX_NBD_REQUESTS) {
+qemu_co_queue_wait(>free_sema);
 assert(s->in_flight < MAX_NBD_REQUESTS);
 }
 s->in_flight++;
@@ -214,7 +214,7 @@ static void nbd_coroutine_end(NbdClientSession *s,
 int i = HANDLE_TO_INDEX(s, request->handle);
 s->recv_coroutine[i] = NULL;
 if (s->in_flight-- == MAX_NBD_REQUESTS) {
-qemu_co_mutex_unlock(>free_sema);
+qemu_co_queue_next(>free_sema);
 }
 }
 
@@ -386,7 +386,7 @@ int nbd_client_init(BlockDriverState *bs,
 }
 
 qemu_co_mutex_init(>send_mutex);
-qemu_co_mutex_init(>free_sema);
+qemu_co_queue_init(>free_sema);
 client->sioc = sioc;
 object_ref(OBJECT(client->sioc));
 
diff --git a/block/nbd-client.h b/block/nbd-client.h
index 044aca4..307b8b1 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -24,7 +24,7 @@ typedef struct NbdClientSession {
 off_t size;
 
 CoMutex send_mutex;
-CoMutex free_sema;
+CoQueue free_sema;
 Coroutine *send_coroutine;
 int in_flight;
 
-- 
1.9.3

Re: [Qemu-block] [PATCH 1/3] block: add BDS field to count in-flight requests

[Kevin Wolf]

Am 11.10.2016 um 18:45 hat Paolo Bonzini geschrieben:
> > I think my point was that you don't have to count requests at the BB
> > level if you know that there are no requests pending on the BB level
> > that haven't reached the BDS level yet.
> 
> I need to count requests at the BB level because the blk_aio_*
> operations have a separate bottom half that is invoked if either 1) they
> never reach BDS (because of an error); or 2) the bdrv_co_* call
> completes without yielding.  The count must be >0 when blk_aio_*
> returns, or bdrv_drain (and thus blk_drain) won't loop.  Because
> bdrv_drain and blk_drain are conflated, the counter must be the BDS one.

Okay, makes sense.

> In turn, the BDS counter is needed because of the lack of isolated
> sections.  The right design would be for blk_isolate_begin to call
> blk_drain on *other* BlockBackends reachable in a child-to-parent visit.

Not really the blk_drain() that completes all pending requests of the
BB, but the BdrvChild callbacks that quiesce the BB. But I think we
really agree here and are just having trouble with the terminology.

>  Instead, until that is implemented, we have bdrv_drained_begin that
> emulates that through the same-named callback, followed by a
> parent-to-child bdrv_drain that is almost always unnecessary.

Yes.

> (By the way, I need to repost this series anyway, but let's finish the
> discussion first to understand what you'd like to have in 2.8).

I'm still not completely sold on the order in which we should do things,
but you've been insisting enough that I'll just trust you on this.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Tue 11 Oct 2016 06:32:39 PM CEST, Markus Armbruster wrote:

>>> 3) QEMU could advertise that feature to the client. This is probably
>>> simpler than trying to figure it out from the API. I guess that's
>>> the idea of 'qmp_capabilities'?
>>
>> I think that was the idea, though it was never used. If we had used
>> it, I'm not sure how long the capabilities list would be today. :-)
>
> QMP capabilities are for changes in the QMP protocol, not for changes
> in commands, events and types.  The protocol has been good enough so
> far, thus no capabilities.

I see. Wouldn't it make sense in general to have a way to ask QEMU
whether some certain feature is supported?

Berto

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

[Markus Armbruster]

"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 ;)

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

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

> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h | 20 +++-
>  qapi/qobject-input-visitor.c | 27 +--
>  tests/test-qobject-input-visitor.c   | 88 
> +++-
>  3 files changed, 117 insertions(+), 18 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index f134d90..1809f48 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -42,6 +42,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * represented as strings. i.e. if visiting a boolean, the value should
>   * be a QString whose contents represent a valid boolean.
>   *
> + * If @autocreate_list is true, then as an alternative to a normal QList,
> + * list values can be stored as a QString or QDict instead, which will
> + * be interpreted as representing single element lists. This should only
> + * by used if compatibility is required with the OptsVisitor which allowed
> + * repeated keys, without list indexes, to represent lists. e.g. set this
> + * to true 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
> + *
>   * The visitor always operates in strict mode, requiring all dict keys
>   * to be consumed during visitation. An error will be reported if this
>   * does not happen.
> @@ -49,7 +62,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * The returned input visitor should be released by calling
>   * visit_free() when no longer required.
>   */
> -Visitor *qobject_input_visitor_new_autocast(QObject *obj);
> +Visitor *qobject_input_visitor_new_autocast(QObject *obj,
> +bool autocreate_list);
>  
>  
>  /**
> @@ -64,6 +78,8 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj);
>   * The returned input visitor should be released by calling
>   * visit_free() when no longer required.
>   */
> -Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, Error **errp);
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
> +bool autocreate_list,
> +Error **errp);
>  
>  #endif
> 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?

> @@ -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) {
>  

Re: [Qemu-block] [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Alberto Garcia]

On Tue 11 Oct 2016 06:50:27 PM CEST, Markus Armbruster wrote:

> * Is the extended command still a sane interface?  If writing clear
>   documentation for it is hard, it perhaps isn't.  Pay special
>   attention to failure modes.  Overloaded arguments are prone to
>   confusing errors.

This is what the current command looks like:

{ 'command': 'block-stream',
  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*backing-file': 'str', '*speed': 'int',
'*on-error': 'BlockdevOnError' } }

If we decide to add a new command, this is what it could look like:

{ 'command': 'blockdev-stream',
  'data': { '*job-id': 'str', 'top': 'str', '*base': 'str',
'*backing-file': 'str', '*speed': 'int',
'*on-error': 'BlockdevOnError' } }

If we decide to extend the existing command, there's essentially two
changes that we have to do:

1) 'device' refers to a device name, it should refer to (or allow) a
   node name. This is trivial to do, the only problem is that the name
   of the parameter is not the best.

2) 'base' takes a file name, but we should have a way to pass a node
name instead. Overloading here is not an option, we need a new
parameter ('base-node' or something like that). 'base' and
'base-node' would be optional but mutually exclusive.

{ 'command': 'block-stream',
  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*base-node': 'str', '*backing-file': 'str',
'*speed': 'int', '*on-error': 'BlockdevOnError' } }

Considering that a new command would be very similar to the original one
(the only problems being an ill-named parameter and an obsolete one), I
actually don't think that extending the current command is such a bad
idea. But I don't have a strong opinion.

Berto

Re: [Qemu-block] [PATCH v4 0/3] iotests: Fix test 162

[Hao QingFeng]

Max,

Just a common question for this case, if sshx block driver wasn't built 
into qemu-img, this case would fail as below:


exec /home/haoqf/KVMonz/qemu/tests/qemu-iotests/../../qemu-img info 
--image-opts driver=ssh,host=localhost,port=0.42,path=/foo
qemu-img: Could not open 
'driver=ssh,host=localhost,port=0.42,path=/foo': Unknown driver 'ssh'


Adding 162.notrun can bypass this case but it would skip it even if 
qemu-img has sshx block driver, in which case I think it should be run.


So How about adding a script to dynamically check at runtime if the 
current env qemu-img can meet the requirement to run the test or not?


I made a sample here whose result is:

[Without sshx built in]

./check -qcow2 162
... ...
162 0s ... [not run] case 162 not applicable!
Not run: 162
Passed all 0 tests

[Without sshx built in]
./check -qcow2 162
... ...
162 0s ...
Passed all 1 tests

Rough code patch is(new file 162.check is introduced to check if sshx is 
built in):


diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4cba215..e7ef395 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -18,7 +18,6 @@
 #
 # Control script for QA
 #
-
 status=0
 needwrap=true
 try=0
@@ -291,6 +290,24 @@ do
 start=`_wallclock`
 $timestamp && echo -n "["`date "+%T"`"]"

+   check_ret=0
+   if [ -f "$source_iotests/$seq.check" -a -x 
"$source_iotests/$seq.check" ]; then
+   if [ "$(head -n 1 "$source_iotests/$seq.check")" == 
"#!/usr/bin/env python" ]; then

+$PYTHON $seq.check
+else
+./$seq.check
+   fi
+   check_ret=$?
+   fi
+   if [ $check_ret -ne 0 ]; then
+$timestamp || echo -n " [not run] "
+$timestamp && echo " [not run]" && echo -n "$seq -- "
+   echo "case $seq not applicable!"
+notrun="$notrun $seq"
+   seq="after_$seq"
+   continue
+   fi
+
 if [ "$(head -n 1 "$source_iotests/$seq")" == "#!/usr/bin/env 
python" ]; then

 run_command="$PYTHON $seq"
 else

diff --git a/tests/qemu-iotests/162.check b/tests/qemu-iotests/162.check
new file mode 100755
index 000..a80df7a
--- /dev/null
+++ b/tests/qemu-iotests/162.check
@@ -0,0 +1,35 @@
+#!/bin/bash
+#Return 0 if the case can run, others can not
+#Typically the block drivers can be queried by "qemu-img --help" and
+#the output is as:
+#Supported formats: dmg luks ssh sheepdog nbd null-aio null-co 
host_cdrom host_device file blkreplay blkverify blkdebug parallels 
quorum qed qcow2 vvfat vpc bochs cloop vmdk vdi qcow raw

+#set -x
+
+#. ./common.config
+found=0
+. ./common.rc
+. ./common.filter
+#_supported_fmt generic
+#_supported_os Linux
+blk_drivers=`$QEMU_IMG --help|grep "Supported formats:"|sed 
's/Supported formats://'`

+#echo "drivers:"$blk_drivers
+#echo $blk_drivers|awk '{print $0}'
+found=$(
+echo $blk_drivers|awk '{n=split($0, arr_drivers, " ");
+   for(i=1; i<=n; i++) print arr_drivers[i]}' | { while 
read driver

+   do
+   if [ "$driver"x = "sshx" ]; then
+   echo 1
+   exit
+   fi
+   done
+   echo 0
+   }
+)
+
+#echo "ret:$found"
+if [ "$found" = "1" ]; then
+   exit 0
+fi
+
+exit 1

Thanks!

Hao QingFeng


在 2016-09-29 4:46, Max Reitz 写道:

162 is potentially racy and makes some invalid assumptions about what
should happen when connecting to a non-existing domain name. This series
fixes both issues.


v4:
- Added documentation for the new --fork option [Kevin]


git-backport-diff against v3:

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

001/3:[0004] [FC] 'qemu-nbd: Add --fork option'
002/3:[] [--] 'iotests: Remove raciness from 162'
003/3:[] [--] 'iotests: Do not rely on unavailable domains in 162'


Max Reitz (3):
   qemu-nbd: Add --fork option
   iotests: Remove raciness from 162
   iotests: Do not rely on unavailable domains in 162

  qemu-nbd.c | 17 -
  qemu-nbd.texi  |  2 ++
  tests/qemu-iotests/162 | 22 --
  tests/qemu-iotests/162.out |  2 +-
  4 files changed, 35 insertions(+), 8 deletions(-)



--
QingFeng Hao

Re: [Qemu-block] [Qemu-devel] [PATCH v10 09/16] block: Add QMP support for streaming to an intermediate layer

[Kevin Wolf]

Am 11.10.2016 um 18:50 hat Markus Armbruster geschrieben:
> Eric Blake  writes:
> 
> > On 10/11/2016 09:57 AM, Kevin Wolf wrote:
> >> Should we introduce a new, clean blockdev-stream command that fixes this
> >> and matches the common name pattern? Of course, block-stream vs.
> >> blockdev-stream could be a bit confusing, too...
> >> 
> >
> > A new command is easy to introspect (query-commands), lets us get rid of
> > cruft, and makes it obvious that we support everything from the get-go.
> > I'm favoring that option, even if it leads to slightly confusing names
> > of the deprecated vs. the new command.
> 
> Let's take a step back and consider extending old commands vs. adding
> new commands.
> 
> A new command is trivial to detect in introspection.
> 
> Command extensions are not as trivial to detect in introspection.  Many
> extensions are exposed in query-qmp-schema, but not all.
> 
> Back when QMP was young, Anthony argued for always adding new commands,
> never extend existing ones.  I opposed it, because it would lead to a
> confusing mess of related commands, unreadable or incomplete
> documentation, and abysmal test coverage.
> 
> However, the other extreme is also unwise: we shouldn't shoehorn new
> functionality into existing commands just because we can.  We should ask
> ourselves questions like these:
> 
> * Is the extended command still a sane interface?  If writing clear
>   documentation for it is hard, it perhaps isn't.  Pay special attention
>   to failure modes.  Overloaded arguments are prone to confusing errors.

It has never been a sane interface in the first place (identifying a
backing file node by its filename).

We ended up having two versions of all block job commands anyway (one
that creates an image file, and later one that just takes a node-name of
an existing node), except for image streaming so far. So it would be
consistent (and enable consistent naming for the preferred commands) to
have it here, too.

> * How will the command's users use the extension?  If it requires new
>   code paths, a new command may be more convenient for them.

Kevin

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

> I received a mail saying my series failed the automatic build test but
> it builds completely fine (after applying Dan's patch obviously) in my
> local environment.
>
> Going through the config output of the test script, I see that the
> supporting library for SSH which is "libssh2" seems to be disabled and
> maybe causes the build to fail. I am able to reproduce it locally with
> "libssh2" disabled.

Sorry!
s/ "libssh2" disabled/ "libssh2" enabled.

Ashijeet

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Kevin Wolf]

Am 12.10.2016 um 10:37 hat Ashijeet Acharya geschrieben:
> On Wed, Oct 12, 2016 at 1:52 PM, Kevin Wolf  wrote:
> > Am 12.10.2016 um 10:09 hat Ashijeet Acharya geschrieben:
> 
> > Of course, we must be able to build qemu correctly both with ssh enabled
> > and disabled, so if you can indeed see a (different) build error with
> > disabled libssh2, that needs to be fixed.
> 
> I am able to reproduce it when I don't apply Dan's patch with libssh2 enabled.
> After disabling libssh2, it compiles perfectly because then the SSH
> driver will get ignored I guess.
> libssh2 doesn't seem to be the problem.
> 
> >
> > But I can't see how that would happen, you don't touch configure and it
> > already compiled the ssh driver out if libssh2 is disabled.
> 
> Yes, with libssh2 disabled it builds with no error just like the first
> output of auto-build shows.
> So, I think with libssh2 enabled it throws an error (just like second
> output of auto-build) since Dan's patches are not merged yet. Right?

Yes. You can ignore that, we'll make sure to merge the patches in the
right order.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2: Support BDRV_REQ_MAY_UNMAP

[Kevin Wolf]

Am 12.10.2016 um 03:14 hat Fam Zheng geschrieben:
> On Wed, 09/28 15:04, Fam Zheng wrote:
> > Handling this is similar to what is done to the L2 entry in the case of
> > compressed clusters.
> 
> Kevin, Max, is there anything else I need to do before this patch can be
> applied?

Hm, actually, it looks like we had a few discussions, but came to the
conclusions that the patch is alright.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

On Wed, Oct 12, 2016 at 1:52 PM, Kevin Wolf  wrote:
> Am 12.10.2016 um 10:09 hat Ashijeet Acharya geschrieben:

> Of course, we must be able to build qemu correctly both with ssh enabled
> and disabled, so if you can indeed see a (different) build error with
> disabled libssh2, that needs to be fixed.

I am able to reproduce it when I don't apply Dan's patch with libssh2 enabled.
After disabling libssh2, it compiles perfectly because then the SSH
driver will get ignored I guess.
libssh2 doesn't seem to be the problem.

>
> But I can't see how that would happen, you don't touch configure and it
> already compiled the ssh driver out if libssh2 is disabled.

Yes, with libssh2 disabled it builds with no error just like the first
output of auto-build shows.
So, I think with libssh2 enabled it throws an error (just like second
output of auto-build) since Dan's patches are not merged yet. Right?

Ashijeet
> Kevin

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Kevin Wolf]

Am 12.10.2016 um 10:09 hat Ashijeet Acharya geschrieben:
> I received a mail saying my series failed the automatic build test but
> it builds completely fine (after applying Dan's patch obviously) in my
> local environment.

The reason why patchew fails to build your series is because it doesn't
understand the dependency on Dan's patches. Just ignore it.

> Going through the config output of the test script, I see that the
> supporting library for SSH which is "libssh2" seems to be disabled and
> maybe causes the build to fail. I am able to reproduce it locally with
> "libssh2" disabled.

Of course, we must be able to build qemu correctly both with ssh enabled
and disabled, so if you can indeed see a (different) build error with
disabled libssh2, that needs to be fixed.

But I can't see how that would happen, you don't touch configure and it
already compiled the ssh driver out if libssh2 is disabled.

Kevin

Re: [Qemu-block] [PATCH 0/4] Allow blockdev-add for SSH

[Ashijeet Acharya]

On Tue, Oct 11, 2016 at 1:07 PM, Ashijeet Acharya
 wrote:
> This series adds blockdev-add support for SSH block driver.
>
> Patch 1 prepares the code for the addition of a new option prefix,
> which is "server.". This is accomplished by adding a
> ssh_has_filename_options_conflict() function which helps to iterate
> over the various options and check for conflict.
>
> Patch 2 first adds InetSocketAddress compatibility to SSH block driver
> and then makes it accept a InetSocketAddress under the "server" option.
> The old options "host" and "port" are supported as legacy options and
> then translated to the respective InetSocketAddress representation.
>
> Patch 3 drops the usage of "host" and "port" outside of
> ssh_has_filename_options_conflict() and
> ssh_process_legacy_socket_options() functions in order to make them
> legacy options completely.
>
> Patch 4 helps to allow blockdev-add support for the SSH block driver
> by making the SSH option available.
>
>
> *** This series depends on the following patch: ***
> "qdict: implement a qdict_crumple method for un-flattening a dict"
> from Daniel's "QAPI/QOM work for non-scalar object properties"
> series.
>
> Ashijeet Acharya (4):
>   block/ssh: Add ssh_has_filename_options_conflict()
>   block/ssh: Add InetSocketAddress and accept it
>   block/ssh: Use InetSocketAddress options
>   qapi: allow blockdev-add for ssh
>
>  block/ssh.c  | 121 
> +++
>  qapi/block-core.json |  24 +-
>  2 files changed, 125 insertions(+), 20 deletions(-)
>
> --
> 2.6.2

I received a mail saying my series failed the automatic build test but
it builds completely fine (after applying Dan's patch obviously) in my
local environment.

Going through the config output of the test script, I see that the
supporting library for SSH which is "libssh2" seems to be disabled and
maybe causes the build to fail. I am able to reproduce it locally with
"libssh2" disabled.

Any other thoughts?

Ashijeet

Re: [Qemu-block] [Qemu-devel] [PATCH v14 08/21] qapi: allow QObjectInputVisitor to be created with QemuOpts

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> Instead of requiring all callers to go through the mutli-step

multi-step

> process of turning QemuOpts into a suitable QObject for visiting,
> add a new constructor that encapsulates this logic. This will
> allow QObjectInputVisitor to be a drop-in replacement for the
> existing OptsVisitor with minimal code changes for callers.
>
> NB, at this point it is only supporting opts syntax which
> explicitly matches the QAPI schema structure, so is not yet
> a true drop-in replacement for OptsVisitor. The patches that
> follow will add the special cases requird for full backwards
> compatibility with OptsVisitor.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h | 15 +++
>  include/qemu/option.h|  2 +-
>  qapi/qobject-input-visitor.c | 25 +
>  util/qemu-option.c   |  2 +-
>  4 files changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 5022297..f134d90 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -51,4 +51,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   */
>  Visitor *qobject_input_visitor_new_autocast(QObject *obj);
>  
> +
> +/**
> + * Create a new input visitor that converts @opts to a QAPI object.
> + *
> + * The QemuOpts will be converted into a QObject using the
> + * qdict_crumple() method to automatically create structs
> + * and lists. The resulting QDict will then be passed to the
> + * qobject_input_visitor_new_autocast() method. See the docs
> + * of that method for further details on processing behaviour.
> + *
> + * The returned input visitor should be released by calling
> + * visit_free() when no longer required.
> + */
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts, Error **errp);
> +
>  #endif
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 2a5266f..29f3f18 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -125,7 +125,7 @@ void qemu_opts_set_defaults(QemuOptsList *list, const 
> char *params,
>  int permit_abbrev);
>  QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> Error **errp);
> -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
> +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict);
>  void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
>  
>  typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts *opts, Error 
> **errp);
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index cf41df6..d9269c9 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -545,3 +545,28 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj)
>  
>  return >visitor;
>  }
> +
> +
> +Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
> +Error **errp)
> +{
> +QDict *pdict;
> +QObject *pobj = NULL;

@pdict and @pobj are unusual names.  Let's stick to the more common
@dict and @obj.

> +Visitor *v = NULL;
> +
> +pdict = qemu_opts_to_qdict(opts, NULL);
> +if (!pdict) {
> +goto cleanup;
> +}
> +
> +pobj = qdict_crumple(pdict, true, errp);
> +if (!pobj) {
> +goto cleanup;
> +}
> +
> +v = qobject_input_visitor_new_autocast(pobj);
> + cleanup:
> +qobject_decref(pobj);
> +QDECREF(pdict);
> +return v;
> +}
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 41b356c..418cbb6 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -1058,7 +1058,7 @@ void qemu_opts_absorb_qdict(QemuOpts *opts, QDict 
> *qdict, Error **errp)
>   * TODO We'll want to use types appropriate for opt->desc->type, but
>   * this is enough for now.

Implementing this TODO could break users that pass it to
qobject_input_visitor_new_autocast(), because despite its name, the
_autocast visitor expects *only* string scalars.

I guess (but have not checked) that these users have null opt->desc
since their opts->list->desc[] is empty.

We should either drop the TODO (needs review of the other users), or
update it to reflect the new situation.  Perhaps users of
qobject_input_visitor_new_autocast() should additionally assert
opts->list->desc[] is empty.

>   */
> -QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict)
> +QDict *qemu_opts_to_qdict(const QemuOpts *opts, QDict *qdict)
>  {
>  QemuOpt *opt;
>  QObject *val;