Proposal to remove two virt-v2v command line options
by Richard W.M. Jones
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
8 years
[PATCH 1/2] sparsify: Refactor handling of checks of copying mode / --in-place.
by Richard W.M. Jones
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
8 years
[PATCH] dib: Rewrite match statement as ordinary if statement.
by Richard W.M. Jones
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
8 years
virt-builder and SimpleStreams
by Richard W.M. Jones
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/
8 years
[PATCH] launch: direct: specify format for appliance drive
by Pino Toscano
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
8 years
[PATCH 1/2] valgrind: Use --trace-children=no --child-silent-after-fork=yes
by Richard W.M. Jones
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
8 years
[PATCH v3 libguestfs] launch: Implement a safer getumask.
by Richard W.M. Jones
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
8 years
More posix_fadvise stuff.
by Richard W.M. Jones
More posix_fadvise stuff, and document what Linux really does
with these calls.
Also fixes a nasty bug in virt-builder.
Rich.
8 years
