Re: [Libguestfs] [PATCH nbdkit] server: Move size parsing code (nbdkit_parse_size) to common/include
On Tue, Sep 05, 2023 at 11:09:02AM +0100, Richard W.M. Jones wrote:
> > > +static inline int64_t
> > > +human_size_parse (const char *str,
> > > + const char **error, const char **pstr)
> > > +{
> > > + int64_t size;
> > > + char *end;
> > > + uint64_t scale = 1;
> > > +
> > > + /* XXX Should we also parse things like '1.5M'? */
> > > + /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
> > > + * because some of them are valid hex digits.
> > > + */
> > > + errno = 0;
> > > + size = strtoimax (str, , 10);
> >
> > (1) A further improvement here (likely best done in a separate patch)
> > could be to change the type of "size" to "intmax_t", from "int64_t".
> > That way, the assignment will be safe even theoretically, *and* the
> > overflow check at the bottom of the function (with the division &
> > comparison of the quotient against INT_MAX) will work just the same.
>
> I'm always very unsure how this all works. In particular I seem to
> recall that intmax_t is no longer really the maximum possible int
> (because of int128) and so it's always 64 bit on platforms we care
> about. Can Eric comment?
intmax_t was supposed to be whatever the compiler supports as its
largest integer type; but you are right that when gcc added
__int128_t, they did NOT change intmax_t at the time (arguably by
using weasel-words that as an implementation-defined type, it was not
an integer type merely because you can't write integer literals of
that type, even though it behaves integral in every other aspect).
We're kind of in a frozen state where making intmax_t larger than 64
bits will break more programs than expected because it has ABI
implications:
https://stackoverflow.com/questions/21265462/why-in-g-stdintmax-t-is-not-a-int128-t
My personal preference is to avoid intmax_t, as it has too much
baggage (the risk of future widening, vs. NOT being the widest type
after all), similar to how size_t already has baggage. In short,
having something that is not platform specific is easier to reason
about (for the same way that using size_t gives me more grief than
directly using int32_t or int64_t; even though size_t is such a
naturally occuring type, the fact that it is not uniform width makes
it trickier to work with).
> > > +
> > > + if (INT64_MAX / scale < size) {
> > > +*error = "could not parse size: size * scale overflows";
> > > +*pstr = str;
> > > +return -1;
> > > + }
And thus I prefer that this comparison stay pegged to INT64_MAX, and
not INT_MAX.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] LIBGUESTFS supported version
Hi Team, Am trying to install libguestfs in my redhat 8.2 version default its installing 1.38.4. Could you please help us with the installation of version 1.50 libguestfs and supported RHEL versions. Regards, Teja K ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll
On Mon, Sep 04, 2023 at 06:59:15PM +, Tage Johansson wrote: > > > > +// can be both readable and writable, but we survive just fine > > > +// if we only see one direction even when both are available. > > > +poll.registry().register( > > > + SourceFd(), > > > +Token(0), > > > +MioInterest::READABLE | MioInterest::WRITABLE, > > > +)?; > > > +poll.poll( events, Some(Duration::ZERO))?; > > Why do we want 'poll.poll()?;', that is, to fail this function if the > > poll returns an error? We _expect_ poll to sometimes return an error > > (namely, the fact that it timed out) if there is nothing pending on > > the fd, at which point we WANT to successfully clear the ready_guard > > for both read and write, rather than to error out of this function. > > > You are right. I thought that the poll() call would return Ok(()) upon > timeout, but according to the documentation: > > > Currently if the timeout elapses without any readiness events triggering > > this will return Ok(()). However we’re not guaranteeing this behaviour > > as this depends on the OS. > > So I guess it is best to ignore any errors from the poll call as in your > patch. Okay, I'll merge in that aspect of my original along with your other improvements, and push something soon. Fingers crossed that we'll finally get a green CI run today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH v9 6/7] rust: async: Add an example
On Mon, Sep 04, 2023 at 04:56:07PM +, Tage Johansson wrote:
>
> On 9/1/2023 10:41 PM, Eric Blake wrote:
> > On Sat, Aug 26, 2023 at 11:29:59AM +, Tage Johansson wrote:
> > > This patch adds an example using the asynchronous Rust bindings.
> > > ---
> > > rust/Cargo.toml| 1 +
> > > rust/examples/concurrent-read-write.rs | 149 +
> > > rust/run-tests.sh.in | 2 +
> > > 3 files changed, 152 insertions(+)
> > > create mode 100644 rust/examples/concurrent-read-write.rs
> > >
> > > diff --git a/rust/Cargo.toml b/rust/Cargo.toml
> > > index c49f9f2..0879b34 100644
> > > --- a/rust/Cargo.toml
> > > +++ b/rust/Cargo.toml
> > > @@ -58,5 +58,6 @@ default = ["log", "tokio"]
> > > anyhow = "1.0.72"
> > > once_cell = "1.18.0"
> > > pretty-hex = "0.3.0"
> > > +rand = { version = "0.8.5", default-features = false, features =
> > > ["small_rng", "min_const_gen"] }
> > > tempfile = "3.6.0"
> > > tokio = { version = "1.29.1", default-features = false, features =
> > > ["rt-multi-thread", "macros"] }
> > > diff --git a/rust/examples/concurrent-read-write.rs
> > > b/rust/examples/concurrent-read-write.rs
> > > new file mode 100644
> > > index 000..4858f76
> > > --- /dev/null
> > > +++ b/rust/examples/concurrent-read-write.rs
> > > @@ -0,0 +1,149 @@
> > > +//! Example usage with nbdkit:
> > > +//! nbdkit -U - memory 100M \
> > > +//! --run 'cargo run --example concurrent-read-write --
> > > $unixsocket'
> > > +//! Or connect over a URI:
> > > +//! nbdkit -U - memory 100M \
> > > +//! --run 'cargo run --example concurrent-read-write -- $uri'
> > Should be "$uri" here (to avoid accidental shell globbing surprises).
> >
> > > +//!
> > > +//! This will read and write randomly over the first megabyte of the
> > This says first megabyte...[1]
>
>
> If I understand my code correctly, it is actually reading and writing
> randomly over the entire memory.
Yes (aka, the comment is wrong in the source file, we should clean it
up there as well as here).
>
> > > +//! plugin using multi-conn, multiple threads and multiple requests in
> > > +//! flight on each thread.
> > > +
> > > +#![deny(warnings)]
> > > +use rand::prelude::*;
> > > +use std::env;
> > > +use std::sync::Arc;
> > > +use tokio::task::JoinSet;
> > > +
> > > +/// Number of simultaneous connections to the NBD server.
> > > +///
> > > +/// Note that some servers only support a limited number of
> > > +/// simultaneous connections, and/or have a configurable thread pool
> > > +/// internally, and if you exceed those limits then something will break.
> > > +const NR_MULTI_CONN: usize = 8;
> > > +
> > > +/// Number of commands that can be "in flight" at the same time on each
> > > +/// connection. (Therefore the total number of requests in flight may
> > > +/// be up to NR_MULTI_CONN * MAX_IN_FLIGHT).
> > > +const MAX_IN_FLIGHT: usize = 16;
> > > +
> > > +/// The size of large reads and writes, must be > 512.
> > > +const BUFFER_SIZE: usize = 1024;
> > 1024 isn't much larger than 512. It looks like you borrowed heavily
> > from examples/threaded-reads-and-writes.c, but that used 1M as the
> > large buffer.
>
>
> The reason for this is that we can't read and write to a shared buffer
> simultaneously in safe Rust. So the buffer gets created on the fly for each
> read/write operation. And creating a 1M buffer so many times seemd like a
> bit too much memory comsumtion.
Agreed that lots of memory use is undesirable. The original C test
exploits and documents that it is doing an unsafe optimization of
reusing a single buffer across simultaneous operations merely to
demonstrate speed of the API; so it is interesting proof that Rust has
succeeded in this instance at its goal of letting the compiler tell us
that our (known) unsafe buffer sharing is not kosher. Still, that's
WHY Rust has the 'unsafe' keyword, to let us explicitly document when
we know we are doing something that is unusual. It seems to me that
this example would be truer to the original example if we are able to
add in 'unsafe' and intentionally share a buffer across multiple
threads (despite access to the contents of that buffer no longer being
well-defined) than to avoid the 'unsafe' keyword.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote: > --- a/info/info-list-uris.sh > +++ b/info/info-list-uris.sh > @@ -22,7 +22,14 @@ set -e > set -x > > requires nbdkit --version > -requires nbdkit file --version > +# Avoid tickling a double-free bug in nbdkit 1.35.11 > +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c " > +try: > + h.opt_info() > +except nbd.Error: > + pass > +h.opt_abort() > +"' Actually I think a problem with this is that it always causes a segfault, which could leave a core dump around (does on my machine). It's unfortunate, because obviously you can't test for a double-free in nbdkit without causing a double-free. It might be a rare case when a version test is better. 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
On Tue, Sep 05, 2023 at 08:47:34AM -0500, Eric Blake wrote:
> diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh
> index 0d6a16a0..d8ea9108 100755
> --- a/info/info-list-uris.sh
> +++ b/info/info-list-uris.sh
> @@ -22,7 +22,14 @@ set -e
> set -x
>
> requires nbdkit --version
> -requires nbdkit file --version
> +# Avoid tickling a double-free bug in nbdkit 1.35.11
> +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
> +try:
> + h.opt_info()
> +except nbd.Error:
> + pass
> +h.opt_abort()
> +"'
>
> # This test requires nbdkit >= 1.22.
I guess the double-free bug is actually in any (or many versions of)
nbdkit between 1.22 and 1.35.11, which is why the requires test is
needed here? The message seems to imply it only affects 1.35.11.
It's a shame that the requires test is so long, but I couldn't see any
way to shorten it.
> minor=$( nbdkit --dump-config | grep ^version_minor | cut -d= -f2 )
> diff --git a/info/info-uri.sh b/info/info-uri.sh
> index d491e904..ba0c36d2 100755
> --- a/info/info-uri.sh
> +++ b/info/info-uri.sh
> @@ -24,7 +24,14 @@ set -e
> set -x
>
> requires nbdkit --version
> -requires nbdkit file --version
> +# Avoid tickling a double-free bug in nbdkit 1.35.11
> +requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
> +try:
> + h.opt_info()
> +except nbd.Error:
> + pass
> +h.opt_abort()
> +"'
> requires nbdkit -U - null --run 'test "$uri" != ""'
> requires jq --version
>
> diff --git a/info/main.c b/info/main.c
> index 204290a5..80d38224 100644
> --- a/info/main.c
> +++ b/info/main.c
> @@ -138,7 +138,6 @@ main (int argc, char *argv[])
>size_t output_len = 0;
>bool content_flag = false, no_content_flag = false;
>bool list_okay = true;
> - bool opt_mode = false;
>
>progname = argv[0];
>colour = isatty (STDOUT_FILENO);
> @@ -281,11 +280,9 @@ main (int argc, char *argv[])
>nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
>
>/* Set optional modes in the handle. */
> - opt_mode = !can && !map && !size_only;
> - if (opt_mode) {
> -nbd_set_opt_mode (nbd, true);
> + nbd_set_opt_mode (nbd, true);
> + if (!can && !map && !size_only)
> nbd_set_full_info (nbd, true);
> - }
>if (map)
> nbd_add_meta_context (nbd, map);
>
> @@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd)
> fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> exit (EXIT_FAILURE);
>}
> +
> + /* If we are in opt mode, request info on the original export name.
> + * However, ignoring failure at this time is okay, as later code
> + * may want to try an alternate export name.
> + */
> + if (nbd_aio_is_negotiating (nbd))
> +nbd_opt_info (nbd);
I maybe don't fully understand when nbd_aio_is_negotiating would not
be true. Aren't we always in opt mode now?
> }
>
> /* The URI field in output is not meaningful unless there's a
> diff --git a/info/map.c b/info/map.c
> index 594f24ff..399e0355 100644
> --- a/info/map.c
> +++ b/info/map.c
> @@ -54,6 +54,13 @@ do_map (void)
>uint64_t offset, align, max_len;
>size_t prev_entries_size;
>
> + /* Map mode requires switching over to transmission phase. */
> + if (nbd_aio_is_negotiating (nbd) &&
> + nbd_opt_go (nbd) == -1) {
> +fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
> +exit (EXIT_FAILURE);
> + }
> +
>/* Did we get the requested map? */
>if (!nbd_can_meta_context (nbd, map)) {
> fprintf (stderr,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 0/3] Simplify nbd_shutdown vs. opt mode
On Fri, Sep 01, 2023 at 10:28:52PM +0100, Richard W.M. Jones wrote: > On Tue, Aug 29, 2023 at 05:20:40PM -0500, Eric Blake wrote: > > While working on a larger set of patches to make nbdinfo favor > > NBD_OPT_INFO over NBD_OPT_GO where possible (which requires use of > > nbd_set_opt_mode(,true) in more cases), I noticed that it got unwieldy > > to have to pick the correct shutdown function in all code paths. So I > > propose making the API smarter, by adding an opt-in flag that does the > > right thing on my behalf. > > > > If you have an idea for a better name for the flag, or think this > > functionality should be enabled by default, let me know. Part of the > > reason for choosing a new flag is that it becomes a compile-time > > witness of whether nbd_shutdown has the desired capability (if we > > allow it to auto-opt_abort without a flag, it's harder to tell whether > > we are running against an older libnbd where it errors out instead). > > My feeling is this should be enabled by default, as that does the > right thing by default. Makes sense. Certainly easier to write nbd_shutdown(nbd, 0) than nbd_shutdown(nbd, LIBNBD_SHUTDOWN_COVER_OPT_MODE). > > Whether or not we need to have a flag to disable it (ie the opposite > sense to the proposed flag) is up to you. At this point, there are so few affected callers (most programs don't use nbd_set_opt_mode, and nbdinfo is patched by this series), so for now I won't bother to add a witness flag; but I will go ahead and backport the fix to stable branches. Adding a flag in a followup patch (with the opposite sense of NOT shutting down if still in opt mode - mainly as the witness of the feature existing) is still an option. > > For the series: > Reviewed-by: Richard W.M. Jones I'll reply again with commit ids once I've amended patch 2 and 3; I've also just replied with a separate mail (oops, I didn't title it 4/3) with the reasons behind this series in the first place, before I can finish checking in the tail end of the 64-bit extension series. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [libnbd PATCH] info: Prefer NBD_OPT_INFO when possible
Now that libnbd supports extended headers, it is worth observing that
the qemu implementation for NBD_CMD_BLOCK_STATUS that supports an
extended header for filtering the server's response, as advertised by
NBD_FLAG_BLOCK_STAT_PAYLOAD, is conditionally advertised. When using
NBD_OPT_GO, the flag is only advertised if the client requested more
than one meta context (as otherwise, there is no use filtering). But
for NBD_OPT_INFO, the flag is advertised if extended headers are
enabled, regardless of whether meta contexts are negotiated yet.
In order for upcoming libnbd patches to add support for probing and
using this bit reliably, we therefore need 'nbdinfo --can ...' to
favor the information learned by NBD_OPT_INFO instead of NBD_OPT_GO,
because that is lighter weight than figuring out whether an export
supports at least two meta contexts that can then be negotiated.
Do this by changing nbdinfo to prefer opt mode always, then touching
up the few places (--map, --content) that need to ensure NBD_OPT_GO.
This is made easier by the previous patches that make nbd_shutdown()
work sanely regardless of whether we are still in opt mode.
In turn, this patch flushed out a double-free bug in 'nbdkit file
dir=...' when querying opt_info on the default name, so a couple of
unit tests need to avoid false negatives on platforms where nbdkit
commit 39d62de9 is not yet backported.
Signed-off-by: Eric Blake
---
info/info-list-uris.sh | 9 -
info/info-uri.sh | 9 -
info/main.c| 14 +-
info/map.c | 7 +++
4 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/info/info-list-uris.sh b/info/info-list-uris.sh
index 0d6a16a0..d8ea9108 100755
--- a/info/info-list-uris.sh
+++ b/info/info-list-uris.sh
@@ -22,7 +22,14 @@ set -e
set -x
requires nbdkit --version
-requires nbdkit file --version
+# Avoid tickling a double-free bug in nbdkit 1.35.11
+requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
+try:
+ h.opt_info()
+except nbd.Error:
+ pass
+h.opt_abort()
+"'
# This test requires nbdkit >= 1.22.
minor=$( nbdkit --dump-config | grep ^version_minor | cut -d= -f2 )
diff --git a/info/info-uri.sh b/info/info-uri.sh
index d491e904..ba0c36d2 100755
--- a/info/info-uri.sh
+++ b/info/info-uri.sh
@@ -24,7 +24,14 @@ set -e
set -x
requires nbdkit --version
-requires nbdkit file --version
+# Avoid tickling a double-free bug in nbdkit 1.35.11
+requires nbdkit -U- -r file dir=. --run 'nbdsh --opt-mode -u "$uri" -c "
+try:
+ h.opt_info()
+except nbd.Error:
+ pass
+h.opt_abort()
+"'
requires nbdkit -U - null --run 'test "$uri" != ""'
requires jq --version
diff --git a/info/main.c b/info/main.c
index 204290a5..80d38224 100644
--- a/info/main.c
+++ b/info/main.c
@@ -138,7 +138,6 @@ main (int argc, char *argv[])
size_t output_len = 0;
bool content_flag = false, no_content_flag = false;
bool list_okay = true;
- bool opt_mode = false;
progname = argv[0];
colour = isatty (STDOUT_FILENO);
@@ -281,11 +280,9 @@ main (int argc, char *argv[])
nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
/* Set optional modes in the handle. */
- opt_mode = !can && !map && !size_only;
- if (opt_mode) {
-nbd_set_opt_mode (nbd, true);
+ nbd_set_opt_mode (nbd, true);
+ if (!can && !map && !size_only)
nbd_set_full_info (nbd, true);
- }
if (map)
nbd_add_meta_context (nbd, map);
@@ -398,6 +395,13 @@ do_connect (struct nbd_handle *nbd)
fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
exit (EXIT_FAILURE);
}
+
+ /* If we are in opt mode, request info on the original export name.
+ * However, ignoring failure at this time is okay, as later code
+ * may want to try an alternate export name.
+ */
+ if (nbd_aio_is_negotiating (nbd))
+nbd_opt_info (nbd);
}
/* The URI field in output is not meaningful unless there's a
diff --git a/info/map.c b/info/map.c
index 594f24ff..399e0355 100644
--- a/info/map.c
+++ b/info/map.c
@@ -54,6 +54,13 @@ do_map (void)
uint64_t offset, align, max_len;
size_t prev_entries_size;
+ /* Map mode requires switching over to transmission phase. */
+ if (nbd_aio_is_negotiating (nbd) &&
+ nbd_opt_go (nbd) == -1) {
+fprintf (stderr, "%s: %s\n", progname, nbd_get_error ());
+exit (EXIT_FAILURE);
+ }
+
/* Did we get the requested map? */
if (!nbd_can_meta_context (nbd, map)) {
fprintf (stderr,
--
2.41.0
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] libnbd | Failed pipeline for master | b244bacc
Pipeline #993040668 has failed! Project: libnbd ( https://gitlab.com/nbdkit/libnbd ) Branch: master ( https://gitlab.com/nbdkit/libnbd/-/commits/master ) Commit: b244bacc ( https://gitlab.com/nbdkit/libnbd/-/commit/b244bacce5acdfaa96ef1c1d5b253abcb8343e7a ) Commit Message: copy: Allow human sizes for --queue-size, --req... Commit Author: Richard W_M_ Jones ( https://gitlab.com/rwmjones ) Pipeline #993040668 ( https://gitlab.com/nbdkit/libnbd/-/pipelines/993040668 ) triggered by Richard W_M_ Jones ( https://gitlab.com/rwmjones ) had 5 failed jobs. Job #5019314566 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019314566/raw ) Stage: builds Name: x86_64-freebsd-current Job #5019314562 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019314562/raw ) Stage: builds Name: x86_64-freebsd-12 Job #5019314565 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019314565/raw ) Stage: builds Name: x86_64-freebsd-13 Job #5019314567 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019314567/raw ) Stage: builds Name: aarch64-macos-13 Job #5019314527 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019314527/raw ) Stage: builds Name: x86_64-opensuse-leap-15-prebuilt-env -- You're receiving this email because of your account on gitlab.com. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse
On Mon, Sep 04, 2023 at 10:13:45AM +0200, Laszlo Ersek wrote:
> On 9/3/23 17:23, Richard W.M. Jones wrote:
> > Allow these options to be specified using human sizes, for example
> > this now works:
> >
> > nbdcopy --request-size=32M ...
> > ---
> > copy/copy-sparse-allocated.sh| 2 +-
> > copy/copy-sparse-no-extents.sh | 2 +-
> > copy/copy-sparse-request-size.sh | 2 +-
> > copy/main.c | 37
> > copy/nbdcopy.h | 2 +-
> > 5 files changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/copy/copy-sparse-allocated.sh b/copy/copy-sparse-allocated.sh
> > index c65ddea79f..e1fe9cf463 100755
> > --- a/copy/copy-sparse-allocated.sh
> > +++ b/copy/copy-sparse-allocated.sh
> > @@ -31,7 +31,7 @@ requires nbdkit eval --version
> > out=copy-sparse-allocated.out
> > cleanup_fn rm -f $out
> >
> > -$VG nbdcopy --allocated --request-size=32768 -- \
> > +$VG nbdcopy --allocated --request-size=32K -- \
> > [ nbdkit --exit-with-parent data data='
> > 1
> > @1073741823 1
> > diff --git a/copy/copy-sparse-no-extents.sh b/copy/copy-sparse-no-extents.sh
> > index cff356978b..9368c564e9 100755
> > --- a/copy/copy-sparse-no-extents.sh
> > +++ b/copy/copy-sparse-no-extents.sh
> > @@ -39,7 +39,7 @@ requires nbdkit eval --version
> > out=copy-sparse-no-extents.out
> > cleanup_fn rm -f $out
> >
> > -$VG nbdcopy --request-size=33554432 --no-extents -S 0 -- \
> > +$VG nbdcopy --request-size=32M --no-extents -S 0 -- \
> > [ nbdkit --exit-with-parent data data='
> > 1
> > @1073741823 1
> > diff --git a/copy/copy-sparse-request-size.sh
> > b/copy/copy-sparse-request-size.sh
> > index dc8caeafd1..dd28695f68 100755
> > --- a/copy/copy-sparse-request-size.sh
> > +++ b/copy/copy-sparse-request-size.sh
> > @@ -39,7 +39,7 @@ requires nbdkit eval --version
> > out=copy-sparse-request-size.out
> > cleanup_fn rm -f $out
> >
> > -$VG nbdcopy --no-extents -S 0 --request-size=1048576 -- \
> > +$VG nbdcopy --no-extents -S 0 --request-size=1M -- \
> > [ nbdkit --exit-with-parent data data='
> > 1
> > @33554431 1
> > diff --git a/copy/main.c b/copy/main.c
> > index 6928a4acde..47b1ea8be0 100644
> > --- a/copy/main.c
> > +++ b/copy/main.c
> > @@ -141,6 +141,8 @@ main (int argc, char *argv[])
> >};
> >int c;
> >size_t i;
> > + int64_t i64;
> > + const char *error, *pstr;
> >
> >/* Set prog to basename argv[0]. */
> >prog = strrchr (argv[0], '/');
> > @@ -210,26 +212,32 @@ main (int argc, char *argv[])
> >break;
> >
> > case QUEUE_SIZE_OPTION:
> > - if (sscanf (optarg, "%u", _size) != 1) {
> > -fprintf (stderr, "%s: --queue-size: could not parse: %s\n",
> > - prog, optarg);
> > + i64 = human_size_parse (optarg, , );
> > + if (i64 == -1) {
> > +fprintf (stderr, "%s: --queue-size: %s: %s\n", prog, error, pstr);
> > exit (EXIT_FAILURE);
> >}
> > + if (i64 > UINT_MAX) {
> > +fprintf (stderr, "%s: --queue-size is too large\n", prog);
>
> (1) Print "optarg" (or even format back "i64") here?
>
> > +exit (EXIT_FAILURE);
> > + }
> > + queue_size = i64;
> >break;
> >
> > case REQUEST_SIZE_OPTION:
> > - if (sscanf (optarg, "%u", _size) != 1) {
> > -fprintf (stderr, "%s: --request-size: could not parse: %s\n",
> > - prog, optarg);
> > + i64 = human_size_parse (optarg, , );
> > + if (i64 == -1) {
> > +fprintf (stderr, "%s: --request-size: %s: %s\n", prog, error,
> > pstr);
> > exit (EXIT_FAILURE);
> >}
> > - if (request_size < MIN_REQUEST_SIZE || request_size >
> > MAX_REQUEST_SIZE ||
> > - !is_power_of_2 (request_size)) {
> > + if (i64 < MIN_REQUEST_SIZE || i64 > MAX_REQUEST_SIZE ||
> > + !is_power_of_2 (i64)) {
> > fprintf (stderr,
> > "%s: --request-size: must be a power of 2 within %d-%d\n",
> > prog, MIN_REQUEST_SIZE, MAX_REQUEST_SIZE);
>
> (2) Same comment as (1).
>
> (Albeit not as much justified as at (1). At (1), the patch *stops*
> printing the out-of-range "optarg", while at (2), the patch *continues
> not to print* the out-of-range "optarg".)
>
> > exit (EXIT_FAILURE);
> >}
> > + request_size = i64;
> >break;
>
> (3) I'll come back to this later...
>
> >
> > case 'R':
> > @@ -241,17 +249,18 @@ main (int argc, char *argv[])
> >break;
> >
> > case 'S':
> > - if (sscanf (optarg, "%u", _size) != 1) {
> > -fprintf (stderr, "%s: --sparse: could not parse: %s\n",
> > - prog, optarg);
> > + i64 = human_size_parse (optarg, , );
> > + if (i64 == -1) {
> > +fprintf (stderr, "%s: --sparse: %s: %s\n", prog, error, pstr);
> > exit (EXIT_FAILURE);
> >}
> > -
[Libguestfs] libnbd | Failed pipeline for master | d814ac4d
Pipeline #993006501 has failed! Project: libnbd ( https://gitlab.com/nbdkit/libnbd ) Branch: master ( https://gitlab.com/nbdkit/libnbd/-/commits/master ) Commit: d814ac4d ( https://gitlab.com/nbdkit/libnbd/-/commit/d814ac4d47cd75b81bb6bb2c9a779d613a1228d0 ) Commit Message: rust: Cleanups in examples/concurrent-read-writ... Commit Author: Tage Johansson Committed by: Richard W_M_ Jones ( https://gitlab.com/rwmjones ) Pipeline #993006501 ( https://gitlab.com/nbdkit/libnbd/-/pipelines/993006501 ) triggered by Richard W_M_ Jones ( https://gitlab.com/rwmjones ) had 5 failed jobs. Job #5019117404 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019117404/raw ) Stage: builds Name: x86_64-freebsd-13 Job #5019117397 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019117397/raw ) Stage: builds Name: x86_64-freebsd-12 Job #5019117408 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019117408/raw ) Stage: builds Name: x86_64-freebsd-current Job #5019117414 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019117414/raw ) Stage: builds Name: aarch64-macos-13 Job #5019117353 ( https://gitlab.com/nbdkit/libnbd/-/jobs/5019117353/raw ) Stage: builds Name: x86_64-opensuse-leap-15-prebuilt-env -- You're receiving this email because of your account on gitlab.com. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit] server: Move size parsing code (nbdkit_parse_size) to common/include
On Mon, Sep 04, 2023 at 09:52:53AM +0200, Laszlo Ersek wrote:
> On 9/3/23 17:22, Richard W.M. Jones wrote:
> > This is broadly simple code motion, intended so that we can reuse the
> > same code in libnbd.
> > ---
> > common/include/Makefile.am | 6 ++
> > common/include/human-size.h | 137 +++
> > common/include/test-human-size.c | 133 ++
> > server/public.c | 78 ++
> > .gitignore | 1 +
> > 5 files changed, 283 insertions(+), 72 deletions(-)
> >
> > diff --git a/common/include/Makefile.am b/common/include/Makefile.am
> > index 3162e92c2..8e4de04f3 100644
> > --- a/common/include/Makefile.am
> > +++ b/common/include/Makefile.am
> > @@ -42,6 +42,7 @@ EXTRA_DIST = \
> > checked-overflow.h \
> > compiler-macros.h \
> > hexdigit.h \
> > + human-size.h \
> > isaligned.h \
> > ispowerof2.h \
> > iszero.h \
> > @@ -63,6 +64,7 @@ TESTS = \
> > test-ascii-string \
> > test-byte-swapping \
> > test-checked-overflow \
> > + test-human-size \
> > test-isaligned \
> > test-ispowerof2 \
> > test-iszero \
> > @@ -93,6 +95,10 @@ test_checked_overflow_SOURCES = test-checked-overflow.c
> > checked-overflow.h
> > test_checked_overflow_CPPFLAGS = -I$(srcdir)
> > test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
> >
> > +test_human_size_SOURCES = test-human-size.c human-size.h
> > +test_human_size_CPPFLAGS = -I$(srcdir)
> > +test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
> > +
> > test_isaligned_SOURCES = test-isaligned.c isaligned.h
> > test_isaligned_CPPFLAGS = -I$(srcdir)
> > test_isaligned_CFLAGS = $(WARNINGS_CFLAGS)
> > diff --git a/common/include/human-size.h b/common/include/human-size.h
> > new file mode 100644
> > index 0..788dbd7ba
> > --- /dev/null
> > +++ b/common/include/human-size.h
> > @@ -0,0 +1,137 @@
> > +/* nbdkit
> > + * Copyright Red Hat
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions are
> > + * met:
> > + *
> > + * * Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + *
> > + * * Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in the
> > + * documentation and/or other materials provided with the distribution.
> > + *
> > + * * Neither the name of Red Hat nor the names of its contributors may be
> > + * used to endorse or promote products derived from this software without
> > + * specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> > + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> > + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> > + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> > + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> > + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > + * SUCH DAMAGE.
> > + */
> > +
> > +#ifndef NBDKIT_HUMAN_SIZE_H
> > +#define NBDKIT_HUMAN_SIZE_H
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +/* Attempt to parse a string with a possible scaling suffix, such as
> > + * "2M". Disk sizes cannot usefully exceed off_t (which is signed)
> > + * and cannot be negative.
> > + *
> > + * On error, returns -1 and sets *error and *pstr. You can form a
> > + * final error message by appending ": ".
> > + */
> > +static inline int64_t
> > +human_size_parse (const char *str,
> > + const char **error, const char **pstr)
> > +{
> > + int64_t size;
> > + char *end;
> > + uint64_t scale = 1;
> > +
> > + /* XXX Should we also parse things like '1.5M'? */
> > + /* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
> > + * because some of them are valid hex digits.
> > + */
> > + errno = 0;
> > + size = strtoimax (str, , 10);
>
> (1) A further improvement here (likely best done in a separate patch)
> could be to change the type of "size" to "intmax_t", from "int64_t".
> That way, the assignment will be safe even theoretically, *and* the
> overflow check at the bottom of the function (with the division &
> comparison of the quotient against INT_MAX) will work just the same.
I'm always very unsure how this all works. In particular I seem to
recall that intmax_t is no longer really the maximum possible int
(because of int128) and
Re: [Libguestfs] [libnbd PATCH] rust: Cleanups in examples/concurrent-read-write.rs.
On Mon, Sep 04, 2023 at 03:21:10PM +, Tage Johansson wrote:
> This patch makes some small cleanups in
> rust/examples/concurrent-read-write.rs. Specificly, it refrases one
> comment, removes some outcommented code, and removes a completely
> redundent if statement. It also replaces a hard coded number with the
> index of the task as seed to an RNG.
> ---
> rust/examples/concurrent-read-write.rs | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/rust/examples/concurrent-read-write.rs
> b/rust/examples/concurrent-read-write.rs
> index 4858f76..589cefd 100644
> --- a/rust/examples/concurrent-read-write.rs
> +++ b/rust/examples/concurrent-read-write.rs
> @@ -3,11 +3,10 @@
> //! --run 'cargo run --example concurrent-read-write -- $unixsocket'
> //! Or connect over a URI:
> //! nbdkit -U - memory 100M \
> -//! --run 'cargo run --example concurrent-read-write -- $uri'
> +//! --run 'cargo run --example concurrent-read-write -- "$uri"'
> //!
> -//! This will read and write randomly over the first megabyte of the
> -//! plugin using multi-conn, multiple threads and multiple requests in
> -//! flight on each thread.
> +//! This will read and write randomly over the plugin using multi-conn,
> +//! multiple threads and multiple requests in flight on each thread.
>
> #![deny(warnings)]
> use rand::prelude::*;
> @@ -111,12 +110,11 @@ async fn run_thread(
> nbd.connect_unix(socket_or_uri).await?;
> }
>
> -let mut rng = SmallRng::seed_from_u64(44 as u64);
> +let mut rng = SmallRng::seed_from_u64(task_idx as u64);
>
> // Issue commands.
> let mut stats = Stats::default();
> let mut join_set = JoinSet::new();
> -//tokio::time::sleep(std::time::Duration::from_secs(1)).await;
> while stats.requests < NR_CYCLES || !join_set.is_empty() {
> while stats.requests < NR_CYCLES && join_set.len() < MAX_IN_FLIGHT {
> // If we want to issue another request, do so. Note that we
> reuse
> @@ -144,6 +142,5 @@ async fn run_thread(
> join_set.join_next().await.unwrap().unwrap()?;
> }
>
> -if task_idx == 0 {}
> Ok(stats)
> }
>
> base-commit: 9afb980d05d6144129c899285e44779757a380e8
> prerequisite-patch-id: 8d1779610795021ed5a3d0973ddf9ef854cd6a24
> prerequisite-patch-id: 1f0f11f11ac9b1ff0be08f5aa0a9904ba4de
> prerequisite-patch-id: dc4af7343c57a4f99dc82918c07470030e542747
> prerequisite-patch-id: 8e8f7a043c80d6c24e883967f5bd952a64db1228
> prerequisite-patch-id: ba7b3482e2e16f76b5f285daeeda30b31a841912
> prerequisite-patch-id: 219a9595e550a8caf43d466dcb2b044114e1b7bf
> prerequisite-patch-id: 3de46c9673221bff1d897970aa983b3f8e6cab74
> prerequisite-patch-id: 4235d5e174fce05b9a947b3b838bebf968f0fa6a
> prerequisite-patch-id: 07773355d5718e0593c4030a8f035fc11fea3715
> prerequisite-patch-id: f023deea8b706706e3c2980403064d90a254af3c
> prerequisite-patch-id: 8639c6cc4fec58f4761771c5d8a9476d538c6251
> prerequisite-patch-id: 9bc660aed54a6266b014993ff0f388a26ac2982a
> --
Thanks - pushed, rebased on current HEAD.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
