Re: [Libguestfs] [libnbd PATCH] rust: Use mio::poll instead of requiring epoll

[Tage Johansson]

On 9/4/2023 8:23 PM, Eric Blake wrote:

On Mon, Sep 04, 2023 at 04:07:54PM +, Tage Johansson wrote:

From: Eric Blake 

CI shows our async handle fails to build on FreeBSD and MacOS (where
epoll() is not available as a syscall, and therefore not available as
a Rust crate).  We can instead accomplish the same level-probing
effects by doing a zero-timeout poll with mio (which defers under the
hood to epoll on Linux, and kqueue on BSD).

Fixes: 223a9965 ("rust: async: Create an async friendly handle type")
CC: Tage Johansson 
Signed-off-by: Eric Blake 
---
  rust/Cargo.toml  |  3 ++-
  rust/src/async_handle.rs | 40 +++-
  2 files changed, 25 insertions(+), 18 deletions(-)

-// Use epoll to check the current read/write availability on the fd.
+// Use mio poll to check the current read/write availability on the fd.
  // This is needed because Tokio supports only edge-triggered
  // notifications but Libnbd requires level-triggered notifications.
-let mut revent = epoll::Event { data: 0, events: 0 };
  // Setting timeout to 0 means that it will return immediately.
-epoll::wait(epfd, 0, std::slice::from_mut( revent))?;
-let revents = Events::from_bits(revent.events).unwrap();
-if !revents.contains(Events::EPOLLIN) {
-ready_guard.clear_ready_matching(IoReady::READABLE);
-}
-if !revents.contains(Events::EPOLLOUT) {
-ready_guard.clear_ready_matching(IoReady::WRITABLE);
+// mio states that it is OS-dependent on whether a single event
+// 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.



--

Best regards,

Tage



+for event in  {
+if !event.is_readable() {
+ready_guard.clear_ready_matching(IoReady::READABLE);
+}
+if !event.is_writable() {
+ready_guard.clear_ready_matching(IoReady::WRITABLE);
+}
  }
  ready_guard.retain_ready();
+poll.registry().deregister( SourceFd())?;

Furthermore, if the regsiter()/deregister() should always occur in
pairs, the early poll()? exit breaks that assumption (I don't know if
Rust has enough smarts to automatically clean up the unpaired
registration).



___
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

[Eric Blake]

On Mon, Sep 04, 2023 at 04:07:54PM +, Tage Johansson wrote:
> From: Eric Blake 
> 
> CI shows our async handle fails to build on FreeBSD and MacOS (where
> epoll() is not available as a syscall, and therefore not available as
> a Rust crate).  We can instead accomplish the same level-probing
> effects by doing a zero-timeout poll with mio (which defers under the
> hood to epoll on Linux, and kqueue on BSD).
> 
> Fixes: 223a9965 ("rust: async: Create an async friendly handle type")
> CC: Tage Johansson 
> Signed-off-by: Eric Blake 
> ---
>  rust/Cargo.toml  |  3 ++-
>  rust/src/async_handle.rs | 40 +++-
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> -// Use epoll to check the current read/write availability on the fd.
> +// Use mio poll to check the current read/write availability on the 
> fd.
>  // This is needed because Tokio supports only edge-triggered
>  // notifications but Libnbd requires level-triggered notifications.
> -let mut revent = epoll::Event { data: 0, events: 0 };
>  // Setting timeout to 0 means that it will return immediately.
> -epoll::wait(epfd, 0, std::slice::from_mut( revent))?;
> -let revents = Events::from_bits(revent.events).unwrap();
> -if !revents.contains(Events::EPOLLIN) {
> -ready_guard.clear_ready_matching(IoReady::READABLE);
> -}
> -if !revents.contains(Events::EPOLLOUT) {
> -ready_guard.clear_ready_matching(IoReady::WRITABLE);
> +// mio states that it is OS-dependent on whether a single event
> +// 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.

> +for event in  {
> +if !event.is_readable() {
> +ready_guard.clear_ready_matching(IoReady::READABLE);
> +}
> +if !event.is_writable() {
> +ready_guard.clear_ready_matching(IoReady::WRITABLE);
> +}
>  }
>  ready_guard.retain_ready();
> +poll.registry().deregister( SourceFd())?;

Furthermore, if the regsiter()/deregister() should always occur in
pairs, the early poll()? exit breaks that assumption (I don't know if
Rust has enough smarts to automatically clean up the unpaired
registration).

-- 
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] rust: Use mio::poll instead of requiring epoll

[Tage Johansson]

From: Eric Blake 

CI shows our async handle fails to build on FreeBSD and MacOS (where
epoll() is not available as a syscall, and therefore not available as
a Rust crate).  We can instead accomplish the same level-probing
effects by doing a zero-timeout poll with mio (which defers under the
hood to epoll on Linux, and kqueue on BSD).

Fixes: 223a9965 ("rust: async: Create an async friendly handle type")
CC: Tage Johansson 
Signed-off-by: Eric Blake 
---
 rust/Cargo.toml  |  3 ++-
 rust/src/async_handle.rs | 40 +++-
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/rust/Cargo.toml b/rust/Cargo.toml
index 0879b34..678848a 100644
--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -49,9 +49,10 @@ thiserror = "1.0.40"
 log = { version = "0.4.19", optional = true }
 libc = "0.2.147"
 tokio = { optional = true, version = "1.29.1", default-features = false, 
features = ["rt", "sync", "net"] }
-epoll = "4.3.3"
+mio = { optional = true, version = "0.8.0" }
 
 [features]
+tokio = ["dep:tokio", "dep:mio"]
 default = ["log", "tokio"]
 
 [dev-dependencies]
diff --git a/rust/src/async_handle.rs b/rust/src/async_handle.rs
index 6793ce9..8f3e8df 100644
--- a/rust/src/async_handle.rs
+++ b/rust/src/async_handle.rs
@@ -35,9 +35,11 @@ use crate::sys;
 use crate::Handle;
 use crate::{Error, FatalErrorKind, Result};
 use crate::{AIO_DIRECTION_BOTH, AIO_DIRECTION_READ, AIO_DIRECTION_WRITE};
-use epoll::Events;
+use mio::unix::SourceFd;
+use mio::{Events, Interest as MioInterest, Poll, Token};
 use std::sync::Arc;
 use std::sync::Mutex;
+use std::time::Duration;
 use tokio::io::{unix::AsyncFd, Interest, Ready as IoReady};
 use tokio::sync::Notify;
 use tokio::task;
@@ -176,13 +178,8 @@ async fn polling_task(handle_data: ) -> 
Result<(), FatalErrorKind> {
 } = handle_data;
 let fd = handle.aio_get_fd().map_err(Error::to_fatal)?;
 let tokio_fd = AsyncFd::new(fd)?;
-let epfd = epoll::create(false)?;
-epoll::ctl(
-epfd,
-epoll::ControlOptions::EPOLL_CTL_ADD,
-fd,
-epoll::Event::new(Events::EPOLLIN | Events::EPOLLOUT, 42),
-)?;
+let mut events = Events::with_capacity(1);
+let mut poll = Poll::new()?;
 
 // The following loop does approximately the following things:
 //
@@ -248,19 +245,28 @@ async fn polling_task(handle_data: ) -> 
Result<(), FatalErrorKind> {
 }
 drop(pending_cmds_lock);
 
-// Use epoll to check the current read/write availability on the fd.
+// Use mio poll to check the current read/write availability on the fd.
 // This is needed because Tokio supports only edge-triggered
 // notifications but Libnbd requires level-triggered notifications.
-let mut revent = epoll::Event { data: 0, events: 0 };
 // Setting timeout to 0 means that it will return immediately.
-epoll::wait(epfd, 0, std::slice::from_mut( revent))?;
-let revents = Events::from_bits(revent.events).unwrap();
-if !revents.contains(Events::EPOLLIN) {
-ready_guard.clear_ready_matching(IoReady::READABLE);
-}
-if !revents.contains(Events::EPOLLOUT) {
-ready_guard.clear_ready_matching(IoReady::WRITABLE);
+// mio states that it is OS-dependent on whether a single event
+// 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))?;
+for event in  {
+if !event.is_readable() {
+ready_guard.clear_ready_matching(IoReady::READABLE);
+}
+if !event.is_writable() {
+ready_guard.clear_ready_matching(IoReady::WRITABLE);
+}
 }
 ready_guard.retain_ready();
+poll.registry().deregister( SourceFd())?;
 }
 }

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: dd415a31fa127d90bcc7d993a2659e39d7d4cae8
prerequisite-patch-id: 618cc5f574207f36818ef803f2e586314c2458f5
prerequisite-patch-id: 8639c6cc4fec58f4761771c5d8a9476d538c6251
prerequisite-patch-id: 

