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

[Hu Tao]

Hi Rich,

On Mon, Sep 08, 2014 at 03:13:32PM +0100, Richard W.M. Jones wrote:
> 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

I've found the reason of this error. It's because of a bug of this patch
related to --expand. I tested it with --resize so haven't been able to
find the bug. I'll send the updated patch later.

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

Thanks, I'll post lsof output and trace output.

> 
> > +  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.

Okay.

> 
> > +  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.

Yes, it is the list of logical partitions. 

> 
> >    (* 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.

Okay.

> 
> 
> > +  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?

It's for restarting the guest so the kernel can re-read the partition
table, otherwise even if the logical partitions have been added
successfully, the kernel can't read them for writing.

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

Basically I was using virt-resize --resize to test the patch, other
commands are very similar with you script, except that I pre-created the
image with partitions and some contents in them. I think a test script
is a good idea, should I add it to the repo?

Regards,
Hu

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