April 2016 - Libguestfs - libguestfs List Archives

v2v: Remove --no-trim, --vmtype options and other fixes.
by Richard W.M. Jones 26 Apr '16

26 Apr '16

The first two patches implement the change discussed here: https://www.redhat.com/archives/libguestfs/2016-April/msg00178.html The third patch fixes the mapping of inspection data to OVF VmType which was inherited directly from old virt-v2v and had never been changed. It had a number of problems. The fourth patch is only slightly related to the others. It adds an extra slow test to ensure that trimming doesn't regress, fixing https://bugzilla.redhat.com/show_bug.cgi?id=1264332 Rich.

1 4

Proposal to remove two virt-v2v command line options
by Richard W.M. Jones 26 Apr '16

26 Apr '16

Virt-v2v aims to convert guests without any per-guest intervention/ configuration/fiddling. Unfortunately there are some command line options of virt-v2v which don't meet this ideal. Although we don't normally change the command line of our tools, there are two options I'd like to remove (by turning them into warnings & no-ops). Please follow up with comments if this is going to be a problem. Option: --no-trim all --no-trim mp[,mp...] Description in manual: By default virt-v2v runs fstrim(8) to reduce the amount of data that needs to be copied. This is known to break some buggy bootloaders causing boot failures after conversion (see for example https://bugzilla.redhat.com/show_bug.cgi?id=1141145#c27) You can use --no-trim all to disable all trimming. Note this will greatly increase the amount of data that has to be copied and can make virt-v2v run much more slowly. You can also disable trimming on selected filesystems only (specified by a comma-separated list of their mount point(s) in the guest). Typically you would use --no-trim /boot to work around the grub bug mentioned above. You can also disable trimming on partitions using the libguestfs naming scheme for devices, eg: --no-trim /dev/sdb2 means do not trim the second partition on the second block device. Use virt-filesystems(1) to list filesystem names in a guest. Discussion: As noted in the description, we found one RHEL 5 guest which had a buggy bootloader. We've never been able to repeat that or find a similarly broken guest. When I added this option, I was suspicious that fstrim would cause other breakage in guests. With the popularity and widespread use of virt-sparsify, I think that fear was unjustified. Supporting the --no-trim option is also a pain, and it's difficult even for users to understand what it does, so I propose we remove it. Note this means that all guests would be fstrimmed. Option: --vmtype desktop --vmtype server Description in manual: For the -o rhev or -o vdsm targets only, specify the type of guest. You can set this to "desktop" or "server". If the option is not given, then a suitable default is chosen based on the detected guest operating system. Discussion: This option was inherited from old virt-v2v and it sets a single flag in the OVF file when targetting oVirt/RHEV. As far as I can tell this flag affects two things in oVirt engine: (1) Whether sound emulation is enabled for a guest ("desktop" = yes, "server" = no). (2) allowConsoleReconnect which disables something called "strict user checking" when connecting to a VM console ("server" = disabled, "desktop" = not disabled). It is highly unlikely that a virt-v2v user understands this setting (even I didn't understand it before now). Note that we choose a default from inspection data, mapping guest server-like operating systems to "server" and the rest to "desktop". So I propose we get rid of this option, but leave the code which performs the default mapping from inspection data. 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

2 1

[PATCH 1/2] sparsify: Refactor handling of checks of copying mode / --in-place.
by Richard W.M. Jones 21 Apr '16

21 Apr '16

