Re: [Qemu-block] [PATCH v6 6/6] tests: Add check-qobject for equality tests

[Eric Blake]

On 10/04/2017 10:25 AM, Max Reitz wrote:
> Add a new test file (check-qobject.c) for unit tests that concern
> QObjects as a whole.
> 
> Its only purpose for now is to test the qobject_is_equal() function.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/Makefile.include |   4 +-
>  tests/check-qobject.c  | 325 
> +
>  tests/.gitignore   |   1 +
>  3 files changed, 329 insertions(+), 1 deletion(-)
>  create mode 100644 tests/check-qobject.c

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 2/6] qapi/qlist: Add qlist_append_null() macro

[Eric Blake]

On 10/04/2017 10:25 AM, Max Reitz wrote:
> Besides the macro itself, this patch also adds a corresponding
> Coccinelle rule.
> 
> Signed-off-by: Max Reitz 
> ---
>  include/qapi/qmp/qlist.h | 3 +++
>  scripts/coccinelle/qobject.cocci | 3 +++
>  2 files changed, 6 insertions(+)

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:

On 2017-10-04 19:05, Manos Pitsidianakis wrote:

On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:

On 2017-08-15 09:45, Manos Pitsidianakis wrote:

block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the
driver
methods bdrv_add_child() and bdrv_del_child(),


Why? :-)

Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.

I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that.  If it doesn't, then just attach the child.

Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached).  Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands.  In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver.  (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)

Max



I think the two use cases are this:

a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.


For me both are the same: Adding/removing nodes into/from the graph.

The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children.  Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).


Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
Now that we're on this topic, quorum might be a good candidate for 
*_reopen when and if it lands on QMP: Reconfiguring the children could 
be done by reopening the BDS with new options.


In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.

(I guess writing too much trying to get my point across, sorry...)

The gist is that for me it's not so much about "quorum" or "filter
nodes".  It's about adding a new child to a node vs. replacing an
existing one.


These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?


Yes, I think it would be. :-)

So say we have these two trees in our graph:

[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]

So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
   [ Parent BDS -> Child BDS ]
 is split into two trees
   [ Parent BDS ] and
   [ Child BDS ]
- Add a child, so we can merge
   [ Parent BDS ] and
   [ Filter BDS -> Child BDS ]
 into
   [ Parent BDS -> Filter BDS -> Child BDS ]



Yes, of course this would have to be done in one transaction.


However, this is only possible with quorum because usually block drivers
don't support detaching children.

And here's what you can do with your commands (from what I can see):
- Replace a child (you call it insertion, but it really is just
 replacement of an existing child with the constraint that both nodes
 involved must have the same child):
   [ Parent BDS -> Child BDS ]
   [ Filter BDS -> Child BDS ]
 to
   [ Parent BDS -> Filter BDS -> Child BDS ]
- Replace a child (you call it removal, but it really is just
 replacement of a child with its child):
   [ Parent BDS -> Filter BDS -> Child BDS ]
 to
   [ Parent BDS -> Child BDS ]

This works on all BDSs because you don't change the number of children.


The interesting thing of course is that the "change" command can
actually add and remove children; where as the "insert" and "remove"
commands can only replace children.  So that's already a bit funny (one
command does two things; two commands do one thing).


That is true, but the replacing is more in terms of inserting and 
removing a node in a BDS chain.


And then of course you can simply modify x-blockdev-change so it can do
the same thing block-insert-node and block-remove-node can do: It just
needs another mode which can be used to replace a child (and its
description already states that it is supposed to be usable for that at
some point in the future).


So after I've written all of this, I feel like the new insert-node and
remove-node commands are exactly what 

Re: [Qemu-block] [PATCH v2 0/4] blockjobs: add explicit job reaping

[Kevin Wolf]

Am 04.10.2017 um 03:52 hat John Snow geschrieben:
> For jobs that complete when a monitor isn't looking, there's no way to
> tell what the job's final return code was. We need to allow jobs to
> remain in the list until queried for reliable management.

Just a short summary of what I discussed with John on IRC:

Another important reason why we want to have an explicit end of block
jobs is that job completion often makes changes to the graph. For a
management tool that manages the block graph on a node level, it is a
big problem if graph changes can happen at any point that can lead to
bad race conditions. Giving the management tool control over the end of
the block job makes it aware that graph changes happen.

This means that compared to this RFC series, we need to move the waiting
earlier in the process:

1. Block job is done and calls block_job_completed()
2. Wait for other block jobs in the same job transaction to complete
3. Send a (new) QMP event to the management tool to notify it that the
   job is ready to be reaped
4. On block-job-reap, call the .commit/.abort callbacks of the jobs in
   the transaction. They will do most of the work that is currently done
   in the completion callbacks, in particular any graph changes. If we
   need to allow error cases, we can add a .prepare_completion callback
   that can still let the whole transaction fail.
5. Send the final QMP completion event for each job in the transaction
   with the final error code. This is the existing BLOCK_JOB_COMPLETED
   or BLOCK_JOB_CANCELLED event.

The current RFC still executes .commit/.abort before block-job-reap, so
the graph changes happen too early to be under control of the management
tool.

> RFC:
> The next version will add tests for transactions.
> Kevin, can you please take a look at bdrv_is_root_node and how it is
> used with respect to do_drive_backup? I suspect that in this case that
> "is root" should actually be "true", but a node in use by a job has
> two roles; child_root and child_job, so it starts returning false here.
> 
> That's fine because we prevent a collision that way, but it makes the
> error messages pretty bad and misleading. Do you have a quick suggestion?
> (Should I just amend the loop to allow non-root nodes as long as they
> happen to be jobs so that the job creation code can check permissions?)

We kind of sidestepped this problem by deciding that there is no real
reason for the backup job to require the source to be a root node.

I'm not completely sure how we could easily get a better message while
still requiring a root node (and I suppose there are other places that
rightfully still require a root node). Ignoring children with child_job
feels ugly, but might be the best option.

Note that this will not make the conflicting command work suddenly,
because every node that has a child_job parent also has op blockers for
everything, but the error message should be less confusing than "is not
a root node".

Kevin

Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Max Reitz]

On 2017-10-04 19:05, Manos Pitsidianakis wrote:
> On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
>> On 2017-08-15 09:45, Manos Pitsidianakis wrote:
>>> block-insert-node and its pair command block-remove-node provide runtime
>>> insertion and removal of filter nodes.
>>>
>>> block-insert-node takes a (parent, child) and (node, child) pair of
>>> edges and unrefs the (parent, child) BdrvChild relationship and creates
>>> a new (parent, node) child with the same BdrvChildRole.
>>>
>>> This is a different approach than x-blockdev-change which uses the
>>> driver
>>> methods bdrv_add_child() and bdrv_del_child(),
>>
>> Why? :-)
>>
>> Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
>> of its roles, and at least I don't want to have both x-blockdev-change
>> and these new commands.
>>
>> I think it would be a good idea just to change bdrv_add_child() and
>> bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
>> invoke that.  If it doesn't, then just attach the child.
>>
>> Of course, it may turn out that x-blockdev-change is lacking something
>> (e.g. a way to specify as what kind of child a new node is to be
>> attached).  Then we should either extend it so that it covers what it's
>> lacking, or replace it completely by these new commands.  In the latter
>> case, however, they would need to cover the existing use case which is
>> reconfiguring the quorum driver.  (And that would mean it would have to
>> invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
>>
>> Max
>>
> 
> I think the two use cases are this:
> 
> a) Adding/removing a replication child to an existing quorum node
> b) Insert a filter between two existing nodes.

For me both are the same: Adding/removing nodes into/from the graph.

The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children.  Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).

In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.

(I guess writing too much trying to get my point across, sorry...)

The gist is that for me it's not so much about "quorum" or "filter
nodes".  It's about adding a new child to a node vs. replacing an
existing one.

> These are not directly compatible semantically, but as you said
> x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
> are not implemented. Wouldn't that be unnecessary overloading?

Yes, I think it would be. :-)

So say we have these two trees in our graph:

[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]

So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
[ Parent BDS -> Child BDS ]
  is split into two trees
[ Parent BDS ] and
[ Child BDS ]
- Add a child, so we can merge
[ Parent BDS ] and
[ Filter BDS -> Child BDS ]
  into
[ Parent BDS -> Filter BDS -> Child BDS ]

However, this is only possible with quorum because usually block drivers
don't support detaching children.

And here's what you can do with your commands (from what I can see):
- Replace a child (you call it insertion, but it really is just
  replacement of an existing child with the constraint that both nodes
  involved must have the same child):
[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]
  to
[ Parent BDS -> Filter BDS -> Child BDS ]
- Replace a child (you call it removal, but it really is just
  replacement of a child with its child):
[ Parent BDS -> Filter BDS -> Child BDS ]
  to
[ Parent BDS -> Child BDS ]

This works on all BDSs because you don't change the number of children.


The interesting thing of course is that the "change" command can
actually add and remove children; where as the "insert" and "remove"
commands can only replace children.  So that's already a bit funny (one
command does two things; two commands do one thing).

And then of course you can simply modify x-blockdev-change so it can do
the same thing block-insert-node and block-remove-node can do: It just
needs another mode which can be used to replace a child (and its
description already states that it is supposed to be usable for that at
some point in the future).


So after I've written all of this, I feel like the new insert-node and
remove-node commands are exactly what x-blockdev-change should do when
asked to replace a node.  The only difference is that x-blockdev-change
would allow you to replace any node with anything, without the
constraints that block-insert-node and block-remove-node exact.

(And node replacement with x-blockdev-change would work by specifying
all three parameters.)

Not sure if that makes sense, I hope it does. 

Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:

On 2017-08-15 09:45, Manos Pitsidianakis wrote:

block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and 
creates

a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),


Why? :-)

Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.

