6:32 p.m.
On 8/30/19 6:10 PM, Eric Blake wrote:
On 8/30/19 1:00 PM, Vladimir Sementsov-Ogievskiy wrote:
> 23.08.2019 17:37, Eric Blake wrote:
>> When creating a read-only image, we are still advertising support for
>> TRIM and WRITE_ZEROES to the client, even though the client should not
>> be issuing those commands. But seeing this requires looking across
>> multiple functions:
>>
>> @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient
*client,
>> return -EINVAL;
>> }
>>
>> - trace_nbd_negotiate_new_style_size_flags(client->exp->size,
>> - client->exp->nbdflags |
myflags);
>> + myflags = client->exp->nbdflags;
>> + if (client->structured_reply) {
>> + myflags |= NBD_FLAG_SEND_DF;
>> + }
>
>
> why we cant do just
> client->exp->nbdflags |= NBD_FLAG_SEND_DF ?
Because myflags is the runtime flags for _this_ client, while
client->exp->nbdflags are the base flags shared by _all_ clients. If
client A requests structured reply, but client B does not, then we don't
want to advertise DF to client B; but amending client->exp->nbdflags
would have that effect.
I stand corrected - it looks like a fresh client->exp is created per
client, as evidenced by:
diff --git i/nbd/client.c w/nbd/client.c
index b9dc829175f9..9e05f1a0e2a3 100644
--- i/nbd/client.c
+++ w/nbd/client.c
@@ -1011,6 +1011,8 @@ int nbd_receive_negotiate(AioContext *aio_context,
QIOChannel *ioc,
assert(info->name);
trace_nbd_receive_negotiate_name(info->name);
+ if (getenv ("MY_HACK"))
+ info->structured_reply = false;
result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname,
outioc,
info->structured_reply, &zeroes, errp);
diff --git i/nbd/server.c w/nbd/server.c
index d5078f7468af..6f3a83704fb3 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -457,6 +457,7 @@ static int
nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
myflags = client->exp->nbdflags;
if (client->structured_reply) {
myflags |= NBD_FLAG_SEND_DF;
+ client->exp->nbdflags |= NBD_FLAG_SEND_DF;
}
trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags);
stq_be_p(buf, client->exp->size);
$ ./qemu-nbd -r -f raw file -t &
$ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32145@1567207628.519883:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x48f
$ MY_HACK=1 ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32156@1567207630.417815:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x40f
$ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags
nbd://localhost:10809 -c quit
32167@1567207635.202940:nbd_receive_negotiate_size_flags Size is
1049088, export flags 0x48f
The export flags change per client, so I _can_ store into
client->exp->nbdflags. Will do that for v2.
Meanwhile, this points out a missing feature in libnbd - for testing
purposes, it would be really nice to be able to purposefully cripple the
client to NOT request structured replies automatically (default enabled,
but the ability to turn it off is useful for interop testing, as in this
thread). I already recently added a --no-sr flag to nbdkit for a
similar reason (but that's creating a server which refuses to advertise,
where here I want a guest that refuses to ask). Guess I'll be adding a
patch for that, too :)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org