[parted-devel] map gpt partitions to msdos primary entries

[Jim Meyering]

Olaf Hering <olh at suse.de> wrote:
> This patch adds a new 'set X map_gpt_to_msdos Y' function.
> Using it will break the EFI spec (I cant verify it because the EFI spec
> is not free).

Thanks for doing this work.
Here are some simple changes you can make to improve the patch:

> + at item map_gpt_to_msdos
> +(GPT) - this flag can be used to establish a mapping between a gpt partition
> +entry and an entry for a primary partition in the msdos partition table.

Please mention the syntax (give an example like you did in your email)
and mention the valid range of values: [2..4]

> +/* Generates the protective MBR (to keep DOS happy), during mklabel gpt */
> +static void
> +gpt_create_pmbr (LegacyMBR_t *pmbr, const PedDevice *dev)
> +{
> +	memset(pmbr, 0, sizeof(LegacyMBR_t));
> +	pmbr->Signature = PED_CPU_TO_LE16(MSDOS_MBR_SIGNATURE);
> +	pmbr->PartitionRecord[0].OSType      = EFI_PMBR_OSTYPE_EFI;
> +	pmbr->PartitionRecord[0].StartingLBA = PED_CPU_TO_LE32(1);
> +	if ((dev->length - 1ULL) > 0xFFFFFFFFULL)
> +		pmbr->PartitionRecord[0].SizeInLBA = PED_CPU_TO_LE32(0xFFFFFFFF);
> +	else
> +		pmbr->PartitionRecord[0].SizeInLBA = PED_CPU_TO_LE32(dev->length - 1UL);
> +}
> +
>  static PedDisk *
>  gpt_alloc (const PedDevice * dev)
>  {
> @@ -545,6 +562,9 @@ gpt_alloc (const PedDevice * dev)
>  	gpt_disk_data->entry_count = GPT_DEFAULT_PARTITION_ENTRIES;
>  	uuid_generate ((unsigned char*) &gpt_disk_data->uuid);
>  	swap_uuid_and_efi_guid((unsigned char*)(&gpt_disk_data->uuid));
> +	memset(&gpt_disk_data->gpt_to_msdos_map[0], 0, sizeof(gpt_disk_data->gpt_to_msdos_map));

Please wrap the above line.  Not only does it keep the line
length below the max of 80, but it's easier to see that the first
and 3rd arguments are the same.  BTW, &array[0] is the same as "array",
so using the latter form is preferred: less distracting syntax:

        memset(gpt_disk_data->gpt_to_msdos_map, 0,
               sizeof(gpt_disk_data->gpt_to_msdos_map));

> +static void
> +gpt_find_and_map_msdos_entry(PedPartition *part, GPTDiskData *gpt_disk_data)

It looks like the "part" parameter may (and hence should) be declared "const",

  gpt_find_and_map_msdos_entry(const PedPartition *part,
                               GPTDiskData *gpt_disk_data)

> +{
> +	LegacyMBR_t *mbr = &gpt_disk_data->mbr;
> +	uint64_t start = (part->geom.dev->sector_size / 512) * part->geom.start;
> +	uint64_t end = (part->geom.dev->sector_size / 512) * part->geom.end;
> +	unsigned short i;
> +
> +	/* msdos partitions can not go beyond 2TB */
> +	if (end & 0xffffffff00000000ULL)
> +		return;
> +	for (i = 0; i < 4; i++) {
> +		uint32_t msdos_s, msdos_e;
> +		if (!mbr->PartitionRecord[i].OSType)
> +			continue;
> +		msdos_s = PED_LE32_TO_CPU(mbr->PartitionRecord[i].StartingLBA);
> +		msdos_e = msdos_s + PED_LE32_TO_CPU(mbr->PartitionRecord[i].SizeInLBA);

Wrap long lines.

> +		if (start == msdos_s && end == msdos_e) {
> +			if ((i + 1) == gpt_disk_data->msdos_efi)
> +				fprintf(stderr, "%s:%s(%u) %d: is EFI partition\n", __FILE__,__func__,__LINE__,i+1);
> +			else if (gpt_disk_data->gpt_to_msdos_map[i])
> +				fprintf(stderr, "%s:%s(%u) %d: already mapped to gpt #%u\n", __FILE__,__func__,__LINE__,i+1,gpt_disk_data->gpt_to_msdos_map[i]);

Don't use fprintf.  (library code must not write to stdout/stderr)
Instead, consider the compromise of calling ped_exception_throw.
I hate to recommend using ped_exception_throw, but at least this
code will then be consistent with the rest.

...
> -	if (!gpt_probe (disk->dev))
> +	if (!ped_device_read (disk->dev, &gpt_disk_data->mbr, GPT_PMBR_LBA, GPT_PMBR_SECTORS))

Another long line.

> +	case PED_PARTITION_MAP_GPT_TO_MSDOS:
> +		if (!command_line_get_integer (_("target msdos partition (2 or 3 or 4)?"), &state))
> +			goto error_destroy_disk;

I suppose this is where you'd do the [2..4] range checking.