Re: [Qemu-devel] [PATCH v3 04/19] nbd/server: Hoist length check to qemp_nbd_server_add

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

12.01.2019 20:57, Eric Blake wrote:
> We only had two callers to nbd_export_new; qemu-nbd.c always
> passed a valid offset/length pair (because it already checked
> the file length, to ensure that offset was in bounds), while
> blockdev-nbd always passed 0/-1.  Then nbd_export_new reduces
> the size to a multiple of BDRV_SECTOR_SIZE (can only happen
> when offset is not sector-aligned, since bdrv_getlength()
> currently rounds up), which can result in offset being greater
> than the enforced length, but that's not fatal (the server
> rejects client requests that exceed the advertised length).
> 
> However, I'm finding it easier to work with the code if we are
> consistent on having both callers pass in a valid length, and
> just assert that things are sane in nbd_export_new.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: new patch
> ---
>   blockdev-nbd.c | 10 +++++++++-
>   nbd/server.c   |  9 ++-------
>   2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index c76d5416b90..d73ac1b026a 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -146,6 +146,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>       BlockDriverState *bs = NULL;
>       BlockBackend *on_eject_blk;
>       NBDExport *exp;
> +    int64_t len;
> 
>       if (!nbd_server) {
>           error_setg(errp, "NBD server not running");
> @@ -168,6 +169,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           return;
>       }
> 
> +    len = bdrv_getlength(bs);
> +    if (len < 0) {
> +        error_setg_errno(errp, -len,
> +                         "Failed to determine the NBD export's length");
> +        return;
> +    }
> +
>       if (!has_writable) {
>           writable = false;
>       }
> @@ -175,7 +183,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           writable = false;
>       }
> 
> -    exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap,
> +    exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
>                            writable ? 0 : NBD_FLAG_READ_ONLY,
>                            NULL, false, on_eject_blk, errp);
>       if (!exp) {
> diff --git a/nbd/server.c b/nbd/server.c
> index e8c56607eff..c9937ccdc2a 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1499,13 +1499,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>       exp->name = g_strdup(name);
>       exp->description = g_strdup(description);
>       exp->nbdflags = nbdflags;
> -    exp->size = size < 0 ? blk_getlength(blk) : size;
> -    if (exp->size < 0) {
> -        error_setg_errno(errp, -exp->size,
> -                         "Failed to determine the NBD export's length");
> -        goto fail;
> -    }
> -    exp->size -= exp->size % BDRV_SECTOR_SIZE;
> +    assert(dev_offset <= size);

@size is not size of the image, but size of the export, so it may be less than dev_offset
(qemu-nbd.c do "fd_size -= dev_offset" before "nbd_export_new(bs, dev_offset, fd_size, "

> +    exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
> 
>       if (bitmap) {
>           BdrvDirtyBitmap *bm = NULL;
> 


-- 
Best regards,
Vladimir