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 <ebl...@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
