diff mbox

[for-2.9?] block/file-posix.c: Fix unused variable warning on OpenBSD

Message ID 1490034784-28177-1-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell March 20, 2017, 6:33 p.m. UTC 
On OpenBSD none of the ioctls probe_logical_blocksize() tries
exist, so the variable sector_size is unused. Refactor the
code to avoid this (and reduce the duplicated code).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
The alternative would be to move the variable so it was
local to a code block inside each #ifdef, but this seemed
a bit nicer anyway.

Tentatively tagged 'for-2.9' just because this is the only
warning in the OpenBSD build, but I don't insist on it.

I've opted to retain the existing behaviour of "try every
ioctl available and use the last one that works" rather
than "stop as soon as something worked".

---
 block/file-posix.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

-- 
2.7.4

Comments

Jeff Cody March 22, 2017, 1:58 p.m. UTC | #1 
On Mon, Mar 20, 2017 at 06:33:04PM +0000, Peter Maydell wrote:
> On OpenBSD none of the ioctls probe_logical_blocksize() tries

> exist, so the variable sector_size is unused. Refactor the

> code to avoid this (and reduce the duplicated code).

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

> The alternative would be to move the variable so it was

> local to a code block inside each #ifdef, but this seemed

> a bit nicer anyway.

> 

> Tentatively tagged 'for-2.9' just because this is the only

> warning in the OpenBSD build, but I don't insist on it.

> 

> I've opted to retain the existing behaviour of "try every

> ioctl available and use the last one that works" rather

> than "stop as soon as something worked".

> 

> ---

>  block/file-posix.c | 28 ++++++++++++++--------------

>  1 file changed, 14 insertions(+), 14 deletions(-)

> 

> diff --git a/block/file-posix.c b/block/file-posix.c

> index 53febd3..b980d23 100644

> --- a/block/file-posix.c

> +++ b/block/file-posix.c

> @@ -219,28 +219,28 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)

>  {

>      unsigned int sector_size;

>      bool success = false;

> +    int i;

>  

>      errno = ENOTSUP;

> -

> -    /* Try a few ioctls to get the right size */

> +    unsigned long ioctl_list[] = {

>  #ifdef BLKSSZGET

> -    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {

> -        *sector_size_p = sector_size;

> -        success = true;

> -    }

> +        BLKSSZGET,

>  #endif

>  #ifdef DKIOCGETBLOCKSIZE

> -    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {

> -        *sector_size_p = sector_size;

> -        success = true;

> -    }

> +        DKIOCGETBLOCKSIZE,

>  #endif

>  #ifdef DIOCGSECTORSIZE

> -    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {

> -        *sector_size_p = sector_size;

> -        success = true;

> -    }

> +        DIOCGSECTORSIZE,

>  #endif

> +    };

> +

> +    /* Try a few ioctls to get the right size */

> +    for (i = 0; i < ARRAY_SIZE(ioctl_list); i++) {

> +        if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {

> +            *sector_size_p = sector_size;

> +            success = true;

> +        }

> +    }

>  

>      return success ? 0 : -errno;

>  }

> -- 

> 2.7.4

> 

>


Reviewed-by: Jeff Cody <jcody@redhat.com>
Philippe Mathieu-Daudé March 22, 2017, 4:23 p.m. UTC | #2 
On 03/22/2017 10:58 AM, Jeff Cody wrote:
> On Mon, Mar 20, 2017 at 06:33:04PM +0000, Peter Maydell wrote:

>> On OpenBSD none of the ioctls probe_logical_blocksize() tries

>> exist, so the variable sector_size is unused. Refactor the

>> code to avoid this (and reduce the duplicated code).

>>

>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>> ---

>> The alternative would be to move the variable so it was

>> local to a code block inside each #ifdef, but this seemed

>> a bit nicer anyway.

>>

>> Tentatively tagged 'for-2.9' just because this is the only

>> warning in the OpenBSD build, but I don't insist on it.

>>

>> I've opted to retain the existing behaviour of "try every

>> ioctl available and use the last one that works" rather

>> than "stop as soon as something worked".

>>

>> ---

>>  block/file-posix.c | 28 ++++++++++++++--------------

>>  1 file changed, 14 insertions(+), 14 deletions(-)

>>

>> diff --git a/block/file-posix.c b/block/file-posix.c

