LinuxLists.cc - [patch 25/26] mount options: fix udf

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

[Miklos Szeredi - Thu, Jan 24, 2008 at 08:34:06PM +0100]
| From: Miklos Szeredi <[email protected]>
|
| Add a .show_options super operation to udf.
|
| Signed-off-by: Miklos Szeredi <[email protected]>
| ---
|
| Index: linux/fs/udf/super.c
| ===================================================================
| --- linux.orig/fs/udf/super.c 2008-01-24 13:48:37.000000000 +0100
| +++ linux/fs/udf/super.c 2008-01-24 15:58:21.000000000 +0100
| @@ -53,6 +53,8 @@
| #include <linux/vfs.h>
| #include <linux/vmalloc.h>
| #include <linux/errno.h>
| +#include <linux/mount.h>
| +#include <linux/seq_file.h>
| #include <asm/byteorder.h>
|
| #include <linux/udf_fs.h>
| @@ -71,6 +73,8 @@
| #define VDS_POS_TERMINATING_DESC 6
| #define VDS_POS_LENGTH 7
|
| +#define UDF_DEFAULT_BLOCKSIZE 2048
| +
| static char error_buf[1024];
|
| /* These are the "meat" - everything else is stuffing */
| @@ -95,6 +99,7 @@ static void udf_open_lvid(struct super_b
| static void udf_close_lvid(struct super_block *);
| static unsigned int udf_count_free(struct super_block *);
| static int udf_statfs(struct dentry *, struct kstatfs *);
| +static int udf_show_options(struct seq_file *, struct vfsmount *);
|
| struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct udf_sb_info *sbi)
| {
| @@ -181,6 +186,7 @@ static const struct super_operations udf
| .write_super = udf_write_super,
| .statfs = udf_statfs,
| .remount_fs = udf_remount_fs,
| + .show_options = udf_show_options,
| };
|
| struct udf_options {
| @@ -247,6 +253,56 @@ static int udf_sb_alloc_partition_maps(s
| return 0;
| }
|
| +static int udf_show_options(struct seq_file *seq, struct vfsmount *mnt)
| +{
| + struct super_block *sb = mnt->mnt_sb;
| + struct udf_sb_info *sbi = UDF_SB(sb);
| +
| + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT))
| + seq_puts(seq, ",nostrict");
| + if (sb->s_blocksize != UDF_DEFAULT_BLOCKSIZE)
| + seq_printf(seq, ",bs=%lu", sb->s_blocksize);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNHIDE))
| + seq_puts(seq, ",unhide");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNDELETE))
| + seq_puts(seq, ",undelete");
| + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_USE_AD_IN_ICB))
| + seq_puts(seq, ",noadinicb");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_USE_SHORT_AD))
| + seq_puts(seq, ",shortad");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_FORGET))
| + seq_puts(seq, ",uid=forget");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_IGNORE))
| + seq_puts(seq, ",uid=ignore");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_FORGET))
| + seq_puts(seq, ",gid=forget");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_IGNORE))
| + seq_puts(seq, ",gid=ignore");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
| + seq_printf(seq, ",uid=%u", sbi->s_uid);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET))
| + seq_printf(seq, ",gid=%u", sbi->s_gid);
| + if (sbi->s_umask != 0)
| + seq_printf(seq, ",umask=%o", sbi->s_umask);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_SESSION_SET))
| + seq_printf(seq, ",session=%u", sbi->s_session);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_LASTBLOCK_SET))
| + seq_printf(seq, ",lastblock=%u", sbi->s_last_block);
| + /* is this correct? */
| + if (sbi->s_anchor[2] != 0)
| + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
| + /*
| + * volume, partition, fileset and rootdir seem to be ignored
| + * currently
| + */
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8))
| + seq_puts(seq, ",utf8");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP) && sbi->s_nls_map)
| + seq_printf(seq, ",iocharset=%s", sbi->s_nls_map->charset);
| +
| + return 0;
| +}
| +
| /*
| * udf_parse_options
| *
| @@ -339,13 +395,14 @@ static match_table_t tokens = {
| {Opt_err, NULL}
| };
|
| -static int udf_parse_options(char *options, struct udf_options *uopt)
| +static int udf_parse_options(char *options, struct udf_options *uopt,
| + bool remount)
| {
| char *p;
| int option;
|
| uopt->novrs = 0;
| - uopt->blocksize = 2048;
| + uopt->blocksize = UDF_DEFAULT_BLOCKSIZE;
| uopt->partition = 0xFFFF;
| uopt->session = 0xFFFFFFFF;
| uopt->lastblock = 0;
| @@ -415,11 +472,15 @@ static int udf_parse_options(char *optio
| if (match_int(args, &option))
| return 0;
| uopt->session = option;
| + if (!remount)
| + uopt->flags |= (1 << UDF_FLAG_SESSION_SET);
| break;
| case Opt_lastblock:
| if (match_int(args, &option))
| return 0;
| uopt->lastblock = option;
| + if (!remount)
| + uopt->flags |= (1 << UDF_FLAG_LASTBLOCK_SET);
| break;
| case Opt_anchor:
| if (match_int(args, &option))
| @@ -497,7 +558,7 @@ static int udf_remount_fs(struct super_b
| uopt.gid = sbi->s_gid;
| uopt.umask = sbi->s_umask;
|
| - if (!udf_parse_options(options, &uopt))
| + if (!udf_parse_options(options, &uopt, true))
| return -EINVAL;
|
| sbi->s_flags = uopt.flags;
| @@ -1679,7 +1740,7 @@ static int udf_fill_super(struct super_b
|
| mutex_init(&sbi->s_alloc_mutex);
|
| - if (!udf_parse_options((char *)options, &uopt))
| + if (!udf_parse_options((char *)options, &uopt, false))
| goto error_out;
|
| if (uopt.flags & (1 << UDF_FLAG_UTF8) &&
| Index: linux/fs/udf/udf_sb.h
| ===================================================================
| --- linux.orig/fs/udf/udf_sb.h 2008-01-24 13:48:37.000000000 +0100
| +++ linux/fs/udf/udf_sb.h 2008-01-24 13:51:08.000000000 +0100
| @@ -26,6 +26,8 @@
| #define UDF_FLAG_GID_IGNORE 14
| #define UDF_FLAG_UID_SET 15
| #define UDF_FLAG_GID_SET 16
| +#define UDF_FLAG_SESSION_SET 17
| +#define UDF_FLAG_LASTBLOCK_SET 18
|
| #define UDF_PART_FLAG_UNALLOC_BITMAP 0x0001
| #define UDF_PART_FLAG_UNALLOC_TABLE 0x0002
|
| --
|

Jan Kara CC'ed

- Cyrill -

2008-01-24 20:21:14

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

[Miklos Szeredi - Thu, Jan 24, 2008 at 08:34:06PM +0100]
| From: Miklos Szeredi <[email protected]>
|
| Add a .show_options super operation to udf.
|
| Signed-off-by: Miklos Szeredi <[email protected]>
| ---
|
| Index: linux/fs/udf/super.c
| ===================================================================
| --- linux.orig/fs/udf/super.c 2008-01-24 13:48:37.000000000 +0100
| +++ linux/fs/udf/super.c 2008-01-24 15:58:21.000000000 +0100
| @@ -53,6 +53,8 @@
| #include <linux/vfs.h>
| #include <linux/vmalloc.h>
| #include <linux/errno.h>
| +#include <linux/mount.h>
| +#include <linux/seq_file.h>
| #include <asm/byteorder.h>
|
| #include <linux/udf_fs.h>
| @@ -71,6 +73,8 @@
| #define VDS_POS_TERMINATING_DESC 6
| #define VDS_POS_LENGTH 7
|
| +#define UDF_DEFAULT_BLOCKSIZE 2048
| +

thanks, that is good cleanup

| static char error_buf[1024];
|
| /* These are the "meat" - everything else is stuffing */
| @@ -95,6 +99,7 @@ static void udf_open_lvid(struct super_b
| static void udf_close_lvid(struct super_block *);
| static unsigned int udf_count_free(struct super_block *);
| static int udf_statfs(struct dentry *, struct kstatfs *);
| +static int udf_show_options(struct seq_file *, struct vfsmount *);
|
| struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct udf_sb_info *sbi)
| {
| @@ -181,6 +186,7 @@ static const struct super_operations udf
| .write_super = udf_write_super,
| .statfs = udf_statfs,
| .remount_fs = udf_remount_fs,
| + .show_options = udf_show_options,
| };
|
| struct udf_options {
| @@ -247,6 +253,56 @@ static int udf_sb_alloc_partition_maps(s
| return 0;
| }
|
| +static int udf_show_options(struct seq_file *seq, struct vfsmount *mnt)
| +{
| + struct super_block *sb = mnt->mnt_sb;
| + struct udf_sb_info *sbi = UDF_SB(sb);
| +
| + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT))
| + seq_puts(seq, ",nostrict");
| + if (sb->s_blocksize != UDF_DEFAULT_BLOCKSIZE)
| + seq_printf(seq, ",bs=%lu", sb->s_blocksize);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNHIDE))
| + seq_puts(seq, ",unhide");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNDELETE))
| + seq_puts(seq, ",undelete");
| + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_USE_AD_IN_ICB))
| + seq_puts(seq, ",noadinicb");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_USE_SHORT_AD))
| + seq_puts(seq, ",shortad");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_FORGET))
| + seq_puts(seq, ",uid=forget");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_IGNORE))
| + seq_puts(seq, ",uid=ignore");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_FORGET))
| + seq_puts(seq, ",gid=forget");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_IGNORE))
| + seq_puts(seq, ",gid=ignore");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
| + seq_printf(seq, ",uid=%u", sbi->s_uid);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET))
| + seq_printf(seq, ",gid=%u", sbi->s_gid);
| + if (sbi->s_umask != 0)
| + seq_printf(seq, ",umask=%o", sbi->s_umask);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_SESSION_SET))
| + seq_printf(seq, ",session=%u", sbi->s_session);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_LASTBLOCK_SET))
| + seq_printf(seq, ",lastblock=%u", sbi->s_last_block);
| + /* is this correct? */
| + if (sbi->s_anchor[2] != 0)
| + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);

