[PATCH] hfsplus: return file attributes on statx

[PATCH] hfsplus: return file attributes on statx

[Ernesto A. Fernández]

The immutable, append-only and no-dump attributes can only be retrieved
with an ioctl; implement the ->getattr() method to return them on statx.
Do not return the inode birthtime yet, because the issue of how best to
handle the post-2038 timestamps is still under discussion.

This patch is needed to pass xfstests generic/424.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfsplus/dir.c        |  1 +
 fs/hfsplus/hfsplus_fs.h |  2 ++
 fs/hfsplus/inode.c      | 21 +++++++++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index f37662675c3a..29a9dcfbe81f 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -565,6 +565,7 @@ const struct inode_operations hfsplus_dir_inode_operations = {
 	.symlink		= hfsplus_symlink,
 	.mknod			= hfsplus_mknod,
 	.rename			= hfsplus_rename,
+	.getattr		= hfsplus_getattr,
 	.listxattr		= hfsplus_listxattr,
 };
 
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index dd7ad9f13e3a..b8471bf05def 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -488,6 +488,8 @@ void hfsplus_inode_write_fork(struct inode *inode,
 			      struct hfsplus_fork_raw *fork);
 int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd);
 int hfsplus_cat_write_inode(struct inode *inode);
+int hfsplus_getattr(const struct path *path, struct kstat *stat,
+		    u32 request_mask, unsigned int query_flags);
 int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 		       int datasync);
 
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index d7ab9d8c4b67..d131c8ea7eb6 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -270,6 +270,26 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
 	return 0;
 }
 
+int hfsplus_getattr(const struct path *path, struct kstat *stat,
+		    u32 request_mask, unsigned int query_flags)
+{
+	struct inode *inode = d_inode(path->dentry);
+	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
+
+	if (inode->i_flags & S_APPEND)
+		stat->attributes |= STATX_ATTR_APPEND;
+	if (inode->i_flags & S_IMMUTABLE)
+		stat->attributes |= STATX_ATTR_IMMUTABLE;
+	if (hip->userflags & HFSPLUS_FLG_NODUMP)
+		stat->attributes |= STATX_ATTR_NODUMP;
+
+	stat->attributes_mask |= STATX_ATTR_APPEND | STATX_ATTR_IMMUTABLE |
+				 STATX_ATTR_NODUMP;
+
+	generic_fillattr(inode, stat);
+	return 0;
+}
+
 int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 		       int datasync)
 {
@@ -329,6 +349,7 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
 
 static const struct inode_operations hfsplus_file_inode_operations = {
 	.setattr	= hfsplus_setattr,
+	.getattr	= hfsplus_getattr,
 	.listxattr	= hfsplus_listxattr,
 };
 
-- 
2.11.0

Re: [PATCH] hfsplus: return file attributes on statx

[Viacheslav Dubeyko]

On Sun, 2018-10-14 at 13:35 -0300, Ernesto A. Fernández wrote:
> The immutable, append-only and no-dump attributes can only be retrieved
> with an ioctl; implement the ->getattr() method to return them on statx.
> Do not return the inode birthtime yet, because the issue of how best to
> handle the post-2038 timestamps is still under discussion.
> 

As far as I can see, the stable branch doesn't contain the inode
birthtime yet. So, I believe we have no troubles with it.

> This patch is needed to pass xfstests generic/424.

Do you mean that the patch isn't been tested yet? Do it needs to wait
the testing result report before taking the patch? Otherwise, it looks
weird to have such remark in the comment section of the patch.

> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> ---
>  fs/hfsplus/dir.c        |  1 +
>  fs/hfsplus/hfsplus_fs.h |  2 ++
>  fs/hfsplus/inode.c      | 21 +++++++++++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index f37662675c3a..29a9dcfbe81f 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -565,6 +565,7 @@ const struct inode_operations hfsplus_dir_inode_operations = {
>  	.symlink		= hfsplus_symlink,
>  	.mknod			= hfsplus_mknod,
>  	.rename			= hfsplus_rename,
> +	.getattr		= hfsplus_getattr,
>  	.listxattr		= hfsplus_listxattr,
>  };
>  
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index dd7ad9f13e3a..b8471bf05def 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -488,6 +488,8 @@ void hfsplus_inode_write_fork(struct inode *inode,
>  			      struct hfsplus_fork_raw *fork);
>  int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd);
>  int hfsplus_cat_write_inode(struct inode *inode);
> +int hfsplus_getattr(const struct path *path, struct kstat *stat,
> +		    u32 request_mask, unsigned int query_flags);
>  int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
>  		       int datasync);
>  
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index d7ab9d8c4b67..d131c8ea7eb6 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -270,6 +270,26 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
>  	return 0;
>  }
>  
> +int hfsplus_getattr(const struct path *path, struct kstat *stat,
> +		    u32 request_mask, unsigned int query_flags)
> +{
> +	struct inode *inode = d_inode(path->dentry);
> +	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> +
> +	if (inode->i_flags & S_APPEND)
> +		stat->attributes |= STATX_ATTR_APPEND;
> +	if (inode->i_flags & S_IMMUTABLE)
> +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> +	if (hip->userflags & HFSPLUS_FLG_NODUMP)
> +		stat->attributes |= STATX_ATTR_NODUMP;
> +
> +	stat->attributes_mask |= STATX_ATTR_APPEND | STATX_ATTR_IMMUTABLE |
> +				 STATX_ATTR_NODUMP;
> +
> +	generic_fillattr(inode, stat);
> +	return 0;
> +}
> +
>  int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
>  		       int datasync)
>  {
> @@ -329,6 +349,7 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
>  
>  static const struct inode_operations hfsplus_file_inode_operations = {
>  	.setattr	= hfsplus_setattr,
> +	.getattr	= hfsplus_getattr,
>  	.listxattr	= hfsplus_listxattr,
>  };
>  

The patch looks good. I don't see any issue with it.

Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: return file attributes on statx

[Ernesto A. Fernández]

On Mon, Oct 15, 2018 at 05:26:23PM -0700, Viacheslav Dubeyko wrote:
> On Sun, 2018-10-14 at 13:35 -0300, Ernesto A. Fernández wrote:
> > The immutable, append-only and no-dump attributes can only be retrieved
> > with an ioctl; implement the ->getattr() method to return them on statx.
> > Do not return the inode birthtime yet, because the issue of how best to
> > handle the post-2038 timestamps is still under discussion.
> > 
> 
> As far as I can see, the stable branch doesn't contain the inode
> birthtime yet. So, I believe we have no troubles with it.

What stable branch?  What are you talking about?  Of course the inode
birthtime is in the code, how could it not be?

> 
> > This patch is needed to pass xfstests generic/424.
> 
> Do you mean that the patch isn't been tested yet? Do it needs to wait
> the testing result report before taking the patch? Otherwise, it looks
> weird to have such remark in the comment section of the patch.

Look, I'm not a native speaker either, but I think that's a pretty simple
sentence.  You need this patch if you want xfstests generic/424 to pass.

> 
> > 
> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> > ---
> >  fs/hfsplus/dir.c        |  1 +
> >  fs/hfsplus/hfsplus_fs.h |  2 ++
> >  fs/hfsplus/inode.c      | 21 +++++++++++++++++++++
> >  3 files changed, 24 insertions(+)
> > 
> > diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> > index f37662675c3a..29a9dcfbe81f 100644
> > --- a/fs/hfsplus/dir.c
> > +++ b/fs/hfsplus/dir.c
> > @@ -565,6 +565,7 @@ const struct inode_operations hfsplus_dir_inode_operations = {
> >  	.symlink		= hfsplus_symlink,
> >  	.mknod			= hfsplus_mknod,
> >  	.rename			= hfsplus_rename,
> > +	.getattr		= hfsplus_getattr,
> >  	.listxattr		= hfsplus_listxattr,
> >  };
> >  
> > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> > index dd7ad9f13e3a..b8471bf05def 100644
> > --- a/fs/hfsplus/hfsplus_fs.h
> > +++ b/fs/hfsplus/hfsplus_fs.h
> > @@ -488,6 +488,8 @@ void hfsplus_inode_write_fork(struct inode *inode,
> >  			      struct hfsplus_fork_raw *fork);
> >  int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd);
> >  int hfsplus_cat_write_inode(struct inode *inode);
> > +int hfsplus_getattr(const struct path *path, struct kstat *stat,
> > +		    u32 request_mask, unsigned int query_flags);
> >  int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
> >  		       int datasync);
> >  
> > diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> > index d7ab9d8c4b67..d131c8ea7eb6 100644
> > --- a/fs/hfsplus/inode.c
> > +++ b/fs/hfsplus/inode.c
> > @@ -270,6 +270,26 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
> >  	return 0;
> >  }
> >  
> > +int hfsplus_getattr(const struct path *path, struct kstat *stat,
> > +		    u32 request_mask, unsigned int query_flags)
> > +{
> > +	struct inode *inode = d_inode(path->dentry);
> > +	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> > +
> > +	if (inode->i_flags & S_APPEND)
> > +		stat->attributes |= STATX_ATTR_APPEND;
> > +	if (inode->i_flags & S_IMMUTABLE)
> > +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> > +	if (hip->userflags & HFSPLUS_FLG_NODUMP)
> > +		stat->attributes |= STATX_ATTR_NODUMP;
> > +
> > +	stat->attributes_mask |= STATX_ATTR_APPEND | STATX_ATTR_IMMUTABLE |
> > +				 STATX_ATTR_NODUMP;
> > +
> > +	generic_fillattr(inode, stat);
> > +	return 0;
> > +}
> > +
> >  int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
> >  		       int datasync)
> >  {
> > @@ -329,6 +349,7 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end,
> >  
> >  static const struct inode_operations hfsplus_file_inode_operations = {
> >  	.setattr	= hfsplus_setattr,
> > +	.getattr	= hfsplus_getattr,
> >  	.listxattr	= hfsplus_listxattr,
> >  };
> >  
> 
> The patch looks good. I don't see any issue with it.
> 
> Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>
> 
> Thanks,
> Vyacheslav Dubeyko.
> 
> 

Re: [PATCH] hfsplus: return file attributes on statx

[Viacheslav Dubeyko]

On Tue, 2018-10-16 at 20:38 -0300, Ernesto A. Fernández wrote:
> On Mon, Oct 15, 2018 at 05:26:23PM -0700, Viacheslav Dubeyko wrote:
> > On Sun, 2018-10-14 at 13:35 -0300, Ernesto A. Fernández wrote:
> > > The immutable, append-only and no-dump attributes can only be retrieved
> > > with an ioctl; implement the ->getattr() method to return them on statx.
> > > Do not return the inode birthtime yet, because the issue of how best to
> > > handle the post-2038 timestamps is still under discussion.
> > > 
> > 
> > As far as I can see, the stable branch doesn't contain the inode
> > birthtime yet. So, I believe we have no troubles with it.
> 
> What stable branch?  What are you talking about?  Of course the inode
> birthtime is in the code, how could it not be?
> 

I mean the latest stable kernel branch
(git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git).

> > 
> > > This patch is needed to pass xfstests generic/424.
> > 
> > Do you mean that the patch isn't been tested yet? Do it needs to wait
> > the testing result report before taking the patch? Otherwise, it looks
> > weird to have such remark in the comment section of the patch.
> 
> Look, I'm not a native speaker either, but I think that's a pretty simple
> sentence.  You need this patch if you want xfstests generic/424 to pass.
> 

Currently, it sounds confusing. It makes sense to rework the comment
section and to resend the second version of the patch.

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH] hfsplus: return file attributes on statx

[Ernesto A. Fernández]

On Tue, Oct 16, 2018 at 05:07:55PM -0700, Viacheslav Dubeyko wrote:
> On Tue, 2018-10-16 at 20:38 -0300, Ernesto A. Fernández wrote:
> > On Mon, Oct 15, 2018 at 05:26:23PM -0700, Viacheslav Dubeyko wrote:
> > > On Sun, 2018-10-14 at 13:35 -0300, Ernesto A. Fernández wrote:
> > > > The immutable, append-only and no-dump attributes can only be retrieved
> > > > with an ioctl; implement the ->getattr() method to return them on statx.
> > > > Do not return the inode birthtime yet, because the issue of how best to
> > > > handle the post-2038 timestamps is still under discussion.
> > > > 
> > > 
> > > As far as I can see, the stable branch doesn't contain the inode
> > > birthtime yet. So, I believe we have no troubles with it.
> > 
> > What stable branch?  What are you talking about?  Of course the inode
> > birthtime is in the code, how could it not be?
> > 
> 
> I mean the latest stable kernel branch
> (git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git).

OK, that's the stable tree.  I still have no idea why you are bringing
it up, or what you mean by "doesn't contain the inode birthtime".

> 
> > > 
> > > > This patch is needed to pass xfstests generic/424.
> > > 
> > > Do you mean that the patch isn't been tested yet? Do it needs to wait
> > > the testing result report before taking the patch? Otherwise, it looks
> > > weird to have such remark in the comment section of the patch.
> > 
> > Look, I'm not a native speaker either, but I think that's a pretty simple
> > sentence.  You need this patch if you want xfstests generic/424 to pass.
> > 
> 
> Currently, it sounds confusing.

I don't think it does, and I don't quite trust your judgement in the matter.

> It makes sense to rework the comment
> section and to resend the second version of the patch.
 
> Thanks,
> Vyacheslav Dubeyko.
> 
> 

Re: [PATCH] hfsplus: return file attributes on statx

[Ernesto A. Fernández]

On Fri, Nov 09, 2018 at 02:26:30PM -0800, Andrew Morton wrote:
> On Sun, 14 Oct 2018 13:35:58 -0300 Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> wrote:
> 
> > The immutable, append-only and no-dump attributes can only be retrieved
> > with an ioctl; implement the ->getattr() method to return them on statx.
> > Do not return the inode birthtime yet, because the issue of how best to
> > handle the post-2038 timestamps is still under discussion.
> > 
> > This patch is needed to pass xfstests generic/424.
> 
> It's been a while since I looked into such things...
> 
> > --- a/fs/hfsplus/inode.c
> > +++ b/fs/hfsplus/inode.c
> > @@ -270,6 +270,26 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
> >  	return 0;
> >  }
> >  
> > +int hfsplus_getattr(const struct path *path, struct kstat *stat,
> > +		    u32 request_mask, unsigned int query_flags)
> > +{
> > +	struct inode *inode = d_inode(path->dentry);
> > +	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
> > +
> > +	if (inode->i_flags & S_APPEND)
> > +		stat->attributes |= STATX_ATTR_APPEND;
> > +	if (inode->i_flags & S_IMMUTABLE)
> > +		stat->attributes |= STATX_ATTR_IMMUTABLE;
> 
> ext4_getattr() inspects the underlying ext4_inode's flags to determine
> the above.  But here hfs is looking at the vfs-level inode.  Which is
> correct, which is best, do we know why they differ, etc?

ext4 is not reading the flags from the ext4_inode directly, it reads them
from ext4_inode_info.  hfsplus_inode_info doesn't have that information
anymore, since 722c55d13e72 ("hfsplus: remove superflous rootflags field in
hfsplus_inode_info").

My intention here was to follow what the FS_IOC_GETFLAGS ioctl is doing, so
I just copied the checks from hfsplus_ioctl_getflags() at ioctl.c.

> 
> > +	if (hip->userflags & HFSPLUS_FLG_NODUMP)
> > +		stat->attributes |= STATX_ATTR_NODUMP;
> > +
> > +	stat->attributes_mask |= STATX_ATTR_APPEND | STATX_ATTR_IMMUTABLE |
> > +				 STATX_ATTR_NODUMP;
> > +
> > +	generic_fillattr(inode, stat);
> > +	return 0;
> > +}
> 
>