Re: [PATCH 3/5] fdisk: gpt: create empty disklabels

[Petr Uzel <petr.uzel@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 7444 bytes --]

On Sun, Oct 07, 2012 at 04:33:53PM +0200, Davidlohr Bueso wrote:
> This patch enables creating a new, empty, GPT disklabel from either
> an empty disk or one that already has a disklabel. For this
> purpose, a 'g' option is added to the main menu and is visible to all
> labels. Here's an example for a scsi_debug device (/dev/sdb):
> 
[SNIP]
> ---
>  fdisks/fdisk.c |    4 ++
>  fdisks/gpt.c   |  211 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 204 insertions(+), 11 deletions(-)
> 
> diff --git a/fdisks/fdisk.c b/fdisks/fdisk.c
> index 1295d54..612bf87 100644
> --- a/fdisks/fdisk.c
> +++ b/fdisks/fdisk.c
> @@ -90,6 +90,7 @@ static const struct menulist_descr menulist[] = {
>  	{'e', N_("edit drive data"), {OSF_LABEL, 0}},
>  	{'f', N_("fix partition order"), {0, DOS_LABEL}},
>  	{'g', N_("create an IRIX (SGI) partition table"), {0, ANY_LABEL}},
> +	{'g', N_("create a new empty GPT partition table"), {ANY_LABEL, 0}},
>  	{'h', N_("change number of heads"), {0, DOS_LABEL | SUN_LABEL}},
>  	{'i', N_("change interleave factor"), {0, SUN_LABEL}},
>  	{'i', N_("change the disk identifier"), {0, DOS_LABEL}},
> @@ -1693,6 +1694,9 @@ static void command_prompt(struct fdisk_context *cxt)
>  		case 'd':
>  			delete_partition(cxt, get_existing_partition(cxt, 1, partitions));
>  			break;
> +		case 'g':
> +			fdisk_create_disklabel(cxt, "gpt");
> +			break;
>  		case 'i':
>  			if (disklabel == SGI_LABEL)
>  				create_sgiinfo(cxt);
[SNIP]
> +
> +/* some universal differences between the headers */
> +static void gpt_mknew_header_common(struct fdisk_context *cxt,
> +				    struct gpt_header *header, uint64_t lba)
> +{
> +	if (!cxt || !header)
> +		return;
> +
> +	header->my_lba = cpu_to_le32(lba);

This should be cpu_to_le64(lba);

> +
> +	if (lba == GPT_PRIMARY_PARTITION_TABLE_LBA) { /* primary */
> +		header->alternative_lba = cpu_to_le64(cxt->total_sectors - 1);
> +		header->partition_entry_lba = cpu_to_le64(2);
> +	} else { /* backup */
> +		header->alternative_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA);
> +		header->partition_entry_lba = header->last_usable_lba + cpu_to_le64(1);

Shouldn't this ^^^ rather be:

		header->partition_entry_lba = cpu_to_le64(header->last_usable_lba + 1);

?

Even with this modification, I'm still not sure if the computation is
correct. Are you sure that partition_entry_lba (in alternative header)
should be set to this value?

> +	}
> +}
> +
> +/*
> + * Builds a new GPT header (at sector lba) from a backup header2.
> + * If building a primary header, then backup is the secondary, and vice versa.
> + *
> + * Always pass a new (zeroized) header to build upon as we don't
> + * explicitly zero-set some values such as CRCs and reserved.

Why is this? Why not zeroize the to-be-built header in this function?

> + *
> + * Returns 0 on success, otherwise < 0 on error.
> + */
> +static int gpt_mknew_header_from_bkp(struct fdisk_context *cxt,
> +				     struct gpt_header *header,
> +				     uint64_t lba,
> +				     struct gpt_header *header2)
> +{
> +	if (!cxt || !header || !header2)
> +		return -ENOSYS;

Perhaps -EINVAL would be better if the arguments passed to the
function are invalid?

> +
> +	header->signature              = header2->signature;
> +	header->revision               = header2->revision;
> +	header->size                   = header2->size;
> +	header->npartition_entries     = header2->npartition_entries;
> +	header->sizeof_partition_entry = header2->sizeof_partition_entry;
> +	header->first_usable_lba       = header2->first_usable_lba;
> +	header->last_usable_lba        = header2->last_usable_lba;
> +
> +	memcpy(&header->disk_guid,
> +	       &header2->disk_guid, sizeof(header2->disk_guid));
> +	gpt_mknew_header_common(cxt, header, lba);
> +
> +	return 0;
> +}
> +
> +/*
> + * Builds a clean new GPT header (currently under revision 1.0).
> + *
> + * Always pass a new (zeroized) header to build upon as we don't
> + * explicitly zero-set some values such as CRCs and reserved.

Again; Why?

> + *
> + * Returns 0 on success, otherwise < 0 on error.
> + */
> +static int gpt_mknew_header(struct fdisk_context *cxt,
> +			    struct gpt_header *header, uint64_t lba)
> +{
> +	if (!cxt || !header)
> +		return -ENOSYS;

-EINVAL ?

> +
> +	gpt_mknew_header_common(cxt, header, lba);
> +
> +	header->signature = cpu_to_le64(GPT_HEADER_SIGNATURE);
> +	header->revision  = cpu_to_le32(GPT_HEADER_REVISION_V1_00);
> +	header->size      = cpu_to_le32(sizeof(struct gpt_header));
> +
> +	/*
> +	 * 128 partitions is the default. It can go behond this, however,
> +	 * we're creating a de facto header here, so no funny business.
> +	 */
> +	header->npartition_entries     = cpu_to_le32(128);
> +	header->sizeof_partition_entry = cpu_to_le32(sizeof(struct gpt_entry));
> +
> +	header->first_usable_lba =
> +		(header->sizeof_partition_entry * header->npartition_entries /
> +		 cpu_to_le64(cxt->sector_size)) + cpu_to_le64(2);
> +	header->last_usable_lba =
> +		cpu_to_le64(cxt->total_sectors) -  header->first_usable_lba;
> +
> +	uuid_generate_random((unsigned char *) &header->disk_guid);
> +	swap_efi_guid(&header->disk_guid);
> +
> +	return 0;
> +}
> +
> +/*
>   * Checks if there is a valid protective MBR partition table.
>   * Returns 0 if it is invalid or failure. Otherwise, return
>   * GPT_MBR_PROTECTIVE or GPT_MBR_HYBRID, depeding on the detection.
> @@ -849,6 +966,22 @@ static void gpt_init(void)
>  	partitions = le32_to_cpu(pheader->npartition_entries);
>  }
>  
> +/*
> + * Deinitialize fdisk-specific variables
> + */
> +static void gpt_deinit(void)
> +{
> +	free(ents);
> +	free(pheader);
> +	free(bheader);
> +	ents = NULL;
> +	pheader = NULL;
> +	bheader = NULL;
> +
> +	disklabel = ANY_LABEL;
> +	partitions = 0;
> +}
> +
>  static int gpt_probe_label(struct fdisk_context *cxt)
>  {
>  	int mbr_type;
> @@ -1005,7 +1138,7 @@ fail:
>   * Returns 0 on success, or corresponding error otherwise.
>   */
>  static int gpt_write_header(struct fdisk_context *cxt,
> -			    struct gpt_header *header, uint64_t lba)
> +		     struct gpt_header *header, uint64_t lba)
>  {
>  	off_t offset = lba * cxt->sector_size;
>  
> @@ -1056,8 +1189,10 @@ static int gpt_write_pmbr(struct fdisk_context *cxt)
>  	offset = GPT_PMBR_LBA * cxt->sector_size;
>  	if (offset != lseek(cxt->dev_fd, offset, SEEK_SET))
>  		goto fail;
> -
> -	if (1 == write(cxt->dev_fd, pmbr, 1))
> +	
> +	/* pMBR covers the first sector (LBA) of the disk */
> +	if (cxt->sector_size == 
> +	    (unsigned long) write(cxt->dev_fd, pmbr, cxt->sector_size))
>  		return 0;

Why not use write_all() from all-io.h ?

>  fail:
>  	return -errno;
> @@ -1318,7 +1453,7 @@ static int gpt_create_new_partition(int partnum, uint64_t fsect, uint64_t lsect,
>  
>  /* Performs logical checks to add a new partition entry */
>  static void gpt_add_partition(struct fdisk_context *cxt, int partnum,
> -			     struct fdisk_parttype *t)
> +			      struct fdisk_parttype *t)
>  {
>  	char msg[256];
>  	uint32_t tmp;
> @@ -1370,6 +1505,60 @@ static void gpt_add_partition(struct fdisk_context *cxt, int partnum,
>  		printf(_("Created partition %d\n"), partnum + 1);
>  }
>  
[SNIP]

Petr

-- 
Petr Uzel
IRC: ptr_uzl @ freenode

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]