Re: [Qemu-devel] [PATCH 1/2] block: Add Error argument to bdrv_refresh_limits()

[Kevin Wolf]

Am 16.07.2014 um 20:07 hat Eric Blake geschrieben:
> On 07/16/2014 09:48 AM, Kevin Wolf wrote:
> > @@ -1154,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> > BlockDriverState *backing_hd)
> >      bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
> >                      bs->backing_blocker);
> >  out:
> > -    bdrv_refresh_limits(bs);
> > +    bdrv_refresh_limits(bs, NULL);
> >  }
> >  
> >  /*
> > @@ -1778,7 +1791,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
> >                                                BDRV_O_CACHE_WB);
> >      reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
> >  
> > -    bdrv_refresh_limits(reopen_state->bs);
> > +    bdrv_refresh_limits(reopen_state->bs, NULL);
> 
> > +++ b/block/stream.c
> > @@ -76,7 +76,7 @@ static void close_unused_images(BlockDriverState *top, 
> > BlockDriverState *base,
> >          bdrv_unref(unused);
> >      }
> >  
> > -    bdrv_refresh_limits(top);
> > +    bdrv_refresh_limits(top, NULL);
> >  }
> >  
> 
> Should these three callers be concerned about failure?  If so, would
> &error_abort be better than NULL?  But as for this patch, you are
> preserving existing semantics, so you could save it for a later patch.

They probably should, but I couldn't figure out what they should to on
failure. Aborting qemu because a single block device fails isn't nice.
&error_abort has similar semantics as assert() for me ("This can't ever
happen"), and this is definitely not the case here as I/O errors can
happen anytime.

If anything, it's similar to a qcow2 image that has detected corruption
and therefore made the BDS unusable. But setting bs->drv = NULL within a
single block driver is already tricky (and so it was buggy at first),
doing it in block.c with BDSes of any driver sounds even worse.

This is the reason why I kept NULL here, even if it's not completely
right.

Kevin

pgpymxHyAeJ5Y.pgp
Description: PGP signature