you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
in sake of style unification but we should wait for Jan's
decision (i'm not the expert in this area ;)

| + /*
| + * volume, partition, fileset and rootdir seem to be ignored
| + * currently
| + */
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8))
| + seq_puts(seq, ",utf8");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP) && sbi->s_nls_map)
| + seq_printf(seq, ",iocharset=%s", sbi->s_nls_map->charset);
| +
| + return 0;
| +}
| +
| /*
| * udf_parse_options
| *
| @@ -339,13 +395,14 @@ static match_table_t tokens = {
| {Opt_err, NULL}
| };
|
| -static int udf_parse_options(char *options, struct udf_options *uopt)
| +static int udf_parse_options(char *options, struct udf_options *uopt,
| + bool remount)
| {
| char *p;
| int option;
|
| uopt->novrs = 0;
| - uopt->blocksize = 2048;
| + uopt->blocksize = UDF_DEFAULT_BLOCKSIZE;
| uopt->partition = 0xFFFF;
| uopt->session = 0xFFFFFFFF;
| uopt->lastblock = 0;
| @@ -415,11 +472,15 @@ static int udf_parse_options(char *optio
| if (match_int(args, &option))
| return 0;
| uopt->session = option;
| + if (!remount)
| + uopt->flags |= (1 << UDF_FLAG_SESSION_SET);
| break;
| case Opt_lastblock:
| if (match_int(args, &option))
| return 0;
| uopt->lastblock = option;
| + if (!remount)
| + uopt->flags |= (1 << UDF_FLAG_LASTBLOCK_SET);
| break;
| case Opt_anchor:
| if (match_int(args, &option))
| @@ -497,7 +558,7 @@ static int udf_remount_fs(struct super_b
| uopt.gid = sbi->s_gid;
| uopt.umask = sbi->s_umask;
|
| - if (!udf_parse_options(options, &uopt))
| + if (!udf_parse_options(options, &uopt, true))
| return -EINVAL;
|
| sbi->s_flags = uopt.flags;
| @@ -1679,7 +1740,7 @@ static int udf_fill_super(struct super_b
|
| mutex_init(&sbi->s_alloc_mutex);
|
| - if (!udf_parse_options((char *)options, &uopt))
| + if (!udf_parse_options((char *)options, &uopt, false))
| goto error_out;
|
| if (uopt.flags & (1 << UDF_FLAG_UTF8) &&
| Index: linux/fs/udf/udf_sb.h
| ===================================================================
| --- linux.orig/fs/udf/udf_sb.h 2008-01-24 13:48:37.000000000 +0100
| +++ linux/fs/udf/udf_sb.h 2008-01-24 13:51:08.000000000 +0100
| @@ -26,6 +26,8 @@
| #define UDF_FLAG_GID_IGNORE 14
| #define UDF_FLAG_UID_SET 15
| #define UDF_FLAG_GID_SET 16
| +#define UDF_FLAG_SESSION_SET 17
| +#define UDF_FLAG_LASTBLOCK_SET 18
|
| #define UDF_PART_FLAG_UNALLOC_BITMAP 0x0001
| #define UDF_PART_FLAG_UNALLOC_TABLE 0x0002
|
| --
|

Other then that it looks ok for me (feel free to Ack by me
if you need it ;)

- Cyrill -

2008-01-24 20:46:43

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

[Miklos Szeredi - Thu, Jan 24, 2008 at 08:34:06PM +0100]
| From: Miklos Szeredi <[email protected]>
|
| Add a .show_options super operation to udf.
|
| Signed-off-by: Miklos Szeredi <[email protected]>
| ---
|
| Index: linux/fs/udf/super.c
| ===================================================================
| --- linux.orig/fs/udf/super.c 2008-01-24 13:48:37.000000000 +0100
| +++ linux/fs/udf/super.c 2008-01-24 15:58:21.000000000 +0100
| @@ -53,6 +53,8 @@
| #include <linux/vfs.h>
| #include <linux/vmalloc.h>
| #include <linux/errno.h>
| +#include <linux/mount.h>
| +#include <linux/seq_file.h>
| #include <asm/byteorder.h>
|
| #include <linux/udf_fs.h>
| @@ -71,6 +73,8 @@
| #define VDS_POS_TERMINATING_DESC 6
| #define VDS_POS_LENGTH 7
|
| +#define UDF_DEFAULT_BLOCKSIZE 2048
| +
| static char error_buf[1024];
|
| /* These are the "meat" - everything else is stuffing */
| @@ -95,6 +99,7 @@ static void udf_open_lvid(struct super_b
| static void udf_close_lvid(struct super_block *);
| static unsigned int udf_count_free(struct super_block *);
| static int udf_statfs(struct dentry *, struct kstatfs *);
| +static int udf_show_options(struct seq_file *, struct vfsmount *);
|
| struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct udf_sb_info *sbi)
| {
| @@ -181,6 +186,7 @@ static const struct super_operations udf
| .write_super = udf_write_super,
| .statfs = udf_statfs,
| .remount_fs = udf_remount_fs,
| + .show_options = udf_show_options,
| };
|
| struct udf_options {
| @@ -247,6 +253,56 @@ static int udf_sb_alloc_partition_maps(s
| return 0;
| }
|
| +static int udf_show_options(struct seq_file *seq, struct vfsmount *mnt)
| +{
| + struct super_block *sb = mnt->mnt_sb;
| + struct udf_sb_info *sbi = UDF_SB(sb);
| +
| + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT))
| + seq_puts(seq, ",nostrict");
| + if (sb->s_blocksize != UDF_DEFAULT_BLOCKSIZE)
| + seq_printf(seq, ",bs=%lu", sb->s_blocksize);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNHIDE))
| + seq_puts(seq, ",unhide");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNDELETE))
| + seq_puts(seq, ",undelete");
| + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_USE_AD_IN_ICB))
| + seq_puts(seq, ",noadinicb");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_USE_SHORT_AD))
| + seq_puts(seq, ",shortad");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_FORGET))
| + seq_puts(seq, ",uid=forget");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_IGNORE))
| + seq_puts(seq, ",uid=ignore");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_FORGET))
| + seq_puts(seq, ",gid=forget");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_IGNORE))
| + seq_puts(seq, ",gid=ignore");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
| + seq_printf(seq, ",uid=%u", sbi->s_uid);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET))
| + seq_printf(seq, ",gid=%u", sbi->s_gid);
| + if (sbi->s_umask != 0)
| + seq_printf(seq, ",umask=%o", sbi->s_umask);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_SESSION_SET))
| + seq_printf(seq, ",session=%u", sbi->s_session);
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_LASTBLOCK_SET))
| + seq_printf(seq, ",lastblock=%u", sbi->s_last_block);
| + /* is this correct? */
| + if (sbi->s_anchor[2] != 0)
| + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);

