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 <kw...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
