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
> >>>
> >>>
> >
>
>
