| Message ID | 20210126012457.39046-4-j@getutm.app (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iOS and Apple Silicon host support | expand |
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__ Warner > { > uint64_t sectors = 0; > uint32_t sector_size = 0; > -- > 2.28.0 > > >
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. -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__ > > Warner > >> >> { >> uint64_t sectors = 0; >> uint32_t sector_size = 0; >> -- >> 2.28.0 >> >>
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(...)) 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 >>> >>> >
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, §ors) == 0 && ioctl(fd, DKIOCGETBLOCKSIZE, §or_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 > >> > >> >
On Tue, Jan 26, 2021 at 12:08 AM Philippe Mathieu-Daudé <philmd@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 <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(...)) > > 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 > >>> > >>> > > > >
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 >>>> >>>> >> >
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__) { uint64_t sectors = 0; uint32_t sector_size = 0;
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(-)