qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [PATCH v5 06/11] nbd: Minimal structured read for serve


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v5 06/11] nbd: Minimal structured read for server
Date: Fri, 20 Oct 2017 22:30:03 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

20.10.2017 22:11, Eric Blake wrote:
On 10/20/2017 02:03 PM, Vladimir Sementsov-Ogievskiy wrote:
20.10.2017 01:26, Eric Blake wrote:
From: Vladimir Sementsov-Ogievskiy <address@hidden>

Minimal implementation of structured read: one structured reply chunk,
no segmentation.
Minimal structured error implementation: no text message.
Support DF flag, but just ignore it, as there is no segmentation any
way.

+
+            case NBD_OPT_STRUCTURED_REPLY:
+                if (length) {
+                    ret = nbd_check_zero_length(client, length,
option, errp);
here same thing like in previous patch, about success in
nbd_check_zero_length
Here, successfully answering the client with an error message should NOT
stop negotiating, so THIS return is correct.

ok, you are right


+                } else if (client->structured_reply) {
+                    ret = nbd_negotiate_send_rep_err(
+                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
+                        "structured reply already negotiated");
+                } else {
+                    ret = nbd_negotiate_send_rep(client->ioc,
NBD_REP_ACK,
+                                                 option, errp);
+                }
you've dropped "if (ret < 0) { return ret }", but the following two
lines should not be
executed if ret < 0.. May be it doesn't matter (we will abort connection
anyway after switch {}) but
it looks strange.

+                client->structured_reply = true;
+                myflags |= NBD_FLAG_SEND_DF;
+                break;
Indeed, these two lines are harmless due to the catch-all 'ret < 0'
after the switch at the tail end of the loop (which is why I dropped the
'if' here).



yes. but it looks strange) Anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


--
Best regards,
Vladimir


reply via email to

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