QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success
@ 2019-08-23 18:47 Max Reitz
  2019-08-23 18:47 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Max Reitz @ 2019-08-23 18:47 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, John Snow, qemu-devel, Max Reitz

Jobs are expected to return 0 on success.  .bdrv_co_create() on the
other hand is a block layer function, and as such returns a non-negative
value on success.

blockdev_create_run() should translate between the two (patch 1).

Without patch 1, blockdev-create is likely to fail for VPC images.
Hence patch 2.


Max Reitz (2):
  block: Let blockdev-create return 0 on success
  iotests: Test blockdev-create for vpc

 block/create.c             |   3 +-
 tests/qemu-iotests/266     | 182 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/266.out | 107 ++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/266
 create mode 100644 tests/qemu-iotests/266.out

-- 
2.21.0



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 1/2] block: Let blockdev-create return 0 on success
  2019-08-23 18:47 [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success Max Reitz
@ 2019-08-23 18:47 ` " Max Reitz
  2019-08-23 18:47 ` [Qemu-devel] [PATCH 2/2] iotests: Test blockdev-create for vpc Max Reitz
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Max Reitz @ 2019-08-23 18:47 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, John Snow, qemu-devel, Max Reitz

Block drivers should let their .bdrv_co_create() implementation return a
non-negative value to indicate success.  However, jobs should return
exactly 0.  Thus, we need to translate positive return values to 0 in
blockdev_create_run().

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/create.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/create.c b/block/create.c
index 1bd00ed5f8..4b23672e1b 100644
--- a/block/create.c
+++ b/block/create.c
@@ -48,7 +48,8 @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
 
     qapi_free_BlockdevCreateOptions(s->opts);
 
-    return ret;
+    /* Jobs must return 0 to indicate success */
+    return ret < 0 ? ret : 0;
 }
 
 static const JobDriver blockdev_create_job_driver = {
-- 
2.21.0



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 2/2] iotests: Test blockdev-create for vpc
  2019-08-23 18:47 [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success Max Reitz
  2019-08-23 18:47 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
@ 2019-08-23 18:47 ` Max Reitz
  2019-08-27 21:15   ` John Snow
  2019-08-27 21:15 ` [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success John Snow
  2019-09-02 13:32 ` Kevin Wolf
  3 siblings, 1 reply; 6+ messages in thread
From: Max Reitz @ 2019-08-23 18:47 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, John Snow, qemu-devel, Max Reitz

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/266     | 182 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/266.out | 107 ++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 290 insertions(+)
 create mode 100755 tests/qemu-iotests/266
 create mode 100644 tests/qemu-iotests/266.out

diff --git a/tests/qemu-iotests/266 b/tests/qemu-iotests/266
new file mode 100755
index 0000000000..19b7b29535
--- /dev/null
+++ b/tests/qemu-iotests/266
@@ -0,0 +1,182 @@
+#!/usr/bin/env python
+#
+# Test VPC and file image creation
+#
+# Copyright (C) 2019 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/>.
+#
+
+import iotests
+from iotests import imgfmt
+
+iotests.verify_image_format(supported_fmts=['vpc'])
+iotests.verify_protocol(supported=['file'])
+
+def blockdev_create(vm, options):
+    result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
+                        filters=[iotests.filter_qmp_testfiles])
+
+    if 'return' in result:
+        assert result['return'] == {}
+        vm.run_job('job0')
+    iotests.log("")
+
+with iotests.FilePath('t.vpc') as disk_path, \
+     iotests.VM() as vm:
+
+    #
+    # Successful image creation (defaults)
+    #
+    iotests.log("=== Successful image creation (defaults) ===")
+    iotests.log("")
+
+    # 8 heads, 964 cyls/head, 17 secs/cyl
+    # (Close to 64 MB)
+    size = 8 * 964 * 17 * 512
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'imgfile',
+                          'size': size })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Successful image creation (explicit defaults)
+    #
+    iotests.log("=== Successful image creation (explicit defaults) ===")
+    iotests.log("")
+
+    # 16 heads, 964 cyls/head, 17 secs/cyl
+    # (Close to 128 MB)
+    size = 16 * 964 * 17 * 512
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'imgfile',
+                          'size': size,
+                          'subformat': 'dynamic',
+                          'force-size': False })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Successful image creation (non-default options)
+    #
+    iotests.log("=== Successful image creation (non-default options) ===")
+    iotests.log("")
+
+    # Not representable in CHS (fine with force-size=True)
+    size = 1048576
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'imgfile',
+                          'size': size,
+                          'subformat': 'fixed',
+                          'force-size': True })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Size not representable in CHS with force-size=False
+    #
+    iotests.log("=== Size not representable in CHS ===")
+    iotests.log("")
+
+    # Not representable in CHS (will not work with force-size=False)
+    size = 1048576
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': 'file',
+                          'filename': disk_path,
+                          'size': 0 })
+
+    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
+
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'imgfile',
+                          'size': size,
+                          'force-size': False })
+    vm.shutdown()
+
+    #
+    # Zero size
+    #
+    iotests.log("=== Zero size===")
+    iotests.log("")
+
+    vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
+    vm.launch()
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'node0',
+                          'size': 0 })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Maximum CHS size
+    #
+    iotests.log("=== Maximum CHS size===")
+    iotests.log("")
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'node0',
+                          'size': 16 * 65535 * 255 * 512 })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
+
+    #
+    # Actual maximum size
+    #
+    iotests.log("=== Actual maximum size===")
+    iotests.log("")
+
+    vm.launch()
+    blockdev_create(vm, { 'driver': imgfmt,
+                          'file': 'node0',
+                          'size': 0xff000000 * 512,
+                          'force-size': True })
+    vm.shutdown()
+
+    iotests.img_info_log(disk_path)
diff --git a/tests/qemu-iotests/266.out b/tests/qemu-iotests/266.out
new file mode 100644
index 0000000000..43244a4574
--- /dev/null
+++ b/tests/qemu-iotests/266.out
@@ -0,0 +1,107 @@
+=== Successful image creation (defaults) ===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
+{"return": {}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "size": 67125248}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 64 MiB (67125248 bytes)
+cluster_size: 2097152
+
+=== Successful image creation (explicit defaults) ===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
+{"return": {}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "force-size": false, "size": 134250496, "subformat": "dynamic"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 128 MiB (134250496 bytes)
+cluster_size: 2097152
+
+=== Successful image creation (non-default options) ===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
+{"return": {}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "force-size": true, "size": 1048576, "subformat": "fixed"}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1 MiB (1048576 bytes)
+
+=== Size not representable in CHS ===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
+{"return": {}}
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "force-size": false, "size": 1048576}}}
+{"return": {}}
+Job failed: The requested image size cannot be represented in CHS geometry
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+=== Zero size===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "node0", "size": 0}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 0 B (0 bytes)
+cluster_size: 2097152
+
+=== Maximum CHS size===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "node0", "size": 136899993600}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 127 GiB (136899993600 bytes)
+cluster_size: 2097152
+
+=== Actual maximum size===
+
+{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "node0", "force-size": true, "size": 2190433320960}}}
+{"return": {}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.99 TiB (2190433320960 bytes)
+cluster_size: 2097152
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d95d556414..8b96456278 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -274,3 +274,4 @@
 257 rw
 258 rw quick
 262 rw quick migration
+266 rw quick
-- 
2.21.0



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] iotests: Test blockdev-create for vpc
  2019-08-23 18:47 ` [Qemu-devel] [PATCH 2/2] iotests: Test blockdev-create for vpc Max Reitz
@ 2019-08-27 21:15   ` John Snow
  0 siblings, 0 replies; 6+ messages in thread
