| Message ID | 20190823184733.18929-1-mreitz@redhat.com |
|---|---|
| Headers | show |
| Series | block: Let blockdev-create return 0 on success | expand |
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>
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