Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Stephen Rothwell 
On Mon, 31 Mar 2014 15:32:32 -0700 Andrew Morton  
wrote:
>
> On Mon, 31 Mar 2014 15:21:17 -0700 Conrad Meyer  wrote:
> 
> > >> + if (get_unaligned_le16(>sector_size) != 0 || b->sec_per_clus != 
> > >> 0 ||
> > >> + b->reserved != 0 || b->fats != 0 ||
> > >> + get_unaligned_le16(>dir_entries) != 0 ||
> > >> + get_unaligned_le16(>sectors) != 0 || b->media != 0 ||
> > >> + b->fat_length != 0 || b->secs_track != 0 || b->heads != 0 
> > >> ||
> > >> + b->secs_track != 0 || b->heads != 0)
> > >
> > > Impressive!
> > 
> > I aim to please.
> 
> No great improvements immediately occur to me ;)
> 
> One could do
> 
>   /* nice comment */
>   if (get_unaligned_le16(>sector_size) != 0)
>   return;
>   /* another nice comment */
>   if (b->sec_per_clus != 0)
>   return;
>   ...
> 
> but one would quickly run out of nice comments.
> 
> You could do s/ != 0//g.

You could also put the boolean expression in a (hopefully expressively
named) helper function and do the tests separately there.

-- 
Cheers,
Stephen Rothwells...@canb.auug.org.au


pgp4kYwXbBv0E.pgp
Description: PGP signature

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Andrew Morton 
On Mon, 31 Mar 2014 15:21:17 -0700 Conrad Meyer  wrote:

> >> + if (get_unaligned_le16(>sector_size) != 0 || b->sec_per_clus != 0 
> >> ||
> >> + b->reserved != 0 || b->fats != 0 ||
> >> + get_unaligned_le16(>dir_entries) != 0 ||
> >> + get_unaligned_le16(>sectors) != 0 || b->media != 0 ||
> >> + b->fat_length != 0 || b->secs_track != 0 || b->heads != 0 ||
> >> + b->secs_track != 0 || b->heads != 0)
> >
> > Impressive!
> 
> I aim to please.

No great improvements immediately occur to me ;)

One could do

/* nice comment */
if (get_unaligned_le16(>sector_size) != 0)
return;
/* another nice comment */
if (b->sec_per_clus != 0)
return;
...

but one would quickly run out of nice comments.

You could do s/ != 0//g.

> Not sure what would be better -- memcmp() part of the
> struct to a zeroed array?

memcmp would be hacky.  And possibly buggy if there are holes in the
struct, which is arch-dependent, shudder.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Conrad Meyer 
On Mon, Mar 31, 2014 at 3:13 PM, Andrew Morton
 wrote:
> On Sat, 29 Mar 2014 12:10:35 -0700 Conrad Meyer  wrote:
>
>> + .nr_sectors = 360 * KB_IN_SECTORS,
>> + .sec_per_clus = 2,
>> + .dir_entries = 112,
>> + .media = 0xFD,
>> + .fat_length = 2,
>> +},
>> +{ 0 } };
>
> We don't really need this EOF element.

Ah, right, I forgot about ARRAY_SIZE. This is an old version of this
patch, see v3 here: https://lkml.org/lkml/2014/3/31/275

But the same criticism is valid there, too.

>> + if (get_unaligned_le16(>sector_size) != 0 || b->sec_per_clus != 0 ||
>> + b->reserved != 0 || b->fats != 0 ||
>> + get_unaligned_le16(>dir_entries) != 0 ||
>> + get_unaligned_le16(>sectors) != 0 || b->media != 0 ||
>> + b->fat_length != 0 || b->secs_track != 0 || b->heads != 0 ||
>> + b->secs_track != 0 || b->heads != 0)
>
> Impressive!

I aim to please. Not sure what would be better -- memcmp() part of the
struct to a zeroed array?