From: John Snow @ 2019-08-27 21:15 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel



On 8/23/19 2:47 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/266     | 182 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/266.out | 107 ++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 290 insertions(+)
>  create mode 100755 tests/qemu-iotests/266
>  create mode 100644 tests/qemu-iotests/266.out
> 
> diff --git a/tests/qemu-iotests/266 b/tests/qemu-iotests/266
> new file mode 100755
> index 0000000000..19b7b29535
> --- /dev/null
> +++ b/tests/qemu-iotests/266
> @@ -0,0 +1,182 @@
> +#!/usr/bin/env python
> +#
> +# Test VPC and file image creation
> +#
> +# Copyright (C) 2019 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/>.
> +#
> +
> +import iotests
> +from iotests import imgfmt
> +
> +iotests.verify_image_format(supported_fmts=['vpc'])
> +iotests.verify_protocol(supported=['file'])
> +

Oh, I guess I haven't successfully lobbied for the inclusion of the
other thing yet. oh-kay.

> +def blockdev_create(vm, options):
> +    result = vm.qmp_log('blockdev-create', job_id='job0', options=options,
> +                        filters=[iotests.filter_qmp_testfiles])
> +
> +    if 'return' in result:
> +        assert result['return'] == {}
> +        vm.run_job('job0')
> +    iotests.log("")
> +

