Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
by Alberto Faria
> On Wed, Jan 04, 2023 at 06:14:34PM +0000, Richard W.M. Jones wrote:
> > (3) It seems like some drivers require pre-allocated memory regions,
> > and since some do that means we might as well implement this. It
> > also seems like some drivers require file-backed pre-allocated
> > memory regions, and so we might as well implement that too.
> >
> > However what is not clear: does memfd_create(2) always allocate
> > sufficiently aligned memory regions, such that we never need to bother
> > reading the mem-region-alignment property?
> >
> > I notice that the example:
> > https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> > just passes on this and calls blkio_alloc_mem_region(). Is that the
> > safest and easiest thing to do which will always work?
>
> So this seems to be the reverse of what I thought.
>
> In nbdkit plugins ideally we'd like to avoid using bounce buffers if
> possible. Also NBD requests can be up to (IIRC) 32M, although we can
> hint to the client to limit them to some smaller number.
Since you're currently serializing requests, you may be able to
manually break up big requests into smaller requests that don't exceed
max-transfer, and then wait for the right number of completions with
blkioq_do_io().
> If I call blkio_alloc_mem_region() unconditionally then it seems as if
> I always need to use this as a bounce buffer. Plus, is 32M too large
> for a memory region allocated this way? The example allocates 128K.
There is no general limit to the size of memory regions. 32M should
always be fine. In particular, using different parts of a single big
memory region for many concurrent requests is a reasonable use case.
> It seems like for drivers which _don't_ require pre-allocated memory
> regions then we should avoid calling blkio_alloc_mem_region which
> would avoid bounce buffers.
>
> Some guidance about this would be appreciated.
A possible approach is to make nbdkit always allocate page-aligned,
file-backed buffers, and attempt to blkio_map_mem_region() them at
init time in the libblkio plugin. Note that mapping memory regions
even for drivers that don't strictly need them can still improve
performance.
If this fails and the driver requires mapped memory regions, then one
could fall back to bounce buffering with buffers allocated by
blkio_alloc_mem_region(). But this probably won't happen in practice,
so just failing in this case is probably fine.
Alberto
1 year, 4 months
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
by Alberto Faria
> (5) "The application is responsible for thread-safety. No thread
> synchronization is necessary when a queue is only used from a single
> thread. Proper synchronization is required when sharing a queue
> between multiple threads."
>
> Does this apply across multiple struct blkio handles? ie. Is there
> now, or could there be in future, some global state which would be
> corrupted by parallel calls across multiple handles?
There will never be any need to synchronize accesses to separate blkio
instances. We will clarify this in the docs.
Alberto
> This matters because we could use NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
> instead of NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
> (https://libguestfs.org/nbdkit-plugin.3.html#Threads).
1 year, 4 months
Re: [Libguestfs] [PATCH nbdkit] New plugin: blkio [incomplete]
by Alberto Faria
> This is an incomplete outline implementation for a libblkio plugin for
> nbdkit. At the moment it only supports reading the same ("capacity")
> of the device, and not even reading or writing. I have some questions
> about the libblkio API before I can complete the plugin (see below).
>
> The idea here is that by connecting libblkio to NBD we can use the
> existing set of scripting tools to script access to devices. For
> example you could use Python to read or modify a device:
>
> ----------------------------------------------------------------------
> $ nbdsh -c - <<'EOF'
>
> from subprocess import *
>
> # Run nbdkit-blkio-plugin.
> h.connect_systemd_socket_activation ("nbdkit" "blkio",
> "virtio-blk-vhost-user",
> "path=unix.sock")
>
> print("device size=%d", h.get_size())
>
> # Dump the boot sector.
> bootsect = h.pread(512, 0)
> p = Popen("hexdump -C", shell=True, stdin=PIPE)
> p.stdin.write(bootsect)
> EOF
> ----------------------------------------------------------------------
>
> So my questions and comments about libblkio:
>
> (1) There is no way to know which properties are readable, writable,
> and those which need to be set before or after blkio_connect (see
> is_preconnect_property in the plugin). It should be possible to
> introspect this information. Also might be nice to be able read a
> list of all available properties.
>
> (2) It would be nice if libblkio had a way to enable debugging and
> call the caller back for debug messages. We could then redirect the
> callbacks into the nbdkit debug API (nbdkit_debug()) where they would
> eventually end up on stderr or syslog.
Thanks, I created a couple issues for these features.
> However don't send debug messages to stderr, or at least allow that
> behaviour to be overridden.
>
> (3) It seems like some drivers require pre-allocated memory regions,
> and since some do that means we might as well implement this. It
> also seems like some drivers require file-backed pre-allocated
> memory regions, and so we might as well implement that too.
>
> However what is not clear: does memfd_create(2) always allocate
> sufficiently aligned memory regions, such that we never need to bother
> reading the mem-region-alignment property?
mem-region-alignment is currently allowed to exceed the page size, but
I'm not sure if it ever could in practice. (Maybe it could due to
IOMMU alignment restrictions?)
> I notice that the example:
> https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> just passes on this and calls blkio_alloc_mem_region(). Is that the
> safest and easiest thing to do which will always work?
Yes.
> (4) As a first pass, I only want to bother implementing blocking mode.
> It'll be slow, but it doesn't matter for this application. Also I've
> chosen nbdkit's NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS so nbdkit
> will serialise all requests (again, only for a very simple first pass).
>
> Looking at: https://libblkio.gitlab.io/libblkio/blkio.html#performing-i-o
> seems simple enough but:
>
> (4a) What is the queue number? Always 0? Is it affected by num-entries?
With n = num-queues, (non-poll) queues have indices 0 through n-1.
num-queues is 1 by default.
num-entries is the size of each of those queues for io_uring drivers,
and doesn't affect their index.
> (4b) It's unclear how completions work. If I set min=max=1, will it
> return after the whole operation has completed? Do I need to
> call it again? What about if the request is very large, can it
> get split?
blkioq_do_io() will only return after at least min completions have
been copied out into the user's completion array. (Unless it fails, in
which case it returns an error and no completions are copied
out/consumed.)
With min=max=1, blkioq_do_io() will wait until at least one completion
is available, copy it out, and return 1. It will never return 0.
> Reading the example
> https://gitlab.com/libblkio/libblkio/-/blob/main/examples/blkio-copy.c
> it appears that requests cannot ever be split?
Requests are never split (by libblkio). If read/write requests are
larger than max-transfer, they fail.
Alberto
1 year, 4 months
[v2v PATCH] rhv: Use osinfo to distinguish Windows >= 10 variants in "ovirt:id" too
by Laszlo Ersek
Reflect commit 38b35f3b7e5c ("rhv: Use osinfo to distinguish Windows >= 10
variants", 2022-12-02) to the "ovirt:id" field in the OVF as well. Take
the values from "packaging/conf/osinfo-defaults.properties" in the
ovirt-engine tree, as the comment on "get_ovirt_osid" explains.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2152465
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
lib/create_ovf.ml | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml
index 8aff3d8f0b53..79b3285766c9 100644
--- a/lib/create_ovf.ml
+++ b/lib/create_ovf.ml
@@ -437,13 +437,22 @@ and get_ovirt_osid = function
i_arch = "i386" } ->
26
+ (* For Windows NT 10.0 always use the <osinfo> field since the
+ * other fields will not accurately reflect the version.
+ *)
| { i_type = "windows"; i_major_version = 10; i_minor_version = 0;
- i_arch = "x86_64"; i_product_variant = "Client" } ->
- 27
-
- | { i_type = "windows"; i_major_version = 10; i_minor_version = 0;
- i_arch = "x86_64" } ->
- 29
+ i_arch = "x86_64"; i_osinfo = osinfo; i_product_name = product } ->
+ (match osinfo with
+ | "win10" -> (* windows_10x64 *) 27
+ | "win11" -> (* windows_11 *) 36
+ | "win2k16" -> (* windows_2016x64 *) 29
+ | "win2k19" -> (* windows_2019x64 *) 31
+ | "win2k22" -> (* windows_2022 *) 37
+ | _ ->
+ warning (f_"unknown Windows 10 variant: %s (%s)")
+ osinfo product;
+ (* windows_2022 *) 37
+ )
| { i_type = typ; i_distro = distro;
i_major_version = major; i_minor_version = minor; i_arch = arch;
1 year, 4 months
[PATCH nbdkit] ssh: Improve the error message when all authentication methods fail
by Richard W.M. Jones
The current error message:
nbdkit: ssh[1]: error: all possible authentication methods failed
is confusing and non-actionable. It's hard even for experts to
understand the relationship between the authentication methods offered
by a server and what we require.
Try to improve the error message in some common situations, especially
where password authentication on the server side is disabled but the
client supplied a password=... parameter. After this change, you will
see an actionable error:
nbdkit: ssh[1]: error: the server does not offer password
authentication, but you tried to use a password; if you have root
access to the server, try editing 'sshd_config' and setting
'PasswordAuthentication yes'; otherwise try using an SSH agent with
a passphrase
Also remove an incidental comment left over when I copied the libssh
example code.
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2158300
---
plugins/ssh/ssh.c | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
index 6cf40c26f..23c0b46f9 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -355,14 +355,35 @@ authenticate (struct ssh_handle *h)
rc = authenticate_pubkey (h->session);
if (rc == SSH_AUTH_SUCCESS) return 0;
}
+ else if (password == NULL) {
+ /* Because the password method below requires a password, we know
+ * that it will fail, so print an actionable error message and
+ * bail now.
+ */
+ nbdkit_error ("the server does not offer SSH agent authentication; "
+ "try using a password=... parameter, see the "
+ "nbdkit-ssh-plugin(1) manual page");
+ return -1;
+ }
- /* Example code tries keyboard-interactive here, but we cannot use
- * that method from a server.
- */
-
- if (password != NULL && (method & SSH_AUTH_METHOD_PASSWORD)) {
- rc = authenticate_password (h->session, password);
- if (rc == SSH_AUTH_SUCCESS) return 0;
+ if (password != NULL) {
+ if (method & SSH_AUTH_METHOD_PASSWORD) {
+ rc = authenticate_password (h->session, password);
+ if (rc == SSH_AUTH_SUCCESS) return 0;
+ else {
+ nbdkit_error ("password authentication failed, "
+ "is the username and password correct?");
+ return -1;
+ }
+ }
+ else {
+ nbdkit_error ("the server does not offer password authentication, "
+ "but you tried to use a password; if you have root access "
+ "to the server, try editing 'sshd_config' and setting "
+ "'PasswordAuthentication yes'; otherwise try using "
+ "an SSH agent with a passphrase");
+ return -1;
+ }
}
nbdkit_error ("all possible authentication methods failed");
--
2.37.3
1 year, 4 months
[v2v PATCH] windows_virtio: favor "fwcfg" over "qemufwcfg"
by Laszlo Ersek
Virtio-win may provide the "qemufwcfg" stub driver and/or the "fwcfg"
actual driver. If both are provided, we must not install both, as they
conflict with each other. Pick "fwcfg" in this case.
Because the drivers can originate from two sources (libosinfo vs.
virtio-win), *and* because "copy_from_virtio_win" is reused for the QEMU
guest agent (i.e., not just for the drivers), do not sink the above
filtering into "copy_drivers" (or even more deeply). Instead, let copying
complete, and then clean up "driverdir" -- remove "qemufwcfg" if "fwcfg"
is present. (We'd have to consult the full list of drivers anyway, to see
if "fwcfg" indicates we should exclude "qemufwcfg".)
A note on annotating the "install_drivers" parameter list (OCaml obscurity
level one million):
> -let rec install_drivers ((g, _) as reg) inspect =
> +let rec install_drivers ((g, _) as reg : Registry.t) inspect =
Turns out that in this module, OCaml doesn't really have an idea that all
the "g#method" calls are made for an object of type "Guestfs.guestfs".
Instead, the compiler infers an interface from the methods we call, and
then tries to shoehorn that "collected interface" into "Guestfs.guestfs"
when it reaches:
> reg_import reg (regedits @ common_regedits)
This used to work fine until now; however, once we call
> g#glob_expand (driverdir // "qemufwcfg.*")
in this patch, the "reg_import" call fails like this:
> File "windows_virtio.ml", line 152, characters 13-16:
> 152 | reg_import reg (regedits @ common_regedits)
> ^^^
> Error: This expression has type
> (< [snip]
> glob_expand : string -> 'weak1 array;
> [snip] >
> as 'a) *
> 'weak2
> but an expression was expected of type
> Registry.t = Guestfs.guestfs * Registry.node
> Type
> < [snip]
> glob_expand : string -> 'weak1 array;
> [snip] >
> as 'a
> is not compatible with type
> Guestfs.guestfs =
> < [snip]
> glob_expand : ?directoryslash:bool -> string -> string array;
> [snip] >
> Types for method glob_expand are incompatible
The problem is that in our "g#glob_expand" call, we silently omit the
optional parameter "directoryslash", so at that point the compiler
"infers" a parameter list
> glob_expand : string -> 'weak1 array;
for the "interface" we require. And then that blows up because the actual
object provides only
> glob_expand : ?directoryslash:bool -> string -> string array;
The solution is to enlighten the compiler in "install_drivers" about the
actual type of "g", so that it need not infer or "collect" an interface
when we call "g#glob_expand" with "directoryslash" elided.
Now, "install_drivers" is called (as a callback!) ultimately from
"Registry.with_hive_write", and so its "reg" parameter has type
"Registry.t":
> type t = Guestfs.guestfs * node
(This is in fact reported by the compiler too, in the above-quoted error
message. Except said error message is like ten pages long -- see those
[snip] markers? --, so you will only ever find the relevant bit in the
error report if you already know what to look for. Helpful, that!)
Therefore, annotating the "reg" parameter like this:
> -let rec install_drivers ((g, _) as reg) inspect =
> +let rec install_drivers ((g, _) as reg : Registry.t) inspect =
forces "g" to have type "Guestfs.guestfs".
This is of course super obscure. The hint was that both
"linux_bootloaders.ml" and "convert_linux.ml" already used "g#glob_expand"
without the optional parameter -- and the important difference was that
these files had been type-annotated previously.
In *retrospect* -- that is, rather uselessly... --, the language
documentation does highlight this
<https://v2.ocaml.org/manual/lablexamples.html#s:label-inference>:
> We will not try here to explain in detail how type inference works. One
> must just understand that there is not enough information in the above
> program to deduce the correct type of g or bump. That is, there is no
> way to know whether an argument is optional or not, or which is the
> correct order, by looking only at how a function is applied. The
> strategy used by the compiler is to assume that there are no optional
> arguments, and that applications are done in the right order.
>
> [...]
>
> In practice, such problems appear mostly when using objects whose
> methods have optional arguments, so that writing the type of object
> arguments is often a good idea.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2151752
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
convert/windows_virtio.ml | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml
index 3156694d114b..d9fda13f999b 100644
--- a/convert/windows_virtio.ml
+++ b/convert/windows_virtio.ml
@@ -44,7 +44,7 @@ let viostor_modern_pciid = "VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01"
let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00"
let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01"
-let rec install_drivers ((g, _) as reg) inspect =
+let rec install_drivers ((g, _) as reg : Registry.t) inspect =
(* Copy the virtio drivers to the guest. *)
let driverdir = sprintf "%s/Drivers/VirtIO" inspect.i_windows_systemroot in
g#mkdir_p driverdir;
@@ -103,6 +103,23 @@ let rec install_drivers ((g, _) as reg) inspect =
else
Virtio_net in
+ (* The "fwcfg" driver binds the fw_cfg device for real, and provides three
+ * files -- ".cat", ".inf", ".sys". (Possibly ".pdb" too.)
+ *
+ * The "qemufwcfg" driver is only a stub driver; it placates Device Manager
+ * (hides the "unknown device" question mark) but does not actually drive
+ * the fw_cfg device. It provides two files only -- ".cat", ".inf".
+ *
+ * These drivers conflict with each other (RHBZ#2151752). If we've copied
+ * both (either from libosinfo of virtio-win), let "fwcfg" take priority:
+ * remove "qemufwcfg".
+ *)
+ if g#exists (driverdir // "fwcfg.inf") &&
+ g#exists (driverdir // "qemufwcfg.inf") then (
+ debug "windows: skipping qemufwcfg stub driver in favor of fwcfg driver";
+ Array.iter g#rm (g#glob_expand (driverdir // "qemufwcfg.*"))
+ );
+
(* Did we install the miscellaneous drivers? *)
let virtio_rng_supported = g#exists (driverdir // "viorng.inf") in
let virtio_ballon_supported = g#exists (driverdir // "balloon.inf") in
1 year, 4 months
