Libguestfs - Jan 2018 - [PATCH v2 1/3] daemon: make sgdisk_info_extract_uuid_field more generic

Hi all,

Here is the latest version of the series addressing Pino's comments.

Cédric Bosdonnat (3):
  daemon: make sgdisk_info_extract_uuid_field more generic
  New APIs: part_set_gpt_attributes and part_get_gpt_attributes
  resize: copy GPT partition flags

 daemon/parted.ml          | 45 +++++++++++++++++++++++++++++++++++----------
 daemon/parted.mli         |  3 +++
 generator/actions_core.ml | 37 +++++++++++++++++++++++++++++++++++++
 generator/proc_nr.ml      |  2 ++
 lib/MAX_PROC_NR           |  2 +-
 resize/resize.ml          | 15 ++++++++++++---
 6 files changed, 90 insertions(+), 14 deletions(-)

-- 
2.15.1

Rename the sgdisk_info_extract_uuid_field to
sgdisk_info_extract_field in order to reuse it for other field types.
Just like its C ancestor, it now needs an extractor function to be
passed as parameter.
---
 daemon/parted.ml | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/daemon/parted.ml b/daemon/parted.ml
index d6638867a..6fe803613 100644
--- a/daemon/parted.ml
+++ b/daemon/parted.ml
@@ -124,12 +124,11 @@ let part_get_parttype device    | _ ->
      failwithf "%s: cannot parse the output of parted" device
 
-let rec part_get_gpt_type device partnum -  sgdisk_info_extract_uuid_field
device partnum "Partition GUID code"
-and part_get_gpt_guid device partnum -  sgdisk_info_extract_uuid_field device
partnum "Partition unique GUID"
+let extract_guid value +  (* The value contains only valid GUID characters. *)
+  String.sub value 0 (String.span value "-0123456789ABCDEF")
 
-and sgdisk_info_extract_uuid_field device partnum field +let
sgdisk_info_extract_field device partnum field extractor    if partnum <= 0
then failwith "partition number must be >= 1";
 
   udev_settle ();
@@ -167,13 +166,16 @@ and sgdisk_info_extract_uuid_field device partnum field   
(* Skip any whitespace after the colon. *)
        let value = String.triml value in
 
-       (* Extract the UUID. *)
-       extract_uuid value
+       (* Extract the value. *)
+       extractor value
 
     | _ :: lines -> loop lines
   in
   loop lines
 
-and extract_uuid value -  (* The value contains only valid GUID characters. *)
-  String.sub value 0 (String.span value "-0123456789ABCDEF")
+let rec part_get_gpt_type device partnum +  sgdisk_info_extract_field device
partnum "Partition GUID code"
+                            extract_guid
+and part_get_gpt_guid device partnum +  sgdisk_info_extract_field device
partnum "Partition unique GUID"
+                            extract_guid
-- 
2.15.1

Allow reading and setting the GPT partition attribute flags.
---
 daemon/parted.ml          | 23 +++++++++++++++++++++++
 daemon/parted.mli         |  3 +++
 generator/actions_core.ml | 37 +++++++++++++++++++++++++++++++++++++
 generator/proc_nr.ml      |  2 ++
 lib/MAX_PROC_NR           |  2 +-
 5 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/daemon/parted.ml b/daemon/parted.ml
index 6fe803613..e3ab823bd 100644
--- a/daemon/parted.ml
+++ b/daemon/parted.ml
@@ -124,10 +124,30 @@ let part_get_parttype device    | _ ->
      failwithf "%s: cannot parse the output of parted" device
 
+let part_set_gpt_attributes device partnum attributes +  if partnum <= 0
then failwith "partition number must be >= 1";
+
+  udev_settle ();
+
+  let hex = Printf.sprintf "%LX" attributes in
+  let arg = string_of_int partnum ^ ":=:" ^ hex in
+  let r, _, err +    commandr ~fold_stdout_on_stderr:true
+             "sgdisk" [ device; "-A"; arg ] in
+  if r <> 0 then
+    failwithf "sgdisk: %s" err;
+
+  udev_settle ()
+
 let extract_guid value    (* The value contains only valid GUID characters. *)
   String.sub value 0 (String.span value "-0123456789ABCDEF")
 
+let extract_hex value +  (* The value contains only valid numeric characters.
*)
+  let str = String.sub value 0 (String.span value "0123456789ABCDEF")
in
+  Int64.of_string ("0x" ^ str)
+
 let sgdisk_info_extract_field device partnum field extractor    if partnum