Re: [Libguestfs] [libnbd PATCH 2/2] rust: Use mio::poll instead of requiring epoll

[Tage Johansson]

On 9/1/2023 3:41 PM, Eric Blake wrote:

On Fri, Sep 01, 2023 at 08:35:58AM +, Tage Johansson wrote:

On 8/30/2023 10:11 PM, Eric Blake wrote:

CI shows our async handle fails to build on FreeBSD and MacOS (where
epoll() is not available as a syscall, and therefore not available as
a Rust crate).  We can instead accomplish the same level-probing
effects by doing a zero-timeout poll with mio (which defers under the
hood to epoll on Linux, and kqueue on BSD).


The problem with mio is that it uses edge triggered notifications. According
to this page :


Draining readiness
Once a readiness event is received, the corresponding operation must be
performed repeatedly until it returns variant
std::io::error::ErrorKind::WouldBlock. Unless this is done, there is no
guarantee that another readiness event will be delivered, even if
further data is received for the event source.

Would that still be true even if we deregister the interest after our
one-shot poll, and reregister it immediately before-hand?  In other
words, even if registration sets up edge triggers for future
notifications, the fact that we are only doing a one-shot query seems
like it would be sufficient to use our one-shot as a level probe
(assuming that the registration starts by learning the current state
of the fd before waiting for any future edges).

