[Qemu-devel] Re: [PATCH] block: Use error codes from lower levels for error message

[Kevin Wolf]

Am 19.07.2010 22:55, schrieb Stefan Weil:
> Am 19.07.2010 14:26, schrieb Kevin Wolf:
>> Am 18.07.2010 21:42, schrieb Stefan Weil:
>>    
>>> "No such file or directory" is a misleading error message
>>> when a user tries to open a file with wrong permissions.
>>>
>>> Cc: Kevin Wolf<address@hidden>
>>> Signed-off-by: Stefan Weil<address@hidden>
>>> ---
>>>   block.c |   12 ++++++++----
>>>   1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index f837876..2f80540 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -330,16 +330,20 @@ BlockDriver *bdrv_find_protocol(const char *filename)
>>>       return NULL;
>>>   }
>>>
>>> -static BlockDriver *find_image_format(const char *filename)
>>> +static BlockDriver *find_image_format(const char *filename, int *error)
>>>      
>> Wouldn't it be a more natural interface to return an 0/-errno int and
>> pass the BlockDriver* by reference? I think we already have some
>> function that work this way in the block code, but I can't remember any
>> that get an int *error.
>>    
> 
> ... nor did I find a function which takes a BlockDriver**.
> But if you prefer it like that, I can send a new version of the patch.

Yes, I would prefer it like that. If you don't mind, please send a new
version.

>>    
>>>   {
>>>       int ret, score, score_max;
>>>       BlockDriver *drv1, *drv;
>>>       uint8_t buf[2048];
>>>       BlockDriverState *bs;
>>>
>>> +    *error = -ENOENT;
>>>      
>> Why -ENOENT is the default would be clearer if you moved it down next to
>> the drv = NULL before the loop that searches for the driver.
>>
>>    
> 
> What about the "return bdrv_find_format" lines?
> They need a default value, too.
> 
> And I did not want to change too much because I cannot
> run a complete test for all cases.
> 
> So setting *error at the beginning should be the safest
> modification.

You mean the return bdrv_find_format("raw")? Shouldn't ever fail unless
someone was crazy enough to compile raw out, but you're right. Better
leave it as it is.

Kevin