Re: [PATCH 1/3] block/file-posix: add raw_getlength_fd

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4958 bytes --]

On 30.01.20 16:22, Vladimir Sementsov-Ogievskiy wrote:
> Add function which can handle separate fd, to be called from
> raw_probe_alignment in the following commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/file-posix.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 1b805bd938..7f366046c2 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -178,7 +178,21 @@ typedef struct BDRVRawReopenState {
>  } BDRVRawReopenState;
>  
>  static int fd_open(BlockDriverState *bs);
> -static int64_t raw_getlength(BlockDriverState *bs);
> +static int64_t raw_getlength_fd(BlockDriverState *bs, int fd);
> +
> +static int64_t raw_getlength(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    int ret;
> +
> +    ret = fd_open(bs);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return raw_getlength_fd(bs, s->fd);
> +}
> +
>  
>  typedef struct RawPosixAIOData {
>      BlockDriverState *bs;
> @@ -2063,10 +2077,8 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
>  }
>  
>  #ifdef __OpenBSD__
> -static int64_t raw_getlength(BlockDriverState *bs)
> +static int64_t raw_getlength_fd(BlockDriverState *bs, int fd)
>  {
> -    BDRVRawState *s = bs->opaque;
> -    int fd = s->fd;
>      struct stat st;
>  
>      if (fstat(fd, &st))
> @@ -2082,10 +2094,8 @@ static int64_t raw_getlength(BlockDriverState *bs)
>          return st.st_size;
>  }
>  #elif defined(__NetBSD__)
> -static int64_t raw_getlength(BlockDriverState *bs)
> +static int64_t raw_getlength_fd(BlockDriverState *bs, int fd)
>  {
> -    BDRVRawState *s = bs->opaque;
> -    int fd = s->fd;
>      struct stat st;
>  
>      if (fstat(fd, &st))
> @@ -2107,22 +2117,16 @@ static int64_t raw_getlength(BlockDriverState *bs)
>          return st.st_size;
>  }
>  #elif defined(__sun__)
> -static int64_t raw_getlength(BlockDriverState *bs)
> +static int64_t raw_getlength_fd(BlockDriverState *bs, int fd)
>  {
> -    BDRVRawState *s = bs->opaque;
>      struct dk_minfo minfo;
>      int ret;
>      int64_t size;
>  
> -    ret = fd_open(bs);
> -    if (ret < 0) {
> -        return ret;
> -    }
> -
>      /*
>       * Use the DKIOCGMEDIAINFO ioctl to read the size.
>       */
> -    ret = ioctl(s->fd, DKIOCGMEDIAINFO, &minfo);
> +    ret = ioctl(fd, DKIOCGMEDIAINFO, &minfo);
>      if (ret != -1) {
>          return minfo.dki_lbsize * minfo.dki_capacity;
>      }
> @@ -2131,17 +2135,16 @@ static int64_t raw_getlength(BlockDriverState *bs)
>       * There are reports that lseek on some devices fails, but
>       * irc discussion said that contingency on contingency was overkill.
>       */
> -    size = lseek(s->fd, 0, SEEK_END);
> +    size = lseek(fd, 0, SEEK_END);
>      if (size < 0) {
>          return -errno;
>      }
>      return size;
>  }
>  #elif defined(CONFIG_BSD)
> -static int64_t raw_getlength(BlockDriverState *bs)
> +static int64_t raw_getlength_fd(BlockDriverState *bs, int fd)
>  {
>      BDRVRawState *s = bs->opaque;

I think we should also drop this, and the fd_open() call in this function.

> -    int fd = s->fd;
>      int64_t size;
>      struct stat sb;
>  #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
> @@ -2212,9 +2215,8 @@ again:
>      return size;
>  }
>  #else
> -static int64_t raw_getlength(BlockDriverState *bs)
> +static int64_t raw_getlength_fd(BlockDriverState *bs, int fd)
>  {
> -    BDRVRawState *s = bs->opaque;
>      int ret;
>      int64_t size;
>  
> @@ -2223,7 +2225,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
>          return ret;
>      }

And we can drop the fd_open() call here, too.

>  
> -    size = lseek(s->fd, 0, SEEK_END);
> +    size = lseek(fd, 0, SEEK_END);
>      if (size < 0) {
>          return -errno;
>      }
> 

If we drop those fd_open() calls, there is only one reason we’d still
need the @bs parameter anyway, and that’s the CD-ROM handling code for
FreeBSD.  Speaking of which: That code is broken after this patch
because cdrom_reopen() will change s->fd, which, after this patch, is
completely ignored by raw_getlength_fd().

So I don’t know.  Should that CD handling code set “fd = s->fd” after
cdrom_reopen()?  Or should we just drop it, because I actually can’t
imagine it to be that important or used ever?  (I suppose it might do
something when you change the physical disk while qemu is running and
the device file is attached to qemu, but I’m not sure whether such
passthrough things even work anymore.)

If we dropped it, we could drop the @bs parameter from
raw_getlength_fd() altogether.  I don’t know whether that would actually
have any benefit in practice, though.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]