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 <vsementsov@virtuozzo.com>
---
 nbd/server.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index c3dd402b45..1fba21414b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -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);
     }
-- 
2.18.0

On 10/3/18 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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/

> 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?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

CC: qemu-stable@nongnu.org

Reviewed-by: Eric Blake <eblake@redhat.com>

Will queue through my NBD tree.

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

03.10.2018 17:57, Eric Blake wrote:
> On 10/3/18 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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/
>
>> 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..

>
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> CC: qemu-stable@nongnu.org
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Will queue through my NBD tree.
>


-- 
Best regards,
Vladimir

Am 03.10.2018 um 17:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 03.10.2018 17:57, Eric Blake wrote:
> > On 10/3/18 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> >> 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/
> >
> >> 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..

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

On 10/4/18 8:02 AM, Kevin Wolf wrote:
> Am 03.10.2018 um 17:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 03.10.2018 17:57, Eric Blake wrote:
>>> On 10/3/18 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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/
>>>
>>>> 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..
> 
> 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.

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