FAT: check for partition 0 not 1 for whole-disk fs

PATCH New Stephen Warren 2 participants 8 messages Oct 5, 2012 – Oct 8, 2012 VFAT/FAT/msdos filesystem

lore

Share & tools

lore

Stephen Warren

author

2012-10-05 23:17 UTC #0 lore raw

PATCH 1/2 FAT: check for partition 0 not 1 for whole-disk fs

From: Stephen Warren <[email protected]>

The recent switch to use get_device_and_partition() from do_fat_ls() broke the ability to access a FAT filesystem directly on a whole device; FAT only works within a partition on a device.

This change makes e.g. "fatls mmc 0:0" work; explicitly requesting partition ID 0 is something that get_device_and_partition() fully supports. However, fat_register_device() expects partition ID 1 to be used in the full-disk case; partition ID 1 was previously implicitly specified when the user didn't actually specify a partition ID. Update fat_register_device() to expect the correct ID.

This change does imply that if a user explicitly executes "fatls mmc 0:1" then this will fail, and may be a change in behaviour.

Note that this still prevents "fatls mmc 0:auto" from working. The next patch will fix that.

Signed-off-by Stephen Warren <[email protected]>

---
Tom, this series is really a bug-fix and should go in before the 9-long series I posted earlier today. I'll need to rebase that other series on top of this and repost, once any comments are addressed.

---
 fs/fat/fat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/fat/fat.c +1 -1

