Re: [Libguestfs] Preliminary release notes for libguestfs 1.50, guestfs-tools 1.50

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 01:03:46PM -0600, Eric Blake wrote:
> On Tue, Jan 31, 2023 at 06:36:04PM +, Richard W.M. Jones wrote:
> > 
> > $SUBJECT here ...
> > 
> >   
> > https://github.com/libguestfs/libguestfs/blob/master/docs/guestfs-release-notes-1.50.pod
> 
> Currently contains a pod error:
>  
> https://github.com/libguestfs/libguestfs/blob/master/docs/guestfs-release-notes-1.50.pod#pod-errors
> 
> I presume there's a way to mark the .pod file, or at least tweak the
> Makefile that renders the page from .pod, to state that the input is
> intentionally UTF-8, in order to clear out that error.

So the reason for this is because what we write isn't strictly a POD
file.  Instead, podwrapper[1] adds extra stuff at the top and bottom
of the file before parsing it.  One of the things it adds is the
correct "=encoding utf8" directive, which is necessary because POD
otherwise assumes the input is 7 bit ASCII.

The reason that we don't just write POD directly is because:

 - We want to add standard sections at the end.

 - To avoid a complicated problem in translated files generated by
   po4a where it would generate bad =encoding lines[2]

Of course github's renderer knows nothing about this and assumes the
source must be ASCII [by the looks of it, it's running pod2html on the
file].

We might see if the po4a problem is fixed now (or if we can fix it),
and then we could add =encoding lines back into the files, but TBH
it's not high on my list.

Rich.

[1] https://github.com/libguestfs/libguestfs/blob/master/podwrapper.pl.in

[2] https://listman.redhat.com/archives/libguestfs/2014-March/thread.html#8326


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Preliminary release notes for libguestfs 1.50, guestfs-tools 1.50

[Eric Blake]

On Tue, Jan 31, 2023 at 06:36:04PM +, Richard W.M. Jones wrote:
> 
> $SUBJECT here ...
> 
>   
> https://github.com/libguestfs/libguestfs/blob/master/docs/guestfs-release-notes-1.50.pod

Currently contains a pod error:
 
https://github.com/libguestfs/libguestfs/blob/master/docs/guestfs-release-notes-1.50.pod#pod-errors

I presume there's a way to mark the .pod file, or at least tweak the
Makefile that renders the page from .pod, to state that the input is
intentionally UTF-8, in order to clear out that error.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] Preliminary release notes for libguestfs 1.50, guestfs-tools 1.50

[Richard W.M. Jones]

$SUBJECT here ...

  
https://github.com/libguestfs/libguestfs/blob/master/docs/guestfs-release-notes-1.50.pod

  
https://github.com/rwmjones/guestfs-tools/blob/master/docs/guestfs-tools-release-notes-1.50.pod

Any extra stuff that's pending for either project that isn't too
invasive for 1.50, then let me know.

By the way this will be the 28th major release of libguestfs.
Depending on how you count -- we weren't very good about having stable
releases in the early days.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v2 3/4] generator: Add APIs to get/set the socket activation socket name

[Eric Blake]

