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