[Libguestfs] [PATCH VERSION 4] Generic partition creation interface.

[Jim Meyering]

Richard W.M. Jones wrote:
> On Thu, Nov 05, 2009 at 01:15:03PM +0000, Richard W.M. Jones wrote:
>> After a bit of discussion with Jim Meyering I've decided to withdraw
>> this and rewrite it based on libparted.  Using the parted command line
>> tool is just too unpredictable.
>
> After going down this rathole I decided maybe that wasn't such a
> good plan after all.
>
> The following patch goes back to using the command line interface, but
> we have made guestfs_part_disk work for both MBR and GPT formats
> (thanks to Jim).
>
> Most of the rest of the patch is concerned with updating documentation
> and examples to emphasize using guestfs_part* instead of
> guestfs_sfdisk* calls.
>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> virt-df lists disk usage of guests without needing to install any
> software inside the virtual machine.  Supports Linux and Windows.
> http://et.redhat.com/~rjones/virt-df/
>
>>From 74914b6fd255d849c8d5caf482e9c727f0a13afd Mon Sep 17 00:00:00 2001
> From: Richard Jones <rjones at redhat.com>
> Date: Wed, 4 Nov 2009 23:15:26 +0000
> Subject: [PATCH] Generic partition creation interface.
>
> The current sfdisk interface works, but is somewhat limited
> since it can only deal with MBR-style partitions.  For disks
> larger than 2TB, MBR partitions are unsuitable and we need
> to provide access to other partition table types (eg. GPT).

Hi Rich,

This all looks fine.
Haven't tested yet, though.
Two comments:

...
> +int
> +do_part_add (const char *device, const char *prlogex,
> +             int64_t startsect, int64_t endsect)
> +{
> +  char *err;
> +  int r;
> +  char startstr[32];
> +  char endstr[32];
> +
> +  /* Check and translate prlogex. */
> +  if (strcmp (prlogex, "primary") == 0 ||
> +      strcmp (prlogex, "logical") == 0 ||
> +      strcmp (prlogex, "extended") == 0)
> +    ;
> +  else if (strcmp (prlogex, "p") == 0)
> +    prlogex = "primary";
> +  else if (strcmp (prlogex, "l") == 0)
> +    prlogex = "logical";
> +  else if (strcmp (prlogex, "e") == 0)
> +    prlogex = "extended";
> +  else {
> +    reply_with_error ("part-add: unknown partition type: %s: this should be \"primary\", \"logical\" or \"extended\"", prlogex);
> +    return -1;

Did you consider enforcing this only for msdos partition tables
and allowing arbitrary names for GPT?

...
> +int
> +do_part_disk (const char *device, const char *parttype)
> +{
> +  char *err;
> +  int r;
> +  const char *startstr;
> +  const char *endstr;
> +
> +  parttype = check_parttype (parttype);
> +  if (!parttype) {
> +    reply_with_error ("part-disk: unknown partition type: common choices are \"gpt\" and \"msdos\"");
> +    return -1;
> +  }
> +
> +  /* Voooooodooooooooo (thanks Jim Meyering for working this out). */
> +  if (strcmp (parttype, "msdos") == 0) {
> +    startstr = "1s";
> +    endstr = "-1s";
> +  } else if (strcmp (parttype, "gpt") == 0) {
> +    startstr = "34s";
> +    endstr = "-34s";
> +  } else {
> +    /* untested */
> +    startstr = "1s";
> +    endstr = "-1s";
> +  }
> +
> +  r = command (NULL, &err,
> +               "/sbin/parted", "-s", "--", device,
> +               "mklabel", parttype,
> +               /* This slightly wrong - in the GPT case it will give
> +                * the partition the name "primary".  We have to give
> +                * it some name, so this is as good as any.
> +                */
> +               "mkpart", "primary", startstr, endstr,

I created many GPT partition tables with "primary" as the name
of each partition, before I noticed the error.
To avoid giving the impression that libguestfs is doing
this out of ignorance, how about using e.g.,

      strcmp (parttype, "gpt") == 0 ? "p1" : "primary"

...