index 41ae15e..80156c8 100644
@@ -85,7 +85,7 @@ int fat_register_device(block_dev_desc_t * dev_desc, int part_no)
 
 	/* Otherwise it might be a superfloppy (whole-disk FAT filesystem) */
 	if (!cur_dev) {
-		if (part_no != 1) {
+		if (part_no != 0) {
 			printf("** Partition %d not valid on device %d **\n",
 					part_no, dev_desc->dev);
 			return -1;

Signature · 1 line

1.7.0.4

Stephen Warren

author

2012-10-05 23:17 UTC #1 lore raw

PATCH 2/2 disk: part_dos: don't claim whole-disk FAT filesystems

From: Stephen Warren <[email protected]>

Logically, a disk that contains a raw FAT filesystem does not in fact have a partition table. However, test_part_dos() was claiming that such disks did in fact have a DOS-style partition table. This caused get_device_and_partition() not to return a whole-disk disk_partition_t, since part_type != PART_TYPE_UNKNOWN.

part_dos.c's print_partition_extended() detected the raw FAT filesystem condition and printed a fake partition table that encompassed the whole disk.

However, part_dos.c's get_partition_info_extended() did not return any valid partitions in this case. This combination caused get_device_and_partition() not to find any valid partitions, and hence to return an error.

Fix test_part_dos() not to claim that raw FAT filesystems are DOS partition tables. In turn, this causes get_device_and_partition() to return a whole-disk disk_partition_t, and hence the following commands work:

fatls mmc 0 /
fatls mmc 0:auto /

An alternative would be to modify print_partition_extended() to detect raw FAT filesystems, just like print_partition_extended() does, and to return a fake partition in this case. However, this seems logically incorrect, and also duplicates code, since get_device_and_partition() falls back to returning a whole-disk partition when there is no partition table on the device.

Signed-off-by Stephen Warren <[email protected]>

---
 disk/part_dos.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

disk/part_dos.c +9 ~~-12~~

index c9a3e2b..5c454e6 100644
@@ -94,12 +94,13 @@ int test_part_dos (block_dev_desc_t *dev_desc)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 
-	if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) ||
-	    (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) ||
-	    (buffer[DOS_PART_MAGIC_OFFSET + 1] != 0xaa) ) {
-		return (-1);
-	}
-	return (0);
+	if (dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1)
+		return -1;
+
+	if (test_block_type(buffer) != DOS_MBR)
+		return -1;
+
+	return 0;
 }
 
 /*  Print a partition that is relative to its Extended partition table

@@ -117,17 +118,13 @@ static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_s
 		return;
 	}
 	i=test_block_type(buffer);
-	if(i==-1) {
+	if (i != DOS_MBR) {
 		printf ("bad MBR sector signature 0x%02x%02x\n",
 			buffer[DOS_PART_MAGIC_OFFSET],
 			buffer[DOS_PART_MAGIC_OFFSET + 1]);
 		return;
 	}
-	if(i==DOS_PBR) {
-		printf ("    1\t\t         0\t%10ld\t%2x\n",
-			dev_desc->lba, buffer[DOS_PBR_MEDIA_TYPE_OFFSET]);
-		return;
-	}
+
 	/* Print all primary/logical partitions */
 	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);
 	for (i = 0; i < 4; i++, pt++) {

Signature · 1 line

1.7.0.4

Tom Rini

2012-10-08 18:47 UTC #2 lore raw

PATCH 2/2 disk: part_dos: don't claim whole-disk FAT filesystems

Quoted 25 lines · Stephen Warren

From: Stephen Warren <[email protected]>
Logically, a disk that contains a raw FAT filesystem does not in fact have a partition table. However, test_part_dos() was claiming that such disks did in fact have a DOS-style partition table. This caused get_device_and_partition() not to return a whole-disk disk_partition_t, since part_type != PART_TYPE_UNKNOWN.
part_dos.c's print_partition_extended() detected the raw FAT filesystem condition and printed a fake partition table that encompassed the whole disk.
However, part_dos.c's get_partition_info_extended() did not return any valid partitions in this case. This combination caused get_device_and_partition() not to find any valid partitions, and hence to return an error.
Fix test_part_dos() not to claim that raw FAT filesystems are DOS partition tables. In turn, this causes get_device_and_partition() to return a whole-disk disk_partition_t, and hence the following commands work:
fatls mmc 0 /
fatls mmc 0:auto /

Quoted 8 lines

An alternative would be to modify print_partition_extended() to detect raw FAT filesystems, just like print_partition_extended() does, and to return a fake partition in this case. However, this seems logically incorrect, and also duplicates code, since get_device_and_partition() falls back to returning a whole-disk partition when there is no partition table on the device.
Signed-off-by Stephen Warren <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121008/cdc79a84/attachment.pgp>

Tom Rini

2012-10-05 23:27 UTC #3 lore raw

PATCH 1/2 FAT: check for partition 0 not 1 for whole-disk fs

In reply to Stephen Warren

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 14 lines · Stephen Warren

From: Stephen Warren <[email protected]>
The recent switch to use get_device_and_partition() from do_fat_ls() broke the ability to access a FAT filesystem directly on a whole device; FAT only works within a partition on a device.
This change makes e.g. "fatls mmc 0:0" work; explicitly requesting partition ID 0 is something that get_device_and_partition() fully supports. However, fat_register_device() expects partition ID 1 to be used in the full-disk case; partition ID 1 was previously implicitly specified when the user didn't actually specify a partition ID. Update fat_register_device() to expect the correct ID.

Quoted 2 lines

This change does imply that if a user explicitly executes "fatls mmc 0:1" then this will fail, and may be a change in behaviour.

So wait, you can't list device 0, bus 1 after this patch?

Quoted 7 lines

Note that this still prevents "fatls mmc 0:auto" from working. The next patch will fix that.
Signed-off-by Stephen Warren <[email protected]> --- Tom, this
series is really a bug-fix and should go in before the 9-long series I posted earlier today. I'll need to rebase that other series on top of this and repost, once any comments are addressed.

Should this also go in the release?

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQb2zkAAoJENk4IS6UOR1WbOoP/jHEC/4KcnWN8xPES0T35Ji1 GwJUdc2O+myjoxNgZaANPWBc8Mcy8Qq+QSSx/gti2BhhCww8ly6IuoVBr7unHAF2 syPoRGw3MP6av67MCPItcycIH6Ym/Y1ZZg+SdZHMJzDIAt7L8u7t5R21sym74iS9 xdamVX98ew6ZwUFHv5AhCt+47CqzAjugU3H1lYuer8RXcgX+kcHeNDVUHBmqNA69 HuysditT3UQWtE2Wcmq98xI3Mvd7mRFUdx6+BFla+lfh2C9/OfWoVzipMP84d9xI k61VAT3ZpMd5maL4SLy+k7E92bMGN0wvxD2cvlCLnEeKrcd7c6+CZqDMFRmdtiPI 43rklb9ZX1K2vjQzIZL3GOOEGEHQvJ7mTcWkQCHeyyrJaRkx3uChPpIyaar3y1/Y 8ezqO4iVqhiY5YJyUd2v1Kj28S0ilCqxKVl/dt5yzde9EGzsOpDoCHmMCQTzfOiD NhZyiyMtg8IzEhCaaqKL975O6JlsHPMNFmsESAkFExl+LYRYHq1itCZUPkrH400p gV/72J7K5q+Z8o+iCp68glL2BMLIj6Vz3aGDngQIziKJJQtksVHcshpuFyDYqaB4 ebgE3AMQJnOBllLPlf+BZXAZpCCL5zDY1kO8pEpp7MRNgEEsUIOKb//E6wDP5PUL 7+kooEgl/EbnUpQZmfEb
=goTb
-----END PGP SIGNATURE-----

Stephen Warren

author

2012-10-05 23:36 UTC #4 lore raw

PATCH 1/2 FAT: check for partition 0 not 1 for whole-disk fs

Quoted 16 lines · Tom Rini

Quoted 14 lines · Stephen Warren
From: Stephen Warren <[email protected]>
The recent switch to use get_device_and_partition() from do_fat_ls() broke the ability to access a FAT filesystem directly on a whole device; FAT only works within a partition on a device.
This change makes e.g. "fatls mmc 0:0" work; explicitly requesting partition ID 0 is something that get_device_and_partition() fully supports. However, fat_register_device() expects partition ID 1 to be used in the full-disk case; partition ID 1 was previously implicitly specified when the user didn't actually specify a partition ID. Update fat_register_device() to expect the correct ID.

Quoted 4 lines

Quoted 2 lines
This change does imply that if a user explicitly executes "fatls mmc 0:1" then this will fail, and may be a change in behaviour.
So wait, you can't list device 0, bus 1 after this patch?

That's partition 1 not bus 1.

In the context of having a raw FAT filesystem on a device with no partition table, the partition specification "0:1" doesn't work before or after this patch; I believe (if it worked at all ever before) it was broken by the previous get_device_and_partition() rework.

If you do have a partition table, then "0:1" works just fine with/without this patch.

Quoted 10 lines

Quoted 8 lines
Note that this still prevents "fatls mmc 0:auto" from working. The next patch will fix that.
Signed-off-by Stephen Warren <[email protected]> --- Tom, this
series is really a bug-fix and should go in before the 9-long series I posted earlier today. I'll need to rebase that other series on top of this and repost, once any comments are addressed.
Should this also go in the release?

I imagine so; that's why I rebased it below the previous series I sent which I assume isn't going into this release.

Tom Rini

2012-10-05 23:39 UTC #5 lore raw

PATCH 1/2 FAT: check for partition 0 not 1 for whole-disk fs

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 17 lines · Stephen Warren

Quoted 16 lines · Tom Rini
Quoted 14 lines · Stephen Warren
From: Stephen Warren <[email protected]>
The recent switch to use get_device_and_partition() from do_fat_ls() broke the ability to access a FAT filesystem directly on a whole device; FAT only works within a partition on a device.
This change makes e.g. "fatls mmc 0:0" work; explicitly requesting partition ID 0 is something that get_device_and_partition() fully supports. However, fat_register_device() expects partition ID 1 to be used in the full-disk case; partition ID 1 was previously implicitly specified when the user didn't actually specify a partition ID. Update fat_register_device() to expect the correct ID.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
This change does imply that if a user explicitly executes "fatls mmc 0:1" then this will fail, and may be a change in behaviour.
So wait, you can't list device 0, bus 1 after this patch?
That's partition 1 not bus 1.

Er yes, thinko there.

Quoted 8 lines

In the context of having a raw FAT filesystem on a device with no partition table, the partition specification "0:1" doesn't work before or after this patch; I believe (if it worked at all ever before) it was broken by the previous get_device_and_partition() rework.
If you do have a partition table, then "0:1" works just fine with/without this patch.

OK, so the behavior change here, potentially involves 2 partitions without a partition table? What is the case where fatls mmc 0:1 would fail now?

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQb2/IAAoJENk4IS6UOR1WKbEQALH+HYDHhJeA0EYf8iypop65 2FIekgWaT2lrlQNEowEVGHp7kenHPVgQ8EjbE3TzymWS3QLdD7V9gCJqOXa5ozqQ Nw/sFTcw8j/u83yy7mby0scwxkc3ce0PNly1JH4+L1axKuPUdcXWa52ITy4W1XmZ 348xamJQktrWOUaaUJRMaQP7qCWUNiBZniCTKUIhJhZITEch/JHoF7QkY4PM082X pZB3wDjQJ8CdnsdutzeegiHLwLRm4bWjfy159uvTQePoSx0NJdSsJpU/Zxf+0t6i CCf8rvYz1vKiH2u7D2cWuB61Cg/VwCfhN+wEGL4FSvKSh1zUsKa9fJtHtpw/ueB8 /iAi8dKA1cGzr5Ym0aUrz9UL2ZZ6IO4HTrNW5xGRK8TsbgL+kcgSzF8C9V5T2WF9 J++Ol5yGIjgqHPf/H7pulQ0uJ5NU70JbUB5AYLc5FWW+rULJP8rECQpSC74F3fo0 4HfoOrry/IwCBRSHE1crWIyvP+DmnDEHKX7XEhh8P3nqagwhF9LlSQwcEhATjrBp wEeqZD8A7hViqjuz8aG5J7qVxUPOB1VfzwTzNM6zKMgHbizahmbokcYLFHpORoQ8 YRNjdxgksWAtVSizJlfxOa4D7/GV4pLXUJhqfUYIk8afkbt48YuOUbtwApkLSNLN QM3oQOs6y1RK8qy+wBHq
=bUng
-----END PGP SIGNATURE-----

Stephen Warren

author

2012-10-05 23:46 UTC #6 lore raw

PATCH 1/2 FAT: check for partition 0 not 1 for whole-disk fs

Quoted 34 lines · Tom Rini

Quoted 17 lines · Stephen Warren
Quoted 16 lines · Tom Rini
Quoted 14 lines · Stephen Warren
From: Stephen Warren <[email protected]>
The recent switch to use get_device_and_partition() from do_fat_ls() broke the ability to access a FAT filesystem directly on a whole device; FAT only works within a partition on a device.
This change makes e.g. "fatls mmc 0:0" work; explicitly requesting partition ID 0 is something that get_device_and_partition() fully supports. However, fat_register_device() expects partition ID 1 to be used in the full-disk case; partition ID 1 was previously implicitly
specified when the user didn't actually specify a partition
ID. Update fat_register_device() to expect the correct ID.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
This change does imply that if a user explicitly executes "fatls mmc 0:1" then this will fail, and may be a change in behaviour.
So wait, you can't list device 0, bus 1 after this patch?
That's partition 1 not bus 1.
Er yes, thinko there.
Quoted 5 lines
In the context of having a raw FAT filesystem on a device with no
partition table, the partition specification "0:1" doesn't work
before or after this patch; I believe (if it worked at all ever before) it was broken by the previous get_device_and_partition() rework.

Quoted 6 lines

Quoted 2 lines
If you do have a partition table, then "0:1" works just fine with/without this patch.
OK, so the behavior change here, potentially involves 2 partitions without a partition table? What is the case where fatls mmc 0:1 would fail now?

The failure case involves zero partitions. The issue is that the following works just fine:

fdisk /dev/sda
mkfs.dos /dev/sdaN for any N

but the following fails:

mkfs.dos /dev/sda (i.e. raw on the disk with no partition table)

In the latter case, I /think/ either of the following might have worked in U-Boot until recently:

(1) fatls mmc 0 / # Surely this worked
(2) fatls mmc 0:1 / # I'm not sure if this works

This patch makes (1) above work.

Patch 2 in this series makes the new equivalent of (2) work, namely:

fatls mmc 0:0 /

The second 0 there is "partition 0" which means "the whole disk". IIRC, I called out that behaviour change in the get_device_and_partition() series anyway.

Tom Rini

2012-10-08 18:47 UTC #7 lore raw

PATCH 1/2 FAT: check for partition 0 not 1 for whole-disk fs

In reply to Stephen Warren

Quoted 19 lines · Stephen Warren

From: Stephen Warren <[email protected]>
The recent switch to use get_device_and_partition() from do_fat_ls() broke the ability to access a FAT filesystem directly on a whole device; FAT only works within a partition on a device.
This change makes e.g. "fatls mmc 0:0" work; explicitly requesting partition ID 0 is something that get_device_and_partition() fully supports. However, fat_register_device() expects partition ID 1 to be used in the full-disk case; partition ID 1 was previously implicitly specified when the user didn't actually specify a partition ID. Update fat_register_device() to expect the correct ID.
This change does imply that if a user explicitly executes "fatls mmc 0:1" then this will fail, and may be a change in behaviour.
Note that this still prevents "fatls mmc 0:auto" from working. The next patch will fix that.

Quoted 6 lines

Signed-off-by Stephen Warren <[email protected]>
---
Tom, this series is really a bug-fix and should go in before the 9-long series I posted earlier today. I'll need to rebase that other series on top of this and repost, once any comments are addressed.

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121008/333d45e1/attachment.pgp>