Re: [PATCH v9 03/11] configure: check for sys/disk.h

[Philippe Mathieu-Daudé]

On 1/26/21 8:08 AM, Philippe Mathieu-Daudé 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 <imp@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(...))

If DKIOCGETBLOCKCOUNT were in a know header, we could use Meson's

  cc.has_header_symbol('header.h', 'DKIOCGETBLOCKCOUNT')

But as the previous hunk shows, sys/disk.h isn't on DragonFlyBSD.

If there were only 2 known headers, you could do:

  config_host_data.set('HAVE_DKIOCGETBLOCKCOUNT',
      cc.has_header_symbol('header.h', 'DKIOCGETBLOCKCOUNT') or
      cc.has_header_symbol('sys/disk.h', 'DKIOCGETBLOCKCOUNT'))

> 
> Then this block would be easier to read:
> 
>   #if defined(HAVE_DKIOCGETBLOCKCOUNT)
>   ...
> 
> (Maybe this is what Warner meant?)
> 
>>>
>>> Warner
>>>
>>>>
>>>>          {
>>>>              uint64_t sectors = 0;
>>>>              uint32_t sector_size = 0;
>>>> --
>>>> 2.28.0
>>>>
>>>>
>>
>