<= 0 then failwith "partition number must be >= 1";
 
@@ -179,3 +199,6 @@ let rec part_get_gpt_type device partnum  and
part_get_gpt_guid device partnum    sgdisk_info_extract_field device partnum
"Partition unique GUID"
                             extract_guid
+and part_get_gpt_attributes device partnum +  sgdisk_info_extract_field device
partnum "Attribute flags"
+                            extract_hex
diff --git a/daemon/parted.mli b/daemon/parted.mli
index cbcb7b503..9f57bbac7 100644
--- a/daemon/parted.mli
+++ b/daemon/parted.mli
@@ -30,3 +30,6 @@ val part_get_parttype : string -> string
 
 val part_get_gpt_type : string -> int -> string
 val part_get_gpt_guid : string -> int -> string
+val part_get_gpt_attributes : string -> int -> int64
+
+val part_set_gpt_attributes : string -> int -> int64 -> unit
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 02759a6b7..78eee61dd 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -8265,6 +8265,43 @@ Return the type GUID of numbered GPT partition
C<partnum>. For MBR partitions,
 return an appropriate GUID corresponding to the MBR type. Behaviour is
undefined
 for other partition types." };
 
+  { defaults with
+    name = "part_set_gpt_attributes"; added = (1, 21, 1);
+    style = RErr, [String (Device, "device"); Int
"partnum"; Int64 "attributes"], [];
+    impl = OCaml "Parted.part_set_gpt_attributes";
+    optional = Some "gdisk";
+    tests = [
+      InitGPT, Always, TestResult (
+        [["part_set_gpt_attributes"; "/dev/sda";
"1";
+          "4"];
+         ["part_get_gpt_attributes"; "/dev/sda";
"1"]],
+        "4"), [];
+    ];
+    shortdesc = "set the attribute flags of a GPT partition";
+    longdesc = "\
+Set the attribute flags of numbered GPT partition C<partnum> to
C<attributes>. Return an
+error if the partition table of C<device> isn't GPT.
+
+See
L<https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries>
+for a useful list of partition attributes." };
+
+  { defaults with
+    name = "part_get_gpt_attributes"; added = (1, 21, 1);
+    style = RInt64 "attributes", [String (Device,
"device"); Int "partnum"], [];
+    impl = OCaml "Parted.part_get_gpt_attributes";
+    optional = Some "gdisk";
+    tests = [
+      InitGPT, Always, TestResult (
+        [["part_set_gpt_attributes"; "/dev/sda";
"1";
+          "0"];
+         ["part_get_gpt_attributes"; "/dev/sda";
"1"]],
+        "4"), [];
+    ];
+    shortdesc = "get the attribute flags of a GPT partition";
+    longdesc = "\
+Return the attribute flags of numbered GPT partition C<partnum> as a
hexadecimal bit mask.
+For MBR partitions, return all flags set to 0." };
+
   { defaults with
     name = "rename"; added = (1, 21, 5);
     style = RErr, [String (Pathname, "oldpath"); String (Pathname,
"newpath")], [];
diff --git a/generator/proc_nr.ml b/generator/proc_nr.ml
index 3e393da73..9e16ab14a 100644
--- a/generator/proc_nr.ml
+++ b/generator/proc_nr.ml
@@ -510,6 +510,8 @@ let proc_nr = [
 500, "inspect_get_mountpoints";
 501, "inspect_get_filesystems";
 502, "inspect_get_drive_mappings";
+503, "part_set_gpt_attributes";
+504, "part_get_gpt_attributes";
 ]
 
 (* End of list.  If adding a new entry, add it at the end of the list
diff --git a/lib/MAX_PROC_NR b/lib/MAX_PROC_NR
index cc5027eed..3091e8eea 100644
--- a/lib/MAX_PROC_NR
+++ b/lib/MAX_PROC_NR
@@ -1 +1 @@
-502
+504
-- 
2.15.1

In some cases, the first stage bootloader needs the 'Legacy BIOS
bootable' flag to be set on the partition. This change copies all
flags (including this one) for each partition of the old disk to the
new one to avoid ending up with non-bootable disks.
---
 resize/resize.ml | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/resize/resize.ml b/resize/resize.ml
index 880fa98cb..1a21e4dff 100644
--- a/resize/resize.ml
+++ b/resize/resize.ml
@@ -50,6 +50,7 @@ type partition = {
   p_type : partition_content;    (* Content type and content size. *)
   p_label : string option;       (* Label/name. *)
   p_guid : string option;        (* Partition GUID (GPT only). *)
+  p_attributes : int64 option;   (* Partition attributes bit mask (GPT only).
*)
 
   (* What we're going to do: *)
   mutable p_operation : partition_operation;
@@ -493,6 +494,12 @@ read the man page virt-resize(1).
           let label              try Some (g#part_get_name "/dev/sda"
part_num)
             with G.Error _ -> None in
+          let attributes +            match parttype with
+            | MBR -> None
+            | GPT ->
+              try Some (g#part_get_gpt_attributes "/dev/sda"
part_num)
+              with G.Error _ -> None in
           let guid              match parttype with
             | MBR -> None
@@ -502,7 +509,7 @@ read the man page virt-resize(1).
 
           { p_name = name; p_part = part;
             p_bootable = bootable; p_id = id; p_type = typ;
-            p_label = label; p_guid = guid;
+            p_label = label; p_guid = guid; p_attributes = attributes;
             p_operation = OpCopy; p_target_partnum = 0;
             p_target_start = 0L; p_target_end = 0L }
       ) parts in
@@ -1150,6 +1157,7 @@ read the man page virt-resize(1).
                      part_size = 0L };
           p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
           p_label = None; p_guid = None;
+          p_attributes = None;
 
           (* Target information is meaningful. *)
           p_operation = OpIgnore;
@@ -1191,12 +1199,13 @@ read the man page virt-resize(1).
    * is changed from primary to extended.  Thus we need to set the
    * MBR ID before doing the copy so sfdisk doesn't corrupt things.
    *)
-  let set_partition_bootable_and_id p +  let set_partition_attributes p       
if p.p_bootable then
         g#part_set_bootable "/dev/sdb" p.p_target_partnum true;
 
       Option.may (g#part_set_name "/dev/sdb" p.p_target_partnum)
p.p_label;
       Option.may (g#part_set_gpt_guid "/dev/sdb" p.p_target_partnum)
p.p_guid;
+      Option.may (g#part_set_gpt_attributes "/dev/sdb"
p.p_target_partnum) p.p_attributes;
 
       match parttype, p.p_id with
       | GPT, GPT_Type gpt_type ->
@@ -1205,7 +1214,7 @@ read the man page virt-resize(1).
         g#part_set_mbr_id "/dev/sdb" p.p_target_partnum mbr_id
       | GPT, (No_ID|MBR_ID _) | MBR, (No_ID|GPT_Type _) -> ()
   in
-  List.iter set_partition_bootable_and_id partitions;
+  List.iter set_partition_attributes partitions;
 
   (* Copy over the data. *)
   let copy_partition p -- 
2.15.1

On Mon, Jan 15, 2018 at 03:13:59PM +0100, Cédric Bosdonnat
wrote:
> Rename the sgdisk_info_extract_uuid_field to
> sgdisk_info_extract_field in order to reuse it for other field types.
> Just like its C ancestor, it now needs an extractor function to be
> passed as parameter.
> ---
>  daemon/parted.ml | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/daemon/parted.ml b/daemon/parted.ml
> index d6638867a..6fe803613 100644
> --- a/daemon/parted.ml
> +++ b/daemon/parted.ml
> @@ -124,12 +124,11 @@ let part_get_parttype device >    | _ ->
>       failwithf "%s: cannot parse the output of parted" device
>  
> -let rec part_get_gpt_type device partnum > - 
sgdisk_info_extract_uuid_field device partnum "Partition GUID code"
> -and part_get_gpt_guid device partnum > - 
sgdisk_info_extract_uuid_field device partnum "Partition unique GUID"
> +let extract_guid value > +  (* The value contains only valid GUID
characters. *)
> +  String.sub value 0 (String.span value "-0123456789ABCDEF")
>  
> -and sgdisk_info_extract_uuid_field device partnum field > +let
sgdisk_info_extract_field device partnum field extractor >    if partnum
<= 0 then failwith "partition number must be >= 1";
>  
>    udev_settle ();
> @@ -167,13 +166,16 @@ and sgdisk_info_extract_uuid_field device partnum
field >         (* Skip any whitespace after the colon. *)
>         let value = String.triml value in
>  
> -       (* Extract the UUID. *)
> -       extract_uuid value
> +       (* Extract the value. *)
> +       extractor value
>  
>      | _ :: lines -> loop lines
>    in
>    loop lines
>  
> -and extract_uuid value > -  (* The value contains only valid GUID
characters. *)
> -  String.sub value 0 (String.span value "-0123456789ABCDEF")
> +let rec part_get_gpt_type device partnum > +  sgdisk_info_extract_field
device partnum "Partition GUID code"
> +                            extract_guid
> +and part_get_gpt_guid device partnum > +  sgdisk_info_extract_field
device partnum "Partition unique GUID"
> +                            extract_guid
> -- 
> 2.15.1
Simple refactoring, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Mon, Jan 15, 2018 at 03:14:00PM +0100, Cédric Bosdonnat
wrote:
> Allow reading and setting the GPT partition attribute flags.
> ---
>  daemon/parted.ml          | 23 +++++++++++++++++++++++
>  daemon/parted.mli         |  3 +++
>  generator/actions_core.ml | 37 +++++++++++++++++++++++++++++++++++++
>  generator/proc_nr.ml      |  2 ++
>  lib/MAX_PROC_NR           |  2 +-
>  5 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/parted.ml b/daemon/parted.ml
> index 6fe803613..e3ab823bd 100644
> --- a/daemon/parted.ml
> +++ b/daemon/parted.ml
> @@ -124,10 +124,30 @@ let part_get_parttype device >    | _ ->
>       failwithf "%s: cannot parse the output of parted" device
>  
> +let part_set_gpt_attributes device partnum attributes > +  if partnum
<= 0 then failwith "partition number must be >= 1";
> +
> +  udev_settle ();
> +
> +  let hex = Printf.sprintf "%LX" attributes in
> +  let arg = string_of_int partnum ^ ":=:" ^ hex in
You could write these two lines more simply and clearly as:

  let arg = sprintf "%d:=:%LX" partnum attributes in

You will also need to add ‘open Printf’ after ‘open Scanf’ at the top
of the file so that all the Printf.* functions are available without
needing to be prefixed.
> diff --git a/daemon/parted.mli b/daemon/parted.mli
> index cbcb7b503..9f57bbac7 100644
> --- a/daemon/parted.mli
> +++ b/daemon/parted.mli
> @@ -30,3 +30,6 @@ val part_get_parttype : string -> string
>  
>  val part_get_gpt_type : string -> int -> string
>  val part_get_gpt_guid : string -> int -> string
> +val part_get_gpt_attributes : string -> int -> int64
> +
> +val part_set_gpt_attributes : string -> int -> int64 -> unit
Unnecessary blank line added here?
> diff --git a/generator/actions_core.ml b/generator/actions_core.ml
> index 02759a6b7..78eee61dd 100644
> --- a/generator/actions_core.ml
> +++ b/generator/actions_core.ml
> @@ -8265,6 +8265,43 @@ Return the type GUID of numbered GPT partition
C<partnum>. For MBR partitions,
>  return an appropriate GUID corresponding to the MBR type. Behaviour is
undefined
>  for other partition types." };
>  
> +  { defaults with
> +    name = "part_set_gpt_attributes"; added = (1, 21, 1);
> +    style = RErr, [String (Device, "device"); Int
"partnum"; Int64 "attributes"], [];
> +    impl = OCaml "Parted.part_set_gpt_attributes";
> +    optional = Some "gdisk";
> +    tests = [
> +      InitGPT, Always, TestResult (
> +        [["part_set_gpt_attributes"; "/dev/sda";
"1";
> +          "4"];
> +         ["part_get_gpt_attributes"; "/dev/sda";
"1"]],
> +        "4"), [];
> +    ];
> +    shortdesc = "set the attribute flags of a GPT partition";
> +    longdesc = "\
> +Set the attribute flags of numbered GPT partition C<partnum> to
C<attributes>. Return an
> +error if the partition table of C<device> isn't GPT.
> +
> +See
L<https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries>
> +for a useful list of partition attributes." };
So we're implicitly importing the ABI of GPT partition attributes into
the libguestfs ABI, which I think is fine as it seems to be
well-defined.
> +  { defaults with
> +    name = "part_get_gpt_attributes"; added = (1, 21, 1);
> +    style = RInt64 "attributes", [String (Device,
"device"); Int "partnum"], [];
> +    impl = OCaml "Parted.part_get_gpt_attributes";
> +    optional = Some "gdisk";
> +    tests = [
> +      InitGPT, Always, TestResult (
> +        [["part_set_gpt_attributes"; "/dev/sda";
"1";
> +          "0"];
> +         ["part_get_gpt_attributes"; "/dev/sda";
"1"]],
> +        "4"), [];
> +    ];
> +    shortdesc = "get the attribute flags of a GPT partition";
> +    longdesc = "\
> +Return the attribute flags of numbered GPT partition C<partnum> as a
hexadecimal bit mask.
It's not actually returning a "hexadecimal bit mask".  Just remove
that.

Seems fine apart from the changes noted.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Mon, Jan 15, 2018 at 03:14:01PM +0100, Cédric Bosdonnat
wrote:
> In some cases, the first stage bootloader needs the 'Legacy BIOS
> bootable' flag to be set on the partition. This change copies all
> flags (including this one) for each partition of the old disk to the
> new one to avoid ending up with non-bootable disks.
> ---
>  resize/resize.ml | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/resize/resize.ml b/resize/resize.ml
> index 880fa98cb..1a21e4dff 100644
> --- a/resize/resize.ml
> +++ b/resize/resize.ml
> @@ -50,6 +50,7 @@ type partition = {
>    p_type : partition_content;    (* Content type and content size. *)
>    p_label : string option;       (* Label/name. *)
>    p_guid : string option;        (* Partition GUID (GPT only). *)
> +  p_attributes : int64 option;   (* Partition attributes bit mask (GPT
only). *)
>  
>    (* What we're going to do: *)
>    mutable p_operation : partition_operation;
> @@ -493,6 +494,12 @@ read the man page virt-resize(1).
>            let label >              try Some (g#part_get_name
"/dev/sda" part_num)
>              with G.Error _ -> None in
> +          let attributes > +            match parttype with
> +            | MBR -> None
> +            | GPT ->
> +              try Some (g#part_get_gpt_attributes "/dev/sda"
part_num)
> +              with G.Error _ -> None in
>            let guid >              match parttype with
>              | MBR -> None
> @@ -502,7 +509,7 @@ read the man page virt-resize(1).
>  
>            { p_name = name; p_part = part;
>              p_bootable = bootable; p_id = id; p_type = typ;
> -            p_label = label; p_guid = guid;
> +            p_label = label; p_guid = guid; p_attributes = attributes;
>              p_operation = OpCopy; p_target_partnum = 0;
>              p_target_start = 0L; p_target_end = 0L }
>        ) parts in
> @@ -1150,6 +1157,7 @@ read the man page virt-resize(1).
>                       part_size = 0L };
>            p_bootable = false; p_id = No_ID; p_type = ContentUnknown;
>            p_label = None; p_guid = None;
> +          p_attributes = None;
>  
>            (* Target information is meaningful. *)
>            p_operation = OpIgnore;
> @@ -1191,12 +1199,13 @@ read the man page virt-resize(1).
>     * is changed from primary to extended.  Thus we need to set the
>     * MBR ID before doing the copy so sfdisk doesn't corrupt things.
>     *)
> -  let set_partition_bootable_and_id p > +  let set_partition_attributes
p >        if p.p_bootable then
>          g#part_set_bootable "/dev/sdb" p.p_target_partnum true;
>  
>        Option.may (g#part_set_name "/dev/sdb" p.p_target_partnum)
p.p_label;
>        Option.may (g#part_set_gpt_guid "/dev/sdb"
p.p_target_partnum) p.p_guid;
> +      Option.may (g#part_set_gpt_attributes "/dev/sdb"
p.p_target_partnum) p.p_attributes;
>  
>        match parttype, p.p_id with
>        | GPT, GPT_Type gpt_type ->
> @@ -1205,7 +1214,7 @@ read the man page virt-resize(1).
>          g#part_set_mbr_id "/dev/sdb" p.p_target_partnum mbr_id
>        | GPT, (No_ID|MBR_ID _) | MBR, (No_ID|GPT_Type _) -> ()
>    in
> -  List.iter set_partition_bootable_and_id partitions;
> +  List.iter set_partition_attributes partitions;
>  
>    (* Copy over the data. *)
>    let copy_partition p > -- 
Looks good, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org