Re: [PATCH v8 02/27] block: Fixes nfs compiling error on msys2/mingw

[Philippe Mathieu-Daudé]

On 9/13/20 6:00 PM, 罗勇刚(Yonggang Luo) wrote:
> 
> 
> On Sun, Sep 13, 2020 at 11:47 PM Philippe Mathieu-Daudé
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> 
>     On 9/13/20 12:44 AM, Yonggang Luo wrote:
>     > These compiling errors are fixed:
>     > ../block/nfs.c:27:10: fatal error: poll.h: No such file or directory
>     >    27 | #include <poll.h>
>     >       |          ^~~~~~~~
>     > compilation terminated.
>     >
>     > ../block/nfs.c:63:5: error: unknown type name 'blkcnt_t'
>     >    63 |     blkcnt_t st_blocks;
>     >       |     ^~~~~~~~
>     > ../block/nfs.c: In function 'nfs_client_open':
>     > ../block/nfs.c:550:27: error: 'struct _stat64' has no member named
>     'st_blocks'
>     >   550 |     client->st_blocks = st.st_blocks;
>     >       |                           ^
>     > ../block/nfs.c: In function 'nfs_get_allocated_file_size':
>     > ../block/nfs.c:751:41: error: 'struct _stat64' has no member named
>     'st_blocks'
>     >   751 |     return (task.ret < 0 ? task.ret : st.st_blocks * 512);
>     >       |                                         ^
>     > ../block/nfs.c: In function 'nfs_reopen_prepare':
>     > ../block/nfs.c:805:31: error: 'struct _stat64' has no member named
>     'st_blocks'
>     >   805 |         client->st_blocks = st.st_blocks;
>     >       |                               ^
>     > ../block/nfs.c: In function 'nfs_get_allocated_file_size':
>     > ../block/nfs.c:752:1: error: control reaches end of non-void
>     function [-Werror=return-type]
>     >   752 | }
>     >       | ^
>     >
>     > On msys2/mingw, there is no st_blocks in struct _stat64, so we use
>     consistence st_size instead.
>     >
>     > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com
>     <mailto:luoyonggang@gmail.com>>
>     > ---
>     >  block/nfs.c | 26 ++++++++++++++++++++++----
>     >  1 file changed, 22 insertions(+), 4 deletions(-)
>     >
>     > diff --git a/block/nfs.c b/block/nfs.c
>     > index 61a249a9fc..98b48f363b 100644
>     > --- a/block/nfs.c
>     > +++ b/block/nfs.c
>     > @@ -24,7 +24,9 @@
>     > 
>     >  #include "qemu/osdep.h"
>     > 
>     > +#if !defined(_WIN32)
>     >  #include <poll.h>
>     > +#endif
>     >  #include "qemu/config-file.h"
>     >  #include "qemu/error-report.h"
>     >  #include "qapi/error.h"
>     > @@ -51,6 +53,12 @@
>     >  #define QEMU_NFS_MAX_PAGECACHE_SIZE (8388608 / NFS_BLKSIZE)
>     >  #define QEMU_NFS_MAX_DEBUG_LEVEL 2
>     > 
>     > +#if defined (_WIN32)
>     > +#define nfs_stat __stat64
>     > +#else
>     > +#define nfs_stat stat
>     > +#endif
>     > +
>     >  typedef struct NFSClient {
>     >      struct nfs_context *context;
>     >      struct nfsfh *fh;
>     > @@ -58,7 +66,9 @@ typedef struct NFSClient {
>     >      bool has_zero_init;
>     >      AioContext *aio_context;
>     >      QemuMutex mutex;
>     > +#if !defined (_WIN32)
>     >      blkcnt_t st_blocks;
> 
>     What about adding a "typedef off_t blkcnt_t;" or
>     similar typedef instead? Then no need to #ifdef
>     the st_blocks uses.
> 
> No, I did that before and someone else have objection 

What objection?

Again I had to look at the archives to find a comment from
Peter.

Maybe can be justified with:

"As st_blocks and st_size are not the same (st_blocks is
the number of allocated blocks on disk, while st_size is
the virtual size of a file as it may contain holes), we can
not easily add a typedef for blkcnt_t.
Anyhow as the get_allocated_file_size() block drive handler
is not mandatory, we can avoid implementing it on WIN32 by
using some #ifdef'ry."

These comments are useful for future developers. Because else
someone might want to improve your patch, add the typedef and
if Peter is not reviewing this, we might miss that again.

> 
> 
>     > +#endif
>     >      bool cache_used;
>     >      NFSServer *server;
>     >      char *path;
>     > @@ -70,7 +80,7 @@ typedef struct NFSRPC {
>     >      int ret;
>     >      int complete;
>     >      QEMUIOVector *iov;
>     > -    struct stat *st;
>     > +    struct nfs_stat *st;
>     >      Coroutine *co;
>     >      NFSClient *client;
>     >  } NFSRPC;
>     > @@ -419,7 +429,7 @@ static int64_t nfs_client_open(NFSClient
>     *client, BlockdevOptionsNfs *opts,
>     >                                 int flags, int open_flags, Error
>     **errp)
>     >  {
>     >      int64_t ret = -EINVAL;
>     > -    struct stat st;
>     > +    struct nfs_stat st;
>     >      char *file = NULL, *strp = NULL;
>     > 
>     >      qemu_mutex_init(&client->mutex);
>     > @@ -545,7 +555,9 @@ static int64_t nfs_client_open(NFSClient
>     *client, BlockdevOptionsNfs *opts,
>     >      }
>     > 
>     >      ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
>     > +#if !defined (_WIN32)
>     >      client->st_blocks = st.st_blocks;
>     > +#endif
>     >      client->has_zero_init = S_ISREG(st.st_mode);
>     >      *strp = '/';
>     >      goto out;
>     > @@ -706,6 +718,7 @@ static int nfs_has_zero_init(BlockDriverState *bs)
>     >      return client->has_zero_init;
>     >  }
>     > 
>     > +#if !defined (_WIN32)
>     >  /* Called (via nfs_service) with QemuMutex held.  */
>     >  static void
>     >  nfs_get_allocated_file_size_cb(int ret, struct nfs_context *nfs,
>     void *data,
>     > @@ -729,7 +742,7 @@ static int64_t
>     nfs_get_allocated_file_size(BlockDriverState *bs)
>     >  {
>     >      NFSClient *client = bs->opaque;
>     >      NFSRPC task = {0};
>     > -    struct stat st;
>     > +    struct nfs_stat st;
>     > 
>     >      if (bdrv_is_read_only(bs) &&
>     >          !(bs->open_flags & BDRV_O_NOCACHE)) {
>     > @@ -748,6 +761,7 @@ static int64_t
>     nfs_get_allocated_file_size(BlockDriverState *bs)
>     > 
>     >      return (task.ret < 0 ? task.ret : st.st_blocks * 512);
>     >  }
>     > +#endif
>     > 
>     >  static int coroutine_fn
>     >  nfs_file_co_truncate(BlockDriverState *bs, int64_t offset, bool
>     exact,
>     > @@ -778,7 +792,7 @@ static int nfs_reopen_prepare(BDRVReopenState
>     *state,
>     >                                BlockReopenQueue *queue, Error **errp)
>     >  {
>     >      NFSClient *client = state->bs->opaque;
>     > -    struct stat st;
>     > +    struct nfs_stat st;
>     >      int ret = 0;
>     > 
>     >      if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {
>     > @@ -800,7 +814,9 @@ static int nfs_reopen_prepare(BDRVReopenState
>     *state,
>     >                         nfs_get_error(client->context));
>     >              return ret;
>     >          }
>     > +#if !defined (_WIN32)
>     >          client->st_blocks = st.st_blocks;
>     > +#endif
>     >      }
>     > 
>     >      return 0;
>     > @@ -869,7 +885,9 @@ static BlockDriver bdrv_nfs = {
>     >      .create_opts                    = &nfs_create_opts,
>     > 
>     >      .bdrv_has_zero_init             = nfs_has_zero_init,
>     > +#if !defined (_WIN32)
>     >      .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
>     > +#endif
>     >      .bdrv_co_truncate               = nfs_file_co_truncate,
>     > 
>     >      .bdrv_file_open                 = nfs_file_open,
>     >
> 
> 
> 
> -- 
>          此致
> 礼
> 罗勇刚
> Yours
>     sincerely,
> Yonggang Luo