>
>> + return;
>> +
>> + bd_sects = part_nr_sects_read(sb->s_bdev->bd_part);
>> + for (di = floppy_defaults; di->nr_sectors; di++) {
>
> Can do something like
>
> for (di = floppy_defaults;
> di < floppy_defaults + ARRAY_SIZE(floppy_defaults); di++) {

Yep, I should revise and send a v4 patch. Thanks.

>
>> + if (di->nr_sectors == bd_sects)
>> + break;
>> + }
>> + if (di->nr_sectors == 0) {
>> + fat_msg(sb, KERN_WARNING,
>> + "DOS volume lacks BPB and isn't a recognized floppy 
>> size (%ld sectors)",
>> + (long)bd_sects);
>
> sector_t can be u64 on 32-bit so one should really use %Lu and cast to
> u64.

Thanks, will fix.

Conrad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Andrew Morton 
On Sat, 29 Mar 2014 12:10:35 -0700 Conrad Meyer  wrote:

> When possible, infer DOS 2.x BIOS Parameter Block from block device
> geometry (for floppies and floppy images). Update in-memory only. We
> only perform this update when the entire BPB region is zeroed, like
> produced by DOS 1.x-era FORMAT (and other OEM variations on DOS).
> 
> Fixes kernel.org bug #42617.
> 
> BPB default values are inferred from media size and a table.[0] Media
> size is assumed to be static for archaic FAT volumes. See also [1].
> 
> [0]: https://en.wikipedia.org/wiki/File_Allocation_Table#Exceptions
> [1]: http://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html
> 
> ...
>
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -35,9 +35,47 @@
>  #define CONFIG_FAT_DEFAULT_IOCHARSET ""
>  #endif
>  
> +#define KB_IN_SECTORS 2
> +
>  static int fat_default_codepage = CONFIG_FAT_DEFAULT_CODEPAGE;
>  static char fat_default_iocharset[] = CONFIG_FAT_DEFAULT_IOCHARSET;
>  
> +static struct fat_floppy_defaults {
> + unsigned nr_sectors;
> + unsigned sec_per_clus;
> + unsigned dir_entries;
> + unsigned media;
> + unsigned fat_length;
> +} floppy_defaults[] = {
> +{
> + .nr_sectors = 160 * KB_IN_SECTORS,
> + .sec_per_clus = 1,
> + .dir_entries = 64,
> + .media = 0xFE,
> + .fat_length = 1,
> +},
> +{
> + .nr_sectors = 180 * KB_IN_SECTORS,
> + .sec_per_clus = 1,
> + .dir_entries = 64,
> + .media = 0xFC,
> + .fat_length = 2,
> +},
> +{
> + .nr_sectors = 320 * KB_IN_SECTORS,
> + .sec_per_clus = 2,
> + .dir_entries = 112,
> + .media = 0xFF,
> + .fat_length = 1,
> +},
> +{
> + .nr_sectors = 360 * KB_IN_SECTORS,
> + .sec_per_clus = 2,
> + .dir_entries = 112,
> + .media = 0xFD,
> + .fat_length = 2,
> +},
> +{ 0 } };

We don't really need this EOF element.

>  static int fat_add_cluster(struct inode *inode)
>  {
> @@ -1246,6 +1284,58 @@ static unsigned long calc_fat_clusters(struct 
> super_block *sb)
>  }
>  
>  /*
> + * If this FAT filesystem is archaic (lacking a BIOS Parameter Block, ca. DOS
> + * 1.x), fix it up in-place by creating a DOS 2.x BIOS Parameter Block from
> + * defaults for the media size.
> + */
> +static void fat_update_archaic_boot_sector(struct super_block *sb,
> + struct fat_boot_sector *b)
> +{
> + struct fat_floppy_defaults *di;
> + sector_t bd_sects;
> +
> + /* 16-bit DOS 1.x reliably wrote bootstrap short-jmp code */
> + if (b->ignored[0] != 0xeb || b->ignored[2] != 0x90)
> + return;
> +
> + /*
> +  * If any value in this region is non-zero, don't assume it is archaic
> +  * DOS.
> +  */
> + if (get_unaligned_le16(>sector_size) != 0 || b->sec_per_clus != 0 ||
> + b->reserved != 0 || b->fats != 0 ||
> + get_unaligned_le16(>dir_entries) != 0 ||
> + get_unaligned_le16(>sectors) != 0 || b->media != 0 ||
> + b->fat_length != 0 || b->secs_track != 0 || b->heads != 0 ||
> + b->secs_track != 0 || b->heads != 0)

Impressive!

> + return;
> +
> + bd_sects = part_nr_sects_read(sb->s_bdev->bd_part);
> + for (di = floppy_defaults; di->nr_sectors; di++) {

Can do something like

for (di = floppy_defaults;
di < floppy_defaults + ARRAY_SIZE(floppy_defaults); di++) {

> + if (di->nr_sectors == bd_sects)
> + break;
> + }
> + if (di->nr_sectors == 0) {
> + fat_msg(sb, KERN_WARNING,
> + "DOS volume lacks BPB and isn't a recognized floppy 
> size (%ld sectors)",
> + (long)bd_sects);

sector_t can be u64 on 32-bit so one should really use %Lu and cast to
u64.

>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Conrad Meyer 
On Mon, 31 Mar 2014 23:07:32 +0900
OGAWA Hirofumi  wrote:

> Conrad Meyer  writes:
> 
> > +static void fat_update_archaic_boot_sector(struct
> > super_block *sb,
> > +   struct fat_boot_sector *b)
> > +{
> > +   struct fat_floppy_defaults *di;
> > +   sector_t bd_sects;
> > +
> > +   /* 16-bit DOS 1.x reliably wrote bootstrap
> > short-jmp code */
> > +   if (b->ignored[0] != 0xeb || b->ignored[2] !=
> > 0x90)
> > +   return;
> > +
> > +   /*
> > +* If any value in this region is non-zero,
> > don't assume it is archaic
> > +* DOS.
> > +*/
> > +   if (get_unaligned_le16(>sector_size) != 0 ||
> > b->sec_per_clus != 0 ||
> > +   b->reserved != 0 || b->fats != 0 ||
> > +   get_unaligned_le16(>dir_entries) != 0
> > ||
> > +   get_unaligned_le16(>sectors) != 0 ||
> > b->media != 0 ||
> > +   b->fat_length != 0 || b->secs_track != 0
> > || b->heads != 0 ||
> > +   b->secs_track != 0 || b->heads != 0)
> > +   return;
> 
> Probably, too weak detection to use by default. So, how
> about to use mount option to enable this?
> 
> And only if user asked to enable explicitly by mount
> option, allow this format.

Okay. Do you have a preference for option name? "archaicdos",
"dos1x", "guessbpb", other?

> > +   bd_sects =
> > part_nr_sects_read(sb->s_bdev->bd_part);
> > +   for (di = floppy_defaults; di->nr_sectors; di++)
> > {
> > +   if (di->nr_sectors == bd_sects)
> > +   break;
> > +   }
> > +   if (di->nr_sectors == 0) {
> > +   fat_msg(sb, KERN_WARNING,
> > +   "DOS volume lacks BPB and isn't
> > a recognized floppy size (%ld sectors)",
> > +   (long)bd_sects);
> > +   return;
> > +   }
> > +
> > +   fat_msg(sb, KERN_INFO,
> > +   "Volume lacks BPB but looks like archaic
> > DOS; assuming default BPB values"); +
> > +   b->sec_per_clus = di->sec_per_clus;
> > +   put_unaligned_le16(di->dir_entries,
> > >dir_entries);
> > +   b->media = di->media;
> > +   b->fat_length = cpu_to_le16(di->fat_length);
> > +   put_unaligned_le16(SECTOR_SIZE, >sector_size);
> > +   b->reserved = cpu_to_le16(1);
> > +   b->fats = 2;
> > +   put_unaligned_le16(bd_sects, >sectors);
> > +}
> > +
> > +/*
> >   * Read the super block of an MS-DOS FS.
> >   */
> >  int fat_fill_super(struct super_block *sb, void *data,
> > int silent, int isvfat, @@ -1297,6 +1387,8 @@ int
> > fat_fill_super(struct super_block *sb, void *data, int
> > silent, int isvfat, } 
> > b = (struct fat_boot_sector *) bh->b_data;
> > +   fat_update_archaic_boot_sector(sb, b);
> 
> This would be better to set sbi->* directly, not via
> modified BPB.

Hm, I thought so too, but there are lots of sbi-> fields and
I didn't want to duplicate any shared filling logic in a
different place. But I will make the change...

> 
> > if (!b->reserved) {
> > if (!silent)
> > fat_msg(sb, KERN_ERR, "bogus
> > number of reserved sectors"); @@ -1364,6 +1456,7 @@ int
> > fat_fill_super(struct super_block *sb, void *data, int
> > silent, int isvfat, goto out_fail; }
> > b = (struct fat_boot_sector *)
> > bh->b_data;
> > +   fat_update_archaic_boot_sector(sb, b);
> 
> This doesn't need. If logical_sector_size is 512, this
> format doesn't work.

Right. Okay, give me some time to revise a v3 patch.

Thanks,
Conrad
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread OGAWA Hirofumi 
Conrad Meyer  writes:

> +static void fat_update_archaic_boot_sector(struct super_block *sb,
> + struct fat_boot_sector *b)
> +{
> + struct fat_floppy_defaults *di;
> + sector_t bd_sects;
> +
> + /* 16-bit DOS 1.x reliably wrote bootstrap short-jmp code */
> + if (b->ignored[0] != 0xeb || b->ignored[2] != 0x90)
> + return;
> +
> + /*
> +  * If any value in this region is non-zero, don't assume it is archaic
> +  * DOS.
> +  */
> + if (get_unaligned_le16(>sector_size) != 0 || b->sec_per_clus != 0 ||
> + b->reserved != 0 || b->fats != 0 ||
> + get_unaligned_le16(>dir_entries) != 0 ||
> + get_unaligned_le16(>sectors) != 0 || b->media != 0 ||
> + b->fat_length != 0 || b->secs_track != 0 || b->heads != 0 ||
> + b->secs_track != 0 || b->heads != 0)
> + return;

Probably, too weak detection to use by default. So, how about to use
mount option to enable this?

And only if user asked to enable explicitly by mount option, allow this
format.

> + bd_sects = part_nr_sects_read(sb->s_bdev->bd_part);
> + for (di = floppy_defaults; di->nr_sectors; di++) {
> + if (di->nr_sectors == bd_sects)
> + break;
> + }
> + if (di->nr_sectors == 0) {
> + fat_msg(sb, KERN_WARNING,
> + "DOS volume lacks BPB and isn't a recognized floppy 
> size (%ld sectors)",
> + (long)bd_sects);
> + return;
> + }
> +
> + fat_msg(sb, KERN_INFO,
> + "Volume lacks BPB but looks like archaic DOS; assuming default 
> BPB values");
> +
> + b->sec_per_clus = di->sec_per_clus;
> + put_unaligned_le16(di->dir_entries, >dir_entries);
> + b->media = di->media;
> + b->fat_length = cpu_to_le16(di->fat_length);
> + put_unaligned_le16(SECTOR_SIZE, >sector_size);
> + b->reserved = cpu_to_le16(1);
> + b->fats = 2;
> + put_unaligned_le16(bd_sects, >sectors);
> +}
> +
> +/*
>   * Read the super block of an MS-DOS FS.
>   */
>  int fat_fill_super(struct super_block *sb, void *data, int silent, int 
> isvfat,
> @@ -1297,6 +1387,8 @@ int fat_fill_super(struct super_block *sb, void *data, 
> int silent, int isvfat,
>   }
>  
>   b = (struct fat_boot_sector *) bh->b_data;
> + fat_update_archaic_boot_sector(sb, b);

This would be better to set sbi->* directly, not via modified BPB.

>   if (!b->reserved) {
>   if (!silent)
>   fat_msg(sb, KERN_ERR, "bogus number of reserved 
> sectors");
> @@ -1364,6 +1456,7 @@ int fat_fill_super(struct super_block *sb, void *data, 
> int silent, int isvfat,
>   goto out_fail;
>   }
>   b = (struct fat_boot_sector *) bh->b_data;
> + fat_update_archaic_boot_sector(sb, b);

This doesn't need. If logical_sector_size is 512, this format doesn't work.
-- 
OGAWA Hirofumi 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread OGAWA Hirofumi 
Conrad Meyer ceme...@uw.edu writes:

 +static void fat_update_archaic_boot_sector(struct super_block *sb,
 + struct fat_boot_sector *b)
 +{
 + struct fat_floppy_defaults *di;
 + sector_t bd_sects;
 +
 + /* 16-bit DOS 1.x reliably wrote bootstrap short-jmp code */
 + if (b-ignored[0] != 0xeb || b-ignored[2] != 0x90)
 + return;
 +
 + /*
 +  * If any value in this region is non-zero, don't assume it is archaic
 +  * DOS.
 +  */
 + if (get_unaligned_le16(b-sector_size) != 0 || b-sec_per_clus != 0 ||
 + b-reserved != 0 || b-fats != 0 ||
 + get_unaligned_le16(b-dir_entries) != 0 ||
 + get_unaligned_le16(b-sectors) != 0 || b-media != 0 ||
 + b-fat_length != 0 || b-secs_track != 0 || b-heads != 0 ||
 + b-secs_track != 0 || b-heads != 0)
 + return;

Probably, too weak detection to use by default. So, how about to use
mount option to enable this?

And only if user asked to enable explicitly by mount option, allow this
format.

 + bd_sects = part_nr_sects_read(sb-s_bdev-bd_part);
 + for (di = floppy_defaults; di-nr_sectors; di++) {
 + if (di-nr_sectors == bd_sects)
 + break;
 + }
 + if (di-nr_sectors == 0) {
 + fat_msg(sb, KERN_WARNING,
 + DOS volume lacks BPB and isn't a recognized floppy 
 size (%ld sectors),
 + (long)bd_sects);
 + return;
 + }
 +
 + fat_msg(sb, KERN_INFO,
 + Volume lacks BPB but looks like archaic DOS; assuming default 
 BPB values);
 +
 + b-sec_per_clus = di-sec_per_clus;
 + put_unaligned_le16(di-dir_entries, b-dir_entries);
 + b-media = di-media;
 + b-fat_length = cpu_to_le16(di-fat_length);
 + put_unaligned_le16(SECTOR_SIZE, b-sector_size);
 + b-reserved = cpu_to_le16(1);
 + b-fats = 2;
 + put_unaligned_le16(bd_sects, b-sectors);
 +}
 +
 +/*
   * Read the super block of an MS-DOS FS.
   */
  int fat_fill_super(struct super_block *sb, void *data, int silent, int 
 isvfat,
 @@ -1297,6 +1387,8 @@ int fat_fill_super(struct super_block *sb, void *data, 
 int silent, int isvfat,
   }
  
   b = (struct fat_boot_sector *) bh-b_data;
 + fat_update_archaic_boot_sector(sb, b);

This would be better to set sbi-* directly, not via modified BPB.

   if (!b-reserved) {
   if (!silent)
   fat_msg(sb, KERN_ERR, bogus number of reserved 
 sectors);
 @@ -1364,6 +1456,7 @@ int fat_fill_super(struct super_block *sb, void *data, 
 int silent, int isvfat,
   goto out_fail;
   }
   b = (struct fat_boot_sector *) bh-b_data;
 + fat_update_archaic_boot_sector(sb, b);

This doesn't need. If logical_sector_size is 512, this format doesn't work.
-- 
OGAWA Hirofumi hirof...@mail.parknet.co.jp
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Conrad Meyer 
On Mon, 31 Mar 2014 23:07:32 +0900
OGAWA Hirofumi hirof...@mail.parknet.co.jp wrote:

 Conrad Meyer ceme...@uw.edu writes:
 
  +static void fat_update_archaic_boot_sector(struct
  super_block *sb,
  +   struct fat_boot_sector *b)
  +{
  +   struct fat_floppy_defaults *di;
  +   sector_t bd_sects;
  +
  +   /* 16-bit DOS 1.x reliably wrote bootstrap
  short-jmp code */
  +   if (b-ignored[0] != 0xeb || b-ignored[2] !=
  0x90)
  +   return;
  +
  +   /*
  +* If any value in this region is non-zero,
  don't assume it is archaic
  +* DOS.
  +*/
  +   if (get_unaligned_le16(b-sector_size) != 0 ||
  b-sec_per_clus != 0 ||
  +   b-reserved != 0 || b-fats != 0 ||
  +   get_unaligned_le16(b-dir_entries) != 0
  ||
  +   get_unaligned_le16(b-sectors) != 0 ||
  b-media != 0 ||
  +   b-fat_length != 0 || b-secs_track != 0
  || b-heads != 0 ||
  +   b-secs_track != 0 || b-heads != 0)
  +   return;
 
 Probably, too weak detection to use by default. So, how
 about to use mount option to enable this?
 
 And only if user asked to enable explicitly by mount
 option, allow this format.

Okay. Do you have a preference for option name? archaicdos,
dos1x, guessbpb, other?

  +   bd_sects =
  part_nr_sects_read(sb-s_bdev-bd_part);
  +   for (di = floppy_defaults; di-nr_sectors; di++)
  {
  +   if (di-nr_sectors == bd_sects)
  +   break;
  +   }
  +   if (di-nr_sectors == 0) {
  +   fat_msg(sb, KERN_WARNING,
  +   DOS volume lacks BPB and isn't
  a recognized floppy size (%ld sectors),
  +   (long)bd_sects);
  +   return;
  +   }
  +
  +   fat_msg(sb, KERN_INFO,
  +   Volume lacks BPB but looks like archaic
  DOS; assuming default BPB values); +
  +   b-sec_per_clus = di-sec_per_clus;
  +   put_unaligned_le16(di-dir_entries,
  b-dir_entries);
  +   b-media = di-media;
  +   b-fat_length = cpu_to_le16(di-fat_length);
  +   put_unaligned_le16(SECTOR_SIZE, b-sector_size);
  +   b-reserved = cpu_to_le16(1);
  +   b-fats = 2;
  +   put_unaligned_le16(bd_sects, b-sectors);
  +}
  +
  +/*
* Read the super block of an MS-DOS FS.
*/
   int fat_fill_super(struct super_block *sb, void *data,
  int silent, int isvfat, @@ -1297,6 +1387,8 @@ int
  fat_fill_super(struct super_block *sb, void *data, int
  silent, int isvfat, } 
  b = (struct fat_boot_sector *) bh-b_data;
  +   fat_update_archaic_boot_sector(sb, b);
 
 This would be better to set sbi-* directly, not via
 modified BPB.

Hm, I thought so too, but there are lots of sbi- fields and
I didn't want to duplicate any shared filling logic in a
different place. But I will make the change...

 
  if (!b-reserved) {
  if (!silent)
  fat_msg(sb, KERN_ERR, bogus
  number of reserved sectors); @@ -1364,6 +1456,7 @@ int
  fat_fill_super(struct super_block *sb, void *data, int
  silent, int isvfat, goto out_fail; }
  b = (struct fat_boot_sector *)
  bh-b_data;
  +   fat_update_archaic_boot_sector(sb, b);
 
 This doesn't need. If logical_sector_size is 512, this
 format doesn't work.

Right. Okay, give me some time to revise a v3 patch.

Thanks,
Conrad
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Andrew Morton 
On Sat, 29 Mar 2014 12:10:35 -0700 Conrad Meyer ceme...@uw.edu wrote:

 When possible, infer DOS 2.x BIOS Parameter Block from block device
 geometry (for floppies and floppy images). Update in-memory only. We
 only perform this update when the entire BPB region is zeroed, like
 produced by DOS 1.x-era FORMAT (and other OEM variations on DOS).
 
 Fixes kernel.org bug #42617.
 
 BPB default values are inferred from media size and a table.[0] Media
 size is assumed to be static for archaic FAT volumes. See also [1].
 
 [0]: https://en.wikipedia.org/wiki/File_Allocation_Table#Exceptions
 [1]: http://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html
 
 ...

 --- a/fs/fat/inode.c
 +++ b/fs/fat/inode.c
 @@ -35,9 +35,47 @@
  #define CONFIG_FAT_DEFAULT_IOCHARSET 
  #endif
  
 +#define KB_IN_SECTORS 2
 +
  static int fat_default_codepage = CONFIG_FAT_DEFAULT_CODEPAGE;
  static char fat_default_iocharset[] = CONFIG_FAT_DEFAULT_IOCHARSET;
  
 +static struct fat_floppy_defaults {
 + unsigned nr_sectors;
 + unsigned sec_per_clus;
 + unsigned dir_entries;
 + unsigned media;
 + unsigned fat_length;
 +} floppy_defaults[] = {
 +{
 + .nr_sectors = 160 * KB_IN_SECTORS,
 + .sec_per_clus = 1,
 + .dir_entries = 64,
 + .media = 0xFE,
 + .fat_length = 1,
 +},
 +{
 + .nr_sectors = 180 * KB_IN_SECTORS,
 + .sec_per_clus = 1,
 + .dir_entries = 64,
 + .media = 0xFC,
 + .fat_length = 2,
 +},
 +{
 + .nr_sectors = 320 * KB_IN_SECTORS,
 + .sec_per_clus = 2,
 + .dir_entries = 112,
 + .media = 0xFF,
 + .fat_length = 1,
 +},
 +{
 + .nr_sectors = 360 * KB_IN_SECTORS,
 + .sec_per_clus = 2,
 + .dir_entries = 112,
 + .media = 0xFD,
 + .fat_length = 2,
 +},
 +{ 0 } };

We don't really need this EOF element.

  static int fat_add_cluster(struct inode *inode)
  {
 @@ -1246,6 +1284,58 @@ static unsigned long calc_fat_clusters(struct 
 super_block *sb)
  }
  
  /*
 + * If this FAT filesystem is archaic (lacking a BIOS Parameter Block, ca. DOS
 + * 1.x), fix it up in-place by creating a DOS 2.x BIOS Parameter Block from
 + * defaults for the media size.
 + */
 +static void fat_update_archaic_boot_sector(struct super_block *sb,
 + struct fat_boot_sector *b)
 +{
 + struct fat_floppy_defaults *di;
 + sector_t bd_sects;
 +
 + /* 16-bit DOS 1.x reliably wrote bootstrap short-jmp code */
 + if (b-ignored[0] != 0xeb || b-ignored[2] != 0x90)
 + return;
 +
 + /*
 +  * If any value in this region is non-zero, don't assume it is archaic
 +  * DOS.
 +  */
 + if (get_unaligned_le16(b-sector_size) != 0 || b-sec_per_clus != 0 ||
 + b-reserved != 0 || b-fats != 0 ||
 + get_unaligned_le16(b-dir_entries) != 0 ||
 + get_unaligned_le16(b-sectors) != 0 || b-media != 0 ||
 + b-fat_length != 0 || b-secs_track != 0 || b-heads != 0 ||
 + b-secs_track != 0 || b-heads != 0)

Impressive!

 + return;
 +
 + bd_sects = part_nr_sects_read(sb-s_bdev-bd_part);
 + for (di = floppy_defaults; di-nr_sectors; di++) {

Can do something like

for (di = floppy_defaults;
di  floppy_defaults + ARRAY_SIZE(floppy_defaults); di++) {

 + if (di-nr_sectors == bd_sects)
 + break;
 + }
 + if (di-nr_sectors == 0) {
 + fat_msg(sb, KERN_WARNING,
 + DOS volume lacks BPB and isn't a recognized floppy 
 size (%ld sectors),
 + (long)bd_sects);

sector_t can be u64 on 32-bit so one should really use %Lu and cast to
u64.


 ...

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Conrad Meyer 
On Mon, Mar 31, 2014 at 3:13 PM, Andrew Morton
a...@linux-foundation.org wrote:
 On Sat, 29 Mar 2014 12:10:35 -0700 Conrad Meyer ceme...@uw.edu wrote:

 + .nr_sectors = 360 * KB_IN_SECTORS,
 + .sec_per_clus = 2,
 + .dir_entries = 112,
 + .media = 0xFD,
 + .fat_length = 2,
 +},
 +{ 0 } };

 We don't really need this EOF element.

Ah, right, I forgot about ARRAY_SIZE. This is an old version of this
patch, see v3 here: https://lkml.org/lkml/2014/3/31/275

But the same criticism is valid there, too.

 + if (get_unaligned_le16(b-sector_size) != 0 || b-sec_per_clus != 0 ||
 + b-reserved != 0 || b-fats != 0 ||
 + get_unaligned_le16(b-dir_entries) != 0 ||
 + get_unaligned_le16(b-sectors) != 0 || b-media != 0 ||
 + b-fat_length != 0 || b-secs_track != 0 || b-heads != 0 ||
 + b-secs_track != 0 || b-heads != 0)

 Impressive!

I aim to please. Not sure what would be better -- memcmp() part of the
struct to a zeroed array?


 + return;
 +
 + bd_sects = part_nr_sects_read(sb-s_bdev-bd_part);
 + for (di = floppy_defaults; di-nr_sectors; di++) {

 Can do something like

 for (di = floppy_defaults;
 di  floppy_defaults + ARRAY_SIZE(floppy_defaults); di++) {

Yep, I should revise and send a v4 patch. Thanks.


 + if (di-nr_sectors == bd_sects)
 + break;
 + }
 + if (di-nr_sectors == 0) {
 + fat_msg(sb, KERN_WARNING,
 + DOS volume lacks BPB and isn't a recognized floppy 
 size (%ld sectors),
 + (long)bd_sects);

 sector_t can be u64 on 32-bit so one should really use %Lu and cast to
 u64.

Thanks, will fix.

Conrad
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Andrew Morton 
On Mon, 31 Mar 2014 15:21:17 -0700 Conrad Meyer cse@gmail.com wrote:

  + if (get_unaligned_le16(b-sector_size) != 0 || b-sec_per_clus != 0 
  ||
  + b-reserved != 0 || b-fats != 0 ||
  + get_unaligned_le16(b-dir_entries) != 0 ||
  + get_unaligned_le16(b-sectors) != 0 || b-media != 0 ||
  + b-fat_length != 0 || b-secs_track != 0 || b-heads != 0 ||
  + b-secs_track != 0 || b-heads != 0)
 
  Impressive!
 
 I aim to please.

No great improvements immediately occur to me ;)

One could do

/* nice comment */
if (get_unaligned_le16(b-sector_size) != 0)
return;
/* another nice comment */
if (b-sec_per_clus != 0)
return;
...

but one would quickly run out of nice comments.

You could do s/ != 0//g.

 Not sure what would be better -- memcmp() part of the
 struct to a zeroed array?

memcmp would be hacky.  And possibly buggy if there are holes in the
struct, which is arch-dependent, shudder.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-31 Thread Stephen Rothwell 
On Mon, 31 Mar 2014 15:32:32 -0700 Andrew Morton a...@linux-foundation.org 
wrote:

 On Mon, 31 Mar 2014 15:21:17 -0700 Conrad Meyer cse@gmail.com wrote:
 
   + if (get_unaligned_le16(b-sector_size) != 0 || b-sec_per_clus != 
   0 ||
   + b-reserved != 0 || b-fats != 0 ||
   + get_unaligned_le16(b-dir_entries) != 0 ||
   + get_unaligned_le16(b-sectors) != 0 || b-media != 0 ||
   + b-fat_length != 0 || b-secs_track != 0 || b-heads != 0 
   ||
   + b-secs_track != 0 || b-heads != 0)
  
   Impressive!
  
  I aim to please.
 
 No great improvements immediately occur to me ;)
 
 One could do
 
   /* nice comment */
   if (get_unaligned_le16(b-sector_size) != 0)
   return;
   /* another nice comment */
   if (b-sec_per_clus != 0)
   return;
   ...
 
 but one would quickly run out of nice comments.
 
 You could do s/ != 0//g.

You could also put the boolean expression in a (hopefully expressively
named) helper function and do the tests separately there.

-- 
Cheers,
Stephen Rothwells...@canb.auug.org.au


pgp4kYwXbBv0E.pgp
Description: PGP signature

[PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-29 Thread Conrad Meyer 
When possible, infer DOS 2.x BIOS Parameter Block from block device
geometry (for floppies and floppy images). Update in-memory only. We
only perform this update when the entire BPB region is zeroed, like
produced by DOS 1.x-era FORMAT (and other OEM variations on DOS).

Fixes kernel.org bug #42617.

BPB default values are inferred from media size and a table.[0] Media
size is assumed to be static for archaic FAT volumes. See also [1].

[0]: https://en.wikipedia.org/wiki/File_Allocation_Table#Exceptions
[1]: http://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html

Signed-off-by: Conrad Meyer 
---
Diff from v3.14-rc8.

Changes since v1:
  * Check for FAT bootstrap prefix (EB xx 90) to help avoid conflicting with
other filesystems
  * Move default from a switch to a static table

Thanks!
---
 fs/fat/inode.c | 93 ++
 1 file changed, 93 insertions(+)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 854b578..c31fbdc 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -35,9 +35,47 @@
 #define CONFIG_FAT_DEFAULT_IOCHARSET   ""
 #endif
 
+#define KB_IN_SECTORS 2
+
 static int fat_default_codepage = CONFIG_FAT_DEFAULT_CODEPAGE;
 static char fat_default_iocharset[] = CONFIG_FAT_DEFAULT_IOCHARSET;
 
+static struct fat_floppy_defaults {
+   unsigned nr_sectors;
+   unsigned sec_per_clus;
+   unsigned dir_entries;
+   unsigned media;
+   unsigned fat_length;
+} floppy_defaults[] = {
+{
+   .nr_sectors = 160 * KB_IN_SECTORS,
+   .sec_per_clus = 1,
+   .dir_entries = 64,
+   .media = 0xFE,
+   .fat_length = 1,
+},
+{
+   .nr_sectors = 180 * KB_IN_SECTORS,
+   .sec_per_clus = 1,
+   .dir_entries = 64,
+   .media = 0xFC,
+   .fat_length = 2,
+},
+{
+   .nr_sectors = 320 * KB_IN_SECTORS,
+   .sec_per_clus = 2,
+   .dir_entries = 112,
+   .media = 0xFF,
+   .fat_length = 1,
+},
+{
+   .nr_sectors = 360 * KB_IN_SECTORS,
+   .sec_per_clus = 2,
+   .dir_entries = 112,
+   .media = 0xFD,
+   .fat_length = 2,
+},
+{ 0 } };
 
 static int fat_add_cluster(struct inode *inode)
 {
@@ -1246,6 +1284,58 @@ static unsigned long calc_fat_clusters(struct 
super_block *sb)
 }
 
 /*
+ * If this FAT filesystem is archaic (lacking a BIOS Parameter Block, ca. DOS
+ * 1.x), fix it up in-place by creating a DOS 2.x BIOS Parameter Block from
+ * defaults for the media size.
+ */
+static void fat_update_archaic_boot_sector(struct super_block *sb,
+   struct fat_boot_sector *b)
+{
+   struct fat_floppy_defaults *di;
+   sector_t bd_sects;
+
+   /* 16-bit DOS 1.x reliably wrote bootstrap short-jmp code */
+   if (b->ignored[0] != 0xeb || b->ignored[2] != 0x90)
+   return;
+
+   /*
+* If any value in this region is non-zero, don't assume it is archaic
+* DOS.
+*/
+   if (get_unaligned_le16(>sector_size) != 0 || b->sec_per_clus != 0 ||
+   b->reserved != 0 || b->fats != 0 ||
+   get_unaligned_le16(>dir_entries) != 0 ||
+   get_unaligned_le16(>sectors) != 0 || b->media != 0 ||
+   b->fat_length != 0 || b->secs_track != 0 || b->heads != 0 ||
+   b->secs_track != 0 || b->heads != 0)
+   return;
+
+   bd_sects = part_nr_sects_read(sb->s_bdev->bd_part);
+   for (di = floppy_defaults; di->nr_sectors; di++) {
+   if (di->nr_sectors == bd_sects)
+   break;
+   }
+   if (di->nr_sectors == 0) {
+   fat_msg(sb, KERN_WARNING,
+   "DOS volume lacks BPB and isn't a recognized floppy 
size (%ld sectors)",
+   (long)bd_sects);
+   return;
+   }
+
+   fat_msg(sb, KERN_INFO,
+   "Volume lacks BPB but looks like archaic DOS; assuming default 
BPB values");
+
+   b->sec_per_clus = di->sec_per_clus;
+   put_unaligned_le16(di->dir_entries, >dir_entries);
+   b->media = di->media;
+   b->fat_length = cpu_to_le16(di->fat_length);
+   put_unaligned_le16(SECTOR_SIZE, >sector_size);
+   b->reserved = cpu_to_le16(1);
+   b->fats = 2;
+   put_unaligned_le16(bd_sects, >sectors);
+}
+
+/*
  * Read the super block of an MS-DOS FS.
  */
 int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
@@ -1297,6 +1387,8 @@ int fat_fill_super(struct super_block *sb, void *data, 
int silent, int isvfat,
}
 
b = (struct fat_boot_sector *) bh->b_data;
+   fat_update_archaic_boot_sector(sb, b);
+
if (!b->reserved) {
if (!silent)
fat_msg(sb, KERN_ERR, "bogus number of reserved 
sectors");
@@ -1364,6 +1456,7 @@ int fat_fill_super(struct super_block *sb, void *data, 
int silent, int isvfat,
goto out_fail;
}
b = (struct fat_boot_sector *) bh->b_data;
+   fat_update_archaic_boot_sector(sb, b);

[PATCH v2] fs: FAT: Add support for DOS 1.x formatted volumes

2014-03-29 Thread Conrad Meyer 
When possible, infer DOS 2.x BIOS Parameter Block from block device
geometry (for floppies and floppy images). Update in-memory only. We
only perform this update when the entire BPB region is zeroed, like
produced by DOS 1.x-era FORMAT (and other OEM variations on DOS).

Fixes kernel.org bug #42617.

BPB default values are inferred from media size and a table.[0] Media
size is assumed to be static for archaic FAT volumes. See also [1].

[0]: https://en.wikipedia.org/wiki/File_Allocation_Table#Exceptions
[1]: http://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html

Signed-off-by: Conrad Meyer cse@gmail.com
---
Diff from v3.14-rc8.

Changes since v1:
  * Check for FAT bootstrap prefix (EB xx 90) to help avoid conflicting with
other filesystems
  * Move default from a switch to a static table

Thanks!
---
 fs/fat/inode.c | 93 ++
 1 file changed, 93 insertions(+)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 854b578..c31fbdc 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -35,9 +35,47 @@
 #define CONFIG_FAT_DEFAULT_IOCHARSET   
 #endif
 
+#define KB_IN_SECTORS 2
+
 static int fat_default_codepage = CONFIG_FAT_DEFAULT_CODEPAGE;
 static char fat_default_iocharset[] = CONFIG_FAT_DEFAULT_IOCHARSET;
 
+static struct fat_floppy_defaults {
+   unsigned nr_sectors;
+   unsigned sec_per_clus;
+   unsigned dir_entries;
+   unsigned media;
+   unsigned fat_length;
+} floppy_defaults[] = {
+{
+   .nr_sectors = 160 * KB_IN_SECTORS,
+   .sec_per_clus = 1,
+   .dir_entries = 64,
+   .media = 0xFE,
+   .fat_length = 1,
+},
+{
+   .nr_sectors = 180 * KB_IN_SECTORS,
+   .sec_per_clus = 1,
+   .dir_entries = 64,
+   .media = 0xFC,
+   .fat_length = 2,
+},
+{
+   .nr_sectors = 320 * KB_IN_SECTORS,
+   .sec_per_clus = 2,
+   .dir_entries = 112,
+   .media = 0xFF,
+   .fat_length = 1,
+},
+{
+   .nr_sectors = 360 * KB_IN_SECTORS,
+   .sec_per_clus = 2,
+   .dir_entries = 112,
+   .media = 0xFD,
+   .fat_length = 2,
+},
+{ 0 } };
 
 static int fat_add_cluster(struct inode *inode)
 {
@@ -1246,6 +1284,58 @@ static unsigned long calc_fat_clusters(struct 
super_block *sb)
 }
 
 /*
+ * If this FAT filesystem is archaic (lacking a BIOS Parameter Block, ca. DOS
+ * 1.x), fix it up in-place by creating a DOS 2.x BIOS Parameter Block from
+ * defaults for the media size.
+ */
+static void fat_update_archaic_boot_sector(struct super_block *sb,
+   struct fat_boot_sector *b)
+{
+   struct fat_floppy_defaults *di;
+   sector_t bd_sects;
+
+   /* 16-bit DOS 1.x reliably wrote bootstrap short-jmp code */
+   if (b-ignored[0] != 0xeb || b-ignored[2] != 0x90)
+   return;
+
+   /*
+* If any value in this region is non-zero, don't assume it is archaic
+* DOS.
+*/
+   if (get_unaligned_le16(b-sector_size) != 0 || b-sec_per_clus != 0 ||
+   b-reserved != 0 || b-fats != 0 ||
+   get_unaligned_le16(b-dir_entries) != 0 ||
+   get_unaligned_le16(b-sectors) != 0 || b-media != 0 ||
+   b-fat_length != 0 || b-secs_track != 0 || b-heads != 0 ||
+   b-secs_track != 0 || b-heads != 0)
+   return;
+
+   bd_sects = part_nr_sects_read(sb-s_bdev-bd_part);
+   for (di = floppy_defaults; di-nr_sectors; di++) {
+   if (di-nr_sectors == bd_sects)
+   break;
+   }
+   if (di-nr_sectors == 0) {
+   fat_msg(sb, KERN_WARNING,
+   DOS volume lacks BPB and isn't a recognized floppy 
size (%ld sectors),
+   (long)bd_sects);
+   return;
+   }
+
+   fat_msg(sb, KERN_INFO,
+   Volume lacks BPB but looks like archaic DOS; assuming default 
BPB values);
+
+   b-sec_per_clus = di-sec_per_clus;
+   put_unaligned_le16(di-dir_entries, b-dir_entries);
+   b-media = di-media;
+   b-fat_length = cpu_to_le16(di-fat_length);
+   put_unaligned_le16(SECTOR_SIZE, b-sector_size);
+   b-reserved = cpu_to_le16(1);
+   b-fats = 2;
+   put_unaligned_le16(bd_sects, b-sectors);
+}
+
+/*
  * Read the super block of an MS-DOS FS.
  */
 int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
@@ -1297,6 +1387,8 @@ int fat_fill_super(struct super_block *sb, void *data, 
int silent, int isvfat,
}
 
b = (struct fat_boot_sector *) bh-b_data;
+   fat_update_archaic_boot_sector(sb, b);
+
if (!b-reserved) {
if (!silent)
fat_msg(sb, KERN_ERR, bogus number of reserved 
sectors);
@@ -1364,6 +1456,7 @@ int fat_fill_super(struct super_block *sb, void *data, 
int silent, int isvfat,
goto out_fail;
}
b = (struct fat_boot_sector *) bh-b_data;
+   fat_update_archaic_boot_sector(sb, b);
}