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

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

[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 <kwolf@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 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)
 {
     int ret, score, score_max;
     BlockDriver *drv1, *drv;
     uint8_t buf[2048];
     BlockDriverState *bs;
 
+    *error = -ENOENT;
+
     ret = bdrv_file_open(&bs, filename, 0);
-    if (ret < 0)
+    if (ret < 0) {
+        *error = ret;
         return NULL;
+    }
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
     if (bs->sg || !bdrv_is_inserted(bs)) {
@@ -350,6 +354,7 @@ static BlockDriver *find_image_format(const char *filename)
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
     bdrv_delete(bs);
     if (ret < 0) {
+        *error = ret;
         return NULL;
     }
 
@@ -571,12 +576,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
 
     /* Find the right image format driver */
     if (!drv) {
-        drv = find_image_format(filename);
+        drv = find_image_format(filename, &ret);
         probed = 1;
     }
 
     if (!drv) {
-        ret = -ENOENT;
         goto unlink_and_fail;
     }
 
-- 
1.7.1

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

[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 <kwolf@redhat.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  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.

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

Apart from these minor nitpicks it looks good.

Kevin

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

[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<kwolf@redhat.com>
>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>> ---
>>   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.

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

> Apart from these minor nitpicks it looks good.
>
> Kevin
>
>    
Thanks.

Stefan

[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<kwolf@redhat.com>
>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>>> ---
>>>   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

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

[Stefan Weil]

"No such file or directory" is a misleading error message
when a user tries to open a file with wrong permissions.

v2: return an 0/-errno int and pass the BlockDriver* by reference
    as suggested by Kevin Wolf

Cc: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 block.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index b133b70..b49abf2 100644
--- a/block.c
+++ b/block.c
@@ -330,7 +330,7 @@ BlockDriver *bdrv_find_protocol(const char *filename)
     return NULL;
 }
 
-static BlockDriver *find_image_format(const char *filename)
+static int find_image_format(const char *filename, BlockDriver **pdrv)
 {
     int ret, score, score_max;
     BlockDriver *drv1, *drv;
@@ -338,19 +338,27 @@ static BlockDriver *find_image_format(const char *filename)
     BlockDriverState *bs;
 
     ret = bdrv_file_open(&bs, filename, 0);
-    if (ret < 0)
-        return NULL;
+    if (ret < 0) {
+        *pdrv = NULL;
+        return ret;
+    }
 
     /* Return the raw BlockDriver * to scsi-generic devices or empty drives */
     if (bs->sg || !bdrv_is_inserted(bs)) {
         bdrv_delete(bs);
-        return bdrv_find_format("raw");
+        drv = bdrv_find_format("raw");
+        if (!drv) {
+            ret = -ENOENT;
+        }
+        *pdrv = drv;
+        return ret;
     }
 
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
     bdrv_delete(bs);
     if (ret < 0) {
-        return NULL;
+        *pdrv = NULL;
+        return ret;
     }
 
     score_max = 0;
@@ -364,7 +372,11 @@ static BlockDriver *find_image_format(const char *filename)
             }
         }
     }
-    return drv;
+    if (!drv) {
+        ret = -ENOENT;
+    }
+    *pdrv = drv;
+    return ret;
 }
 
 /**
@@ -571,12 +583,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
 
     /* Find the right image format driver */
     if (!drv) {
-        drv = find_image_format(filename);
+        ret = find_image_format(filename, &drv);
         probed = 1;
     }
 
     if (!drv) {
-        ret = -ENOENT;
         goto unlink_and_fail;
     }
 
-- 
1.7.1

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

[Kevin Wolf]

Am 21.07.2010 21:51, schrieb Stefan Weil:
> "No such file or directory" is a misleading error message
> when a user tries to open a file with wrong permissions.
> 
> v2: return an 0/-errno int and pass the BlockDriver* by reference
>     as suggested by Kevin Wolf
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

Thanks, applied to the block branch.

Kevin