Re: [RFC PATCH 06/22] qemu-nbd: Use raw block driver for --offset

[Eric Blake <eblake@redhat.com>]

On 8/13/20 11:29 AM, Kevin Wolf wrote:
> Instead of implementing qemu-nbd --offset in the NBD code, just put a
> raw block node with the requested offset on top of the user image and
> rely on that doing the job.
> 
> This does not only simplify the nbd_export_new() interface and bring it
> closer to the set of options that the nbd-server-add QMP command offers,
> but in fact it also eliminates a potential source for bugs in the NBD
> code which previously had to add the offset manually in all relevant
> places.

Yay!  This patch alone is worth having, regardless of the fate of the 
rest of the series: no change in end-user functionality, but by making 
qemu-nbd turn it into proper syntactic sugar, we've reduced the 
maintenance burden of duplicated code.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/block/nbd.h |  4 ++--
>   blockdev-nbd.c      |  9 +--------
>   nbd/server.c        | 34 +++++++++++++++++-----------------
>   qemu-nbd.c          | 27 ++++++++++++---------------
>   4 files changed, 32 insertions(+), 42 deletions(-)
> 

> +++ b/nbd/server.c
> @@ -89,7 +89,6 @@ struct NBDExport {
>       BlockBackend *blk;
>       char *name;
>       char *description;
> -    uint64_t dev_offset;
>       uint64_t size;

I'm trying to figure out if we can also drop 'size' here.  If we do, the 
consequence would be that an NBD client could ask for beyond-EOF I/O, 
and presumably the block layer would reject that gracefully (although 
not necessarily with the same errno as NBD currently reports).  I'm fine 
leaving it alone in this patch, though.

> @@ -1569,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>           exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
>                             NBD_FLAG_SEND_FAST_ZERO);
>       }
> -    assert(size <= INT64_MAX - dev_offset);
> +    assert(size <= INT64_MAX);

As Max caught, this is now dead code.

> @@ -2386,8 +2388,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>           if (request->flags & NBD_CMD_FLAG_FAST_ZERO) {
>               flags |= BDRV_REQ_NO_FALLBACK;
>           }
> -        ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
> -                                request->len, flags);
> +        ret = blk_pwrite_zeroes(exp->blk, request->from, request->len, flags);
>           return nbd_send_generic_reply(client, request->handle, ret,
>                                         "writing to file failed", errp);
>   
> @@ -2401,8 +2402,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>                                         "flush failed", errp);
>   
>       case NBD_CMD_TRIM:
> -        ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
> -                              request->len);
> +        ret = blk_co_pdiscard(exp->blk, request->from, request->len);

Merge conflicts with 890cbccb08; should be obvious enough to resolve, 
though.

> +++ b/qemu-nbd.c
> @@ -523,7 +523,6 @@ int main(int argc, char **argv)
>       const char *port = NULL;
>       char *sockpath = NULL;
>       char *device = NULL;
> -    int64_t fd_size;
>       QemuOpts *sn_opts = NULL;
>       const char *sn_id_or_name = NULL;
>       const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:B:L";
> @@ -1028,6 +1027,17 @@ int main(int argc, char **argv)
>       }
>       bs = blk_bs(blk);
>   
> +    if (dev_offset) {
> +        QDict *raw_opts = qdict_new();
> +        qdict_put_str(raw_opts, "driver", "raw");
> +        qdict_put_str(raw_opts, "file", bs->node_name);
> +        qdict_put_int(raw_opts, "offset", dev_offset);

Huh.  When 0bc16997f5 got rid of the --partition option, it also got rid 
of the only way that the NBD driver could clamp down requests to a range 
smaller than the end of the file.  Now that you are adding a raw driver 
in the mix, that ability to clamp the end of the range (aka a --size 
option, in addition to an --offset option) may be worth reinstating. 
But that can be done as a separate patch, if at all (and whether 
qemu-nbd should do it, or qemu-storage-daemon, or whether we just point 
people at 'nbdkit --filter=partition', is part of that discussion).  But 
for this patch, it looks like you are making a straight-across conversion.

> +        bs = bdrv_open(NULL, NULL, raw_opts, flags, &error_fatal);
> +        blk_remove_bs(blk);
> +        blk_insert_bs(blk, bs, &error_fatal);
> +        bdrv_unref(bs);
> +    }
> +

Slick.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org