Re: [Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t
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
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
> Signed-off-by: Eric Blake
>
> ---
> 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
> const char *bitmap, uint16_t nbdflags,
> void (*close)(NBDExport *), bool writethrough,
> BlockBackend *on_eject_blk, Error **errp);
> diff --git a/nbd/server.c b/nbd/server.c
> index c9937ccdc2a..15357d40fd7 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -77,8 +77,8 @@ struct NBDExport {
> BlockBackend *blk;
> char *name;
> char *description;
> -off_t dev_offset;
> -off_t size;
> +uint64_t dev_offset;
> +int64_t size;
> uint16_t nbdflags;
> QTAILQ_HEAD(, NBDClient) clients;
> QTAILQ_ENTRY(NBDExport) next;
> @@ -1455,8 +1455,8 @@ static void nbd_eject_notifier(Notifier *n, void *data)
> nbd_export_close(exp);
> }
>
> -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,
> const char *bitmap, uint16_t nbdflags,
> void (*close)(NBDExport *), bool writethrough,
> BlockBackend *on_eject_blk, Error **errp)
> @@ -1497,7 +1497,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t
> dev_offset, off_t size,
> exp->blk = blk;
> exp->dev_offset = dev_offset;
> exp->name = g_strdup(name);
> -exp->description = g_strdup(description);
> +exp->description = g_strdup(desc);
unrelated but at least obvious, OK. However tiny note in commit message won't
hurt.
> exp->nbdflags = nbdflags;
> assert(dev_offset <= size);
> exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
> @@ -2131,8 +2131,8 @@ static int nbd_co_receive_request(NBDRequestData *req,
> NBDRequest *request,
> if (request->from > client->exp->size ||
> request->from + request->len > client->exp->size) {
> error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %"
> PRIu32
> - ", Size: %" PRIu64, request->from, request->len,
> - (uint64_t)client->exp->size);
> + ", Size: %" PRId64, request->from, request->len,
> + client->exp->size);
> return (request->type == NBD_CMD_WRITE ||
> request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL;
> }
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index ff4adb9b3eb..96c0829970c 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -176,7 +176,7 @@ static void read_partition(uint8_t *p, struct
> partition_record *r)
> }
>
> 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?
Also, type conversion (uint64_t) should be dropped from the function code I
think.
> {
> struct partition_record mbr[4];
> uint8_t data[MBR_SIZE];
> @@ -500,14 +500,14 @@ int main(int argc, char **argv)
> {
> BlockBackend *blk;
> BlockDriverState *bs;
> -off_t dev_offset = 0;
> +uint64_t dev_offset = 0;
> uint16_t nbdflags = 0;
> bool disconnect = false;
Re: [Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t
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
>> Signed-off-by: Eric Blake
>>
>> ---
>> 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 (3
Re: [Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t
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
>>> Signed-off-by: Eric Blake
>>>
>>> ---
>>> 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.
Oops, I'm stupid)
I thought about something like (uint64_t),
but pointed to =
(uint64_t)
>
>>> @@ -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);
>>> +
Re: [Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t
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
>>> Signed-off-by: Eric Blake
>>>
>>> ---
>>> 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 "
>>> +
Re: [Qemu-block] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t
On 1/16/19 2:23 AM, Vladimir Sementsov-Ogievskiy wrote:
>
> 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) {
uint64_t + uint32_t > uint64_t
>
> shouldn't it be
>
> if (request->from > client->exp->size ||
> request->len > client->exp->size - request->from) {
>
> to avoid overflow?
You are correct that the canonical way to check for overflow is to
compare against subtraction, rather than do addition before compare.
But we got lucky: even though client->exp->size is uint64_t, in practice
it can only represent at most off_t (remember, off_t is signed), so the
first leg of the branch proves that request->from fits in 63 bits, and
thus the second is performing 63-bit + 32-bit which can be at most 64
bits, and therefore does not overflow. Still, I might rewrite it.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
