[Top][All Lists]

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

Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()
Date: Mon, 17 Dec 2018 09:30:30 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

On 12/15/18 9:19 AM, Richard W.M. Jones wrote:
On Sat, Dec 15, 2018 at 07:53:14AM -0600, Eric Blake wrote:
Always allocate space for the reply returned by the server and
hoist the trace earlier, as it is more interesting to trace the
server's reply (even if it is unexpected) than parroting our
request only on success.  After all, skipping the allocation
for a wrong size was merely a micro-optimization that only
benefitted a broken server, rather than the common case of a
compliant server that meets our expectations.

Then turn the reply handling into a loop (even though we still
never iterate more than once), to make this code easier to use
when later patches do support multiple server replies.  This
changes the error message for a server with two replies (a
corner case we are unlikely to hit in practice) from:

Unexpected reply type 4 (meta context), expected 0 (ack)


Server replied with more than one context

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

v2: split patch into easier-to-review pieces [Rich, Vladimir]
  nbd/client.c | 16 ++++++++++++----
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index bcccd5f555e..b6a85fc3ef8 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -684,10 +684,11 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
          return ret;

-    if (reply.type == NBD_REP_META_CONTEXT) {
+    while (reply.type == NBD_REP_META_CONTEXT) {

I'm not sure I understand why this change is safe.

As far as I can see reply.type is only updated in the loop by
nbd_receive_option_reply, and that reads from the server, and so the
server might keep sending NBD_REP_META_CONTEXT packets (instead of the
expected NBD_REP_ACK), so it could now loop forever against a
malicious server?  (This is not taking into account any later patches)

Hmm - now that I've already responded to why the conversion to a loop does not change this code, I'm now wondering if I even need this patch. In v1 of the series, both SET and LIST shared a common function, and since LIST needs the loop, converting SET to use a loop that exits early if it executes more than once was needed to make the two actions share a common entry point. But since v2 uses different entry points (because it separated the common code into separate helper functions, leaving the SET entry point unchanged and adding a new LIST entry point), where only the LIST entry point actually has to loop, I might be able to just drop this patch entirely and still achieve the same effect.

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]