On Mon, Sep 15, 2014 at 10:54:17AM +0100, Richard W.M. Jones wrote:
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).
Thanks, I'll check it out.
+ 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.
Working on it.
(* 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.
Sure. Regards, Hu