[Qemu-devel] [PATCH 0/3] blockdev: blockdev-add error path fixes and tests

[Qemu-devel] [PATCH 0/3] blockdev: blockdev-add error path fixes and tests

[Kevin Wolf]

Kevin Wolf (3):
  blockdev: Fail blockdev-add with encrypted images
  blockdev: Fix NULL pointer dereference in blockdev-add
  qemu-iotests: Test a few blockdev-add error cases

 blockdev.c                 |  15 ++++--
 tests/qemu-iotests/087     | 122 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/087.out |  40 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 175 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/087
 create mode 100644 tests/qemu-iotests/087.out

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] blockdev: Fail blockdev-add with encrypted images

[Kevin Wolf]

Encrypted images need a password before they can be used, and we don't
want blockdev-add to create BDSes that aren't fully initialised. So for
now simply forbid encrypted images; we can come back to it later if we
need the functionality.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 357f760..561cb81 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2266,6 +2266,7 @@ void qmp_block_job_complete(const char *device, Error **errp)
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
     QmpOutputVisitor *ov = qmp_output_visitor_new();
+    DriveInfo *dinfo;
     QObject *obj;
     QDict *qdict;
     Error *local_err = NULL;
@@ -2301,12 +2302,18 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     qdict_flatten(qdict);
 
-    blockdev_init(NULL, qdict, &local_err);
+    dinfo = blockdev_init(NULL, qdict, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
     }
 
+    if (bdrv_key_required(dinfo->bdrv)) {
+        drive_uninit(dinfo);
+        error_setg(errp, "blockdev-add doesn't support encrypted devices");
+        goto fail;
+    }
+
 fail:
     qmp_output_visitor_cleanup(ov);
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/3] blockdev: Fail blockdev-add with encrypted images

[Benoît Canet]

The Thursday 06 Mar 2014 à 16:44:27 (+0100), Kevin Wolf wrote :
> Encrypted images need a password before they can be used, and we don't
> want blockdev-add to create BDSes that aren't fully initialised. So for
> now simply forbid encrypted images; we can come back to it later if we
> need the functionality.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 357f760..561cb81 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2266,6 +2266,7 @@ void qmp_block_job_complete(const char *device, Error **errp)
>  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  {
>      QmpOutputVisitor *ov = qmp_output_visitor_new();
> +    DriveInfo *dinfo;
>      QObject *obj;
>      QDict *qdict;
>      Error *local_err = NULL;
> @@ -2301,12 +2302,18 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  
>      qdict_flatten(qdict);
>  
> -    blockdev_init(NULL, qdict, &local_err);
> +    dinfo = blockdev_init(NULL, qdict, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto fail;
>      }
>  
> +    if (bdrv_key_required(dinfo->bdrv)) {
> +        drive_uninit(dinfo);
> +        error_setg(errp, "blockdev-add doesn't support encrypted devices");

> +        goto fail;
This goto fail; is an extra the code will flow to fail anyway.

Best regards

Benoît

> +    }
> +
>  fail:
>      qmp_output_visitor_cleanup(ov);
>  }
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH 1/3] blockdev: Fail blockdev-add with encrypted images

[Eric Blake]

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

On 03/06/2014 09:02 AM, Benoît Canet wrote:
> The Thursday 06 Mar 2014 à 16:44:27 (+0100), Kevin Wolf wrote :
>> Encrypted images need a password before they can be used, and we don't
>> want blockdev-add to create BDSes that aren't fully initialised. So for
>> now simply forbid encrypted images; we can come back to it later if we
>> need the functionality.
>>

>> -    blockdev_init(NULL, qdict, &local_err);
>> +    dinfo = blockdev_init(NULL, qdict, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          goto fail;
>>      }
>>  
>> +    if (bdrv_key_required(dinfo->bdrv)) {
>> +        drive_uninit(dinfo);
>> +        error_setg(errp, "blockdev-add doesn't support encrypted devices");
> 
>> +        goto fail;
> This goto fail; is an extra the code will flow to fail anyway.

Ah, but notice how the above block also had the same pattern, and now
that we injected a new if clause, the earlier block is still correct.
This is defensive programming, so that future additions don't have to
mess with control flow of earlier code when adding new code later on.
I'm happy with the patch as-is:

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

-- 
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 2/3] blockdev: Fix NULL pointer dereference in blockdev-add

