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

[Max Reitz]

On 23.03.2017 16:32, Stefan Hajnoczi wrote:
> On Wed, Mar 22, 2017 at 05:50:59PM +0100, Max Reitz wrote:
>> On 22.03.2017 17:28, Stefan Hajnoczi wrote:
>>> 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:
>>
>> Yes, but the generic bdrv_truncate() will always set errp if the driver
>> hasn't done so.
> 
> I prefer consistently setting errp to make patch review easy (no
> checking all callers to see how they behave), making it safe to
> copy-paste the code elsewhere, etc.

The thing is, there is only a single caller for all of the
BlockDriver.bdrv_truncate() functions and that is the generic
bdrv_truncate() function.

I mean, of course I can copy-paste my generic error message from the
global bdrv_truncate() function into every driver where I'm not sure
what error to emit, but I find that a bit... Well, it looks bad when you
just look at the code without the history behind it.

But since you insist, I'll do so gladly. :-)

Max

> It's safer to be consistent, can produce more detailed error messages,
> and the cost of writing error_setg() calls is low.  But it's a matter of
> style, you've pointed out the code is technically correct right now.
> 
> Stefan
> 




signature.asc
Description: OpenPGP digital signature

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

[Stefan Hajnoczi]

On Wed, Mar 22, 2017 at 05:50:59PM +0100, Max Reitz wrote:
> On 22.03.2017 17:28, Stefan Hajnoczi wrote:
> > 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:
> 
> Yes, but the generic bdrv_truncate() will always set errp if the driver
> hasn't done so.

I prefer consistently setting errp to make patch review easy (no
checking all callers to see how they behave), making it safe to
copy-paste the code elsewhere, etc.

It's safer to be consistent, can produce more detailed error messages,
and the cost of writing error_setg() calls is low.  But it's a matter of
style, you've pointed out the code is technically correct right now.

Stefan


signature.asc
Description: PGP signature

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

[Max Reitz]

On 22.03.2017 17:28, Stefan Hajnoczi wrote:
> 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:

Yes, but the generic bdrv_truncate() will always set errp if the driver
hasn't done so.

Max

>   ret = foo(..., _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
> 




signature.asc
Description: OpenPGP digital signature

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

[Stefan Hajnoczi]

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(..., _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


signature.asc
Description: PGP signature

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

[Max Reitz]

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...

Max



signature.asc
Description: OpenPGP digital signature

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

[Stefan Hajnoczi]

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").

stefan


signature.asc
Description: PGP signature

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

[Stefan Hajnoczi]

On Mon, Mar 13, 2017 at 10:39:46PM +0100, Max Reitz wrote:
> Add a PreallocMode parameter to the bdrv_truncate() function implemented
> by each block driver. Currently, we always pass PREALLOC_MODE_OFF and no
> driver accepts anything else.
> 
> Signed-off-by: Max Reitz 
> ---
>  include/block/block_int.h |  3 ++-
>  block.c   |  2 +-
>  block/blkdebug.c  |  9 -
>  block/crypto.c|  8 +++-
>  block/file-posix.c|  9 -
>  block/file-win32.c|  9 -
>  block/gluster.c   |  6 +-
>  block/iscsi.c |  7 ++-
>  block/nfs.c   |  8 +++-
>  block/qcow2.c |  9 -
>  block/qed.c   |  9 -
>  block/raw-format.c|  9 -
>  block/rbd.c   |  7 ++-
>  block/sheepdog.c  | 11 +--
>  14 files changed, 91 insertions(+), 15 deletions(-)

This patch reminds me that some block drivers aren't using Error **errp
yet.  I have added a BiteSizedTasks entry so that this can be fixed
someday.

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature