Stefan Weil <s...@weilnetz.de> writes: > Am 18.01.2013 09:53, schrieb Markus Armbruster: >> Stefan Weil <s...@weilnetz.de> writes: >>> This improves error reports for bochs, cow, qcow, qcow2, qed and vmdk >>> when a file with the wrong format is selected. >>> >>> Signed-off-by: Stefan Weil <s...@weilnetz.de> >>> --- >>> block/bochs.c | 2 +- >>> block/cow.c | 2 +- >>> block/qcow.c | 2 +- >>> block/qcow2.c | 2 +- >>> block/qed.c | 2 +- >>> block/vmdk.c | 4 ++-- >>> 6 files changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/block/bochs.c b/block/bochs.c >>> index 1b1d9cd..3737583 100644 >>> --- a/block/bochs.c >>> +++ b/block/bochs.c >>> @@ -126,7 +126,7 @@ static int bochs_open(BlockDriverState *bs, int flags) >>> strcmp(bochs.subtype, GROWING_TYPE) || >>> ((le32_to_cpu(bochs.version) != HEADER_VERSION) && >>> (le32_to_cpu(bochs.version) != HEADER_V1))) { >>> - goto fail; >>> + return -EMEDIUMTYPE; >>> } >>> >>> if (le32_to_cpu(bochs.version) == HEADER_V1) { >> You make the function return either 0, -1 or -EMEDIUMTYPE. Please make >> it return either 0 or a negative errno code, like this (untested): > > Hi Markus, > > returning 0, -1 is like before, only returning -EMEDIUMTYPE is new. > > You are right, a return value of -1 should be replaced by a negative > error value. I fixed this for block/vdi.c in a separate patch as > suggested by Kevin, see http://patchwork.ozlabs.org/patch/213375/. > > The same kind of improvement should be done for other block > drivers which currently use -1, but that can be done after my > patch series was applied. > > The primary purpose of my patch series was fixing open bugreports. > For vdi I did more because I feel responsible for that part of the > code.
I had a closer look at the various bdrv_open() methods, and how they're used. Turns out that we already assume they return 0/-errno, yet the following methods return -1 on some or all errors: bochs_open cloop_open dmg_open parallels_open vdi_open vpc_open They all need to be fixed. I appreciate you fixing vdi_open(). However, you "improve" bochs_open() from completely and obviously broken (return -1 on error always) to half-broken (return -1 on some errors, and -errno on others). I don't like that. Fixing it up doesn't look hard to me (sketch appended). Could you do that for us? If not, I'd prefer to leave bochs_open() completely and obviously broken. diff --git a/block/bochs.c b/block/bochs.c index 1b1d9cd..57b2dc8 100644 --- a/block/bochs.c +++ b/block/bochs.c @@ -111,13 +111,14 @@ static int bochs_probe(const uint8_t *buf, int buf_size, const char *filename) static int bochs_open(BlockDriverState *bs, int flags) { BDRVBochsState *s = bs->opaque; - int i; + int ret, i; struct bochs_header bochs; struct bochs_header_v1 header_v1; bs->read_only = 1; // no write support yet - if (bdrv_pread(bs->file, 0, &bochs, sizeof(bochs)) != sizeof(bochs)) { + ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs)); + if (ret < 0) { goto fail; } @@ -126,6 +127,7 @@ static int bochs_open(BlockDriverState *bs, int flags) strcmp(bochs.subtype, GROWING_TYPE) || ((le32_to_cpu(bochs.version) != HEADER_VERSION) && (le32_to_cpu(bochs.version) != HEADER_V1))) { + ret = -EMEDIUMTYPE; goto fail; } @@ -138,8 +140,9 @@ static int bochs_open(BlockDriverState *bs, int flags) s->catalog_size = le32_to_cpu(bochs.extra.redolog.catalog); s->catalog_bitmap = g_malloc(s->catalog_size * 4); - if (bdrv_pread(bs->file, le32_to_cpu(bochs.header), s->catalog_bitmap, - s->catalog_size * 4) != s->catalog_size * 4) + ret = bdrv_pread(bs->file, le32_to_cpu(bochs.header), s->catalog_bitmap, + s->catalog_size * 4); + if (ret < 0) goto fail; for (i = 0; i < s->catalog_size; i++) le32_to_cpus(&s->catalog_bitmap[i]); @@ -154,7 +157,7 @@ static int bochs_open(BlockDriverState *bs, int flags) qemu_co_mutex_init(&s->lock); return 0; fail: - return -1; + return ret; } static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)