Skip to contentSkip to conversation

nbd/server: fix NBD_CMD_CACHE

PATCH In review Vladimir Sementsov-Ogievskiy 3 participants 5 messages Oct 3, 2018 – Oct 4, 2018

lore
Share & tools
lore

Vladimir Sementsov-Ogievskiy

author
#0 lore raw

PATCH 1/1 nbd/server: fix NBD_CMD_CACHE

We should not go to structured-read branch on CACHE command, fix that.

Bug intoroduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE" with the whole feature and affects 3.0.0 release.

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
---
 nbd/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
nbd/server.c +2 -1
index c3dd402b45..1fba21414b 100644
@@ -2177,7 +2177,8 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
     }
 
     if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) &&
-        request->len) {
+        request->len && request->type != NBD_CMD_CACHE)
+    {
         return nbd_co_send_sparse_read(client, request->handle, request->from,
                                        data, request->len, errp);
     }
Signature · 1 line
2.18.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 3 lines · Vladimir Sementsov-Ogievskiy

We should not go to structured-read branch on CACHE command, fix that.

Bug intoroduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE"

s/intoroduced/introduced/

Quoted 1 line

with the whole feature and affects 3.0.0 release.

Ouch. It's because I don't have an NBD client that can issue the command, so the server side got released without sufficient testing. Is there some way we could enhance qemu-io as NBD client to issue such a command?

Quoted 2 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
CC [email protected]
Reviewed-by Eric Blake <[email protected]>

Will queue through my NBD tree.

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
author
#2 lore raw
Quoted 5 lines · Eric Blake
Quoted 3 lines · Vladimir Sementsov-Ogievskiy

We should not go to structured-read branch on CACHE command, fix that.

Bug intoroduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE"

Quoted 8 lines

s/intoroduced/introduced/

Quoted 1 line

with the whole feature and affects 3.0.0 release.

Ouch. It's because I don't have an NBD client that can issue the command, so the server side got released without sufficient testing. Is there some way we could enhance qemu-io as NBD client to issue such a command?

may be, just add qemu-io command, like x-debug-nbd-cmd, which will just send any nbd command? and prints all server replies? Then we'll be able to write any unit tests on nbd-server. It's not the first time the problem arise..

Quoted 10 lines
Quoted 3 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
CC [email protected]
Reviewed-by Eric Blake <[email protected]>

Will queue through my NBD tree.

Signature · 2 lines
Best regards,
Vladimir
#3 lore raw

Am 03.10.2018 um 17:10 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 15 lines
Quoted 5 lines · Eric Blake
Quoted 3 lines · Vladimir Sementsov-Ogievskiy

We should not go to structured-read branch on CACHE command, fix that.

Bug intoroduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE"

Quoted 8 lines

s/intoroduced/introduced/

Quoted 1 line

with the whole feature and affects 3.0.0 release.

Ouch. It's because I don't have an NBD client that can issue the command, so the server side got released without sufficient testing. Is there some way we could enhance qemu-io as NBD client to issue such a command?

Quoted 4 lines

may be, just add qemu-io command, like x-debug-nbd-cmd, which will just send any nbd command? and prints all server replies? Then we'll be able to write any unit tests on nbd-server. It's not the first time the problem arise..

Shouldn't it be easy to write a simple NBD client in Python and then use that for test cases? I don't see why this needs to be in qemu-io, and testing illegal requests is certainly easier with a custom client.

Kevin

#4 lore raw
Quoted 16 lines · Kevin Wolf

Am 03.10.2018 um 17:10 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 15 lines
Quoted 13 lines · Eric Blake
Quoted 3 lines · Vladimir Sementsov-Ogievskiy

We should not go to structured-read branch on CACHE command, fix that.

Bug intoroduced in bc37b06a5cde24 "nbd/server: introduce NBD_CMD_CACHE"

s/intoroduced/introduced/

Quoted 1 line

with the whole feature and affects 3.0.0 release.

Ouch. It's because I don't have an NBD client that can issue the command, so the server side got released without sufficient testing. Is there some way we could enhance qemu-io as NBD client to issue such a command?

Quoted 8 lines
Quoted 4 lines

may be, just add qemu-io command, like x-debug-nbd-cmd, which will just send any nbd command? and prints all server replies? Then we'll be able to write any unit tests on nbd-server. It's not the first time the problem arise..

Shouldn't it be easy to write a simple NBD client in Python and then use that for test cases? I don't see why this needs to be in qemu-io, and testing illegal requests is certainly easier with a custom client.

Indeed, and we already have tests/qemu-iotests/nbd-fault-injector.py which implements a custom server (unfortunately, it has not been updated to use newstyle yet!), as a reference for implementing a similar custom client.

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org