[PATCH] nbd: Zero from and len fields in NBD_CMD_DISCONNECT.

[PATCH] nbd: Zero from and len fields in NBD_CMD_DISCONNECT.

[Hani Benhabiles]

Len field is already set to zero, but not the from field which is sent as
0xfffffffffffffe00. This makes no sense, and may cause confuse server
implementations doing sanity checks (qemu-nbd is an example.)

Signed-off-by: Hani Benhabiles <hani@linux.com>
---
 drivers/block/nbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 3a70ea2..657bdac 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -246,7 +246,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
 	request.magic = htonl(NBD_REQUEST_MAGIC);
 	request.type = htonl(nbd_cmd(req));
 
-	if (nbd_cmd(req) == NBD_CMD_FLUSH) {
+	if (nbd_cmd(req) == NBD_CMD_FLUSH || nbd_cmd(req) == NBD_CMD_DISC) {
 		/* Other values are reserved for FLUSH requests.  */
 		request.from = 0;
 		request.len = 0;
-- 
1.8.3.2

Re: [PATCH] nbd: Zero from and len fields in NBD_CMD_DISCONNECT.

[Hani Benhabiles]

On Fri, May 16, 2014 at 10:56:33PM -0400, Paul Clements wrote:
> Agreed. But better yet, the request structure should just be zeroed when
> it's allocated.
> 

It is already initialized  in __nbd_ioctl() with the blk_rq_init() call which
sets the __sector value to -1 (which is 0xfffffffffffffe00 after the left shifts.)

This is the only (non-ugly / non-intrusive) way to do it afaict.

> --
> Paul
> 
> 
> On Fri, May 16, 2014 at 7:43 PM, Hani Benhabiles <kroosec@gmail.com> wrote:
> 
> > Len field is already set to zero, but not the from field which is sent as
> > 0xfffffffffffffe00. This makes no sense, and may cause confuse server
> > implementations doing sanity checks (qemu-nbd is an example.)
> >
> > Signed-off-by: Hani Benhabiles <hani@linux.com>
> > ---
> >  drivers/block/nbd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 3a70ea2..657bdac 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -246,7 +246,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct
> > request *req)
> >         request.magic = htonl(NBD_REQUEST_MAGIC);
> >         request.type = htonl(nbd_cmd(req));
> >
> > -       if (nbd_cmd(req) == NBD_CMD_FLUSH) {
> > +       if (nbd_cmd(req) == NBD_CMD_FLUSH || nbd_cmd(req) == NBD_CMD_DISC)
> > {
> >                 /* Other values are reserved for FLUSH requests.  */
> >                 request.from = 0;
> >                 request.len = 0;
> > --
> > 1.8.3.2
> >
> >

Re: [PATCH] nbd: Zero from and len fields in NBD_CMD_DISCONNECT.

[Hani Benhabiles]

On Sun, May 18, 2014 at 10:11:13AM +0100, Hani Benhabiles wrote:
> On Fri, May 16, 2014 at 10:56:33PM -0400, Paul Clements wrote:
> > Agreed. But better yet, the request structure should just be zeroed when
> > it's allocated.
> > 
> 
> It is already initialized  in __nbd_ioctl() with the blk_rq_init() call which
> sets the __sector value to -1 (which is 0xfffffffffffffe00 after the left shifts.)
> 
> This is the only (non-ugly / non-intrusive) way to do it afaict.
> 

Ping!

Anything blocking this patch ?

Thanks.

> > --
> > Paul
> > 
> > 
> > On Fri, May 16, 2014 at 7:43 PM, Hani Benhabiles <kroosec@gmail.com> wrote:
> > 
> > > Len field is already set to zero, but not the from field which is sent as
> > > 0xfffffffffffffe00. This makes no sense, and may cause confuse server
> > > implementations doing sanity checks (qemu-nbd is an example.)
> > >
> > > Signed-off-by: Hani Benhabiles <hani@linux.com>
> > > ---
> > >  drivers/block/nbd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > index 3a70ea2..657bdac 100644
> > > --- a/drivers/block/nbd.c
> > > +++ b/drivers/block/nbd.c
> > > @@ -246,7 +246,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct
> > > request *req)
> > >         request.magic = htonl(NBD_REQUEST_MAGIC);
> > >         request.type = htonl(nbd_cmd(req));
> > >
> > > -       if (nbd_cmd(req) == NBD_CMD_FLUSH) {
> > > +       if (nbd_cmd(req) == NBD_CMD_FLUSH || nbd_cmd(req) == NBD_CMD_DISC)
> > > {
> > >                 /* Other values are reserved for FLUSH requests.  */
> > >                 request.from = 0;
> > >                 request.len = 0;
> > > --
> > > 1.8.3.2
> > >
> > >

Re: [PATCH] nbd: Zero from and len fields in NBD_CMD_DISCONNECT.

[Paul Clements]

On Sun, May 25, 2014 at 6:18 AM, Hani Benhabiles <kroosec@gmail.com> wrote:
> On Sun, May 18, 2014 at 10:11:13AM +0100, Hani Benhabiles wrote:
>> On Fri, May 16, 2014 at 10:56:33PM -0400, Paul Clements wrote:
>> > Agreed. But better yet, the request structure should just be zeroed when
>> > it's allocated.
>> >
>>
>> It is already initialized  in __nbd_ioctl() with the blk_rq_init() call which
>> sets the __sector value to -1 (which is 0xfffffffffffffe00 after the left shifts.)
>>
>> This is the only (non-ugly / non-intrusive) way to do it afaict.
>>
>
> Ping!
>
> Anything blocking this patch ?

It's cleaner to just zero the struct and get rid of the conditional
zeroing of specific fields. I'll prepare a patch in the next few days.

Thanks,
Paul

>> > On Fri, May 16, 2014 at 7:43 PM, Hani Benhabiles <kroosec@gmail.com> wrote:
>> >
>> > > Len field is already set to zero, but not the from field which is sent as
>> > > 0xfffffffffffffe00. This makes no sense, and may cause confuse server
>> > > implementations doing sanity checks (qemu-nbd is an example.)
>> > >
>> > > Signed-off-by: Hani Benhabiles <hani@linux.com>
>> > > ---
>> > >  drivers/block/nbd.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > > index 3a70ea2..657bdac 100644
>> > > --- a/drivers/block/nbd.c
>> > > +++ b/drivers/block/nbd.c
>> > > @@ -246,7 +246,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct
>> > > request *req)
>> > >         request.magic = htonl(NBD_REQUEST_MAGIC);
>> > >         request.type = htonl(nbd_cmd(req));
>> > >
>> > > -       if (nbd_cmd(req) == NBD_CMD_FLUSH) {
>> > > +       if (nbd_cmd(req) == NBD_CMD_FLUSH || nbd_cmd(req) == NBD_CMD_DISC)
>> > > {
>> > >                 /* Other values are reserved for FLUSH requests.  */
>> > >                 request.from = 0;
>> > >                 request.len = 0;
>> > > --
>> > > 1.8.3.2
>> > >
>> > >

Re: [PATCH] nbd: Zero from and len fields in NBD_CMD_DISCONNECT.

[Hani Benhabiles]

On Sun, May 25, 2014 at 02:11:13PM -0400, Paul Clements wrote:
> On Sun, May 25, 2014 at 6:18 AM, Hani Benhabiles <kroosec@gmail.com> wrote:
> > On Sun, May 18, 2014 at 10:11:13AM +0100, Hani Benhabiles wrote:
> >> On Fri, May 16, 2014 at 10:56:33PM -0400, Paul Clements wrote:
> >> > Agreed. But better yet, the request structure should just be zeroed when
> >> > it's allocated.
> >> >
> >>
> >> It is already initialized  in __nbd_ioctl() with the blk_rq_init() call which
> >> sets the __sector value to -1 (which is 0xfffffffffffffe00 after the left shifts.)
> >>
> >> This is the only (non-ugly / non-intrusive) way to do it afaict.
> >>
> >
> > Ping!
> >
> > Anything blocking this patch ?
> 
> It's cleaner to just zero the struct and get rid of the conditional
> zeroing of specific fields. I'll prepare a patch in the next few days.
> 

Sorry, I misunderstood which struct you were talking about!

Will send a v2 shortly.

> Thanks,
> Paul
> 
> >> > On Fri, May 16, 2014 at 7:43 PM, Hani Benhabiles <kroosec@gmail.com> wrote:
> >> >
> >> > > Len field is already set to zero, but not the from field which is sent as
> >> > > 0xfffffffffffffe00. This makes no sense, and may cause confuse server
> >> > > implementations doing sanity checks (qemu-nbd is an example.)
> >> > >
> >> > > Signed-off-by: Hani Benhabiles <hani@linux.com>
> >> > > ---
> >> > >  drivers/block/nbd.c | 2 +-
> >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> >> > > index 3a70ea2..657bdac 100644
> >> > > --- a/drivers/block/nbd.c
> >> > > +++ b/drivers/block/nbd.c
> >> > > @@ -246,7 +246,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct
> >> > > request *req)
> >> > >         request.magic = htonl(NBD_REQUEST_MAGIC);
> >> > >         request.type = htonl(nbd_cmd(req));
> >> > >
> >> > > -       if (nbd_cmd(req) == NBD_CMD_FLUSH) {
> >> > > +       if (nbd_cmd(req) == NBD_CMD_FLUSH || nbd_cmd(req) == NBD_CMD_DISC)
> >> > > {
> >> > >                 /* Other values are reserved for FLUSH requests.  */
> >> > >                 request.from = 0;
> >> > >                 request.len = 0;
> >> > > --
> >> > > 1.8.3.2
> >> > >
> >> > >