Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 01/16] block: Add PreallocMode to BD.bdrv_truncate()

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

On Mon, Mar 20, 2017 at 04:07:16PM +0100, Max Reitz wrote:
> On 20.03.2017 11:18, Stefan Hajnoczi wrote:
> > On Mon, Mar 13, 2017 at 10:39:46PM +0100, Max Reitz wrote:
> >> diff --git a/block/iscsi.c b/block/iscsi.c
> >> index ab559a6f71..5d6265c4a6 100644
> >> --- a/block/iscsi.c
> >> +++ b/block/iscsi.c
> >> @@ -2060,11 +2060,16 @@ static void iscsi_reopen_commit(BDRVReopenState *reopen_state)
> >>      }
> >>  }
> >>  
> >> -static int iscsi_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
> >> +static int iscsi_truncate(BlockDriverState *bs, int64_t offset,
> >> +                          PreallocMode prealloc, Error **errp)
> >>  {
> >>      IscsiLun *iscsilun = bs->opaque;
> >>      Error *local_err = NULL;
> >>  
> >> +    if (prealloc != PREALLOC_MODE_OFF) {
> >> +        return -ENOTSUP;
> >> +    }
> >> +
> >>      if (iscsilun->type != TYPE_DISK) {
> >>          return -ENOTSUP;
> >>      }
> > 
> > Nevermind what I said about adding a BiteSizedTasks entry:
> > 
> > The missing errp usage is not in qemu.git/master yet.  Please fix up
> > your bdrv_truncate() errp patch to use errp in all cases, e.g.
> > error_setg("Unable to truncate non-disk LUN").
> 
> The thing is that I wasn't comfortable doing that for all block drivers.
> I mean, I can take another look but I'd rather have vague error messages
> ("truncation failed: #{strerror}") than outright wrong ones because I
> didn't know what error message to use.
> 
> Of course you could argue that this may probably come out during review
> but that implies that every submaintainer for every block driver would
> actually come out for review...

I'm worried about errp being set in only a subset of error cases.

This is likely to cause bugs if callers use if (local_err).  Grepping
through the codebase I can see instances of:

  ret = foo(..., &local_err);
  if (local_err) { /* no ret check! */
      ...
  }

The code would work fine with qcow2 but not iscsi, for example.

IMO we should always set errp, even if the error message is vague
("truncation failed: #{strerror}").

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]