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

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.