[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t |
Date: |
Tue, 15 Jan 2019 09:33:57 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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.
>> @@ -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).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 06/19] qemu-nbd: Avoid strtol open-coding, (continued)
[Qemu-devel] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t, Eric Blake, 2019/01/12
[Qemu-devel] [PATCH v3 03/19] qemu-nbd: Sanity check partition bounds, Eric Blake, 2019/01/12
[Qemu-devel] [PATCH v3 14/19] nbd/client: Pull out oldstyle size determination, Eric Blake, 2019/01/12