Re: [PATCH 3/5] nbd: use flags instead of bool

[Jens Axboe]

On 09/09/2016 10:15 AM, Joe Perches wrote:

On Fri, 2016-09-09 at 10:11 -0600, Jens Axboe wrote:

The variable is called 'runtime_flags' - if that doesn't already tell
the reader how it's used, then I'd suggest the reader go read something
else.

I'm all for using established APIs where it makes sense. Declaring a
bitmap for a few fields isn't that.


Deviating from established APIs makes no sense.


Look, doing set/test/clear bit on an unsigned long is a very well
established API. We've been doing that _forever_. As I said in my
original email, I have nothing against using bitmaps _where they make
sense_. Manually allocating an array for X number of bits (where X >
32), yes, by all means use the bitmap helpers.

This is my final reply on this topic.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] nbd: use flags instead of bool

[Joe Perches]

On Fri, 2016-09-09 at 10:11 -0600, Jens Axboe wrote:
> The variable is called 'runtime_flags' - if that doesn't already tell
> the reader how it's used, then I'd suggest the reader go read something
> else.
> 
> I'm all for using established APIs where it makes sense. Declaring a
> bitmap for a few fields isn't that.

Deviating from established APIs makes no sense.


--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] nbd: use flags instead of bool

[Jens Axboe]

On 09/09/2016 10:04 AM, Joe Perches wrote:

On Fri, 2016-09-09 at 07:55 -0600, Jens Axboe wrote:

On 09/08/2016 07:20 PM, Joe Perches wrote:

On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote:

In preparation for some future changes, change a few of the state bools over to
normal bits to set/clear properly.

[]

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c

[]

@@ -41,8 +41,12 @@

 #include 

+#define NBD_TIMEDOUT   0
+#define NBD_DISCONNECT_REQUESTED   1
+
 struct nbd_device {
u32 flags;

+   unsigned long runtime_flags;
Better to use DECLARE_BITMAP



It's a few flags, we know it fits in a long. There's no point to using
anything but that, and set/test/clear_bit().


It lets the reader know how it's used.


The variable is called 'runtime_flags' - if that doesn't already tell
the reader how it's used, then I'd suggest the reader go read something
else.

I'm all for using established APIs where it makes sense. Declaring a
bitmap for a few fields isn't that.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] nbd: use flags instead of bool

[Joe Perches]

On Fri, 2016-09-09 at 07:55 -0600, Jens Axboe wrote:
> On 09/08/2016 07:20 PM, Joe Perches wrote:
> > On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote:
> > > In preparation for some future changes, change a few of the state bools 
> > > over to
> > > normal bits to set/clear properly.
> > []
> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > []
> > > @@ -41,8 +41,12 @@
> > >
> > > #include 
> > >
> > > +#define NBD_TIMEDOUT 0
> > > +#define NBD_DISCONNECT_REQUESTED 1
> > > +
> > > struct nbd_device {
> > >   u32 flags;
> > +   unsigned long runtime_flags;
> > Better to use DECLARE_BITMAP

> It's a few flags, we know it fits in a long. There's no point to using
> anything but that, and set/test/clear_bit().

It lets the reader know how it's used.


--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] nbd: use flags instead of bool

[Jens Axboe]

On 09/08/2016 07:20 PM, Joe Perches wrote:

On Thu, 2016-09-08 at 17:12 -0400, Josef Bacik wrote:

In preparation for some future changes, change a few of the state bools over to
normal bits to set/clear properly.

[]

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c

[]

@@ -41,8 +41,12 @@

 #include 

+#define NBD_TIMEDOUT   0
+#define NBD_DISCONNECT_REQUESTED   1
+
 struct nbd_device {
u32 flags;
+   unsigned long runtime_flags;


Better to use DECLARE_BITMAP


It's a few flags, we know it fits in a long. There's no point to using
anything but that, and set/test/clear_bit().

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nbd: use flags instead of bool

[Josef Bacik]

In preparation for some future changes, change a few of the state bools over to
normal bits to set/clear properly.

Signed-off-by: Josef Bacik 
---
 drivers/block/nbd.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 4b7d0f3..cf855a1 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -41,8 +41,12 @@
 
 #include 
 
+#define NBD_TIMEDOUT   0
+#define NBD_DISCONNECT_REQUESTED   1
+
 struct nbd_device {
u32 flags;
+   unsigned long runtime_flags;
struct socket * sock;   /* If == NULL, device is not ready, yet */
int magic;
 
@@ -54,8 +58,6 @@ struct nbd_device {
int blksize;
loff_t bytesize;
int xmit_timeout;
-   bool timedout;
-   bool disconnect; /* a disconnect has been requested by user */
 
struct timer_list timeout_timer;
/* protects initialization and shutdown of the socket */
@@ -192,7 +194,7 @@ static void nbd_xmit_timeout(unsigned long arg)
 
spin_lock_irqsave(>sock_lock, flags);
 
-   nbd->timedout = true;
+   set_bit(NBD_TIMEDOUT, >runtime_flags);
 
if (nbd->sock) {
sock = nbd->sock;
@@ -562,8 +564,7 @@ out:
 /* Reset all properties of an NBD device */
 static void nbd_reset(struct nbd_device *nbd)
 {
-   nbd->disconnect = false;
-   nbd->timedout = false;
+   nbd->runtime_flags = 0;
nbd->blksize = 1024;
nbd->bytesize = 0;
set_capacity(nbd->disk, 0);
@@ -626,7 +627,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
nbd_device *nbd,
return -EINVAL;
}
 
-   nbd->disconnect = true;
+   set_bit(NBD_DISCONNECT_REQUESTED, >runtime_flags);
 
nbd_send_cmd(nbd, blk_mq_rq_to_pdu(sreq));
blk_mq_free_request(sreq);
@@ -706,9 +707,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct 
nbd_device *nbd,
kill_bdev(bdev);
nbd_bdev_reset(bdev);
 
-   if (nbd->disconnect) /* user requested, ignore socket errors */
+   /* user requested, ignore socket errors */
+   if (test_bit(NBD_DISCONNECT_REQUESTED, >runtime_flags))
error = 0;
-   if (nbd->timedout)
+   if (test_bit(NBD_TIMEDOUT, >runtime_flags))
error = -ETIMEDOUT;
 
nbd_reset(nbd);
-- 
2.8.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html