Re: [Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t

[Vladimir Sementsov-Ogievskiy]

15.01.2019 18:33, Eric Blake wrote:
> On 1/15/19 4:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.01.2019 20:57, Eric Blake wrote:
>>> Although our compile-time environment is set up so that we always
>>> support long files with 64-bit off_t, we have no guarantee whether
>>> off_t is the same type as int64_t.  This requires casts when
>>> printing values, and prevents us from directly using qemu_strtoi64().
>>> Let's just flip to [u]int64_t (signed for length, because we have to
>>> detect failure of blk_getlength()
>>
>> we have not, after previous patch
> 
> nbd/server.c no longer has to check for blk_getlength() failures, but
> blockdev-nbd.c and qemu-nbd.c still do.  Since the callers have to use
> an int64_t type for the length as part of their error checking, it's
> easier to accept an int64_t length to nbd_export_new(), even if
> nbd_export_new() could also use an unsigned type.
> 
>>
>> and because off_t was signed;
>>> unsigned for offset because it lets us simplify some math without
>>> having to worry about signed overflow).
>>>
>>> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>>> ---
>>> v3: new patch
>>> ---
>>>    include/block/nbd.h |  4 ++--
>>>    nbd/server.c        | 14 +++++++-------
>>>    qemu-nbd.c          | 26 ++++++++++----------------
>>>    3 files changed, 19 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>>> index 1971b557896..0f252829376 100644
>>> --- a/include/block/nbd.h
>>> +++ b/include/block/nbd.h
>>> @@ -294,8 +294,8 @@ int nbd_errno_to_system_errno(int err);
>>>    typedef struct NBDExport NBDExport;
>>>    typedef struct NBDClient NBDClient;
>>>
>>> -NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t 
>>> size,
>>> -                          const char *name, const char *description,
>>> +NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>> +                          int64_t size, const char *name, const char *desc,
>>
>> in previous patch you drop use of negative @size parameter, so it looks 
>> better
>> to use unsigned for size like for offset
> 
> You can't have a size larger than 2^63; but an unsigned type holds
> nearly 2^64.  I prefer a signed size, for the same reason that off_t is
> signed, in that checking for a negative size is easier to rule out sizes
> that are too large.
> 
> 
>>>
>>>    static int find_partition(BlockBackend *blk, int partition,
>>> -                          off_t *offset, off_t *size)
>>> +                          uint64_t *offset, int64_t *size)
>>
>> function never return negative @size, so what is the reason to keep it 
>> signed?
> 
> Because the C compiler does NOT like:
> 
> int64_t len;
> find_partition(..., &len);
> 
> with a uint64_t* parameter type - you HAVE to match the signed-ness of
> your caller's parameter with your pointer type. Since the caller already
> has to use a signed type (to check for blk_getlength() failure AND
> because sizes really cannot exceed 2^63), it's easier to keep it signed
> here.
> 
>>
>> Also, type conversion (uint64_t) should be dropped from the function code I 
>> think.
> 
> Are you talking about this part:
> 
>                  if ((ext_partnum + j + 1) == partition) {
>                      *offset = (uint64_t)ext[j].start_sector_abs << 9;
>                      *size = (uint64_t)ext[j].nb_sectors_abs << 9;
>                      return 0;
>                  }
>              }
>              ext_partnum += 4;
>          } else if ((i + 1) == partition) {
>              *offset = (uint64_t)mbr[i].start_sector_abs << 9;
>              *size = (uint64_t)mbr[i].nb_sectors_abs << 9;
>              return 0;
> 
> No - that has to keep the cast, because .start_sector_abs and
> .nb_sectors_abs are uint32_t values, but we want to shift into 64-bit
> results.  You need the cast to force the correct arithmetic rather than
> truncating into a 32-bit value that then gets widened into 64-bit storage.
> 
>>> @@ -665,10 +665,6 @@ int main(int argc, char **argv)
>>>                    error_report("Invalid offset `%s'", optarg);
>>>                    exit(EXIT_FAILURE);
>>>                }
>>> -            if (dev_offset < 0) {
>>> -                error_report("Offset must be positive `%s'", optarg);
>>> -                exit(EXIT_FAILURE);
>>> -            }
>>
>> hm, then, may be, s/strtoll/strtoull before this?
> 
> I clean that up in patch 6/19.
> 
>>
>>>                break;
>>>            case 'l':
>>>                if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
>>> @@ -1005,15 +1001,14 @@ int main(int argc, char **argv)
>>>        }
>>>
>>>        if (dev_offset >= fd_size) {
>>> -        error_report("Offset (%lld) has to be smaller than the image size "
>>> -                     "(%lld)",
>>> -                     (long long int)dev_offset, (long long int)fd_size);
>>> +        error_report("Offset (%" PRIu64 ") has to be smaller than the 
>>> image "
>>> +                     "size (%" PRIu64 ")", dev_offset, fd_size);
>>
>> PRId64 for fd_size
> 
> Sure.

With this one fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Interesting, decided to search a bit, about don't we have an overflow,
when adding request.offset to exp.dev_offset and found in 
nbd_co_receive_request:

     if (request->from > client->exp->size ||
         request->from + request->len > client->exp->size) {

shouldn't it be

     if (request->from > client->exp->size ||
         request->len > client->exp->size - request->from) {

to avoid overflow?


> 
> 
>>> @@ -1026,12 +1021,11 @@ int main(int argc, char **argv)
>>>                exit(EXIT_FAILURE);
>>>            }
>>>            /* partition limits are (32-bit << 9); can't overflow 64 bits */
>>> -        assert(dev_offset >= 0 && dev_offset + limit >= dev_offset);
>>> +        assert(dev_offset + limit >= dev_offset);
>>>            if (dev_offset + limit > fd_size) {
>>> -            error_report("Discovered partition %d at offset %lld size 
>>> %lld, "
>>> -                         "but size exceeds file length %lld", partition,
>>> -                         (long long int) dev_offset, (long long int) limit,
>>> -                         (long long int) fd_size);
>>> +            error_report("Discovered partition %d at offset %" PRIu64
>>> +                         " size %" PRId64 ", but size exceeds file length 
>>> %"
>>> +                         PRId64, partition, dev_offset, limit, fd_size);
>>>                exit(EXIT_FAILURE);
>>
>> hmm, it may be better to place this patch before [03], to squash this chunk 
>> into [03]
> 
> I didn't mind the churn; also, I prefer patch 3 first, because it's more
> likely to get backported as a bug fix than the rest of the series (and
> the earlier you stick backport candidates in a series, the easier it is
> to backport).
> 


-- 
Best regards,
Vladimir