[Qemu-devel] [PATCH V2 0/3] Fix some quorum nits after merge

[Qemu-devel] [PATCH V2 0/3] Fix some quorum nits after merge

[Benoît Canet]

in v2:

    make error optional and return strerror(-ret) [Eric]
    better documentation of the error string [Eric]
    apply Eric reviewed by

Benoît Canet (3):
  qmp: Fix BlockdevOptionQuorum.
  qmp: Make Quorum error events more palatable.
  qemu-io-test: Disable Quorum test when not compiled in.

 block/quorum.c             |  9 ++++--
 docs/qmp/qmp-events.txt    | 75 ++++++++++++++++++++++++----------------------
 qapi-schema.json           |  5 ++--
 tests/qemu-iotests/081     |  3 ++
 tests/qemu-iotests/081.out |  2 +-
 5 files changed, 52 insertions(+), 42 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH V2 1/3] qmp: Fix BlockdevOptionQuorum.

[Benoît Canet]

Fix some nits before QEMU 2.0 freeze.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi-schema.json | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index fcb22800..ab5cdde 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4437,10 +4437,11 @@
 # Driver specific block device options for Quorum
 #
 # @blkverify:      #optional true if the driver must print content mismatch
+#                  set to false by default
 #
-# @children:       the children block device to use
+# @children:       the children block devices to use
 #
-# @vote_threshold: the vote limit under which a read will fail
+# @vote-threshold: the vote limit under which a read will fail
 #
 # Since: 2.0
 ##
-- 
1.8.3.2

[Qemu-devel] [PATCH V2 2/3] qmp: Make Quorum error events more palatable.

[Benoît Canet]

Insert quorum QMP events documentation alphabetically.

Also change the "ret" errno value by an optional "error" being an strerror(-ret)
in the QUORUM_REPORT_BAD qmp event.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block/quorum.c             |  9 ++++--
 docs/qmp/qmp-events.txt    | 75 ++++++++++++++++++++++++----------------------
 tests/qemu-iotests/081.out |  2 +-
 3 files changed, 46 insertions(+), 40 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 6c28239..bd997b7 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -200,11 +200,14 @@ static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
 {
     QObject *data;
     assert(node_name);
-    data = qobject_from_jsonf("{ 'ret': %d"
-                              ", 'node-name': %s"
+    data = qobject_from_jsonf("{ 'node-name': %s"
                               ", 'sector-num': %" PRId64
                               ", 'sectors-count': %d }",
-                              ret, node_name, acb->sector_num, acb->nb_sectors);
+                              node_name, acb->sector_num, acb->nb_sectors);
+    if (ret < 0) {
+        QDict *dict = qobject_to_qdict(data);
+        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
+    }
     monitor_protocol_event(QEVENT_QUORUM_REPORT_BAD, data);
     qobject_decref(data);
 }
diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index 00f9515..145402e 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -225,6 +225,45 @@ Data:
   "timestamp": { "seconds": 1368697518, "microseconds": 326866 } }
 }
 
+QUORUM_FAILURE
+--------------
+
+Emitted by the Quorum block driver if it fails to establish a quorum.
+
+Data:
+
+- "reference":    device name if defined else node name.
+- "sector-num":   Number of the first sector of the failed read operation.
+- "sector-count": Failed read operation sector count.
+
+Example:
+
+{ "event": "QUORUM_FAILURE",
+     "data": { "reference": "usr1", "sector-num": 345435, "sector-count": 5 },
+     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
+
+QUORUM_REPORT_BAD
+-----------------
+
+Emitted to report a corruption of a Quorum file.
+
+Data:
+
+- "error":        Error message (json-string, optional)
+                  Only present on failure.  This field contains a human-readable
+                  error message.  There are no semantics other than that the
+                  block layer reported an error and clients should not try to
+                  interpret the error string.
+- "node-name":    The graph node name of the block driver state.
+- "sector-num":   Number of the first sector of the failed read operation.
+- "sector-count": Failed read operation sector count.
+
+Example:
+
+{ "event": "QUORUM_REPORT_BAD",
+     "data": { "node-name": "1.raw", "sector-num": 345435, "sector-count": 5 },
+     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
+
 RESET
 -----
 
@@ -500,39 +539,3 @@ Example:
 
 Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
 followed respectively by the RESET, SHUTDOWN, or STOP events.
-
-QUORUM_FAILURE
---------------
-
-Emitted by the Quorum block driver if it fails to establish a quorum.
-
-Data:
-
-- "reference":    device name if defined else node name.
-- "sector-num":   Number of the first sector of the failed read operation.
-- "sector-count": Failed read operation sector count.
-
-Example:
-
-{ "event": "QUORUM_FAILURE",
-     "data": { "reference": "usr1", "sector-num": 345435, "sector-count": 5 },
-     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
-
-QUORUM_REPORT_BAD
------------------
-
-Emitted to report a corruption of a Quorum file.
-
-Data:
-
-- "ret":          The IO return code.
-- "node-name":    The graph node name of the block driver state.
-- "sector-num":   Number of the first sector of the failed read operation.
-- "sector-count": Failed read operation sector count.
-
-Example:
-
-{ "event": "QUORUM_REPORT_BAD",
-     "data": { "ret": 0, "node-name": "1.raw", "sector-num": 345435,
-               "sector-count": 5 },
-     "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index 4fe2f95..84aeb0c 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -30,7 +30,7 @@ Testing: -drive file=TEST_DIR/2.IMGFMT,format=IMGFMT,if=none,id=drive2
 QMP_VERSION
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "", "ret": 0, "sectors-count": 20480, "sector-num": 0}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "", "sectors-count": 20480, "sector-num": 0}}
 read 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": ""}
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH V2 2/3] qmp: Make Quorum error events more palatable.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

