On Tue, Jan 26, 2021 at 12:08 AM Philippe Mathieu-Daudé <phi...@redhat.com> wrote:
> On 1/26/21 6:55 AM, Joelle van Dyne wrote: > > Previously, the only case where sys/disk.h does not exist is on > > platforms that define __DragonFly__. However, iOS also does not have > > this header. Previously, I had it as > > > > #if defined(__DragonFly__) || defined(CONFIG_IOS) > > > > But there was a code review comment that we should use feature flags > > instead of platform defines. So I added the HAS_SYS_DISK_H flag. > > On technical lists, it's best to avoid top-posting, and to > instead reply inline to make the conversation easier to follow. > > > > > -j > > > > On Mon, Jan 25, 2021 at 8:35 PM Warner Losh <i...@bsdimp.com> wrote: > >> > >> > >> > >> On Mon, Jan 25, 2021 at 6:33 PM Joelle van Dyne <j...@getutm.app> wrote: > >>> > >>> Some BSD platforms do not have this header. > >>> > >>> Signed-off-by: Joelle van Dyne <j...@getutm.app> > >>> --- > >>> meson.build | 1 + > >>> block.c | 2 +- > >>> block/file-posix.c | 2 +- > >>> 3 files changed, 3 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/meson.build b/meson.build > >>> index 27110075df..6818d97df5 100644 > >>> --- a/meson.build > >>> +++ b/meson.build > >>> @@ -1117,6 +1117,7 @@ config_host_data.set('HAVE_PTY_H', > cc.has_header('pty.h')) > >>> config_host_data.set('HAVE_SYS_IOCCOM_H', > cc.has_header('sys/ioccom.h')) > >>> config_host_data.set('HAVE_SYS_KCOV_H', cc.has_header('sys/kcov.h')) > >>> config_host_data.set('HAVE_HOST_BLOCK_DEVICE', have_host_block_device) > >>> +config_host_data.set('HAVE_SYS_DISK_H', cc.has_header('sys/disk.h')) > >>> > >>> ignored = ['CONFIG_QEMU_INTERP_PREFIX'] # actually per-target > >>> arrays = ['CONFIG_AUDIO_DRIVERS', 'CONFIG_BDRV_RW_WHITELIST', > 'CONFIG_BDRV_RO_WHITELIST'] > >>> diff --git a/block.c b/block.c > >>> index 8b9d457546..c4cf391dea 100644 > >>> --- a/block.c > >>> +++ b/block.c > >>> @@ -54,7 +54,7 @@ > >>> #ifdef CONFIG_BSD > >>> #include <sys/ioctl.h> > >>> #include <sys/queue.h> > >>> -#ifndef __DragonFly__ > >>> +#if defined(HAVE_SYS_DISK_H) > >>> #include <sys/disk.h> > >>> #endif > >>> #endif > >>> diff --git a/block/file-posix.c b/block/file-posix.c > >>> index 11d2021346..666d3e7504 100644 > >>> --- a/block/file-posix.c > >>> +++ b/block/file-posix.c > >>> @@ -2320,7 +2320,7 @@ again: > >>> } > >>> if (size == 0) > >>> #endif > >>> -#if defined(__APPLE__) && defined(__MACH__) > >>> +#if defined(HAVE_SYS_DISK_H) && defined(__APPLE__) && > defined(__MACH__) > >> > >> > >> Why is this needed? __DragonFly__ doesn't define either __APPLE__ or > __MACH__ > > Hmm we could also add: > > config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT', cc.compiles(...)) > > Then this block would be easier to read: > > #if defined(HAVE_DKIOCGETBLOCKCOUNT) > ... > > (Maybe this is what Warner meant?) > Close. I'd test it more directly since DKIOCGETBLOCKCOUNT is already a #define, and is unlikely to change... When I saw Joelle's response, I realized I'd been needlessly cryptic in my comments, so posted what I had in mind for cleanup. I'm not sure if the norms of qemu code reviews would say my suggestion was too big to be in scope, or not. Warner >> > >> Warner > >> > >>> > >>> { > >>> uint64_t sectors = 0; > >>> uint32_t sector_size = 0; > >>> -- > >>> 2.28.0 > >>> > >>> > > > >