I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that.  If it doesn't, then just attach the child.

Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached).  Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands.  In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver.  (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)

Max



I think the two use cases are this:

a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.

These are not directly compatible semantically, but as you said 
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child() 
are not implemented. Wouldn't that be unnecessary overloading?


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v2 00/10] cleanup qemu-iotests

[Kevin Wolf]

Am 12.09.2017 um 16:44 hat Paolo Bonzini geschrieben:
> The purpose of this series is to separate the "check" sources from
> the tests.  After these patches, common.config is reduced to simple
> shell initialization, and common.rc is only included by the tests.
> 
> Along the way, a lot of dead code is removed too.
> 
> In v2, the following patches:
> 
>   qemu-iotests: do not do useless search for QEMU_*_PROG
>   qemu-iotests: do not search for binaries in the current directory
>   qemu-iotests: include common.env and common.config early
> 
> have been replaced by "qemu-iotests: cleanup and fix search for programs",
> which also preserves the behavior of searching for programs as symlinks
> in the current directory.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-virtio' QMP command

[Eric Blake]

On 10/04/2017 09:26 AM, Jan Dakinevich wrote:

> +{
> +'struct': 'VirtioInfo',
> +'data': {
> +'feature-names': ['VirtioInfoBit'],

 Why is feature-names listed at two different nestings of the return value?

>>>
>>> These are different feature names. First names are common and predefined
>>> for all devices. Second names are device-specific.
>>
>> If you can turn these into enums (union'd enums?) then you might
>> be able to get rid of a lot of your array filling/naming conversion
>> boilerplate. (Not sure if it's worth it, but it's worth looking).
>>
> 
> I would be happy to drop this boilerplate, but how enum could help here?
> To respond my requirement it should be something like set, not enum.
> Even so, having set, I would have been needed to declare mapping between
> names in set type and bit numbers within feature bitmask.

Instead of returning a bitmask ("mask":123) as well as an array naming
those bits
([{"bit":1,"name":"bit1"},{"bit":2","name":"bit2"},{"bit":4,"name":"bit4},...]),
you could omit the bit numbers and just return an array of named bits
(["bit1", "bit2", "bit4"]).  An enum lets you declare up front what
named bits are supported (and code can introspect when new named bits
are supported in newer qemu).

Perhaps it's easier to first take a step back, and show what the desired
output might be like, and then we can figure out how to represent that
output in QAPI.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v6 5/6] iotests: Add test for non-string option reopening

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Kevin Wolf 
Reviewed-by: Eric Blake 
---
 tests/qemu-iotests/133 | 9 +
 tests/qemu-iotests/133.out | 5 +
 2 files changed, 14 insertions(+)

diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index 9d35a6a1ca..af6b3e1dd4 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -83,6 +83,15 @@ $QEMU_IO -c 'reopen -o driver=qcow2' $TEST_IMG
 $QEMU_IO -c 'reopen -o file.driver=file' $TEST_IMG
 $QEMU_IO -c 'reopen -o backing.driver=qcow2' $TEST_IMG
 
+echo
+echo "=== Check that reopening works with non-string options ==="
+echo
+
+# Using the json: pseudo-protocol we can create non-string options
+# (Invoke 'info' just so we get some output afterwards)
+IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
+"json:{'driver': 'null-co', 'size': 65536}"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index cc86b94880..f4a85aeb63 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -19,4 +19,9 @@ Cannot change the option 'driver'
 
 === Check that unchanged driver is okay ===
 
+
+=== Check that reopening works with non-string options ===
+
+format name: null-co
+format name: null-co
 *** done
-- 
2.13.6

[Qemu-block] [PATCH v6 6/6] tests: Add check-qobject for equality tests

[Max Reitz]

Add a new test file (check-qobject.c) for unit tests that concern
QObjects as a whole.

Its only purpose for now is to test the qobject_is_equal() function.

Signed-off-by: Max Reitz 
---
 tests/Makefile.include |   4 +-
 tests/check-qobject.c  | 325 +
 tests/.gitignore   |   1 +
 3 files changed, 329 insertions(+), 1 deletion(-)
 create mode 100644 tests/check-qobject.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index abc6707ef2..20622139f8 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -41,6 +41,7 @@ check-unit-y += tests/check-qlist$(EXESUF)
 gcov-files-check-qlist-y = qobject/qlist.c
 check-unit-y += tests/check-qnull$(EXESUF)
 gcov-files-check-qnull-y = qobject/qnull.c
+check-unit-y += tests/check-qobject$(EXESUF)
 check-unit-y += tests/check-qjson$(EXESUF)
 gcov-files-check-qjson-y = qobject/qjson.c
 check-unit-y += tests/check-qlit$(EXESUF)
@@ -543,7 +544,7 @@ GENERATED_FILES += tests/test-qapi-types.h 
tests/test-qapi-visit.h \
tests/test-qmp-introspect.h
 
 test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
-   tests/check-qlist.o tests/check-qnull.o \
+   tests/check-qlist.o tests/check-qnull.o tests/check-qobject.o \
tests/check-qjson.o tests/check-qlit.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
@@ -577,6 +578,7 @@ tests/check-qstring$(EXESUF): tests/check-qstring.o 
$(test-util-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
 tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
 tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
+tests/check-qobject$(EXESUF): tests/check-qobject.o $(test-util-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
 tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o 
$(test-qom-obj-y)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
new file mode 100644
index 00..e6b1556c2b
--- /dev/null
+++ b/tests/check-qobject.c
@@ -0,0 +1,325 @@
+/*
+ * Generic QObject unit-tests.
+ *
+ * Copyright (C) 2017 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+#include "qemu/osdep.h"
+
+#include "qapi/qmp/types.h"
+#include "qemu-common.h"
+
+#include 
+
+/* Marks the end of the test_equality() argument list.
+ * We cannot use NULL there because that is a valid argument. */
+static QObject test_equality_end_of_arguments;
+
+/**
+ * Test whether all variadic QObject *arguments are equal (@expected
+ * is true) or whether they are all not equal (@expected is false).
+ * Every QObject is tested to be equal to itself (to test
+ * reflexivity), all tests are done both ways (to test symmetry), and
+ * transitivity is not assumed but checked (each object is compared to
+ * every other one).
+ *
+ * Note that qobject_is_equal() is not really an equivalence relation,
+ * so this function may not be used for all objects (reflexivity is
+ * not guaranteed, e.g. in the case of a QNum containing NaN).
+ */
+static void do_test_equality(bool expected, ...)
+{
+va_list ap_count, ap_extract;
+QObject **args;
+int arg_count = 0;
+int i, j;
+
+va_start(ap_count, expected);
+va_copy(ap_extract, ap_count);
+while (va_arg(ap_count, QObject *) != _equality_end_of_arguments) {
+arg_count++;
+}
+va_end(ap_count);
+
+args = g_new(QObject *, arg_count);
+for (i = 0; i < arg_count; i++) {
+args[i] = va_arg(ap_extract, QObject *);
+}
+va_end(ap_extract);
+
+for (i = 0; i < arg_count; i++) {
+g_assert(qobject_is_equal(args[i], args[i]) == true);
+
+for (j = i + 1; j < arg_count; j++) {
+g_assert(qobject_is_equal(args[i], args[j]) == expected);
+}
+}
+}
+
+#define check_equal(...) \
+do_test_equality(true, __VA_ARGS__, _equality_end_of_arguments)
+#define check_unequal(...) \
+do_test_equality(false, __VA_ARGS__, _equality_end_of_arguments)
+
+static void do_free_all(int _, ...)
+{
+va_list ap;
+QObject *obj;
+
+va_start(ap, _);
+while ((obj = va_arg(ap, QObject *)) != NULL) {
+qobject_decref(obj);
+}
+va_end(ap);
+}
+
+#define free_all(...) \
+do_free_all(0, __VA_ARGS__, NULL)
+
+static void qobject_is_equal_null_test(void)
+{
+check_unequal(qnull(), NULL);
+}
+
+static void qobject_is_equal_num_test(void)
+{
+QNum *u0, *i0, *d0, *dnan, *um42, *im42, *dm42;
+
+u0 = qnum_from_uint(0u);
+i0 = qnum_from_int(0);
+d0 = qnum_from_double(0.0);
+dnan = qnum_from_double(NAN);
+um42 = qnum_from_uint((uint64_t)-42);
+im42 = qnum_from_int(-42);
+dm42 = qnum_from_double(-42.0);
+
+/* 

[Qemu-block] [PATCH v6 3/6] qapi: Add qobject_is_equal()

[Max Reitz]

This generic function (along with its implementations for different
types) determines whether two QObjects are equal.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Reviewed-by: Markus Armbruster 
---
 include/qapi/qmp/qbool.h   |  1 +
 include/qapi/qmp/qdict.h   |  1 +
 include/qapi/qmp/qlist.h   |  1 +
 include/qapi/qmp/qnull.h   |  2 ++
 include/qapi/qmp/qnum.h|  1 +
 include/qapi/qmp/qobject.h |  9 
 include/qapi/qmp/qstring.h |  1 +
 qobject/qbool.c|  8 +++
 qobject/qdict.c| 29 +
 qobject/qlist.c| 32 +++
 qobject/qnull.c|  9 
 qobject/qnum.c | 54 ++
 qobject/qobject.c  | 29 +
 qobject/qstring.c  |  9 
 14 files changed, 186 insertions(+)

diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
index a4c309..f77ea86c4e 100644
--- a/include/qapi/qmp/qbool.h
+++ b/include/qapi/qmp/qbool.h
@@ -24,6 +24,7 @@ typedef struct QBool {
 QBool *qbool_from_bool(bool value);
 bool qbool_get_bool(const QBool *qb);
 QBool *qobject_to_qbool(const QObject *obj);
+bool qbool_is_equal(const QObject *x, const QObject *y);
 void qbool_destroy_obj(QObject *obj);
 
 #endif /* QBOOL_H */
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 7ea5120c4a..fc218e7be6 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -43,6 +43,7 @@ void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
 QObject *qdict_get(const QDict *qdict, const char *key);
 QDict *qobject_to_qdict(const QObject *obj);
+bool qdict_is_equal(const QObject *x, const QObject *y);
 void qdict_iter(const QDict *qdict,
 void (*iter)(const char *key, QObject *obj, void *opaque),
 void *opaque);
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index 59d209bbae..ec3fcc1a4c 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -61,6 +61,7 @@ QObject *qlist_peek(QList *qlist);
 int qlist_empty(const QList *qlist);
 size_t qlist_size(const QList *qlist);
 QList *qobject_to_qlist(const QObject *obj);
+bool qlist_is_equal(const QObject *x, const QObject *y);
 void qlist_destroy_obj(QObject *obj);
 
 static inline const QListEntry *qlist_first(const QList *qlist)
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
index d075549283..c992ee2ae1 100644
--- a/include/qapi/qmp/qnull.h
+++ b/include/qapi/qmp/qnull.h
@@ -27,4 +27,6 @@ static inline QNull *qnull(void)
 return _;
 }
 
+bool qnull_is_equal(const QObject *x, const QObject *y);
+
 #endif /* QNULL_H */
diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
index d6b0791139..c3d86794bb 100644
--- a/include/qapi/qmp/qnum.h
+++ b/include/qapi/qmp/qnum.h
@@ -69,6 +69,7 @@ double qnum_get_double(QNum *qn);
 char *qnum_to_string(QNum *qn);
 
 QNum *qobject_to_qnum(const QObject *obj);
+bool qnum_is_equal(const QObject *x, const QObject *y);
 void qnum_destroy_obj(QObject *obj);
 
 #endif /* QNUM_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index ef1d1a9237..38ac68845c 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -68,6 +68,15 @@ static inline void qobject_incref(QObject *obj)
 }
 
 /**
+ * qobject_is_equal(): Return whether the two objects are equal.
+ *
+ * Any of the pointers may be NULL; return true if both are.  Always
+ * return false if only one is (therefore a QNull object is not
+ * considered equal to a NULL pointer).
+ */
+bool qobject_is_equal(const QObject *x, const QObject *y);
+
+/**
  * qobject_destroy(): Free resources used by the object
  */
 void qobject_destroy(QObject *obj);
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 10076b7c8c..65c05a9be5 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -31,6 +31,7 @@ void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
 QString *qobject_to_qstring(const QObject *obj);
+bool qstring_is_equal(const QObject *x, const QObject *y);
 void qstring_destroy_obj(QObject *obj);
 
 #endif /* QSTRING_H */
diff --git a/qobject/qbool.c b/qobject/qbool.c
index 0606bbd2a3..ac825fc5a2 100644
--- a/qobject/qbool.c
+++ b/qobject/qbool.c
@@ -52,6 +52,14 @@ QBool *qobject_to_qbool(const QObject *obj)
 }
 
 /**
+ * qbool_is_equal(): Test whether the two QBools are equal
+ */
+bool qbool_is_equal(const QObject *x, const QObject *y)
+{
+return qobject_to_qbool(x)->value == qobject_to_qbool(y)->value;
+}
+
+/**
  * qbool_destroy_obj(): Free all memory allocated by a
  * QBool object
  */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 

[Qemu-block] [PATCH v6 4/6] block: qobject_is_equal() in bdrv_reopen_prepare()

[Max Reitz]

Currently, bdrv_reopen_prepare() assumes that all BDS options are
strings. However, this is not the case if the BDS has been created
through the json: pseudo-protocol or blockdev-add.

Note that the user-invokable reopen command is an HMP command, so you
can only specify strings there. Therefore, specifying a non-string
option with the "same" value as it was when originally created will now
return an error because the values are supposedly similar (and there is
no way for the user to circumvent this but to just not specify the
option again -- however, this is still strictly better than just
crashing).

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index ef5af81f66..68b0655dc5 100644
--- a/block.c
+++ b/block.c
@@ -3040,19 +3040,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 const QDictEntry *entry = qdict_first(reopen_state->options);
 
 do {
-QString *new_obj = qobject_to_qstring(entry->value);
-const char *new = qstring_get_str(new_obj);
+QObject *new = entry->value;
+QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+
 /*
- * Caution: while qdict_get_try_str() is fine, getting
- * non-string types would require more care.  When
- * bs->options come from -blockdev or blockdev_add, its
- * members are typed according to the QAPI schema, but
- * when they come from -drive, they're all QString.
+ * TODO: When using -drive to specify blockdev options, all values
+ * will be strings; however, when using -blockdev, blockdev-add or
+ * filenames using the json:{} pseudo-protocol, they will be
+ * correctly typed.
+ * In contrast, reopening options are (currently) always strings
+ * (because you can only specify them through qemu-io; all other
+ * callers do not specify any options).
+ * Therefore, when using anything other than -drive to create a 
BDS,
+ * this cannot detect non-string options as unchanged, because
+ * qobject_is_equal() always returns false for objects of different
+ * type.  In the future, this should be remedied by correctly 
typing
+ * all options.  For now, this is not too big of an issue because
+ * the user can simply omit options which cannot be changed anyway,
+ * so they will stay unchanged.
  */
-const char *old = qdict_get_try_str(reopen_state->bs->options,
-entry->key);
-
-if (!old || strcmp(new, old)) {
+if (!qobject_is_equal(new, old)) {
 error_setg(errp, "Cannot change the option '%s'", entry->key);
 ret = -EINVAL;
 goto error;
-- 
2.13.6

[Qemu-block] [PATCH v6 1/6] qapi/qnull: Add own header

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
Reviewed-by: Markus Armbruster 
---
 include/qapi/qmp/qdict.h|  1 +
 include/qapi/qmp/qnull.h| 30 ++
 include/qapi/qmp/qobject.h  | 12 
 include/qapi/qmp/types.h|  1 +
 qapi/qapi-clone-visitor.c   |  1 +
 qapi/string-input-visitor.c |  1 +
 qobject/qnull.c |  2 +-
 tests/check-qnull.c |  2 +-
 8 files changed, 36 insertions(+), 14 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 6588c7f0c8..7ea5120c4a 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -15,6 +15,7 @@
 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
 #include "qapi/qmp/qnum.h"
 #include "qemu/queue.h"
 
diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
new file mode 100644
index 00..d075549283
--- /dev/null
+++ b/include/qapi/qmp/qnull.h
@@ -0,0 +1,30 @@
+/*
+ * QNull
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * Authors:
+ *  Markus Armbruster 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later.  See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef QNULL_H
+#define QNULL_H
+
+#include "qapi/qmp/qobject.h"
+
+struct QNull {
+QObject base;
+};
+
+extern QNull qnull_;
+
+static inline QNull *qnull(void)
+{
+QINCREF(_);
+return _;
+}
+
+#endif /* QNULL_H */
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index eab29edd12..ef1d1a9237 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -93,16 +93,4 @@ static inline QType qobject_type(const QObject *obj)
 return obj->type;
 }
 
-struct QNull {
-QObject base;
-};
-
-extern QNull qnull_;
-
-static inline QNull *qnull(void)
-{
-QINCREF(_);
-return _;
-}
-
 #endif /* QOBJECT_H */
diff --git a/include/qapi/qmp/types.h b/include/qapi/qmp/types.h
index a4bc662bfb..749ac44dcb 100644
--- a/include/qapi/qmp/types.h
+++ b/include/qapi/qmp/types.h
@@ -19,5 +19,6 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
 
 #endif /* QAPI_QMP_TYPES_H */
diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
index d8b62792bc..daab6819b4 100644
--- a/qapi/qapi-clone-visitor.c
+++ b/qapi/qapi-clone-visitor.c
@@ -12,6 +12,7 @@
 #include "qapi/clone-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qnull.h"
 
 struct QapiCloneVisitor {
 Visitor visitor;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 67a0a4a58b..b3fdd0827d 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -16,6 +16,7 @@
 #include "qapi/string-input-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
diff --git a/qobject/qnull.c b/qobject/qnull.c
index 69a21d1059..bc9fd31626 100644
--- a/qobject/qnull.c
+++ b/qobject/qnull.c
@@ -12,7 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
-#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qnull.h"
 
 QNull qnull_ = {
 .base = {
diff --git a/tests/check-qnull.c b/tests/check-qnull.c
index 5c6eb0adc8..afa4400da1 100644
--- a/tests/check-qnull.c
+++ b/tests/check-qnull.c
@@ -8,7 +8,7 @@
  */
 #include "qemu/osdep.h"
 
-#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu-common.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
-- 
2.13.6

[Qemu-block] [PATCH v6 0/6] block: Don't compare strings in bdrv_reopen_prepare()

[Max Reitz]

bdrv_reopen_prepare() assumes that all BDS options are strings, which is
not necessarily correct. This series introduces a new qobject_is_equal()
function which can be used to test whether any options have changed,
independently of their type.


v6:
- Patch 2: Added corresponding Coccinelle rule [Eric]
- Patch 6:
  - Added .gitignore entry [Eric]
  - Removed leading underscore [Markus]
  - Replaced test_equality() by check_equal()+check_unequal() [Markus]
  - Put type conversion (or rather non-conversion) tests into an own
function [Markus]
  - Fixed a line > 80 characters


git-backport-diff against v5:

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/6:[] [--] 'qapi/qnull: Add own header'
002/6:[0001] [FC] 'qapi/qlist: Add qlist_append_null() macro'
003/6:[] [--] 'qapi: Add qobject_is_equal()'
004/6:[] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
005/6:[] [--] 'iotests: Add test for non-string option reopening'
006/6:[0097] [FC] 'tests: Add check-qobject for equality tests'


Max Reitz (6):
  qapi/qnull: Add own header
  qapi/qlist: Add qlist_append_null() macro
  qapi: Add qobject_is_equal()
  block: qobject_is_equal() in bdrv_reopen_prepare()
  iotests: Add test for non-string option reopening
  tests: Add check-qobject for equality tests

 tests/Makefile.include   |   4 +-
 include/qapi/qmp/qbool.h |   1 +
 include/qapi/qmp/qdict.h |   2 +
 include/qapi/qmp/qlist.h |   4 +
 include/qapi/qmp/qnull.h |  32 
 include/qapi/qmp/qnum.h  |   1 +
 include/qapi/qmp/qobject.h   |  21 ++-
 include/qapi/qmp/qstring.h   |   1 +
 include/qapi/qmp/types.h |   1 +
 block.c  |  29 ++--
 qapi/qapi-clone-visitor.c|   1 +
 qapi/string-input-visitor.c  |   1 +
 qobject/qbool.c  |   8 +
 qobject/qdict.c  |  29 
 qobject/qlist.c  |  32 
 qobject/qnull.c  |  11 +-
 qobject/qnum.c   |  54 +++
 qobject/qobject.c|  29 
 qobject/qstring.c|   9 ++
 tests/check-qnull.c  |   2 +-
 tests/check-qobject.c| 325 +++
 scripts/coccinelle/qobject.cocci |   3 +
 tests/.gitignore |   1 +
 tests/qemu-iotests/133   |   9 ++
 tests/qemu-iotests/133.out   |   5 +
 25 files changed, 589 insertions(+), 26 deletions(-)
 create mode 100644 include/qapi/qmp/qnull.h
 create mode 100644 tests/check-qobject.c

-- 
2.13.6

[Qemu-block] [PATCH v6 2/6] qapi/qlist: Add qlist_append_null() macro

[Max Reitz]

Besides the macro itself, this patch also adds a corresponding
Coccinelle rule.

Signed-off-by: Max Reitz 
---
 include/qapi/qmp/qlist.h | 3 +++
 scripts/coccinelle/qobject.cocci | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
index c4b5fdad9b..59d209bbae 100644
--- a/include/qapi/qmp/qlist.h
+++ b/include/qapi/qmp/qlist.h
@@ -15,6 +15,7 @@
 
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qnull.h"
 #include "qemu/queue.h"
 
 typedef struct QListEntry {
@@ -37,6 +38,8 @@ typedef struct QList {
 qlist_append(qlist, qbool_from_bool(value))
 #define qlist_append_str(qlist, value) \
 qlist_append(qlist, qstring_from_str(value))
+#define qlist_append_null(qlist) \
+qlist_append(qlist, qnull())
 
 #define QLIST_FOREACH_ENTRY(qlist, var) \
 for ((var) = ((qlist)->head.tqh_first); \
diff --git a/scripts/coccinelle/qobject.cocci b/scripts/coccinelle/qobject.cocci
index 1120eb1a42..47bcafe9a9 100644
--- a/scripts/coccinelle/qobject.cocci
+++ b/scripts/coccinelle/qobject.cocci
@@ -41,4 +41,7 @@ expression Obj, E;
 |
 - qlist_append(Obj, qstring_from_str(E));
 + qlist_append_str(Obj, E);
+|
+- qlist_append(Obj, qnull());
++ qlist_append_null(Obj);
 )
-- 
2.13.6

Re: [Qemu-block] [Qemu-trivial] [PATCH] hw/block/onenand: Remove dead code block

[Kevin Wolf]

Am 03.10.2017 um 12:25 hat Laurent Vivier geschrieben:
> On 03/10/2017 11:57, Thomas Huth wrote:
> > The condition of the for-loop makes sure that b is always smaller
> > than s->blocks, so the "if (b >= s->blocks)" statement is completely
> > superfluous here.
> > 
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1715007
> > Signed-off-by: Thomas Huth 
> > ---
> >  hw/block/onenand.c | 4 
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> > index 30e40f3..de65c9e 100644
> > --- a/hw/block/onenand.c
> > +++ b/hw/block/onenand.c
> > @@ -520,10 +520,6 @@ static void onenand_command(OneNANDState *s)
> >  s->intstatus |= ONEN_INT;
> >  
> >  for (b = 0; b < s->blocks; b ++) {
> > -if (b >= s->blocks) {
> > -s->status |= ONEN_ERR_CMD;
> > -break;
> > -}
> >  if (s->blockwp[b] == ONEN_LOCK_LOCKTIGHTEN)
> >  break;
> >  
> > 
> 
> Looks like a bad cut'n'paste from case 0x23.
> 
> Reviewed-by: Laurent Vivier 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 4/8] qemu-options: Move -iscsi under "Block device options"

[ronnie sahlberg]

Reviewed-by: Ronnie Sahlberg 

Would be nice if this died at some stage :
[,password=password]

But that is for a different patch.

On Wed, Oct 4, 2017 at 8:12 PM, Marc-André Lureau
 wrote:
> On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
>> -iscsi ended up under the "Device URL Syntax" heading by a sequence of
>> errors, as explained in the previous commit.  Move it under the "Block
>> device options" heading.  Nothing left under "Device URL Syntax";
>> drop the heading.
>>
>> Cc: Ronnie Sahlberg 
>> Cc: Kevin Wolf 
>> Cc: Max Reitz 
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Markus Armbruster 
>
> Reviewed-by: Marc-André Lureau 
>
>
>> ---
>>  qemu-options.hx | 15 +++
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index f112281d37..c647fdde62 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -1172,6 +1172,13 @@ STEXI
>>  Create synthetic file system image
>>  ETEXI
>>
>> +DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>> +"-iscsi [user=user][,password=password]\n"
>> +"   [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
>> +"   [,initiator-name=initiator-iqn][,id=target-iqn]\n"
>> +"   [,timeout=timeout]\n"
>> +"iSCSI session parameters\n", QEMU_ARCH_ALL)
>> +
>>  STEXI
>>  @end table
>>  ETEXI
>> @@ -2811,14 +2818,6 @@ STEXI
>>  ETEXI
>>  DEFHEADING()
>>
>> -DEFHEADING(Device URL Syntax:)
>> -DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>> -"-iscsi [user=user][,password=password]\n"
>> -"   [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
>> -"   [,initiator-name=initiator-iqn][,id=target-iqn]\n"
>> -"   [,timeout=timeout]\n"
>> -"iSCSI session parameters\n", QEMU_ARCH_ALL)
>> -
>>  DEFHEADING(Bluetooth(R) options:)
>>  STEXI
>>  @table @option
>> --
>> 2.13.6
>>
>>
>
>
>
> --
> Marc-André Lureau

Re: [Qemu-block] [Qemu-devel] [PATCH 3/8] qemu-options qemu-doc: Move "Device URL Syntax" to qemu-doc

[ronnie sahlberg]

On Wed, Oct 4, 2017 at 8:12 PM, Marc-André Lureau
 wrote:
> On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
>> Commit 0f5314a (v1.0) added section "Device URL Syntax" to
>> qemu-options.hx.  It's enclosed in STEXI..ETEXI, thus affects only
>> qemu-options.texi, not --help.  It appears as a subsection under
>> section "Invocation".  Similarly, qemu.1 has it as a subsection under
>> "OPTIONS".
>>
>> Commit f9dadc9 (v1.1.0) dropped new option -iscsi into the middle of
>> this section.  No effect on qemu-options.texi.  It appears in --help
>> run together with the "Bluetooth(R) options:" header.
>>
>> Commit c70a01e (v1.5.0) gives it is own heading in --help by moving
>> commit 0f5314a's DEFHEADING(Device URL Syntax:) outside STEXI..ETEXI.
>> Trouble is the heading makes no sense for -iscsi.
>>
>> Move all of the "Device URL Syntax" Texinfo to qemu-doc.texi.  Mark it
>> for inclusion in qemu.1 with '@c man begin NOTES'.  This turns it into
>> a separate section outside the list of options both in qemu-doc and in
>> qemu.1.
>>
>> There's substantial overlap with the existing qemu-doc section "Disk
>> Images".  Mark with a TODO comment.
>>
>> Output of --help will be fixed next.
>>
>> Cc: Ronnie Sahlberg 
>> Cc: Kevin Wolf 
>> Cc: Max Reitz 
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Markus Armbruster 
>
> Reviewed-by: Marc-André Lureau 
>
>
>> ---
>>  qemu-doc.texi   | 217 ++
>>  qemu-options.hx | 222 
>> 
>>  2 files changed, 217 insertions(+), 222 deletions(-)
>>
>> diff --git a/qemu-doc.texi b/qemu-doc.texi
>> index ecd186a159..848e49966a 100644
>> --- a/qemu-doc.texi
>> +++ b/qemu-doc.texi
>> @@ -245,6 +245,223 @@ targets do not need a disk image.
>>
>>  @c man end
>>
>> +@node device_url
>> +@subsection Device URL Syntax
>> +@c TODO merge this with section Disk Images
>> +
>> +@c man begin NOTES
>> +
>> +In addition to using normal file images for the emulated storage devices,
>> +QEMU can also use networked resources such as iSCSI devices. These are
>> +specified using a special URL syntax.
>> +
>> +@table @option
>> +@item iSCSI
>> +iSCSI support allows QEMU to access iSCSI resources directly and use as
>> +images for the guest storage. Both disk and cdrom images are supported.
>> +
>> +Syntax for specifying iSCSI LUNs is
>> +``iscsi://[:]//''
>> +
>> +By default qemu will use the iSCSI initiator-name
>> +'iqn.2008-11.org.linux-kvm[:]' but this can also be set from the 
>> command
>> +line or a configuration file.
>> +
>> +Since version Qemu 2.4 it is possible to specify a iSCSI request timeout to 
>> detect
>> +stalled requests and force a reestablishment of the session. The timeout
>> +is specified in seconds. The default is 0 which means no timeout. Libiscsi
>> +1.15.0 or greater is required for this feature.
>> +
>> +Example (without authentication):
>> +@example
>> +qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator 
>> \
>> + -cdrom iscsi://192.0.2.1/iqn.2001-04.com.example/2 \
>> + -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
>> +@end example
>> +
>> +Example (CHAP username/password via URL):
>> +@example
>> +qemu-system-i386 -drive 
>> file=iscsi://user%password@@192.0.2.1/iqn.2001-04.com.example/1
>> +@end example
>> +
>> +Example (CHAP username/password via environment variables):
>> +@example
>> +LIBISCSI_CHAP_USERNAME="user" \
>> +LIBISCSI_CHAP_PASSWORD="password" \
>> +qemu-system-i386 -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
>> +@end example
>> +
>> +@item NBD
>> +QEMU supports NBD (Network Block Devices) both using TCP protocol as well
>> +as Unix Domain Sockets.
>> +
>> +Syntax for specifying a NBD device using TCP
>> +``nbd::[:exportname=]''
>> +
>> +Syntax for specifying a NBD device using Unix Domain Sockets
>> +``nbd:unix:[:exportname=]''
>> +
>> +Example for TCP
>> +@example
>> +qemu-system-i386 --drive file=nbd:192.0.2.1:3
>> +@end example
>> +
>> +Example for Unix Domain Sockets
>> +@example
>> +qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket
>> +@end example
>> +
>> +@item SSH
>> +QEMU supports SSH (Secure Shell) access to remote disks.
>> +
>> +Examples:
>> +@example
>> +qemu-system-i386 -drive file=ssh://user@@host/path/to/disk.img
>> +qemu-system-i386 -drive 
>> file.driver=ssh,file.user=user,file.host=host,file.port=22,file.path=/path/to/disk.img
>> +@end example
>> +
>> +Currently authentication must be done using ssh-agent.  Other
>> +authentication methods may be supported in future.
>> +
>> +@item Sheepdog
>> +Sheepdog is a distributed storage system for QEMU.
>> +QEMU supports using either local sheepdog devices or remote networked
>> +devices.
>> +
>> +Syntax for specifying a sheepdog device
>> 

Re: [Qemu-block] [PATCH 5/8] qemu-options: Add missing -iscsi Texinfo documentation

[ronnie sahlberg]

Reviewed-by: Ronnie Sahlberg 

On Tue, Oct 3, 2017 at 12:03 AM, Markus Armbruster  wrote:
> Cc: Ronnie Sahlberg 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: qemu-block@nongnu.org
> Signed-off-by: Markus Armbruster 
> ---
>  qemu-options.hx | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index c647fdde62..8568ee388c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1180,6 +1180,12 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>  "iSCSI session parameters\n", QEMU_ARCH_ALL)
>
>  STEXI
> +@item -iscsi
> +@findex -iscsi
> +Configure iSCSI session parameters.
> +ETEXI
> +
> +STEXI
>  @end table
>  ETEXI
>  DEFHEADING()
> --
> 2.13.6
>

Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/2] virtio: introduce `query-virtio' QMP command

[Jan Dakinevich]

On 10/03/2017 07:29 PM, Dr. David Alan Gilbert wrote:
> * Jan Dakinevich (jan.dakinev...@virtuozzo.com) wrote:
>>
>>
>> On 10/03/2017 05:02 PM, Eric Blake wrote:
>>> On 10/03/2017 07:47 AM, Jan Dakinevich wrote:
 The command is intended for gathering virtio information such as status,
 feature bits, negotiation status. It is convenient and useful for debug
 purpose.

 The commands returns generic virtio information for virtio such as
 common feature names and status bits names and information for all
 attached to current machine devices.

 To retrieve names of device-specific features `get_feature_name'
 callback in VirtioDeviceClass also was introduced.

 Cc: Denis V. Lunev 
 Signed-off-by: Jan Dakinevich 
 ---
  hw/block/virtio-blk.c   |  21 +
  hw/char/virtio-serial-bus.c |  15 +++
  hw/display/virtio-gpu.c |  13 ++
  hw/net/virtio-net.c |  35 +++
  hw/scsi/virtio-scsi.c   |  16 +++
  hw/virtio/Makefile.objs |   2 +
  hw/virtio/virtio-balloon.c  |  15 +++
  hw/virtio/virtio-stub.c |   9 
  hw/virtio/virtio.c  | 101 
 
  include/hw/virtio/virtio.h  |   2 +
  qapi-schema.json|   1 +
  qapi/virtio.json|  94 
 +
  12 files changed, 324 insertions(+)
  create mode 100644 hw/virtio/virtio-stub.c
  create mode 100644 qapi/virtio.json
>>>
>>> This creates a new .json file, but does not touch MAINTAINERS.  Our idea
>>> in splitting the .json files was to make it easier for each sub-file
>>> that needs a specific maintainer in addition to the overall *.json line
>>> for QAPI maintainers, so this may deserve a MAINTAINERS entry.
>>>
>>
>> Ok.
>>
 +++ b/qapi/virtio.json
 @@ -0,0 +1,94 @@
 +# -*- Mode: Python -*-
 +#
 +
 +##
 +# = Virtio devices
 +##
 +
 +{ 'include': 'common.json' }
 +
 +##
 +# @VirtioInfoBit:
 +#
 +# Named virtio bit
 +#
 +# @bit: bit number
 +#
 +# @name: bit name
 +#
 +# Since: 2.11.0
 +#
 +##
 +{
 +'struct': 'VirtioInfoBit',
 +'data': {
 +'bit': 'uint64',
>>>
>>> Why is this a 64-bit value? Are the values 0-63, or are they 1, 2, 4, 8,
>>> ...?  The documentation on 'bit number' is rather sparse.
>>
>> I would prefer `uint' here, but I don't see generic unsigned type (may
>> be, I am mistaken). I could use uint8 here, though.
>>
>>>
 +'name': 'str'
>>>
>>> Wouldn't an enum type be better than an open-ended string?
>>>
>>
>> Bit names are not known here, they are obtained from virtio device
>> implementations.
>>
 +}
 +}
 +
 +##
 +# @VirtioInfoDevice:
 +#
 +# Information about specific virtio device
 +#
 +# @qom_path: QOM path of the device
>>>
>>> Please make this 'qom-path' - new interfaces should prefer '-' over '_'.
>>
>> Ok.
>>
 +#
 +# @feature-names: names of device-specific features
 +#
 +# @host-features: bitmask of features, provided by devices
 +#
 +# @guest-features: bitmask of features, acknowledged by guest
 +#
 +# @status: virtio device status bitmask
 +#
 +# Since: 2.11.0
 +#
 +##
 +{
 +'struct': 'VirtioInfoDevice',
 +'data': {
 +'qom_path': 'str',
 +'feature-names': ['VirtioInfoBit'],
 +'host-features': 'uint64',
 +'guest-features': 'uint64',
 +'status': 'uint64'
>>>
>>> I'm wondering if this is the best representation (where the caller has
>>> to parse the integer and then lookup in feature-names what each bit of
>>> the integer represents).  But I'm not sure I have anything better off
>>> the top of my head.
>>>
>>
>> Consider it as way to tell caller about names of supported features.
>>
 +}
 +}
 +
 +##
 +# @VirtioInfo:
 +#
 +# Information about virtio devices
 +#
 +# @feature-names: names of common virtio features
 +#
 +# @status-names: names of bits which represents virtio device status
 +#
 +# @devices: list of per-device virtio information
 +#
 +# Since: 2.11.0
 +#
 +##
 +{
 +'struct': 'VirtioInfo',
 +'data': {
 +'feature-names': ['VirtioInfoBit'],
>>>
>>> Why is feature-names listed at two different nestings of the return value?
>>>
>>
>> These are different feature names. First names are common and predefined
>> for all devices. Second names are device-specific.
> 
> If you can turn these into enums (union'd enums?) then you might
> be able to get rid of a lot of your array filling/naming conversion
> boilerplate. (Not sure if it's worth it, but it's worth looking).
> 

I would be happy to drop this boilerplate, but 

Re: [Qemu-block] xen-pci-passthrough PCI Express support? (Re: [Qemu-devel] [Xen-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices)

[Eduardo Habkost]

On Wed, Oct 04, 2017 at 03:08:15AM -0600, Jan Beulich wrote:
> >>> On 03.10.17 at 02:12,  wrote:
> > On Thu, Sep 28, 2017 at 10:12:34AM -0300, Eduardo Habkost wrote:
> >> On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
> >> > >>> On 27.09.17 at 21:56,  wrote:
> >> > > --- a/hw/xen/xen_pt.c
> >> > > +++ b/hw/xen/xen_pt.c
> >> > > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
> >> > >  .instance_size = sizeof(XenPCIPassthroughState),
> >> > >  .instance_finalize = xen_pci_passthrough_finalize,
> >> > >  .class_init = xen_pci_passthrough_class_init,
> >> > > +.interfaces = (InterfaceInfo[]) {
> >> > > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
> >> > > +{ },
> >> > > +},
> >> > >  };
> >> > 
> >> > Passed through devices can be both PCI and PCIe, so following
> >> > the description of the patch I don't think these can be statically
> >> > given either property. Granted quite a bit of PCIe specific
> >> > functionality may be missing in the Xen code ...
> >> 
> >> This is just static data about what the device type supports, not
> >> about what a given device instance really is.  Deciding if the
> >> device is PCIe or Conventional at runtime is out of the scope of
> >> this series.
> >> 
> >> That said, if passed through PCI Express devices are really
> >> supported, it looks like this should be marked as hybrid.
> > 
> > Can anybody confirm if PCI Express devices are really supported
> > by xen-pci-passthrough?
> 
> I think I've clearly said they're supported, with some limitations.

Sorry, thanks.  I thought the possible missing PCIe functionality
could mean the device couldn't appear as PCI Express to the
guest.

I will submit a follow-up patch adding INTERFACE_PCIE_DEVICE to
xen-pci-passthrough.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH 5/8] qemu-options: Add missing -iscsi Texinfo documentation

[Markus Armbruster]

Marc-André Lureau  writes:

> On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
>> Cc: Ronnie Sahlberg 
>> Cc: Kevin Wolf 
>> Cc: Max Reitz 
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Markus Armbruster 
>
> Reviewed-by: Marc-André Lureau 
> (that's very short doc)

No argument.  It's what I could do while cleaning up a mess that's
blocking my path to what I really want to do.  Patches welcome :)

>> ---
>>  qemu-options.hx | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index c647fdde62..8568ee388c 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -1180,6 +1180,12 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>>  "iSCSI session parameters\n", QEMU_ARCH_ALL)
>>
>>  STEXI
>> +@item -iscsi
>> +@findex -iscsi
>> +Configure iSCSI session parameters.
>> +ETEXI
>> +
>> +STEXI
>>  @end table
>>  ETEXI
>>  DEFHEADING()
>> --
>> 2.13.6
>>
>>

Re: [Qemu-block] [Qemu-devel] [PATCH 3/8] qemu-options qemu-doc: Move "Device URL Syntax" to qemu-doc

[Markus Armbruster]

ronnie sahlberg  writes:

> On Wed, Oct 4, 2017 at 8:12 PM, Marc-André Lureau
>  wrote:
>> On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
>>> Commit 0f5314a (v1.0) added section "Device URL Syntax" to
>>> qemu-options.hx.  It's enclosed in STEXI..ETEXI, thus affects only
>>> qemu-options.texi, not --help.  It appears as a subsection under
>>> section "Invocation".  Similarly, qemu.1 has it as a subsection under
>>> "OPTIONS".
>>>
>>> Commit f9dadc9 (v1.1.0) dropped new option -iscsi into the middle of
>>> this section.  No effect on qemu-options.texi.  It appears in --help
>>> run together with the "Bluetooth(R) options:" header.
>>>
>>> Commit c70a01e (v1.5.0) gives it is own heading in --help by moving
>>> commit 0f5314a's DEFHEADING(Device URL Syntax:) outside STEXI..ETEXI.
>>> Trouble is the heading makes no sense for -iscsi.
>>>
>>> Move all of the "Device URL Syntax" Texinfo to qemu-doc.texi.  Mark it
>>> for inclusion in qemu.1 with '@c man begin NOTES'.  This turns it into
>>> a separate section outside the list of options both in qemu-doc and in
>>> qemu.1.
>>>
>>> There's substantial overlap with the existing qemu-doc section "Disk
>>> Images".  Mark with a TODO comment.
>>>
>>> Output of --help will be fixed next.
>>>
>>> Cc: Ronnie Sahlberg 
>>> Cc: Kevin Wolf 
>>> Cc: Max Reitz 
>>> Cc: qemu-block@nongnu.org
>>> Signed-off-by: Markus Armbruster 
>>
>> Reviewed-by: Marc-André Lureau 
>>
>>
>>> ---
>>>  qemu-doc.texi   | 217 
>>> ++
>>>  qemu-options.hx | 222 
>>> 
>>>  2 files changed, 217 insertions(+), 222 deletions(-)
>>>
>>> diff --git a/qemu-doc.texi b/qemu-doc.texi
>>> index ecd186a159..848e49966a 100644
>>> --- a/qemu-doc.texi
>>> +++ b/qemu-doc.texi
>>> @@ -245,6 +245,223 @@ targets do not need a disk image.
>>>
>>>  @c man end
>>>
>>> +@node device_url
>>> +@subsection Device URL Syntax
>>> +@c TODO merge this with section Disk Images
>>> +
>>> +@c man begin NOTES
>>> +
>>> +In addition to using normal file images for the emulated storage devices,
>>> +QEMU can also use networked resources such as iSCSI devices. These are
>>> +specified using a special URL syntax.
>>> +
>>> +@table @option
>>> +@item iSCSI
>>> +iSCSI support allows QEMU to access iSCSI resources directly and use as
>>> +images for the guest storage. Both disk and cdrom images are supported.
>>> +
>>> +Syntax for specifying iSCSI LUNs is
>>> +``iscsi://[:]//''
>>> +
>>> +By default qemu will use the iSCSI initiator-name
>>> +'iqn.2008-11.org.linux-kvm[:]' but this can also be set from the 
>>> command
>>> +line or a configuration file.
>>> +
>>> +Since version Qemu 2.4 it is possible to specify a iSCSI request timeout 
>>> to detect
>>> +stalled requests and force a reestablishment of the session. The timeout
>>> +is specified in seconds. The default is 0 which means no timeout. Libiscsi
>>> +1.15.0 or greater is required for this feature.
>>> +
>>> +Example (without authentication):
>>> +@example
>>> +qemu-system-i386 -iscsi 
>>> initiator-name=iqn.2001-04.com.example:my-initiator \
>>> + -cdrom iscsi://192.0.2.1/iqn.2001-04.com.example/2 \
>>> + -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
>>> +@end example
>>> +
>>> +Example (CHAP username/password via URL):
>>> +@example
>>> +qemu-system-i386 -drive 
>>> file=iscsi://user%password@@192.0.2.1/iqn.2001-04.com.example/1
>>> +@end example
>>> +
>>> +Example (CHAP username/password via environment variables):
>>> +@example
>>> +LIBISCSI_CHAP_USERNAME="user" \
>>> +LIBISCSI_CHAP_PASSWORD="password" \
>>> +qemu-system-i386 -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
>>> +@end example
>>> +
>>> +@item NBD
>>> +QEMU supports NBD (Network Block Devices) both using TCP protocol as well
>>> +as Unix Domain Sockets.
>>> +
>>> +Syntax for specifying a NBD device using TCP
>>> +``nbd::[:exportname=]''
>>> +
>>> +Syntax for specifying a NBD device using Unix Domain Sockets
>>> +``nbd:unix:[:exportname=]''
>>> +
>>> +Example for TCP
>>> +@example
>>> +qemu-system-i386 --drive file=nbd:192.0.2.1:3
>>> +@end example
>>> +
>>> +Example for Unix Domain Sockets
>>> +@example
>>> +qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket
>>> +@end example
>>> +
>>> +@item SSH
>>> +QEMU supports SSH (Secure Shell) access to remote disks.
>>> +
>>> +Examples:
>>> +@example
>>> +qemu-system-i386 -drive file=ssh://user@@host/path/to/disk.img
>>> +qemu-system-i386 -drive 
>>> file.driver=ssh,file.user=user,file.host=host,file.port=22,file.path=/path/to/disk.img
>>> +@end example
>>> +
>>> +Currently authentication must be done using ssh-agent.  Other
>>> +authentication methods may be supported in future.
>>> +
>>> +@item Sheepdog
>>> 

Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Kevin Wolf]

Am 04.10.2017 um 14:23 hat Manos Pitsidianakis geschrieben:
> > > diff --git a/block.c b/block.c
> > > index 81bd51b670..f874aabbfb 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > +/* insert 'node' as child bs of 'parent' node */
> > > +if (check_node_edge(parent, child, errp)) {
> > > +return;
> > > +}
> > > +parent_bs = bdrv_find_node(parent);
> > > +c = bdrv_find_child(parent_bs, child);
> > > +role = c->role;
> > > +assert(role == _file || role == _backing);
> > > +
> > > +bdrv_ref(node_bs);
> > > +
> > > +bdrv_drained_begin(parent_bs);
> > > +bdrv_unref_child(parent_bs, c);
> > > +if (role == _file) {
> > > +parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
> > > +_file, errp);
> > > +if (!parent_bs->file) {
> > > +parent_bs->file = bdrv_attach_child(parent_bs, child_bs, 
> > > "file",
> > > +_file, 
> > > _abort);
> > > +goto out;
> > > +}
> > > +} else if (role == _backing) {
> > > +parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, 
> > > "backing",
> > > +   _backing, errp);
> > > +if (!parent_bs->backing) {
> > > +parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
> > > +   "backing", 
> > > _backing,
> > > +   _abort);
> > > +goto out;
> > > +}
> > > +}
> > 
> > I would prefer if we could find a solution to avoid requiring a specific
> > role. I'm not even sure that your assertion above is correct; can you
> > explain why c couldn't have any other role?
> > 
> > Instead of bdrv_unref_child/bdrv_attach_child, could we just change
> > where the child points to using bdrv_replace_child()? Then
> 
> bdrv_replace_child() uses bdrv_set_perm() and co. When I tried it at first I
> got errors like "Conflicts with use by ** as 'backing', which does not
> allow 'write' on disk". Presumably the permissions do not need to change but
> can we do bdrv_set_perm without bdrv_check_perm?

Which child is conflicting with which other child? Is c conflicting with
itself or something?

If unref_child/attach_child works without any other action in between,
there is no reason why replace_child shouldn't work, too. Maybe this is
a bug in bdrv_

> > parent_bs->file and parent_bs->backing (or whatever other variable
> > contains the BdrvChild pointer) can stay unchanged and just keep
> > working.
> > 
> > > +bdrv_refresh_filename(parent_bs);
> > > +bdrv_refresh_limits(parent_bs, NULL);
> > > +
> > > +out:
> > > +bdrv_drained_end(parent_bs);
> > > +}
> > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 8e2fc6e64c..5195ec1b61 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -4238,3 +4238,47 @@ QemuOptsList qemu_drive_opts = {
> > >  { /* end of list */ }
> > >  },
> > >  };
> > > +
> > > +void qmp_block_insert_node(const char *parent, const char *child,
> > > +   const char *node, Error **errp)
> > > +{
> > > +BlockDriverState *bs = bdrv_find_node(node);
> > > +if (!bs) {
> > > +error_setg(errp, "Node '%s' not found", node);
> > > +return;
> > > +}
> > > +if (!bs->monitor_list.tqe_prev) {
> > > +error_setg(errp, "Node '%s' is not owned by the monitor",
> > > +   bs->node_name);
> > > +return;
> > > +}
> > > +if (!bs->drv->is_filter) {
> > > +error_setg(errp, "Block format '%s' used by node '%s' does not 
> > > support"
> > > +   "insertion", bs->drv->format_name, bs->node_name);
> > > +return;
> > > +}
> > > +
> > > +bdrv_insert_node(parent, child, node, errp);
> > > +}
> > 
> > Do we need to acquire an AioContext lock somewhere?
> 
> the *_child() functions call drained_begin/end which I think might cover
> this case?

I don't think it's enough when you don't own the AioContext lock.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Max Reitz]

On 2017-08-15 09:45, Manos Pitsidianakis wrote:
> block-insert-node and its pair command block-remove-node provide runtime
> insertion and removal of filter nodes.
> 
> block-insert-node takes a (parent, child) and (node, child) pair of
> edges and unrefs the (parent, child) BdrvChild relationship and creates
> a new (parent, node) child with the same BdrvChildRole.
> 
> This is a different approach than x-blockdev-change which uses the driver
> methods bdrv_add_child() and bdrv_del_child(),

Why? :-)

Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.

I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that.  If it doesn't, then just attach the child.

Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached).  Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands.  In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver.  (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

On Fri, Sep 29, 2017 at 07:52:35PM +0200, Kevin Wolf wrote:

Am 15.08.2017 um 09:45 hat Manos Pitsidianakis geschrieben:

block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.

block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.

This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),

Signed-off-by: Manos Pitsidianakis 



diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4d6ba1baef..16e19cb648 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3947,3 +3947,63 @@
   'data' : { 'parent': 'str',
  '*child': 'str',
  '*node': 'str' } }
+
+##
+# @block-insert-node:
+#
+# Insert a filter node between a specific edge in the block driver state graph.
+# @parent:  the name of the parent node or device
+# @node:the name of the node to insert under parent
+# @child:   the name of the child of both node and parent
+#
+# Example:
+# Insert and remove a throttle filter on top of a device chain, between the
+# device 'ide0-hd0' and node 'node-A'
+#
+# -> {'execute': 'object-add',
+# "arguments": {
+#   "qom-type": "throttle-group",
+#   "id": "group0",
+#   "props" : { "limits": { "iops-total": 300 } }
+# }
+#}
+# <- { 'return': {} }
+# -> {'execute': 'blockdev-add',
+#   'arguments': {
+#   'driver': 'throttle',
+#   'node-name': 'throttle0',
+#   'throttle-group': 'group0',
+#   'file': 'node-A'
+#   }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'block-insert-node',
+#   'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 
'throttle0' }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'block-remove-node',
+#   'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 
'throttle0' }
+#}
+# <- { 'return': {} }
+# -> { 'execute': 'blockdev-del',
+#   'arguments': { 'node-name': 'throttle0' }
+#}
+# <- { 'return': {} }
+#
+##
+{ 'command': 'block-insert-node',
+  'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }


I would suggest a change to the meaning of @child: Instead of using the
node-name of the child BDS, I would use the name of the BdrvChild that
represents the link.

The reason for this is that the node-name could be ambiguous, if you
have two edges between the same two nodes.

The only use of the node-name of the child that I can remember was for
checking that the graph still looks like what the user expects. But I
think we came to the conclusion that there are no race conditions to
check for if we have manual block job deletion instead of automatic
completion which can involve surprise changes to the graph. So probably
we don't need the node-name even for this.


+##
+# @block-remove-node:
+#
+# Remove a filter node between two other nodes in the block driver state graph.
+# @parent:  the name of the parent node or device
+# @node:the name of the node to remove from parent
+# @child:   the name of the child of node which will go under parent
+##
+{ 'command': 'block-remove-node',
+  'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }


Same thing here.


diff --git a/block.c b/block.c
index 81bd51b670..f874aabbfb 100644
--- a/block.c
+++ b/block.c
+/* insert 'node' as child bs of 'parent' node */
+if (check_node_edge(parent, child, errp)) {
+return;
+}
+parent_bs = bdrv_find_node(parent);
+c = bdrv_find_child(parent_bs, child);
+role = c->role;
+assert(role == _file || role == _backing);
+
+bdrv_ref(node_bs);
+
+bdrv_drained_begin(parent_bs);
+bdrv_unref_child(parent_bs, c);
+if (role == _file) {
+parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
+_file, errp);
+if (!parent_bs->file) {
+parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
+_file, _abort);
+goto out;
+}
+} else if (role == _backing) {
+parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, "backing",
+   _backing, errp);
+if (!parent_bs->backing) {
+parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
+   "backing", _backing,
+   _abort);
+goto out;
+}
+}


I would prefer if we could find a solution to avoid requiring a specific
role. I'm not even sure that your assertion above is correct; can you
explain why c couldn't have any other role?

Instead of bdrv_unref_child/bdrv_attach_child, could we just 

Re: [Qemu-block] [PATCH v3 28/44] block: Fix pending requests check in bdrv_append()

[Alberto Garcia]

On Tue 28 Feb 2017 01:54:13 PM CET, Kevin Wolf wrote:
> bdrv_append() cares about isolation of the node that it modifies, but
> not about activity in some subtree below it. Instead of using the
> recursive bdrv_requests_pending(), directly check bs->in_flight, which
> considers only the node in question.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Max Reitz 
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9e538a5..5189c7c 100644
> --- a/block.c
> +++ b/block.c
> @@ -2897,8 +2897,8 @@ static void change_parent_backing_link(BlockDriverState 
> *from,
>   */
>  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>  {
> -assert(!bdrv_requests_pending(bs_top));
> -assert(!bdrv_requests_pending(bs_new));
> +assert(!atomic_read(_top->in_flight));
> +assert(!atomic_read(_new->in_flight));
>  
>  bdrv_ref(bs_top);

I don't know if there's still any maintenance on the v2.8.x branch, but
I can make v2.8.1.1 crash easily because of this failing assertion.
Applying this patch fixes the issue.

I can provide more details if needed.

Regards,

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH 5/8] qemu-options: Add missing -iscsi Texinfo documentation

[Marc-André Lureau]

On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
> Cc: Ronnie Sahlberg 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: qemu-block@nongnu.org
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 
(that's very short doc)


> ---
>  qemu-options.hx | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index c647fdde62..8568ee388c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1180,6 +1180,12 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>  "iSCSI session parameters\n", QEMU_ARCH_ALL)
>
>  STEXI
> +@item -iscsi
> +@findex -iscsi
> +Configure iSCSI session parameters.
> +ETEXI
> +
> +STEXI
>  @end table
>  ETEXI
>  DEFHEADING()
> --
> 2.13.6
>
>



-- 
Marc-André Lureau

Re: [Qemu-block] [Qemu-devel] [PATCH 3/8] qemu-options qemu-doc: Move "Device URL Syntax" to qemu-doc

[Marc-André Lureau]

On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
> Commit 0f5314a (v1.0) added section "Device URL Syntax" to
> qemu-options.hx.  It's enclosed in STEXI..ETEXI, thus affects only
> qemu-options.texi, not --help.  It appears as a subsection under
> section "Invocation".  Similarly, qemu.1 has it as a subsection under
> "OPTIONS".
>
> Commit f9dadc9 (v1.1.0) dropped new option -iscsi into the middle of
> this section.  No effect on qemu-options.texi.  It appears in --help
> run together with the "Bluetooth(R) options:" header.
>
> Commit c70a01e (v1.5.0) gives it is own heading in --help by moving
> commit 0f5314a's DEFHEADING(Device URL Syntax:) outside STEXI..ETEXI.
> Trouble is the heading makes no sense for -iscsi.
>
> Move all of the "Device URL Syntax" Texinfo to qemu-doc.texi.  Mark it
> for inclusion in qemu.1 with '@c man begin NOTES'.  This turns it into
> a separate section outside the list of options both in qemu-doc and in
> qemu.1.
>
> There's substantial overlap with the existing qemu-doc section "Disk
> Images".  Mark with a TODO comment.
>
> Output of --help will be fixed next.
>
> Cc: Ronnie Sahlberg 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: qemu-block@nongnu.org
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  qemu-doc.texi   | 217 ++
>  qemu-options.hx | 222 
> 
>  2 files changed, 217 insertions(+), 222 deletions(-)
>
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index ecd186a159..848e49966a 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -245,6 +245,223 @@ targets do not need a disk image.
>
>  @c man end
>
> +@node device_url
> +@subsection Device URL Syntax
> +@c TODO merge this with section Disk Images
> +
> +@c man begin NOTES
> +
> +In addition to using normal file images for the emulated storage devices,
> +QEMU can also use networked resources such as iSCSI devices. These are
> +specified using a special URL syntax.
> +
> +@table @option
> +@item iSCSI
> +iSCSI support allows QEMU to access iSCSI resources directly and use as
> +images for the guest storage. Both disk and cdrom images are supported.
> +
> +Syntax for specifying iSCSI LUNs is
> +``iscsi://[:]//''
> +
> +By default qemu will use the iSCSI initiator-name
> +'iqn.2008-11.org.linux-kvm[:]' but this can also be set from the 
> command
> +line or a configuration file.
> +
> +Since version Qemu 2.4 it is possible to specify a iSCSI request timeout to 
> detect
> +stalled requests and force a reestablishment of the session. The timeout
> +is specified in seconds. The default is 0 which means no timeout. Libiscsi
> +1.15.0 or greater is required for this feature.
> +
> +Example (without authentication):
> +@example
> +qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator \
> + -cdrom iscsi://192.0.2.1/iqn.2001-04.com.example/2 \
> + -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
> +@end example
> +
> +Example (CHAP username/password via URL):
> +@example
> +qemu-system-i386 -drive 
> file=iscsi://user%password@@192.0.2.1/iqn.2001-04.com.example/1
> +@end example
> +
> +Example (CHAP username/password via environment variables):
> +@example
> +LIBISCSI_CHAP_USERNAME="user" \
> +LIBISCSI_CHAP_PASSWORD="password" \
> +qemu-system-i386 -drive file=iscsi://192.0.2.1/iqn.2001-04.com.example/1
> +@end example
> +
> +@item NBD
> +QEMU supports NBD (Network Block Devices) both using TCP protocol as well
> +as Unix Domain Sockets.
> +
> +Syntax for specifying a NBD device using TCP
> +``nbd::[:exportname=]''
> +
> +Syntax for specifying a NBD device using Unix Domain Sockets
> +``nbd:unix:[:exportname=]''
> +
> +Example for TCP
> +@example
> +qemu-system-i386 --drive file=nbd:192.0.2.1:3
> +@end example
> +
> +Example for Unix Domain Sockets
> +@example
> +qemu-system-i386 --drive file=nbd:unix:/tmp/nbd-socket
> +@end example
> +
> +@item SSH
> +QEMU supports SSH (Secure Shell) access to remote disks.
> +
> +Examples:
> +@example
> +qemu-system-i386 -drive file=ssh://user@@host/path/to/disk.img
> +qemu-system-i386 -drive 
> file.driver=ssh,file.user=user,file.host=host,file.port=22,file.path=/path/to/disk.img
> +@end example
> +
> +Currently authentication must be done using ssh-agent.  Other
> +authentication methods may be supported in future.
> +
> +@item Sheepdog
> +Sheepdog is a distributed storage system for QEMU.
> +QEMU supports using either local sheepdog devices or remote networked
> +devices.
> +
> +Syntax for specifying a sheepdog device
> +@example
> +sheepdog[+tcp|+unix]://[host:port]/vdiname[?socket=path][#snapid|#tag]
> +@end example
> +
> +Example
> +@example
> +qemu-system-i386 --drive file=sheepdog://192.0.2.1:3/MyVirtualMachine
> +@end example
> +
> +See also 

Re: [Qemu-block] [Qemu-devel] [PATCH 4/8] qemu-options: Move -iscsi under "Block device options"

[Marc-André Lureau]

On Mon, Oct 2, 2017 at 4:03 PM, Markus Armbruster  wrote:
> -iscsi ended up under the "Device URL Syntax" heading by a sequence of
> errors, as explained in the previous commit.  Move it under the "Block
> device options" heading.  Nothing left under "Device URL Syntax";
> drop the heading.
>
> Cc: Ronnie Sahlberg 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: qemu-block@nongnu.org
> Signed-off-by: Markus Armbruster 

Reviewed-by: Marc-André Lureau 


> ---
>  qemu-options.hx | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index f112281d37..c647fdde62 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1172,6 +1172,13 @@ STEXI
>  Create synthetic file system image
>  ETEXI
>
> +DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
> +"-iscsi [user=user][,password=password]\n"
> +"   [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
> +"   [,initiator-name=initiator-iqn][,id=target-iqn]\n"
> +"   [,timeout=timeout]\n"
> +"iSCSI session parameters\n", QEMU_ARCH_ALL)
> +
>  STEXI
>  @end table
>  ETEXI
> @@ -2811,14 +2818,6 @@ STEXI
>  ETEXI
>  DEFHEADING()
>
> -DEFHEADING(Device URL Syntax:)
> -DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
> -"-iscsi [user=user][,password=password]\n"
> -"   [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
> -"   [,initiator-name=initiator-iqn][,id=target-iqn]\n"
> -"   [,timeout=timeout]\n"
> -"iSCSI session parameters\n", QEMU_ARCH_ALL)
> -
>  DEFHEADING(Bluetooth(R) options:)
>  STEXI
>  @table @option
> --
> 2.13.6
>
>



-- 
Marc-André Lureau

[Qemu-block] xen-pci-passthrough PCI Express support? (Re: [Qemu-devel] [Xen-devel] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices)

[Jan Beulich]

>>> On 03.10.17 at 02:12,  wrote:
> On Thu, Sep 28, 2017 at 10:12:34AM -0300, Eduardo Habkost wrote:
>> On Thu, Sep 28, 2017 at 02:33:57AM -0600, Jan Beulich wrote:
>> > >>> On 27.09.17 at 21:56,  wrote:
>> > > --- a/hw/xen/xen_pt.c
>> > > +++ b/hw/xen/xen_pt.c
>> > > @@ -964,6 +964,10 @@ static const TypeInfo xen_pci_passthrough_info = {
>> > >  .instance_size = sizeof(XenPCIPassthroughState),
>> > >  .instance_finalize = xen_pci_passthrough_finalize,
>> > >  .class_init = xen_pci_passthrough_class_init,
>> > > +.interfaces = (InterfaceInfo[]) {
>> > > +{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> > > +{ },
>> > > +},
>> > >  };
>> > 
>> > Passed through devices can be both PCI and PCIe, so following
>> > the description of the patch I don't think these can be statically
>> > given either property. Granted quite a bit of PCIe specific
>> > functionality may be missing in the Xen code ...
>> 
>> This is just static data about what the device type supports, not
>> about what a given device instance really is.  Deciding if the
>> device is PCIe or Conventional at runtime is out of the scope of
>> this series.
>> 
>> That said, if passed through PCI Express devices are really
>> supported, it looks like this should be marked as hybrid.
> 
> Can anybody confirm if PCI Express devices are really supported
> by xen-pci-passthrough?

I think I've clearly said they're supported, with some limitations.

Jan

Re: [Qemu-block] [Qemu-devel] [PATCH v8 0/8] Optimize VMDK I/O by allocating multiple clusters

[Ashijeet Acharya]

On Wed, Oct 4, 2017 at 1:58 PM, Fam Zheng  wrote:

> On Wed, 10/04 13:47, Ashijeet Acharya wrote:
> > Fam: Ping?
>
> Hi Ashijeet, looks like this patch doesn't apply to current master, could
> you
> rebase and post another version?
>

Hello Fam,

I will try to do it over the weekend then and you can merge it next week
hopefully.

Ashijeet

>
> Fam
>

Re: [Qemu-block] [Qemu-devel] [PATCH v8 0/8] Optimize VMDK I/O by allocating multiple clusters

[Fam Zheng]

On Wed, 10/04 13:47, Ashijeet Acharya wrote:
> Fam: Ping?

Hi Ashijeet, looks like this patch doesn't apply to current master, could you
rebase and post another version?

Fam

Re: [Qemu-block] [PATCH v8 0/8] Optimize VMDK I/O by allocating multiple clusters

[Ashijeet Acharya]

On Thu, Aug 10, 2017 at 11:13 PM, Stefan Hajnoczi 
wrote:

> On Thu, Aug 10, 2017 at 9:18 AM, Ashijeet Acharya
>  wrote:
> > On Thu, Aug 10, 2017 at 1:41 PM, Stefan Hajnoczi 
> wrote:
> >>
> >> On Thu, Jul 27, 2017 at 3:33 PM, Ashijeet Acharya
> >>  wrote:
> >> > Previously posted series patches:
> >> > v1 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg02044.html
> >> > v2 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-03/msg05080.html
> >> > v3 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg00074.html
> >> > v4 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg03851.html
> >> > v5 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg00929.html
> >> > v6 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg00947.html
> >> > v7 -
> >> > http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg06600.html
> >> >
> >> > This series helps to optimize the I/O performance of VMDK driver.
> >> >
> >> > Patch 1 helps us to move vmdk_find_offset_in_cluster.
> >> >
> >> > Patch 2 & 3 perform a simple function re-naming tasks.
> >> >
> >> > Patch 4 is used to factor out metadata loading code and implement it
> in
> >> > separate
> >> > functions. This will help us to avoid code duplication in future
> patches
> >> > of this
> >> > series.
> >> >
> >> > Patch 5 helps to set the upper limit of the bytes handled in one
> cycle.
> >> >
> >> > Patch 6 adds new functions to help us allocate multiple clusters
> >> > according to
> >> > the size requested, perform COW if required and return the offset of
> the
> >> > first
> >> > newly allocated cluster.
> >> >
> >> > Patch 7 changes the metadata update code to update the L2 tables for
> >> > multiple
> >> > clusters at once.
> >> >
> >> > Patch 8 helps us to finally change vmdk_get_cluster_offset() to find
> >> > cluster
> >> > offset only as cluster allocation task is now handled by
> >> > vmdk_alloc_clusters()
> >> >
> >> > Optimization test results:
> >> >
> >> > This patch series improves 128 KB sequential write performance to an
> >> > empty VMDK file by 54%
> >> >
> >> > Benchmark command: ./qemu-img bench -w -c 1024 -s 128K -d 1 -t none -f
> >> > vmdk test.vmdk
> >> >
> >> > Changes in v8:
> >> > - fix minor variable naming issue in patch 6
> >>
> >> Fam: Ping?
> >>
> >> Ashijeet: Feel free to send a ping reply if no one reviews your
> >> patches within a few days.
> >
> >
> > Hi Stefan,
> >
> > I had a chat with Fam on #qemu-block before submitting this series and he
> > said he will be merging it soon when the freeze is over (I am not sure
> if it
> > is yet) since all the patches are already reviewed :-)
>
> Good to hear :).
>
> QEMU 2.10 is scheduled to be released on 22nd or 29th of August.
>
> Stefan
>

Fam: Ping?

Ashijeet