[parted-devel] FYI, preparation for 2048-byte sector sizes across the board

Jim Meyering jim at meyering.net
Fri Jun 1 17:26:03 UTC 2007


This isn't a review request, because it's not ready for commit yet,
but rather a heads-up for what's on the way.

I'm making it so all partition types work with a sector size of 2048-bytes.
Then, things should work with an arbitrary sector size,
as long as it's a multiple of 512 bytes.

Why?  Well, such disks are becoming more and more common, and
the existing code is very brittle.  There are lots of hard-coded
512-byte sector size assumptions (and worse).

So far, the following have had trouble and I've fixed them:

  amiga, bsd, gpt

Next in line is "mac".

Another example of something that will change:
  all of the added read_sector functions will be factored
  out in the final patch.

Since I'm running valgrind as I go, I'm also fixing leaks as
I find them.  Many of the changes below are leak fixes.
They'll end up being in a separate delta, eventually.

diff --git a/libparted/labels/bsd.c b/libparted/labels/bsd.c
index 747a9c5..26a8b60 100644
--- a/libparted/labels/bsd.c
+++ b/libparted/labels/bsd.c
@@ -22,6 +22,7 @@
 
 #include <config.h>
 
+#include <stdbool.h>
 #include <parted/parted.h>
 #include <parted/debug.h>
 #include <parted/endian.h>
@@ -141,30 +142,45 @@ alpha_bootblock_checksum (char *boot) {
 	dp[63] = sum;
 }
 
+/* FIXME: factor out this function: copied from dos.c
+   Read sector, SECTOR_NUM (which has length DEV->sector_size) into malloc'd
+   storage.  If the read fails, free the memory and return zero without
+   modifying *BUF.  Otherwise, set *BUF to the new buffer and return 1.  */
+static int
+read_sector (const PedDevice *dev, PedSector sector_num, char **buf)
+{
+	char *b = ped_malloc (dev->sector_size);
+	PED_ASSERT (b != NULL, return 0);
+	if (!ped_device_read (dev, b, sector_num, 1)) {
+		ped_free (b);
+		return 0;
+	}
+	*buf = b;
+	return 1;
+}
 
 static int
 bsd_probe (const PedDevice *dev)
 {
-	char		boot[512];
-	BSDRawLabel	*label;
+	BSDRawLabel	*partition;
 
 	PED_ASSERT (dev != NULL, return 0);
 
-        if (dev->sector_size != 512)
+        if (dev->sector_size < 512)
                 return 0;
 
-	if (!ped_device_read (dev, boot, 0, 1))
+	char *label;
+	if (!read_sector (dev, 0, &label))
 		return 0;
 
-	label = (BSDRawLabel *) (boot + BSD_LABEL_OFFSET);
+	partition = (BSDRawLabel *) (label + BSD_LABEL_OFFSET);
 
-	alpha_bootblock_checksum(boot);
-	
-	/* check magic */
-	if (PED_LE32_TO_CPU (label->d_magic) != BSD_DISKMAGIC)
-		return 0;
+	alpha_bootblock_checksum(label);
 
-	return 1;
+	/* check magic */
+        bool found = PED_LE32_TO_CPU (partition->d_magic) == BSD_DISKMAGIC;
+	free (label);
+	return found;
 }
 
 static PedDisk*
@@ -248,13 +264,12 @@ bsd_free (PedDisk* disk)
 static int
 bsd_clobber (PedDevice* dev)
 {
-	char		boot [512];
-	BSDRawLabel*	label = (BSDRawLabel *) (boot + BSD_LABEL_OFFSET);
-
-	if (!ped_device_read (dev, boot, 0, 1))
+	char *label;
+	if (!read_sector (dev, 0, &label))
 		return 0;
-	label->d_magic = 0;
-	return ped_device_write (dev, (void*) boot, 0, 1);
+	BSDRawLabel *rawlabel = (BSDRawLabel *) (label + BSD_LABEL_OFFSET);
+	rawlabel->d_magic = 0;
+	return ped_device_write (dev, label, 0, 1);
 }
 #endif /* !DISCOVER_ONLY */
 
@@ -267,8 +282,13 @@ bsd_read (PedDisk* disk)
 	
 	ped_disk_delete_all (disk);
 
