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