btw, Miklos, i think you may strip off != and use just if (val)
form, like "if (sbi->s_umask)"

| + /*
| + * volume, partition, fileset and rootdir seem to be ignored
| + * currently
| + */
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8))
| + seq_puts(seq, ",utf8");
| + if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP) && sbi->s_nls_map)
| + seq_printf(seq, ",iocharset=%s", sbi->s_nls_map->charset);
| +
| + return 0;
| +}
| +
| /*
| * udf_parse_options
| *
| @@ -339,13 +395,14 @@ static match_table_t tokens = {
| {Opt_err, NULL}
| };
|
| -static int udf_parse_options(char *options, struct udf_options *uopt)
| +static int udf_parse_options(char *options, struct udf_options *uopt,
| + bool remount)
| {
| char *p;
| int option;
|
| uopt->novrs = 0;
| - uopt->blocksize = 2048;
| + uopt->blocksize = UDF_DEFAULT_BLOCKSIZE;
| uopt->partition = 0xFFFF;
| uopt->session = 0xFFFFFFFF;
| uopt->lastblock = 0;
| @@ -415,11 +472,15 @@ static int udf_parse_options(char *optio
| if (match_int(args, &option))
| return 0;
| uopt->session = option;
| + if (!remount)
| + uopt->flags |= (1 << UDF_FLAG_SESSION_SET);
| break;
| case Opt_lastblock:
| if (match_int(args, &option))
| return 0;
| uopt->lastblock = option;
| + if (!remount)
| + uopt->flags |= (1 << UDF_FLAG_LASTBLOCK_SET);
| break;
| case Opt_anchor:
| if (match_int(args, &option))
| @@ -497,7 +558,7 @@ static int udf_remount_fs(struct super_b
| uopt.gid = sbi->s_gid;
| uopt.umask = sbi->s_umask;
|
| - if (!udf_parse_options(options, &uopt))
| + if (!udf_parse_options(options, &uopt, true))
| return -EINVAL;
|
| sbi->s_flags = uopt.flags;
| @@ -1679,7 +1740,7 @@ static int udf_fill_super(struct super_b
|
| mutex_init(&sbi->s_alloc_mutex);
|
| - if (!udf_parse_options((char *)options, &uopt))
| + if (!udf_parse_options((char *)options, &uopt, false))
| goto error_out;
|
| if (uopt.flags & (1 << UDF_FLAG_UTF8) &&
| Index: linux/fs/udf/udf_sb.h
| ===================================================================
| --- linux.orig/fs/udf/udf_sb.h 2008-01-24 13:48:37.000000000 +0100
| +++ linux/fs/udf/udf_sb.h 2008-01-24 13:51:08.000000000 +0100
| @@ -26,6 +26,8 @@
| #define UDF_FLAG_GID_IGNORE 14
| #define UDF_FLAG_UID_SET 15
| #define UDF_FLAG_GID_SET 16
| +#define UDF_FLAG_SESSION_SET 17
| +#define UDF_FLAG_LASTBLOCK_SET 18
|
| #define UDF_PART_FLAG_UNALLOC_BITMAP 0x0001
| #define UDF_PART_FLAG_UNALLOC_TABLE 0x0002
|
| --
|
- Cyrill -

2008-01-25 09:29:46

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

> | + /* is this correct? */
> | + if (sbi->s_anchor[2] != 0)
> | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
>
> you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
> in sake of style unification but we should wait for Jan's
> decision (i'm not the expert in this area ;)

I think UDF_SB_ANCHOR macro was removed by some patch in -mm.

I'm more interested if the second element of the s_anchor array really
does always have the value of the 'anchor=N' mount option. I haven't
been able to verify that fully. Do you have some insight into that?

Thanks,
Miklos

2008-01-25 10:57:35

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

On Jan 25, 2008 12:29 PM, Miklos Szeredi <[email protected]> wrote:
> > | + /* is this correct? */
> > | + if (sbi->s_anchor[2] != 0)
> > | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
> >
> > you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
> > in sake of style unification but we should wait for Jan's
> > decision (i'm not the expert in this area ;)
>
> I think UDF_SB_ANCHOR macro was removed by some patch in -mm.
>
> I'm more interested if the second element of the s_anchor array really
> does always have the value of the 'anchor=N' mount option. I haven't
> been able to verify that fully. Do you have some insight into that?
>
> Thanks,
> Miklos
>

Miklos,

I'll check this today evening (a bit busy now).

- Cyrill -

2008-01-25 13:42:30

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

[Miklos Szeredi - Fri, Jan 25, 2008 at 10:29:21AM +0100]
| > | + /* is this correct? */
| > | + if (sbi->s_anchor[2] != 0)
| > | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
| >
| > you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
| > in sake of style unification but we should wait for Jan's
| > decision (i'm not the expert in this area ;)
|
| I think UDF_SB_ANCHOR macro was removed by some patch in -mm.
|
| I'm more interested if the second element of the s_anchor array really
| does always have the value of the 'anchor=N' mount option. I haven't
| been able to verify that fully. Do you have some insight into that?
|
| Thanks,
| Miklos
|

Hello Miklos,

well, actually - no. anchor entities can be set to 0 if we have been failed
to read them in udf_find_anchor(). So it seems you've to use some
additional flag to store it.

Btw, Miklos the patch is over -mm tree?

- Cyrill -

2008-01-25 15:27:21

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

> > | + /* is this correct? */
> > | + if (sbi->s_anchor[2] != 0)
> > | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
> >
> > you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
> > in sake of style unification but we should wait for Jan's
> > decision (i'm not the expert in this area ;)
>
> I think UDF_SB_ANCHOR macro was removed by some patch in -mm.
Yes, it's going to be removed so don't use it. Actually, basing this
patch on top of -mm is a good idea because there are quite some changes
in Andrew's queue.

> I'm more interested if the second element of the s_anchor array really
> does always have the value of the 'anchor=N' mount option. I haven't
> been able to verify that fully. Do you have some insight into that?
As Cyrill wrote, it could be zeroed out in case there is no anchor in
the specified block. So I guess you have to store the passed value
somewhere else..

Honza
--
Jan Kara <[email protected]>
SuSE CR Labs

2008-01-25 15:31:15

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

> From: Miklos Szeredi <[email protected]>
>
> Add a .show_options super operation to udf.
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> ---
>
> Index: linux/fs/udf/super.c
> ===================================================================
> --- linux.orig/fs/udf/super.c 2008-01-24 13:48:37.000000000 +0100
> +++ linux/fs/udf/super.c 2008-01-24 15:58:21.000000000 +0100
> @@ -53,6 +53,8 @@
> #include <linux/vfs.h>
> #include <linux/vmalloc.h>
> #include <linux/errno.h>
> +#include <linux/mount.h>
> +#include <linux/seq_file.h>
> #include <asm/byteorder.h>
>
> #include <linux/udf_fs.h>
> @@ -71,6 +73,8 @@
> #define VDS_POS_TERMINATING_DESC 6
> #define VDS_POS_LENGTH 7
>
> +#define UDF_DEFAULT_BLOCKSIZE 2048
> +
> static char error_buf[1024];
>
> /* These are the "meat" - everything else is stuffing */
> @@ -95,6 +99,7 @@ static void udf_open_lvid(struct super_b
> static void udf_close_lvid(struct super_block *);
> static unsigned int udf_count_free(struct super_block *);
> static int udf_statfs(struct dentry *, struct kstatfs *);
> +static int udf_show_options(struct seq_file *, struct vfsmount *);
>
> struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct udf_sb_info *sbi)
> {
> @@ -181,6 +186,7 @@ static const struct super_operations udf
> .write_super = udf_write_super,
> .statfs = udf_statfs,
> .remount_fs = udf_remount_fs,
> + .show_options = udf_show_options,
> };
>
> struct udf_options {
> @@ -247,6 +253,56 @@ static int udf_sb_alloc_partition_maps(s
> return 0;
> }
>
> +static int udf_show_options(struct seq_file *seq, struct vfsmount *mnt)
> +{
> + struct super_block *sb = mnt->mnt_sb;
> + struct udf_sb_info *sbi = UDF_SB(sb);
> +
> + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_STRICT))
> + seq_puts(seq, ",nostrict");
> + if (sb->s_blocksize != UDF_DEFAULT_BLOCKSIZE)
> + seq_printf(seq, ",bs=%lu", sb->s_blocksize);
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNHIDE))
> + seq_puts(seq, ",unhide");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UNDELETE))
> + seq_puts(seq, ",undelete");
> + if (!UDF_QUERY_FLAG(sb, UDF_FLAG_USE_AD_IN_ICB))
> + seq_puts(seq, ",noadinicb");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_USE_SHORT_AD))
> + seq_puts(seq, ",shortad");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_FORGET))
> + seq_puts(seq, ",uid=forget");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_IGNORE))
> + seq_puts(seq, ",uid=ignore");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_FORGET))
> + seq_puts(seq, ",gid=forget");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_IGNORE))
> + seq_puts(seq, ",gid=ignore");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UID_SET))
> + seq_printf(seq, ",uid=%u", sbi->s_uid);
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_GID_SET))
> + seq_printf(seq, ",gid=%u", sbi->s_gid);
> + if (sbi->s_umask != 0)
> + seq_printf(seq, ",umask=%o", sbi->s_umask);
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_SESSION_SET))
> + seq_printf(seq, ",session=%u", sbi->s_session);
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_LASTBLOCK_SET))
> + seq_printf(seq, ",lastblock=%u", sbi->s_last_block);
> + /* is this correct? */
> + if (sbi->s_anchor[2] != 0)
> + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
> + /*
> + * volume, partition, fileset and rootdir seem to be ignored
> + * currently
> + */
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8))
> + seq_puts(seq, ",utf8");
> + if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP) && sbi->s_nls_map)
> + seq_printf(seq, ",iocharset=%s", sbi->s_nls_map->charset);
> +
> + return 0;
> +}
> +
> /*
> * udf_parse_options
> *
> @@ -339,13 +395,14 @@ static match_table_t tokens = {
> {Opt_err, NULL}
> };
>
> -static int udf_parse_options(char *options, struct udf_options *uopt)
> +static int udf_parse_options(char *options, struct udf_options *uopt,
> + bool remount)
> {
Please use just 'int' for 'remount' option. We are slowly trying to
get rid of these strange things in UDF code so adding new ones isn't
desirable.

> char *p;
> int option;
>
> uopt->novrs = 0;
> - uopt->blocksize = 2048;
> + uopt->blocksize = UDF_DEFAULT_BLOCKSIZE;
> uopt->partition = 0xFFFF;
> uopt->session = 0xFFFFFFFF;
> uopt->lastblock = 0;
> @@ -415,11 +472,15 @@ static int udf_parse_options(char *optio
> if (match_int(args, &option))
> return 0;
> uopt->session = option;
> + if (!remount)
> + uopt->flags |= (1 << UDF_FLAG_SESSION_SET);
> break;
> case Opt_lastblock:
> if (match_int(args, &option))
> return 0;
> uopt->lastblock = option;
> + if (!remount)
> + uopt->flags |= (1 << UDF_FLAG_LASTBLOCK_SET);
> break;
> case Opt_anchor:
> if (match_int(args, &option))
> @@ -497,7 +558,7 @@ static int udf_remount_fs(struct super_b
> uopt.gid = sbi->s_gid;
> uopt.umask = sbi->s_umask;
>
> - if (!udf_parse_options(options, &uopt))
> + if (!udf_parse_options(options, &uopt, true))
> return -EINVAL;
>
> sbi->s_flags = uopt.flags;
> @@ -1679,7 +1740,7 @@ static int udf_fill_super(struct super_b
>
> mutex_init(&sbi->s_alloc_mutex);
>
> - if (!udf_parse_options((char *)options, &uopt))
> + if (!udf_parse_options((char *)options, &uopt, false))
> goto error_out;
>
> if (uopt.flags & (1 << UDF_FLAG_UTF8) &&
> Index: linux/fs/udf/udf_sb.h
> ===================================================================
> --- linux.orig/fs/udf/udf_sb.h 2008-01-24 13:48:37.000000000 +0100
> +++ linux/fs/udf/udf_sb.h 2008-01-24 13:51:08.000000000 +0100
> @@ -26,6 +26,8 @@
> #define UDF_FLAG_GID_IGNORE 14
> #define UDF_FLAG_UID_SET 15
> #define UDF_FLAG_GID_SET 16
> +#define UDF_FLAG_SESSION_SET 17
> +#define UDF_FLAG_LASTBLOCK_SET 18
>
> #define UDF_PART_FLAG_UNALLOC_BITMAP 0x0001
> #define UDF_PART_FLAG_UNALLOC_TABLE 0x0002
Otherwise (apart from comments I wrote in the other email) the patch
is fine.

Honza
--
Jan Kara <[email protected]>
SuSE CR Labs

2008-01-25 15:50:36

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

> > > | + /* is this correct? */
> > > | + if (sbi->s_anchor[2] != 0)
> > > | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
> > >
> > > you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
> > > in sake of style unification but we should wait for Jan's
> > > decision (i'm not the expert in this area ;)
> >
> > I think UDF_SB_ANCHOR macro was removed by some patch in -mm.
> Yes, it's going to be removed so don't use it. Actually, basing this
> patch on top of -mm is a good idea because there are quite some changes
> in Andrew's queue.
>
> > I'm more interested if the second element of the s_anchor array really
> > does always have the value of the 'anchor=N' mount option. I haven't
> > been able to verify that fully. Do you have some insight into that?
> As Cyrill wrote, it could be zeroed out in case there is no anchor in
> the specified block. So I guess you have to store the passed value
> somewhere else..

But in that case, would the value of the anchor= option matter?

This is actually a somewhat philosophical question about what the
mount options in /proc/mounts mean:

1) Options _given_ by the user for the mount
2) Options which are _effective_ for the mount