>> index 53febd3..b980d23 100644

>> --- a/block/file-posix.c

>> +++ b/block/file-posix.c

>> @@ -219,28 +219,28 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)

>>  {

>>      unsigned int sector_size;

>>      bool success = false;

>> +    int i;

>>

>>      errno = ENOTSUP;

>> -

>> -    /* Try a few ioctls to get the right size */

>> +    unsigned long ioctl_list[] = {


this can be `const` but not a bit win.

>>  #ifdef BLKSSZGET

>> -    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {

>> -        *sector_size_p = sector_size;

>> -        success = true;

>> -    }

>> +        BLKSSZGET,

>>  #endif

>>  #ifdef DKIOCGETBLOCKSIZE

>> -    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {

>> -        *sector_size_p = sector_size;

>> -        success = true;

>> -    }

>> +        DKIOCGETBLOCKSIZE,

>>  #endif

>>  #ifdef DIOCGSECTORSIZE

>> -    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {

>> -        *sector_size_p = sector_size;

>> -        success = true;

>> -    }

>> +        DIOCGSECTORSIZE,

>>  #endif

>> +    };

>> +

>> +    /* Try a few ioctls to get the right size */

>> +    for (i = 0; i < ARRAY_SIZE(ioctl_list); i++) {

>> +        if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {

>> +            *sector_size_p = sector_size;

>> +            success = true;

>> +        }

>> +    }

>>

>>      return success ? 0 : -errno;

>>  }

>> --

>> 2.7.4

>>

>>

>

> Reviewed-by: Jeff Cody <jcody@redhat.com>


Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Philippe Mathieu-Daudé March 22, 2017, 4:25 p.m. UTC | #3 
On 03/22/2017 01:23 PM, Philippe Mathieu-Daudé wrote:
> On 03/22/2017 10:58 AM, Jeff Cody wrote:

>> On Mon, Mar 20, 2017 at 06:33:04PM +0000, Peter Maydell wrote:

>>> On OpenBSD none of the ioctls probe_logical_blocksize() tries

>>> exist, so the variable sector_size is unused. Refactor the

>>> code to avoid this (and reduce the duplicated code).

>>>

>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>>> ---

>>> The alternative would be to move the variable so it was

>>> local to a code block inside each #ifdef, but this seemed

>>> a bit nicer anyway.

>>>

>>> Tentatively tagged 'for-2.9' just because this is the only

>>> warning in the OpenBSD build, but I don't insist on it.

>>>

>>> I've opted to retain the existing behaviour of "try every

>>> ioctl available and use the last one that works" rather

>>> than "stop as soon as something worked".

>>>

>>> ---

>>>  block/file-posix.c | 28 ++++++++++++++--------------

>>>  1 file changed, 14 insertions(+), 14 deletions(-)

>>>

>>> diff --git a/block/file-posix.c b/block/file-posix.c

>>> index 53febd3..b980d23 100644

>>> --- a/block/file-posix.c

>>> +++ b/block/file-posix.c

>>> @@ -219,28 +219,28 @@ static int probe_logical_blocksize(int fd,

>>> unsigned int *sector_size_p)

>>>  {

>>>      unsigned int sector_size;

>>>      bool success = false;

>>> +    int i;

>>>

>>>      errno = ENOTSUP;

>>> -

>>> -    /* Try a few ioctls to get the right size */

>>> +    unsigned long ioctl_list[] = {

>

> this can be `const` but not a bit win.


I mean `static const`, sorry.

>

>>>  #ifdef BLKSSZGET

>>> -    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {

>>> -        *sector_size_p = sector_size;

>>> -        success = true;

>>> -    }

>>> +        BLKSSZGET,

>>>  #endif

>>>  #ifdef DKIOCGETBLOCKSIZE

>>> -    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {

>>> -        *sector_size_p = sector_size;

>>> -        success = true;

>>> -    }

>>> +        DKIOCGETBLOCKSIZE,

>>>  #endif

>>>  #ifdef DIOCGSECTORSIZE

>>> -    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {

>>> -        *sector_size_p = sector_size;

>>> -        success = true;

>>> -    }

>>> +        DIOCGSECTORSIZE,

>>>  #endif

>>> +    };

>>> +

>>> +    /* Try a few ioctls to get the right size */

>>> +    for (i = 0; i < ARRAY_SIZE(ioctl_list); i++) {

>>> +        if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {

>>> +            *sector_size_p = sector_size;

>>> +            success = true;

>>> +        }

>>> +    }

>>>

>>>      return success ? 0 : -errno;

>>>  }

