diff mbox

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

Message ID 1473369130-22986-4-git-send-email-jbacik@fb.com
State New, archived
Headers show

Commit Message

Josef Bacik Sept. 8, 2016, 9:12 p.m. UTC
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 <jbacik@fb.com>
---
 drivers/block/nbd.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Joe Perches Sept. 9, 2016, 1:20 a.m. UTC | #1
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 <linux/nbd.h>
>  
> +#define NBD_TIMEDOUT			0
> +#define NBD_DISCONNECT_REQUESTED	1
> +
>  struct nbd_device {
>  	u32 flags;
> +	unsigned long runtime_flags;

Better to use DECLARE_BITMAP

> @@ -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, &nbd->runtime_flags);

And remove the & from runtime_flags here

--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe Sept. 9, 2016, 1:55 p.m. UTC | #2
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 <linux/nbd.h>
>>
>> +#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().
Joe Perches Sept. 9, 2016, 4:04 p.m. UTC | #3
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 <linux/nbd.h>
> > >
> > > +#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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe Sept. 9, 2016, 4:11 p.m. UTC | #4
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 <linux/nbd.h>
>>>>
>>>> +#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.
Joe Perches Sept. 9, 2016, 4:15 p.m. UTC | #5
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe Sept. 9, 2016, 4:20 p.m. UTC | #6
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.
diff mbox

Patch

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 <linux/nbd.h>
 
+#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(&nbd->sock_lock, flags);
 
-	nbd->timedout = true;
+	set_bit(NBD_TIMEDOUT, &nbd->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, &nbd->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, &nbd->runtime_flags))
 			error = 0;
-		if (nbd->timedout)
+		if (test_bit(NBD_TIMEDOUT, &nbd->runtime_flags))
 			error = -ETIMEDOUT;
 
 		nbd_reset(nbd);