If we take interpretation 2) and there was no anchor (whatever that
means), then the anchor=N option wasn't effective, and not giving it
would have had the same effect.

This could be confusing to the user, though...

Thanks,
Miklos

2008-01-25 15:56:27

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

> Please use just 'int' for 'remount' option. We are slowly trying to
> get rid of these strange things in UDF code so adding new ones isn't
> desirable.

What's wrong with bool?

I'm not advocating mass replacements, but all new code _should_ use
it, because it's a very useful and good type.

We are just not very much used to it yet, but don't tell me it's
confusing to see a type like this ;)

Miklos

2008-01-25 15:57:50

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

[Miklos Szeredi - Fri, Jan 25, 2008 at 04:50:15PM +0100]
| > > > | + /* is this correct? */
| > > > | + if (sbi->s_anchor[2] != 0)
| > > > | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
| > > >
| > > > you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
| > > > in sake of style unification but we should wait for Jan's
| > > > decision (i'm not the expert in this area ;)
| > >
| > > I think UDF_SB_ANCHOR macro was removed by some patch in -mm.
| > Yes, it's going to be removed so don't use it. Actually, basing this
| > patch on top of -mm is a good idea because there are quite some changes
| > in Andrew's queue.
| >
| > > I'm more interested if the second element of the s_anchor array really
| > > does always have the value of the 'anchor=N' mount option. I haven't
| > > been able to verify that fully. Do you have some insight into that?
| > As Cyrill wrote, it could be zeroed out in case there is no anchor in
| > the specified block. So I guess you have to store the passed value
| > somewhere else..
|
| But in that case, would the value of the anchor= option matter?
|
| This is actually a somewhat philosophical question about what the
| mount options in /proc/mounts mean:
|
| 1) Options _given_ by the user for the mount
| 2) Options which are _effective_ for the mount
|
| If we take interpretation 2) and there was no anchor (whatever that
| means), then the anchor=N option wasn't effective, and not giving it
| would have had the same effect.
|
| This could be confusing to the user, though...
|
| Thanks,
| Miklos
|

