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?
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 was also worried about the semantics, for instance, the outcome of:
>
> fd = open("f1.tmp");
> write(fd, ...);
> link("f1.tmp", "f1");
> unlink("f1.tmp");
> fd2 = open("f1.tmp");
> fchmod(fd, 0644);
> 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?
> 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.
> err = inode_change_ok(dentry->d_inode, attr);
> if (err)
> @@ -864,7 +870,7 @@ int hostfs_setattr(struct dentry *dentry, struct iattr
> *attr) }
> name = dentry_name(dentry, 0);
> if(name == NULL) return(-ENOMEM);
> - err = set_attr(name, &attrs);
> + err = set_attr(name, &attrs, fd);
> kfree(name);
> if(err)
> return(err);
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.
> ma = HOSTFS_ATTR_ATIME_SET | HOSTFS_ATTR_MTIME_SET;
> if((attrs->ia_valid & ma) == ma){
> + struct timeval times[2];
> +
> buf.actime = attrs->ia_atime.tv_sec;
> buf.modtime = attrs->ia_mtime.tv_sec;
> - if(utime(file, &buf) != 0) return(-errno);
> +
> + /* atime */
> + times[0].tv_sec = attrs->ia_atime.tv_sec;
> + times[0].tv_usec = attrs->ia_atime.tv_nsec * 1000;
> +
> + /* mtime */
> + times[1].tv_sec = attrs->ia_mtime.tv_sec;
> + times[1].tv_usec = attrs->ia_mtime.tv_nsec * 1000;
> +
> + if(fd >= 0){
> + if(futimes(fd, times) != 0)
> + return(-errno);
> + } else if(utime(file, &buf) != 0){
> + return(-errno);
> + }
> }
> else {
> struct timespec ts;
> + struct timeval times[2];
>
> if(attrs->ia_valid & HOSTFS_ATTR_ATIME_SET){
> err = stat_file(file, NULL, NULL, NULL, NULL, NULL,
> - NULL, NULL, &ts, NULL, NULL, NULL);
> + NULL, NULL, &ts, NULL, NULL, NULL, fd);
> if(err != 0)
> return(err);
> buf.actime = attrs->ia_atime.tv_sec;
> buf.modtime = ts.tv_sec;
> - if(utime(file, &buf) != 0)
> +
> + /* atime */
> + times[0].tv_sec = attrs->ia_atime.tv_sec;
> + times[0].tv_usec = attrs->ia_atime.tv_nsec * 1000;
> +
> + /* mtime */
> + times[1].tv_sec = ts.tv_sec;
> + times[1].tv_usec = ts.tv_nsec * 1000;
> +
> + if(fd >= 0){
> + if(futimes(fd, times) != 0)
> + return(-errno);
> + } else if(utime(file, &buf) != 0){
> return(-errno);
> + }
> }
> if(attrs->ia_valid & HOSTFS_ATTR_MTIME_SET){
> err = stat_file(file, NULL, NULL, NULL, NULL, NULL,
> - NULL, &ts, NULL, NULL, NULL, NULL);
> + NULL, &ts, NULL, NULL, NULL, NULL, fd);
> if(err != 0)
> return(err);
> buf.actime = ts.tv_sec;
> buf.modtime = attrs->ia_mtime.tv_sec;
> - if(utime(file, &buf) != 0)
> +
> + /* atime */
> + times[0].tv_sec = ts.tv_sec;
> + times[0].tv_usec = ts.tv_nsec * 1000;
> +
> + /* mtime */
> + times[1].tv_sec = attrs->ia_mtime.tv_sec;
> + times[1].tv_usec = attrs->ia_mtime.tv_nsec * 1000;
> +
> + if(fd >= 0){
> + if(futimes(fd, times) != 0)
> + return(-errno);
> + } else if(utime(file, &buf) != 0){
> return(-errno);
> + }
> }
> }
> if(attrs->ia_valid & HOSTFS_ATTR_CTIME) ;
> if(attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)){
> err = stat_file(file, NULL, NULL, NULL, NULL, NULL, NULL,
> &attrs->ia_atime, &attrs->ia_mtime, NULL,
> - NULL, NULL);
> + NULL, NULL, fd);
> if(err != 0) return(err);
> }
> return(0);
--
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade
-------------------------------------------------------------------------
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
