qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 13/22] nbd/client: Split out nbd_send_one_met


From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v2 13/22] nbd/client: Split out nbd_send_one_meta_context()
Date: Mon, 17 Dec 2018 09:34:56 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

On 12/15/18 9:22 AM, Richard W.M. Jones wrote:
On Sat, Dec 15, 2018 at 07:53:15AM -0600, Eric Blake wrote:
Refactor nbd_negotiate_simple_meta_context() to pull out the
code that can be reused to send a LIST request for 0 or 1 query.
No semantic change.

Signed-off-by: Eric Blake <address@hidden>

---
v2: split patch into easier-to-review pieces [Rich, Vladimir]
---
  nbd/client.c     | 64 ++++++++++++++++++++++++++++++++++--------------
  nbd/trace-events |  2 +-
  2 files changed, 46 insertions(+), 20 deletions(-)

+    uint32_t queries = !!query;

This initialization...


+    uint32_t context_len = 0;
+    uint32_t data_len;
+    char *data;
+    char *p;
+
+    data_len = sizeof(export_len) + export_len + sizeof(queries);

...plus the fact that it is now sizeof(variable) instead of sizeof(type)...

@@ -651,26 +693,10 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
*ioc,
      NBDOptionReply reply;
      const char *context = info->x_dirty_bitmap ?: "base:allocation";
      bool received = false;
-    uint32_t export_len = strlen(info->name);
-    uint32_t context_len = strlen(context);
-    uint32_t data_len = sizeof(export_len) + export_len +
-                        sizeof(uint32_t) + /* number of queries */

...was the reason that I dropped the comment here. The comment made sense for explaining why a sizeof(type) was being injected into data_len,

-                        sizeof(context_len) + context_len;
-    char *data = g_malloc(data_len);
-    char *p = data;

-    trace_nbd_opt_meta_request(context, info->name);
-    stl_be_p(p, export_len);
-    memcpy(p += sizeof(export_len), info->name, export_len);
-    stl_be_p(p += export_len, 1);

...because the old code was hard-coding 1 query, while the new code uses the variable (whose value is the number of queries) rather than a hard-coding.



Although a straightforward refactoring, we still lost the comment
/* number of queries */.  I'd still perhaps like to see a bit more
explanation of the layout and reasoning behind the data buffer.

Perhaps a possible improvement would be to introduce a packed struct that matches the protocol layout, instead of piece-meal constructing the struct. But I'm not sure how much effort to spend on this code.

But anyway ..

Reviewed-by: Richard W.M. Jones <address@hidden>

Thanks. I'm glad you forced me to split the v1 patch into more manageable pieces; I like how it turned out.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]