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 <vsement...@virtuozzo.com> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >>> >>> --- >>> 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)<variable that was off_t, but now it is uint64_t>, but pointed to <variable that was off_t, but now it is uint64_t> = (uint64_t)<something other> > >>> @@ -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). > -- Best regards, Vladimir