RE: [PATCH] block: Return the real error code in bdrv_getlength

RE: [PATCH] block: Return the real error code in bdrv_getlength

[Tuguoyi]

Sorry, please ignore this patch, it's not a right fix

--
Best regards,
Guoyi


> -----Original Message-----
> From: tuguoyi (Cloud)
> Sent: Thursday, November 05, 2020 11:11 AM
> To: 'Kevin Wolf' <kwolf@redhat.com>; 'Max Reitz' <mreitz@redhat.com>;
> 'qemu-block@nongnu.org' <qemu-block@nongnu.org>
> Cc: 'qemu-devel@nongnu.org' <qemu-devel@nongnu.org>
> Subject: [PATCH] block: Return the real error code in bdrv_getlength
> 
> The return code from  bdrv_nb_sectors() should be checked before doing
> the following sanity check.
> 
> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
> ---
>  block.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 430edf7..19ebbc0 100644
> --- a/block.c
> +++ b/block.c
> @@ -5082,6 +5082,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>  {
>      int64_t ret = bdrv_nb_sectors(bs);
> 
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>      ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;
>      return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
>  }
> --
> 2.7.4
> 
> 
> --
> Best regards,
> Guoyi
> 

Re: [PATCH] block: Return the real error code in bdrv_getlength

[Eric Blake]

On 11/4/20 11:41 PM, Tuguoyi wrote:
> Sorry, please ignore this patch, it's not a right fix

What's not right about it?


>> +++ b/block.c
>> @@ -5082,6 +5082,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
>>  {
>>      int64_t ret = bdrv_nb_sectors(bs);
>>
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>>      ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;

You are correct that the old code was broken (since
INT64_MAX/BDRV_SECTOR_SIZE is unsigned, any negative ret is likewise
promoted to unsigned and we mistakenly return -EFBIG instead of the
original error).  But your new code works just fine: because we
guarantee with the early return above that ret is a positive value, it
doesn't matter that the rest of this ?: is performed in unsigned math.

>>      return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;

Of course, having two ?: in a row is odd; we could instead write:

if (ret < 0){
    return ret;
}
if (ret > INT64_MAX / BDRV_SECTOR_SIZE) {
    return -EFBIG;
}
return ret * BDRV_SECTOR_SIZE;

for the same result.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] block: Return the real error code in bdrv_getlength

[Eric Blake]

On 11/4/20 9:10 PM, Tuguoyi wrote:
> The return code from  bdrv_nb_sectors() should be checked before doing
> the following sanity check.
> 
> Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>

It looks like you sent several variations on this patch.  A
meta-observation: your mailer is attributing the patch to the spelling
"Tuguoyi", while your S-o-b line is spelling "Guoyi Tu".  It's worth
being consistent between the two.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH] block: Return the real error code in bdrv_getlength

[Tuguoyi]

The return code from  bdrv_nb_sectors() should be checked before doing
the following sanity check.

Signed-off-by: Guoyi Tu <tu.guoyi@h3c.com>
---
 block.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index 430edf7..19ebbc0 100644
--- a/block.c
+++ b/block.c
@@ -5082,6 +5082,10 @@ int64_t bdrv_getlength(BlockDriverState *bs)
 {
     int64_t ret = bdrv_nb_sectors(bs);
 
+    if (ret < 0) {
+        return ret;
+    }
+
     ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;
     return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
 }
-- 
2.7.4


--
Best regards,
Guoyi