Libguestfs - May 2015 - Re: [PATCH v2 02/11] resize: add logical_partitions and extended_partition

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@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

In data giovedì 28 maggio 2015 12:06:18, Richard W.M. Jones ha
scritto:
> > +  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
Or, even better:

  let logical_partitions, partitions     List.partition is_logical_partition
partitions in

-- 
Pino Toscano

On Thu, May 28, 2015 at 01:13:28PM +0200, Pino Toscano
wrote:
> In data giovedì 28 maggio 2015 12:06:18, Richard W.M. Jones ha scritto:
> > > +  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
> 
> Or, even better:
> 
>   let logical_partitions, partitions >     List.partition
is_logical_partition partitions in
Indeed, that is better :-)

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