Re: [uml-devel] [PATCH] Make hostfs_setattr() support operations on closed files.

[Alberto Bertogli <albertito@gmail.com>]

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 <albertito@gmail.com>
> > ---
> 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
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel