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

Reply via email to