Re: [Libguestfs] [Nbd] Testing NBD server implementations for correctness
Hi, On 26.09.2016 09:53, Wouter Verhelst wrote:
On Mon, Sep 26, 2016 at 03:22:52AM +0200, Carl-Daniel Hailfinger wrote:
Running nbd-tester-client against nbdkit with oldstyle negotiation was fun. I managed to segfault nbdkit
Side note: I'm going to try and get a backtrace from the nbdkit segfault and submit a bug report there.
and noticed that nbd-tester-client speaks the oldstyle protocol incorrectly, ignoring flags sent by the server. Patch follows. Whoops.
Honestly though, the oldstyle protocol shouldn't be used anymore, but yeah, I know there are still implementations out in the wild that do (and there isn't much I can do to fix that, other than discouraging).
Well, nbdkit supports both, and the protocol style has to be specified on the command line. I just happened to leave out the switch telling nbdkit to use newstyle protocol. Admittedly nbdkit also has some implementation bugs in oldstyle negotiation, but they cancel each other out internally. Lots of documentation out there does not mention that the oldstyle protocol should not be used, and I fear most of that documentation hasn't been touched in years, so changing it would be impractical. Maybe it would make sense to have nbdkit print some diagnostics that it's using the (default) oldstyle protocol?
In that light, I guess it does make sense to fix bugs in the test client, even if we won't be using it ourselves.
Thanks.
diff -r e691aad45ed2 tests/run/nbd-tester-client.c --- a/tests/run/nbd-tester-client.c Thu Sep 15 13:25:47 2016 +0100 +++ b/tests/run/nbd-tester-client.c Mon Sep 26 03:18:26 2016 +0200 @@ -384,7 +384,12 @@ READ_ALL_ERRCHK(sock, &size, sizeof(size), err, "Could not read size: %s", strerror(errno)); size = ntohll(size); - READ_ALL_ERRCHK(sock, buf, 128, err, "Could not read data: %s", + uint32_t flags; + READ_ALL_ERRCHK(sock, &flags, sizeof(uint32_t), err, + "Could not read flags: %s", strerror(errno)); + flags = ntohl(flags); + *serverflags = flags; + READ_ALL_ERRCHK(sock, buf, 124, err, "Could not read data: %s", strerror(errno)); goto end; }
Without this patch, oldstyle connections of nbd-tester-client to any server will cause the message "Server did not supply flush capability flags" if flush is requested by nbd-tester-client, even if it is supported by the server. Thanks, applied.
Wow, that was quick! Thank you. I stumbled upon another problem: Apparently nbd-tester-client and nbdkit disagree on what constitutes a valid flush request. nbd-tester-client complains: ./flush 15901: Requests: 3536 ** (process:15901): WARNING **: Could not run test: Received error from server: 22 (0x16). Handle is -9223372036854764544 (0x8000000000002C00). nbdkit complains: nbdkit: python[7]: error: invalid flush request: expecting offset and length == 0 Not sure where that bug is, nbdkit or nbd-tester-client. - Is the request correct and should have been accepted? - Is the reject correct, but the response is screwed up? Regards, Carl-Daniel
On 26 Sep 2016, at 10:21, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
Wow, that was quick! Thank you.
I stumbled upon another problem: Apparently nbd-tester-client and nbdkit disagree on what constitutes a valid flush request. nbd-tester-client complains: ./flush 15901: Requests: 3536 ** (process:15901): WARNING **: Could not run test: Received error from server: 22 (0x16). Handle is -9223372036854764544 (0x8000000000002C00).
nbdkit complains: nbdkit: python[7]: error: invalid flush request: expecting offset and length == 0
Not sure where that bug is, nbdkit or nbd-tester-client. - Is the request correct and should have been accepted? - Is the reject correct, but the response is screwed up?
Per the docs: • For a flush request, length and offset are reserved, and MUST be set to all-zero. So if nbd-tester-client.c is sending a flush with non-zero length or offset, it is wrong. -- Alex Bligh
On Mon, Sep 26, 2016 at 11:43:42AM +0100, Alex Bligh wrote:
On 26 Sep 2016, at 10:21, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
Wow, that was quick! Thank you.
I stumbled upon another problem: Apparently nbd-tester-client and nbdkit disagree on what constitutes a valid flush request. nbd-tester-client complains: ./flush 15901: Requests: 3536 ** (process:15901): WARNING **: Could not run test: Received error from server: 22 (0x16). Handle is -9223372036854764544 (0x8000000000002C00).
nbdkit complains: nbdkit: python[7]: error: invalid flush request: expecting offset and length == 0
Not sure where that bug is, nbdkit or nbd-tester-client. - Is the request correct and should have been accepted? - Is the reject correct, but the response is screwed up?
Per the docs:
• For a flush request, length and offset are reserved, and MUST be set to all-zero.
So if nbd-tester-client.c is sending a flush with non-zero length or offset, it is wrong.
Right. We have had cases in the past where out-in-the-wild implementations turned out to behave differently to how the spec behaved, but in this case I think it's a pretty safe bet to say that most implementations should generally DTRT here. I'm guessing that nbd-tester-client doesn't because of uninitialized code or something (haven't actually looked at it, must go, will do so later). TLDR: nbdkit is correct, nbd-tester-client is not, patches welcome. -- < ron> I mean, the main *practical* problem with C++, is there's like a dozen people in the world who think they really understand all of its rules, and pretty much all of them are just lying to themselves too. -- #debian-devel, OFTC, 2016-02-12
On 26.09.2016 20:37, Alex Bligh wrote:
On 26 Sep 2016, at 19:15, Wouter Verhelst <w@uter.be> wrote:
TLDR: nbdkit is correct, nbd-tester-client is not, patches welcome. I have pushed a fix.
Thank you, you beat me to it by a few minutes. Will test ASAP. Regards, Carl-Daniel
participants (3)
-
Alex Bligh -
Carl-Daniel Hailfinger -
Wouter Verhelst