Message ID | 1490034784-28177-1-git-send-email-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
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, §or_size) >= 0) { > - *sector_size_p = sector_size; > - success = true; > - } > + BLKSSZGET, > #endif > #ifdef DKIOCGETBLOCKSIZE > - if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { > - *sector_size_p = sector_size; > - success = true; > - } > + DKIOCGETBLOCKSIZE, > #endif > #ifdef DIOCGSECTORSIZE > - if (ioctl(fd, DIOCGSECTORSIZE, §or_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], §or_size) >= 0) { > + *sector_size_p = sector_size; > + success = true; > + } > + } > > return success ? 0 : -errno; > } > -- > 2.7.4 > > Reviewed-by: Jeff Cody <jcody@redhat.com>
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, §or_size) >= 0) { >> - *sector_size_p = sector_size; >> - success = true; >> - } >> + BLKSSZGET, >> #endif >> #ifdef DKIOCGETBLOCKSIZE >> - if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { >> - *sector_size_p = sector_size; >> - success = true; >> - } >> + DKIOCGETBLOCKSIZE, >> #endif >> #ifdef DIOCGSECTORSIZE >> - if (ioctl(fd, DIOCGSECTORSIZE, §or_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], §or_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>
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, §or_size) >= 0) { >>> - *sector_size_p = sector_size; >>> - success = true; >>> - } >>> + BLKSSZGET, >>> #endif >>> #ifdef DKIOCGETBLOCKSIZE >>> - if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { >>> - *sector_size_p = sector_size; >>> - success = true; >>> - } >>> + DKIOCGETBLOCKSIZE, >>> #endif >>> #ifdef DIOCGSECTORSIZE >>> - if (ioctl(fd, DIOCGSECTORSIZE, §or_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], §or_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>
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, §or_size) >= 0) { > - *sector_size_p = sector_size; > - success = true; > - } > + BLKSSZGET, > #endif > #ifdef DKIOCGETBLOCKSIZE > - if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { > - *sector_size_p = sector_size; > - success = true; > - } > + DKIOCGETBLOCKSIZE, > #endif > #ifdef DIOCGSECTORSIZE > - if (ioctl(fd, DIOCGSECTORSIZE, §or_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], §or_size) >= 0) { > + *sector_size_p = sector_size; > + success = true; > + } > + } > > return success ? 0 : -errno; > } >
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, §or_size) >= 0) { - *sector_size_p = sector_size; - success = true; - } + BLKSSZGET, #endif #ifdef DKIOCGETBLOCKSIZE - if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { - *sector_size_p = sector_size; - success = true; - } + DKIOCGETBLOCKSIZE, #endif #ifdef DIOCGSECTORSIZE - if (ioctl(fd, DIOCGSECTORSIZE, §or_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], §or_size) >= 0) { + *sector_size_p = sector_size; + success = true; + } + } return success ? 0 : -errno; }
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