Libguestfs - Sep 2010 - [PATCH 0/4] Fixes for virt-resize

Fixes for both of these bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=633096
virt-resize calculates block device size incorrectly, doesn't work with
qcow2 target

https://bugzilla.redhat.com/show_bug.cgi?id=633766
virt-resize --shrink fails

I'm still doing testing on these, but the patches seem good
enough to review.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top

The first two patches add pread-device.

(pwrite-device, which I added a few days ago, is also required for this).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
-------------- next part --------------
>From e65f5213637e71f6f88763ce177fd23c65e1033d Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Mon, 27 Sep 2010 10:51:38 +0100
Subject: [PATCH 1/4] pread: Check count and offset parameters are not negative.

---
 daemon/file.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/daemon/file.c b/daemon/file.c
index 9403100..5bc5f41 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -415,6 +415,16 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
   ssize_t r;
   char *buf;
 
+  if (count < 0) {
+    reply_with_error ("count is negative");
+    return NULL;
+  }
+
+  if (offset < 0) {
+    reply_with_error ("offset is negative");
+    return NULL;
+  }
+
   /* The actual limit on messages is smaller than this.  This check
    * just limits the amount of memory we'll try and allocate in the
    * function.  If the message is larger than the real limit, that
-- 
1.7.3

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
-------------- next part --------------
>From 7e4bf29fdf3067f3e83cd818a79ee522ae28e7ef Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Mon, 27 Sep 2010 12:00:10 +0100
Subject: [PATCH 2/4] New API: pread-device, partial read for devices.

---
 daemon/file.c                  |   53 ++++++++++++++++++++++++++++-----------
 generator/generator_actions.ml |   15 ++++++++++-
 src/MAX_PROC_NR                |    2 +-
 3 files changed, 53 insertions(+), 17 deletions(-)

diff --git a/daemon/file.c b/daemon/file.c
index 5bc5f41..c16a00a 100644
--- a/daemon/file.c
+++ b/daemon/file.c
@@ -408,20 +408,22 @@ do_read_file (const char *path, size_t *size_r)
   return r;
 }
 
-char *
-do_pread (const char *path, int count, int64_t offset, size_t *size_r)
+static char *
+pread_fd (int fd, int count, int64_t offset, size_t *size_r,
+          const char *display_path)
 {
-  int fd;
   ssize_t r;
   char *buf;
 
   if (count < 0) {
     reply_with_error ("count is negative");
+    close (fd);
     return NULL;
   }
 
   if (offset < 0) {
     reply_with_error ("offset is negative");
+    close (fd);
     return NULL;
   }
 
@@ -431,16 +433,8 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
    * will be caught later when we try to serialize the message.
    */
   if (count >= GUESTFS_MESSAGE_MAX) {
-    reply_with_error ("%s: count is too large for the protocol, use
smaller reads", path);
-    return NULL;
-  }
-
-  CHROOT_IN;
-  fd = open (path, O_RDONLY);
-  CHROOT_OUT;
-
-  if (fd == -1) {
-    reply_with_perror ("open: %s", path);
+    reply_with_error ("%s: count is too large for the protocol, use
smaller reads", display_path);
+    close (fd);
     return NULL;
   }
 
@@ -453,14 +447,14 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
 
   r = pread (fd, buf, count, offset);
   if (r == -1) {
-    reply_with_perror ("pread: %s", path);
+    reply_with_perror ("pread: %s", display_path);
     close (fd);
     free (buf);
     return NULL;
   }
 
   if (close (fd) == -1) {
-    reply_with_perror ("close: %s", path);
+    reply_with_perror ("close: %s", display_path);
     close (fd);
     free (buf);
     return NULL;
@@ -473,6 +467,35 @@ do_pread (const char *path, int count, int64_t offset,
size_t *size_r)
   return buf;
 }
 
+char *
+do_pread (const char *path, int count, int64_t offset, size_t *size_r)
+{
+  int fd;
+
+  CHROOT_IN;
+  fd = open (path, O_RDONLY);
+  CHROOT_OUT;
+
+  if (fd == -1) {
+    reply_with_perror ("open: %s", path);
+    return NULL;
+  }
+
+  return pread_fd (fd, count, offset, size_r, path);
+}
+
+char *
+do_pread_device (const char *device, int count, int64_t offset, size_t *size_r)
+{
+  int fd = open (device, O_RDONLY);
+  if (fd == -1) {
+    reply_with_perror ("open: %s", device);
+    return NULL;
+  }
+
+  return pread_fd (fd, count, offset, size_r, device);
+}
+
 static int
 pwrite_fd (int fd, const char *content, size_t size, int64_t offset,
            const char *display_path)
diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
index e94fcbd..c9890a6 100644
--- a/generator/generator_actions.ml
+++ b/generator/generator_actions.ml
@@ -4190,7 +4190,7 @@ bytes of the file, starting at C<offset>, from file
C<path>.
 This may read fewer bytes than requested.  For further details
 see the L<pread(2)> system call.
 
-See also C<guestfs_pwrite>.");
+See also C<guestfs_pwrite>, C<guestfs_pread_device>.");
 
   ("part_init", (RErr, [Device "device"; String
"parttype"]), 208, [],
    [InitEmpty, Always, TestRun (
@@ -5183,6 +5183,19 @@ probably impossible with standard Linux kernels).
 
 See also C<guestfs_pwrite>.");
 
+  ("pread_device", (RBufferOut "content", [Device
"device"; Int "count"; Int64 "offset"]), 276,
[ProtocolLimitWarning],
+   [InitEmpty, Always, TestOutputBuffer (
+      [["pread_device"; "/dev/sdd"; "8";
"32768"]], "\001CD001\001\000")],
+   "read part of a device",
+   "\
+This command lets you read part of a file.  It reads C<count>
+bytes of C<device>, starting at C<offset>.
+
+This may read fewer bytes than requested.  For further details
+see the L<pread(2)> system call.
+
+See also C<guestfs_pread>.");
+
 ]
 
 let all_functions = non_daemon_functions @ daemon_functions
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 4c738e3..15007f1 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-275
+276
-- 
1.7.3

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
-------------- next part --------------
>From 2281b0c3cbe1e040f78c8e44009808f2bbee8139 Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at 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..82e7667 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;
- at s = stat $infile;
-my $insize = S_ISREG ($s[2]) ? $s[7] : host_blockdevsize ($infile);
- at 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", 446, 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", $max_bootloader, $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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
-------------- next part --------------
>From 7d14ce68915df10c4581b7adfcf49f1bcff33bf5 Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Mon, 27 Sep 2010 17:06:14 +0100
Subject: [PATCH 4/4] Add test for virt-resize.

This tests a number of things which have caused problems for us:

 - resizing PVs and LV content
 - handling GPT format disks
 - using qcow2 as a target disk format
 - shrinking disk images

Note that the disk content is empty (not a real VM), but this is
adequate since all we want to test are the operations and calculations
done by virt-resize.  We are not interested here in whether e2fsprogs
and LVM actually works.
---
 tools/Makefile.am         |    1 +
 tools/test-virt-resize.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100755 tools/test-virt-resize.sh

diff --git a/tools/Makefile.am b/tools/Makefile.am
index ab14fe0..b92243c 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -86,6 +86,7 @@ TESTS = test-virt-cat.sh \
 	test-virt-list-filesystems.sh \
 	test-virt-ls.sh \
 	test-virt-make-fs.sh \
+	test-virt-resize.sh \
 	test-virt-tar.sh
 
 endif
diff --git a/tools/test-virt-resize.sh b/tools/test-virt-resize.sh
new file mode 100755
index 0000000..75d5147
--- /dev/null
+++ b/tools/test-virt-resize.sh
@@ -0,0 +1,31 @@
+#!/bin/bash -
+
+export LANG=C
+set -e
+
+# Test expanding.
+#
+# This exercises a number of interesting codepaths including resizing
+# LV content, handling GPT, and using qcow2 as a target.
+
+../fish/guestfish -N bootrootlv:/dev/VG/LV:ext2:ext4:400M:32M:gpt </dev/null
+
+qemu-img create -f qcow2 test2.img 500M
+./virt-resize -d --expand /dev/sda2 --lv-expand /dev/VG/LV test1.img test2.img
+
+# Test shrinking in a semi-realistic scenario.  Although the disk
+# image created above contains no data, we will nevertheless use
+# similar operations to ones that might be used by a real admin.
+
+../fish/guestfish -a test1.img <<EOF
+run
+resize2fs-size /dev/VG/LV 190M
+lvresize /dev/VG/LV 190
+pvresize-size /dev/sda2 200M
+fsck ext4 /dev/VG/LV
+EOF
+
+rm -f test2.img; truncate -s 300M test2.img
+./virt-resize -d --shrink /dev/sda2 test1.img test2.img
+
+rm -f test1.img test2.img
-- 
1.7.3

On Mon, Sep 27, 2010 at 05:14:08PM +0100, Richard W.M. Jones
wrote:
> I'm still doing testing on these, but the patches seem good
> enough to review.
I should note that this won't necessarily let you do a resize:
><fs> pvresize-size /dev/sda2 6G
libguestfs: error: pvresize_size: /dev/vda2:   /dev/vda2: cannot resize to 191
extents as later ones are allocated.

It turns out that pvresize won't "defrag" physical extents,
therefore
it is a matter of luck whether you can shrink a PV.

Reading around the issue it seems you can manually move PEs using
pvmove, lvdisplay, guesswork and luck.  There might be scope for a
"pvdefrag" command in libguestfs at some point ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html

I have done some further sanity on the new virt-resize and it seems to
be working on real VMs, so I have pushed these patches.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v