Re: [Qemu-devel] [PATCH v2 2/5] block: Use error code EMEDIUMTYPE for wrong format in some block drivers

[Stefan Weil]

Am 21.01.2013 12:03, schrieb Markus Armbruster:
> Stefan Weil<address@hidden>  writes:
> > Am 18.01.2013 09:53, schrieb Markus Armbruster:
> > > Stefan Weil<address@hidden>  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<address@hidden>
> > > > ---
> > > >   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