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 <[email protected]> > Signed-off-by: Eric Blake <[email protected]> > > --- > 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; > const char *bindto = NULL; > const char *port = NULL; > char *sockpath = NULL; > char *device = NULL; > - off_t fd_size; > + int64_t fd_size; and here signed type is reasonable > QemuOpts *sn_opts = NULL; > const char *sn_id_or_name = NULL; > const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:"; > @@ -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? > 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 > exit(EXIT_FAILURE); > } > fd_size -= dev_offset; > > if (partition != -1) { > - off_t limit; > + int64_t limit; > > if (dev_offset) { > error_report("Cannot request partition and offset together"); > @@ -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] > } > fd_size = limit; > -- Best regards, Vladimir