On 02/21/2014 06:20 PM, Benoît Canet wrote:
> Insert quorum QMP events documentation alphabetically.
> 
> Also change the "ret" errno value by an optional "error" being an strerror(-ret)
> in the QUORUM_REPORT_BAD qmp event.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---

>      assert(node_name);
> -    data = qobject_from_jsonf("{ 'ret': %d"
> -                              ", 'node-name': %s"
> +    data = qobject_from_jsonf("{ 'node-name': %s"
>                                ", 'sector-num': %" PRId64
>                                ", 'sectors-count': %d }",
> -                              ret, node_name, acb->sector_num, acb->nb_sectors);
> +                              node_name, acb->sector_num, acb->nb_sectors);
> +    if (ret < 0) {
> +        QDict *dict = qobject_to_qdict(data);
> +        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
> +    }

This puts it in dict, but never modifies data...

>      monitor_protocol_event(QEVENT_QUORUM_REPORT_BAD, data);

...but you send data, not dict, over the wire.

It might be easier to do something like...

char *str = NULL;
if (ret < 0)
    str = strerror(-ret);
data = qobject_from_jsonf("{ 'node-name': %s"
                          "%s%s%s"
                          ", 'sector-num': %" PRId64
                          ", 'sectors-cont': %d }",
                          node_name,
                          str ? ", 'error': \"" : "", str ? str : "",
                          str ? "\"" : "",
...);

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V2 2/3] qmp: Make Quorum error events more palatable.

[Benoît Canet]

The Friday 21 Feb 2014 à 18:42:01 (-0700), Eric Blake wrote :
> On 02/21/2014 06:20 PM, Benoît Canet wrote:
> > Insert quorum QMP events documentation alphabetically.
> >
> > Also change the "ret" errno value by an optional "error" being an strerror(-ret)
> > in the QUORUM_REPORT_BAD qmp event.
> >
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
>
> >      assert(node_name);
> > -    data = qobject_from_jsonf("{ 'ret': %d"
> > -                              ", 'node-name': %s"
> > +    data = qobject_from_jsonf("{ 'node-name': %s"
> >                                ", 'sector-num': %" PRId64
> >                                ", 'sectors-count': %d }",
> > -                              ret, node_name, acb->sector_num, acb->nb_sectors);
> > +                              node_name, acb->sector_num, acb->nb_sectors);
> > +    if (ret < 0) {
> > +        QDict *dict = qobject_to_qdict(data);
> > +        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
> > +    }
>
> This puts it in dict, but never modifies data...

qobject_to_qdict is an enhanced cast.

/**
 * qobject_to_qdict(): Convert a QObject into a QDict
 */
QDict *qobject_to_qdict(const QObject *obj)
{
    if (qobject_type(obj) != QTYPE_QDICT)
        return NULL;

    return container_of(obj, QDict, base);
}

Also I took this snippet of code from what you pointed me as an example for the
error handling.

Best regards

Benoît

>
> >      monitor_protocol_event(QEVENT_QUORUM_REPORT_BAD, data);
>
> ...but you send data, not dict, over the wire.
>
> It might be easier to do something like...
>
> char *str = NULL;
> if (ret < 0)
>     str = strerror(-ret);
> data = qobject_from_jsonf("{ 'node-name': %s"
>                           "%s%s%s"
>                           ", 'sector-num': %" PRId64
>                           ", 'sectors-cont': %d }",
>                           node_name,
>                           str ? ", 'error': \"" : "", str ? str : "",
>                           str ? "\"" : "",
> ...);
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH V2 2/3] qmp: Make Quorum error events more palatable.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On 02/21/2014 07:09 PM, Benoît Canet wrote:

