[PATCH 0/5] partitions: Changes to fs/partitions for readability and efficiency

[PATCH 0/5] partitions: Changes to fs/partitions for readability and efficiency

[John Anthony Kazos Jr.]

In addition to the Kconfig help text patch I submitted earlier, this is a 
set of patches to touch up the partition handling files and also to change 
the "array of function pointers" algorithm of the main checking function 
to "list of calls to possible stub functions" to better fit in with the 
rest of the kernel code, to reduce memory usage by a few dozen bytes, and 
to generally be easier (in my opinion) to understand.

[PATCH 1/5] partitions: Touch up comments for check.h and ibm.h

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Adds top-of-file identifying comments to check.h and ibm.h in 
fs/partitions similar to the other files in the directory. Removes an 
obsolescent comment from check.h leftover from devfs.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

add_gd_partition used to be in check.c for devfs. That identifier no 
longer exists anywhere within the tree.

--- linux-2.6.20.6-orig/fs/partitions/check.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/check.h	2007-04-07 21:26:10.000000000 -0400
@@ -1,10 +1,10 @@
+/*
+ *  fs/partitions/check.h
+ */
+
 #include <linux/pagemap.h>
 #include <linux/blkdev.h>
 
-/*
- * add_gd_partition adds a partitions details to the devices partition
- * description.
- */
 enum { MAX_PART = 256 };
 
 struct parsed_partitions {
--- linux-2.6.20.6-orig/fs/partitions/ibm.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/ibm.h	2007-04-07 22:02:18.000000000 -0400
@@ -1 +1,5 @@
+/*
+ *  fs/partitions/ibm.h
+ */
+
 int ibm_partition(struct parsed_partitions *, struct block_device *);

Re: [PATCH 1/5] partitions: Touch up comments for check.h and ibm.h

[Christoph Hellwig]

On Sat, Apr 07, 2007 at 11:38:46PM -0400, John Anthony Kazos Jr. wrote:
> +/*
> + *  fs/partitions/check.h
> + */

this one is utterly useless and actually harmful because it can easily
get out of sync.

> @@ -1 +1,5 @@
> +/*
> + *  fs/partitions/ibm.h
> + */

ditto

[PATCH 2/5] partitions: Add Kconfig dependency to clear benign compiler warning

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Adds a dependency to ACORN_PARTITION_RISCIX in fs/partitions/Kconfig to 
prevent compilation of the function riscix_partition which is used only 
within ACORN_PARTITION_CUMANA and ACORN_PARTITION_ADFS sections, thereby 
preventing an unused-function compiler warning if ACORN_PARTITION_RISCIX 
is defined and neither of the other two are.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

Nothing depending on RISCIX is defined except inside CUMANA and ADFS, so 
without either of those, nothing using the RISCIX stuff is compiled, hance 
the unused-function warning.

--- linux-2.6.20.6-orig/fs/partitions/Kconfig	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/Kconfig	2007-04-07 22:10:15.000000000 -0400
@@ -62,7 +62,7 @@ config ACORN_PARTITION_POWERTEC
 config ACORN_PARTITION_RISCIX
 	bool "RISCiX partition support" if PARTITION_ADVANCED
 	default y if ARCH_ACORN
-	depends on ACORN_PARTITION
+	depends on ACORN_PARTITION && (ACORN_PARTITION_CUMANA || ACORN_PARTITION_ADFS)
 	help
 	  Once upon a time, there was a native Unix port for the Acorn series
 	  of machines called RISCiX.  If you say 'Y' here, Linux will be able

[PATCH 3/5] partitions: Add conditionals to acorn.c to clear benign compiler warnings

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Adds conditional-compilation directives to fs/partitions/acorn.c to 
prevent compilation of the functions adfs_partition and linux_partition 
which are used only within ACORN_PARTITION_CUMANA and ACORN_PARTITION_ADFS 
sections, thereby preventing unused-function compiler warnings if 
ACORN_PARTITION is defined and neither of the other two are.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

Nothing using these two functions is defined except inside CUMANA and 
ADFS, so without either of those, nothing using these functions is 
compiled, hence the unused-function warning. Both CUMANA and ADFS code use 
both functions so they must be included if either symbol is defined.

--- linux-2.6.20.6-orig/fs/partitions/acorn.c	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/acorn.c	2007-04-07 21:40:39.000000000 -0400
@@ -25,6 +25,8 @@
 #define PARTITION_RISCIX_SCSI	2
 #define PARTITION_LINUX		9
 
+#if defined(CONFIG_ACORN_PARTITION_CUMANA) || defined(CONFIG_ACORN_PARTITION_ADFS)
+
 static struct adfs_discrecord *
 adfs_partition(struct parsed_partitions *state, char *name, char *data,
 	       unsigned long first_sector, int slot)
@@ -49,6 +51,8 @@ adfs_partition(struct parsed_partitions 
 	return dr;
 }
 
+#endif
+
 #ifdef CONFIG_ACORN_PARTITION_RISCIX
 
 struct riscix_part {
@@ -106,6 +110,8 @@ riscix_partition(struct parsed_partition
 }
 #endif
 
+#if defined(CONFIG_ACORN_PARTITION_CUMANA) || defined(CONFIG_ACORN_PARTITION_ADFS)
+
 #define LINUX_NATIVE_MAGIC 0xdeafa1de
 #define LINUX_SWAP_MAGIC   0xdeafab1e
 
@@ -147,6 +153,8 @@ linux_partition(struct parsed_partitions
 	return slot;
 }
 
+#endif
+
 #ifdef CONFIG_ACORN_PARTITION_CUMANA
 int
 adfspart_check_CUMANA(struct parsed_partitions *state, struct block_device *bdev)

[PATCH 4/5] partitions: Add conditionals and static inline stubs to helpers in headers

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Functions of the form adfspart_check_FOO and foo_partition defined in 
fs/partitions/*.h are helper functions called in a deliberate order by 
check_partition in check.c. Add conditional-compilation directives and 
static inline no-op functions to allow code to indiscriminately call these 
functions irrespective of whether they do anything, removing the necessity 
of oodles of #ifdef/#endif in function bodies.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

The next patch changes the check.c code to use these function definitions 
in a nicer way.

--- linux-2.6.20.6-orig/fs/partitions/acorn.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/acorn.h	2007-04-07 20:02:51.000000000 -0400
@@ -7,8 +7,47 @@
  *  format, and everyone stick to it?
  */
 
-int adfspart_check_CUMANA(struct parsed_partitions *state, struct block_device *bdev);
-int adfspart_check_ADFS(struct parsed_partitions *state, struct block_device *bdev);
-int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev);
-int adfspart_check_POWERTEC(struct parsed_partitions *state, struct block_device *bdev);
-int adfspart_check_EESOX(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_ACORN_PARTITION_ICS
+	int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
+
+#ifdef CONFIG_ACORN_PARTITION_POWERTEC
+	int adfspart_check_POWERTEC(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int adfspart_check_POWERTEC(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
+
+#ifdef CONFIG_ACORN_PARTITION_EESOX
+	int adfspart_check_EESOX(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int adfspart_check_EESOX(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
+
+#ifdef CONFIG_ACORN_PARTITION_CUMANA
+	int adfspart_check_CUMANA(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int adfspart_check_CUMANA(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
+
+#ifdef CONFIG_ACORN_PARTITION_ADFS
+	int adfspart_check_ADFS(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int adfspart_check_ADFS(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/amiga.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/amiga.h	2007-04-07 20:04:10.000000000 -0400
@@ -2,5 +2,11 @@
  *  fs/partitions/amiga.h
  */
 
-int amiga_partition(struct parsed_partitions *state, struct block_device *bdev);
-
+#ifdef CONFIG_AMIGA_PARTITION
+	int amiga_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int amiga_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/atari.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/atari.h	2007-04-07 20:05:34.000000000 -0400
@@ -31,4 +31,11 @@ struct rootsector
   u16 checksum;			/* checksum for bootable disks */
 } __attribute__((__packed__));
 
-int atari_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_ATARI_PARTITION
+	int atari_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int atari_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/efi.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/efi.h	2007-04-07 20:06:28.000000000 -0400
@@ -106,7 +106,14 @@ typedef struct _legacy_mbr {
 } __attribute__ ((packed)) legacy_mbr;
 
 /* Functions */
-extern int efi_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_EFI_PARTITION
+	int efi_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int efi_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
 
 #endif
 
--- linux-2.6.20.6-orig/fs/partitions/ibm.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/ibm.h	2007-04-07 22:06:16.000000000 -0400
@@ -1 +1,8 @@
-int ibm_partition(struct parsed_partitions *, struct block_device *);
+#ifdef CONFIG_IBM_PARTITION
+	int ibm_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int ibm_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/karma.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/karma.h	2007-04-07 20:13:57.000000000 -0400
@@ -4,5 +4,11 @@
 
 #define KARMA_LABEL_MAGIC		0xAB56
 
-int karma_partition(struct parsed_partitions *state, struct block_device *bdev);
-
+#ifdef CONFIG_KARMA_PARTITION
+	int karma_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int karma_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/ldm.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/ldm.h	2007-04-07 20:14:34.000000000 -0400
@@ -214,7 +214,14 @@ struct ldmdb {				/* Cache of the databa
 	struct list_head v_part;
 };
 
-int ldm_partition (struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_LDM_PARTITION
+	int ldm_partition (struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int ldm_partition (struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
 
 #endif /* _FS_PT_LDM_H_ */
 
--- linux-2.6.20.6-orig/fs/partitions/mac.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/mac.h	2007-04-07 20:15:09.000000000 -0400
@@ -41,4 +41,11 @@ struct mac_driver_desc {
     /* ... more stuff */
 };
 
-int mac_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_MAC_PARTITION
+	int mac_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int mac_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/msdos.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/msdos.h	2007-04-07 20:15:40.000000000 -0400
@@ -4,5 +4,11 @@
 
 #define MSDOS_LABEL_MAGIC		0xAA55
 
-int msdos_partition(struct parsed_partitions *state, struct block_device *bdev);
-
+#ifdef CONFIG_MSDOS_PARTITION
+	int msdos_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int msdos_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/osf.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/osf.h	2007-04-07 20:16:14.000000000 -0400
@@ -4,4 +4,11 @@
 
 #define DISKLABELMAGIC (0x82564557UL)
 
-int osf_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_OSF_PARTITION
+	int osf_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int osf_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/sgi.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/sgi.h	2007-04-07 20:16:49.000000000 -0400
@@ -2,7 +2,14 @@
  *  fs/partitions/sgi.h
  */
 
-extern int sgi_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_SGI_PARTITION
+	int sgi_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int sgi_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
 
 #define SGI_LABEL_MAGIC 0x0be5a941
 
--- linux-2.6.20.6-orig/fs/partitions/sun.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/sun.h	2007-04-07 20:17:18.000000000 -0400
@@ -4,4 +4,11 @@
 
 #define SUN_LABEL_MAGIC          0xDABE
 
-int sun_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_SUN_PARTITION
+	int sun_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int sun_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif
--- linux-2.6.20.6-orig/fs/partitions/ultrix.h	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/ultrix.h	2007-04-07 20:17:45.000000000 -0400
@@ -2,4 +2,11 @@
  *  fs/partitions/ultrix.h
  */
 
-int ultrix_partition(struct parsed_partitions *state, struct block_device *bdev);
+#ifdef CONFIG_ULTRIX_PARTITION
+	int ultrix_partition(struct parsed_partitions *state, struct block_device *bdev);
+#else
+	static inline int ultrix_partition(struct parsed_partitions *state, struct block_device *bdev)
+	{
+		return 0;
+	}
+#endif

Re: [PATCH 4/5] partitions: Add conditionals and static inline stubs to helpers in headers

[Randy Dunlap]

On Sat, 7 Apr 2007 23:40:50 -0400 (EDT) John Anthony Kazos Jr. wrote:

> From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>
> 
> Functions of the form adfspart_check_FOO and foo_partition defined in 
> fs/partitions/*.h are helper functions called in a deliberate order by 
> check_partition in check.c. Add conditional-compilation directives and 
> static inline no-op functions to allow code to indiscriminately call these 
> functions irrespective of whether they do anything, removing the necessity 
> of oodles of #ifdef/#endif in function bodies.
> 
> Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>
> 
> ---
> 
> The next patch changes the check.c code to use these function definitions 
> in a nicer way.
> 
> --- linux-2.6.20.6-orig/fs/partitions/acorn.h	2007-04-06 16:02:48.000000000 -0400
> +++ linux-2.6.20.6-mod/fs/partitions/acorn.h	2007-04-07 20:02:51.000000000 -0400

Send patches against the latest Linus-tree unless the patches are
specifically for the -stable branch (where 2.6.20.y is -stable
and 2.6.21-rc6 or -git is Linus).  (Maybe it won't matter for
these patches....)

See Andrew's The Perfect Patch for more info:
  http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt


> @@ -7,8 +7,47 @@
>   *  format, and everyone stick to it?
>   */
>  
> -int adfspart_check_CUMANA(struct parsed_partitions *state, struct block_device *bdev);
> -int adfspart_check_ADFS(struct parsed_partitions *state, struct block_device *bdev);
> -int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev);
> -int adfspart_check_POWERTEC(struct parsed_partitions *state, struct block_device *bdev);
> -int adfspart_check_EESOX(struct parsed_partitions *state, struct block_device *bdev);
> +#ifdef CONFIG_ACORN_PARTITION_ICS
> +	int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev);
> +#else
> +	static inline int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev)
> +	{
> +		return 0;
> +	}
> +#endif
> +
> +#ifdef CONFIG_ACORN_PARTITION_POWERTEC
> +	int adfspart_check_POWERTEC(struct parsed_partitions *state, struct block_device *bdev);
> +#else
> +	static inline int adfspart_check_POWERTEC(struct parsed_partitions *state, struct block_device *bdev)
> +	{
> +		return 0;
> +	}
> +#endif

We don't indent functions inside ifdef/else/endif blocks.
Just act as though the preprocessor lines are not there.
(That's our current common practice; I don't see that documented
anywhere.)


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 4/5] partitions: Add conditionals and static inline stubs to helpers in headers

[Christoph Hellwig]

On Sat, Apr 07, 2007 at 11:40:50PM -0400, John Anthony Kazos Jr. wrote:
> +#ifdef CONFIG_ACORN_PARTITION_ICS
> +	int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev);
> +#else
> +	static inline int adfspart_check_ICS(struct parsed_partitions *state, struct block_device *bdev)
> +	{
> +		return 0;
> +	}
> +#endif

Please don't indent the content of #ifdef statements.  Also make sure
lines are not longer than 80 characters.

> 

[PATCH 5/5] partitions: Rewrite check_partition to remove necessity of check_part

[John Anthony Kazos Jr.]

From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

Removes the entire check_part array and uses the presence of new stub 
functions in header files in fs/partitions to call them directly in a list 
and let the compiler optimize away any that aren't compiled in. Also fixes 
a bug where " unable to read partition table" would never be printed.

Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>

---

I did this because it seems to be much easier to understand. And even 
though the memory used by the array was only sizeof(void*)*n+1 for the 
number of partitions configured to be included, that's still unnecessary 
usage.

This code also has two warn_unused_result problems, but I don't feel up to 
the task of fixing those yet.

Within the old loop, if res < 0, res = 0, and immediately after the loop, 
the function returns if res > 0. Therefore, it must be that res == 0 after 
the loop. if (!err) is true only if err == 0 so again, res == 0, so if 
(!res) is true, and the else condition can never be reached. I 
re-interpreted the intent to be "log a message if the partition format is 
unrecognized, and log a message if the partition cannot be read but only 
if the warning is requested". Judging by the "This is ugly" comment, the 
warning is not intended to account for actual I/O errors when reading the 
partition-table block, but rather to detect when the partitions are 
checked for a device with a medium that has been removed.

--- linux-2.6.20.6-orig/fs/partitions/check.c	2007-04-06 16:02:48.000000000 -0400
+++ linux-2.6.20.6-mod/fs/partitions/check.c	2007-04-07 21:50:22.000000000 -0400
@@ -41,73 +41,6 @@ extern void md_autodetect_dev(dev_t dev)
 
 int warn_no_part = 1; /*This is ugly: should make genhd removable media aware*/
 
-static int (*check_part[])(struct parsed_partitions *, struct block_device *) = {
-	/*
-	 * Probe partition formats with tables at disk address 0
-	 * that also have an ADFS boot block at 0xdc0.
-	 */
-#ifdef CONFIG_ACORN_PARTITION_ICS
-	adfspart_check_ICS,
-#endif
-#ifdef CONFIG_ACORN_PARTITION_POWERTEC
-	adfspart_check_POWERTEC,
-#endif
-#ifdef CONFIG_ACORN_PARTITION_EESOX
-	adfspart_check_EESOX,
-#endif
-
-	/*
-	 * Now move on to formats that only have partition info at
-	 * disk address 0xdc0.  Since these may also have stale
-	 * PC/BIOS partition tables, they need to come before
-	 * the msdos entry.
-	 */
-#ifdef CONFIG_ACORN_PARTITION_CUMANA
-	adfspart_check_CUMANA,
-#endif
-#ifdef CONFIG_ACORN_PARTITION_ADFS
-	adfspart_check_ADFS,
-#endif
-
-#ifdef CONFIG_EFI_PARTITION
-	efi_partition,		/* this must come before msdos */
-#endif
-#ifdef CONFIG_SGI_PARTITION
-	sgi_partition,
-#endif
-#ifdef CONFIG_LDM_PARTITION
-	ldm_partition,		/* this must come before msdos */
-#endif
-#ifdef CONFIG_MSDOS_PARTITION
-	msdos_partition,
-#endif
-#ifdef CONFIG_OSF_PARTITION
-	osf_partition,
-#endif
-#ifdef CONFIG_SUN_PARTITION
-	sun_partition,
-#endif
-#ifdef CONFIG_AMIGA_PARTITION
-	amiga_partition,
-#endif
-#ifdef CONFIG_ATARI_PARTITION
-	atari_partition,
-#endif
-#ifdef CONFIG_MAC_PARTITION
-	mac_partition,
-#endif
-#ifdef CONFIG_ULTRIX_PARTITION
-	ultrix_partition,
-#endif
-#ifdef CONFIG_IBM_PARTITION
-	ibm_partition,
-#endif
-#ifdef CONFIG_KARMA_PARTITION
-	karma_partition,
-#endif
-	NULL
-};
- 
 /*
  * disk_name() is used by partition check code and the genhd driver.
  * It formats the devicename of the indicated disk into
@@ -149,11 +82,125 @@ const char *__bdevname(dev_t dev, char *
 
 EXPORT_SYMBOL(__bdevname);
 
+static int
+check_succeeded(int res, struct parsed_partitions *state, int *err)
+{
+	if (res < 0) {
+		/* We have hit an I/O error which we don't report now.
+		 * But record it, and let the others do their job.
+		 */
+		*err = res;
+		res = 0;
+	}
+
+	if (!res) {
+		memset(state->parts, 0, sizeof(state->parts));
+	}
+
+	return res;
+}
+
+static struct parsed_partitions *
+do_check_list(struct parsed_partitions *state, struct block_device *bdev)
+{
+	int err;
+
+	err = 0;
+
+        /*
+	 * Probe partition formats with tables at disk address 0
+	 * that also have an ADFS boot block at 0xdc0.
+	 */
+	if (check_succeeded(adfspart_check_ICS(state, bdev), state, &err)) {
+		return state;
+	}
+	if (check_succeeded(adfspart_check_POWERTEC(state, bdev), state, &err)) {
+		return state;
+	}
+	if (check_succeeded(adfspart_check_EESOX(state, bdev), state, &err)) {
+		return state;
+	}
+
+        /*
+	 * Now move on to formats that only have partition info at
+	 * disk address 0xdc0.  Since these may also have stale
+	 * PC/BIOS partition tables, they need to come before
+	 * the msdos entry.
+	 */
+	if (check_succeeded(adfspart_check_CUMANA(state, bdev), state, &err)) {
+		return state;
+	}
+	if (check_succeeded(adfspart_check_ADFS(state, bdev), state, &err)) {
+		return state;
+	}
+
+	/* this must come before msdos */
+	if (check_succeeded(efi_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(sgi_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	/* this must come before msdos */
+	if (check_succeeded(ldm_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(msdos_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(osf_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(sun_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(amiga_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(atari_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(mac_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(ultrix_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(ibm_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	if (check_succeeded(karma_partition(state, bdev), state, &err)) {
+		return state;
+	}
+
+	kfree(state);
+
+	if (err) {
+		if (warn_no_part) {
+			printk(" unable to read partition table\n");
+		}
+	} else {
+		printk(" unknown partition table\n");
+	}
+
+	return ERR_PTR(err);
+}
+
 static struct parsed_partitions *
 check_partition(struct gendisk *hd, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
-	int i, res, err;
 
 	state = kmalloc(sizeof(struct parsed_partitions), GFP_KERNEL);
 	if (!state)
@@ -165,30 +212,8 @@ check_partition(struct gendisk *hd, stru
 		sprintf(state->name, "p");
 
 	state->limit = hd->minors;
-	i = res = err = 0;
-	while (!res && check_part[i]) {
-		memset(&state->parts, 0, sizeof(state->parts));
-		res = check_part[i++](state, bdev);
-		if (res < 0) {
-			/* We have hit an I/O error which we don't report now.
-		 	* But record it, and let the others do their job.
-		 	*/
-			err = res;
-			res = 0;
-		}
 
-	}
-	if (res > 0)
-		return state;
-	if (!err)
-	/* The partition is unrecognized. So report I/O errors if there were any */
-		res = err;
-	if (!res)
-		printk(" unknown partition table\n");
-	else if (warn_no_part)
-		printk(" unable to read partition table\n");
-	kfree(state);
-	return ERR_PTR(res);
+	return do_check_list(state, bdev);
 }
 
 /*

Re: [PATCH 5/5] partitions: Rewrite check_partition to remove necessity of check_part

[Randy Dunlap]

On Sat, 7 Apr 2007 23:42:00 -0400 (EDT) John Anthony Kazos Jr. wrote:

> From: John Anthony Kazos Jr. <jakj@j-a-k-j.com>
> 
> Removes the entire check_part array and uses the presence of new stub 
> functions in header files in fs/partitions to call them directly in a list 
> and let the compiler optimize away any that aren't compiled in. Also fixes 
> a bug where " unable to read partition table" would never be printed.
> 
> Signed-off-by: John Anthony Kazos Jr. <jakj@j-a-k-j.com>
> 
> ---
> 
> I did this because it seems to be much easier to understand. And even 
> though the memory used by the array was only sizeof(void*)*n+1 for the 
> number of partitions configured to be included, that's still unnecessary 
> usage.
> 
> This code also has two warn_unused_result problems, but I don't feel up to 
> the task of fixing those yet.
> 
> Within the old loop, if res < 0, res = 0, and immediately after the loop, 
> the function returns if res > 0. Therefore, it must be that res == 0 after 
> the loop. if (!err) is true only if err == 0 so again, res == 0, so if 
> (!res) is true, and the else condition can never be reached. I 
> re-interpreted the intent to be "log a message if the partition format is 
> unrecognized, and log a message if the partition cannot be read but only 
> if the warning is requested". Judging by the "This is ugly" comment, the 
> warning is not intended to account for actual I/O errors when reading the 
> partition-table block, but rather to detect when the partitions are 
> checked for a device with a medium that has been removed.
> 
> --- linux-2.6.20.6-orig/fs/partitions/check.c	2007-04-06 16:02:48.000000000 -0400
> +++ linux-2.6.20.6-mod/fs/partitions/check.c	2007-04-07 21:50:22.000000000 -0400
> @@ -41,73 +41,6 @@ extern void md_autodetect_dev(dev_t dev)
>  
>  int warn_no_part = 1; /*This is ugly: should make genhd removable media aware*/

[snip]

>  /*
>   * disk_name() is used by partition check code and the genhd driver.
>   * It formats the devicename of the indicated disk into
> @@ -149,11 +82,125 @@ const char *__bdevname(dev_t dev, char *
>  
>  EXPORT_SYMBOL(__bdevname);
>  
> +static int
> +check_succeeded(int res, struct parsed_partitions *state, int *err)
> +{
> +	if (res < 0) {
> +		/* We have hit an I/O error which we don't report now.
> +		 * But record it, and let the others do their job.
> +		 */
> +		*err = res;
> +		res = 0;
> +	}
> +
> +	if (!res) {
> +		memset(state->parts, 0, sizeof(state->parts));
> +	}

No braces on single-statement blocks.  (many of these below also)

> +
> +	return res;
> +}
> +
> +static struct parsed_partitions *
> +do_check_list(struct parsed_partitions *state, struct block_device *bdev)
> +{
> +	int err;
> +
> +	err = 0;
> +
> +        /*

Use tab to indent (above), not (only) spaces.

> +	 * Probe partition formats with tables at disk address 0
> +	 * that also have an ADFS boot block at 0xdc0.
> +	 */
> +	if (check_succeeded(adfspart_check_ICS(state, bdev), state, &err)) {
> +		return state;
> +	}
> +	if (check_succeeded(adfspart_check_POWERTEC(state, bdev), state, &err)) {
> +		return state;
> +	}
> +	if (check_succeeded(adfspart_check_EESOX(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +        /*

tab above

> +	 * Now move on to formats that only have partition info at
> +	 * disk address 0xdc0.  Since these may also have stale
> +	 * PC/BIOS partition tables, they need to come before
> +	 * the msdos entry.
> +	 */
> +	if (check_succeeded(adfspart_check_CUMANA(state, bdev), state, &err)) {
> +		return state;
> +	}
> +	if (check_succeeded(adfspart_check_ADFS(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	/* this must come before msdos */
> +	if (check_succeeded(efi_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(sgi_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	/* this must come before msdos */
> +	if (check_succeeded(ldm_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(msdos_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(osf_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(sun_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(amiga_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(atari_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(mac_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(ultrix_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(ibm_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	if (check_succeeded(karma_partition(state, bdev), state, &err)) {
> +		return state;
> +	}
> +
> +	kfree(state);
> +
> +	if (err) {
> +		if (warn_no_part) {
> +			printk(" unable to read partition table\n");
> +		}
> +	} else {
> +		printk(" unknown partition table\n");
> +	}
> +
> +	return ERR_PTR(err);
> +}


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 0/5] partitions: Changes to fs/partitions for readability and efficiency

[Russell King]

On Sat, Apr 07, 2007 at 11:33:06PM -0400, John Anthony Kazos Jr. wrote:
> In addition to the Kconfig help text patch I submitted earlier, this is a 
> set of patches to touch up the partition handling files and also to change 
> the "array of function pointers" algorithm of the main checking function 
> to "list of calls to possible stub functions" to better fit in with the 
> rest of the kernel code, to reduce memory usage by a few dozen bytes, and 
> to generally be easier (in my opinion) to understand.

I'm not convinced.  Let's dispell the myth that this reduces memory usage.
Two configurations used (see below for their details):

[1]:
   text    data     bss     dec     hex filename
  10868     268       0   11136    2b80 unpatched/fs/partitions/built-in.o
  11260     228       0   11488    2ce0 patched/fs/partitions/built-in.o

[2]:
   7336     248       0    7584    1da0 unpatched/fs/partitions/built-in.o
   7668     228       0    7896    1ed8 patched/fs/partitions/built-in.o


It quite clearly has an average 330-ish byte cost increase rather than a
reduction.  That's quite obvious since instead of interating over data,
we're linearly executing effectively unrolled code which just repeats the
same operations time and time again, the only difference being that you're
calling some other function.

I'm also unconvinced that an array of function pointers is any harder to
read than open coding a list of calls.  In terms of clutter I'd say the
latter was worse.

Finally note that the following fix has been committed to mainline,
which requires the corresponding fix in your patch.  See commit ID
9bebff6ca5871e07b665cdaf71028ea21eb0bf0e for the full info.

-	if (!err)
+       if (err)
        /* The partition is unrecognized. So report I/O errors if there were any */
                res = err;

Configs used:

[1]:
CONFIG_PARTITION_ADVANCED=y
CONFIG_ACORN_PARTITION=y
# CONFIG_ACORN_PARTITION_CUMANA is not set
# CONFIG_ACORN_PARTITION_EESOX is not set
CONFIG_ACORN_PARTITION_ICS=y
CONFIG_ACORN_PARTITION_ADFS=y
CONFIG_ACORN_PARTITION_POWERTEC=y
CONFIG_ACORN_PARTITION_RISCIX=y
CONFIG_OSF_PARTITION=y
CONFIG_AMIGA_PARTITION=y
# CONFIG_ATARI_PARTITION is not set
CONFIG_MAC_PARTITION=y
CONFIG_MSDOS_PARTITION=y
# CONFIG_MINIX_SUBPARTITION is not set
CONFIG_SOLARIS_X86_PARTITION=y
# CONFIG_LDM_PARTITION is not set
CONFIG_SGI_PARTITION=y
# CONFIG_ULTRIX_PARTITION is not set
CONFIG_SUN_PARTITION=y
# CONFIG_KARMA_PARTITION is not set
# CONFIG_EFI_PARTITION is not set

[2]:
CONFIG_PARTITION_ADVANCED=y
CONFIG_ACORN_PARTITION=y
# CONFIG_ACORN_PARTITION_CUMANA is not set
# CONFIG_ACORN_PARTITION_EESOX is not set
CONFIG_ACORN_PARTITION_ICS=y
CONFIG_ACORN_PARTITION_ADFS=y
CONFIG_ACORN_PARTITION_POWERTEC=y
CONFIG_ACORN_PARTITION_RISCIX=y
# CONFIG_OSF_PARTITION is not set
# CONFIG_AMIGA_PARTITION is not set
# CONFIG_ATARI_PARTITION is not set
# CONFIG_MAC_PARTITION is not set
CONFIG_MSDOS_PARTITION=y
# CONFIG_MINIX_SUBPARTITION is not set
# CONFIG_SOLARIS_X86_PARTITION is not set
# CONFIG_LDM_PARTITION is not set
# CONFIG_SGI_PARTITION is not set
# CONFIG_ULTRIX_PARTITION is not set
# CONFIG_SUN_PARTITION is not set
# CONFIG_KARMA_PARTITION is not set
# CONFIG_EFI_PARTITION is not set

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of: