Libguestfs - Sep 2014 - Re: [RFC PATCH] resize: add support for MBR logical partitions some question

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

Attached is the test script I was using.

You have to run it like this:

  ./run ./test.sh

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

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

On Mon, Sep 15, 2014 at 05:47:03PM +0800, Hu Tao wrote:
> 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.
This weekend I found a bug that might be similar to this one.  See:

  https://bugzilla.redhat.com/show_bug.cgi?id=1141451

If it's the same thing, it might be fixed by calling 'udev_settle'
after copy_device_to_device (see daemon/copy.c).
> > 
> > > +  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. 
So let's make that clear by having a 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.
> 
> 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?
Yes, definitely it needs a test.

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