1. Patchew
  2. QEMU
  3. [PATCH v3 0/7] block: file-posix queue
  4. View patch

[PATCH v3 7/7] block: detect DKIOCGETBLOCKCOUNT/SIZE before use

Paolo Bonzini posted 7 patches 4 years, 8 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Eric Blake <eblake@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Markus Armbruster <armbru@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Max Reitz <mreitz@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH v3 7/7] block: detect DKIOCGETBLOCKCOUNT/SIZE before use
Posted by Paolo Bonzini 4 years, 8 months ago 
From: Joelle van Dyne <j@getutm.app>

iOS hosts do not have these defined so we fallback to the
default behaviour.

Co-authored-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Joelle van Dyne <j@getutm.app>
Message-Id: <20210315180341.31638-4-j@getutm.app>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/file-posix.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 5821e1afed..4e2f7cf508 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2322,8 +2322,11 @@ 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))
+        if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)&size)) {
+            size = 0;
+        }
 #elif defined(DIOCGPART)
         {
                 struct partinfo pi;
@@ -2332,9 +2335,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;
@@ -2342,19 +2343,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:
-- 
2.31.1
Re: [PATCH v3 7/7] block: detect DKIOCGETBLOCKCOUNT/SIZE before use
Posted by Max Reitz 4 years, 7 months ago 
On 03.06.21 15:37, Paolo Bonzini wrote:
> From: Joelle van Dyne <j@getutm.app>
>
> iOS hosts do not have these defined so we fallback to the
> default behaviour.
>
> Co-authored-by: Warner Losh <imp@bsdimp.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Joelle van Dyne <j@getutm.app>
> Message-Id: <20210315180341.31638-4-j@getutm.app>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/file-posix.c | 21 +++++++++------------
>   1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 5821e1afed..4e2f7cf508 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2322,8 +2322,11 @@ 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))
> +        if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)&size)) {

Pre-existing, but I feel compelled to express my unease about this cast.

> +            size = 0;
> +        }
>   #elif defined(DIOCGPART)
>           {
>                   struct partinfo pi;
> @@ -2332,9 +2335,7 @@ again:
>                   else
>                           size = 0;
>           }
> -        if (size == 0)
> -#endif
> -#if defined(__APPLE__) && defined(__MACH__)
> +#elif defined(DKIOCGETBLOCKCOUNT) && defined(DKIOCGETBLOCKSIZE)

As far a I can tell, before this patch, if the DIOCGMEDIASIZE ioctl 
failed, we fell back to this DKIOCGETBLOCKCOUNT/SIZE block (if compiled 
in). Now this is an #elif and so will not be used if DIOCGMEDIASIZE was 
defined. Is that intentional?

This may be fine, and apart from that, this patch looks good to me, but 
this change in behavior wasn’t mentioned in the commit message, hence me 
asking.

>           {
>               uint64_t sectors = 0;
>               uint32_t sector_size = 0;
> @@ -2342,19 +2343,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:
Re: [PATCH v3 7/7] block: detect DKIOCGETBLOCKCOUNT/SIZE before use
Posted by Daniel P. Berrangé 4 years, 7 months ago 
On Tue, Jun 15, 2021 at 06:50:57PM +0200, Max Reitz wrote:
> On 03.06.21 15:37, Paolo Bonzini wrote:
> > From: Joelle van Dyne <j@getutm.app>
> > 
> > iOS hosts do not have these defined so we fallback to the
> > default behaviour.
> > 
> > Co-authored-by: Warner Losh <imp@bsdimp.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Joelle van Dyne <j@getutm.app>
> > Message-Id: <20210315180341.31638-4-j@getutm.app>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   block/file-posix.c | 21 +++++++++------------
> >   1 file changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 5821e1afed..4e2f7cf508 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2322,8 +2322,11 @@ 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))
> > +        if (ioctl(fd, DIOCGMEDIASIZE, (off_t *)&size)) {
> 
> Pre-existing, but I feel compelled to express my unease about this cast.

We set -D_FILE_OFFSET_BITS=64, so IIUC,  off_t should be 64-bits
on both 32 and 64 bit build hosts. IIUC, it is defined to be a
signed integer.  So while off_t may not have the same typedef
as int64_t, it should be the same size and signedness. I expect
we have other code with this same assumption about off-t/int64_t
interchangeability.

We could assert sizeof(int64_t) == sizeof(off_t) in a header
somewhere if we want to be super paranoid.

> 
> > +            size = 0;
> > +        }
> >   #elif defined(DIOCGPART)
> >           {
> >                   struct partinfo pi;
> > @@ -2332,9 +2335,7 @@ again:
> >                   else
> >                           size = 0;
> >           }
> > -        if (size == 0)
> > -#endif
> > -#if defined(__APPLE__) && defined(__MACH__)
> > +#elif defined(DKIOCGETBLOCKCOUNT) && defined(DKIOCGETBLOCKSIZE)
> 
> As far a I can tell, before this patch, if the DIOCGMEDIASIZE ioctl failed,
> we fell back to this DKIOCGETBLOCKCOUNT/SIZE block (if compiled in). Now
> this is an #elif and so will not be used if DIOCGMEDIASIZE was defined. Is
> that intentional?
> 
> This may be fine, and apart from that, this patch looks good to me, but this
> change in behavior wasn’t mentioned in the commit message, hence me asking.
> 
> >           {
> >               uint64_t sectors = 0;
> >               uint32_t sector_size = 0;
> > @@ -2342,19 +2343,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:
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.