On 06/29/2018 05:01 AM, Vladimir Sementsov-Ogievskiy wrote:
Sorry for being late, here are some thoughts. Anyway, idea is good,
we've planned to do something like this, but you were the first)
Actually, I'm now leaning towards a bit more extensive improvement that
would let 'qemu-img map --output=json' show BOTH normal block status AND
the NBD dirty bitmap status at once (that is, let the client query two
status fields at once when passing an NBD option, rather than this
hack's approach of replacing normal block status with just dirty bitmap
status). We'll see if I can come up with something before 3.0
softfreeze (at any rate, I have my work cut out for me today).
+++ b/qapi/block-core.json
@@ -3484,12 +3484,22 @@
#
# @tls-creds: TLS credentials ID
#
+# @x-block-status: A string containing a semicolon-separated list of
+# block status context names to query and then
+# request
and then request only the first one
Ideally, I'd like to report dirty bitmap in addition to normal block
status (and actually, we really only need to support exactly one string,
none of this semicolon-separated stuff). We'll see where v2 gets us; but
yes, this v1 hack uses only the first string.
My v1 hack served two purposes: stress-test the server (where the
semicolon separation DID let me slam multiple requests, with multiple
different spellings, to double-check that I was happy with the server's
response in all cases), and hack a single reader for bitmap status; only
the latter one is important in the long run.
@@ -438,8 +443,8 @@ static int nbd_open(BlockDriverState *bs, QDict
*options, int flags,
}
/* NBD handshake */
- ret = nbd_client_init(bs, sioc, s->export,
- tlscreds, hostname, errp);
+ ret = nbd_client_init(bs, sioc, s->export, tlscreds, hostname,
+ qemu_opt_get(opts, "x-block-status"), errp);
hm, so, after this nbd_open finish, info.x_block_status will become
invalid pointer..
It's not used in other places, but looks like bad idea anyway. If we
don't want to allocate string,
we can pass it as a separate const char* paramter to nbd_receive_negotiate.
As a hack, I just plumbed in the bare minimum to get from command line
to the wire. Yes, a more formal patch would have to strdup() something
to last for the proper lifetime, and maybe pass it through via a struct
rather than having to add yet more parameters for future additions.
+/* nbd_negotiate_list_meta_context:
+ * Hack for testing meta context negotiation. Subdivide list on
semicolons,
+ * then pass that many queries for info and trace the results.
+ * return 0 for successful negotiation
+ * -1 with errp set for any other error
+ */
+static int nbd_negotiate_list_meta_context(QIOChannel *ioc,
+ const char *export,
+ const char *context,
+ Error **errp)
+{
hm, I'd prefer to refactor this, to not duplicate code.. this function
(may be a bit improved) may be called from
nbd_negotiate_simple_meta_context(), and we will test normal code path
when debugging with the hack.
However, I'm ok with this duplication for now, as a debugging hack.
Yes, very true. I did a LOT of copy-and-paste which would be nicer as
parameterized reusable code.
+ while (reply.type == NBD_REP_META_CONTEXT) {
+ char *name;
+
don't you want to check reply.length before read?
Yes, in a formal patch, we must not trust that the server is
non-malicious. It might be worth refactoring cleanup along the lines of
nbd_opt_read() in server.c that guarantees that a subsequent read fits
within a predetermined remaining length, for less code at each point in
the sequence that has to do such length validations.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
