Re: [PATCH 1/5] FAT: add 'errors' mount option

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Denis Karpov <ext-denis.2.karpov@nokia.com> writes:

> +#define FAT_ERRORS_CONT		0x1
> +#define FAT_ERRORS_PANIC	0x2
> +#define FAT_ERRORS_RO		0x4

I think this should be scalar? (i.e. 1, 2, 3)

>  struct fat_mount_options {
>  	uid_t fs_uid;
>  	gid_t fs_gid;
> @@ -39,7 +43,8 @@ struct fat_mount_options {
>  		 nocase:1,	  /* Does this need case conversion? 0=need case conversion*/
>  		 usefree:1,	  /* Use free_clusters for FAT32 */
>  		 tz_utc:1,	  /* Filesystem timestamps are in UTC */
> -		 rodir:1;	  /* allow ATTR_RO for directory */
> +		 rodir:1,	  /* allow ATTR_RO for directory */
> +		 errors:3;	  /* On error: continue, panic, go ro */
>  };

Please use "unsigned char errors", instead of bit filed. Below of
name_check would have 1 byte hole.

> @@ -834,6 +834,12 @@ static int fat_show_options(struct seq_file *m, struct vfsmount *mnt)
>  		seq_puts(m, ",flush");
>  	if (opts->tz_utc)
>  		seq_puts(m, ",tz=UTC");
> +	if (opts->errors & FAT_ERRORS_CONT)
> +		seq_puts(m, ",errors=continue");
> +	if (opts->errors & FAT_ERRORS_PANIC)
> +		seq_puts(m, ",errors=panic");
> +	if (opts->errors & FAT_ERRORS_RO)
> +		seq_puts(m, ",errors=ro");

Also this should be scalar?

	if (opts->errors == FAT_ERRORS_CONT)
		seq_puts(m, ",errors=continue");
	else if (opts->errors == FAT_ERRORS_PANIC)
		seq_puts(m, ",errors=panic");
	else
		seq_puts(m, ",errors=ro");

>  static const match_table_t fat_tokens = {
> @@ -882,6 +889,9 @@ static const match_table_t fat_tokens = {
>  	{Opt_obsolate, "posix"},
>  	{Opt_flush, "flush"},
>  	{Opt_tz_utc, "tz=UTC"},
> +	{Opt_err_cont, "errors=continue"},
> +	{Opt_err_panic, "errors=panic"},
> +	{Opt_err_ro, "errors=remount-ro"},
>  	{Opt_err, NULL},
>  };
>  static const match_table_t msdos_tokens = {
> @@ -889,6 +899,9 @@ static const match_table_t msdos_tokens = {
>  	{Opt_nodots, "dotsOK=no"},
>  	{Opt_dots, "dots"},
>  	{Opt_dots, "dotsOK=yes"},
> +	{Opt_err_cont, "errors=continue"},
> +	{Opt_err_panic, "errors=panic"},
> +	{Opt_err_ro, "errors=remount-ro"},
>  	{Opt_err, NULL}
>  };
>  static const match_table_t vfat_tokens = {
> @@ -919,6 +932,9 @@ static const match_table_t vfat_tokens = {
>  	{Opt_nonumtail_yes, "nonumtail=true"},
>  	{Opt_nonumtail_yes, "nonumtail"},
>  	{Opt_rodir, "rodir"},
> +	{Opt_err_cont, "errors=continue"},
> +	{Opt_err_panic, "errors=panic"},
> +	{Opt_err_ro, "errors=remount-ro"},
>  	{Opt_err, NULL}
>  };

"fat_tokens" is used for the both of vfat and msdos. So, you don't need
to add those to "msdos/vfat_tokens".

> @@ -1043,6 +1059,15 @@ static int parse_options(char *options, int is_vfat, int silent, int *debug,
>  		case Opt_tz_utc:
>  			opts->tz_utc = 1;
>  			break;
> +		case Opt_err_cont:
> +			opts->errors = FAT_ERRORS_CONT;
> +			break;
> +		case Opt_err_panic:
> +			opts->errors = FAT_ERRORS_PANIC;
> +			break;
> +		case Opt_err_ro:
> +			opts->errors = FAT_ERRORS_RO;
> +			break;
>  
>  		/* msdos specific */
>  		case Opt_dots:
> @@ -1128,6 +1153,9 @@ out:
>  		opts->allow_utime = ~opts->fs_dmask & (S_IWGRP | S_IWOTH);
>  	if (opts->unicode_xlate)
>  		opts->utf8 = 0;
> +	/* Default behavior on fs errors - go r/o */
> +	if (!opts->errors)
> +		opts->errors = FAT_ERRORS_RO;

Is there any reason opts->errors isn't initialized with FAT_ERRORS_RO
like other options?

> +	if (sbi->options.errors & FAT_ERRORS_PANIC)
> +		panic("    FAT fs panic from previous error\n");
> +	if ((sbi->options.errors & FAT_ERRORS_RO) &&
> +				!(s->s_flags & MS_RDONLY)) {
>  		s->s_flags |= MS_RDONLY;
>  		printk(KERN_ERR "    File system has been set read-only\n");
>  	}
>  }

Also, scalar?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>