-	if (!ped_device_read (disk->dev, bsd_specific->boot_code, 0, 1))
-		goto error;
+	char *s0;
+	if (!read_sector (disk->dev, 0, &s0))
+		return 0;
+
+	memcpy (bsd_specific->boot_code, s0, sizeof (bsd_specific->boot_code));
+	free (s0);
+
 	label = (BSDRawLabel *) (bsd_specific->boot_code + BSD_LABEL_OFFSET);
 
 	for (i = 1; i <= BSD_MAXPARTITIONS; i++) {
diff --git a/libparted/labels/gpt.c b/libparted/labels/gpt.c
index 86c31c7..0b874b1 100644
--- a/libparted/labels/gpt.c
+++ b/libparted/labels/gpt.c
@@ -254,6 +254,23 @@ typedef struct _GPTPartitionData {
 static PedDiskType gpt_disk_type;
 
 
+/* FIXME: factor out this function: copied from dos.c
+   Read sector, SECTOR_NUM (which has length DEV->sector_size) into malloc'd
+   storage.  If the read fails, free the memory and return zero without
+   modifying *BUF.  Otherwise, set *BUF to the new buffer and return 1.  */
+static int
+read_sector (const PedDevice *dev, PedSector sector_num, char **buf)
+{
+	char *b = ped_malloc (dev->sector_size);
+	PED_ASSERT (b != NULL, return 0);
+	if (!ped_device_read (dev, b, sector_num, 1)) {
+		ped_free (b);
+		return 0;
+	}
+	*buf = b;
+	return 1;
+}
+
 static inline uint32_t
 pth_get_size (const PedDevice* dev)
 {
@@ -425,7 +442,6 @@ gpt_probe (const PedDevice * dev)
 {
 	GuidPartitionTableHeader_t* gpt = NULL;
         uint8_t* pth_raw = ped_malloc (pth_get_size (dev));
-	LegacyMBR_t legacy_mbr;
 	int gpt_sig_found = 0;
 
 	PED_ASSERT (dev != NULL, return 0);
@@ -446,26 +462,31 @@ gpt_probe (const PedDevice * dev)
 	if (!gpt_sig_found)
 		return 0;
 
-	if (ped_device_read(dev, &legacy_mbr, 0, GPT_HEADER_SECTORS)) {
-		if (!_pmbr_is_valid (&legacy_mbr)) {
-			int ex_status = ped_exception_throw (
-				PED_EXCEPTION_WARNING,
-				PED_EXCEPTION_YES_NO,
-			_("%s contains GPT signatures, indicating that it has "
-			  "a GPT table.  However, it does not have a valid "
-			  "fake msdos partition table, as it should.  Perhaps "
-			  "it was corrupted -- possibly by a program that "
-			  "doesn't understand GPT partition tables.  Or "
-			  "perhaps you deleted the GPT table, and are now "
-			  "using an msdos partition table.  Is this a GPT "
-			  "partition table?"),
-				dev->path);
-			if (ex_status == PED_EXCEPTION_NO)
-				return 0;
-		}
+
+	char *label;
+	if (!read_sector (dev, 0, &label))
+		return 0;
+
+        int ok = 1;
+	if (!_pmbr_is_valid (label)) {
+		int ex_status = ped_exception_throw (
+			PED_EXCEPTION_WARNING,
+			PED_EXCEPTION_YES_NO,
+		_("%s contains GPT signatures, indicating that it has "
+		  "a GPT table.  However, it does not have a valid "
+		  "fake msdos partition table, as it should.  Perhaps "
+		  "it was corrupted -- possibly by a program that "
+		  "doesn't understand GPT partition tables.  Or "
+		  "perhaps you deleted the GPT table, and are now "
+		  "using an msdos partition table.  Is this a GPT "
+		  "partition table?"),
+			dev->path);
+		if (ex_status == PED_EXCEPTION_NO)
+			ok = 0;
 	}
 
-	return 1;
+        ped_free (label);
+	return ok;
 }
 
 #ifndef DISCOVER_ONLY
@@ -828,8 +849,10 @@ gpt_read (PedDisk * disk)
 		  "should be.  This might mean that another operating system "
 		  "believes the disk is smaller.  Fix, by moving the backup "
 		  "to the end (and removing the old backup)?"))
-					== PED_EXCEPTION_CANCEL)
+				== PED_EXCEPTION_CANCEL) {
+				free (zeros);
 				goto error_free_gpt;
+                        }
 
 			write_back = 1;
 			memset (zeros, 0, disk->dev->sector_size);
