Re: [Qemu-devel] [PATCH v2 06/20] 9p: Avoid warning if FS_IOC_GETVERSION is not defined

[Greg Kurz]

On Thu, 31 May 2018 21:26:01 -0400
Keno Fischer <address@hidden> wrote:

> Both `stbuf` and `local_ioc_getversion` where unused when
> FS_IOC_GETVERSION was not defined, causing a compiler warning.
> 
> Reorgnaize the code to avoid this warning.
> 
> Signed-off-by: Keno Fischer <address@hidden>
> ---
> 
> Changes since v1:
>  * As request in review, logic is factored into a
>    local_ioc_getversion_init function.
> 
>  hw/9pfs/9p-local.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 576c8e3..6222891 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1375,10 +1375,10 @@ static int local_unlinkat(FsContext *ctx, V9fsPath 
> *dir,
>      return ret;
>  }
>  
> +#ifdef FS_IOC_GETVERSION
>  static int local_ioc_getversion(FsContext *ctx, V9fsPath *path,
>                                  mode_t st_mode, uint64_t *st_gen)
>  {
> -#ifdef FS_IOC_GETVERSION
>      int err;
>      V9fsFidOpenState fid_open;
>  
> @@ -1397,32 +1397,19 @@ static int local_ioc_getversion(FsContext *ctx, 
> V9fsPath *path,
>      err = ioctl(fid_open.fd, FS_IOC_GETVERSION, st_gen);
>      local_close(ctx, &fid_open);
>      return err;
> -#else
> -    errno = ENOTTY;
> -    return -1;
> -#endif
>  }
> +#endif
>  
> -static int local_init(FsContext *ctx, Error **errp)
> +static int local_ioc_getversion_init(FsContext *ctx, LocalData *data)
>  {
> +#ifdef FS_IOC_GETVERSION
>      struct statfs stbuf;
> -    LocalData *data = g_malloc(sizeof(*data));
>  
> -    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
> -    if (data->mountfd == -1) {
> -        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
> -        goto err;
> -    }
> -
> -#ifdef FS_IOC_GETVERSION
>      /*
>       * use ioc_getversion only if the ioctl is definied
>       */
>      if (fstatfs(data->mountfd, &stbuf) < 0) {
> -        close_preserve_errno(data->mountfd);
> -        error_setg_errno(errp, errno,
> -            "failed to stat file system at '%s'", ctx->fs_root);
> -        goto err;

Hmm, I'd prefer to keep the error_setg_errno() with fstatfs(), ie,
add an errp argument to this function.

> +        return -1;
>      }
>      switch (stbuf.f_type) {
>      case EXT2_SUPER_MAGIC:
> @@ -1433,6 +1420,26 @@ static int local_init(FsContext *ctx, Error **errp)
>          break;
>      }
>  #endif
> +    return 0;
> +}
> +
> +static int local_init(FsContext *ctx, Error **errp)
> +{
> +    LocalData *data = g_malloc(sizeof(*data));
> +
> +    data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
> +    if (data->mountfd == -1) {
> +        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
> +        goto err;
> +    }
> +
> +    if (local_ioc_getversion_init(ctx, data) < 0) {
> +        close_preserve_errno(data->mountfd);

And this could even be a plain close()

> +        error_setg_errno(errp, errno,
> +            "failed initialize ioc_getversion for file system at '%s'",

True, but I think "failed to stat file system" is more meaningful,
especially with the errno.

> +            ctx->fs_root);
> +        goto err;
> +    }
>  
>      if (ctx->export_flags & V9FS_SM_PASSTHROUGH) {
>          ctx->xops = passthrough_xattr_ops;