On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
Minimal implementation: always send DF flag, to not deal with fragmented
replies.
This works well with your minimal server implementation, but I worry
that it will cause us to fall over when talking to a fully-compliant
server that chooses to send EOVERFLOW errors for any request larger than
64k when DF is set; it also makes it impossible to benefit from sparse
reads. I guess that means we need to start thinking about followup
patches to flush out our implementation. But maybe I can live with this
patch as is, since the goal of your series was not so much the full
power of structured reads, but getting to a point where we could use
structured reply for block status, even if it means your client can only
communicate with qemu-nbd as server for now, as long as we do get to the
rest of the patches for a full-blown structured read.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
block/nbd-client.c | 47 +++++++++++----
block/nbd-client.h | 2 +
include/block/nbd.h | 15 +++--
nbd/client.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++------
qemu-nbd.c | 2 +-
5 files changed, 203 insertions(+), 33 deletions(-)
Hmm - no change to the testsuite. Structured reads seems like the sort
of thing that it would be nice to test with some canned server replies,
particularly with server behavior that is permitted by the NBD protocol
but does not happen by default in qemu-nbd.
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 3779c6c999..ff96bd1635 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -180,13 +180,20 @@ static void nbd_co_receive_reply(NBDClientSession *s,
*reply = s->reply;
if (reply->handle != request->handle ||
!s->ioc) {
+ reply->simple = true;
reply->error = EIO;
I don't think this is quite right - by setting reply->simple to true,
you are forcing the caller to treat this as the final packet related to
this request->handle, even though that might not be the case.
As it is, I wonder if this code is correct, even before your patch - the
server is allowed to give responses out-of-order (if we request multiple
reads without waiting for the first response) - I don't see how setting
reply->error to EIO if the request->handle indicates that we are
receiving an out-of-order response to some other packet, but that our
request is still awaiting traffic.