@@ -837,6 +860,7 @@ gpt_read (PedDisk * disk)
 					  PED_LE64_TO_CPU (gpt->AlternateLBA),
 					  1);
 #endif /* !DISCOVER_ONLY */
+			free (zeros);
 		}
 	} else { /* primary GPT *not* ok */
 		int alternate_ok = 0;
@@ -914,6 +938,7 @@ gpt_read (PedDisk * disk)
 		ped_disk_commit_to_dev (disk);
 #endif
 
+        pth_free (gpt);
 	return 1;
 
 error_delete_all:
@@ -931,22 +956,26 @@ error:
 static int
 _write_pmbr (PedDevice * dev)
 {
-	LegacyMBR_t pmbr;
-
-	memset(&pmbr, 0, sizeof(pmbr));
-	pmbr.Signature = PED_CPU_TO_LE16(MSDOS_MBR_SIGNATURE);
-	pmbr.PartitionRecord[0].OSType      = EFI_PMBR_OSTYPE_EFI;
-	pmbr.PartitionRecord[0].StartSector = 1;
-	pmbr.PartitionRecord[0].EndHead     = 0xFE;
-	pmbr.PartitionRecord[0].EndSector   = 0xFF;
-	pmbr.PartitionRecord[0].EndTrack    = 0xFF;
-	pmbr.PartitionRecord[0].StartingLBA = PED_CPU_TO_LE32(1);
+	size_t buf_len = pth_get_size (dev);
+	LegacyMBR_t *pmbr = ped_malloc (buf_len);
+
+	memset(pmbr, 0, buf_len);
+	pmbr->Signature = PED_CPU_TO_LE16(MSDOS_MBR_SIGNATURE);
+	pmbr->PartitionRecord[0].OSType      = EFI_PMBR_OSTYPE_EFI;
+	pmbr->PartitionRecord[0].StartSector = 1;
+	pmbr->PartitionRecord[0].EndHead     = 0xFE;
+	pmbr->PartitionRecord[0].EndSector   = 0xFF;
+	pmbr->PartitionRecord[0].EndTrack    = 0xFF;
+	pmbr->PartitionRecord[0].StartingLBA = PED_CPU_TO_LE32(1);
 	if ((dev->length - 1ULL) > 0xFFFFFFFFULL) 
-		pmbr.PartitionRecord[0].SizeInLBA = PED_CPU_TO_LE32(0xFFFFFFFF);
+		pmbr->PartitionRecord[0].SizeInLBA = PED_CPU_TO_LE32(0xFFFFFFFF);
 	else
-		pmbr.PartitionRecord[0].SizeInLBA = PED_CPU_TO_LE32(dev->length - 1UL);
+		pmbr->PartitionRecord[0].SizeInLBA = PED_CPU_TO_LE32(dev->length - 1UL);
 
-	return ped_device_write (dev, &pmbr, GPT_PMBR_LBA, GPT_PMBR_SECTORS);
+        int write_ok = ped_device_write (dev, pmbr, GPT_PMBR_LBA,
+                                         GPT_PMBR_SECTORS);
+        ped_free (pmbr);
+	return write_ok;
 }
 
 static void
@@ -1022,7 +1051,7 @@ gpt_write(const PedDisk * disk)
 	GPTDiskData* gpt_disk_data;
 	GuidPartitionEntry_t* ptes;
 	uint32_t ptes_crc;
-        uint8_t* pth_raw = ped_malloc (pth_get_size (disk->dev));
+        uint8_t* pth_raw;
 	GuidPartitionTableHeader_t* gpt;
 	PedPartition* part;
 	int ptes_size;
@@ -1054,16 +1083,20 @@ gpt_write(const PedDisk * disk)
 	/* Write PTH and PTEs */
 	_generate_header (disk, 0, ptes_crc, &gpt);
         pth_raw = pth_get_raw (disk->dev, gpt);
+        pth_free (gpt);
 	if (!ped_device_write (disk->dev, pth_raw, 1, 1))
-		goto error_free_ptes;
+		goto error_free_pth_raw;
+        ped_free (pth_raw);
 	if (!ped_device_write (disk->dev, ptes, 2, ptes_size / disk->dev->sector_size))
 		goto error_free_ptes;
 
 	/* Write Alternate PTH & PTEs */
 	_generate_header (disk, 1, ptes_crc, &gpt);
         pth_raw = pth_get_raw (disk->dev, gpt);
+        pth_free (gpt);
 	if (!ped_device_write (disk->dev, pth_raw, disk->dev->length - 1, 1))
-		goto error_free_ptes;
+		goto error_free_pth_raw;
+        ped_free (pth_raw);
 	if (!ped_device_write (disk->dev, ptes,
 			       disk->dev->length - 1 - ptes_size / disk->dev->sector_size,
 			       ptes_size / disk->dev->sector_size))
@@ -1072,6 +1105,8 @@ gpt_write(const PedDisk * disk)
 	ped_free (ptes);
 	return ped_device_sync (disk->dev);
 
+error_free_pth_raw:
+	ped_free (pth_raw);
 error_free_ptes:
 	ped_free (ptes);
 error:
diff --git a/libparted/labels/rdb.c b/libparted/labels/rdb.c
index 483a292..c521416 100644
--- a/libparted/labels/rdb.c
+++ b/libparted/labels/rdb.c
@@ -349,7 +349,7 @@ amiga_alloc (const PedDevice* dev)
 	if (!(disk = _ped_disk_alloc (dev, &amiga_disk_type)))
 		return NULL;
 
-	if (!(disk->disk_specific = ped_malloc (PED_SECTOR_SIZE_DEFAULT))) {
+	if (!(disk->disk_specific = ped_malloc (disk->dev->sector_size))) {
 		ped_free (disk);
 		return NULL;
 	}
@@ -360,7 +360,7 @@ amiga_alloc (const PedDevice* dev)
 	rdb->rdb_ID = PED_CPU_TO_BE32 (IDNAME_RIGIDDISK);
 	rdb->rdb_SummedLongs = PED_CPU_TO_BE32 (64);
 	rdb->rdb_HostID = PED_CPU_TO_BE32 (0);
-	rdb->rdb_BlockBytes = PED_CPU_TO_BE32 (PED_SECTOR_SIZE_DEFAULT);
+	rdb->rdb_BlockBytes = PED_CPU_TO_BE32 (disk->dev->sector_size);
 	rdb->rdb_Flags = PED_CPU_TO_BE32 (0);
 
 	/* Block lists */
@@ -446,7 +446,7 @@ amiga_clobber (PedDevice* dev)
 	int result = 0;
 	PED_ASSERT(dev != NULL, return 0);
 
-	if ((rdb=RDSK(ped_malloc(PED_SECTOR_SIZE_DEFAULT)))==NULL)
+	if ((rdb=RDSK(ped_malloc(dev->sector_size)))==NULL)
 		return 0;
 
 	while ((i = _amiga_find_rdb (dev, rdb)) != AMIGA_RDB_NOT_FOUND) {
@@ -650,14 +650,14 @@ amiga_write (const PedDisk* disk)
 	PED_ASSERT (disk->dev != NULL, return 0);
 	PED_ASSERT (disk->disk_specific != NULL, return 0);
 
-	if (!(rdb = ped_malloc (PED_SECTOR_SIZE_DEFAULT)))
+	if (!(rdb = ped_malloc (disk->dev->sector_size)))
 		return 0;
 
 	/* Let's read the rdb */
 	if ((rdb_num = _amiga_find_rdb (disk->dev, rdb)) == AMIGA_RDB_NOT_FOUND) {
 		rdb_num = 2;
 	} else {
-		memcpy (RDSK(disk->disk_specific), rdb, PED_SECTOR_SIZE_DEFAULT);
+		memcpy (RDSK(disk->disk_specific), rdb, disk->dev->sector_size);
 	}
 	ped_free (rdb);
 	rdb = RDSK(disk->disk_specific);
@@ -676,7 +676,7 @@ amiga_write (const PedDisk* disk)
 	for (i = 0; i<=rdb_num; i++) table[i] = IDNAME_RIGIDDISK;
 	
 	/* Let's allocate a partition block */
-	if (!(block = ped_malloc (PED_SECTOR_SIZE_DEFAULT))) {
+	if (!(block = ped_malloc (disk->dev->sector_size))) {
 		ped_free (table);
 		return 0;
 	}
@@ -790,7 +790,7 @@ amiga_partition_new (const PedDisk* disk, PedPartitionType part_type,
 		return NULL;
 
 	if (ped_partition_is_active (part)) {
-		if (!(part->disk_specific = ped_malloc (PED_SECTOR_SIZE_DEFAULT))) {
+		if (!(part->disk_specific = ped_malloc (disk->dev->sector_size))) {
 			ped_free (part);
 			return NULL;
 		}



More information about the parted-devel mailing list