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
