| Message ID | 87obgire5t.fsf@blackfin.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
Am 21.01.2013 12:03, schrieb Markus Armbruster: > Stefan Weil<sw@weilnetz.de> writes: >> Am 18.01.2013 09:53, schrieb Markus Armbruster: >>> Stefan Weil<sw@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<sw@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. Kevin, Stefan, maybe you can take patches 1, 3, 4 and 5 from this series, so I don't have to resend them. I'll split patch 2 in order to realize Markus' suggestion. Or you take it as it is, resulting in half-broken (instead of full broken) xxx_open functions, and patches which fix the remaining half can be applied on top. Stefan
Stefan Weil <sw@weilnetz.de> writes: > Kevin, Stefan, maybe you can take patches 1, 3, 4 and 5 from > this series, so I don't have to resend them. > > I'll split patch 2 in order to realize Markus' suggestion. > Or you take it as it is, resulting in half-broken (instead of > full broken) xxx_open functions, and patches which fix the > remaining half can be applied on top. FYI, Kevin applied them all, and cleaned up the mess on top ([PATCH v2 0/6] bdrv_open() error return fixes).
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)