[Libguestfs] [RFC PATCH] resize: add support for MBR logical partitions some question

[Richard W.M. Jones]

On Tue, Aug 26, 2014 at 06:16:50PM +0800, Hu Tao wrote:
> Hi,
> 
> The attached patch adds support for resizing MBR logical partitions. The
> failure is still there, I can't get any helpful information from lsof.
> Any suggestions?

I don't see the error:

  Error: Error informing the kernel about modifications to partition /dev/sdb5

However I do see this error:

  virt-resize: error: libguestfs error: copy_device_to_device: 
  copy_device_to_device_stub: /dev/sdb5: No such file or directory

For debugging with lsof, I would need to actually see the trace output
and the lsof output.  See what I wrote here:

  https://www.redhat.com/archives/libguestfs/2014-July/msg00051.html

> +  p_part_num: int;               (* partition number *)

I think you should split out this change into a separate patch.  It's
uncontroversial to keep p_part_num in the structure, and will simplify
review of the patch.

> +  mutable p_partitions : partition list; (* MBR logical partitions. Non-empty
> +                                            list implies extended partition 

I'm very unclear about this change to the structure.

Originally 'type partition' is a single primary/extended partition,
and we keep a list of them.  That's simple to understand.

After this patch, how does it work?

Looking at the rest of the patch it seemed to me that you'd probably
want to keep the list of logical partitions as a completely separate
list.

>    (* Helper function calculates the surplus space, given the total
>     * required so far for the current partition layout, compared to
>     * the size of the target disk.  If the return value >= 0 then it's
> @@ -816,29 +911,31 @@ read the man page virt-resize(1).
>      printf "**********\n\n";
>      printf "Summary of changes:\n\n";
>  
> -    List.iter (
> -      fun ({ p_name = name; p_part = { G.part_size = oldsize }} as p) ->
> +    let rec print_summary p =
>          let text =
>            match p.p_operation with
>            | OpCopy ->
> -              sprintf (f_"%s: This partition will be left alone.") name
> +              sprintf (f_"%s: This partition will be left alone.") p.p_name
>            | OpIgnore ->
> -              sprintf (f_"%s: This partition will be created, but the contents will be ignored (ie. not copied to the target).") name
> +              sprintf (f_"%s: This partition will be created, but the contents will be ignored (ie. not copied to the target).") p.p_name
>            | OpDelete ->
> -              sprintf (f_"%s: This partition will be deleted.") name
> +              sprintf (f_"%s: This partition will be deleted.") p.p_name
>            | OpResize newsize ->
>                sprintf (f_"%s: This partition will be resized from %s to %s.")
> -                name (human_size oldsize) (human_size newsize) ^
> +                p.p_name (human_size p.p_part.G.part_size) (human_size newsize) ^
>                if can_expand_content p.p_type then (
>                  sprintf (f_"  The %s on %s will be expanded using the '%s' method.")
>                    (string_of_partition_content_no_size p.p_type)
> -                  name
> +                  p.p_name
>                    (string_of_expand_content_method
>                       (expand_content_method p.p_type))
>                ) else "" in

This bit seems to rename a variable for no particular reason.  If you
think this is simpler to read, then please submit it as a separate
patch.  Otherwise leave it out.


> +  let g =
> +    g#shutdown ();
> +      g#close ();
> +
> +      let g = new G.guestfs () in
> +      if trace then g#set_trace true;
> +      if verbose then g#set_verbose true;
> +      let _, { URI.path = path; protocol = protocol;
> +             server = server; username = username;
> +             password = password } = infile in
> +      g#add_drive ?format ~readonly:true ~protocol ?server ?username ?secret:password path;
> +      (* The output disk is being created, so use cache=unsafe here. *)
> +      g#add_drive ?format:output_format ~readonly:false ~cachemode:"unsafe"
> +        outfile;
> +      if not quiet then Progress.set_up_progress_bar ~machine_readable g;
> +      g#launch ();
> +      g in

What's this bit for?


----------------------------------------------------------------------

How are you testing this?  It really needs a script that tests this
case, since it obviously makes the code a lot more complex.  Also when
you see the error message, what virt-resize and other commands are you
using?

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