[Libguestfs] [Nbd] Testing NBD server implementations for correctness

[Carl-Daniel Hailfinger]

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