Libguestfs - Sep 2016 - Re: [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