RE: [RFC PATCH 1/2] 9p: retrieve fid from file when file instance exist.

[Jianyong Wu]

Hi Dominique,

> -----Original Message-----
> From: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
> Sent: Sunday, June 28, 2020 2:28 PM
> To: Jianyong Wu <Jianyong.Wu@xxxxxxx>
> Cc: ericvh@xxxxxxxxx; lucho@xxxxxxxxxx; v9fs-
> developer@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Steve
> Capper <Steve.Capper@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; Justin He
> <Justin.He@xxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>
> Subject: Re: [RFC PATCH 1/2] 9p: retrieve fid from file when file instance
> exist.
>
> Jianyong Wu wrote on Sun, Jun 28, 2020:
> > In the current setattr implementation in 9p, fid will always retrieved
> > from dentry no matter file instance exist or not when setattr. There
> > will be some info related to open file instance dropped. so it's
> > better to retrieve fid from file instance if file instance is passed to setattr.
> >
> > for example:
> > fd=open("tmp", O_RDWR);
> > ftruncate(fd, 10);
> >
> > the file context related with fd info will lost as fid will always be
> > retrieved from dentry, then the backend can't get the info of file context.
> > it is against the original intention of user and may lead to bug.
>
> I agree on principle, this makes more sense to use the file's fid.
>
Thanks!

> Just a comment below, but while I'm up in commit message I'll also be
> annoying with it -- please try to fix grammar mistakes for next
> patches/version (mostly missing some 'be' for future passive form; but I don't
> understand why you use future at all and some passive forms could probably
> be made active to simplify... Anyway we're not here to discuss English
> grammar but words missing out is sloppy and that gives a bad impression for
> no good reason)
>
Sorry to my poor English and thanks to point out the grammar mistakes,  I'll fix it.

> >
> > Signed-off-by: Jianyong Wu <jianyong.wu@xxxxxxx>
> > ---
> >  fs/9p/vfs_inode.c      | 5 ++++-
> >  fs/9p/vfs_inode_dotl.c | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index
> > c9255d399917..010869389523 100644
> > --- a/fs/9p/vfs_inode.c
> > +++ b/fs/9p/vfs_inode.c
> > @@ -1100,7 +1100,10 @@ static int v9fs_vfs_setattr(struct dentry
> > *dentry, struct iattr *iattr)
> >
> >  retval = -EPERM;
> >  v9ses = v9fs_dentry2v9ses(dentry);
> > -fid = v9fs_fid_lookup(dentry);
> > +if (iattr->ia_valid & ATTR_FILE)
> > +fid = iattr->ia_file->private_data;
>
> hmm, normally setattr cannot happen on a file that hasn't been opened so
> private_data should always be set, but it doesn't cost much to play safe and
> check? e.g. something like this is more conservative:
>
> struct p9_fid *fid = NULL;
> ...
> if (iattr->ia_valid & ATTR_FILE) {
> fid = iattr->ia_file->private_data;
> WARN_ON(!fid);
> }
> if (!fid)
> fid = v9fs_fid_lookup(dentry);
>
>
>
> What do you think?
>
Thanks, I think it's better. I'll take it.

Thanks
Jianyong

> --
> Dominique
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.