[PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Phillip Susi]

This patch fixes a bug in udf where it would write uid/gid = 0 to the
disk for files owned by the id given with the uid=/gid= mount options.
It also adds 4 new mount options: uid/gid=forget and uid/gid=ignore.
Without any options the id in core and on disk always match.  Giving
uid/gid=nnn specifies a default ID to be used in core when the on disk ID
is -1.  uid/gid=ignore forces the in core ID to allways be used no matter
what the on disk ID is.  uid/gid=forget forces the on disk ID to always be
written out as -1.

The use of these options allows you to override ownerships on a disk or
disable ownwership information from being written, allowing the media
to be used portably between different computers and possibly different users
without permissions issues that would require root to correct.

Signed-off-by: Phillip Susi <psusi@cfl.rr.com>

---

fs/udf/inode.c  |   14 ++++++++++----
fs/udf/super.c  |   18 +++++++++++++++++-
fs/udf/udf_sb.h |    4 ++++
3 files changed, 31 insertions(+), 5 deletions(-)

2428c90c6e52a317808896b6cd639199388f7ddd
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 395e582..2f47bf6 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1045,10 +1045,12 @@ static void udf_fill_inode(struct inode 
	}

	inode->i_uid = le32_to_cpu(fe->uid);
-	if ( inode->i_uid == -1 ) inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
+	if ( inode->i_uid == -1 || UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_IGNORE) )
+	  inode->i_uid = UDF_SB(inode->i_sb)->s_uid;

	inode->i_gid = le32_to_cpu(fe->gid);
-	if ( inode->i_gid == -1 ) inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
+	if ( inode->i_gid == -1 || UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_IGNORE) )
+	  inode->i_gid = UDF_SB(inode->i_sb)->s_gid;

	inode->i_nlink = le16_to_cpu(fe->fileLinkCount);
	if (!inode->i_nlink)
@@ -1335,10 +1337,14 @@ udf_update_inode(struct inode *inode, in
		return err;
	}

-	if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
+	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
+	  fe->uid = cpu_to_le32(-1);
+	else if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
		fe->uid = cpu_to_le32(inode->i_uid);

-	if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
+	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
+	  fe->gid = cpu_to_le32(-1);
+	else if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
		fe->gid = cpu_to_le32(inode->i_gid);

	udfperms =	((inode->i_mode & S_IRWXO)     ) |
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 4a6f49a..368d8f8 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -269,7 +269,7 @@ enum {
	Opt_gid, Opt_uid, Opt_umask, Opt_session, Opt_lastblock,
	Opt_anchor, Opt_volume, Opt_partition, Opt_fileset,
	Opt_rootdir, Opt_utf8, Opt_iocharset,
-	Opt_err
+	Opt_err, Opt_uforget, Opt_uignore, Opt_gforget, Opt_gignore
};