Just refactoring, no change. --- sparsify/cmdline.ml | 49 +++++++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/sparsify/cmdline.ml b/sparsify/cmdline.ml index ce2b913..bd49e71 100644 --- a/sparsify/cmdline.ml +++ b/sparsify/cmdline.ml @@ -98,6 +98,7 @@ read the man page virt-sparsify(1). let check_tmpdir = !check_tmpdir in let compress = !compress in let convert = match !convert with "" -> None | str -> Some str in + let disks = List.rev !disks in let format = match !format with "" -> None | str -> Some str in let ignores = List.rev !ignores in let in_place = !in_place in @@ -109,7 +110,7 @@ read the man page virt-sparsify(1). (* No arguments and machine-readable mode? Print out some facts * about what this binary supports. *) - if !disks = [] && machine_readable then ( + if disks = [] && machine_readable then ( printf "virt-sparsify\n"; printf "linux-swap\n"; printf "zero\n"; @@ -126,23 +127,19 @@ read the man page virt-sparsify(1). exit 0 ); - (* Verify we got exactly 1 or 2 disks, depending on the mode. *) - let indisk, outdisk = - match in_place, List.rev !disks with - | false, [indisk; outdisk] -> indisk, outdisk - | true, [disk] -> disk, "" - | _ -> - error "usage is: %s [--options] indisk outdisk OR %s --in-place disk" - prog prog in + let indisk, mode = + if not in_place then ( (* copying mode checks *) + let indisk, outdisk = + match disks with + | [indisk; outdisk] -> indisk, outdisk + | _ -> error (f_"usage: %s [--options] indisk outdisk") prog in - (* Simple-minded check that the user isn't trying to use the - * same disk for input and output. - *) - if indisk = outdisk then - error (f_"you cannot use the same disk image for input and output"); + (* Simple-minded check that the user isn't trying to use the + * same disk for input and output. + *) + if indisk = outdisk then + error (f_"you cannot use the same disk image for input and output"); - let indisk = - if not in_place then ( (* The input disk must be an absolute path, so we can store the name * in the overlay disk. *) @@ -155,11 +152,17 @@ read the man page virt-sparsify(1). (* Check the output is not a char special (RHBZ#1056290). *) if is_char_device outdisk then error (f_"output '%s' cannot be a character device, it must be a regular file") - outdisk; + outdisk; - indisk + indisk, + Mode_copying (outdisk, check_tmpdir, compress, convert, option, tmp) ) - else ( (* --in-place checks *) + else ( (* --in-place checks *) + let indisk = + match disks with + | [indisk] -> indisk + | _ -> error "usage: %s --in-place [--options] indisk" prog in + if check_tmpdir <> `Warn then error (f_"you cannot use --in-place and --check-tmpdir options together"); @@ -175,15 +178,9 @@ read the man page virt-sparsify(1). if tmp <> None then error (f_"you cannot use --in-place and --tmp options together"); - indisk + indisk, Mode_in_place ) in - let mode = - if not in_place then - Mode_copying (outdisk, check_tmpdir, compress, convert, option, tmp) - else - Mode_in_place in - { indisk = indisk; format = format; ignores = ignores; -- 2.7.4

2 2

[PATCH] dib: Rewrite match statement as ordinary if statement.
by Richard W.M. Jones 21 Apr '16

21 Apr '16

Just stylistic change, no functional change. --- dib/dib.ml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dib/dib.ml b/dib/dib.ml index 06a1f67..35ae6b7 100644 --- a/dib/dib.ml +++ b/dib/dib.ml @@ -781,9 +781,8 @@ let main () = ) @ mkfs_options @ [ "-t"; cmdline.fs_type; blockdev ] in ignore (g#debug "sh" (Array.of_list ([ "mkfs" ] @ mkfs_options))); g#set_label blockdev root_label; - (match cmdline.fs_type with - | x when String.is_prefix x "ext" -> g#set_uuid blockdev rootfs_uuid - | _ -> ()); + if String.is_prefix cmdline.fs_type "ext" then + g#set_uuid blockdev rootfs_uuid; g#mount blockdev "/"; g#mkmountpoint "/tmp"; mount_aux (); -- 2.7.4

2 1

virt-builder and SimpleStreams
by Richard W.M. Jones 21 Apr '16

21 Apr '16

Pino, do you remember what the missing data was that prevents virt-builder from working properly with SimpleStreams? IIRC it was the target (expanded) size of the image, as it means we don't know during the planning stage if we will need to resize the image. CentOS may start to publish this data, but we will need to ask Karanbir to add the extra data we need in JSON field(s). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/

2 3

[PATCH] launch: direct: specify format for appliance drive
by Pino Toscano 18 Apr '16

18 Apr '16

The drive used for the appliance is a raw (sparse) disk: specify that explicitly in its -drive qemu command line options, so qemu can skip the autodetection of its format and save a tiny bit of time. --- src/launch-direct.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/launch-direct.c b/src/launch-direct.c index ee0a855..8521e5a 100644 --- a/src/launch-direct.c +++ b/src/launch-direct.c @@ -563,7 +563,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg) /* Add the ext2 appliance drive (after all the drives). */ if (has_appliance_drive) { ADD_CMDLINE ("-drive"); - ADD_CMDLINE_PRINTF ("file=%s,snapshot=on,id=appliance,cache=unsafe,if=none", + ADD_CMDLINE_PRINTF ("file=%s,snapshot=on,id=appliance,cache=unsafe,if=none,format=raw", appliance); if (virtio_scsi) { -- 2.5.5

2 2

[PATCH 1/2] valgrind: Use --trace-children=no --child-silent-after-fork=yes
by Richard W.M. Jones 15 Apr '16

15 Apr '16

When we are valgrinding we don't really care about the child processes, which might be qemu, libvirtd, etc. So disable tracing into children (at least, as far as is possible with valgrind, which is not entirely disabling it, but suppressing it). --- m4/guestfs_progs.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/m4/guestfs_progs.m4 b/m4/guestfs_progs.m4 index e876942..070fd6d 100644 --- a/m4/guestfs_progs.m4 +++ b/m4/guestfs_progs.m4 @@ -125,7 +125,7 @@ dnl Check for valgrind AC_CHECK_PROG([VALGRIND],[valgrind],[valgrind],[no]) AS_IF([test "x$VALGRIND" != "xno"],[ # Substitute the whole valgrind command. - VG='$(VALGRIND) --vgdb=no --log-file=$(abs_top_builddir)/tmp/valgrind-%q{T}-%p.log --leak-check=full --error-exitcode=119 --suppressions=$(abs_top_srcdir)/valgrind-suppressions' + VG='$(VALGRIND) --vgdb=no --log-file=$(abs_top_builddir)/tmp/valgrind-%q{T}-%p.log --leak-check=full --error-exitcode=119 --suppressions=$(abs_top_srcdir)/valgrind-suppressions --trace-children=no --child-silent-after-fork=yes' ],[ # No valgrind, so substitute VG with something that will break. VG=VALGRIND_IS_NOT_INSTALLED -- 2.7.4

1 1

[PATCH v3 libguestfs] launch: Implement a safer getumask.
by Richard W.M. Jones 15 Apr '16

15 Apr '16

The current implementation of getumask involves writing a file with mode 0777 and then testing what mode was created by the kernel. This doesn't work properly if the user set a per-mount umask (or fmask/ dmask). This alternative method was suggested by Josh Stone. By forking, we can use the thread-unsafe method (calling umask) and pass the result back over a pipe. This change also fixes another problem: mode_t is unsigned, so cannot be used to return an error indication (ie. -1). Return a plain int instead. Thanks: Josh Stone, Jiri Jaburek, Eric Blake. --- docs/C_SOURCE_FILES | 1 + po/POTFILES | 1 + src/Makefile.am | 1 + src/guestfs-internal.h | 3 ++ src/launch.c | 41 ++---------------- src/umask.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 120 insertions(+), 37 deletions(-) create mode 100644 src/umask.c diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES index a477744..f828911 100644 --- a/docs/C_SOURCE_FILES +++ b/docs/C_SOURCE_FILES @@ -269,6 +269,7 @@ src/structs-free.c src/structs-print.c src/test-utils.c src/tmpdirs.c +src/umask.c src/utils.c src/whole-file.c test-tool/test-tool.c diff --git a/po/POTFILES b/po/POTFILES index 9e4d9cc..23599b9 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -359,6 +359,7 @@ src/structs-free.c src/structs-print.c src/test-utils.c src/tmpdirs.c +src/umask.c src/utils.c src/whole-file.c test-tool/test-tool.c diff --git a/src/Makefile.am b/src/Makefile.am index 3b4cd10..34c4fa6 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -130,6 +130,7 @@ libguestfs_la_SOURCES = \ structs-copy.c \ structs-free.c \ tmpdirs.c \ + umask.c \ whole-file.c \ libguestfs.syms diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index 61f384c..bf107d0 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -915,4 +915,7 @@ void guestfs_int_init_unix_backend (void) __attribute__((constructor)); /* guid.c */ extern int guestfs_int_validate_guid (const char *); +/* umask.c */ +extern int guestfs_int_getumask (guestfs_h *g); + #endif /* GUESTFS_INTERNAL_H_ */ diff --git a/src/launch.c b/src/launch.c index a4326fe..460ed29 100644 --- a/src/launch.c +++ b/src/launch.c @@ -50,8 +50,6 @@ static struct backend { const struct backend_ops *ops; } *backends = NULL; -static mode_t get_umask (guestfs_h *g); - int guestfs_impl_launch (guestfs_h *g) { @@ -75,6 +73,7 @@ guestfs_impl_launch (guestfs_h *g) guestfs_version (g); struct backend *b; CLEANUP_FREE char *backend = guestfs_get_backend (g); + int mask; debug (g, "launch: program=%s", g->program); if (STRNEQ (g->identifier, "")) @@ -87,7 +86,9 @@ guestfs_impl_launch (guestfs_h *g) debug (g, "launch: backend=%s", backend); debug (g, "launch: tmpdir=%s", g->tmpdir); - debug (g, "launch: umask=0%03o", get_umask (g)); + mask = guestfs_int_getumask (g); + if (mask >= 0) + debug (g, "launch: umask=0%03o", (unsigned) mask); debug (g, "launch: euid=%ju", (uintmax_t) geteuid ()); } @@ -475,40 +476,6 @@ guestfs_int_create_socketname (guestfs_h *g, const char *filename, } /** - * glibc documents, but does not actually implement, a L<getumask(3)> - * call. - * - * This function implements a thread-safe way to get the umask. Note - * this is only called when C<g-E<gt>verbose> is true and after - * C<g-E<gt>tmpdir> has been created. - */ -static mode_t -get_umask (guestfs_h *g) -{ - mode_t ret; - int fd; - struct stat statbuf; - CLEANUP_FREE char *filename = safe_asprintf (g, "%s/umask-check", g->tmpdir); - - fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY|O_CLOEXEC, 0777); - if (fd == -1) - return -1; - - if (fstat (fd, &statbuf) == -1) { - close (fd); - return -1; - } - - close (fd); - - ret = statbuf.st_mode; - ret &= 0777; - ret = ret ^ 0777; - - return ret; -} - -/** * When the library is loaded, each backend calls this function to * register itself in a global list. */ diff --git a/src/umask.c b/src/umask.c new file mode 100644 index 0000000..3fc7bc6 --- /dev/null +++ b/src/umask.c @@ -0,0 +1,110 @@ +/* libguestfs + * Copyright (C) 2016 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/** + * Return current umask in a thread-safe way. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <fcntl.h> +#include <errno.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <sys/wait.h> + +#include "ignore-value.h" + +#include "guestfs.h" +#include "guestfs-internal.h" + +/** + * glibc documents, but does not actually implement, a L<getumask(3)> + * call. + * + * This function implements an expensive, but thread-safe way to get + * the current process's umask. + * + * Returns the current process's umask. On failure, returns C<-1> and + * sets the error in the guestfs handle. + * + * Thanks to: Josh Stone, Jiri Jaburek, Eric Blake. + */ +int +guestfs_int_getumask (guestfs_h *g) +{ + pid_t pid; + int fd[2], r; + int mask; + int status; + + r = pipe2 (fd, O_CLOEXEC); + if (r == -1) { + perrorf (g, "pipe2"); + return -1; + } + + pid = fork (); + if (pid == -1) { + perrorf (g, "fork"); + close (fd[0]); + close (fd[1]); + return -1; + } + if (pid == 0) { + /* The child process must ONLY call async-safe functions. */ + close (fd[0]); + + /* umask can't fail. */ + mask = umask (0); + + if (write (fd[1], &mask, sizeof mask) != sizeof mask) + _exit (EXIT_FAILURE); + if (close (fd[1]) == -1) + _exit (EXIT_FAILURE); + + _exit (EXIT_SUCCESS); + } + + /* Parent. */ + close (fd[1]); + + /* Read the umask. */ + if (read (fd[0], &mask, sizeof mask) != sizeof mask) { + perrorf (g, "read"); + close (fd[0]); + return -1; + } + close (fd[0]); + + again: + if (waitpid (pid, &status, 0) == -1) { + if (errno == EINTR) goto again; + perrorf (g, "waitpid"); + return -1; + } + else if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) { + guestfs_int_external_command_failed (g, status, "umask", NULL); + return -1; + } + + return mask; +} -- 2.7.4

2 7

[PATCH] Add safe wrapper around waitpid which deals with EINTR correctly.
by Richard W.M. Jones 14 Apr '16

14 Apr '16

As Eric Blake noted in: https://www.redhat.com/archives/libguestfs/2016-April/msg00154.html libguestfs doesn't correctly handle the case where waitpid receives a SIGCHLD signal and the main program has registered a non-restartable signal handler. In this case waitpid would return -EINTR and we would print an error, but actually we should retry this case. This adds two new internal functions, one for handling waitpid and printing (real) errors from it, the other for just waiting for a process where we don't particularly care about errors. Rich.

2 2

More posix_fadvise stuff.
by Richard W.M. Jones 14 Apr '16

14 Apr '16

More posix_fadvise stuff, and document what Linux really does with these calls. Also fixes a nasty bug in virt-builder. Rich.

1 3