December 2017 - Libguestfs - libguestfs List Archives

[PATCH] v2v: -o null: Use the qemu null device driver.
by Richard W.M. Jones 07 Dec '17

07 Dec '17

Instead of writing to a temporary file and deleting it, use the null block driver in qemu to throw it away. --- v2v/output_null.ml | 47 +++++++++++++++++++++++++++++++++-------------- v2v/virt-v2v.pod | 4 ---- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/v2v/output_null.ml b/v2v/output_null.ml index d01f45654..4d06aa0de 100644 --- a/v2v/output_null.ml +++ b/v2v/output_null.ml @@ -26,16 +26,23 @@ open Common_gettext.Gettext open Types open Utils +(* Notes: + * + * This only happens to work because we run qemu-img convert + * with the -n [no create output] option, since null-co doesn't + * support creation. If -n is removed in the main program then + * the tests will break very obviously. + * + * The null-co device is not zero-sized. It actually has a fixed + * size (defaults to 2^30 I believe). + * + * qemu-img convert checks the output size and will fail if it's + * too small, so we have to set the size. We could set it to + * match the input size but it's easier to set it to some huge + * size instead. + *) + class output_null = - (* It would be nice to be able to write to /dev/null. - * Unfortunately qemu-img convert cannot do that. Instead create a - * temporary directory which is always deleted at exit. - *) - let tmpdir = - let base_dir = (open_guestfs ())#get_cachedir () in - let t = Mkdtemp.temp_dir ~base_dir "null." in - rmdir_on_exit t; - t in object inherit output @@ -44,13 +51,25 @@ object method supported_firmware = [ TargetBIOS; TargetUEFI ] method prepare_targets source targets = - List.map ( - fun t -> - let target_file = tmpdir // t.target_overlay.ov_sd in - { t with target_file = target_file } - ) targets + let json_params = [ + "file.driver", JSON.String "null-co"; + "file.size", JSON.String "1E"; + ] in + let target_file = "json:" ^ JSON.string_of_doc json_params in + (* XXX It's not really right to set target_format here, but + * it works. + *) + let target_format = "raw" in + List.map (fun t -> { t with target_file; target_format }) targets method create_metadata _ _ _ _ _ _ = () + + (* Stops the main program from calling g#disk_create to try to create + * the null device. + *) + method disk_create ?backingfile ?backingformat ?preallocation ?compat + ?clustersize path format size = + () end let output_null () = new output_null diff --git a/v2v/virt-v2v.pod b/v2v/virt-v2v.pod index aad06ead0..4ac44988e 100644 --- a/v2v/virt-v2v.pod +++ b/v2v/virt-v2v.pod @@ -1901,10 +1901,6 @@ This temporarily places a full copy of the output disks in C<$TMPDIR>. You must ensure there is sufficient space in the output directory for the converted guest. -=item I<-o null> - -This temporarily places a full copy of the output disks in C<$TMPDIR>. - =back See also L</Minimum free space check in the host> below. -- 2.13.2

1 1

v2v: vddk: Switch to using ‘-it vddk’ to specify VDDK as input transport.
by Richard W.M. Jones 07 Dec '17

07 Dec '17

Proposed small change to the command line of virt-v2v when specifying that you want VDDK mode. Rich.

1 1

[PATCH] builder: use the template arch when caching all templates
by Pino Toscano 07 Dec '17

07 Dec '17

When caching all the templates, use the architecture of each template, instead of the architecture passed as --arch (or the host architecture, as default). This way, the right destination filename will be used. Fixes commit b1cf6246f3c80762cf27dbdb24168589a34daf00. Thanks to: Erik Skultety. --- builder/Makefile.am | 2 + builder/builder.ml | 4 +- builder/test-virt-builder-cacheall.sh | 88 +++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 2 deletions(-) create mode 100755 builder/test-virt-builder-cacheall.sh diff --git a/builder/Makefile.am b/builder/Makefile.am index 979726bff..4be9bd617 100644 --- a/builder/Makefile.am +++ b/builder/Makefile.am @@ -32,6 +32,7 @@ EXTRA_DIST = \ test-simplestreams/streams/v1/net.cirros-cloud_released_download.json \ test-virt-builder.sh \ test-docs.sh \ + test-virt-builder-cacheall.sh \ test-virt-builder-list.sh \ test-virt-builder-list-simplestreams.sh \ test-virt-builder-planner.sh \ @@ -386,6 +387,7 @@ index_parser_tests_LINK = \ TESTS = \ test-docs.sh \ + test-virt-builder-cacheall.sh \ test-virt-builder-list.sh \ test-virt-index-validate.sh \ $(SLOW_TESTS) diff --git a/builder/builder.ml b/builder/builder.ml index 41c0a4ccc..c84352fec 100644 --- a/builder/builder.ml +++ b/builder/builder.ml @@ -251,8 +251,8 @@ let main () = | Some _ -> List.iter ( fun (name, - { Index.revision; file_uri; proxy }) -> - let template = name, Index.Arch cmdline.arch, revision in + { Index.revision; file_uri; proxy; arch }) -> + let template = name, arch, revision in message (f_"Downloading: %s") file_uri; let progress_bar = not (quiet ()) in ignore (Downloader.download downloader ~template ~progress_bar diff --git a/builder/test-virt-builder-cacheall.sh b/builder/test-virt-builder-cacheall.sh new file mode 100755 index 000000000..c80d9ecd2 --- /dev/null +++ b/builder/test-virt-builder-cacheall.sh @@ -0,0 +1,88 @@ +#!/bin/bash - +# libguestfs virt-builder test script +# Copyright (C) 2017 Red Hat Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program 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 General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +set -e + +$TEST_FUNCTIONS +skip_if_skipped + +tmpdir="$(mktemp -d)" +echo "tmpdir= $tmpdir" +reposdir="$tmpdir/virt-builder/repos.d" +repodir="$tmpdir/repo" +indexfile="$repodir/index" +cachedir="$tmpdir/cache" + +mkdir -p "$reposdir" +mkdir -p "$repodir" + +# Create some fake images. +img1_path="$repodir/img1.raw" +img1_size=10485760 # 10G +qemu-img create -f raw "$img1_path" $img1_size +img1_csum=`do_sha256 "$img1_path"` + +img2_path="$repodir/img2.qcow2" +img2_size=5242880 # 5G +qemu-img create -f qcow2 "$img2_path" $img2_size +img2_csum=`do_sha256 "$img2_path"` + +# Create an index for the images. +cat > "$indexfile" <<EOF +[img1] +name=img1 +file=$(basename "$img1_path") +arch=x86_64 +size=$img1_size +checksum[sha512]=$img1_csum +revision=1 + +[img2] +name=img2 +file=$(basename "$img2_path") +arch=aarch64 +size=$img2_size +checksum[sha512]=$img2_csum +revision=3 +EOF + +# Create the repository. +cat > "$reposdir/repo1.conf" <<EOF +[repo1] +uri=$indexfile +EOF + +export XDG_CONFIG_HOME= +export XDG_CONFIG_DIRS="$tmpdir" +export XDG_CACHE_HOME="$cachedir" + +short_list=$($VG virt-builder --no-check-signature --no-cache --list) + +if [ "$short_list" != "img1 x86_64 img1 +img2 aarch64 img2" ]; then + echo "$0: unexpected --list output:" + echo "$short_list" + exit 1 +fi + +$VG virt-builder --no-check-signature --cache-all-templates +ls -lh "$cachedir/virt-builder" +test -f "$cachedir/virt-builder/img1.x86_64.1" +test -f "$cachedir/virt-builder/img2.aarch64.3" + +rm -rf "$tmpdir" -- 2.14.3

2 1

[PATCH] lib: libvirt: stop using <shareable/> for appliance disk (RHBZ#1518517)
by Pino Toscano 05 Dec '17

05 Dec '17

Commit aa9e0057b19e29f76c9a81f9aebeeb1cb5bf1fdb made the libvirt backend use <shareable/> for the disk of the appliance, since at that time all the instances were using the disk directly. OTOH, commit 3ad44c866042919374e2d840502e53da2ed8aef0 switched to overlays for read-only disks, including the appliance, so effectively protecting the appliance. Because of this, the libvirt backend does not need <shareable/> anymore. Thanks to: Daniel Berrange, Richard W.M. Jones, Peter Krempa. --- lib/launch-libvirt.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c index e5121799e..5aae1b611 100644 --- a/lib/launch-libvirt.c +++ b/lib/launch-libvirt.c @@ -985,10 +985,6 @@ static int construct_libvirt_xml_appliance (guestfs_h *g, const struct libvirt_x } \ } while (0) -/* <element/> */ -#define empty_element(element) \ - do { start_element(element) {} end_element (); } while (0) - /* key=value attribute of the current element. */ #define attribute(key,value) \ if (xmlTextWriterWriteAttribute (xo, BAD_CAST (key), BAD_CAST (value)) == -1){ \ @@ -1787,8 +1783,6 @@ construct_libvirt_xml_appliance (guestfs_h *g, == -1) return -1; - empty_element ("shareable"); - } end_element (); return 0; -- 2.14.3

2 1

virt-builder: --ssh-inject fails because it is executed before --firstboot-command?
by Guido Schmidt 05 Dec '17

05 Dec '17

Version 1.34.6 (on Debian Stretch) Using virt-builder I tried to create a user and inject my ssh key: virt-builder -v -x centos-7.4 --output testimage --format qcow2 --selinux-relabel --hostname testimage --firstboot-command 'useradd -m -p "" dummy' --firstboot-command 'chage -d 0 dummy' --ssh-inject dummy ... [ 16.1] Installing firstboot command: useradd -m -p "" dummy [ 16.1] Installing firstboot command: chage -d 0 dummy [ 16.1] SSH key inject: dummy virt-builder: error: ssh-inject: the user dummy does not exist on the guest (Full debug output attached) Seems like this can never work as the injection is carried out before the first boot. Is this a bug in my version? Or is this limited to users that are already in the image? Guido

2 1

[nbdkit PATCH] nbd: Fix sporadic use-after-free
by Eric Blake 04 Dec '17

04 Dec '17

Now that we properly clean up 'trans' in the reader thread, we must not dereference 'trans' from the write thread at any point after trans has been added to the list unless we have grabbed it back off the list ourselves, or we risk an occasional use-after-free or even double free that valgrind can detect. Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Fixes: cb189648f11df6c4f7287dcaed4bc856650e2c3b Signed-off-by: Eric Blake <eblake(a)redhat.com> --- Perhaps I should also add more comments to the code about transfer of ownership of 'trans' between threads? plugins/nbd/nbd.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index e79042c..9d40e87 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -227,6 +227,26 @@ nbd_mark_dead (struct handle *h) return -1; } +/* Find and remove the transaction corresponding to cookie from the list. */ +static struct transaction * +find_trans_by_cookie (struct handle *h, uint64_t cookie) +{ + struct transaction **ptr; + struct transaction *trans; + + nbd_lock (h); + ptr = &h->trans; + while ((trans = *ptr) != NULL) { + if (cookie == trans->u.cookie) + break; + ptr = &trans->next; + } + if (trans) + *ptr = trans->next; + nbd_unlock (h); + return trans; +} + /* Send a request, return 0 on success or -1 on write failure. */ static int nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset, @@ -260,6 +280,8 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t offset, { int err; struct transaction *trans; + int fd; + uint64_t cookie; trans = calloc (1, sizeof *trans); if (!trans) { @@ -282,9 +304,14 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t offset, } trans->next = h->trans; h->trans = trans; + fd = trans->u.fds[0]; + cookie = trans->u.cookie; nbd_unlock (h); - if (nbd_request_raw (h, type, offset, count, trans->u.cookie, req_buf) == 0) - return trans->u.fds[0]; + if (nbd_request_raw (h, type, offset, count, cookie, req_buf) == 0) + return fd; + trans = find_trans_by_cookie (h, cookie); + if (!trans) + return nbd_mark_dead (h); err: err = errno; @@ -309,7 +336,6 @@ static int nbd_reply_raw (struct handle *h, int *fd) { struct reply rep; - struct transaction **ptr; struct transaction *trans; void *buf; uint32_t count; @@ -320,16 +346,7 @@ nbd_reply_raw (struct handle *h, int *fd) if (be32toh (rep.magic) != NBD_REPLY_MAGIC) return nbd_mark_dead (h); nbdkit_debug ("received reply for cookie %#" PRIx64, rep.handle); - nbd_lock (h); - ptr = &h->trans; - while ((trans = *ptr) != NULL) { - if (rep.handle == trans->u.cookie) - break; - ptr = &trans->next; - } - if (trans) - *ptr = trans->next; - nbd_unlock (h); + trans = find_trans_by_cookie (h, rep.handle); if (!trans) { nbdkit_error ("reply with unexpected cookie %#" PRIx64, rep.handle); return nbd_mark_dead (h); -- 2.14.3

2 1

[nbdkit PATCH v2] nbd: Fix memory leak
by Eric Blake 02 Dec '17

02 Dec '17

When converting from a single transaction to a linked list, I forgot to free the storage for each member of the list. Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Fixes: 7f5bb9bf13f041ea7702bda557d9dd668bc3423a Signed-off-by: Eric Blake <eblake(a)redhat.com> --- plugins/nbd/nbd.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index b844bf5..e79042c 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -311,6 +311,8 @@ nbd_reply_raw (struct handle *h, int *fd) struct reply rep; struct transaction **ptr; struct transaction *trans; + void *buf; + uint32_t count; *fd = -1; if (read_full (h->fd, &rep, sizeof rep) < 0) @@ -334,9 +336,12 @@ nbd_reply_raw (struct handle *h, int *fd) } *fd = trans->u.fds[1]; + buf = trans->buf; + count = trans->count; + free (trans); switch (be32toh (rep.error)) { case NBD_SUCCESS: - if (trans->buf && read_full (h->fd, trans->buf, trans->count) < 0) + if (buf && read_full (h->fd, buf, count) < 0) return nbd_mark_dead (h); return 0; case NBD_EPERM: @@ -399,6 +404,7 @@ nbd_reader (void *handle) abort (); } close (trans->u.fds[1]); + free (trans); } return NULL; } -- 2.14.3

2 1

[nbdkit PATCH] nbd: Fix memory leak
by Eric Blake 02 Dec '17

02 Dec '17

When converting from a single transaction to a linked list, I forgot to free the storage for each member of the list. Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Fixes: 7f5bb9bf13f041ea7702bda557d9dd668bc3423a Signed-off-by: Eric Blake <eblake(a)redhat.com> --- I'm still not sure why 'make check' passes while 'make check-valgrind' fails for TESTS=test-nbd, but this at least avoids the memory leak. plugins/nbd/nbd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index b844bf5..425abe4 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -334,6 +334,7 @@ nbd_reply_raw (struct handle *h, int *fd) } *fd = trans->u.fds[1]; + free (trans); switch (be32toh (rep.error)) { case NBD_SUCCESS: if (trans->buf && read_full (h->fd, trans->buf, trans->count) < 0) @@ -399,6 +400,7 @@ nbd_reader (void *handle) abort (); } close (trans->u.fds[1]); + free (trans); } return NULL; } -- 2.14.3

2 2

[PATCH nbdkit nbd] nbd: Unsuccessful attempt to fix memory leak.
by Richard W.M. Jones 02 Dec '17

02 Dec '17

Hi Eric, There's a memory leak in the nbd client. The message (below) is not very useful because somehow debuginfo is missing in the plugin. However it's easily reproducible by doing: make check-valgrind TESTS=test-nbd I tried the attached patch to fix what I thought was the bug, but sadly the fix doesn't work for me :-( So I guess something else is going on, but it does look as if the transactions list also needs to be freed. Rich. ==20307== ==20307== HEAP SUMMARY: ==20307== in use at exit: 2,267 bytes in 23 blocks ==20307== total heap usage: 2,298 allocs, 2,275 frees, 307,606 bytes allocated ==20307== ==20307== 96 bytes in 3 blocks are definitely lost in loss record 4 of 10 ==20307== at 0x4C2FA1E: calloc (vg_replace_malloc.c:711) ==20307== by 0x77EBBAF: ??? ==20307== by 0x77EBD17: ??? ==20307== by 0x40595C: handle_request (connections.c:894) ==20307== by 0x40595C: recv_request_send_reply (connections.c:1061) ==20307== by 0x405AE7: connection_worker (connections.c:200) ==20307== by 0x55DC55A: start_thread (in /usr/lib64/libpthread-2.26.9000.so) ==20307== by 0x58E85AE: clone (in /usr/lib64/libc-2.26.9000.so) ==20307== ==20307== 320 bytes in 10 blocks are definitely lost in loss record 9 of 10 ==20307== at 0x4C2FA1E: calloc (vg_replace_malloc.c:711) ==20307== by 0x77EBBAF: ??? ==20307== by 0x77EBD98: ??? ==20307== by 0x405986: handle_request (connections.c:884) ==20307== by 0x405986: recv_request_send_reply (connections.c:1061) ==20307== by 0x405AE7: connection_worker (connections.c:200) ==20307== by 0x55DC55A: start_thread (in /usr/lib64/libpthread-2.26.9000.so) ==20307== by 0x58E85AE: clone (in /usr/lib64/libc-2.26.9000.so) ==20307== ==20307== LEAK SUMMARY: ==20307== definitely lost: 416 bytes in 13 blocks ==20307== indirectly lost: 0 bytes in 0 blocks ==20307== possibly lost: 0 bytes in 0 blocks ==20307== still reachable: 411 bytes in 5 blocks ==20307== suppressed: 1,440 bytes in 5 blocks ==20307== Reachable blocks (those to which a pointer was found) are not shown. ==20307== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==20307== ==20307== For counts of detected and suppressed errors, rerun with: -v ==20307== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 3 from 3)

2 2

[PATCH] v2v: bootloaders: handle no default grubby kernel (RHBZ#1519204)
by Pino Toscano 02 Dec '17

02 Dec '17

When using grubby to get the default kernel of a guest, do not fail with a bogus error like: virt-v2v: error: libguestfs error: statns: statns_stub: path must start with a / character in case there is no default kernel that can be determined (e.g. because of a bogus configuration). --- v2v/linux_bootloaders.ml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/v2v/linux_bootloaders.ml b/v2v/linux_bootloaders.ml index 8de3f0469..b820f731f 100644 --- a/v2v/linux_bootloaders.ml +++ b/v2v/linux_bootloaders.ml @@ -239,7 +239,10 @@ object (self) let res = match get_default_method with | MethodGrubby -> - Some (g#command [| "grubby"; "--default-kernel" |]) + let res = g#command [| "grubby"; "--default-kernel" |] in + (match res with + | "" -> None + | _ -> Some res) | MethodPerlBootloader -> let cmd = [| "/usr/bin/perl"; "-MBootloader::Tools"; "-e"; " -- 2.14.3

2 1