I think _effective_ options is much more important - they could
show you that something bad happened (and if this zeroing of anchor
has been happened udf print debug message) Anyway, Miklos, I think
the options _given_ by a user does not mean anything in that case
because it just doesn't reveal what is being used in _real_.

- Cyrill -

2008-01-25 16:04:26

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

On Fri 25-01-08 16:56:13, Miklos Szeredi wrote:
> > Please use just 'int' for 'remount' option. We are slowly trying to
> > get rid of these strange things in UDF code so adding new ones isn't
> > desirable.
>
> What's wrong with bool?
>
> I'm not advocating mass replacements, but all new code _should_ use
> it, because it's a very useful and good type.
>
> We are just not very much used to it yet, but don't tell me it's
> confusing to see a type like this ;)
It's not so confusing but one really isn't used to it ;) But OK, I don't
mind that much...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2008-01-25 16:07:37

[permalink] [raw]

Subject: Re: [patch 25/26] mount options: fix udf

On Fri 25-01-08 16:50:15, Miklos Szeredi wrote:
> > > > | + /* is this correct? */
> > > > | + if (sbi->s_anchor[2] != 0)
> > > > | + seq_printf(seq, ",anchor=%u", sbi->s_anchor[2]);
> > > >
> > > > you know, I would prefer to use form UDF_SB_ANCHOR(sb)[2]
> > > > in sake of style unification but we should wait for Jan's
> > > > decision (i'm not the expert in this area ;)
> > >
> > > I think UDF_SB_ANCHOR macro was removed by some patch in -mm.
> > Yes, it's going to be removed so don't use it. Actually, basing this
> > patch on top of -mm is a good idea because there are quite some changes
> > in Andrew's queue.
> >
> > > I'm more interested if the second element of the s_anchor array really
> > > does always have the value of the 'anchor=N' mount option. I haven't
> > > been able to verify that fully. Do you have some insight into that?
> > As Cyrill wrote, it could be zeroed out in case there is no anchor in
> > the specified block. So I guess you have to store the passed value
> > somewhere else..
>
> But in that case, would the value of the anchor= option matter?
No, it would not.

> This is actually a somewhat philosophical question about what the
> mount options in /proc/mounts mean:
>
> 1) Options _given_ by the user for the mount
> 2) Options which are _effective_ for the mount
>
> If we take interpretation 2) and there was no anchor (whatever that
> means), then the anchor=N option wasn't effective, and not giving it
> would have had the same effect.
>
> This could be confusing to the user, though...
Hmm, given that options are modified by remount for some filesystems,
it's probably the best to display the effective state. So your code should
display the right thing as it is.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2008-01-25 16:11:21