I agree that once we get a notification after a .poll(), the normal
assumption is that we must loop and consume all data before calling
.poll again (because the .poll would block until the next edge - but
if we didn't consume to the blocking point, there is no next edge).
But since we are NOT looping, nor doing any consumption work, our
timeout of 0 is trying to behave as an instant level check, and if
adding a reregister/deregister wrapper around the .poll makes it more
reliable (by wiping out all prior events and re-priming the
registration to start with the current level), that may be all the
more we need.



It should work. I tried to do that and it works on my machine at least. 
Although it might be a bit uggly, it may be a short term solution. I 
will send your patch with these modifications soon as reply to this message.




The motivation behind using epoll in addition to tokio in the first place
was to check if fd is readable/writable without necessarily knowing if the
last read/write operation returned [ErrorKind::WouldBlock]. And it doesn't
seems that mio full fills that requirenment.

Do we need to expand the libnbd API itself to make it more obvious
whether our state machine completed because it hit a EWOULDBLOCK
scenario?  In general, once you kick the state machine (by sending a
new command, or calling one of the two aio_notify calls), the state
machine tries to then process as much data as possible until it blocks
again, rather than stopping after just processing a single
transaction.  That is, if libnbd isn't already designed to work with
edge triggers, how much harder would it be to modify it (perhaps by
adding a new API) so that it DOES work nicely with edge triggers?



This would be the cleanest and best solution I think. Adding two methods 
to the handle: aio_read_blocked() and aio_write_blocked() which return 
true iff the last read/write operation blocked. It could be two bool 
fields on the handle struct which are set to false by default and reset 
to false whenever a read/write operation is performed without blocking. 
It is very important that aio_read_blocked() will return true only if 
the last read operation blocked, (and the same thing for 
aio_write_blocked()).




If we can solve _that_ problem, then the Rust async handle wouldn't
need to use epoll, mio, or anything else; tokio's edge trigger
behavior would already be sufficient on its own.



Yes, this is true. It would be more efficient, easier to understand, and 
require less dependencies.



--

Best regards,

Tage


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [libnbd PATCH] rust: Cleanups in examples/concurrent-read-write.rs.

[Tage Johansson]

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
-- 
2.42.0

___
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

[Tage Johansson]

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.



+//! 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.




+
+/// Number of commands we issue (per [task][tokio::task]).
+const NR_CYCLES: usize = 32;
+
+/// Statistics gathered during the run.
+#[derive(Debug, Default)]
+struct Stats {
+/// The total number of requests made.
+requests: usize,
+}
+
+#[tokio::main]
+async fn main() -> anyhow::Result<()> {
+let args = env::args_os().collect::>();
+if args.len() != 2 {
+anyhow::bail!("Usage: {:?} socket", args[0]);
+}
+
+// We begin by making a connection to the server to get the export size
+// and ensure that it supports multiple connections and is writable.
+let nbd = libnbd::Handle::new()?;
+
+// Check if the user provided a URI or a unix socket.
+let socket_or_uri = args[1].to_str().unwrap();
+if socket_or_uri.contains("://") {
+nbd.connect_uri(socket_or_uri)?;
+} else {
+nbd.connect_unix(socket_or_uri)?;
+}
+
+let export_size = nbd.get_size()?;
+anyhow::ensure!(
+(BUFFER_SIZE as u64) < export_size,
+"export is {export_size}B, must be larger than {BUFFER_SIZE}B"
+);
+anyhow::ensure!(
+!nbd.is_read_only()?,
+"error: this NBD export is read-only"
+);
+anyhow::ensure!(
+nbd.can_multi_conn()?,
+"error: this NBD export does not support multi-conn"
+);
+drop(nbd); // Close the connection.
+
+// Start the worker tasks, one per connection.
+let mut tasks = JoinSet::new();
+for i in 0..NR_MULTI_CONN {
+tasks.spawn(run_thread(i, socket_or_uri.to_owned(), export_size));
+}
+
+// Wait for the tasks to complete.
+let mut stats = Stats::default();
+while !tasks.is_empty() {
+let this_stats = tasks.join_next().await.unwrap().unwrap()?;
+stats.requests += this_stats.requests;
+}
+
+// Make sure the number of requests that were required matches what
+// we expect.
+assert_eq!(stats.requests, NR_MULTI_CONN * NR_CYCLES);
+
+Ok(())
+}
+
+async fn run_thread(
+task_idx: usize,
+socket_or_uri: String,
+export_size: u64,
+) -> 

Re: [Libguestfs] [PATCH libnbd 5/5] copy: Allow human sizes for --queue-size, --request-size, --sparse

[Laszlo Ersek]

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);
>}
> -  if (sparse_size != 0 &&
> -  (sparse_size < 512 || !is_power_of_2 (sparse_size))) {
> -fprintf (stderr, "%s: --sparse: must be a power of 2 and >= 512\n",
> +  if (i64 != 0 &&
> +  (i64 < 512 || i64 > UINT_MAX || !is_power_of_2 (i64))) {
> +fprintf (stderr, "%s: --sparse: must be a power of 2, 

Re: [Libguestfs] [PATCH libnbd 4/5] Revert "copy, info: Include common/utils/human-size.h"

[Laszlo Ersek]

On 9/3/23 17:23, Richard W.M. Jones wrote:
> This reverts commit XXX FILL IN LATER XXX
> ---
>  copy/main.c | 2 +-
>  info/show.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Laszlo Ersek 


> 
> diff --git a/copy/main.c b/copy/main.c
> index d2f415dc47..6928a4acde 100644
> --- a/copy/main.c
> +++ b/copy/main.c
> @@ -39,7 +39,7 @@
>  #include 
>  
>  #include "ispowerof2.h"
> -#include "../utils/human-size.h"
> +#include "human-size.h"
>  #include "minmax.h"
>  #include "version.h"
>  #include "nbdcopy.h"
> diff --git a/info/show.c b/info/show.c
> index 99b7b5b60a..920bbb0a27 100644
> --- a/info/show.c
> +++ b/info/show.c
> @@ -29,7 +29,7 @@
>  #include 
>  
>  #include "ansi-colours.h"
> -#include "../utils/human-size.h"
> +#include "human-size.h"
>  #include "string-vector.h"
>  
>  #include "nbdinfo.h"

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd 3/5] common: Combine human-size.h headers into one

[Laszlo Ersek]

On 9/3/23 17:23, Richard W.M. Jones wrote:
> Copy the human_size() function from common/utils/ into the new
> human-size.h header in common/include/.  Remove human-size.c and
> combine the tests into one.
> ---
>  common/include/human-size.h  | 51 ++
>  common/include/test-human-size.c | 79 +---
>  common/utils/Makefile.am | 10 +---
>  common/utils/human-size.c| 56 
>  common/utils/human-size.h| 49 --
>  common/utils/test-human-size.c   | 89 
>  6 files changed, 126 insertions(+), 208 deletions(-)

Hopefully you won't get too many rebase conflicts here...

Reviewed-by: Laszlo Ersek 


___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd 1/5] copy, info: Include common/utils/human-size.h

[Laszlo Ersek]

On 9/3/23 17:23, Richard W.M. Jones wrote:
> The next commit will add a new "human-size.h" header copied in from
> nbdkit.  This breaks existing code that uses common/utils/human-size.h
> (which we will combine later).
> 
> As a temporary hack to maintain bisection, make sure we are using the
> deprecated "human-size.h" header.  This hack will be removed later.
> ---
>  copy/main.c | 2 +-
>  info/show.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/copy/main.c b/copy/main.c
> index 6928a4acde..d2f415dc47 100644
> --- a/copy/main.c
> +++ b/copy/main.c
> @@ -39,7 +39,7 @@
>  #include 
>  
>  #include "ispowerof2.h"
> -#include "human-size.h"
> +#include "../utils/human-size.h"
>  #include "minmax.h"
>  #include "version.h"
>  #include "nbdcopy.h"
> diff --git a/info/show.c b/info/show.c
> index 920bbb0a27..99b7b5b60a 100644
> --- a/info/show.c
> +++ b/info/show.c
> @@ -29,7 +29,7 @@
>  #include 
>  
>  #include "ansi-colours.h"
> -#include "human-size.h"
> +#include "../utils/human-size.h"
>  #include "string-vector.h"
>  
>  #include "nbdinfo.h"

Reviewed-by: Laszlo Ersek 

___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd 2/5] common/include: Import the human_size_parse (nbdkit_parse_size) function

[Laszlo Ersek]

On 9/3/23 17:23, Richard W.M. Jones wrote:
> This function is copied from nbdkit commit XXX [fill in after nbdkit
> change is upstream] XXX

Right, if you end up updating the nbdkit patch, please refresh this one.

Reviewed-by: Laszlo Ersek 

Laszlo

___
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

[Laszlo Ersek]

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.

> +  if (str == end) {
> +*error = "could not parse size string";
> +*pstr = str;
> +return -1;
> +  }
> +  if (size < 0) {
> +*error = "size cannot be negative";
> +*pstr = str;
> +return -1;
> +  }
> +  if (errno) {
> +*error = "size exceeds maximum value";
> +*pstr = str;
> +return -1;
> +  }
> +
> +  switch (*end) {
> +/* No suffix */
> +  case '\0':
> +end--; /* Safe,