On Tue, Jan 31, 2023 at 05:28:18PM +, Richard W.M. Jones wrote:
> > > +  /* Check the proposed name is short and alphanumeric. */
> > > +  if (len > 32) {
> > > +set_error (ENAMETOOLONG, "socket activation name should be "
> > > +   "<= 32 characters");
> > 
> > I don't mind keeping us strict to start with and loosening it later if
> > someone demonstrates why it is needed.  But systemd documents a larger
> > set of possible names:
> > 
> > https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html
> > 
> > FDNAME=…
> > 
> > When used in combination with FDSTORE=1, specifies a name for the
> > submitted file descriptors. When used with FDSTOREREMOVE=1,
> > specifies the name for the file descriptors to remove. This name
> > is passed to the service during activation, and may be queried
> > using sd_listen_fds_with_names(3). File descriptors submitted
> > without this field set, will implicitly get the name "stored"
> > assigned. Note that, if multiple file descriptors are submitted at
> > once, the specified name will be assigned to all of them. In order
> > to assign different names to submitted file descriptors, submit
> > them in separate invocations of sd_pid_notify_with_fds(). The name
> > may consist of arbitrary ASCII characters except control
> > characters or ":". It may not be longer than 255 characters. If a
> > submitted name does not follow these restrictions, it is ignored.
> 
> I didn't know about this documentation before.

I only found it this morning.

> 
> Arbitrary ASCII characters doesn't sound that great though.  I'm sure
> that q-s-d will want its own much more strict limitations, eg. I
> assume you can't possibly support any characters which are meaningful
> to JSON / QMP.  Any thoughts on that?

You have a strong point there.  Just because systemd allows it doesn't
make it wise; I'm a big fan of "it's easier to resrict now and loosen
later when we see the need", as being easier than "let's try and be as
loose as we can now, then regret it later when we find it was a
security hole".  Limiting to alphanumeric and 32 bytes until someone
proves they have a use case for needing more than that works for me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 10:41:46AM -0600, Eric Blake wrote:
> On Tue, Jan 31, 2023 at 11:52:27AM +, Richard W.M. Jones wrote:
> > On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
> > > On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones  
> > > wrote:
> > > >
> > > > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > > > > Document all options in --help output.  If -n is not in use, then
> > > > > enhance the banner to print the current state of h, and further tailor
> > > > > the advice given on useful next steps to take to mention opt_go when
> > > > > using --opt-mode.
> > > >
> > > > I had to partially revert this patch (reverting most of it) because it
> > > > unfortunately breaks the implicit handle creation :-(
> > > >
> > > > https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2
> > > >
> > > > I'm not actually sure how to do this correctly in Python.  I made
> > > > several attempts, but I don't think Python is very good about having a
> > > > variable which is only defined on some paths -- maybe it's not
> > > > possible at all.
> > > 
> > > Can you share the error when it breaks?
> > 
> > $ rpm -qf /usr/bin/nbdsh
> > python3-libnbd-1.15.9-2.fc38.x86_64
> > 
> > $ nbdsh 
> > 
> > Welcome to nbdsh, the shell for interacting with
> > Network Block Device (NBD) servers.
> > 
> > The ‘nbd’ module has already been imported and there
> > is an open NBD handle called ‘h’ in state 'START'.
> > 
> > h.connect_tcp("remote", "10809")   # Connect to a remote server.
> > h.get_size()   # Get size of the remote disk.
> > buf = h.pread(512, 0)  # Read the first sector.
> > exit() or Ctrl-D   # Quit the shell
> > help(nbd)  # Display documentation
> > 
> > nbd> print(h)
> > Traceback (most recent call last):
> >   File "/usr/lib64/python3.11/code.py", line 90, in runcode
> > exec(code, self.locals)
> >   File "", line 1, in 
> > NameError: name 'h' is not defined
> 
> Eww. I didn't mean for that to happen, obviously.
> 
> > 
> > > I'm not sure what is the issue, but usually if you have a global
> > > variable created only in
> > > some flows, adding:
> > > 
> > > thing = None
> > > 
> > > At the start of the module makes sure that the name exists later,
> > > regardless of the flow
> > > taken. Code can take the right action based on:
> > > 
> > > if thing is None:
> > > ...
> > 
> > Good point, I was trying 'undef h' instead.  'h' not being present in
> > the dictionary at all vs 'h = None' are slightly different, although I
> > suppose it doesn't matter in this particular case.
> > 
> > The other point is that 'h' (when defined) is not a global.  The patch
> > assumes it is a global, but then uses it in some places as if it is a
> > local.
> 
> Do you want me to try to play with this as well (since I do like the
> improved help message, if we can get it to work reliably), or would I
> be duplicating efforts at this point?

I don't have any further plans here, so if you want to.  However I
would say it's very subtle.  In particular I don't have a clear mental
model of how variable scope works in Python.

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] [PATCH libnbd v2 3/4] generator: Add APIs to get/set the socket activation socket name

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 10:29:00AM -0600, Eric Blake wrote:
> On Mon, Jan 30, 2023 at 10:55:20PM +, Richard W.M. Jones wrote:
> > To allow us to name the socket passed down to the NBD server when
> > calling nbd_connect_systemd_socket_activation(3), we need to add the
> > field to the handle and add access functions.
> > ---
> >  generator/API.ml | 49 ++
> >  lib/handle.c | 56 
> >  lib/internal.h   |  1 +
> >  3 files changed, 106 insertions(+)
> > 
> > diff --git a/generator/API.ml b/generator/API.ml
> > index 25a612a2e8..08fc80960b 100644
> > --- a/generator/API.ml
> > +++ b/generator/API.ml
> > @@ -2036,15 +2036,62 @@   "connect_systemd_socket_activation", {
> >  
> >  When the NBD handle is closed the server subprocess
> >  is killed.
> > +
> > +=head3 Socket name
> > +
> > +The socket activation protocol lets you optionally give
> > +the socket a name.  If used, the name is passed to the
> > +NBD server using the C environment
> > +variable.  To provide a socket name, call
> > +L before calling
> > +the connect function.
> 
> This creates an implicit client-side stateful API: to pass socket
> names, you must call two APIs in the correct sequence:
> 
> nbd_set_socket_activation_name (h, "foo");
> nbd_connect_systemd_socket_activation (h, argv);
> 
> I can live with that design, but I recall Laszlo questioning in the
> past if we always need to force this stateful design onto clients, or
> if it would be easier to instead add a alternative API that takes the
> socket name as an additional parameter, in one shot:
> 
> nbd_connect_systemd_named_socket_activation (h, "foo", argv);

While I'm not totally opposed to this, I would say a few things:

 - the API is already stateful, even used in the most basic way,
   eg you must connect before you can do other things

 - having the state modified by various nbd_set_* calls allows us to
   easily add more variants, instead of having to create a
   combinatorial future set of nbd_connect_systemd_*_socket_activation
   calls

So I would lean towards my design.  (Also it's the same thing we
already do, eg. for opt mode).

> > +int
> > +nbd_unlocked_set_socket_activation_name (struct nbd_handle *h,
> > + const char *name)
> > +{
> > +  size_t i, len;
> > +  char *new_name;
> > +
> > +  len = strlen (name);
> > +
> > +  /* Setting it to empty string stores NULL in the handle. */
> > +  if (len == 0) {
> > +free (h->sa_name);
> > +h->sa_name = NULL;
> > +return 0;
> > +  }
> > +
> > +  /* Check the proposed name is short and alphanumeric. */
> > +  if (len > 32) {
> > +set_error (ENAMETOOLONG, "socket activation name should be "
> > +   "<= 32 characters");
> 
> I don't mind keeping us strict to start with and loosening it later if
> someone demonstrates why it is needed.  But systemd documents a larger
> set of possible names:
> 
> https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html
> 
> FDNAME=…
> 
> When used in combination with FDSTORE=1, specifies a name for the
> submitted file descriptors. When used with FDSTOREREMOVE=1,
> specifies the name for the file descriptors to remove. This name
> is passed to the service during activation, and may be queried
> using sd_listen_fds_with_names(3). File descriptors submitted
> without this field set, will implicitly get the name "stored"
> assigned. Note that, if multiple file descriptors are submitted at
> once, the specified name will be assigned to all of them. In order
> to assign different names to submitted file descriptors, submit
> them in separate invocations of sd_pid_notify_with_fds(). The name
> may consist of arbitrary ASCII characters except control
> characters or ":". It may not be longer than 255 characters. If a
> submitted name does not follow these restrictions, it is ignored.

I didn't know about this documentation before.

Arbitrary ASCII characters doesn't sound that great though.  I'm sure
that q-s-d will want its own much more strict limitations, eg. I
assume you can't possibly support any characters which are meaningful
to JSON / QMP.  Any thoughts on that?

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] [PATCH libnbd] generator: Pass LISTEN_FDNAMES=nbd with systemd socket activation

[Eric Blake]

On Tue, Jan 31, 2023 at 01:07:53PM +, Richard W.M. Jones wrote:
> > 
> > I really didn't want to obsess about this -- I spent like 10+ minutes on
> > curbing my enthusiasm! :) --, but I believe there's a semantic bug in
> > the patch, one that's directly related to my "hidden" thoughts.
> > 
> > (1) In the last hunk, strlen() is applied to "LISTEN_FDS=". However, the
> > zero-index element of the env array holds
> > "LISTEN_PID=". In other words, the patch
> > only gets lucky because "PID" and "FDS" are both three characters long.
> 
> Yup that is totally wrong :-(
> 
> > Relatedly, my hidden thought was that we shouldn't use so many naked
> > string literals all over the code.
> > 
> > May I take a stab at rewriting this? I feel that's the easiest way for
> > me to express what I'd propose. Basically I'd propose:
> > 
> > - an enum for listing the "keys" we need,
> > 
> > - a static array of structure elements, for expressing the environment
> >   variables (name=value), *and* the prefix lengths,
> > 
> > - a macro for populating an element of the array -- use "sizeof" for
> >   grabbing the prefix length
> 
> Sure, go for it please.

More structure will pay off if we have more variables to handle in the
long run.  For just 3 (instead of 2), it's still a toss-up in my mind
if the extra structure is worth it, but I'm not opposed to seeing an
attempt at a patch along these lines.

> 
> > (2) Pre-patch, at commit 5a02c7d2cc6a, the error handling tail of
> > prepare_socket_activation_environment() is less than ideal, IMO. Namely,
> > we have (excerpt)
> > 
> > >  err:
> > >   set_error (errno, "malloc");
> > >   string_vector_iter (env, (void *) free);
> > >   free (env->ptr);
> > >   return -1;
> > 
> > (2a) we free the vector's pointer field, but don't NULL it, nor do we
> > zero the len or cap fields.
> > 
> > We should call string_vector_reset() instead.
> 
> Yup.

The lone caller doesn't utilize env after error, but I agree that we
are better off not leaving this function to expose an incomplete 'env'
back to the caller in case we start using this function in more
places.

> 
> > (2b) Casting the address of the free() function to (void*) makes me
> > uncomfortable. It is undefined behavior by ISO C.
> > 
> > Now, I seem to remember that POSIX says in various places that pointers
> > to functions and pointers to void have identical representation, and
> > also that pointers to void and pointers to structures have identical
> > representation. One of those locations is the dlsym() spec
> > .
> > The other locations elude me, unfortunately. I think at least one of
> > those "other" locations may be in one of the Conformance sections; Eric
> > will know better.

I recall it being discussed in the Austin Group that POSIX explicitly
requires void* and function pointers to be cross-assignable, but only
_because_ of dlsym().  After searching for variations of 'void *',
'void pointer', 'function pointer' and 'pointer to function', I
couldn't locate anything besides the dlsym() section that elaborates
on the requirement that a POSIX compiler must allow conversion of a
function pointer into void* and back.

> > 
> > Regardless, casting "free" to a pointer-to-object, just because
> > string_vector_iter() takes a (void(*)(char*)), and not a
> > (void(*)(void*)), is questionable style, IMO.
> > 
> > I've grepped the tree for this pattern:
> > 
> >   git grep -E '\(void ?\*\) ?free'
> > 
> > and there are eleven hits.
> > 
> > Furthermore, there are *no other* _vector_iter() calls -- and not just
> > string_vector_iter() calls, but in general, _vector_iter() ones! -- than
> > these eleven.
> > 
> > I think it's time we designed either a general freeing iterator API for
> > vector, or at least added a trivial (stop-gap) wrapper function like
> > this:
> > 
> > > diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
> > > index 80d7311debfb..5221c70e3f78 100644
> > > --- a/common/utils/string-vector.h
> > > +++ b/common/utils/string-vector.h
> > > @@ -39,4 +39,10 @@
> > >
> > >  DEFINE_VECTOR_TYPE(string_vector, char *);
> > >
> > > +static inline void
> > > +string_free (char *string)
> > > +{
> > > +  free (string);
> > > +}
> > > +
> > >  #endif /* STRING_VECTOR_H */
> > 
> > Comments please :)
> 
> Agreed.

I'm also in agreement that tweaking our vector interface for the ways
in which we actively use it (without having to spell out explicit
(void*) casts) would be welcome.

> 
> > (3) At the last hunk, the code suggests we're between fork() and exec().
> > Per POSIX
> > ,
> > there we can only call async-signal-safe functions:
> > 
> > > the child process may only execute async-signal-safe operations until
> > > such time as one of the exec functions is called
> > 
> > The list of async-signal-safe functions can be found at

Re: [Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.

[Eric Blake]

On Tue, Jan 31, 2023 at 11:52:27AM +, Richard W.M. Jones wrote:
> On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
> > On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones  
> > wrote:
> > >
> > > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > > > Document all options in --help output.  If -n is not in use, then
> > > > enhance the banner to print the current state of h, and further tailor
> > > > the advice given on useful next steps to take to mention opt_go when
> > > > using --opt-mode.
> > >
> > > I had to partially revert this patch (reverting most of it) because it
> > > unfortunately breaks the implicit handle creation :-(
> > >
> > > https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2
> > >
> > > I'm not actually sure how to do this correctly in Python.  I made
> > > several attempts, but I don't think Python is very good about having a
> > > variable which is only defined on some paths -- maybe it's not
> > > possible at all.
> > 
> > Can you share the error when it breaks?
> 
> $ rpm -qf /usr/bin/nbdsh
> python3-libnbd-1.15.9-2.fc38.x86_64
> 
> $ nbdsh 
> 
> Welcome to nbdsh, the shell for interacting with
> Network Block Device (NBD) servers.
> 
> The ‘nbd’ module has already been imported and there
> is an open NBD handle called ‘h’ in state 'START'.
> 
> h.connect_tcp("remote", "10809")   # Connect to a remote server.
> h.get_size()   # Get size of the remote disk.
> buf = h.pread(512, 0)  # Read the first sector.
> exit() or Ctrl-D   # Quit the shell
> help(nbd)  # Display documentation
> 
> nbd> print(h)
> Traceback (most recent call last):
>   File "/usr/lib64/python3.11/code.py", line 90, in runcode
> exec(code, self.locals)
>   File "", line 1, in 
> NameError: name 'h' is not defined

Eww. I didn't mean for that to happen, obviously.

> 
> > I'm not sure what is the issue, but usually if you have a global
> > variable created only in
> > some flows, adding:
> > 
> > thing = None
> > 
> > At the start of the module makes sure that the name exists later,
> > regardless of the flow
> > taken. Code can take the right action based on:
> > 
> > if thing is None:
> > ...
> 
> Good point, I was trying 'undef h' instead.  'h' not being present in
> the dictionary at all vs 'h = None' are slightly different, although I
> suppose it doesn't matter in this particular case.
> 
> The other point is that 'h' (when defined) is not a global.  The patch
> assumes it is a global, but then uses it in some places as if it is a
> local.

Do you want me to try to play with this as well (since I do like the
improved help message, if we can get it to work reliably), or would I
be duplicating efforts at this point?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v2 4/4] generator/states-connect-socket-activation.c: Set LISTEN_FDNAMES

[Eric Blake]

On Mon, Jan 30, 2023 at 10:55:21PM +, Richard W.M. Jones wrote:
> When the user calls nbd_set_socket_activation_name before calling
> nbd_connect_system_socket_activation, pass the name down to the server
> through LISTEN_FDNAMES.  This has no effect unless the new API has
> been called to set the socket name to a non-empty string.
> ---
>  generator/states-connect-socket-activation.c | 35 +++-
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 

> + *
> + * env[2] (if used) is "LISTEN_FDNAMES=" + h->sa_name

> @@ -61,11 +69,20 @@ prepare_socket_activation_environment (string_vector *env)
>if (p == NULL)
>  goto err;
>string_vector_append (env, p);
> +  if (using_name) {
> +if (asprintf (, "LISTEN_FDNAMES=%s", h->sa_name) == -1)
> +  goto err;
> +string_vector_append (env, p);
> +  }
>  
> -  /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
> +  /* Append the current environment, but remove the special
> +   * environment variables.
> +   */
>for (i = 0; environ[i] != NULL; ++i) {
>  if (strncmp (environ[i], "LISTEN_PID=", strlen ("LISTEN_PID=")) != 0 &&
> -strncmp (environ[i], "LISTEN_FDS=", strlen ("LISTEN_FDS=")) != 0) {
> +strncmp (environ[i], "LISTEN_FDS=", strlen ("LISTEN_FDS=")) != 0 &&
> +strncmp (environ[i], "LISTEN_FDNAMES=",
> + strlen ("LISTEN_FDNAMES=")) != 0) {

Hmm. Even if we _don't_ expose the ability to set LISTEN_FDNAMES to
users, we should probably be stripping it from the environment
(without replacement), rather than just stripping the other two
LISTEN_* variables.  Which might be worth doing in a separate patch,
in case it has to be backported across different versions of libnbd.

But overall, I agree with exposing the ability for the client to
programatically set the name they want, whether by this series or by
my idea of an alternative API that takes the socket name alongside the
argv; and whether we keep to our 32-byte [[:alnum:]] limit or instead
allow for a larger name up to 255 bytes in the regex range notation by
ASCII byte value [\x20-\x39\x41-\x7e] (aka [ -9;-~], or
[^\x00-\x1f\x7f-\xff]).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v2 3/4] generator: Add APIs to get/set the socket activation socket name

[Eric Blake]

On Mon, Jan 30, 2023 at 10:55:20PM +, Richard W.M. Jones wrote:
> To allow us to name the socket passed down to the NBD server when
> calling nbd_connect_systemd_socket_activation(3), we need to add the
> field to the handle and add access functions.
> ---
>  generator/API.ml | 49 ++
>  lib/handle.c | 56 
>  lib/internal.h   |  1 +
>  3 files changed, 106 insertions(+)
> 
> diff --git a/generator/API.ml b/generator/API.ml
> index 25a612a2e8..08fc80960b 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -2036,15 +2036,62 @@   "connect_systemd_socket_activation", {
>  
>  When the NBD handle is closed the server subprocess
>  is killed.
> +
> +=head3 Socket name
> +
> +The socket activation protocol lets you optionally give
> +the socket a name.  If used, the name is passed to the
> +NBD server using the C environment
> +variable.  To provide a socket name, call
> +L before calling
> +the connect function.

This creates an implicit client-side stateful API: to pass socket
names, you must call two APIs in the correct sequence:

nbd_set_socket_activation_name (h, "foo");
nbd_connect_systemd_socket_activation (h, argv);

I can live with that design, but I recall Laszlo questioning in the
past if we always need to force this stateful design onto clients, or
if it would be easier to instead add a alternative API that takes the
socket name as an additional parameter, in one shot:

nbd_connect_systemd_named_socket_activation (h, "foo", argv);

>  
> +int
> +nbd_unlocked_set_socket_activation_name (struct nbd_handle *h,
> + const char *name)
> +{
> +  size_t i, len;
> +  char *new_name;
> +
> +  len = strlen (name);
> +
> +  /* Setting it to empty string stores NULL in the handle. */
> +  if (len == 0) {
> +free (h->sa_name);
> +h->sa_name = NULL;
> +return 0;
> +  }
> +
> +  /* Check the proposed name is short and alphanumeric. */
> +  if (len > 32) {
> +set_error (ENAMETOOLONG, "socket activation name should be "
> +   "<= 32 characters");

I don't mind keeping us strict to start with and loosening it later if
someone demonstrates why it is needed.  But systemd documents a larger
set of possible names:

https://www.freedesktop.org/software/systemd/man/sd_pid_notify_with_fds.html

FDNAME=…

When used in combination with FDSTORE=1, specifies a name for the
submitted file descriptors. When used with FDSTOREREMOVE=1,
specifies the name for the file descriptors to remove. This name
is passed to the service during activation, and may be queried
using sd_listen_fds_with_names(3). File descriptors submitted
without this field set, will implicitly get the name "stored"
assigned. Note that, if multiple file descriptors are submitted at
once, the specified name will be assigned to all of them. In order
to assign different names to submitted file descriptors, submit
them in separate invocations of sd_pid_notify_with_fds(). The name
may consist of arbitrary ASCII characters except control
characters or ":". It may not be longer than 255 characters. If a
submitted name does not follow these restrictions, it is ignored.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH 1/2] retry: Add in retry support during .open

[Eric Blake]

On Sat, Jan 28, 2023 at 01:47:32PM +, Richard W.M. Jones wrote:
> On Fri, Jan 27, 2023 at 02:41:22PM -0600, Eric Blake wrote:
> > Now that a filter can open a backend as many times as it wants,
> > there's no longer a technical reason we can't retry .open.  However,
> > adding retry logic here does mean we have to weaken an assert in the
> > server backend code, since prepare can now be reached more than once.
> > 
> > Test coverage will be added in a separate patch, so that it becomes
> > easy to swap patch order and see that the test fails without this
> > patch.
> > 
> > See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
> > ---
> 
> Seems OK, ACK (modulo your comment about leaking memory
> in your reply).
> 
> I checked the documentation of the filter and it doesn't mention the
> limitation, so it seems we don't need to adjust the documentation at
> all.

The memory bug is fixed, and this is now in as commits
42b198c9..4aabef5e

> 
> I will try this out with the ssh filter once the bug moves into post.

Ready for you to test.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd] generator: Pass LISTEN_FDNAMES=nbd with systemd socket activation

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 01:49:53PM +0100, Laszlo Ersek wrote:
> On 1/28/23 13:47, Richard W.M. Jones wrote:
> > systemd allows sockets passed through socket activation to be named
> > with the protocol they require.  We only ever pass one socket, name
> > it.  This environment variable is currently ignored by qemu-nbd and
> > nbdkit, but might be used by qemu-storage-daemon:
> >
> > https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html
> > ---
> >  generator/states-connect-socket-activation.c | 41 +++-
> >  1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/generator/states-connect-socket-activation.c 
> > b/generator/states-connect-socket-activation.c
> > index 9a83834915..22f06d4fd3 100644
> > --- a/generator/states-connect-socket-activation.c
> > +++ b/generator/states-connect-socket-activation.c
> > @@ -34,16 +34,18 @@
> >  /* This is baked into the systemd socket activation API. */
> >  #define FIRST_SOCKET_ACTIVATION_FD 3
> >
> > -/* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */
> > -#define PREFIX_LENGTH 11
> > -
> >  extern char **environ;
> >
> >  /* Prepare environment for calling execvp when doing systemd socket
> >   * activation.  Takes the current environment and copies it.  Removes
> > - * any existing LISTEN_PID or LISTEN_FDS and replaces them with new
> > - * variables.  env[0] is "LISTEN_PID=..." which is filled in by
> > - * CONNECT_SA.START, and env[1] is "LISTEN_FDS=1".
> > + * any existing LISTEN_PID, LISTEN_FDS or LISTEN_FDNAMES, and replaces
> > + * them with new variables.
> > + *
> > + * env[0] is "LISTEN_PID=..." which is filled in by CONNECT_SA.START
> > + *
> > + * env[1] is "LISTEN_FDS=1"
> > + *
> > + * env[2] is "LISTEN_FDNAMES=nbd"
> >   */
> >  static int
> >  prepare_socket_activation_environment (string_vector *env)
> > @@ -53,26 +55,29 @@ prepare_socket_activation_environment (string_vector 
> > *env)
> >
> >assert (env->len == 0);
> >
> > -  /* Reserve slots env[0] and env[1]. */
> > +  /* Reserve slots env[0]..env[2] */
> > +  if (string_vector_reserve (env, 3) == -1)
> > +goto err;
> >p = strdup ("LISTEN_PID=");
> >if (p == NULL)
> >  goto err;
> > -  if (string_vector_append (env, p) == -1) {
> > -free (p);
> > -goto err;
> > -  }
> > +  string_vector_append (env, p);
> >p = strdup ("LISTEN_FDS=1");
> >if (p == NULL)
> >  goto err;
> > -  if (string_vector_append (env, p) == -1) {
> > -free (p);
> > +  string_vector_append (env, p);
> > +  p = strdup ("LISTEN_FDNAMES=nbd");
> > +  if (p == NULL)
> >  goto err;
> > -  }
> > +  string_vector_append (env, p);
> >
> > -  /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
> > +  /* Append the current environment, but remove the special
> > +   * environment variables.
> > +   */
> >for (i = 0; environ[i] != NULL; ++i) {
> > -if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0 &&
> > -strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) {
> > +if (strncmp (environ[i], "LISTEN_PID=", 11) != 0 &&
> > +strncmp (environ[i], "LISTEN_FDS=", 11) != 0 &&
> > +strncmp (environ[i], "LISTEN_FDNAMES=", 15) != 0) {
> >char *copy = strdup (environ[i]);
> >if (copy == NULL)
> >  goto err;
> > @@ -194,7 +199,7 @@  CONNECT_SA.START:
> >  char buf[32];
> >  const char *v =
> >nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf);
> > -strcpy ([0][PREFIX_LENGTH], v);
> > +strcpy ([0][strlen ("LISTEN_FDS=")], v);
> >
> >  /* Restore SIGPIPE back to SIG_DFL. */
> >  signal (SIGPIPE, SIG_DFL);
> 
> I really didn't want to obsess about this -- I spent like 10+ minutes on
> curbing my enthusiasm! :) --, but I believe there's a semantic bug in
> the patch, one that's directly related to my "hidden" thoughts.
> 
> (1) In the last hunk, strlen() is applied to "LISTEN_FDS=". However, the
> zero-index element of the env array holds
> "LISTEN_PID=". In other words, the patch
> only gets lucky because "PID" and "FDS" are both three characters long.

Yup that is totally wrong :-(

> Relatedly, my hidden thought was that we shouldn't use so many naked
> string literals all over the code.
> 
> May I take a stab at rewriting this? I feel that's the easiest way for
> me to express what I'd propose. Basically I'd propose:
> 
> - an enum for listing the "keys" we need,
> 
> - a static array of structure elements, for expressing the environment
>   variables (name=value), *and* the prefix lengths,
> 
> - a macro for populating an element of the array -- use "sizeof" for
>   grabbing the prefix length

Sure, go for it please.

> (2) Pre-patch, at commit 5a02c7d2cc6a, the error handling tail of
> prepare_socket_activation_environment() is less than ideal, IMO. Namely,
> we have (excerpt)
> 
> >  err:
> >   set_error (errno, "malloc");
> >   

Re: [Libguestfs] [p2v PATCH 11/11] make-kickstart: add URLs for RHEL-9

[Laszlo Ersek]

On 1/30/23 19:16, Richard W.M. Jones wrote:
> 
> I had a few reservations in a couple of comments, but basically the
> series is fine and therefore:
> 
> Reviewed-by: Richard W.M. Jones 
> 
> Please push it whenever you want.

I'll wait for your new comments first. I think a v2 is worthwhile:
- extend the POD manual (basically, repeat the tooltip)
- emphasize "no spaces around commas" in the tooltip.

Thanks!
Laszlo

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

Re: [Libguestfs] [p2v PATCH 09/11] gui: expose "p2v.output.misc" (-oo)

[Laszlo Ersek]

On 1/30/23 19:15, Richard W.M. Jones wrote:
> On Mon, Jan 30, 2023 at 03:22:26PM +0100, Laszlo Ersek wrote:
>> +  str = gtk_entry_get_text (GTK_ENTRY (oo_entry));
>> +  guestfs_int_free_string_list (config->output.misc);
>> +  config->output.misc = guestfs_int_split_string (',', str);
> 
> My concern here is that someone is going to put "foo=bar, baz=1" in
> this field, and it will improperly split above.  So maybe a bit of
> trimming on the resulting array?

Not a bad idea at all, but it creates inconsistency with the kernel
cmdline interface. No trimming happens there, and I figured it best to
keep the format on the GUI and the kernel cmdline interchangeable.

(More precisely, I figured that the GUI should be consistent with the
*pre-existent* ConfigStringList items of the kernel cmdline too, such as
disks, removables, interfaces, network-map, and so on.)

I can update the tooltip to spell out "no spaces" or something.

> 
>> +  if (config->output.misc == NULL)
>> +error (EXIT_FAILURE, errno, "strdup");
>> +
>>/* Display the UI for conversion. */
>>show_running_dialog ();
>>  
>> diff --git a/virt-p2v.pod b/virt-p2v.pod
>> index ecdae46eaaf6..a7e8edcbf9f0 100644
>> --- a/virt-p2v.pod
>> +++ b/virt-p2v.pod
>> @@ -175,6 +175,8 @@ first-time virt-p2v user.
>>   │
>>   │ Output allocation (-oa): [sparse▼]
>>   │
>> + │ Misc. options (-oo): [___]
>> + │
>>  
>>  All output options and paths are relative to the conversion server
>>  (I to the physical server).
> 
> Be kind of nice to add a tiny bit more documentation here, especially
> as there's only a tool tip telling you what the field should contain.

I agree; I think I can just copy the tooltip (effectively) here.

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

Re: [Libguestfs] [PATCH libnbd] generator: Pass LISTEN_FDNAMES=nbd with systemd socket activation

[Laszlo Ersek]

On 1/28/23 13:47, Richard W.M. Jones wrote:
> systemd allows sockets passed through socket activation to be named
> with the protocol they require.  We only ever pass one socket, name
> it.  This environment variable is currently ignored by qemu-nbd and
> nbdkit, but might be used by qemu-storage-daemon:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html
> ---
>  generator/states-connect-socket-activation.c | 41 +++-
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/generator/states-connect-socket-activation.c 
> b/generator/states-connect-socket-activation.c
> index 9a83834915..22f06d4fd3 100644
> --- a/generator/states-connect-socket-activation.c
> +++ b/generator/states-connect-socket-activation.c
> @@ -34,16 +34,18 @@
>  /* This is baked into the systemd socket activation API. */
>  #define FIRST_SOCKET_ACTIVATION_FD 3
>
> -/* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */
> -#define PREFIX_LENGTH 11
> -
>  extern char **environ;
>
>  /* Prepare environment for calling execvp when doing systemd socket
>   * activation.  Takes the current environment and copies it.  Removes
> - * any existing LISTEN_PID or LISTEN_FDS and replaces them with new
> - * variables.  env[0] is "LISTEN_PID=..." which is filled in by
> - * CONNECT_SA.START, and env[1] is "LISTEN_FDS=1".
> + * any existing LISTEN_PID, LISTEN_FDS or LISTEN_FDNAMES, and replaces
> + * them with new variables.
> + *
> + * env[0] is "LISTEN_PID=..." which is filled in by CONNECT_SA.START
> + *
> + * env[1] is "LISTEN_FDS=1"
> + *
> + * env[2] is "LISTEN_FDNAMES=nbd"
>   */
>  static int
>  prepare_socket_activation_environment (string_vector *env)
> @@ -53,26 +55,29 @@ prepare_socket_activation_environment (string_vector *env)
>
>assert (env->len == 0);
>
> -  /* Reserve slots env[0] and env[1]. */
> +  /* Reserve slots env[0]..env[2] */
> +  if (string_vector_reserve (env, 3) == -1)
> +goto err;
>p = strdup ("LISTEN_PID=");
>if (p == NULL)
>  goto err;
> -  if (string_vector_append (env, p) == -1) {
> -free (p);
> -goto err;
> -  }
> +  string_vector_append (env, p);
>p = strdup ("LISTEN_FDS=1");
>if (p == NULL)
>  goto err;
> -  if (string_vector_append (env, p) == -1) {
> -free (p);
> +  string_vector_append (env, p);
> +  p = strdup ("LISTEN_FDNAMES=nbd");
> +  if (p == NULL)
>  goto err;
> -  }
> +  string_vector_append (env, p);
>
> -  /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
> +  /* Append the current environment, but remove the special
> +   * environment variables.
> +   */
>for (i = 0; environ[i] != NULL; ++i) {
> -if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0 &&
> -strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) {
> +if (strncmp (environ[i], "LISTEN_PID=", 11) != 0 &&
> +strncmp (environ[i], "LISTEN_FDS=", 11) != 0 &&
> +strncmp (environ[i], "LISTEN_FDNAMES=", 15) != 0) {
>char *copy = strdup (environ[i]);
>if (copy == NULL)
>  goto err;
> @@ -194,7 +199,7 @@  CONNECT_SA.START:
>  char buf[32];
>  const char *v =
>nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf);
> -strcpy ([0][PREFIX_LENGTH], v);
> +strcpy ([0][strlen ("LISTEN_FDS=")], v);
>
>  /* Restore SIGPIPE back to SIG_DFL. */
>  signal (SIGPIPE, SIG_DFL);

I really didn't want to obsess about this -- I spent like 10+ minutes on
curbing my enthusiasm! :) --, but I believe there's a semantic bug in
the patch, one that's directly related to my "hidden" thoughts.

(1) In the last hunk, strlen() is applied to "LISTEN_FDS=". However, the
zero-index element of the env array holds
"LISTEN_PID=". In other words, the patch
only gets lucky because "PID" and "FDS" are both three characters long.

Relatedly, my hidden thought was that we shouldn't use so many naked
string literals all over the code.

May I take a stab at rewriting this? I feel that's the easiest way for
me to express what I'd propose. Basically I'd propose:

- an enum for listing the "keys" we need,

- a static array of structure elements, for expressing the environment
  variables (name=value), *and* the prefix lengths,

- a macro for populating an element of the array -- use "sizeof" for
  grabbing the prefix length


(2) Pre-patch, at commit 5a02c7d2cc6a, the error handling tail of
prepare_socket_activation_environment() is less than ideal, IMO. Namely,
we have (excerpt)

>  err:
>   set_error (errno, "malloc");
>   string_vector_iter (env, (void *) free);
>   free (env->ptr);
>   return -1;

(2a) we free the vector's pointer field, but don't NULL it, nor do we
zero the len or cap fields.

We should call string_vector_reset() instead.

(2b) Casting the address of the free() function to (void*) makes me
uncomfortable. It is undefined behavior by ISO C.

Now, I seem to remember that POSIX 

Re: [Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.

[Richard W.M. Jones]

On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
> On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones  wrote:
> >
> > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > > Document all options in --help output.  If -n is not in use, then
> > > enhance the banner to print the current state of h, and further tailor
> > > the advice given on useful next steps to take to mention opt_go when
> > > using --opt-mode.
> >
> > I had to partially revert this patch (reverting most of it) because it
> > unfortunately breaks the implicit handle creation :-(
> >
> > https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2
> >
> > I'm not actually sure how to do this correctly in Python.  I made
> > several attempts, but I don't think Python is very good about having a
> > variable which is only defined on some paths -- maybe it's not
> > possible at all.
> 
> Can you share the error when it breaks?

$ rpm -qf /usr/bin/nbdsh
python3-libnbd-1.15.9-2.fc38.x86_64

$ nbdsh 

Welcome to nbdsh, the shell for interacting with
Network Block Device (NBD) servers.

The ‘nbd’ module has already been imported and there
is an open NBD handle called ‘h’ in state 'START'.

h.connect_tcp("remote", "10809")   # Connect to a remote server.
h.get_size()   # Get size of the remote disk.
buf = h.pread(512, 0)  # Read the first sector.
exit() or Ctrl-D   # Quit the shell
help(nbd)  # Display documentation

nbd> print(h)
Traceback (most recent call last):
  File "/usr/lib64/python3.11/code.py", line 90, in runcode
exec(code, self.locals)
  File "", line 1, in 
NameError: name 'h' is not defined

> I'm not sure what is the issue, but usually if you have a global
> variable created only in
> some flows, adding:
> 
> thing = None
> 
> At the start of the module makes sure that the name exists later,
> regardless of the flow
> taken. Code can take the right action based on:
> 
> if thing is None:
> ...

Good point, I was trying 'undef h' instead.  'h' not being present in
the dictionary at all vs 'h = None' are slightly different, although I
suppose it doesn't matter in this particular case.

The other point is that 'h' (when defined) is not a global.  The patch
assumes it is a global, but then uses it in some places as if it is a
local.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.

[Nir Soffer]

On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones  wrote:
>
> On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > Document all options in --help output.  If -n is not in use, then
> > enhance the banner to print the current state of h, and further tailor
> > the advice given on useful next steps to take to mention opt_go when
> > using --opt-mode.
>
> I had to partially revert this patch (reverting most of it) because it
> unfortunately breaks the implicit handle creation :-(
>
> https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2
>
> I'm not actually sure how to do this correctly in Python.  I made
> several attempts, but I don't think Python is very good about having a
> variable which is only defined on some paths -- maybe it's not
> possible at all.

Can you share the error when it breaks?

I'm not sure what is the issue, but usually if you have a global
variable created only in
some flows, adding:

thing = None

At the start of the module makes sure that the name exists later,
regardless of the flow
taken. Code can take the right action based on:

if thing is None:
...

Nir

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