>>> +    if (ret < 0) {
>>> +        QDict *dict = qobject_to_qdict(data);
>>> +        qdict_put(dict, "error", qstring_from_str(strerror(-ret)));
>>> +    }
>>
>> This puts it in dict, but never modifies data...
> 
> qobject_to_qdict is an enhanced cast.
> 
> /**
>  * qobject_to_qdict(): Convert a QObject into a QDict
>  */
> QDict *qobject_to_qdict(const QObject *obj)
> {
>     if (qobject_type(obj) != QTYPE_QDICT)
>         return NULL;
> 
>     return container_of(obj, QDict, base);
> }
> 
> Also I took this snippet of code from what you pointed me as an example for the
> error handling.

Oh - now I see.  Slick.  In that case,

Reviewed-by: Eric Blake <eblake@redhat.com>

I'll let Kevin review 3/3.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH V2 3/3] qemu-io-test: Disable Quorum test when not compiled in.

[Benoît Canet]

Quorum is not compiled by default: make the quorum 081 test aware of this.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 tests/qemu-iotests/081 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index f053f11..c1078f6 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -56,6 +56,9 @@ function run_qemu()
     do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io
 }
 
+test_quorum=$(qemu-img --help|grep quorum)
+[ "$test_quorum" = "" ] && _supported_fmt quorum
+
 quorum="file.driver=quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
 quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw"
 quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw,file.vote-threshold=2"
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH V2 3/3] qemu-io-test: Disable Quorum test when not compiled in.

[Fam Zheng]

On Sat, 02/22 02:20, Benoît Canet wrote:
> Quorum is not compiled by default: make the quorum 081 test aware of this.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  tests/qemu-iotests/081 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
> index f053f11..c1078f6 100755
> --- a/tests/qemu-iotests/081
> +++ b/tests/qemu-iotests/081
> @@ -56,6 +56,9 @@ function run_qemu()
>      do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io
>  }
>  
> +test_quorum=$(qemu-img --help|grep quorum)
> +[ "$test_quorum" = "" ] && _supported_fmt quorum
> +

How does this disable the test? When quorum is not compiled, the two lines are
no-op. Did you test it?

qemu-img should be $QEMU_IMG.

I think you mean this:

    $QEMU_IMG --help | grep -q quorum && _not_run "quorum is not compiled"

Thanks,
Fam

Re: [Qemu-devel] [PATCH V2 3/3] qemu-io-test: Disable Quorum test when not compiled in.

[Benoît Canet]

The Saturday 22 Feb 2014 à 15:38:15 (+0800), Fam Zheng wrote :
> On Sat, 02/22 02:20, Benoît Canet wrote:
> > Quorum is not compiled by default: make the quorum 081 test aware of this.
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  tests/qemu-iotests/081 | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
> > index f053f11..c1078f6 100755
> > --- a/tests/qemu-iotests/081
> > +++ b/tests/qemu-iotests/081
> > @@ -56,6 +56,9 @@ function run_qemu()
> >      do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io
> >  }
> >  
> > +test_quorum=$(qemu-img --help|grep quorum)
> > +[ "$test_quorum" = "" ] && _supported_fmt quorum
> > +
> 
> How does this disable the test? When quorum is not compiled, the two lines are
> no-op. Did you test it?

Yes I did test it. It works.

> 
> qemu-img should be $QEMU_IMG.
> 
> I think you mean this:
> 
>     $QEMU_IMG --help | grep -q quorum && _not_run "quorum is not compiled"
> 
> Thanks,
> Fam

Re: [Qemu-devel] [PATCH V2 3/3] qemu-io-test: Disable Quorum test when not compiled in.

[Fam Zheng]

On Sat, 02/22 12:58, Benoît Canet wrote:
> The Saturday 22 Feb 2014 à 15:38:15 (+0800), Fam Zheng wrote :
> > On Sat, 02/22 02:20, Benoît Canet wrote:
> > > Quorum is not compiled by default: make the quorum 081 test aware of this.
> > > 
> > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > ---
> > >  tests/qemu-iotests/081 | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
> > > index f053f11..c1078f6 100755
> > > --- a/tests/qemu-iotests/081
> > > +++ b/tests/qemu-iotests/081
> > > @@ -56,6 +56,9 @@ function run_qemu()
> > >      do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io
> > >  }
> > >  
> > > +test_quorum=$(qemu-img --help|grep quorum)
> > > +[ "$test_quorum" = "" ] && _supported_fmt quorum
> > > +
> > 
> > How does this disable the test? When quorum is not compiled, the two lines are
> > no-op. Did you test it?
> 
> Yes I did test it. It works.
> 

Yes, I read it wrong, it should overwrite raw with quorum, so it works.

Still, qemu-img should be written $QEMU_IMG.

Thanks,
Fam