[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH 3/4] resize: Fix handling of GPT and qcow2 (RHBZ#633766, RHBZ#633096).



On Mon, Sep 27, 2010 at 05:16:54PM +0100, Richard W.M. Jones wrote:
> +    $g->pwrite_device ("/dev/sdb", 446, 0);
> +    $g->pwrite_device ("/dev/sdb", $max_bootloader, $start);

Oops, rather obvious bug.

The attached patch fixes this, and I have successfully used it to
shrink a Fedora disk image.  Even booted up after I did it ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
>From 0dc1f8e7aeb390e72125f459930850b8050318df Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones redhat com>
Date: Mon, 27 Sep 2010 17:00:45 +0100
Subject: [PATCH 3/4] resize: Fix handling of GPT and qcow2 (RHBZ#633766, RHBZ#633096).

Previously we copied the bootloader data directly from the
source disk image to the target disk image using host file
operations (before launching libguestfs).  This has two problems:
firstly it has no chance of working with qcow2, and secondly
it didn't behave properly with GPT.

This changes the code so that everything is done through
libguestfs.  Block device sizes are now calculated properly
for qcow2 (RHBZ#633096) because this is done using the libguestfs
blockdev_getsize64 call.  The partition table is still created
by parted, but to workaround a bug in parted this is done before
copying the bootloader.  Finally the bootloader copy is done
using the new APIs pread-device and pwrite-device.

Shrinking now works, at least for simple cases (RHBZ#633766).
---
 tools/virt-resize |  149 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 79 insertions(+), 70 deletions(-)

diff --git a/tools/virt-resize b/tools/virt-resize
index 28f51af..6e11d47 100755
--- a/tools/virt-resize
+++ b/tools/virt-resize
@@ -530,17 +530,66 @@ die __x("virt-resize: {file}: does not exist or is not readable\n", file => $inf
 die __x("virt-resize: {file}: does not exist or is not writable\nYou have to create the destination disk before running this program.\nPlease read the virt-resize(1) manpage for more information.\n", file => $outfile)
     unless -w $outfile;
 
-my @s;
- s = stat $infile;
-my $insize = S_ISREG ($s[2]) ? $s[7] : host_blockdevsize ($infile);
- s = stat $outfile;
-my $outsize = S_ISREG ($s[2]) ? $s[7] : host_blockdevsize ($outfile);
+# Add them to the handle and launch the appliance.
+my $g;
+launch_guestfs ();
+
+sub launch_guestfs
+{
+    $g = Sys::Guestfs->new ();
+    $g->set_trace (1) if $debug;
+    $g->add_drive_ro ($infile);
+    $g->add_drive ($outfile);
+    $g->set_progress_callback (\&progress_callback) unless $quiet;
+    $g->launch ();
+}
+
+my $sectsize = $g->blockdev_getss ("/dev/sdb");
+
+# Get the size in bytes of each disk.
+#
+# Originally we computed this by looking at the same of the host file,
+# but of course this failed for qcow2 images (RHBZ#633096).  The right
+# way to do it is with $g->blockdev_getsize64.
+my $insize = $g->blockdev_getsize64 ("/dev/sda");
+my $outsize = $g->blockdev_getsize64 ("/dev/sdb");
 
 if ($debug) {
     print "$infile size $insize bytes\n";
     print "$outfile size $outsize bytes\n";
 }
 
+# Create a partition table.
+#
+# We *must* do this before copying the bootloader across, and copying
+# the bootloader must be careful not to disturb this partition table
+# (RHBZ#633766).  There are two reasons for this:
+#
+# (1) The 'parted' library is stupid and broken.  In many ways.  In
+# this particular instance the stupid and broken bit is that it
+# overwrites the whole boot sector when initializating a partition
+# table.  (Upstream don't consider this obvious problem to be a bug).
+#
+# (2) GPT has a backup partition table located at the end of the disk.
+# It's non-movable, because the primary GPT contains fixed references
+# to both the size of the disk and the backup partition table at the
+# end.  This would be a problem for any resize that didn't either
+# carefully move the backup GPT (and rewrite those references) or
+# recreate the whole partition table from scratch.
+
+my $parttype;
+create_partition_table ();
+
+sub create_partition_table
+{
+    local $_;
+
+    $parttype = $g->part_get_parttype ("/dev/sda");
+    print "partition table type: $parttype\n" if $debug;
+
+    $g->part_init ("/dev/sdb", $parttype);
+}
+
 # In reality the number of sectors containing boot loader data will be
 # less than this (although Windows 7 defaults to putting the first
 # partition on sector 2048, and has quite a large boot loader).
@@ -550,14 +599,14 @@ if ($debug) {
 # offset of the first partition.
 #
 # It doesn't matter if we copy too much.
-my $boot_sectors = 4096;
+my $max_bootloader = 4096 * 512;
 
 die __x("virt-resize: {file}: file is too small to be a disk image ({sz} bytes)\n",
         file => $infile, sz => $insize)
-    if $insize < $boot_sectors * 512;
+    if $insize < $max_bootloader;
 die __x("virt-resize: {file}: file is too small to be a disk image ({sz} bytes)\n",
         file => $outfile, sz => $outsize)
-    if $outsize < $boot_sectors * 512;
+    if $outsize < $max_bootloader;
 
 # Copy the boot loader across.
 do_copy_boot_loader () if $copy_boot_loader;
@@ -565,31 +614,28 @@ do_copy_boot_loader () if $copy_boot_loader;
 sub do_copy_boot_loader
 {
     print "copying boot loader ...\n" if $debug;
-    open IFILE, $infile or die "$infile: $!";
-    my $s;
-    my $r = sysread (IFILE, $s, $boot_sectors * 512) or die "$infile: $!";
-    die "$infile: short read" if $r < $boot_sectors * 512;
-    open OFILE, "+<$outfile" or die "$outfile: $!";
-    sysseek OFILE, 0, SEEK_SET or die "$outfile: seek: $!";
-    $r = syswrite (OFILE, $s, $boot_sectors * 512) or die "$outfile: $!";
-    die "$outfile: short write" if $r < $boot_sectors * 512;
-}
 
-# Add them to the handle and launch the appliance.
-my $g;
-launch_guestfs ();
+    # Don't disturb the partition table that we just wrote.
+    # https://secure.wikimedia.org/wikipedia/en/wiki/Master_Boot_Record
+    # https://secure.wikimedia.org/wikipedia/en/wiki/GUID_Partition_Table
 
-sub launch_guestfs
-{
-    $g = Sys::Guestfs->new ();
-    $g->set_trace (1) if $debug;
-    $g->add_drive_ro ($infile);
-    $g->add_drive ($outfile);
-    $g->set_progress_callback (\&progress_callback) unless $quiet;
-    $g->launch ();
-}
+    my $bootsect = $g->pread_device ("/dev/sda", 446, 0);
+    die __"virt-resize: short read" if length ($bootsect) < 446;
 
-my $sectsize = $g->blockdev_getss ("/dev/sdb");
+    $g->pwrite_device ("/dev/sdb", $bootsect, 0);
+
+    my $start = 512;
+    if ($parttype eq "gpt") {
+        # XXX With 4K sectors does GPT just fit more entries in a
+        # sector, or does it always use 34 sectors?
+        $start = 17408;
+    }
+
+    my $loader = $g->pread_device ("/dev/sda", $max_bootloader, $start);
+    die __"virt-resize: short read" if length ($loader) < $max_bootloader;
+
+    $g->pwrite_device ("/dev/sdb", $loader, $start);
+}
 
 my $to_be_expanded = 0;
 
@@ -888,9 +934,9 @@ sub calculate_surplus
     # EFI partitioning + massive per-partition alignment.
     my $overhead = $sectsize * (
         2 * 64 +                   # GPT start and end
-        (64 * (@partitions + 1)) + # Maximum alignment
-        ($boot_sectors - 64)       # Boot loader
-        );
+        (64 * (@partitions + 1))   # Maximum alignment
+        ) +
+        ($max_bootloader - 64 * 512); # boot loader
 
     my $required = 0;
     foreach (@partitions) {
@@ -999,36 +1045,12 @@ exit 0 if $dryrun;
 
 # Repartition the target disk.
 my $nextpart = 1;
-my $parttype;
 repartition ();
 
 sub repartition
 {
     local $_;
 
-    if ($copy_boot_loader) {
-        $parttype = $g->part_get_parttype ("/dev/sdb");
-    } else {
-        $parttype = "efi";
-    }
-    print "partition table type: $parttype\n" if $debug;
-
-    # Delete any existing partitions on the destination disk,
-    # but leave the bootloader that we copied over intact.
-    if ($copy_boot_loader) {
-        # Delete in reverse as an easy way to deal with extended
-        # partitions.
-        foreach (sort { $b cmp $a } $g->list_partitions ()) {
-            if (m{^/dev/.db(\d+)$}) {
-                $g->part_del ("/dev/sdb", $1);
-            }
-        }
-    } else {
-        # Didn't copy over the initial boot loader, so we need
-        # to make a new partition table here.
-        $g->part_init ("/dev/sdb", $parttype);
-    }
-
     # Work out where to start the first partition.
     die __"virt-resize: source disk does not have a first partition\n"
         unless exists ($partitions{"/dev/sda1"});
@@ -1303,19 +1325,6 @@ sub human_size
     }
 }
 
-# Return the size in bytes of a HOST block device.
-sub host_blockdevsize
-{
-    local $_;
-    my $dev = shift;
-
-    open BD, "PATH=/usr/sbin:/sbin:\$PATH blockdev --getsize64 $dev |"
-        or die "blockdev: $!";
-    $_ = <BD>;
-    chomp $_;
-    $_;
-}
-
 # The reverse of device name translation, see
 # BLOCK DEVICE NAMING in guestfs(3).
 sub canonicalize
-- 
1.7.3


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]