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