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

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 4866 bytes --]

On Mon, Jan 25, 2021 at 10:55 PM Joelle van Dyne <j@getutm.app> 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.
>

Right. I like that the #include is now protected like that. However,
sys/disk.h never was standardized and varies considerably among the systems
that it exists on.


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

Which is why I asked this question...

Let me ask it another way. Why not base this on the
ioctl  DKIOCGETBLOCKCOUNT like the rest of this function? It's simple and
on platforms that don't have that ioctl, it won't be used.  I don't even
know how to read the proposed change logically. If IOS doesn't have this
interface, then you'll need another #else <something-else> to work reliably
anyway, since the seek trick that's used there may or may not work. However
that starts to get kinda nested and twisty.  So maybe something more like
the following would make it clearer... though that might be beyond the
scope of what you're trying to do.

diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d4..704ded68b0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2295,8 +2295,10 @@ static int64_t raw_getlength(BlockDriverState *bs)
 again:
 #endif
     if (!fstat(fd, &sb) && (S_IFCHR & sb.st_mode)) {
+        size = 0;
 #ifdef DIOCGMEDIASIZE
         if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)&size))
+            size = 0;
 #elif defined(DIOCGPART)
         {
                 struct partinfo pi;
@@ -2305,9 +2307,7 @@ again:
                 else
                         size = 0;
         }
-        if (size == 0)
-#endif
-#if defined(__APPLE__) && defined(__MACH__)
+#elif defined(DKIOCGETBLOCKCOUNT) && defined(DKIOCGETBLOCKSIZE)
         {
             uint64_t sectors = 0;
             uint32_t sector_size = 0;
@@ -2315,19 +2315,15 @@ again:
             if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
                && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
                 size = sectors * sector_size;
-            } else {
-                size = lseek(fd, 0LL, SEEK_END);
-                if (size < 0) {
-                    return -errno;
-                }
             }
         }
-#else
-        size = lseek(fd, 0LL, SEEK_END);
+#endif
+        if (size == 0) {
+            size = lseek(fd, 0LL, SEEK_END);
+        }
         if (size < 0) {
             return -errno;
         }
-#endif
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
         switch(s->type) {
         case FTYPE_CD:

Warner

>
> >>
> >>          {
> >>              uint64_t sectors = 0;
> >>              uint32_t sector_size = 0;
> >> --
> >> 2.28.0
> >>
> >>
>

[-- Attachment #2: Type: text/html, Size: 6831 bytes --]