Probably approaching the time when we want a standard for this in
iotests, but I'm not insisting today.

> +with iotests.FilePath('t.vpc') as disk_path, \
> +     iotests.VM() as vm:
> +
> +    #
> +    # Successful image creation (defaults)
> +    #
> +    iotests.log("=== Successful image creation (defaults) ===")
> +    iotests.log("")
> +
> +    # 8 heads, 964 cyls/head, 17 secs/cyl
> +    # (Close to 64 MB)
> +    size = 8 * 964 * 17 * 512
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': 'file',
> +                          'filename': disk_path,
> +                          'size': 0 })
> +
> +    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> +               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
> +
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'imgfile',
> +                          'size': size })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> +
> +    #
> +    # Successful image creation (explicit defaults)
> +    #
> +    iotests.log("=== Successful image creation (explicit defaults) ===")
> +    iotests.log("")
> +
> +    # 16 heads, 964 cyls/head, 17 secs/cyl
> +    # (Close to 128 MB)
> +    size = 16 * 964 * 17 * 512
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': 'file',
> +                          'filename': disk_path,
> +                          'size': 0 })
> +
> +    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> +               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
> +
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'imgfile',
> +                          'size': size,
> +                          'subformat': 'dynamic',
> +                          'force-size': False })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> +
> +    #
> +    # Successful image creation (non-default options)
> +    #
> +    iotests.log("=== Successful image creation (non-default options) ===")
> +    iotests.log("")
> +
> +    # Not representable in CHS (fine with force-size=True)
> +    size = 1048576
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': 'file',
> +                          'filename': disk_path,
> +                          'size': 0 })
> +
> +    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> +               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
> +
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'imgfile',
> +                          'size': size,
> +                          'subformat': 'fixed',
> +                          'force-size': True })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> +
> +    #
> +    # Size not representable in CHS with force-size=False
> +    #
> +    iotests.log("=== Size not representable in CHS ===")
> +    iotests.log("")
> +
> +    # Not representable in CHS (will not work with force-size=False)
> +    size = 1048576
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': 'file',
> +                          'filename': disk_path,
> +                          'size': 0 })
> +
> +    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
> +               node_name='imgfile', filters=[iotests.filter_qmp_testfiles])
> +
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'imgfile',
> +                          'size': size,
> +                          'force-size': False })
> +    vm.shutdown()
> +
> +    #
> +    # Zero size
> +    #
> +    iotests.log("=== Zero size===")
> +    iotests.log("")
> +
> +    vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path))
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 0 })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> +
> +    #
> +    # Maximum CHS size
> +    #
> +    iotests.log("=== Maximum CHS size===")
> +    iotests.log("")
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 16 * 65535 * 255 * 512 })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> +
> +    #
> +    # Actual maximum size
> +    #
> +    iotests.log("=== Actual maximum size===")
> +    iotests.log("")
> +
> +    vm.launch()
> +    blockdev_create(vm, { 'driver': imgfmt,
> +                          'file': 'node0',
> +                          'size': 0xff000000 * 512,
> +                          'force-size': True })
> +    vm.shutdown()
> +
> +    iotests.img_info_log(disk_path)
> diff --git a/tests/qemu-iotests/266.out b/tests/qemu-iotests/266.out
> new file mode 100644
> index 0000000000..43244a4574
> --- /dev/null
> +++ b/tests/qemu-iotests/266.out
> @@ -0,0 +1,107 @@
> +=== Successful image creation (defaults) ===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
> +{"return": {}}
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "size": 67125248}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 64 MiB (67125248 bytes)
> +cluster_size: 2097152
> +
> +=== Successful image creation (explicit defaults) ===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
> +{"return": {}}
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "force-size": false, "size": 134250496, "subformat": "dynamic"}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 128 MiB (134250496 bytes)
> +cluster_size: 2097152
> +
> +=== Successful image creation (non-default options) ===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
> +{"return": {}}
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "force-size": true, "size": 1048576, "subformat": "fixed"}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 1 MiB (1048576 bytes)
> +
> +=== Size not representable in CHS ===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "size": 0}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +{"execute": "blockdev-add", "arguments": {"driver": "file", "filename": "TEST_DIR/PID-t.vpc", "node-name": "imgfile"}}
> +{"return": {}}
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "imgfile", "force-size": false, "size": 1048576}}}
> +{"return": {}}
> +Job failed: The requested image size cannot be represented in CHS geometry
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +=== Zero size===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "node0", "size": 0}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 0 B (0 bytes)
> +cluster_size: 2097152
> +
> +=== Maximum CHS size===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "node0", "size": 136899993600}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 127 GiB (136899993600 bytes)
> +cluster_size: 2097152
> +
> +=== Actual maximum size===
> +
> +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vpc", "file": "node0", "force-size": true, "size": 2190433320960}}}
> +{"return": {}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 1.99 TiB (2190433320960 bytes)
> +cluster_size: 2097152
> +
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index d95d556414..8b96456278 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -274,3 +274,4 @@
>  257 rw
>  258 rw quick
>  262 rw quick migration
> +266 rw quick
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success
  2019-08-23 18:47 [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success Max Reitz
  2019-08-23 18:47 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
  2019-08-23 18:47 ` [Qemu-devel] [PATCH 2/2] iotests: Test blockdev-create for vpc Max Reitz
@ 2019-08-27 21:15 ` John Snow
  2019-09-02 13:32 ` Kevin Wolf
  3 siblings, 0 replies; 6+ messages in thread
From: John Snow @ 2019-08-27 21:15 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel



On 8/23/19 2:47 PM, Max Reitz wrote:
> Jobs are expected to return 0 on success.  .bdrv_co_create() on the
> other hand is a block layer function, and as such returns a non-negative
> value on success.
> 
> blockdev_create_run() should translate between the two (patch 1).
> 
> Without patch 1, blockdev-create is likely to fail for VPC images.
> Hence patch 2.
> 
> 
> Max Reitz (2):
>   block: Let blockdev-create return 0 on success
>   iotests: Test blockdev-create for vpc
> 
>  block/create.c             |   3 +-
>  tests/qemu-iotests/266     | 182 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/266.out | 107 ++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  4 files changed, 292 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/266
>  create mode 100644 tests/qemu-iotests/266.out
> 

Reviewed-by: John Snow <jsnow@redhat.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success
  2019-08-23 18:47 [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success Max Reitz
                   ` (2 preceding siblings ...)
  2019-08-27 21:15 ` [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success John Snow
@ 2019-09-02 13:32 ` Kevin Wolf
  3 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2019-09-02 13:32 UTC (permalink / raw)
  To: Max Reitz; +Cc: John Snow, qemu-devel, qemu-block

Am 23.08.2019 um 20:47 hat Max Reitz geschrieben:
> Jobs are expected to return 0 on success.  .bdrv_co_create() on the
> other hand is a block layer function, and as such returns a
> non-negative value on success.

I don't agree that >= 0 is the block layer way. The block layer uses
0/-errno for the largest part of its interfaces; and I think the
BlockDriver callbacks might even be consistent in this. Of course, we
never documented this anywhere, maybe we should...

The only historical exceptions I'm aware of are blk/bdrv_pread/pwrite(),
which return the byte count instead of 0. They should be fixed
eventually, but it just never seemed important enough, even though it
did cause bugs every now and then.

> blockdev_create_run() should translate between the two (patch 1).
> 
> Without patch 1, blockdev-create is likely to fail for VPC images.
> Hence patch 2.

I'd argue this is a VPC bug. In the success path, it shouldn't return
ret as it happens to be at the end (it comes from bdrv_pwrite()), but
set it to 0 right before the 'fail:' label.

This is really a regression Jeff introduced in commit fef6070eff2,
though the bug was only latent then (five years ago).

Kevin


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 18:47 [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success Max Reitz
2019-08-23 18:47 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
2019-08-23 18:47 ` [Qemu-devel] [PATCH 2/2] iotests: Test blockdev-create for vpc Max Reitz
2019-08-27 21:15   ` John Snow
2019-08-27 21:15 ` [Qemu-devel] [PATCH 0/2] block: Let blockdev-create return 0 on success John Snow
2019-09-02 13:32 ` Kevin Wolf

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox