linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hfsplus: return ENODATA when no xattr is found
@ 2017-10-19 19:41 Ernesto A. Fernández
  0 siblings, 0 replies; 5+ messages in thread
From: Ernesto A. Fernández @ 2017-10-19 19:41 UTC (permalink / raw)
  To: linux-fsdevel
  Cc: Al Viro, Andreas Gruenbacher, Hin-Tak Leung, Vyacheslav Dubeyko,
	Andrew Morton, Ernesto A. Fernández

There are several points in the code where ENOENT or EOPNOTSUPP are
used to signal that an extended attribute does not exist. This is
clearly noticeable from the odd error messages shown by setfattr and
getfattr. Use ENODATA instead.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfsplus/attributes.c | 6 ++++--
 fs/hfsplus/xattr.c      | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index e5b221d..77b0d40 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -268,7 +268,7 @@ static int __hfsplus_delete_attr(struct inode *inode, u32 cnid,
 			offsetof(struct hfsplus_attr_key, cnid),
 			sizeof(__be32));
 	if (cnid != be32_to_cpu(found_cnid))
-		return -ENOENT;
+		return -ENODATA;
 
 	hfs_bnode_read(fd->bnode, &record_type,
 			fd->entryoffset, sizeof(record_type));
@@ -283,7 +283,7 @@ static int __hfsplus_delete_attr(struct inode *inode, u32 cnid,
 		return -EOPNOTSUPP;
 	default:
 		pr_err("invalid extended attribute record\n");
-		return -ENOENT;
+		return -ENODATA;
 	}
 
 	err = hfs_brec_remove(fd);
@@ -324,6 +324,8 @@ int hfsplus_delete_attr(struct inode *inode, const char *name)
 	}
 
 	err = hfs_brec_find(&fd, hfs_find_rec_by_key);
+	if (err == -ENOENT)
+		err = -ENODATA;
 	if (err)
 		goto out;
 
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index d37bb88..814082c 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -511,7 +511,7 @@ ssize_t __hfsplus_getxattr(struct inode *inode, const char *name,
 		return hfsplus_getxattr_finder_info(inode, value, size);
 
 	if (!HFSPLUS_SB(inode->i_sb)->attr_tree)
-		return -EOPNOTSUPP;
+		return -ENODATA;
 
 	entry = hfsplus_alloc_attr_entry();
 	if (!entry) {
@@ -697,7 +697,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
 	if (res < 0)
 		return res;
 	else if (!HFSPLUS_SB(inode->i_sb)->attr_tree)
-		return (res == 0) ? -EOPNOTSUPP : res;
+		return (res == 0) ? -ENODATA : res;
 
 	err = hfs_find_init(HFSPLUS_SB(inode->i_sb)->attr_tree, &fd);
 	if (err) {
@@ -780,7 +780,7 @@ static int hfsplus_removexattr(struct inode *inode, const char *name)
 	int is_all_xattrs_deleted = 0;
 
 	if (!HFSPLUS_SB(inode->i_sb)->attr_tree)
-		return -EOPNOTSUPP;
+		return -ENODATA;
 
 	if (!strcmp_xattr_finder_info(name))
 		return -EOPNOTSUPP;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] hfsplus: return ENODATA when no xattr is found
       [not found] <51697605.866452.1508445053840.ref@mail.yahoo.com>
@ 2017-10-19 20:30 ` Hin-Tak Leung
  2017-10-19 23:02   ` Ernesto A. Fernández
  0 siblings, 1 reply; 5+ messages in thread
From: Hin-Tak Leung @ 2017-10-19 20:30 UTC (permalink / raw)
  To: linux-fsdevel, Ernesto A. Fernández
  Cc: Al Viro, Andreas Gruenbacher, Hin-Tak Leung, Vyacheslav Dubeyko,
	Andrew Morton, Ernesto A. Fernández


--------------------------------------------
On Thu, 19/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> wrote:

> There are several points in the code where ENOENT or
> EOPNOTSUPP are
> used to signal that an extended attribute does not exist.
> This is
> clearly noticeable from the odd error messages shown by
> setfattr and
> getfattr. Use ENODATA instead.
 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>

Nacked.

I think you perhaps mis-understood the code. Some (older) HFS+ file system does not have attribute records so it gives  EOPNOTSUPP . It is not NODATA, but that you cannot write attribute data(or read) to such old fs. Likewise, the other changes from ENOENT to ENODATA seems wrong too. "Not found" is not "no data" ( = "found but null").

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hfsplus: return ENODATA when no xattr is found
  2017-10-19 20:30 ` [PATCH] hfsplus: return ENODATA when no xattr is found Hin-Tak Leung
@ 2017-10-19 23:02   ` Ernesto A. Fernández
  2017-10-20  0:15     ` Viacheslav Dubeyko
  0 siblings, 1 reply; 5+ messages in thread
From: Ernesto A. Fernández @ 2017-10-19 23:02 UTC (permalink / raw)
  To: Hin-Tak Leung, linux-fsdevel
  Cc: Al Viro, Andreas Gruenbacher, Hin-Tak Leung, Vyacheslav Dubeyko,
	Andrew Morton, Ernesto A. Fernández

On Thu, Oct 19, 2017 at 08:30:53PM +0000, Hin-Tak Leung wrote:
> 
> --------------------------------------------
> On Thu, 19/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> wrote:
> 
> > There are several points in the code where ENOENT or
> > EOPNOTSUPP are
> > used to signal that an extended attribute does not exist.
> > This is
> > clearly noticeable from the odd error messages shown by
> > setfattr and
> > getfattr. Use ENODATA instead.
>  
> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> 
> Nacked.
> 
> I think you perhaps mis-understood the code. Some (older) HFS+ file system does not have attribute records so it gives  EOPNOTSUPP . It is not NODATA, but that you cannot write attribute data(or read) to such old fs. Likewise, the other changes from ENOENT to ENODATA seems wrong too. "Not found" is not "no data" ( = "found but null").
> 

If you create a fresh hfsplus filesystem and run:

touch test
setfattr -x user.1 test

you will get an "operation not supported" error. That isn't right. Now
if you run:

setfattr -n user.1 test
setfattr -x user.1 test
setfattr -x user.1 test

you will get a "no such file or directory" error. Also bad, the file is
right there. This one is caused by the ENOENT.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hfsplus: return ENODATA when no xattr is found
  2017-10-19 23:02   ` Ernesto A. Fernández
@ 2017-10-20  0:15     ` Viacheslav Dubeyko
  2017-10-20  0:54       ` Ernesto A. Fernández
  0 siblings, 1 reply; 5+ messages in thread
From: Viacheslav Dubeyko @ 2017-10-20  0:15 UTC (permalink / raw)
  To: Ernesto A. Fernández, Hin-Tak Leung, linux-fsdevel
  Cc: Al Viro, Andreas Gruenbacher, Andrew Morton

On Thu, 2017-10-19 at 20:02 -0300, Ernesto A. Fernández wrote:
> On Thu, Oct 19, 2017 at 08:30:53PM +0000, Hin-Tak Leung wrote:
> > 
> > 
> > --------------------------------------------
> > On Thu, 19/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail
> > .com> wrote:
> > 
> > > 
> > > There are several points in the code where ENOENT or
> > > EOPNOTSUPP are
> > > used to signal that an extended attribute does not exist.
> > > This is
> > > clearly noticeable from the odd error messages shown by
> > > setfattr and
> > > getfattr. Use ENODATA instead.
> >  
> > > 
> > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.
> > > com>
> > Nacked.
> > 
> > I think you perhaps mis-understood the code. Some (older) HFS+ file
> > system does not have attribute records so it gives  EOPNOTSUPP . It
> > is not NODATA, but that you cannot write attribute data(or read) to
> > such old fs. Likewise, the other changes from ENOENT to ENODATA
> > seems wrong too. "Not found" is not "no data" ( = "found but
> > null").
> > 
> If you create a fresh hfsplus filesystem and run:
> 
> touch test
> setfattr -x user.1 test
> 
> you will get an "operation not supported" error. That isn't right.
> Now
> if you run:
> 
> setfattr -n user.1 test
> setfattr -x user.1 test
> setfattr -x user.1 test
> 
> you will get a "no such file or directory" error. Also bad, the file
> is
> right there. This one is caused by the ENOENT.


You suggested to exclude all this code (xattr support) from the HFS+
file system driver. What is the point to review your patches for the
code that will be deleted? If this code will be deleted then no
troubles anymore. Forget and relax.

Regards,
Vyacheslav Dubeyko.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] hfsplus: return ENODATA when no xattr is found
  2017-10-20  0:15     ` Viacheslav Dubeyko
@ 2017-10-20  0:54       ` Ernesto A. Fernández
  0 siblings, 0 replies; 5+ messages in thread
From: Ernesto A. Fernández @ 2017-10-20  0:54 UTC (permalink / raw)
  To: Viacheslav Dubeyko, linux-fsdevel
  Cc: Ernesto A. Fernández, Hin-Tak Leung, Al Viro,
	Andreas Gruenbacher, Andrew Morton

On Thu, Oct 19, 2017 at 05:15:13PM -0700, Viacheslav Dubeyko wrote:
> On Thu, 2017-10-19 at 20:02 -0300, Ernesto A. Fernández wrote:
> > On Thu, Oct 19, 2017 at 08:30:53PM +0000, Hin-Tak Leung wrote:
> > > 
> > > 
> > > --------------------------------------------
> > > On Thu, 19/10/17, Ernesto A. Fernández <ernesto.mnd.fernandez@gmail
> > > .com> wrote:
> > > 
> > > > 
> > > > There are several points in the code where ENOENT or
> > > > EOPNOTSUPP are
> > > > used to signal that an extended attribute does not exist.
> > > > This is
> > > > clearly noticeable from the odd error messages shown by
> > > > setfattr and
> > > > getfattr. Use ENODATA instead.
> > >  
> > > > 
> > > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.
> > > > com>
> > > Nacked.
> > > 
> > > I think you perhaps mis-understood the code. Some (older) HFS+ file
> > > system does not have attribute records so it gives  EOPNOTSUPP . It
> > > is not NODATA, but that you cannot write attribute data(or read) to
> > > such old fs. Likewise, the other changes from ENOENT to ENODATA
> > > seems wrong too. "Not found" is not "no data" ( = "found but
> > > null").
> > > 
> > If you create a fresh hfsplus filesystem and run:
> > 
> > touch test
> > setfattr -x user.1 test
> > 
> > you will get an "operation not supported" error. That isn't right.
> > Now
> > if you run:
> > 
> > setfattr -n user.1 test
> > setfattr -x user.1 test
> > setfattr -x user.1 test
> > 
> > you will get a "no such file or directory" error. Also bad, the file
> > is
> > right there. This one is caused by the ENOENT.
> 
> 
> You suggested to exclude all this code (xattr support) from the HFS+
> file system driver. What is the point to review your patches for the
> code that will be deleted? If this code will be deleted then no
> troubles anymore. Forget and relax.

I never suggested anything like that. I'm guessing your confusion is
because of the patch I sent to drop ACL support, but I never spoke of
xattrs.

And you seem to believe it was my idea, but it wasn't. I only wrote
the patch because I was the one who first noticed it was broken a
couple of months ago.

Ernest

> 
> Regards,
> Vyacheslav Dubeyko.
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-10-20  0:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <51697605.866452.1508445053840.ref@mail.yahoo.com>
2017-10-19 20:30 ` [PATCH] hfsplus: return ENODATA when no xattr is found Hin-Tak Leung
2017-10-19 23:02   ` Ernesto A. Fernández
2017-10-20  0:15     ` Viacheslav Dubeyko
2017-10-20  0:54       ` Ernesto A. Fernández
2017-10-19 19:41 Ernesto A. Fernández

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).