>>> --

>>> 2.7.4

>>>

>>>

>>

>> Reviewed-by: Jeff Cody <jcody@redhat.com>

>

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Max Reitz March 22, 2017, 5:05 p.m. UTC | #4 
On 20.03.2017 19:33, Peter Maydell wrote:
> On OpenBSD none of the ioctls probe_logical_blocksize() tries

> exist, so the variable sector_size is unused. Refactor the

> code to avoid this (and reduce the duplicated code).

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

> The alternative would be to move the variable so it was

> local to a code block inside each #ifdef, but this seemed

> a bit nicer anyway.

> 

> Tentatively tagged 'for-2.9' just because this is the only

> warning in the OpenBSD build, but I don't insist on it.


I'm fine with targeting 2.9.

> I've opted to retain the existing behaviour of "try every

> ioctl available and use the last one that works" rather

> than "stop as soon as something worked".

> 

> ---

>  block/file-posix.c | 28 ++++++++++++++--------------

>  1 file changed, 14 insertions(+), 14 deletions(-)

> 

> diff --git a/block/file-posix.c b/block/file-posix.c

> index 53febd3..b980d23 100644

> --- a/block/file-posix.c

> +++ b/block/file-posix.c

> @@ -219,28 +219,28 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)

>  {

>      unsigned int sector_size;

>      bool success = false;

> +    int i;

>  

>      errno = ENOTSUP;

> -

> -    /* Try a few ioctls to get the right size */

> +    unsigned long ioctl_list[] = {

>  #ifdef BLKSSZGET

> -    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {

> -        *sector_size_p = sector_size;

> -        success = true;

> -    }

> +        BLKSSZGET,

>  #endif

>  #ifdef DKIOCGETBLOCKSIZE

> -    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {

> -        *sector_size_p = sector_size;

> -        success = true;

> -    }

> +        DKIOCGETBLOCKSIZE,

>  #endif

>  #ifdef DIOCGSECTORSIZE

> -    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {

> -        *sector_size_p = sector_size;

> -        success = true;

> -    }

> +        DIOCGSECTORSIZE,

>  #endif

> +    };

> +

> +    /* Try a few ioctls to get the right size */

> +    for (i = 0; i < ARRAY_SIZE(ioctl_list); i++) {


On 32-bit, though:

./block/file-posix.c: In function ‘probe_logical_blocksize’:
./qemu/block/file-posix.c:229:19: error: comparison of unsigned
expression < 0 is always false [-Werror=type-limits]

A cast (int)ARRAY_SIZE() would fix that (I had the same problem once...).

Max

> +        if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {

> +            *sector_size_p = sector_size;

> +            success = true;

> +        }

> +    }

>  

>      return success ? 0 : -errno;

>  }

>
diff mbox

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index 53febd3..b980d23 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -219,28 +219,28 @@  static int probe_logical_blocksize(int fd, unsigned int *sector_size_p)
 {
     unsigned int sector_size;
     bool success = false;
+    int i;
 
     errno = ENOTSUP;
-
-    /* Try a few ioctls to get the right size */
+    unsigned long ioctl_list[] = {
 #ifdef BLKSSZGET
-    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
-        *sector_size_p = sector_size;
-        success = true;
-    }
+        BLKSSZGET,
 #endif
 #ifdef DKIOCGETBLOCKSIZE
-    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
-        *sector_size_p = sector_size;
-        success = true;
-    }
+        DKIOCGETBLOCKSIZE,
 #endif
 #ifdef DIOCGSECTORSIZE
-    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
-        *sector_size_p = sector_size;
-        success = true;
-    }
+        DIOCGSECTORSIZE,
 #endif
+    };
+
+    /* Try a few ioctls to get the right size */
+    for (i = 0; i < ARRAY_SIZE(ioctl_list); i++) {
+        if (ioctl(fd, ioctl_list[i], &sector_size) >= 0) {
+            *sector_size_p = sector_size;
+            success = true;
+        }
+    }
 
     return success ? 0 : -errno;
 }