On Wed, Apr 25, 2007 at 05:51:10PM +0200, Blaisorblade wrote:
> On martedì 24 aprile 2007, Alberto Bertogli wrote:
> > This patch allows hostfs_setattr() to work on closed files by calling
> > set_attr() (the userspace part) with the inode's fd.
> >
> > Without this, applications that depend on doing attribute changes to
> > closed files will fail.
> >
> > It works by using the fd versions instead of the path ones (for example
> > fchmod() instead of chmod(), fchown() instead of chown()) when an fd is
> > available.
> >
> >
> > Signed-off-by: Alberto Bertogli <[EMAIL PROTECTED]>
> > ---
> This makes sense and is useful - but:
> 1) when I read the examples, I understood you meant 'unlinked open files',
> not 'closed files'. Right?
Right, sorry about that, the terminology I used is completely
misleading. I'll change the description.
> The patch is of good quality; I've not seen any single bug, which is rare in
> such submission, but before merging, some little things should be fixed up
> (either by you or by Jeff).
>
> 2) if (...) return ...; on a single line is horrible style, should be
> corrected (I see sometimes the original code uses it, well it shouldn't).
I couldn't agree more. I did it that way because I tried to follow the
coding style of the original code.
> > In the host fchmod() changes f1 and not f1.tmp, but maybe the open() in the
> > middle got hostfs confused. Luckily this is not the case and it works fine;
> > but as I'm not familiar with hostfs there may be other issues.
>
> I guess the patch would make this work, right?
Yes, that case works with the patch.
> > diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
> > index fd301a9..c704d9c 100644
>
> > @@ -817,8 +817,14 @@ int hostfs_permission(struct inode *ino, int desired,
> > struct nameidata *nd) int hostfs_setattr(struct dentry *dentry, struct
> > iattr *attr)
> > {
> > struct hostfs_iattr attrs;
> > + struct hostfs_inode_info *iinfo;
> > char *name;
> > int err;
> > + int fd = -1;
> > +
> > + iinfo = HOSTFS_I(dentry->d_inode);
> > + if (iinfo != NULL)
> > + fd = iinfo->fd;
>
> iinfo cannot be NULL, so please remove that if. HOSTFS_I does not access a
> field of d_inode; rather, d_inode points to a field of iinfo, i.e. the
> vfs_inode field of struct hostfs_inode_info.
That's good to know =) I'll fix it.
> I do not like the below code. The different code is for setting buf; what
> follows that is common to all cases, and the conversion probably should be
> factored out into a conversion function for better clarity: verifying what
> the code does is difficult here.
I agree. I tried to make the changes to look as much as possible to the
original, but if you want me to reorganize that code a bit, I'll be glad
to give it a try.
I'll address all these issues and post and updated patch (probably
tomorrow).
Thanks a lot,
Alberto
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
