Re: [Qemu-devel] [PATCH] 9pfs: add xattrwalk_fid field in V9fsFidState struct

[Greg Kurz]

On Wed, 12 Oct 2016 19:55:42 -0700
Li Qiang <address@hidden> wrote:

> From: Li Qiang <address@hidden>
> 
> Currently, 9pfs sets the fs.xattr.copied_len field in V9fsFidState
> to -1 to indicate a xattr walk fid. As the fs.xattr.copied_len is also
> used to account for copied bytes, this may cause confusion. This patch
> add a bool variable to represent the xattr walk fid.
> 
> Signed-off-by: Li Qiang <address@hidden>
> ---

Please send a patchset instead of individual patches... it makes reviewing
difficult.

For example, this patch and the "9pfs: fix integer overflow issue in xattr
read/write" one are about fixing how xattr.copied_len and xattr.len are
being used. They definitely should be sent together.

I suggest you send a new patchset with a cover letter and 3 patches:
- the cover letter and the patches should be tagged v3, since this will be
  your third post on the same topic
- the cover letter should describe the overall problem: wrong types, unsafe
  computations, copied_len used both to account bytes and to tag the xattr
  fid.
- patch 1/3: this patch, with changes since the previous version below the ---
- patch 2/3: conversion of copied_len/len to uint64_t
- patch 3/3: "9pfs: fix integer overflow...", with changes since the previous
  version below the ---

>  hw/9pfs/9p.c | 7 ++++---
>  hw/9pfs/9p.h | 1 +
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 8c7488f..9625296 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -325,7 +325,7 @@ static int v9fs_xattr_fid_clunk(V9fsPDU *pdu, 
> V9fsFidState *fidp)
>  {
>      int retval = 0;
>  
> -    if (fidp->fs.xattr.copied_len == -1) {
> +    if (fidp->xattrwalk_fid) {
>          /* getxattr/listxattr fid */
>          goto free_value;
>      }
> @@ -3181,7 +3181,7 @@ static void v9fs_xattrwalk(void *opaque)
>           */
>          xattr_fidp->fs.xattr.len = size;
>          xattr_fidp->fid_type = P9_FID_XATTR;
> -        xattr_fidp->fs.xattr.copied_len = -1;
> +        xattr_fidp->xattrwalk_fid  = true;
>          if (size) {
>              xattr_fidp->fs.xattr.value = g_malloc(size);
>              err = v9fs_co_llistxattr(pdu, &xattr_fidp->path,
> @@ -3214,7 +3214,7 @@ static void v9fs_xattrwalk(void *opaque)
>           */
>          xattr_fidp->fs.xattr.len = size;
>          xattr_fidp->fid_type = P9_FID_XATTR;
> -        xattr_fidp->fs.xattr.copied_len = -1;
> +        xattr_fidp->xattrwalk_fid  = true;
>          if (size) {
>              xattr_fidp->fs.xattr.value = g_malloc(size);
>              err = v9fs_co_lgetxattr(pdu, &xattr_fidp->path,
> @@ -3269,6 +3269,7 @@ static void v9fs_xattrcreate(void *opaque)
>      /* Make the file fid point to xattr */
>      xattr_fidp = file_fidp;
>      xattr_fidp->fid_type = P9_FID_XATTR;
> +    xattr_fidp->xattrwalk_fid  = false;
>      xattr_fidp->fs.xattr.copied_len = 0;
>      xattr_fidp->fs.xattr.len = size;
>      xattr_fidp->fs.xattr.flags = flags;
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 22198f6..7e1e70b 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -214,6 +214,7 @@ struct V9fsFidState
>      uid_t uid;
>      int ref;
>      int clunked;
> +    bool xattrwalk_fid;

This belongs to the V9fsXattr type.

>      V9fsFidState *next;
>      V9fsFidState *rclm_lst;
>  };

Cheers.

--
Greg