[Kevin Wolf]

If aio=native, we check that cache.direct is set as well. If however
cache wasn't specified at all, qemu just segfaulted.

The old condition didn't make any sense anyway because it effectively
only checked for the default cache mode case, but not for an explicitly
set cache.direct=off mode.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 561cb81..c3422a1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2283,8 +2283,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
      *
      * For now, simply forbidding the combination for all drivers will do. */
     if (options->has_aio && options->aio == BLOCKDEV_AIO_OPTIONS_NATIVE) {
-        bool direct = options->cache->has_direct && options->cache->direct;
-        if (!options->has_cache && !direct) {
+        bool direct = options->has_cache &&
+                      options->cache->has_direct &&
+                      options->cache->direct;
+        if (!direct) {
             error_setg(errp, "aio=native requires cache.direct=true");
             goto fail;
         }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/3] blockdev: Fix NULL pointer dereference in blockdev-add

[Benoît Canet]

The Thursday 06 Mar 2014 à 16:44:28 (+0100), Kevin Wolf wrote :
> If aio=native, we check that cache.direct is set as well. If however
> cache wasn't specified at all, qemu just segfaulted.
> 
> The old condition didn't make any sense anyway because it effectively
> only checked for the default cache mode case, but not for an explicitly
> set cache.direct=off mode.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 561cb81..c3422a1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2283,8 +2283,10 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>       *
>       * For now, simply forbidding the combination for all drivers will do. */
>      if (options->has_aio && options->aio == BLOCKDEV_AIO_OPTIONS_NATIVE) {
> -        bool direct = options->cache->has_direct && options->cache->direct;
> -        if (!options->has_cache && !direct) {
> +        bool direct = options->has_cache &&
> +                      options->cache->has_direct &&
> +                      options->cache->direct;
> +        if (!direct) {
>              error_setg(errp, "aio=native requires cache.direct=true");
>              goto fail;
>          }
> -- 
> 1.8.1.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 2/3] blockdev: Fix NULL pointer dereference in blockdev-add

[Eric Blake]

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

On 03/06/2014 08:44 AM, Kevin Wolf wrote:
> If aio=native, we check that cache.direct is set as well. If however
> cache wasn't specified at all, qemu just segfaulted.
> 
> The old condition didn't make any sense anyway because it effectively
> only checked for the default cache mode case, but not for an explicitly
> set cache.direct=off mode.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

-- 
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 3/3] qemu-iotests: Test a few blockdev-add error cases

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/087     | 122 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/087.out |  40 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 163 insertions(+)
 create mode 100755 tests/qemu-iotests/087
 create mode 100644 tests/qemu-iotests/087.out

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
new file mode 100755
index 0000000..53b6c43
--- /dev/null
+++ b/tests/qemu-iotests/087
@@ -0,0 +1,122 @@
+#!/bin/bash
+#
+# Test unsupported blockdev-add cases
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | sed -e 's/\("actual-size":\s*\)[0-9]\+/\1SIZE/g'
+}
+
+size=128M
+
+_make_test_img $size
+
+echo
+echo === Missing ID ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+  "arguments": {
+      "options": {
+        "driver": "$IMGFMT",
+        "file": {
+            "driver": "file",
+            "filename": "$TEST_IMG"
+        }
+      }
+    }
+  }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === aio=native without O_DIRECT ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+  "arguments": {
+      "options": {
+        "driver": "$IMGFMT",
+        "id": "disk",
+        "aio": "native",
+        "file": {
+            "driver": "file",
+            "filename": "$TEST_IMG"
+        }
+      }
+    }
+  }
+{ "execute": "quit" }
+EOF
+
+echo
+echo === Encrypted image ===
+echo
+
+_make_test_img -o encryption=on $size
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+  "arguments": {
+      "options": {
+        "driver": "$IMGFMT",
+        "id": "disk",
+        "file": {
+            "driver": "file",
+            "filename": "$TEST_IMG"
+        }
+      }
+    }
+  }
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
new file mode 100644
index 0000000..b871032
--- /dev/null
+++ b/tests/qemu-iotests/087.out
@@ -0,0 +1,40 @@
+QA output created by 087
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+
+=== Missing ID ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Block device needs an ID"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+
+=== aio=native without O_DIRECT ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "aio=native requires cache.direct=true"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+
+=== Encrypted image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on 
+Testing:
+QMP_VERSION
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "blockdev-add doesn't support encrypted devices"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 901730d..9ec62d2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -86,3 +86,4 @@
 081 rw auto
 082 rw auto quick
 086 rw auto quick
