[V2V PATCH v3 0/6] Bring support for virtio-scsi back to Windows
by Andrey Drobyshev
Discussion on v2:
https://listman.redhat.com/archives/libguestfs/2023-March/030987.html
v2 -> v3:
* Patch 2/6 ("convert_windows: add Inject_virtio_win.Virtio_SCSI as a
possible block type"): omit "Inject_virtio_win." prefix in favor of type
inference. Add a short commit message body;
* Add tests/test-v2v-block-driver.sh testing the new "--block-driver"
option.
Discussion on v1:
https://listman.redhat.com/archives/libguestfs/2023-February/030849.html
https://listman.redhat.com/archives/libguestfs/2023-March/030917.html
v1 -> v2:
* Adapt the patch suggested by Richard, splitting it up into 3:
https://listman.redhat.com/archives/libguestfs/2023-March/030975.html
Now we have "--block-driver" command line option which regulates the
order in which block drivers are being searched for (and, as a
consequence, the default driver).
Andrey Drobyshev (3):
Revert "Remove guestcaps_block_type Virtio_SCSI"
convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block
type
tests: add --block-driver option test
Richard W.M. Jones (3):
convert: introduce "block_driver" convert option
convert_windows: set block driver priority according to block_driver
option
v2v, in-place: introduce --block-driver command line option
convert/convert.ml | 11 ++-
convert/convert.mli | 1 +
convert/convert_linux.ml | 11 ++-
convert/convert_linux.mli | 3 +-
convert/convert_windows.ml | 11 ++-
convert/convert_windows.mli | 3 +-
convert/target_bus_assignment.ml | 1 +
docs/virt-v2v-in-place.pod | 10 ++
docs/virt-v2v.pod | 10 ++
in-place/in_place.ml | 12 ++-
inspector/inspector.ml | 3 +-
lib/create_ovf.ml | 1 +
lib/types.ml | 3 +-
lib/types.mli | 2 +-
output/openstack_image_properties.ml | 7 ++
tests/Makefile.am | 2 +
tests/test-v2v-block-driver.sh | 143 +++++++++++++++++++++++++++
v2v/v2v.ml | 12 ++-
18 files changed, 231 insertions(+), 15 deletions(-)
create mode 100755 tests/test-v2v-block-driver.sh
--
2.31.1
2 years, 6 months
[libguestfs-common PATCH 0/2] detect_kernels: deal with RHEL's kernel-core / kernel-modules-core split
by Laszlo Ersek
https://bugzilla.redhat.com/show_bug.cgi?id=2175703
Vera Wu's testing covered a backport of this series to the virt-v2v
project's rhel-9.2 branch (on top of commit 86517b17be98), where the
"detect_kernels" function is still unique/internal to the virt-v2v
project. The backport / cherry-pick from libguestfs-common to virt-v2v
cannot be automated, but it's not difficult; only file names in the
patch files change.
The first patch is split out only in order not to muddy the second
patch; the second patch requires some eyeballing anyway, so keeping it
focused is good.
Laszlo
Laszlo Ersek (2):
detect_kernels: tighten "try" scope
detect_kernels: deal with RHEL's kernel-core / kernel-modules-core
split
mldrivers/linux_kernels.ml | 26 ++++++++++++++------
1 file changed, 19 insertions(+), 7 deletions(-)
base-commit: 402f7600d7077cc0c60b75cfc72413af93dc4e6e
2 years, 7 months
[nbdkit PATCH 0/2] various
by Laszlo Ersek
I originally meant to post only the "vector.h" patch, but then
(independently) nbdkit wouldn't build. Hence the other (rust plugin)
patch.
Laszlo
Laszlo Ersek (2):
common/utils: document empty_vector compound literal assignment
plugins/rust: restrict predicates-{tree,core} to {1.0.7,1.0.5}
common/utils/vector.h | 8 +++++++-
plugins/rust/Cargo.toml | 2 ++
2 files changed, 9 insertions(+), 1 deletion(-)
base-commit: 45b72f5bd8fc1b475fa130d06c86cd877bf595d5
2 years, 7 months
[libnbd PATCH v5 0/4] pass LISTEN_FDNAMES with systemd socket activation
by Laszlo Ersek
V4 was here (incorrectly versioned on the mailing list as v3):
<http://mid.mail-archive.com/20230323121016.1442655-1-lersek@redhat.com>.
See the Notes section on each patch for the v5 updates.
Laszlo Ersek (2):
socket activation: generalize environment construction
socket activation: set LISTEN_FDNAMES
Richard W.M. Jones (2):
common/include: Copy ascii-ctype functions from nbdkit
generator: Add APIs to get/set the socket activation socket name
.gitignore | 1 +
common/include/Makefile.am | 6 +
common/include/ascii-ctype.h | 75 +++++++++
common/include/test-ascii-ctype.c | 88 ++++++++++
generator/API.ml | 50 ++++++
generator/states-connect-socket-activation.c | 170 ++++++++++++++++----
lib/handle.c | 56 +++++++
lib/internal.h | 1 +
8 files changed, 412 insertions(+), 35 deletions(-)
create mode 100644 common/include/ascii-ctype.h
create mode 100644 common/include/test-ascii-ctype.c
base-commit: a48a1142bc54b09dbd9ed45cc9f9f4945f8174ef
2 years, 7 months
[libnbd PATCH v3 00/19] pass LISTEN_FDNAMES with systemd socket activation
by Laszlo Ersek
V3 was here:
<http://mid.mail-archive.com/20230215141158.2426855-1-lersek@redhat.com>.
See the Notes section on each patch for the v4 updates.
The series is nearly ready for merging: every patch has at least one R-b
tag, except "socket activation: avoid manipulating the sign bit".
The series builds, and passes "make check" and "make check-valgrind", at
every stage.
Thanks for reviewing!
Laszlo
Laszlo Ersek (17):
socket activation: fix error message upon asprintf() failure
socket activation: clean up responsibilities of prep.sock.act.env.()
socket activation: avoid manipulating the sign bit
socket activation: check syscalls for errors in the child process
socket activation: centralize resource release
socket activation: plug AF_UNIX socket address (filesystem) leak on
error
socket activation: replace execvp() call with fork-safe variant
CONNECT_COMMAND.START: fix small comment thinko about socket pair
usage
CONNECT_COMMAND.START: set the NBD error when fcntl() fails
CONNECT_COMMAND.START: use symbolic constants for fd#0 and fd#1
CONNECT_COMMAND.START: sanitize close() calls in the child process
CONNECT_COMMAND.START: check syscalls for errors in the child process
CONNECT_COMMAND.START: centralize resource release
CONNECT_COMMAND.START: plug child process leak on error
CONNECT_COMMAND.START: replace execvp() call with fork-safe variant
socket activation: generalize environment construction
socket activation: set LISTEN_FDNAMES
Richard W.M. Jones (2):
common/include: Copy ascii-ctype functions from nbdkit
generator: Add APIs to get/set the socket activation socket name
lib/internal.h | 1 +
common/include/ascii-ctype.h | 75 +++++
generator/API.ml | 49 ++++
generator/states-connect-socket-activation.c | 287 ++++++++++++++------
generator/states-connect.c | 123 ++++++---
lib/handle.c | 56 ++++
common/include/Makefile.am | 6 +
common/include/test-ascii-ctype.c | 88 ++++++
.gitignore | 1 +
9 files changed, 570 insertions(+), 116 deletions(-)
create mode 100644 common/include/ascii-ctype.h
create mode 100644 common/include/test-ascii-ctype.c
base-commit: 9075f68ffc8bed320d0d1d46f1f0456d10626878
2 years, 7 months
Re: [Libguestfs] [PATCH 1/1] nbd/server: push pending frames after sending reply
by Eric Blake
On Fri, Mar 24, 2023 at 11:47:20AM +0100, Florian Westphal wrote:
> qemu-nbd doesn't set TCP_NODELAY on the tcp socket.
>
> Kernel waits for more data and avoids transmission of small packets.
> Without TLS this is barely noticeable, but with TLS this really shows.
>
> Booting a VM via qemu-nbd on localhost (with tls) takes more than
> 2 minutes on my system. tcpdump shows frequent wait periods, where no
> packets get sent for a 40ms period.
Thank you for this analysis.
>
> Add explicit (un)corking when processing (and responding to) requests.
> "TCP_CORK, &zero" after earlier "CORK, &one" will flush pending data.
>
> VM Boot time:
> main: no tls: 23s, with tls: 2m45s
> patched: no tls: 14s, with tls: 15s
>
> VM Boot time, qemu-nbd via network (same lan):
> main: no tls: 18s, with tls: 1m50s
> patched: no tls: 17s, with tls: 18s
And the timings bear proof that it matters.
>
> Future optimization: if we could detect if there is another pending
> request we could defer the uncork operation because more data would be
> appended.
nbdkit and libnbd do this with the MSG_MORE flag (plaintext) and TLS
corking (tls); when building up a message to the other side, a flag is
set any time we know we are likely to send more data very shortly.
nbdkit wraps it under a flag SEND_MORE, which applies to both plaintext:
https://gitlab.com/nbdkit/nbdkit/-/blob/master/server/connections.c#L415
and to TLS connections:
https://gitlab.com/nbdkit/nbdkit/-/blob/master/server/crypto.c#L396
while libnbd uses MSG_MORE a bit more directly for the same purpose
for plaintext, but isn't (yet) doing TLS corking:
https://gitlab.com/nbdkit/libnbd/-/blob/master/generator/states-issue-com...
https://gitlab.com/nbdkit/libnbd/-/blob/master/lib/internal.h#L57
I would love to see a future patch to qio_channel code to support
MSG_MORE in the same way as nbdkit is using its SEND_MORE flag, as the
caller often has more info on whether it is sending a short prefix or
is done with a conceptual message and ready to uncork, and where the
use of a flag can be more efficient than separate passes through
cork/uncork calls. But even your initial work at properly corking is
a good step in the right direction.
And surprisingly, qemu IS using corking on the client side:
https://gitlab.com/qemu-project/qemu/-/blob/master/block/nbd.c#L525
just not on the server side, before your patch.
>
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
> nbd/server.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index a4750e41880a..848836d41405 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2667,6 +2667,8 @@ static coroutine_fn void nbd_trip(void *opaque)
> goto disconnect;
> }
>
> + qio_channel_set_cork(client->ioc, true);
> +
> if (ret < 0) {
> /* It wasn't -EIO, so, according to nbd_co_receive_request()
> * semantics, we should return the error to the client. */
> @@ -2692,6 +2694,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> goto disconnect;
> }
>
> + qio_channel_set_cork(client->ioc, false);
Reviewed-by: Eric Blake <eblake(a)redhat.com>
> done:
> nbd_request_put(req);
> nbd_client_put(client);
> --
> 2.39.2
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
2 years, 7 months
[libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
by Laszlo Ersek
This is version 4 of the following sub-series:
[libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()
[libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe execvpe()
http://mid.mail-archive.com/20230215141158.2426855-10-lersek@redhat.com
http://mid.mail-archive.com/20230215141158.2426855-11-lersek@redhat.com
The Notes section on each patch records the updates for that patch.
For assisting with incremental review, here's a range-diff:
> 1: c5f89eaa0aaf ! 1: 2a3c95d0701f lib/utils: introduce async-signal-safe execvpe()
> @@ Commit message
> not to pass it, per APPLICATION USAGE [09], but on Linux/glibc, O_EXEC
> does not seem supported, only O_PATH does [10].
>
> - Thus the chosen approach -- pre-generate filenames -- contains a small
> + Thus the chosen approach -- pre-generate filenames -- contains a small
> TOCTTOU race (highlighted by Eric) after all, but it should be harmless.
>
> Implementation-defined details:
> @@ Commit message
>
> If PATH is set but empty ("set to null") [02], or PATH is unset and
> confstr(_CS_PATH) fails or returns no information or returns the empty
> - string, we fail the initial scanning (!) with ENOENT. This is consistent
> - with bash's behavior on Linux/glibc:
> -
> - $ PATH= ls
> - bash: ls: No such file or directory
> + string, we fail the initial scanning (!) with ENOENT.
>
> Details chosen for unspecified behavior:
>
> @@ Commit message
>
> Suggested-by: Eric Blake <eblake(a)redhat.com>
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> + Reviewed-by: Eric Blake <eblake(a)redhat.com>
> + Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
> +
> +
> + ## Notes ##
> + v4:
> +
> + - remove double space typo from commit message [Eric]
> +
> + - remove the allusion to bash compatibility in the
> + "Implementation-defined details" part of the commit message, where the
> + latter discusses PATH being "set to null" [Eric]
> +
> + - pick up R-b's from Eric and Rich
> +
> + - keep the #include "..." list sorted -- #include "checked-overflow.h"
> + above "minmax.h", not below it
> +
> + - in get_path(), replace the FIXME comments with notes that explain why
> + we don't lock the environment [Eric]
>
> ## lib/internal.h ##
> @@ lib/internal.h: struct command {
> @@ lib/internal.h: extern void nbd_internal_fork_safe_assert (int result, const cha
>
> ## lib/utils.c ##
> @@
> - #include <limits.h>
> + #include <sys/uio.h>
>
> - #include "minmax.h"
> + #include "array-size.h"
> +#include "checked-overflow.h"
> + #include "minmax.h"
>
> #include "internal.h"
> -
> @@ lib/utils.c: nbd_internal_fork_safe_assert (int result, const char *file, long line,
> - xwrite (STDERR_FILENO, "' failed.\n", 10);
> + assertion, "' failed.\n", (char *)NULL);
> abort ();
> }
> +
> @@ lib/utils.c: nbd_internal_fork_safe_assert (int result, const char *file, long l
> + bool env_path_found;
> + size_t path_size, path_size2;
> +
> -+ /* FIXME: lock the environment. */
> ++ /* Note: per POSIX, here we should lock the environment, even just for
> ++ * getenv(). However, glibc and any other high-quality libc will not be
> ++ * modifying "environ" during getenv(), and no sane application should modify
> ++ * the environment after launching threads.
> ++ */
> + path = getenv ("PATH");
> + if ((env_path_found = (path != NULL)))
> + path = strdup (path);
> -+ /* FIXME: unlock the environment. */
> ++ /* This is where we'd unlock the environment. */
> +
> + if (env_path_found) {
> + /* This handles out-of-memory as well. */
> 2: e8fba75ecf93 ! 2: 647a46b965c4 lib/utils: add unit tests for async-signal-safe execvpe()
> @@ Commit message
> nbd_internal_fork_safe_execvpe().
>
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> + Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
> + Reviewed-by: Eric Blake <eblake(a)redhat.com>
> +
> +
> + ## Notes ##
> + v4:
> +
> + - pick up R-b's from Rich and Eric
> +
> + - "errors.c" makes the test case dependent on pthread_getspecific(), so
> + reflect Eric's commit 742cbd8c7adc ("lib: Use PTHREAD_LIBS where
> + needed", 2023-03-17), that is, "xxx_LDADD = $(PTHREAD_LIBS)", to this
> + test case [thanks to Eric for that fixup BTW]
> +
> + - replace EXIT trap handler with cleanup_fn [Eric]
> +
> + - Create "subdir/f" as a directory, and extend two test scenarios to
> + show that "subdir/f", even though "f" has search (execute) permission,
> + results in EACCES (directly), and does not stop advancement through
> + PATH="...:subdir:..." (indirectly) [Eric]. Use "mkdir + mkdir" for
> + creating the "f" directory, rather than "mkdir -p", for symmetry with
> + "mkdir + mkfifo" before, and "mkdir + touch" after.
> +
> + - replace "<imperative>, such that <subjunctive>" with "<imperative>,
> + such that <indicative>" (= s/lead/leads/) [Eric]
>
> ## lib/test-fork-safe-execvpe.c (new) ##
> @@
> @@ lib/Makefile.am: pkgconfig_DATA = libnbd.pc
>
> test_fork_safe_assert_SOURCES = \
> @@ lib/Makefile.am: test_fork_safe_assert_SOURCES = \
> - test-fork-safe-assert.c \
> utils.c \
> $(NULL)
> + test_fork_safe_assert_LDADD = $(PTHREAD_LIBS)
> +
> +test_fork_safe_execvpe_SOURCES = \
> + $(top_srcdir)/common/utils/vector.c \
> @@ lib/Makefile.am: test_fork_safe_assert_SOURCES = \
> + test-fork-safe-execvpe.c \
> + utils.c \
> + $(NULL)
> ++test_fork_safe_execvpe_LDADD = $(PTHREAD_LIBS)
>
> ## lib/test-fork-safe-execvpe.sh (new) ##
> @@
> @@ lib/test-fork-safe-execvpe.sh (new)
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +
> ++. ../tests/functions.sh
> ++
> +set -e
> +
> +# Determine the absolute pathname of the execvpe helper binary. The "realpath"
> @@ lib/test-fork-safe-execvpe.sh (new)
> +
> +# Create a temporary directory and change the working directory to it.
> +tmpd=$(mktemp -d)
> -+trap 'rm -r -- "$tmpd"' EXIT
> ++cleanup_fn rm -r -- "$tmpd"
> +cd "$tmpd"
> +
> +# If the "file" parameter of execvpe() is an empty string, then we must fail --
> @@ lib/test-fork-safe-execvpe.sh (new)
> +mkdir fifo
> +mkfifo fifo/f
> +
> ++# Create a directory with a directory in it.
> ++mkdir subdir
> ++mkdir subdir/f
> ++
> +# Create a directory with a non-executable file in it.
> +mkdir nxregf
> +touch nxregf/f
> @@ lib/test-fork-safe-execvpe.sh (new)
> +# the "file" parameter didn't contain a <slash>.)
> +run "" empty/f; execve_fail empty/f ENOENT
> +run "" fifo/f; execve_fail fifo/f EACCES
> ++run "" subdir/f; execve_fail subdir/f EACCES
> +run "" nxregf/f; execve_fail nxregf/f EACCES
> +run "" nxregf/f/; execve_fail nxregf/f/ ENOTDIR
> +run "" symlink/f; execve_fail symlink/f ELOOP
> @@ lib/test-fork-safe-execvpe.sh (new)
> +#
> +# Show that, if the last candidate fails execve() with an error number that
> +# would not be fatal otherwise, we do get that error number.
> -+run empty:fifo:nxregf:symlink f
> -+execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP
> ++run empty:fifo:subdir:nxregf:symlink f
> ++execve_fail empty/f,fifo/f,subdir/f,nxregf/f,symlink/f ELOOP
> +
> -+# Put a single prefix in PATH, such that it lead to a successful execution. This
> -+# exercises two things at the same time: (a) that nbd_internal_execvpe_init()
> -+# produces *one* candidate (i.e., that no <colon> is seen), and (b) that
> -+# nbd_internal_fork_safe_execvpe() succeeds for the *last* candidate. Repeat the
> -+# test with "expr" (called "f" under "bin") and the shell script (called "f"
> -+# under "sh", triggering the ENOEXEC branch).
> ++# Put a single prefix in PATH, such that it leads to a successful execution.
> ++# This exercises two things at the same time: (a) that
> ++# nbd_internal_execvpe_init() produces *one* candidate (i.e., that no <colon> is
> ++# seen), and (b) that nbd_internal_fork_safe_execvpe() succeeds for the *last*
> ++# candidate. Repeat the test with "expr" (called "f" under "bin") and the shell
> ++# script (called "f" under "sh", triggering the ENOEXEC branch).
> +run bin f 1 + 1; success bin/f,2
> +run sh f arg1; success sh/f,"sh/f arg1"
> +
Thanks for reviewing,
Laszlo
Laszlo Ersek (2):
lib/utils: introduce async-signal-safe execvpe()
lib/utils: add unit tests for async-signal-safe execvpe()
.gitignore | 1 +
lib/Makefile.am | 11 +
lib/internal.h | 22 ++
lib/test-fork-safe-execvpe.c | 117 +++++++
lib/test-fork-safe-execvpe.sh | 277 +++++++++++++++
lib/utils.c | 355 ++++++++++++++++++++
6 files changed, 783 insertions(+)
create mode 100644 lib/test-fork-safe-execvpe.c
create mode 100755 lib/test-fork-safe-execvpe.sh
base-commit: 742cbd8c7adce91eb61b74524df3eb0180799653
2 years, 7 months
[libnbd PATCH 0/3] reenable execvpe unit testing in Alpine Linux containers
by Laszlo Ersek
These patches have been pushed (f5a065aa3a9c..9075f68ffc8b); I'm posting
them for visibility.
Laszlo
Laszlo Ersek (3):
lib/test-fork-safe-execvpe.sh: generalize "run" to "run0"
lib/test-fork-safe-execvpe.sh: cope with Alpine Linux / BusyBox
limitations
Revert "ci: skip "lib/test-fork-safe-execvpe.sh" on Alpine Linux"
ci/skipped_tests | 10 ----
lib/test-fork-safe-execvpe.sh | 63 +++++++++++++-------
2 files changed, 42 insertions(+), 31 deletions(-)
2 years, 7 months
