[Libguestfs] [PATCH v2 02/11] resize: add logical_partitions and extended_partition

[Richard W.M. Jones]

On Wed, May 20, 2015 at 06:51:28AM -0400, Chen Hanxiao wrote:
> For MBR, logical partitions laid inside extended partition.
> Add 3 variables to describe this:
>  - partitions
>   flat list of primary partitions (as now, the global 'partitions').
>   extended partitions is essentially  primary partition
> 
>  - logical_partitions
>   flat list of logical partitions
> 
>  - extended_partition
>   one MBR extended partition
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> ---
>  resize/resize.ml | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/resize/resize.ml b/resize/resize.ml
> index 4c97405..d7a8ce1 100644
> --- a/resize/resize.ml
> +++ b/resize/resize.ml
> @@ -26,19 +26,16 @@ module G = Guestfs
>  (* Minimum surplus before we create an extra partition. *)
>  let min_extra_partition = 10L *^ 1024L *^ 1024L
>  
> +(* mbr extended partition *)
> +let extended_partition_list = []

This assignment doesn't really do anything.  'let' bindings in OCaml
don't create global variables.  This just makes
'extended_partition_list' be a name for the empty list, ...

[...]

> +  let extended_partition_list = List.append
> +    extended_partition_list

... up to here, where you create a second name, also
'extended_partition_list', completely independent from the one above,
which for no reason appends to the empty list.

What *doesn't* happen is 'extended_partition_list' that you created at
the top is modified.

> +  let extended_partition = if (List.length extended_partition_list) > 0 then
> +    List.hd extended_partition_list else List.hd partitions in

You could write this more naturally as:

  let extended_partition =
    match extended_partition_list with
    | h :: _ -> h
    | [] -> List.hd partitions in

Rewriting it like this also makes the bug in this code clearer: why is
extended_partition initialized to the first partition, if there are no
extended partitions?

I think you probably meant to write:

  let extended_partition =
    match extended_partition_list with
    | h :: _ -> Some h
    | [] -> None in


> +  let logical_partitions =
> +    List.filter (fun p -> parttype = MBR && p.p_mbr_p_type = LogicalPartition) partitions in
> +  (* Filter out logical partitions.  See note above. *)
> +  let partitions =
> +    (* for GPT, all partitions are regarded as Primary Partition,
> +     * e.g. there is no Extended Partition or Logical Partition. *)
> +    List.filter (fun p -> parttype <> MBR || p.p_mbr_p_type <> LogicalPartition) partitions in

Although this is code is correct, it is clearer and shorter to write
the test using a separate function, so it would become:

  let is_logical_partition p =
    parttype = MBR && p.p_mbr_p_type = LogicalPartition
  in
  let logical_partitions = List.filter is_logical_partition partitions in
  let partitions =
    List.filter (fun p -> not (is_logical_partition p)) partitions in

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v