+087 rw auto quick
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test a few blockdev-add error cases

[Benoît Canet]

The Thursday 06 Mar 2014 à 16:44:29 (+0100), Kevin Wolf wrote :
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/087     | 122 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/087.out |  40 +++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 163 insertions(+)
>  create mode 100755 tests/qemu-iotests/087
>  create mode 100644 tests/qemu-iotests/087.out
> 
> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> new file mode 100755
> index 0000000..53b6c43
> --- /dev/null
> +++ b/tests/qemu-iotests/087
> @@ -0,0 +1,122 @@
> +#!/bin/bash
> +#
> +# Test unsupported blockdev-add cases
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +function do_run_qemu()
> +{
> +    echo Testing: "$@"
> +    $QEMU -nographic -qmp stdio -serial none "$@"
> +    echo
> +}
> +
> +function run_qemu()
> +{
> +    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | sed -e 's/\("actual-size":\s*\)[0-9]\+/\1SIZE/g'
> +}
> +
> +size=128M
> +
> +_make_test_img $size
> +
> +echo
> +echo === Missing ID ===
> +echo
> +
> +run_qemu <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> +  "arguments": {
> +      "options": {
> +        "driver": "$IMGFMT",
> +        "file": {
> +            "driver": "file",
> +            "filename": "$TEST_IMG"
> +        }
> +      }
> +    }
> +  }
> +{ "execute": "quit" }
> +EOF
> +
> +echo
> +echo === aio=native without O_DIRECT ===
> +echo
> +
> +run_qemu <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> +  "arguments": {
> +      "options": {
> +        "driver": "$IMGFMT",
> +        "id": "disk",
> +        "aio": "native",
> +        "file": {
> +            "driver": "file",
> +            "filename": "$TEST_IMG"
> +        }
> +      }
> +    }
> +  }
> +{ "execute": "quit" }
> +EOF
> +
> +echo
> +echo === Encrypted image ===
> +echo
> +
> +_make_test_img -o encryption=on $size
> +run_qemu <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> +  "arguments": {
> +      "options": {
> +        "driver": "$IMGFMT",
> +        "id": "disk",
> +        "file": {
> +            "driver": "file",
> +            "filename": "$TEST_IMG"
> +        }
> +      }
> +    }
> +  }
> +{ "execute": "quit" }
> +EOF
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> new file mode 100644
> index 0000000..b871032
> --- /dev/null
> +++ b/tests/qemu-iotests/087.out
> @@ -0,0 +1,40 @@
> +QA output created by 087
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
> +
> +=== Missing ID ===
> +
> +Testing:
> +QMP_VERSION
> +{"return": {}}
> +{"error": {"class": "GenericError", "desc": "Block device needs an ID"}}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
> +
> +
> +=== aio=native without O_DIRECT ===
> +
> +Testing:
> +QMP_VERSION
> +{"return": {}}
> +{"error": {"class": "GenericError", "desc": "aio=native requires cache.direct=true"}}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
> +
> +
> +=== Encrypted image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on 
> +Testing:
> +QMP_VERSION
> +{"return": {}}
> +{"error": {"class": "GenericError", "desc": "blockdev-add doesn't support encrypted devices"}}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
> +
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 901730d..9ec62d2 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -86,3 +86,4 @@
>  081 rw auto
>  082 rw auto quick
>  086 rw auto quick
> +087 rw auto quick
> -- 
> 1.8.1.4
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>