Re: [Qemu-devel] [PATCH v3 05/19] nbd/server: Favor [u]int64_t over off_t

[Vladimir Sementsov-Ogievskiy]

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 <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

>                             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