qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 01/16] block: Add Prealloc


From: Max Reitz
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 01/16] block: Add PreallocMode to BD.bdrv_truncate()
Date: Wed, 22 Mar 2017 17:50:59 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

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(..., &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: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]