static match_table_t tokens = {
@@ -282,6 +282,10 @@ static match_table_t tokens = {
	{Opt_adinicb, "adinicb"},
	{Opt_shortad, "shortad"},
	{Opt_longad, "longad"},
+	{Opt_uforget, "uid=forget"},
+	{Opt_uignore, "uid=ignore"},
+	{Opt_gforget, "gid=forget"},
+	{Opt_gignore, "gid=ignore"},
	{Opt_gid, "gid=%u"},
	{Opt_uid, "uid=%u"},
	{Opt_umask, "umask=%o"},
@@ -414,6 +418,18 @@ udf_parse_options(char *options, struct 
				uopt->flags |= (1 << UDF_FLAG_NLS_MAP);
				break;
#endif
+			case Opt_uignore:
+				uopt->flags |= (1 << UDF_FLAG_UID_IGNORE);
+				break;
+			case Opt_uforget:
+				uopt->flags |= (1 << UDF_FLAG_UID_FORGET);
+				break;
+			case Opt_gignore:
+			    uopt->flags |= (1 << UDF_FLAG_GID_IGNORE);
+				break;
+			case Opt_gforget:
+			    uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
+				break;
			default:
				printk(KERN_ERR "udf: bad mount option \"%s\" "
						"or missing value\n", p);
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 6636698..110f8d6 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -20,6 +20,10 @@
#define UDF_FLAG_VARCONV		8
#define UDF_FLAG_NLS_MAP		9
#define UDF_FLAG_UTF8			10
+#define UDF_FLAG_UID_FORGET     11    /* save -1 for uid to disk */
+#define UDF_FLAG_UID_IGNORE     12    /* use sb uid instead of on disk uid */
+#define UDF_FLAG_GID_FORGET     13
+#define UDF_FLAG_GID_IGNORE     14

#define UDF_PART_FLAG_UNALLOC_BITMAP	0x0001
#define UDF_PART_FLAG_UNALLOC_TABLE	0x0002
-- 
1.1.3

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Pekka Enberg]

[-- Attachment #1: Type: text/plain, Size: 3793 bytes --]

On 3/4/06, Phillip Susi <psusi@cfl.rr.com> wrote:
> This patch fixes a bug in udf where it would write uid/gid = 0 to the
> disk for files owned by the id given with the uid=/gid= mount options.

I don't think it does. You can still get zero uid/gid written on disk
if you specify uid/gid mount options when not specifying uid=forget. I
am not against new features but lets fix the bug first, okay? Adding
new mount options to work around buggy code is not the right solution.

The unconditional memset() is clearly a regression caused by Ben
Fennema's commit "[PATCH] UDF sync with CVS" in the old-2.6-bkcvs.git
repository and should be fixed. Care to try out the included untested
patch to see if it fixes the problem? (Sorry for the attachment, I am
replying via web mail.)

                                   Pekka

[PATCH] udf: fix uid/gid changing to zero

This patch fixes uid/gid changing to zero when uid/gid mount option is
used for UDF. The problem appears when the overriding uid/gid matches
inode. The fix is simple, instead of unconditionally zeroing buffer,
we now only do it for new inodes.

The regression appeared in commit "[PATCH] UDF sync with CVS" and was
first reported by Phillip Susi.

Cc: Phillip Susi <psusi@cfl.rr.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 fs/udf/ialloc.c          |    1 +
 fs/udf/inode.c           |    9 +++++++--
 fs/udf/udf_i.h           |    1 +
 include/linux/udf_fs_i.h |    3 ++-
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
index c9b707b..4bc3dc9 100644
--- a/fs/udf/ialloc.c
+++ b/fs/udf/ialloc.c
@@ -146,6 +146,7 @@ struct inode * udf_new_inode (struct ino
 		UDF_I_ALLOCTYPE(inode) = ICBTAG_FLAG_AD_LONG;
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		UDF_I_CRTIME(inode) = current_fs_time(inode->i_sb);
+	UDF_I_NEW_INODE(inode) = 1;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
 	up(&sbi->s_alloc_sem);
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 395e582..6937da2 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1003,6 +1003,8 @@ static void udf_fill_inode(struct inode
 	long convtime_usec;
 	int offset;

+	UDF_I_NEW_INODE(inode) = 0;
+
 	fe = (struct fileEntry *)bh->b_data;
 	efe = (struct extendedFileEntry *)bh->b_data;

@@ -1307,11 +1309,14 @@ udf_update_inode(struct inode *inode, in
 		return -EIO;
 	}

-	memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
-
 	fe = (struct fileEntry *)bh->b_data;
 	efe = (struct extendedFileEntry *)bh->b_data;

+	if (UDF_I_NEW_INODE(inode) == 1) {
+		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
+		UDF_I_NEW_INODE(inode) = 0;
+	}
+
 	if (le16_to_cpu(fe->descTag.tagIdent) == TAG_IDENT_USE)
 	{
 		struct unallocSpaceEntry *use =
diff --git a/fs/udf/udf_i.h b/fs/udf/udf_i.h
index d7dbe6f..87f82f7 100644
--- a/fs/udf/udf_i.h
+++ b/fs/udf/udf_i.h
@@ -16,6 +16,7 @@ static inline struct udf_inode_info *UDF
 #define UDF_I_EFE(X)		( UDF_I(X)->i_efe )
 #define UDF_I_USE(X)		( UDF_I(X)->i_use )
 #define UDF_I_STRAT4096(X)	( UDF_I(X)->i_strat4096 )
+#define UDF_I_NEW_INODE(X)	( UDF_I(X)->i_new_inode )
 #define UDF_I_NEXT_ALLOC_BLOCK(X)	( UDF_I(X)->i_next_alloc_block )
 #define UDF_I_NEXT_ALLOC_GOAL(X)	( UDF_I(X)->i_next_alloc_goal )
 #define UDF_I_CRTIME(X)		( UDF_I(X)->i_crtime )
diff --git a/include/linux/udf_fs_i.h b/include/linux/udf_fs_i.h
index 1e75084..c21415a 100644
--- a/include/linux/udf_fs_i.h
+++ b/include/linux/udf_fs_i.h
@@ -51,7 +51,8 @@ struct udf_inode_info
 	unsigned		i_efe : 1;
 	unsigned		i_use : 1;
 	unsigned		i_strat4096 : 1;
-	unsigned		reserved : 26;
+	unsigned		i_new_inode : 1;
+	unsigned		reserved : 25;
 	union
 	{
 		short_ad	*i_sad;

[-- Attachment #2: udf-fix --]
[-- Type: application/octet-stream, Size: 2873 bytes --]

[PATCH] udf: fix uid/gid changing to zero

This patch fixes uid/gid changing to zero when uid/gid mount option is
used for UDF. The problem appears when the overriding uid/gid matches
inode. The fix is simple, instead of unconditionally zeroing buffer,
we now only do it for new inodes.

The regression appeared in commit "[PATCH] UDF sync with CVS" and was
first reported by Phillip Susi.

Cc: Phillip Susi <psusi@cfl.rr.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 fs/udf/ialloc.c          |    1 +
 fs/udf/inode.c           |    9 +++++++--
 fs/udf/udf_i.h           |    1 +
 include/linux/udf_fs_i.h |    3 ++-
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/udf/ialloc.c b/fs/udf/ialloc.c
index c9b707b..4bc3dc9 100644
--- a/fs/udf/ialloc.c
+++ b/fs/udf/ialloc.c
@@ -146,6 +146,7 @@ struct inode * udf_new_inode (struct ino
 		UDF_I_ALLOCTYPE(inode) = ICBTAG_FLAG_AD_LONG;
 	inode->i_mtime = inode->i_atime = inode->i_ctime =
 		UDF_I_CRTIME(inode) = current_fs_time(inode->i_sb);
+	UDF_I_NEW_INODE(inode) = 1;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
 	up(&sbi->s_alloc_sem);
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 395e582..6937da2 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1003,6 +1003,8 @@ static void udf_fill_inode(struct inode 
 	long convtime_usec;
 	int offset;
 
+	UDF_I_NEW_INODE(inode) = 0;
+
 	fe = (struct fileEntry *)bh->b_data;
 	efe = (struct extendedFileEntry *)bh->b_data;
 
@@ -1307,11 +1309,14 @@ udf_update_inode(struct inode *inode, in
 		return -EIO;
 	}
 
-	memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
-
 	fe = (struct fileEntry *)bh->b_data;
 	efe = (struct extendedFileEntry *)bh->b_data;
 
+	if (UDF_I_NEW_INODE(inode) == 1) {
+		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
+		UDF_I_NEW_INODE(inode) = 0;
+	}
+
 	if (le16_to_cpu(fe->descTag.tagIdent) == TAG_IDENT_USE)
 	{
 		struct unallocSpaceEntry *use =
diff --git a/fs/udf/udf_i.h b/fs/udf/udf_i.h
index d7dbe6f..87f82f7 100644
--- a/fs/udf/udf_i.h
+++ b/fs/udf/udf_i.h
@@ -16,6 +16,7 @@ static inline struct udf_inode_info *UDF
 #define UDF_I_EFE(X)		( UDF_I(X)->i_efe )
 #define UDF_I_USE(X)		( UDF_I(X)->i_use )
 #define UDF_I_STRAT4096(X)	( UDF_I(X)->i_strat4096 )
+#define UDF_I_NEW_INODE(X)	( UDF_I(X)->i_new_inode )
 #define UDF_I_NEXT_ALLOC_BLOCK(X)	( UDF_I(X)->i_next_alloc_block )
 #define UDF_I_NEXT_ALLOC_GOAL(X)	( UDF_I(X)->i_next_alloc_goal )
 #define UDF_I_CRTIME(X)		( UDF_I(X)->i_crtime )
diff --git a/include/linux/udf_fs_i.h b/include/linux/udf_fs_i.h
index 1e75084..c21415a 100644
--- a/include/linux/udf_fs_i.h
+++ b/include/linux/udf_fs_i.h
@@ -51,7 +51,8 @@ struct udf_inode_info
 	unsigned		i_efe : 1;
 	unsigned		i_use : 1;
 	unsigned		i_strat4096 : 1;
-	unsigned		reserved : 26;
+	unsigned		i_new_inode : 1;
+	unsigned		reserved : 25;
 	union
 	{
 		short_ad	*i_sad;

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Phillip Susi]

Pekka Enberg wrote:
> On 3/4/06, Phillip Susi <psusi@cfl.rr.com> wrote:
>> This patch fixes a bug in udf where it would write uid/gid = 0 to the
>> disk for files owned by the id given with the uid=/gid= mount options.
> 
> I don't think it does. You can still get zero uid/gid written on disk
> if you specify uid/gid mount options when not specifying uid=forget. I
> am not against new features but lets fix the bug first, okay? Adding
> new mount options to work around buggy code is not the right solution.
> 

My bad, I meant to remove those ifs, not just prepend them with an else.  Try this one:

Subject: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

This patch fixes a bug in udf where it would write uid/gid = 0 to the
disk for files owned by the id given with the uid=/gid= mount options.
It also adds 4 new mount options: uid/gid=forget and uid/gid=ignore.
Without any options the id in core and on disk always match.  Giving
uid/gid=nnn specifies a default ID to be used in core when the on disk ID
is -1.  uid/gid=ignore forces the in core ID to allways be used no matter
what the on disk ID is.  uid/gid=forget forces the on disk ID to always be
written out as -1.

The use of these options allows you to override ownerships on a disk or
disable ownwership information from being written, allowing the media
to be used portably between different computers and possibly different users
without permissions issues that would require root to correct.

Signed-off-by: Phillip Susi <psusi@cfl.rr.com>

---

 fs/udf/inode.c  |   18 +++++++++++-------
 fs/udf/super.c  |   18 +++++++++++++++++-
 fs/udf/udf_sb.h |    4 ++++
 3 files changed, 32 insertions(+), 8 deletions(-)

c8393f6e4fe6159fd916f3c68091e76bbfdc5fd8
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 395e582..49aeac3 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1045,10 +1045,12 @@ static void udf_fill_inode(struct inode 
 	}
 
 	inode->i_uid = le32_to_cpu(fe->uid);
-	if ( inode->i_uid == -1 ) inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
+	if ( inode->i_uid == -1 || UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_IGNORE) )
+	  inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
 
 	inode->i_gid = le32_to_cpu(fe->gid);
-	if ( inode->i_gid == -1 ) inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
+	if ( inode->i_gid == -1 || UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_IGNORE) )
+	  inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
 
 	inode->i_nlink = le16_to_cpu(fe->fileLinkCount);
 	if (!inode->i_nlink)
@@ -1335,11 +1337,13 @@ udf_update_inode(struct inode *inode, in
 		return err;
 	}
 
-	if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
-		fe->uid = cpu_to_le32(inode->i_uid);
-
-	if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
-		fe->gid = cpu_to_le32(inode->i_gid);
+	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
+	  fe->uid = cpu_to_le32(-1);
+	else fe->uid = cpu_to_le32(inode->i_uid);
+
+	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
+	  fe->gid = cpu_to_le32(-1);
+	else fe->gid = cpu_to_le32(inode->i_gid);
 
 	udfperms =	((inode->i_mode & S_IRWXO)     ) |
 			((inode->i_mode & S_IRWXG) << 2) |
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 4a6f49a..368d8f8 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -269,7 +269,7 @@ enum {
 	Opt_gid, Opt_uid, Opt_umask, Opt_session, Opt_lastblock,
 	Opt_anchor, Opt_volume, Opt_partition, Opt_fileset,
 	Opt_rootdir, Opt_utf8, Opt_iocharset,
-	Opt_err
+	Opt_err, Opt_uforget, Opt_uignore, Opt_gforget, Opt_gignore
 };
 
 static match_table_t tokens = {
@@ -282,6 +282,10 @@ static match_table_t tokens = {
 	{Opt_adinicb, "adinicb"},
 	{Opt_shortad, "shortad"},
 	{Opt_longad, "longad"},
+	{Opt_uforget, "uid=forget"},
+	{Opt_uignore, "uid=ignore"},
+	{Opt_gforget, "gid=forget"},
+	{Opt_gignore, "gid=ignore"},
 	{Opt_gid, "gid=%u"},
 	{Opt_uid, "uid=%u"},
 	{Opt_umask, "umask=%o"},
@@ -414,6 +418,18 @@ udf_parse_options(char *options, struct 
 				uopt->flags |= (1 << UDF_FLAG_NLS_MAP);
 				break;
 #endif
+			case Opt_uignore:
+				uopt->flags |= (1 << UDF_FLAG_UID_IGNORE);
+				break;
+			case Opt_uforget:
+				uopt->flags |= (1 << UDF_FLAG_UID_FORGET);
+				break;
+			case Opt_gignore:
+			    uopt->flags |= (1 << UDF_FLAG_GID_IGNORE);
+				break;
+			case Opt_gforget:
+			    uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
+				break;
 			default:
 				printk(KERN_ERR "udf: bad mount option \"%s\" "
 						"or missing value\n", p);
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 6636698..110f8d6 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -20,6 +20,10 @@
 #define UDF_FLAG_VARCONV		8
 #define UDF_FLAG_NLS_MAP		9
 #define UDF_FLAG_UTF8			10
+#define UDF_FLAG_UID_FORGET     11    /* save -1 for uid to disk */
+#define UDF_FLAG_UID_IGNORE     12    /* use sb uid instead of on disk uid */
+#define UDF_FLAG_GID_FORGET     13
+#define UDF_FLAG_GID_IGNORE     14
 
 #define UDF_PART_FLAG_UNALLOC_BITMAP	0x0001
 #define UDF_PART_FLAG_UNALLOC_TABLE	0x0002
-- 
1.1.3

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Pekka J Enberg]

Hi Phillip,

On Sun, 5 Mar 2006, Phillip Susi wrote:
> My bad, I meant to remove those ifs, not just prepend them with an else.  Try
> this one:
> 
> c8393f6e4fe6159fd916f3c68091e76bbfdc5fd8
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index 395e582..49aeac3 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1045,10 +1045,12 @@ static void udf_fill_inode(struct inode        }
> 
>        inode->i_uid = le32_to_cpu(fe->uid);
> -      if ( inode->i_uid == -1 ) inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
> +      if ( inode->i_uid == -1 || UDF_QUERY_FLAG(inode->i_sb,
> UDF_FLAG_UID_IGNORE) )
> +        inode->i_uid = UDF_SB(inode->i_sb)->s_uid;

Formatting.

>        inode->i_gid = le32_to_cpu(fe->gid);
> -      if ( inode->i_gid == -1 ) inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
> +      if ( inode->i_gid == -1 || UDF_QUERY_FLAG(inode->i_sb,
> UDF_FLAG_GID_IGNORE) )
> +        inode->i_gid = UDF_SB(inode->i_sb)->s_gid;

Same here.

>        inode->i_nlink = le16_to_cpu(fe->fileLinkCount);
>        if (!inode->i_nlink)
> @@ -1335,11 +1337,13 @@ udf_update_inode(struct inode *inode, in
>        return err;
>        }
> 
> -      if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
> -      fe->uid = cpu_to_le32(inode->i_uid);
> -
> -      if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
> -      fe->gid = cpu_to_le32(inode->i_gid);
> +      if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
> +        fe->uid = cpu_to_le32(-1);
> +      else fe->uid = cpu_to_le32(inode->i_uid);

This is better, but if id was -1 on disk, we're overriding it unless the 
forget mount option was specified. Do we want that? I think my patch is a 
better fix (if it works anyway) and yours should be on top of that. Did 
you have the chance to test it?

Also, formatting is, wrong, just make it

	if (forget)
		fe->uid = ...;
	else
		fe->uid = ...;

> +      if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
> +        fe->gid = cpu_to_le32(-1);
> +      else fe->gid = cpu_to_le32(inode->i_gid);
> 
>        udfperms =      ((inode->i_mode & S_IRWXO)     ) |
>        ((inode->i_mode & S_IRWXG) << 2) |

Same here.

Please document the new mount options in Documentation/filesystems/udf.txt.

			Pekka

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Phillip Susi]

Pekka J Enberg wrote:
> Hi Phillip,
> 
> On Sun, 5 Mar 2006, Phillip Susi wrote:
>> My bad, I meant to remove those ifs, not just prepend them with an else.  Try
>> this one:
>>
>> c8393f6e4fe6159fd916f3c68091e76bbfdc5fd8
>> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
>> index 395e582..49aeac3 100644
>> --- a/fs/udf/inode.c
>> +++ b/fs/udf/inode.c
>> @@ -1045,10 +1045,12 @@ static void udf_fill_inode(struct inode        }
>>
>>        inode->i_uid = le32_to_cpu(fe->uid);
>> -      if ( inode->i_uid == -1 ) inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
>> +      if ( inode->i_uid == -1 || UDF_QUERY_FLAG(inode->i_sb,
>> UDF_FLAG_UID_IGNORE) )
>> +        inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
> 
> Formatting.
> 

What exactly do you mean by formatting?

>>        inode->i_gid = le32_to_cpu(fe->gid);
>> -      if ( inode->i_gid == -1 ) inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
>> +      if ( inode->i_gid == -1 || UDF_QUERY_FLAG(inode->i_sb,
>> UDF_FLAG_GID_IGNORE) )
>> +        inode->i_gid = UDF_SB(inode->i_sb)->s_gid;
> 
> Same here.
> 
>>        inode->i_nlink = le16_to_cpu(fe->fileLinkCount);
>>        if (!inode->i_nlink)
>> @@ -1335,11 +1337,13 @@ udf_update_inode(struct inode *inode, in
>>        return err;
>>        }
>>
>> -      if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
>> -      fe->uid = cpu_to_le32(inode->i_uid);
>> -
>> -      if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
>> -      fe->gid = cpu_to_le32(inode->i_gid);
>> +      if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
>> +        fe->uid = cpu_to_le32(-1);
>> +      else fe->uid = cpu_to_le32(inode->i_uid);
> 
> This is better, but if id was -1 on disk, we're overriding it unless the 
> forget mount option was specified. Do we want that? I think my patch is a 
> better fix (if it works anyway) and yours should be on top of that. Did 
> you have the chance to test it?
> 

Yes, if you chown a file that is owned by -1 on disk, you do want it to 
be saved back with the new id, unless you set the forget option.

> Also, formatting is, wrong, just make it
> 
> 	if (forget)
> 		fe->uid = ...;
> 	else
> 		fe->uid = ...;
> 

You mean use hard tabstops instead of two spaces to indent?  Is there a 
way to set emacs to do that?  It automatically uses the two spaces.

>> +      if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
>> +        fe->gid = cpu_to_le32(-1);
>> +      else fe->gid = cpu_to_le32(inode->i_gid);
>>
>>        udfperms =      ((inode->i_mode & S_IRWXO)     ) |
>>        ((inode->i_mode & S_IRWXG) << 2) |
> 
> Same here.
> 
> Please document the new mount options in Documentation/filesystems/udf.txt.
> 

Working on it.

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Pekka J Enberg]

On Mon, 6 Mar 2006, Phillip Susi wrote:
> > >        inode->i_uid = le32_to_cpu(fe->uid);
> > > -      if ( inode->i_uid == -1 ) inode->i_uid =
> > > UDF_SB(inode->i_sb)->s_uid;
> > > +      if ( inode->i_uid == -1 || UDF_QUERY_FLAG(inode->i_sb,
> > > UDF_FLAG_UID_IGNORE) )
> > > +        inode->i_uid = UDF_SB(inode->i_sb)->s_uid;
> > 
> > Formatting.
> 
> What exactly do you mean by formatting?

Looks like you're using two spaces. Indentation is one tab and one tab is 
exactly eight characters (see Documentation/CodingStyle).

On Mon, 6 Mar 2006, Phillip Susi wrote:
> Yes, if you chown a file that is owned by -1 on disk, you do want it to be
> saved back with the new id, unless you set the forget option.

Okay, fair enough. I see akpm has taken your patch. Please make sure the 
mount options are documented. Thanks!

(Please note that we didn't fix the unconditional memset now, so there's 
 dead code in udf_update_inode(). The check for TAG_IDENT_USE will always 
 fail.)

			Pekka

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Phillip Susi]

Pekka J Enberg wrote:
> Looks like you're using two spaces. Indentation is one tab and one tab is 
> exactly eight characters (see Documentation/CodingStyle).
> 

Wow!  8 spaces?  I always go with 4, 8 wastes way too much screen space. 
  Is there a handy incantation to get emacs to auto indent with a hard 
tab rather than 2 spaces like it does by default?

> Okay, fair enough. I see akpm has taken your patch. Please make sure the 
> mount options are documented. Thanks!
> 

I wrote up some docs for it last night.

> (Please note that we didn't fix the unconditional memset now, so there's 
>  dead code in udf_update_inode(). The check for TAG_IDENT_USE will always 
>  fail.)

Hrm... I'll take a look at that tonight and see what I come up with. 
Probably will resubmit the patch tomorrow.

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

On Tue, 07 Mar 2006 10:49:14 -0500 Phillip Susi wrote:

> Pekka J Enberg wrote:
> > Looks like you're using two spaces. Indentation is one tab and one tab is 
> > exactly eight characters (see Documentation/CodingStyle).
> > 
> 
> Wow!  8 spaces?  I always go with 4, 8 wastes way too much screen space. 

See Documentation/CodingStyle for reasons.

>   Is there a handy incantation to get emacs to auto indent with a hard 
> tab rather than 2 spaces like it does by default?

C-c . linux RET   (C-c . runs c-set-style; suggestions in
Documentation/CodingStyle are outdated).

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Phillip Susi]

Andrew Morton wrote:
> I didn't see that update, and I don't miss much.
> 

See http://lkml.org/lkml/2006/3/5/171

> Please provide a description of this change.  What problem is it fixing? 
> How does it fix it?  What are the consequences of not making this change?

In the initial patch I made a typo.  As Pekka Enberg pointed out, with 
the if still following the else, you can still get a null uid written to 
the disk if you specify a default uid= without uid=forget.  In other 
words, if the desktop user is uid=1000 and the mount option uid=1000 is 
given ( which is done on ubuntu automatically and probably other 
distributions that use hal ), then if any other user besides uid 1000 
owns a file then a 0 will be written to the media as the owning uid 
instead.  This is exactly what the original patch was trying to prevent.

Re: [PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Andrew Morton]

Phillip Susi <psusi@cfl.rr.com> wrote:
>
> This patch corrects the incorrect patch previously applied in
> commit 4d6660eb3665f22d16aff466eb9d45df6102b254.  I had posted
> the correction as a reply on lkml, but it seems that the earlier
> and incorrect patch got merged into Linus's tree.

I didn't see that update, and I don't miss much.

> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -1341,13 +1341,11 @@ udf_update_inode(struct inode *inode, in
> 
> 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
> 		fe->uid = cpu_to_le32(-1);
> -	else if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
> -		fe->uid = cpu_to_le32(inode->i_uid);
> +	else fe->uid = cpu_to_le32(inode->i_uid);
> 
> 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
> 		fe->gid = cpu_to_le32(-1);
> -	else if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
> -		fe->gid = cpu_to_le32(inode->i_gid);
> +	else fe->gid = cpu_to_le32(inode->i_gid);
> 
> 	udfperms =	((inode->i_mode & S_IRWXO)     ) |
> 			((inode->i_mode & S_IRWXG) << 2) |

This is an unchangelogged alteration.

Please provide a description of this change.  What problem is it fixing? 
How does it fix it?  What are the consequences of not making this change?

Thanks.

[PATCH] udf: fix uid/gid options and add uid/gid=ignore and forget options

[Phillip Susi]

This patch corrects the incorrect patch previously applied in
commit 4d6660eb3665f22d16aff466eb9d45df6102b254.  I had posted
the correction as a reply on lkml, but it seems that the earlier
and incorrect patch got merged into Linus's tree.  This patch
also adds documentation for the changes made last time.

---

Documentation/filesystems/udf.txt |   14 ++++++++++++++
fs/udf/inode.c                    |    6 ++----
2 files changed, 16 insertions(+), 4 deletions(-)

83a730fb2e864dae308008d83c5d804afc539f08
diff --git a/Documentation/filesystems/udf.txt b/Documentation/filesystems/udf.txt
index e5213bc..85ff288 100644
--- a/Documentation/filesystems/udf.txt
+++ b/Documentation/filesystems/udf.txt
@@ -26,6 +26,20 @@ The following mount options are supporte
	nostrict	Unset strict conformance
	iocharset=	Set the NLS character set

+The uid= and gid= options need a bit more explaining.  They will accept a
+decimal numeric value which will be used as the default ID for that mount.
+They will also accept the string "ignore" and "forget".  For files on the disk
+that are owned by nobody ( -1 ), they will instead look as if they are owned
+by the default ID.  The ignore option causes the default ID to override all
+IDs on the disk, not just -1.  The forget option causes all IDs to be written
+to disk as -1, so when the media is later remounted, they will appear to be
+owned by whatever default ID it is mounted with at that time.  
+
+For typical desktop use of removable media, you should set the ID to that
+of the interactively logged on user, and also specify both the forget and
+ignore options.  This way the interactive user will always see the files
+on the disk as belonging to him.  
+
The remaining are for debugging and disaster recovery:

	novrs		Skip volume sequence recognition 
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index d04cff2..81e0e84 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1341,13 +1341,11 @@ udf_update_inode(struct inode *inode, in

	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_UID_FORGET))
		fe->uid = cpu_to_le32(-1);
-	else if (inode->i_uid != UDF_SB(inode->i_sb)->s_uid)
-		fe->uid = cpu_to_le32(inode->i_uid);
+	else fe->uid = cpu_to_le32(inode->i_uid);

	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_GID_FORGET))
		fe->gid = cpu_to_le32(-1);
-	else if (inode->i_gid != UDF_SB(inode->i_sb)->s_gid)
-		fe->gid = cpu_to_le32(inode->i_gid);
+	else fe->gid = cpu_to_le32(inode->i_gid);

	udfperms =	((inode->i_mode & S_IRWXO)     ) |
			((inode->i_mode & S_IRWXG) << 2) |
-- 
1.1.3