Re: [Libguestfs] [PATCH libguestfs 2/2] lib: Return correct osinfo field for Windows 11

[Richard W.M. Jones]

On Thu, Dec 01, 2022 at 10:34:09AM +, Richard W.M. Jones wrote:
> For Windows Client, we can only distinguish between Windows 10 and
> Windows 11 using the build ID.  The product name in both cases is
> "Windows 10 ", apparently intentionally.
> 
> References:
> https://learn.microsoft.com/en-us/answers/questions/586619/windows-11-build-ver-is-still-10022000194.html
> https://github.com/cygwin/cygwin/blob/a263fe0b268580273c1adc4b1bad256147990222/winsup/cygwin/wincap.cc#L429
> https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions
> 
> After this fix, the output of virt-inspector changes to this, which is
> a bit odd, but correct:
> 
> windows
> x86_64
> windows
> Windows 10 Pro
> Client
> 10
> 0
> /Windows
> ControlSet001
> win11
> 
> Thanks: Yaakov Selkowitz
> Reported-by: Yongkui Guo
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2012658
> ---
>  lib/inspect-osinfo.c | 25 +++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/inspect-osinfo.c b/lib/inspect-osinfo.c
> index 90e57e6dfa..9a22a9d037 100644
> --- a/lib/inspect-osinfo.c
> +++ b/lib/inspect-osinfo.c
> @@ -86,6 +86,8 @@ guestfs_impl_inspect_get_osinfo (guestfs_h *g, const char 
> *root)
>else if (STREQ (type, "windows")) {
>  CLEANUP_FREE char *product_name = NULL;
>  CLEANUP_FREE char *product_variant = NULL;
> +CLEANUP_FREE char *build_id_str = NULL;
> +int build_id;
>  
>  product_name = guestfs_inspect_get_product_name (g, root);
>  if (!product_name)
> @@ -142,8 +144,27 @@ guestfs_impl_inspect_get_osinfo (guestfs_h *g, const 
> char *root)
>  return safe_strdup (g, "win2k19");
>else
>  return safe_strdup (g, "win2k16");
> -} else
> -  return safe_strdup (g, "win10");
> +}
> +else {
> +  /* For Windows >= 10 Client we can only distinguish between
> +   * versions by looking at the build ID.  See:
> +   * 
> https://learn.microsoft.com/en-us/answers/questions/586619/windows-11-build-ver-is-still-10022000194.html
> +   * 
> https://github.com/cygwin/cygwin/blob/a263fe0b268580273c1adc4b1bad256147990222/winsup/cygwin/wincap.cc#L429
> +   */
> +  build_id_str = guestfs_inspect_get_build_id (g, root);
> +  if (!build_id_str)
> +return NULL;
> +
> +  if (sscanf (build_id_str, "%d", _id) != 1) {
> +error (g, "cannot parse Windows build ID from this guest");
> +return NULL;
> +  }
> +
> +  if (build_id >= 2200)

This should of course be 22000!  Updated my copy.

Rich.

> +return safe_strdup (g, "win11");
> +  else
> +return safe_strdup (g, "win10");
> +}
>}
>break;
>  }
> -- 
> 2.37.3
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

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

[Libguestfs] [PATCH v2v] rhv: Use osinfo to distinguish Windows >= 10 variants

[Richard W.M. Jones]

Windows versions >= 10 no longer use the NT major.minor numbering
scheme (it is fixed at 10.0).  Libguestfs >= 1.49.8 can distinguish
these versions and it sets  correctly, so use that instead.

After this change the OVF will contain:

  Windows 10 Pro
  windows_11

which is strange, but apparently what Microsoft intended.  As far as
RHV is concern it only needs the  to be correct in order
to choose the correct devices etc.

Reported-by: Tingting Zheng
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2149863
---
 lib/create_ovf.ml | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml
index 18e86d6cf1..8aff3d8f0b 100644
--- a/lib/create_ovf.ml
+++ b/lib/create_ovf.ml
@@ -231,13 +231,22 @@ and get_ostype = function
   i_arch = "i386" } ->
 "windows_10"
 
+  (* For Windows NT 10.0 always use the  field since the
+   * other fields will not accurately reflect the version.
+   *)
   | { i_type = "windows"; i_major_version = 10; i_minor_version = 0;
-  i_arch = "x86_64"; i_product_variant = "Client" } ->
-"windows_10x64"
-
-  | { i_type = "windows"; i_major_version = 10; i_minor_version = 0;
-  i_arch = "x86_64" } ->
-"windows_2016x64"
+  i_arch = "x86_64"; i_osinfo = osinfo; i_product_name = product } ->
+ (match osinfo with
+  | "win10" -> "windows_10x64"
+  | "win11" -> "windows_11"
+  | "win2k16" -> "windows_2016x64"
+  | "win2k19" -> "windows_2019x64"
+  | "win2k22" -> "windows_2022"
+  | _ ->
+ warning (f_"unknown Windows 10 variant: %s (%s)")
+   osinfo product;
+ "windows_2022"
+ )
 
   | { i_type = typ; i_distro = distro;
   i_major_version = major; i_minor_version = minor; i_arch = arch;
-- 
2.37.3

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

[Libguestfs] [PATCH guestfs-tools] inspector: Display the new build ID field

[Richard W.M. Jones]

libguestfs 1.49.8 adds a new API to read the build ID from guests
(especially for Windows).  If the new API is available and if it
returns a string other than "unknown", then print it in virt-inspector
output.
---
 inspector/inspector.c| 8 
 inspector/virt-inspector.rng | 1 +
 2 files changed, 9 insertions(+)

diff --git a/inspector/inspector.c b/inspector/inspector.c
index 2702e3310c..e4b031149b 100644
--- a/inspector/inspector.c
+++ b/inspector/inspector.c
@@ -447,6 +447,14 @@ output_root (xmlTextWriterPtr xo, char *root)
   single_element ("hostname", str);
 free (str);
 
+#ifdef GUESTFS_HAVE_INSPECT_GET_BUILD_ID
+str = guestfs_inspect_get_build_id (g, root);
+if (!str) exit (EXIT_FAILURE);
+if (STRNEQ (str, "unknown"))
+  single_element ("build_id", str);
+free (str);
+#endif
+
 str = guestfs_inspect_get_osinfo (g, root);
 if (!str) exit (EXIT_FAILURE);
 if (STRNEQ (str, "unknown"))
diff --git a/inspector/virt-inspector.rng b/inspector/virt-inspector.rng
index 5b460b3645..29c5798e19 100644
--- a/inspector/virt-inspector.rng
+++ b/inspector/virt-inspector.rng
@@ -38,6 +38,7 @@
 
 
 
+
 
 
 
-- 
2.37.3

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

[Libguestfs] [PATCH libguestfs 2/2] lib: Return correct osinfo field for Windows 11

[Richard W.M. Jones]

For Windows Client, we can only distinguish between Windows 10 and
Windows 11 using the build ID.  The product name in both cases is
"Windows 10 ", apparently intentionally.

References:
https://learn.microsoft.com/en-us/answers/questions/586619/windows-11-build-ver-is-still-10022000194.html
https://github.com/cygwin/cygwin/blob/a263fe0b268580273c1adc4b1bad256147990222/winsup/cygwin/wincap.cc#L429
https://en.wikipedia.org/wiki/List_of_Microsoft_Windows_versions

After this fix, the output of virt-inspector changes to this, which is
a bit odd, but correct:

windows
x86_64
windows
Windows 10 Pro
Client
10
0
/Windows
ControlSet001
win11

Thanks: Yaakov Selkowitz
Reported-by: Yongkui Guo
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2012658
---
 lib/inspect-osinfo.c | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/lib/inspect-osinfo.c b/lib/inspect-osinfo.c
index 90e57e6dfa..9a22a9d037 100644
--- a/lib/inspect-osinfo.c
+++ b/lib/inspect-osinfo.c
@@ -86,6 +86,8 @@ guestfs_impl_inspect_get_osinfo (guestfs_h *g, const char 
*root)
   else if (STREQ (type, "windows")) {
 CLEANUP_FREE char *product_name = NULL;
 CLEANUP_FREE char *product_variant = NULL;
+CLEANUP_FREE char *build_id_str = NULL;
+int build_id;
 
 product_name = guestfs_inspect_get_product_name (g, root);
 if (!product_name)
@@ -142,8 +144,27 @@ guestfs_impl_inspect_get_osinfo (guestfs_h *g, const char 
*root)
 return safe_strdup (g, "win2k19");
   else
 return safe_strdup (g, "win2k16");
-} else
-  return safe_strdup (g, "win10");
+}
+else {
+  /* For Windows >= 10 Client we can only distinguish between
+   * versions by looking at the build ID.  See:
+   * 
https://learn.microsoft.com/en-us/answers/questions/586619/windows-11-build-ver-is-still-10022000194.html
+   * 
https://github.com/cygwin/cygwin/blob/a263fe0b268580273c1adc4b1bad256147990222/winsup/cygwin/wincap.cc#L429
+   */
+  build_id_str = guestfs_inspect_get_build_id (g, root);
+  if (!build_id_str)
+return NULL;
+
+  if (sscanf (build_id_str, "%d", _id) != 1) {
+error (g, "cannot parse Windows build ID from this guest");
+return NULL;
+  }
+
+  if (build_id >= 2200)
+return safe_strdup (g, "win11");
+  else
+return safe_strdup (g, "win10");
+}
   }
   break;
 }
-- 
2.37.3

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

[Libguestfs] [PATCH libguestfs 1/2] New API: inspect_get_build_id

[Richard W.M. Jones]

Add an API to return the build ID of the guest.  This to allow a
future change to be able to distinguish between Windows 10 and Windows 11
which can only be done using the build ID.

For Windows we can read the CurrentBuildNumber key from the registry.
For Linux there happens to be a BUILD_ID field in /etc/os-release.
I've never seen a Linux distro that actually uses this.
---
 daemon/inspect.ml   |  6 ++
 daemon/inspect_fs_unix.ml   |  2 ++
 daemon/inspect_fs_windows.ml| 14 ++
 daemon/inspect_types.ml |  5 +
 daemon/inspect_types.mli|  1 +
 generator/actions_inspection.ml | 19 +++
 generator/proc_nr.ml|  3 ++-
 lib/MAX_PROC_NR |  2 +-
 8 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/daemon/inspect.ml b/daemon/inspect.ml
index fb75b4a6cb..20217c0257 100644
--- a/daemon/inspect.ml
+++ b/daemon/inspect.ml
@@ -335,6 +335,12 @@ and inspect_get_hostname root =
   | Some v -> v
   | None -> "unknown"
 
+and inspect_get_build_id root =
+  let root = search_for_root root in
+  match root.inspection_data.build_id with
+  | Some v -> v
+  | None -> "unknown"
+
 and inspect_get_windows_systemroot root =
   let root = search_for_root root in
   match root.inspection_data.windows_systemroot with
diff --git a/daemon/inspect_fs_unix.ml b/daemon/inspect_fs_unix.ml
index 8045ef0d43..811e23b18c 100644
--- a/daemon/inspect_fs_unix.ml
+++ b/daemon/inspect_fs_unix.ml
@@ -96,6 +96,8 @@ let rec parse_os_release release_file data =
data.product_name <- Some value
  else if key = "VERSION_ID" then
parse_os_release_version_id value data
+ else if key = "BUILD_ID" then
+   data.build_id <- Some value
) values;
 
  (* If we haven't got all the fields, exit right away. *)
diff --git a/daemon/inspect_fs_windows.ml b/daemon/inspect_fs_windows.ml
index c4a05bc383..7bc5de7f77 100644
--- a/daemon/inspect_fs_windows.ml
+++ b/daemon/inspect_fs_windows.ml
@@ -263,6 +263,20 @@ and check_windows_software_registry software_hive data =
  with
Not_found -> ()
 );
+
+(* CurrentBuildNumber (build_id).
+ *
+ * In modern Windows, the "CurrentBuild" and "CurrentBuildNumber"
+ * keys are the same.  But in Windows XP, "CurrentBuild"
+ * contained something quite different.  So always use
+ * "CurrentBuildNumber".
+ *)
+(try
+   let v = List.assoc "CurrentBuildNumber" values in
+   data.build_id <- Some (Hivex.value_string h v)
+ with
+   Not_found -> ()
+);
   with
   | Not_found ->
  if verbose () then
diff --git a/daemon/inspect_types.ml b/daemon/inspect_types.ml
index 9395c51f95..328a2146be 100644
--- a/daemon/inspect_types.ml
+++ b/daemon/inspect_types.ml
@@ -48,6 +48,7 @@ and inspection_data = {
   mutable version : version option;
   mutable arch : string option;
   mutable hostname : string option;
+  mutable build_id : string option;
   mutable fstab : fstab_entry list;
   mutable windows_systemroot : string option;
   mutable windows_software_hive : string option;
@@ -167,6 +168,8 @@ and string_of_inspection_data data =
  data.arch;
   Option.may (fun v -> bpf "hostname: %s\n" v)
  data.hostname;
+  Option.may (fun v -> bpf "build ID: %s\n" v)
+ data.build_id;
   if data.fstab <> [] then (
 let v = List.map (
   fun (a, b) -> sprintf "(%s, %s)" (Mountable.to_string a) b
@@ -272,6 +275,7 @@ let null_inspection_data = {
   version = None;
   arch = None;
   hostname = None;
+  build_id = None;
   fstab = [];
   windows_systemroot = None;
   windows_software_hive = None;
@@ -294,6 +298,7 @@ let merge_inspection_data child parent =
   parent.version <- merge child.version parent.version;
   parent.arch <-merge child.arch parent.arch;
   parent.hostname <-merge child.hostname parent.hostname;
+  parent.build_id <-merge child.build_id parent.build_id;
   parent.fstab <-   child.fstab @ parent.fstab;
   parent.windows_systemroot <-
 merge child.windows_systemroot parent.windows_systemroot;
diff --git a/daemon/inspect_types.mli b/daemon/inspect_types.mli
index 29c76e8abb..05a3ffd4e4 100644
--- a/daemon/inspect_types.mli
+++ b/daemon/inspect_types.mli
@@ -51,6 +51,7 @@ and inspection_data = {
   mutable version : version option;
   mutable arch : string option;
   mutable hostname : string option;
+  mutable build_id : string option;
   mutable fstab : fstab_entry list;
   mutable windows_systemroot : string option;
   mutable windows_software_hive : string option;
diff --git a/generator/actions_inspection.ml b/generator/actions_inspection.ml
index f8b7449938..70de22ec0a 100644
--- a/generator/actions_inspection.ml
+++ b/generator/actions_inspection.ml
@@ -529,6 +529,25 @@ hive is a valid Windows Registry hive.
 
 You can use C to read 

[Libguestfs] [PATCH libguestfs 0/2] lib: Return correct osinfo field for Windows 11

[Richard W.M. Jones]

https://bugzilla.redhat.com/show_bug.cgi?id=2012658

We don't currently detect the correct version for Windows 11 (client,
server is OK).  After some discussion it turns out the accepted way to
do this is to look at the Windows build ID.

So add some code in the daemon to grab the build ID from the
Windows registry and expose it through a new API 
("guestfs_inspect_get_build_id")
and adjust the osinfo code on the library side accordingly.

Because, at least in theory, Linux distros could have build IDs, I
didn't call the API "guestfs_inspect_windows_...".  There is a
possible build ID field in /etc/os-release, although it seems to be
unloved.

This fixes the  field.  I intentionally did not adjust either
the Windows version (still 10.0) nor the product name (which is
completely bogus, but apparently how Microsoft intended it to be).

Rich.


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

Re: [Libguestfs] [PATCH v2v] New virt-v2v-inspector tool

[Richard W.M. Jones]

Thanks for the review.  That's upstream now in
commit 0805ea93796b8b57e7c9f0bc04f83ea76a9820a5.

I fixed up the commit message and added a non-mandatory -O option to
control output.

We can keep it experimental status for now and decide before
virt-v2v 2.2 (next stable) if we want to keep it or modify it.

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 v2v] New virt-v2v-inspector tool

[Richard W.M. Jones]

On Thu, Nov 24, 2022 at 10:54:42AM +0100, Laszlo Ersek wrote:
> On 11/22/22 16:47, Richard W.M. Jones wrote:
> > In Kubernetes and tools like Kubevirt, it's not possible to create
> > some disks and then attach to them (in order to populate them with
> > data) in one step.  This makes virt-v2v conversions awkward because
> > ideally we would like the output mode (-o kubevirt) to both create the
> > target disks and populate them at the same time.
> > 
> > So to work around this problem, we need a tool which can inspect the
> > virt-v2v source hypervisor before we do the conversion in order to
> > find out how many disks are needed and their sizes.  Then we can
> > create the target disks, and then we can run a second container with
> > virt-v2v attached to the disks to do the conversion and populate the
> > output.
> 
> So, is the population of the *pre-created* disk images (aka volumes) a
> feature of the new kubevirt output module?

When I get around to finishing it, yes.  Basically they will be
presented to virt-v2v in the container as /dev devices (PVC
"volumeMode: Block").  When running virt-v2v you have to give the
names of the devices to virt-v2v, since that is only known by whoever
creates the virt-v2v container.

Note they are pre-created but not pre-populated, ie. virt-v2v sees
correctly sized but empty /dev devices initially.

I should also note that raw block device PVCs are very sparsely
documented.  I wonder if they are not widely used?  In particular
every worked example I've found copies from the same example in the
Kubernetes upstream documentation.  Guess I'll find out if it works
soon ...

> > This is a proposed tool to do this.  It essentially uses the same -i*
> > options as virt-v2v (and no -o* options) and outputs various useful
> > metadata.  Example:
> > 
> >   $ ./run virt-v2v-inspector --quiet -i disk /var/tmp/fedora-32.img 
> >   virt-v2v-inspector: The QEMU Guest Agent will be installed for this 
> > guest> > at first boot.
> >   virt-v2v-inspector: warning: /files/boot/grub2/device.map/hd0 references 
> > unknown device "vda".  You may have to fix this entry manually after 
> > conversion.
> >   
> >   
> > 
> > virt-v2v-inspector
> > virt-v2v
> > 2.1.9
> > 
> >   
> > 6442450944
> > 1400897536
> >   
> > 
> > 
> >   linux
> >   fedora
> >   fedora32
> >   x86_64
> >   32
> >   0
> >   rpm
> >   dnf
> >   Fedora 32 (Thirty Two)
> > 
> >   
> > 
> > There should be sufficient information in the  section to
> > allocate target disks, plus additional information is printed which
> > might be useful.
> > 
> > Note that we do a full conversion in order to generate this
> > information.  In particular it's not possible to generate the
> >  estimate without this.  It's plausible we could have a
> > --no-convert option, but I'm not sure it's worthwhile: it would only
> > save a little time, but would make everything less accurate, plus
> > maybe it is a good idea to find out if conversion is going to work
> > before we create the target disks?
> 
> I think this is a great approach. The current problem from the
> "recipient" (kubevirt) side is that temporary storage for the disk
> images "in the middle" is really not wanted. This approach prevents just
> that. All other logic from virt-v2v remains useful and should be kept IMO.
> 
> > I chose XML instead of JSON for output.  XML allows us to annotate
> > elements with attributes like "estimated='true'".  It also lets us
> > represent 64 bit number accurately, where JSON cannot represent such
> > numbers.
> > 
> > One obvious problem is that (without --quiet) the program mixes up
> > informational output with the final document, which is a bit of a
> > pain.  Ideas here?
> 
> I think *small* regular files (fitting under $TMPDIR for example) should
> be in order, as output. Add an "-o desc.xml" or "-O desc.xml" option to
> the new tool, for saving the XML in "desc.xml" at once? (I hope I
> understood the "mixing" issue correctly.)

Yeah, I was just hoping to avoid having to specify an output file, as
outputing to stdout is quite nice.  But perhaps it's unavoidable.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Failing guestfish command in

[Richard W.M. Jones]

On Tue, Nov 22, 2022 at 07:19:31AM +, Addison Gourluck wrote:
> Hey,
> 
> I'm trying to use guestfish to run a few lvm-related commands on an image, but
> I'm getting errors. The error message suggested that I should send the error
> output to this mailing list.
> 
> The simplest version of the command I'm running is as follows:
> 
> ```
> PSEUDO_DISABLED=1 guestfish -v --format=raw -a MY_IMAGE << EOF
> run
> pvcreate /dev/sda
> EOF
> ```
> 
> and the error that I'm getting is quite long, but it seems to start failing
> about here:
> 
> ```
> supermin: deleting initramfs files
> supermin: chroot
> Starting /init script ...
> mount: only root can use "--types" option (effective UID is 65534)
> /init: line 38: /proc/cmdline: No such file or directory
> mount: only root can use "--types" option (effective UID is 65534)
> mount: only root can use "--options" option (effective UID is 65534)
> dd: failed to open '/dev/urandom': No such file or directory
> Failed to redirect standard streams to /dev/null: No such file or directory
> ```
> 
> The errors at this point are quite obviously due to the mount commands somehow
> failing, and most errors are complaining about /dev/ not existing, and other
> fatal failures.
> 
> This is especially unusual, because running this command normally on Ubuntu
> 20.04 works fine, but when it runs as part of my build process, it fails. The
> build tool that I'm using is Yocto's bitbake. I don't think it even begins to
> try to run the LVM commands, as it seems to fail before it even gets there.
> 
> Interestingly, if I clean out the files at /tmp/.guestfs-1000/, then run the
> test tool like this "TMPDIR=/tmp libguestfs-test-tool", then also run my build
> with "TMPDIR=/tmp" set, it will succeed normally. The files in /tmp are
> identical in every way (I shasumed them), except for two: "/tmp/.guestfs-1000/
> appliance.d/initrd" and "/tmp/.guestfs-1000/appliance.d/root". Their
> permissions and user/group are also identical.
> 
> The versions of relevant packages are as follows:
> 
> ```
> supermin 5.1.20
> guestfish 1.40.2

Is it the Debian libguestfs package, or self-built?

Please run libguestfs-test-tool and attach the complete, unedited output.

Rich.

> QEMU emulator version 4.2.1 (Debian 1:4.2-3ubuntu6.23)
> Linux Kernel 5.15.0-53-generic
> ```
> 
> I'm happy to provide more information, or full debug logs, if someone can 
> offer
> instructions on how best to share them.
> 
> Regards,

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

[Libguestfs] [PATCH v2v] New virt-v2v-inspector tool

[Richard W.M. Jones]

In Kubernetes and tools like Kubevirt, it's not possible to create
some disks and then attach to them (in order to populate them with
data) in one step.  This makes virt-v2v conversions awkward because
ideally we would like the output mode (-o kubevirt) to both create the
target disks and populate them at the same time.

So to work around this problem, we need a tool which can inspect the
virt-v2v source hypervisor before we do the conversion in order to
find out how many disks are needed and their sizes.  Then we can
create the target disks, and then we can run a second container with
virt-v2v attached to the disks to do the conversion and populate the
output.
n
This is a proposed tool to do this.  It essentially uses the same -i*
options as virt-v2v (and no -o* options) and outputs various useful
metadata.  Example:

  $ ./run virt-v2v-inspector --quiet -i disk /var/tmp/fedora-32.img 
  virt-v2v-inspector: The QEMU Guest Agent will be installed for this guest 
at first boot.
  virt-v2v-inspector: warning: /files/boot/grub2/device.map/hd0 references 
unknown device "vda".  You may have to fix this entry manually after 
conversion.
  
  

virt-v2v-inspector
virt-v2v
2.1.9

  
6442450944
1400897536
  


  linux
  fedora
  fedora32
  x86_64
  32
  0
  rpm
  dnf
  Fedora 32 (Thirty Two)

  

There should be sufficient information in the  section to
allocate target disks, plus additional information is printed which
might be useful.

Note that we do a full conversion in order to generate this
information.  In particular it's not possible to generate the
 estimate without this.  It's plausible we could have a
--no-convert option, but I'm not sure it's worthwhile: it would only
save a little time, but would make everything less accurate, plus
maybe it is a good idea to find out if conversion is going to work
before we create the target disks?

I chose XML instead of JSON for output.  XML allows us to annotate
elements with attributes like "estimated='true'".  It also lets us
represent 64 bit number accurately, where JSON cannot represent such
numbers.

One obvious problem is that (without --quiet) the program mixes up
informational output with the final document, which is a bit of a
pain.  Ideas here?

Rich.



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

[Libguestfs] [PATCH v2v] New virt-v2v-inspector tool

[Richard W.M. Jones]

ram please contact the
+developers.
+
+=item *
+
+Numbers representing sizes are always given in bytes.
+
+=back
+
+ 
+ 
+   virt-v2v-inspector
+   virt-v2v
+   2.1.9
+
+The EprogramE, EpackageE and EversionE
+elements refer to the current version of virt-v2v-inspector and are
+useful for debugging.  Make sure you use the same version of
+virt-v2v-inspector and virt-v2v.
+
+   
+ 
+   6442450944
+   1400897536
+ 
+ 
+   6442450944
+   45131520
+ 
+   
+
+The EdisksE element lists information about each guest disk.
+The example virtual machine above has two disks.
+Evirtual-sizeE describes the size of the disk as seen from
+inside the guest, while EallocatedE is an estimate of how much
+storage will be needed on the host after conversion.  This is assuming
+you use S> - see the notes below.
+
+   
+ linux
+ fedora
+ fedora32
+ x86_64
+ [...]
+   
+
+The EoperatingsystemE element lists information about the
+guest operating system gleaned during conversion, in a manner similar
+to the L tool from guestfs-tools.
+
+=head2 Output allocation mode and output format
+
+Virt-v2v supports selecting the output allocation mode (I<-oa> option)
+and output format (I<-of> option, eg. S>).  Since it is
+difficult to predict the effect of these options on the actual space
+occupied by the final image this tool does not account for them.
+
+As a rule of thumb:
+
+=over 4
+
+=item S
+
+causes the disk images on the target to consume their full virtual
+size (excluding the effect of zero allocations will depends so much on
+the underlying storage that it is often hard even for experts to
+predict).
+
+=item S
+
+uses the QCOW2 format where supported which means that the apparent
+size of the file will be equal to its sparse size, but otherwise
+should not affect estimates very much.
+
+=back
+
+=head1 OPTIONS
+
+=over 4
+
+=item B<--help>
+
+Display help.
+
+=item B<-v>
+
+=item B<--verbose>
+
+Enable verbose messages for debugging.
+
+=item B<-V>
+
+=item B<--version>
+
+Display version number and exit.
+
+=item B<-x>
+
+Enable tracing of libguestfs API calls.
+
+=item B<-i> ...
+
+=item B<-ic> ...
+
+=item B<-if> ...
+
+=item B<-io> ...
+
+=item B<-ip> ...
+
+=item B<-it> ...
+
+All of the I<-i*> options supported by virt-v2v and also supported by
+virt-v2v-inspector.
+
+=item B<-b> ...
+
+=item B<--bridge> ...
+
+=item B<--colors>
+
+=item B<--colours>
+
+=item B<--echo-keys>
+
+=item B<--key> ...
+
+=item B<--keys-from-stdin>
+
+=item B<--mac> ...
+
+=item B<--machine-readable>
+
+=item B<--machine-readable>=format
+
+=item B<-n> ...
+
+=item B<--network> ...
+
+=item B<-q>
+
+=item B<--quiet>
+
+=item B<--root> ...
+
+=item B<--wrap>
+
+These options work in the same way as the equivalent virt-v2v options.
+
+=back
+
+=head1 FILES
+
+Files used are the same as for virt-v2v.  See L.
+
+=head1 ENVIRONMENT VARIABLES
+
+Environment variables used are the same as for virt-v2v.  See
+L.
+
+=head1 SEE ALSO
+
+L,
+L,
+L,
+L,
+L,
+L,
+L,
+L<http://libguestfs.org/>.
+
+=head1 AUTHORS
+
+Matthew Booth
+
+Cédric Bosdonnat
+
+Laszlo Ersek
+
+Tomáš Golembiovský
+
+Shahar Havivi
+
+Richard W.M. Jones
+
+Roman Kagan
+
+Mike Latimer
+
+Nir Soffer
+
+Pino Toscano
+
+Xiaodai Wang
+
+Ming Xie
+
+Tingting Zheng
+
+=head1 COPYRIGHT
+
+Copyright (C) 2009-2022 Red Hat Inc.
diff --git a/docs/virt-v2v.pod b/docs/virt-v2v.pod
index 4901c8407f..4f3d977a15 100644
--- a/docs/virt-v2v.pod
+++ b/docs/virt-v2v.pod
@@ -21,6 +21,9 @@ There is also a companion front-end called L 
which comes
 as an ISO, CD or PXE image that can be booted on physical machines to
 virtualize those machines (physical to virtual, or p2v).
 
+To estimate the disk space needed before conversion, see
+L.
+
 For in-place conversion, there is a separate tool called
 L.
 
@@ -1624,6 +1627,7 @@ 
L<https://rwmj.wordpress.com/2015/09/18/importing-kvm-guests-to-ovirt-or-rhev/#c
 =head1 SEE ALSO
 
 L,
+L,
 L,
 L,
 L,
diff --git a/configure.ac b/configure.ac
index b2396781d6..f8e2836551 100644
--- a/configure.ac
+++ b/configure.ac
@@ -146,6 +146,7 @@ AC_CONFIG_FILES([Makefile
  gnulib/lib/Makefile
  in-place/Makefile
  input/Makefile
+ inspector/Makefile
  lib/Makefile
  lib/config.ml
  output/Makefile
diff --git a/Makefile.am b/Makefile.am
index cec68d76ce..16cd5f36d9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,7 @@ SUBDIRS += input
 SUBDIRS += output
 SUBDIRS += convert
 SUBDIRS += v2v
+SUBDIRS += inspector
 SUBDIRS += in-place
 
 SUBDIRS += tests
@@ -111,7 +112,7 @@ po/POTFILES: configure.ac
 po/POTFILES-ml: configure.ac
rm -f $@ $@-t
cd $(srcdir); \
-   find common/ml* lib in-place input outp

Re: [Libguestfs] [hivex PATCH] Add OCaml 5.0 support

[Richard W.M. Jones]

On Wed, Nov 16, 2022 at 07:00:57PM +, Kate Deplaix wrote:
> Hi,
> 
> Here is a patch to add support to the upcoming OCaml 5.0 to hivex: https://
> github.com/kit-ty-kate/hivex/commit/e89b4ab014fa764bf392275ba0dfa7631ea7689b
> 
> This new release of the compiler will be released early next month.

Thanks - I have pushed this ...

> Note: this patch breaks support for OCaml < 4.07. If you want to
> keep support I suppose you can add some hacks in the configure.ac
> file to add "module Stdlib = Pervasives" at the start of the
> "generator/generator.ml" file but I'm not sure how worth to you it
> is to support old +5 years old compilers.

Yes this does in fact break RHEL 7 (ocaml 4.05).  However hivex was
already broken on RHEL 7 because of a mismatch with the GNU gettext
version.  Also hivex is a package that we rarely need to touch and
RHEL 7 is going out of support.  So I think we're fine here.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] OpenSUSE virt-builder error

[Richard W.M. Jones]

Since a lot of people have asked me on IRC already this morning, let's
try to make this searchable ...

  $ virt-builder --list
  virt-builder: error: failed to download 
  
http://download.opensuse.org/repositories/Virtualization:/virt-builder-images/images/index:
 
  HTTP status code 404

The workaround is to delete or rename this file:

  /etc/virt-builder/repos.d/opensuse.conf

I will try to work with opensuse to find out what's going on with the
link.  Otherwise I will push out an update that disables this repo.

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] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Tue, Nov 15, 2022 at 06:40:33PM +0100, Laszlo Ersek wrote:
> On 11/15/22 17:02, Richard W.M. Jones wrote:
> > On Thu, Nov 10, 2022 at 04:08:57PM +0100, Laszlo Ersek wrote:
> >> Per perlpod(1), the canonical Formatting Code for this would be Z<>, as
> >> in (untested):
> >>
> >> =item Z<>6
> >>
> >> However, I can't see a single instance of Z<> in the v2v projects, so it
> >> could cause compatibility problems. I guess let's stick with S<> then.
> > 
> > It seems as if Z<> isn't supported in RHEL 7 :-(
> 
> That's strange, at least the perlpod(1) manual on RHEL7 does mention
> Z<>, just not for this one particular use case that we'd need it for.

Ugh ... I should have checked.

Yes you're quite right, it _is_ supported on RHEL 7.  I've now
actually tested it using podwrapper and hacking a POD file in the
nbdkit sources (on RHEL 7).

> Either way, Eric chose S<> in his patches!
> 
> Laszlo

Thanks,

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] [libnbd PATCH] api: Generate flag values as unsigned

[Richard W.M. Jones]

On Fri, Nov 11, 2022 at 11:27:51AM -0600, Eric Blake wrote:
> C says that performing bitwise math on ints can lead to corner-case
> undefined values.  When we document something as being usable as a
> flag with bitwise-OR and similar, we should ensure the values are
> unsigned, to avoid those corner-cases in C.  Even our own codebase was
> potentially falling foul of strict C rules, with code like
> tests/errors-server-invalid-offset.c:
> 
>   uint32_t strict;
>   strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_BOUNDS;
> 
> In theory, this is an API change that might be detectable by someone
> trying to probe which integer promotions occur on a given constant.
> In practice, no one writes code that intentionally tries to exploit
> whether a flag is signed or unsigned, and none of our values were
> large enough to worry about sign-extension effects that occur when
> flipping the sign bit of a signed value (we tend to target machines
> with twos-complement signed values where the behavior is sane despite
> being undefined by C), so this patch is therefore not an ABI change.
> 
> Generated code changes as follows:
> 
> | --- include/libnbd.h.bak2022-11-11 11:15:54.929686924 -0600
> | +++ include/libnbd.h2022-11-11 11:15:56.652698919 -0600
> | @@ -69,32 +69,32 @@
> |  #define LIBNBD_SIZE_MAXIMUM  2
> |  #define LIBNBD_SIZE_PAYLOAD  3
> |
> | -#define LIBNBD_CMD_FLAG_FUA  0x01
> | -#define LIBNBD_CMD_FLAG_NO_HOLE  0x02
> | ...
> | -#define LIBNBD_ALLOW_TRANSPORT_VSOCK 0x04
> | -#define LIBNBD_ALLOW_TRANSPORT_MASK  0x07
> | +#define LIBNBD_CMD_FLAG_FUA  0x01U
> | +#define LIBNBD_CMD_FLAG_NO_HOLE  0x02U
> | ...
> | +#define LIBNBD_ALLOW_TRANSPORT_VSOCK 0x04U
> | +#define LIBNBD_ALLOW_TRANSPORT_MASK  0x07U
> 
> Thanks: Laszlo Ersek 
> ---
> 
> I'll push this along with the LIBNBD_STRICT_PAYLOAD series.
> 
>  generator/C.ml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index cfa2964c..f9171996 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -437,11 +437,11 @@ let
>List.iter (
>  fun (flag, i) ->
>let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in
> -  pr "#define %-40s 0x%02x\n" flag i;
> +  pr "#define %-40s 0x%02xU\n" flag i;
>mask := !mask lor i
>) flags;
>let flag = sprintf "LIBNBD_%s_MASK" flag_prefix in
> -  pr "#define %-40s 0x%02x\n" flag !mask;
> +  pr "#define %-40s 0x%02xU\n" flag !mask;
>pr "\n"
>) all_flags;
>List.iter (
> -- 

Looks good, thanks.

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 1/2] api: Add LIBNBD_STRICT_PAYLOAD to honor qemu 32M write limit

[Richard W.M. Jones]

On Wed, Nov 09, 2022 at 05:26:06PM -0600, Eric Blake wrote:
> Modern qemu tends to advertise a strict 32M payload limit.  Yet we
> default to allowing the user to attempt up to 64M in pwrite.  For
> pread, qemu will reply with EINVAL but keep the connection up; but for
> pwrite, an overlarge buffer is fatal.  It's time to teach libnbd to
> honor qemu's max buffer by default, while still allowing the user to
> disable the strict mode setting if they want to try 64M anyways.
> 
> This also involves advertising a new LIBNBD_SIZE_PAYLOAD via
> nbd_get_block_size, which is more reliable than asking the user to
> manually obey LIBNBD_SIZE_MAXIMUM which may not be set or may be
> larger than 64M.

I think you pushed this, but a couple of comments:

(1) New APIs are cheap!  Did we really want to overload nbd_get_block_size?

(2) There are quite a lot of places elsewhere in libnbd which
hard-code the maximum block size.  Grepping for "REQUEST_SIZE" is
instructive.  I think we should fix those as well.

Rich.

>  lib/internal.h  |  4 +++
>  generator/API.ml| 39 -
>  generator/states-newstyle-opt-export-name.c |  1 +
>  generator/states-newstyle-opt-go.c  |  1 +
>  generator/states-oldstyle.c |  1 +
>  lib/flags.c | 25 +
>  lib/rw.c|  8 -
>  7 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index 22f500b4..bbbd2639 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -146,6 +146,8 @@ struct nbd_handle {
>uint32_t block_minimum;
>uint32_t block_preferred;
>uint32_t block_maximum;
> +  /* Payload cap, always non-zero, based on block_maximum when feasible */
> +  uint32_t payload_maximum;
> 
>/* Server canonical name and description, or NULL if not advertised. */
>char *canonical_name;
> @@ -448,6 +450,8 @@ extern int nbd_internal_set_size_and_flags (struct 
> nbd_handle *h,
>  extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min,
>  uint32_t pref, uint32_t max)
>LIBNBD_ATTRIBUTE_NONNULL(1);
> +extern void nbd_internal_set_payload (struct nbd_handle *h)
> +  LIBNBD_ATTRIBUTE_NONNULL(1);
> 
>  /* is-state.c */
>  extern bool nbd_internal_is_state_created (enum state state);
> diff --git a/generator/API.ml b/generator/API.ml
> index 8d0d9d15..0ebc9298 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -184,6 +184,7 @@ let block_size_enum =
>  "MINIMUM",   0;
>  "PREFERRED", 1;
>  "MAXIMUM",   2;
> +"PAYLOAD",   3;
>]
>  }
>  let all_enums = [ tls_enum; block_size_enum ]
> @@ -221,6 +222,7 @@ let strict_flags =
>  "BOUNDS", 1 lsl 2;
>  "ZERO_SIZE",  1 lsl 3;
>  "ALIGN",  1 lsl 4;
> +"PAYLOAD",1 lsl 5;
>]
>  }
>  let allow_transport_flags = {
> @@ -1060,10 +1062,21 @@   "set_strict_mode", {
>  =item C = 5
> 
>  If set, and the server provided minimum block sizes (see
> -L, this flag rejects client requests that
> -do not have length and offset aligned to the server's minimum
> -requirements.  If clear, unaligned requests are sent to the server,
> -where it is up to the server whether to honor or reject the request.
> +C for L), this
> +flag rejects client requests that do not have length and offset
> +aligned to the server's minimum requirements.  If clear,
> +unaligned requests are sent to the server, where it is up to
> +the server whether to honor or reject the request.
> +
> +=item C = 6
> +
> +If set, the client refuses to send a command to the server
> +with more than libnbd's outgoing payload maximum (see
> +C for L), whether
> +or not the server advertised a block size maximum.  If clear,
> +oversize requests up to 64MiB may be attempted, although
> +requests larger than 32MiB are liable to cause some servers to
> +disconnect.
> 
>  =back
> 
> @@ -2286,6 +2299,15 @@   "get_block_size", {
>  itself imposes a maximum of 64M.  If zero, some NBD servers will
>  abruptly disconnect if a transaction involves more than 32M.
> 
> +=item C = 3
> +
> +This value is not advertised by the server, but rather represents
> +the maximum outgoing payload size for a given connection that
> +libnbd will enforce unless C is cleared
> +in C.  It is always non-zero: never
> +smaller than 1M, never larger than 64M, and matches
> +C when possible.
> +
>  =back
> 
>  Future NBD extensions may result in additional C values.
> @@ -2449,10 +2471,11 @@   "pwrite", {
>  acknowledged by the server, or there is an error.  Note this will
>  generally return an error if L is true.
> 
> -Note that libnbd currently enforces a maximum write buffer of 64MiB,
> -even if the server would permit a larger buffer in a single transaction;
> -attempts to exceed this will result in an C error.  The server
> -may enforce a smaller limit, which can be learned with

Re: [Libguestfs] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Wed, Nov 09, 2022 at 02:43:53PM -0600, Eric Blake wrote:
> Make it possible for the sh and eval plugins to disconnect a client or
> shut down the entire nbdkit server by use of special return values.
> Prior to this patch we had reserved 4-7 for future use; this defines
> 4-8, and extends the set of reserved return values to 9-15.  We figure
> it is unlikely that anyone is using status 4-15 with the intent that
> it behaves identically to status 1; more importantly, the choice of
> soft disconnect with error for status 8 is the one least likely to
> break such an existing sh or eval script.
> 
> For the testsuite, I only covered the eval plugin; but since it shares
> common code with the sh plugin, both styles should work.
> 
> A note about the pod documentation: when the argument to =item would
> otherwise appear to be a single number, the use of S<> to mask it is
> necessary.
> ---
>  plugins/sh/nbdkit-sh-plugin.pod |  41 +-
>  tests/Makefile.am   |   2 +
>  plugins/sh/call.h   |   7 +-
>  plugins/sh/call.c   |  92 ++---
>  plugins/sh/methods.c|   2 +-
>  tests/test-eval-disconnect.sh   | 236 
>  6 files changed, 324 insertions(+), 56 deletions(-)
>  create mode 100755 tests/test-eval-disconnect.sh
> 
> diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
> index 1c539599..2905eced 100644
> --- a/plugins/sh/nbdkit-sh-plugin.pod
> +++ b/plugins/sh/nbdkit-sh-plugin.pod
> @@ -96,7 +96,7 @@ The script should exit with specific exit codes:
> 
>  The method was executed successfully.
> 
> -=item 1 and 8-127
> +=item 1 and 16-255
> 
>  There was an error.  The script may print on stderr an errno name,
>  optionally followed by whitespace and a message, for example:
> @@ -123,9 +123,42 @@ The requested method is not supported by the script.
> 
>  For methods which return booleans, this code indicates false.
> 
> -=item 4, 5, 6, 7
> +=item 4 and 5
> 
> -These exit codes are reserved for future use.
> +Triggers a call to the C function C, which requests
> +an asynchronous exit of the nbdkit server (disconnecting all clients).
> +The client will usually get a response before shutdown is complete
> +(although this is racy); so once the shutdown is requested, code 4
> +then behaves like code 0 (stderr is ignored, and the server tries to
> +return success), and code 5 behaves like code 1 (the server tries to
> +return an error to the client parsed from stderr, although a missing
> +error defaults to C instead of C).
> +
> +=item S<6>
> +
> +Triggers a call to the C function C with C
> +set to true, which requests an abrupt disconnect of the current
> +client.  The contents of stderr are irrelevant with this status, since
> +the client will not get a response.
> +
> +=item 7 and 8

Isn't S<> needed here?

> +Triggers a call to the C function C with C
> +set to false, which requests a soft disconnect of the current client
> +(future client requests are rejected with C without calling
> +into the plugin, but current requests may complete).  Since the client
> +will likely get the response to this command, code 7 then behaves like
> +code 0 (stderr is ignored, and the server tries to return success),
> +and code 8 behaves like code 1 (the server tries to return an error to
> +the client parsed from stderr, although a missing error defaults to
> +C instead of C).
> +
> +=item 9-15
> +
> +These exit codes are reserved for future use.  Note that versions of
> +nbdkit E 1.34 documented that codes S<8> through S<15> behaved
> +like code S<1>; although it is unlikely that many scripts relied on
> +this similarity in practice.
> 
>  =back
> 
> @@ -583,4 +616,4 @@ Richard W.M. Jones
> 
>  =head1 COPYRIGHT
> 
> -Copyright (C) 2018-2020 Red Hat Inc.
> +Copyright (C) 2018-2022 Red Hat Inc.
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index a4e93686..b58d2d65 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -769,6 +769,7 @@ TESTS += \
>   test-eval-exports.sh \
>   test-eval-cache.sh \
>   test-eval-dump-plugin.sh \
> + test-eval-disconnect.sh \
>   $(NULL)
>  EXTRA_DIST += \
>   test-eval.sh \
> @@ -776,6 +777,7 @@ EXTRA_DIST += \
>   test-eval-exports.sh \
>   test-eval-cache.sh \
>   test-eval-dump-plugin.sh \
> + test-eval-disconnect.sh \
>   $(NULL)
> 
>  # file plugin test.
> diff --git a/plugins/sh/call.h b/plugins/sh/call.h
> index fab77a3c..57da7e98 100644
> --- a/plugins/sh/call.h
> +++ b/plugins/sh/call.h
> @@ -52,8 +52,13 @@ typedef enum exit_code {
>ERROR 

Re: [Libguestfs] [nbdkit PATCH v2 2/2] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Thu, Nov 10, 2022 at 04:08:57PM +0100, Laszlo Ersek wrote:
> Per perlpod(1), the canonical Formatting Code for this would be Z<>, as
> in (untested):
> 
> =item Z<>6
> 
> However, I can't see a single instance of Z<> in the v2v projects, so it
> could cause compatibility problems. I guess let's stick with S<> then.

It seems as if Z<> isn't supported in RHEL 7 :-(

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] [nbdkit PATCH v2 1/2] sh: Advertise max known status in --dump-plugin

[Richard W.M. Jones]

On Wed, Nov 09, 2022 at 02:43:52PM -0600, Eric Blake wrote:
> @@ -51,7 +51,9 @@ typedef enum exit_code {
>OK = 0,
>ERROR = 1,   /* all script error codes are mapped to this */
>MISSING = 2, /* method missing */
> -  RET_FALSE = 3/* script exited with code 3 meaning false */
> +  RET_FALSE = 3,   /* script exited with code 3 meaning false */
> +  /* Adjust methods.c:sh_dump_plugin when defining new codes */
> +  /* 4-7 is reserved since 1.8; handle like ERROR for now */
>  } exit_code;
> 
>  extern exit_code call (const char **argv)
> diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
> index 5c1ee5f8..4a75a3a0 100644
> --- a/plugins/sh/methods.c
> +++ b/plugins/sh/methods.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018-2021 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -58,6 +58,10 @@ sh_dump_plugin (void)
>const char *args[] = { script, method, NULL };
>CLEANUP_FREE_STRING string o = empty_vector;
> 
> +  /* Dump information about the sh/eval features */
> +  printf ("max_known_status=%d\n", RET_FALSE);

It also took me a double-take here to realise that this prints the
maximum known status number instead of a boolean of some kind.  So as
Laszlo suggests this might be better with some enum sentinel to record
the first reserved value.

Anyway it's all good so:

Reviewed-by: Richard W.M. Jones 

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH] disks.c: skip SCSI floppy drives with no disk inserted

[Richard W.M. Jones]

On Fri, Nov 11, 2022 at 03:24:18PM +0100, Laszlo Ersek wrote:
> Hi Rich,
> 
> On 11/09/22 14:52, Laszlo Ersek wrote:
> > A SCSI floppy drive with no disk inserted looks like a normal /dev/sd*
> > node, but causes the nbdkit file plugin to fail with ENOMEDIUM. Filter out
> > such devices altogether -- unlike CD-ROMs (for which we create a device
> > model in the target, albeit with no medium inserted), empty floppies
> > should not be converted in any way.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=2140997
> > Signed-off-by: Laszlo Ersek 
> > ---
> > 
> > Notes:
> > This patch still needs testing; the ISO image is at
> > 
> > ,
> > sha256 sum:
> > b0666a9140b03e12829982179bf7da2ac5477737fb53760d2e8c527d8a2bf55a.
> > 
> >  disks.c | 58 
> >  1 file changed, 58 insertions(+)
> > 
> > diff --git a/disks.c b/disks.c
> > index 4eb006246d84..aafe467f9e9a 100644
> > --- a/disks.c
> > +++ b/disks.c
> > @@ -20,6 +20,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -67,6 +68,57 @@ partition_parent (dev_t part_dev)
> >return makedev (parent_major, parent_minor);
> >  }
> >  
> > +/**
> > + * Return true if the named device (eg. C) is a Removable 
> > Media
> > + * SCSI Disk with no media inserted. This covers floppy drives, but not 
> > CD-ROM
> > + * drives (intentionally).
> > + */
> > +static int
> > +device_has_no_media (const char *dev)
> > +{
> > +  int ret;
> > +  gchar *sysfs_pathname;
> > +  gchar *sysfs_contents;
> > +  gsize sysfs_size;
> > +  gchar *dev_pathname;
> > +  int dev_fd;
> > +
> > +  ret = 0;
> > +
> > +  if (!STRPREFIX (dev, "sd"))
> > +return ret;
> > +
> > +  sysfs_pathname = g_strdup_printf ("/sys/block/%s/removable", dev);
> > +
> > +  if (!g_file_get_contents (sysfs_pathname, _contents, _size, 
> > NULL))
> > +goto free_sysfs_pathname;
> > +
> > +  if (sysfs_size < 2 || sysfs_contents[0] != '1' || sysfs_contents[1] != 
> > '\n')
> > +goto free_sysfs_contents;
> > +
> > +  dev_pathname = g_strdup_printf ("/dev/%s", dev);
> > +
> > +  dev_fd = open (dev_pathname, O_RDONLY | O_CLOEXEC);
> > +  if (dev_fd == -1) {
> > +if (errno == ENOMEDIUM)
> > +  ret = 1;
> > +
> > +goto free_dev_pathname;
> > +  }
> > +  close (dev_fd);
> > +
> > +free_dev_pathname:
> > +  g_free (dev_pathname);
> > +
> > +free_sysfs_contents:
> > +  g_free (sysfs_contents);
> > +
> > +free_sysfs_pathname:
> > +  g_free (sysfs_pathname);
> > +
> > +  return ret;
> > +}
> > +
> >  /**
> >   * Return true if the named device (eg. C) contains the
> >   * root filesystem.  C is the major:minor of the root
> > @@ -139,6 +191,12 @@ find_all_disks (char ***disks, char ***removable)
> >  STRPREFIX (d->d_name, "ubd") ||
> >  STRPREFIX (d->d_name, "vd")) {
> >char *p;
> > +  /* Skip SCSI disk drives with removable media that have no media 
> > inserted
> > +   * -- effectively, empty floppy drives. Note that SCSI CD-ROMs are 
> > named
> > +   * C and thus handled on the other branch.
> > +   */
> > +  if (device_has_no_media (d->d_name))
> > +continue;
> >  
> >/* Skip the device containing the root filesystem. */
> >if (device_contains (d->d_name, root_device))
> > ___
> > Libguestfs mailing list
> > Libguestfs@redhat.com
> > https://listman.redhat.com/mailman/listinfo/libguestfs
> > 
> 
> Tingting and Vera have verified this patch, in
> . Are you OK with
> the patch? (Sorry for asking if it has been on your radar already!)

I was on holiday!  Patch looks fine though, so ACK.

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] [PATCH v2] curl: Fix verification of CURLOPT_TLS13_CIPHERS

[Richard W.M. Jones]

On Thu, Nov 10, 2022 at 05:17:35PM +0100, Michal Orzel wrote:
> The code checking for CURLOPT_TLS13_CIPHERS option did not work
> properly, because of incorrect assumption that this symbol was a
> preprocessor macro. It is in fact element of enum type, which
> resulted with #ifdef directive working improperly. Fix changes that
> check to be based on curl version instead.
> 
> Signed-off-by: Michal Orzel 
> ---
>  plugins/curl/curl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
> index 77f88fff..724ffd43 100644
> --- a/plugins/curl/curl.c
> +++ b/plugins/curl/curl.c
> @@ -560,10 +560,11 @@ curl_open (int readonly)
>if (ssl_cipher_list)
>  curl_easy_setopt (h->c, CURLOPT_SSL_CIPHER_LIST, ssl_cipher_list);
>if (tls13_ciphers) {
> -#ifdef CURLOPT_TLS13_CIPHERS
> +#if (LIBCURL_VERSION_MAJOR > 7) || \
> +(LIBCURL_VERSION_MAJOR == 7 && LIBCURL_VERSION_MINOR >= 61)
>  curl_easy_setopt (h->c, CURLOPT_TLS13_CIPHERS, tls13_ciphers);
>  #else
> -/* This is not available in, eg, RHEL 7 */
> +/* This is not available before curl-7.61 */
>  nbdkit_error ("tls13-ciphers is not supported in this build of "
>"nbdkit-curl-plugin");
>  goto err;

Thanks Michal.  I confirmed that this compiles on RHEL 7 and gives an
error (as expected):

$ ./nbdkit -U - curl file:///dev/null tls13-ciphers=test --run '../libnbd/run 
nbdinfo $uri'
nbdkit: curl[1]: error: tls13-ciphers is not supported in this build of 
nbdkit-curl-plugin
nbdkit: curl[1]: error: tls13-ciphers is not supported in this build of 
nbdkit-curl-plugin
/home/rjones/d/libnbd/info/.libs/lt-nbdinfo: nbd_opt_go: server replied with 
error to opt_go request: No such file or directory for the default export
/home/rjones/d/libnbd/info/.libs/lt-nbdinfo: suggestion: to list all exports on 
the server, use --list
protocol: newstyle-fixed without TLS

And on Fedora it works:

$ ./nbdkit -U - curl file:///dev/null tls13-ciphers=test --run 'nbdinfo $uri'
protocol: newstyle-fixed without TLS, using structured packets
export="":
export-size: 0
content: empty
uri: nbd+unix:///?socket=/tmp/nbdkitZwdPXJ/socket
contexts:
base:allocation
is_rotational: false
is_read_only: false
can_cache: false
can_df: true
can_fast_zero: true
can_flush: false
can_fua: false
can_multi_conn: false
can_trim: false
can_zero: true

I have pushed this as commit 242757dd5c9fbf00a487ab934d67db442fe08661.

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] [nbdkit PATCH] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Wed, Nov 09, 2022 at 01:47:15PM -0600, Eric Blake wrote:
> On Wed, Nov 02, 2022 at 01:36:26PM +0000, Richard W.M. Jones wrote:
> > > > -=item 4, 5, 6, 7
> > > > +=item S<4>
> > > 
> > > The S<> notation seems new here (so it's going to be inconsistent with
> > > the rest of this file, I think).
> > 
> > I was going to mention this too.  The S<> notation is used to insert
> > non-breaking spaces (for output formats that support it) in a span of
> > text so that it won't be folded over multiple lines.  AFAIK it
> > shouldn't have any effect here.
> 
> Ah, but it does:
> 
> Pod input around line 121: Expected text after =item, not a number
> 
> The use of S<> is there to keep the pod formatter happy when =item's
> sole argument would otherwise look like a number instead of arbitrary
> text.

Oh indeed!  I had forgotten about this weirdness of pod.

$ cat test.pod
=over 4

=item 2

A

=item 3

B
=back

$ pod2text test.pod 
2   A

test.pod around line 7: Expected text after =item, not a number
3   B

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] [nbdkit PATCH] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Thu, Nov 10, 2022 at 02:23:39PM +0100, Laszlo Ersek wrote:
> On 11/09/22 20:47, Eric Blake wrote:
> > On Wed, Nov 02, 2022 at 01:36:26PM +, Richard W.M. Jones wrote:
> >>>> -=item 4, 5, 6, 7
> >>>> +=item S<4>
> >>>
> >>> The S<> notation seems new here (so it's going to be inconsistent with
> >>> the rest of this file, I think).
> >>
> >> I was going to mention this too.  The S<> notation is used to insert
> >> non-breaking spaces (for output formats that support it) in a span of
> >> text so that it won't be folded over multiple lines.  AFAIK it
> >> shouldn't have any effect here.
> > 
> > Ah, but it does:
> > 
> > Pod input around line 121: Expected text after =item, not a number
> > 
> > The use of S<> is there to keep the pod formatter happy when =item's
> > sole argument would otherwise look like a number instead of arbitrary
> > text.
> > 
> 
> Sigh. :)
> 
> Thanks for the reminder. I've now found that
> <https://libguestfs.org/virt-v2v-output-local.1.html>
> ["docs/virt-v2v-output-local.pod" in the v2v tree] is a good
> demonstration for numbered lists, bullet lists, and "option" list.
> 
> Interestingly, perlpod(1) does not recommend S<> for this kind of
> escaping, it recommends Z<>:
> 
>•   And perhaps most importantly, keep the items
>consistent: either use "=item *" for all of them, to
>produce bullets; or use "=item 1.", "=item 2.", etc.,
>to produce numbered lists; or use "=item foo", "=item
>bar", etc.--namely, things that look nothing like
>bullets or numbers.  (If you have a list that contains
>both: 1) things that don't look like bullets nor
>numbers,  plus 2) things that do, you should preface
>the bullet- or number-like items with "Z<>".  See Z<>
>below for an example.)
> 
>If you start with bullets or numbers, stick with them,
>as formatters use the first "=item" type to decide how
>to format the list.
> ...
>"S" -- text contains non-breaking spaces
>This means that the words in text should not be broken
>across lines.  Example: "S<$x ? $y : $z>".
> ...
>"Z<>" -- a null (zero-effect) formatting code
> ...
>Another use is to indicate that stuff in "=item
>Z<>stuff..."  is not to be considered to be a bullet or
>number.  For example, without the "Z<>", the line
> 
> =item Z<>500 Server error
> 
>could possibly be parsed as an item in a numbered list when
>it isn't meant to be.
> ...
> 
> More interestingly, *this particular use* of Z<> is not documented in
> the RHEL7 manual of perlpod(1) -- the above quote is from Fedora 35! So
> that lack of specific documentation on RHEL7 might be why we chose to
> wrap the digits in S<>, rather than to isolate them with Z<>.

Actually the first I've heard of Z<> formatting.  Could be a recent
addition as you say.  We do need to keep things working on RHEL 7 (as
far as possible without heroics).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH] curl: Fix verification of CURLOPT_TLS13_CIPHERS

[Richard W.M. Jones]

On Tue, Nov 08, 2022 at 12:56:13PM +0100, Michal Orzel wrote:
> The code checking for CURLOPT_TLS13_CIPHERS option did not work
> properly, because of incorrect assumption that this symbol was a
> preprocessor macro. It is in fact element of enum type, which
> resulted with #ifdef directive working improperly. Change replaces
> compile-time verification with run-time, based on return value of
> curl_easy_setopt function.

Understood, but ...

> Signed-off-by: Michal Orzel 
> ---
>  plugins/curl/curl.c | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
> index 9a818bfa..42b70f01 100644
> --- a/plugins/curl/curl.c
> +++ b/plugins/curl/curl.c
> @@ -560,14 +560,13 @@ curl_open (int readonly)
>if (ssl_cipher_list)
>  curl_easy_setopt (h->c, CURLOPT_SSL_CIPHER_LIST, ssl_cipher_list);
>if (tls13_ciphers) {
> -#ifdef CURLOPT_TLS13_CIPHERS
> -curl_easy_setopt (h->c, CURLOPT_TLS13_CIPHERS, tls13_ciphers);
> -#else
> -/* This is not available in, eg, RHEL 7 */
> -nbdkit_error ("tls13-ciphers is not supported in this build of "
> -  "nbdkit-curl-plugin");
> -goto err;
> -#endif
> +r = curl_easy_setopt (h->c, CURLOPT_TLS13_CIPHERS, tls13_ciphers);

... this still fails on RHEL 7 as the enum isn't defined:

$ rpm -q curl
curl-7.29.0-59.el7.x86_64

--
In file included from /usr/include/curl/curl.h:2251:0,
 from curl.c:47:
curl.c: In function 'curl_open':
curl.c:563:33: error: 'CURLOPT_TLS13_CIPHERS' undeclared (first use in this 
function)
 r = curl_easy_setopt (h->c, CURLOPT_TLS13_CIPHERS, tls13_ciphers);
 ^
curl.c:563:33: note: each undeclared identifier is reported only once for each 
function it appears in
--

I think you need to check for the enum in configure.ac.  Unfortunately
autoconf provides no useful facility for this so you have to use
AC_COMPILE_IFELSE :-(

Let me know if you get into any difficulties ...

Rich.


> +if (r != CURLE_OK) {
> +  /* This is not available in, eg, RHEL 7 */
> +  display_curl_error (h, r, "curl_easy_setopt: CURLOPT_TLS13_CIPHERS 
> [%s]",
> +tls13_ciphers);
> +  goto err;
> +}
>}
>if (tcp_keepalive)
>  curl_easy_setopt (h->c, CURLOPT_TCP_KEEPALIVE, 1L);
> -- 
> 2.25.1
> 
> -
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII 
> Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 
> 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy 
> z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w 
> transakcjach handlowych.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
> moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
> wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
> jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the 
> sole use of the intended recipient(s). If you are not the intended recipient, 
> please contact the sender and delete all copies; any review or distribution 
> by others is strictly prohibited.
> 

-- 
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 0/2] ocaml: Simplify the sockaddr test

[Richard W.M. Jones]

On Mon, Nov 07, 2022 at 10:39:27AM +0100, Laszlo Ersek wrote:
> On 11/04/22 13:45, Richard W.M. Jones wrote:
> > Not as bad as I thought it would be!
> > 
> > Rich.
> > 
> > 
> 
> series
> Reviewed-by: Laszlo Ersek 

Thanks - upstream in:

https://gitlab.com/nbdkit/libnbd/-/commit/6233fc579d51e97b07e2d16b887c5133ac13640a
https://gitlab.com/nbdkit/libnbd/-/commit/49c2375ad22903e5e5aea74d98bd0698d5c9cf89

I pushed it on Saturday so sorry I didn't pick up your R-b tag.

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 v2 3/3] nbdsh: Improve --help and initial banner contents.

[Richard W.M. Jones]

For the series:

Acked-by: Richard W.M. Jones 

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
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 Sat, Nov 05, 2022 at 03:39:26AM +0200, Nir Soffer wrote:
> On Fri, Nov 4, 2022 at 11:18 PM Eric Blake  wrote:
> [...]
> 
> @@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
> option_string=None):
>          os.environ["LIBNBD_DEBUG"] = "1"
> 
>      # Create the handle.
> -    if not args.n:
> +    if args.n:
> +        pass
> +    else:
> 
> 
> Why add useless branch?

Yes this is a bit of a strange change.  What was wrong with the original
"if not ..."?

Perl's "unless" statement FTW :-)

> 
> +        global h
>          h = nbd.NBD()
>          h.set_handle_name("nbdsh")
> 
> @@ -165,21 +170,35 @@ def line(x): lines.append(x)
>      def blank(): line("")
>      def example(ex, desc): line("%-34s # %s" % (ex, desc))
> 
> +    connect_hint = False
> +    go_hint = False
>      blank()
>      line("Welcome to nbdsh, the shell for interacting with")
>      line("Network Block Device (NBD) servers.")
>      blank()
> -    if not args.n:
> -        line("The ‘nbd’ module has already been imported and there")
> -        line("is an open NBD handle called ‘h’.")
> -        blank()
> -    else:
> +    if args.n:
>          line("The ‘nbd’ module has already been imported.")
>          blank()
>          example("h = nbd.NBD()", "Create a new handle.")
> -    if False:  # args.uri is None:
> 
> 
> Good that this was removed, but it will be better to remove in the previous
> patch.

I believe for intermediate patches this is OK, if it's resolved in the end.

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

[Libguestfs] [PATCH libnbd 2/2] ocaml: Simplify the sockaddr test

[Richard W.M. Jones]

The sockaddr test added in commit 50500aade9 ("ocaml: Implement
sockaddr type") was quite complicated because it had to run nbdkit as
a subprocess, manage the socket, check for a PID file etc.

The simpler way of doing this is to make it work like the Python test
(python/test-aio-connect-unix.sh) where we run
nbdkit -U - ... --run './test $unixsocket'

The straightforward way would be to a separate shell script etc to
ocaml/tests/Makefile.am:TESTS which is orthogonal to how the existing
tests work.  So instead make the test exec nbdkit when called first
time (without the $unixsocket parameter) and then run the test when
called the second time.

Updates: commit 50500aade9f32899eddd2a7b89ae5c596674f95c
---
 ocaml/tests/test_580_aio_connect.ml | 69 +
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/ocaml/tests/test_580_aio_connect.ml 
b/ocaml/tests/test_580_aio_connect.ml
index 43c6fd0f7a..5a94ac0c9b 100644
--- a/ocaml/tests/test_580_aio_connect.ml
+++ b/ocaml/tests/test_580_aio_connect.ml
@@ -17,51 +17,42 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  *)
 
+(* This test is unusual because we want to run it under nbdkit
+ * rather than having the test run nbdkit as a subprocess.
+ *
+ * Therefore we detect if a $unixsocket parameter is passed
+ * in Sys.argv.  If not then we exec nbdkit:
+ *   nbdkit -U - ... --run '$argv0 \$unixsocket'
+ * If the $unixsocket parameter is present then we run the test.
+ *)
+
 open Unix
 open Printf
 
 let () =
-  let nbd = NBD.create () in
+  match Array.length Sys.argv with
+  | 1 ->(* exec nbdkit *)
+ let argv0 = Sys.argv.(0) in
+ let runcmd = sprintf "%s $unixsocket" (Filename.quote argv0) in
+ execvp "nbdkit" [| "nbdkit"; "-U"; "-"; "--exit-with-parent";
+"memory"; "size=512";
+"--run"; runcmd |]
 
-  (* Unlike other tests, we're going to run nbdkit as a subprocess
-   * by hand and have it listening on a randomly named socket
-   * that we create.
-   *)
-  let sock = Filename.temp_file "580-" ".sock" in
-  unlink sock;
-  let pidfile = Filename.temp_file "580-" ".pid" in
-  unlink pidfile;
-  let cmd =
-sprintf "nbdkit -U %s -P %s --exit-with-parent memory size=512 &"
-  (Filename.quote sock) (Filename.quote pidfile) in
-  if Sys.command cmd <> 0 then
-failwith "nbdkit command failed";
-  let rec loop i =
-if i > 60 then
-  failwith "nbdkit subcommand did not start up";
-if not (Sys.file_exists pidfile) then (
-  sleep 1;
-  loop (i+1)
-)
-  in
-  loop 0;
+  | 2 ->(* run the test *)
+ let unixsocket = Sys.argv.(1) in
+ let nbd = NBD.create () in
 
-  (* Connect to the subprocess using a Unix.sockaddr. *)
-  let sa = ADDR_UNIX sock in
-  NBD.aio_connect nbd sa;
-  while NBD.aio_is_connecting nbd do
-ignore (NBD.poll nbd 1)
-  done;
-  assert (NBD.aio_is_ready nbd);
-  NBD.close nbd;
+ (* Connect to the subprocess using a Unix.sockaddr. *)
+ let sa = ADDR_UNIX unixsocket in
+ NBD.aio_connect nbd sa;
+ while NBD.aio_is_connecting nbd do
+   ignore (NBD.poll nbd 1)
+ done;
+ assert (NBD.aio_is_ready nbd);
+ assert (NBD.get_size nbd = 512_L);
+ NBD.close nbd
 
-  (* Kill the nbdkit subprocess. *)
-  let chan = open_in pidfile in
-  let pid = int_of_string (input_line chan) in
-  kill pid Sys.sigterm;
-
-  (* Clean up files. *)
-  unlink sock;
-  unlink pidfile
+  | _ ->
+ failwith "unexpected test parameters"
 
 let () = Gc.compact ()
-- 
2.37.0.rc2

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

[Libguestfs] [PATCH libnbd 0/2] ocaml: Simplify the sockaddr test

[Richard W.M. Jones]

Not as bad as I thought it would be!

Rich.


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

[Libguestfs] [PATCH libnbd 1/2] python: Rename the aio-connect test

[Richard W.M. Jones]

This is a test, but it had a confusing name, so rename it to test-*.sh.

Updates: commit b8d0dd8e8d15219161dad9d029ece0b49fcbb565
---
 python/Makefile.am| 4 ++--
 .../{python-aio-connect-unix.sh => test-aio-connect-unix.sh}  | 0
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/Makefile.am b/python/Makefile.am
index f51d40e16a..f1e701d03c 100644
--- a/python/Makefile.am
+++ b/python/Makefile.am
@@ -36,7 +36,7 @@ EXTRA_DIST = \
$(generator_built) \
nbdsh.py \
pycodestyle.sh \
-   python-aio-connect-unix.sh \
+   test-aio-connect-unix.sh \
$(srcdir)/t/*.py \
$(NULL)
 
@@ -85,8 +85,8 @@ TESTS_ENVIRONMENT = \
$(NULL)
 LOG_COMPILER = $(top_builddir)/run
 TESTS += \
-   python-aio-connect-unix.sh \
run-python-tests \
+   test-aio-connect-unix.sh \
$(NULL)
 
 endif HAVE_NBDKIT
diff --git a/python/python-aio-connect-unix.sh b/python/test-aio-connect-unix.sh
similarity index 100%
rename from python/python-aio-connect-unix.sh
rename to python/test-aio-connect-unix.sh
-- 
2.37.0.rc2

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

Re: [Libguestfs] [PATCH libnbd] ocaml: Implement sockaddr type

[Richard W.M. Jones]

On Fri, Nov 04, 2022 at 08:22:59AM +0100, Laszlo Ersek wrote:
> On 11/03/22 16:16, Richard W.M. Jones wrote:
> > On Thu, Nov 03, 2022 at 04:03:42PM +0100, Laszlo Ersek wrote:
> >> On 11/02/22 22:10, Richard W.M. Jones wrote:
> >>> +open Unix
> >>> +open Printf
> >>> +
> >>> +let () =
> >>> +  let nbd = NBD.create () in
> >>> +
> >>> +  (* Unlike other tests, we're going to run nbdkit as a subprocess
> >>> +   * by hand and have it listening on a randomly named socket
> >>> +   * that we create.
> >>> +   *)
> >>> +  let sock = Filename.temp_file "580-" ".sock" in
> >>> +  unlink sock;
> >>> +  let pidfile = Filename.temp_file "580-" ".pid" in
> >>> +  unlink pidfile;
> >>> +  let cmd =
> >>> +sprintf "nbdkit -U %s -P %s --exit-with-parent memory size=512 &"
> >>> +  (Filename.quote sock) (Filename.quote pidfile) in
> >>> +  if Sys.command cmd <> 0 then
> >>> +failwith "nbdkit command failed";
> >>> +  let rec loop i =
> >>> +if i > 60 then
> >>> +  failwith "nbdkit subcommand did not start up";
> >>> +if not (Sys.file_exists pidfile) then (
> >>> +  sleep 1;
> >>> +  loop (i+1)
> >>> +)
> >>> +  in
> >>> +  loop 0;
> >>> +
> >>> +  (* Connect to the subprocess using a Unix.sockaddr. *)
> >>> +  let sa = ADDR_UNIX sock in
> >>> +  NBD.aio_connect nbd sa;
> >>> +  while NBD.aio_is_connecting nbd do
> >>> +ignore (NBD.poll nbd 1)
> >>> +  done;
> >>> +  assert (NBD.aio_is_ready nbd);
> >>> +  NBD.close nbd;
> >>> +
> >>> +  (* Kill the nbdkit subprocess. *)
> >>> +  let chan = open_in pidfile in
> >>> +  let pid = int_of_string (input_line chan) in
> >>> +  kill pid Sys.sigint;
> >>
> >> I think it's more customary to send SIGTERM in such situations; SIGINT
> >> is more like an interactive interrupt signal (usually sent by the
> >> terminal driver when the INTR character is entered on the terminal).
> >> POSIX calls SIGINT "Terminal interrupt signal", and SIGTERM "Termination
> >> signal". But it's really tangential.
> > 
> > I changed it to SIGTERM now (commit eb13699a75).
> > 
> > The whole test is very unsatisfactory though.  Compare it to the
> > elegance of the equivalent Python test:
> > 
> > https://gitlab.com/nbdkit/libnbd/-/blob/master/python/python-aio-connect-unix.sh
> 
> Can you perhaps introduce the test not under ML_TESTS but TESTS (called
> "test_580_aio_connect.sh")? Then use the "captive nbdkit" pattern
> (--run) just like in the python example.
> 
> Now, ocaml doesn't support "-c" (I think), so the "$PYTHON -c" idea
> won't work identically, but if you also introduced
> "test_580_aio_connect.ml", and began that with an "ocaml shebang", that
> should work, shouldn't it?
> 
> nbdkit [...] --run 'test_580_aio_connect.ml "$unixsocket"'
> 
> (Not sure if it's worth the churn though.)

I was thinking about a complicated scheme with re-execing the test
under nbdkit, but the above might well work too.  I'll see what I can
do.

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] ocaml: Implement sockaddr type

[Richard W.M. Jones]

On Thu, Nov 03, 2022 at 04:03:42PM +0100, Laszlo Ersek wrote:
> On 11/02/22 22:10, Richard W.M. Jones wrote:
> > +open Unix
> > +open Printf
> > +
> > +let () =
> > +  let nbd = NBD.create () in
> > +
> > +  (* Unlike other tests, we're going to run nbdkit as a subprocess
> > +   * by hand and have it listening on a randomly named socket
> > +   * that we create.
> > +   *)
> > +  let sock = Filename.temp_file "580-" ".sock" in
> > +  unlink sock;
> > +  let pidfile = Filename.temp_file "580-" ".pid" in
> > +  unlink pidfile;
> > +  let cmd =
> > +sprintf "nbdkit -U %s -P %s --exit-with-parent memory size=512 &"
> > +  (Filename.quote sock) (Filename.quote pidfile) in
> > +  if Sys.command cmd <> 0 then
> > +failwith "nbdkit command failed";
> > +  let rec loop i =
> > +if i > 60 then
> > +  failwith "nbdkit subcommand did not start up";
> > +if not (Sys.file_exists pidfile) then (
> > +  sleep 1;
> > +  loop (i+1)
> > +)
> > +  in
> > +  loop 0;
> > +
> > +  (* Connect to the subprocess using a Unix.sockaddr. *)
> > +  let sa = ADDR_UNIX sock in
> > +  NBD.aio_connect nbd sa;
> > +  while NBD.aio_is_connecting nbd do
> > +ignore (NBD.poll nbd 1)
> > +  done;
> > +  assert (NBD.aio_is_ready nbd);
> > +  NBD.close nbd;
> > +
> > +  (* Kill the nbdkit subprocess. *)
> > +  let chan = open_in pidfile in
> > +  let pid = int_of_string (input_line chan) in
> > +  kill pid Sys.sigint;
> 
> I think it's more customary to send SIGTERM in such situations; SIGINT
> is more like an interactive interrupt signal (usually sent by the
> terminal driver when the INTR character is entered on the terminal).
> POSIX calls SIGINT "Terminal interrupt signal", and SIGTERM "Termination
> signal". But it's really tangential.

I changed it to SIGTERM now (commit eb13699a75).

The whole test is very unsatisfactory though.  Compare it to the
elegance of the equivalent Python test:

https://gitlab.com/nbdkit/libnbd/-/blob/master/python/python-aio-connect-unix.sh

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd] ocaml: Implement sockaddr type

[Richard W.M. Jones]

On Wed, Nov 02, 2022 at 09:10:02PM +, Richard W.M. Jones wrote:
> Convert Unix.sockaddr to struct sockaddr.  OCaml provides a function
> to do this ('get_sockaddr' - not namespaced!)  This function was
> present at least as far back as RHEL 7 (OCaml 4.05).

The namespacing has actually been fixed upstream ('caml_unix_get_sockaddr').
There is a backwards compatible #define, but I guess we will need to
have some autoconf test to choose the right symbol.  I don't have a
version of OCaml that has the namespaced symbol.

Rich.

> This also adds a simple test.
> ---
>  generator/OCaml.ml  |  8 ++--
>  ocaml/helpers.c | 23 ++
>  ocaml/nbd-c.h   |  3 ++
>  ocaml/tests/Makefile.am |  1 +
>  ocaml/tests/test_580_aio_connect.ml | 67 +
>  5 files changed, 99 insertions(+), 3 deletions(-)
> 
> diff --git a/generator/OCaml.ml b/generator/OCaml.ml
> index 8711eab57c..6a280b6734 100644
> --- a/generator/OCaml.ml
> +++ b/generator/OCaml.ml
> @@ -49,7 +49,7 @@ and
>| Int _ -> "int"
>| Int64 _ -> "int64"
>| Path _ -> "string"
> -  | SockAddrAndLen _ -> "string" (* XXX not impl *)
> +  | SockAddrAndLen _ -> "Unix.sockaddr"
>| SizeT _ -> "int" (* OCaml int type is always sufficient for counting *)
>| String _ -> "string"
>| StringList _ -> "string list"
> @@ -702,9 +702,11 @@ let
>  | SizeT n ->
> pr "  size_t %s = Int_val (%sv);\n" n n
>  | SockAddrAndLen (n, len) ->
> -   pr "  const struct sockaddr *%s;\n" n;
> +   pr "  struct sockaddr_storage %s_storage;\n" n;
> +   pr "  struct sockaddr *%s = (struct sockaddr *) &%s_storage;\n" n n;
> pr "  socklen_t %s;\n" len;
> -   pr "  abort ();\n" (* XXX *)
> +   pr "  nbd_internal_unix_sockaddr_to_sa (%sv, &%s_storage, &%s);\n"
> + n n len
>  | StringList n ->
> pr "  char **%s = (char **) nbd_internal_ocaml_string_list (%sv);\n" 
> n n
>  | UInt n | UIntPtr n ->
> diff --git a/ocaml/helpers.c b/ocaml/helpers.c
> index aafb970ff9..2981135647 100644
> --- a/ocaml/helpers.c
> +++ b/ocaml/helpers.c
> @@ -23,6 +23,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  #include 
>  #include 
> @@ -30,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -130,6 +133,26 @@ nbd_internal_ocaml_alloc_int64_from_uint32_array 
> (uint32_t *a, size_t len)
>CAMLreturn (rv);
>  }
>  
> +/* Convert a Unix.sockaddr to a C struct sockaddr. */
> +void
> +nbd_internal_unix_sockaddr_to_sa (value sockaddrv,
> +  struct sockaddr_storage *ss,
> +  socklen_t *len)
> +{
> +  CAMLparam1 (sockaddrv);
> +  union sock_addr_union sa_u;
> +  socklen_param_type sl; /* this is really an int or socklen_t */
> +
> +  memset (ss, 0, sizeof *ss);
> +
> +  get_sockaddr (sockaddrv, _u, );
> +  assert (sl <= sizeof *ss);
> +  memcpy (ss, _u, sl);
> +  *len = sl;
> +
> +  CAMLreturn0;
> +}
> +
>  /* Common code when an exception is raised in an OCaml callback.
>   *
>   * We handle Assert_failure specially by abort()-ing.  Other
> diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h
> index 0bf044ca91..8b0c088da7 100644
> --- a/ocaml/nbd-c.h
> +++ b/ocaml/nbd-c.h
> @@ -23,6 +23,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -62,6 +63,8 @@ extern void nbd_internal_ocaml_raise_closed (const char 
> *func) Noreturn;
>  extern const char **nbd_internal_ocaml_string_list (value);
>  extern value nbd_internal_ocaml_alloc_int64_from_uint32_array (uint32_t *,
> size_t);
> +extern void nbd_internal_unix_sockaddr_to_sa (value, struct sockaddr_storage 
> *,
> +  socklen_t *);
>  extern void nbd_internal_ocaml_exception_in_wrapper (const char *, value);
>  
>  /* Extract an NBD handle from an OCaml heap value. */
> diff --git a/ocaml/tests/Makefile.am b/ocaml/tests/Makefile.am
> index 328d53e543..2cd36eb067 100644
> --- a/ocaml/tests/Makefile.am
> +++ b/ocaml/tests/Makefile.am
> @@ -42,6 +42,7 @@ ML_TESTS = \
>   test_500_aio_pread.ml \
>   test_505_aio_pread_structured_callback.ml \
>   test_510_aio_pwrite.ml \
> + test_580_aio_connect.ml \
>   test_590_aio_copy.ml \
> 

[Libguestfs] [PATCH libnbd] ocaml: Implement sockaddr type

[Richard W.M. Jones]

Convert Unix.sockaddr to struct sockaddr.  OCaml provides a function
to do this ('get_sockaddr' - not namespaced!)  This function was
present at least as far back as RHEL 7 (OCaml 4.05).

This also adds a simple test.
---
 generator/OCaml.ml  |  8 ++--
 ocaml/helpers.c | 23 ++
 ocaml/nbd-c.h   |  3 ++
 ocaml/tests/Makefile.am |  1 +
 ocaml/tests/test_580_aio_connect.ml | 67 +
 5 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/generator/OCaml.ml b/generator/OCaml.ml
index 8711eab57c..6a280b6734 100644
--- a/generator/OCaml.ml
+++ b/generator/OCaml.ml
@@ -49,7 +49,7 @@ and
   | Int _ -> "int"
   | Int64 _ -> "int64"
   | Path _ -> "string"
-  | SockAddrAndLen _ -> "string" (* XXX not impl *)
+  | SockAddrAndLen _ -> "Unix.sockaddr"
   | SizeT _ -> "int" (* OCaml int type is always sufficient for counting *)
   | String _ -> "string"
   | StringList _ -> "string list"
@@ -702,9 +702,11 @@ let
 | SizeT n ->
pr "  size_t %s = Int_val (%sv);\n" n n
 | SockAddrAndLen (n, len) ->
-   pr "  const struct sockaddr *%s;\n" n;
+   pr "  struct sockaddr_storage %s_storage;\n" n;
+   pr "  struct sockaddr *%s = (struct sockaddr *) &%s_storage;\n" n n;
pr "  socklen_t %s;\n" len;
-   pr "  abort ();\n" (* XXX *)
+   pr "  nbd_internal_unix_sockaddr_to_sa (%sv, &%s_storage, &%s);\n"
+ n n len
 | StringList n ->
pr "  char **%s = (char **) nbd_internal_ocaml_string_list (%sv);\n" n n
 | UInt n | UIntPtr n ->
diff --git a/ocaml/helpers.c b/ocaml/helpers.c
index aafb970ff9..2981135647 100644
--- a/ocaml/helpers.c
+++ b/ocaml/helpers.c
@@ -23,6 +23,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 #include 
 #include 
@@ -30,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -130,6 +133,26 @@ nbd_internal_ocaml_alloc_int64_from_uint32_array (uint32_t 
*a, size_t len)
   CAMLreturn (rv);
 }
 
+/* Convert a Unix.sockaddr to a C struct sockaddr. */
+void
+nbd_internal_unix_sockaddr_to_sa (value sockaddrv,
+  struct sockaddr_storage *ss,
+  socklen_t *len)
+{
+  CAMLparam1 (sockaddrv);
+  union sock_addr_union sa_u;
+  socklen_param_type sl; /* this is really an int or socklen_t */
+
+  memset (ss, 0, sizeof *ss);
+
+  get_sockaddr (sockaddrv, _u, );
+  assert (sl <= sizeof *ss);
+  memcpy (ss, _u, sl);
+  *len = sl;
+
+  CAMLreturn0;
+}
+
 /* Common code when an exception is raised in an OCaml callback.
  *
  * We handle Assert_failure specially by abort()-ing.  Other
diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h
index 0bf044ca91..8b0c088da7 100644
--- a/ocaml/nbd-c.h
+++ b/ocaml/nbd-c.h
@@ -23,6 +23,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -62,6 +63,8 @@ extern void nbd_internal_ocaml_raise_closed (const char 
*func) Noreturn;
 extern const char **nbd_internal_ocaml_string_list (value);
 extern value nbd_internal_ocaml_alloc_int64_from_uint32_array (uint32_t *,
size_t);
+extern void nbd_internal_unix_sockaddr_to_sa (value, struct sockaddr_storage *,
+  socklen_t *);
 extern void nbd_internal_ocaml_exception_in_wrapper (const char *, value);
 
 /* Extract an NBD handle from an OCaml heap value. */
diff --git a/ocaml/tests/Makefile.am b/ocaml/tests/Makefile.am
index 328d53e543..2cd36eb067 100644
--- a/ocaml/tests/Makefile.am
+++ b/ocaml/tests/Makefile.am
@@ -42,6 +42,7 @@ ML_TESTS = \
test_500_aio_pread.ml \
test_505_aio_pread_structured_callback.ml \
test_510_aio_pwrite.ml \
+   test_580_aio_connect.ml \
test_590_aio_copy.ml \
test_600_debug_callback.ml \
test_610_exception.ml \
diff --git a/ocaml/tests/test_580_aio_connect.ml 
b/ocaml/tests/test_580_aio_connect.ml
new file mode 100644
index 00..95acc18c10
--- /dev/null
+++ b/ocaml/tests/test_580_aio_connect.ml
@@ -0,0 +1,67 @@
+(* hey emacs, this is OCaml code: -*- tuareg -*- *)
+(* libnbd OCaml test case
+ * Copyright (C) 2013-2022 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ *)
+
+open Unix

[Libguestfs] [PATCH libnbd] ocaml: Implement sockaddr type

[Richard W.M. Jones]

This is pushed already, commit 50500aade9.


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

Re: [Libguestfs] [nbdkit PATCH] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Tue, Nov 01, 2022 at 02:56:09PM -0500, Eric Blake wrote:
...
> +=item S<6>
> +
> +Triggers a call to the C function C with C
> +set to false, which requests a soft disconnect of the current client
> +(future client requests are rejected with C without calling
> +into the plugin, but current requests may complete).  Since the client
> +will likely get the response to this command, nbdkit then treats empty
> +stderr as success for the current callback, and parses non-empty
> +stderr as if the script had exited with code S<1>.

OK .. seems like complicated behaviour, but I can sort of see the
reasoning behind it.

I do wonder if we just want to use separate codes for the "soft
disconnect + OK" and the "soft disconnect + error" cases.  We have
reserved more so we won't run out :-)

> +=item 7-15
> +
> +These exit codes are reserved for future use.  Note that versions of
> +nbdkit E 1.34 documented that codes S<8> through S<15> behaved

This is actually a case where S<> around the nbdkit E 1.34 makes
sense.  But it should be removed around S<8> etc and in the next line
below.

> +like code S<1>; although it is unlikely that many scripts relied on
> +this similarity in practice.

[...]

> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index d59b797c..3994fc6a 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -768,12 +768,14 @@ TESTS += \
>   test-eval-file.sh \
>   test-eval-exports.sh \
>   test-eval-cache.sh \
> + test-eval-disconnect.sh \
>   $(NULL)
>  EXTRA_DIST += \
>   test-eval.sh \
>   test-eval-file.sh \
>   test-eval-exports.sh \
>   test-eval-cache.sh \
> + test-eval-disconnect.sh \
>   $(NULL)



>  # file plugin test.
> diff --git a/plugins/sh/call.h b/plugins/sh/call.h
> index 76de23a3..44767e81 100644
> --- a/plugins/sh/call.h
> +++ b/plugins/sh/call.h
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -51,7 +51,12 @@ typedef enum exit_code {
>OK = 0,
>ERROR = 1,   /* all script error codes are mapped to this */
>MISSING = 2, /* method missing */
> -  RET_FALSE = 3/* script exited with code 3 meaning false */
> +  RET_FALSE = 3,   /* script exited with code 3 meaning false */
> +  SHUTDOWN = 4,/* call nbdkit_shutdown() before parsing stderr */
> +  DISC_FORCE = 5,  /* call nbdkit_disconnect(true) */
> +  DISC_SOFT = 6,   /* call nbdkit_disconnect(false) */
> +  /* 7 is reserved since 1.8; handle like ERROR for now */
> +  /* 8-15 are reserved since 1.34; handle like ERROR for now */
>  } exit_code;
> 
>  extern exit_code call (const char **argv)
> diff --git a/plugins/sh/call.c b/plugins/sh/call.c
> index 2fa94d88..7d0f2b16 100644
> --- a/plugins/sh/call.c
> +++ b/plugins/sh/call.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018-2020 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -397,17 +397,47 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to 
> stdin (can be NULL) */
>return ret;
>  }
> 
> -static void
> -handle_script_error (const char *argv0, string *ebuf)
> +/* Normalize return codes and parse error string. */
> +static exit_code
> +handle_script_error (const char *argv0, string *ebuf, exit_code code)
>  {
>int err;
>size_t skip = 0;
>char *p;
> 
> -  if (ebuf->len == 0) {
> +  switch (code) {
> +  case OK:
> +  case MISSING:
> +  case RET_FALSE:
> +/* Script successful. */
> +return code;
> +
> +  case SHUTDOWN:
> +/* Use trigger, then handle empty ebuf specially below */
> +nbdkit_shutdown ();
> +break;
> +
> +  case DISC_FORCE:
> +  case DISC_SOFT:
> +/* Use trigger, then handle empty ebuf specially below */
> +nbdkit_disconnect (code == DISC_FORCE);
> +break;
> +
> +  case ERROR:
> +  default:
> +/* Error even if ebuf is empty */
>  err = EIO;
> +goto parse;
> +  }
> +
> +  assert (code >= SHUTDOWN && code <= DISC_SOFT);
> +  if (ebuf->len == 0)
> +return OK;
> +  err = ESHUTDOWN;
> +
> + parse:
> +  if (ebuf->len == 0)
>  goto no_error_message;
> -  }

I guess if we had the separate codes then we wouldn't need the goto?

>/* Recognize the errno values that match NBD protocol errors */
>if (ascii_strncasecmp (ebuf->ptr, "EPERM", 5) == 0) {
> @@ -502,6 +532,7 @@ handle_script_error (const char *argv0, string *ebuf)
> 
>/* Set errno. */
>errno = err;
> +  return ERROR;
>  }
> 
>  /* Call the script with parameters.  Don't write to stdin or read from
> @@ -516,19 +547,7 @@ call (const char **argv)
>CLEANUP_FREE_STRING string ebuf = empty_vector;
> 
>r = call3 (NULL, 0, , , argv);

Re: [Libguestfs] [nbdkit PATCH] sh: Add exit status triggers for nbdkit_{shutdown, disconnect}

[Richard W.M. Jones]

On Wed, Nov 02, 2022 at 02:15:45PM +0100, Laszlo Ersek wrote:
> On 11/01/22 20:56, Eric Blake wrote:
> > Make it possible for the sh and eval plugins to disconnect a client or
> > shut down the entire nbdkit server by use of special return values.
> > Prior to this patch we had reserved 4-7 for future use; this defines
> > 4-6, and extends the set of reserved return values to 7-15.  We figure
> > it is unlikely that anyone is using status 8-15 with the intent that
> > it behaves identically to status 1.
> > 
> > For the testsuite, I only covered the eval plugin; but since it shares
> > common code with the sh plugin, both styles should work.
> > ---
> > 
> > Finally got the testsuite additions for this in a state that I like.
> > 
> >  plugins/sh/nbdkit-sh-plugin.pod |  37 ++-
> >  tests/Makefile.am   |   2 +
> >  plugins/sh/call.h   |   9 +-
> >  plugins/sh/call.c   |  85 +++
> >  tests/test-eval-disconnect.sh   | 185 
> >  5 files changed, 268 insertions(+), 50 deletions(-)
> >  create mode 100755 tests/test-eval-disconnect.sh
> > 
> > diff --git a/plugins/sh/nbdkit-sh-plugin.pod 
> > b/plugins/sh/nbdkit-sh-plugin.pod
> > index 2a55fdc9..37139e1b 100644
> > --- a/plugins/sh/nbdkit-sh-plugin.pod
> > +++ b/plugins/sh/nbdkit-sh-plugin.pod
> > @@ -96,4 +96,4 @@ The script should exit with specific exit codes:
> > 
> >  The method was executed successfully.
> > 
> > -=item 1 and 8-127
> > +=item 1 and 16-255
> > 
> >  There was an error.  The script may print on stderr an errno name,
> >  optionally followed by whitespace and a message, for example:
> > @@ -123,9 +123,38 @@ The requested method is not supported by the script.
> > 
> >  For methods which return booleans, this code indicates false.
> > 
> > -=item 4, 5, 6, 7
> > +=item S<4>
> 
> The S<> notation seems new here (so it's going to be inconsistent with
> the rest of this file, I think).

I was going to mention this too.  The S<> notation is used to insert
non-breaking spaces (for output formats that support it) in a span of
text so that it won't be folded over multiple lines.  AFAIK it
shouldn't have any effect here.

For some reason this existing list uses:

  =item S<0>

but I think that must be a mistake.

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] [nbdkit PATCH 2/4] server: Give client EOF when we are done writing

[Richard W.M. Jones]

On Mon, Oct 31, 2022 at 08:56:48AM -0500, Eric Blake wrote:
> On Thu, Oct 27, 2022 at 09:49:52AM +0100, Richard W.M. Jones wrote:
> > On Wed, Oct 26, 2022 at 05:18:00PM -0500, Eric Blake wrote:
> > > -  if (conn->sockin >= 0)
> > > -closesocket (conn->sockin);
> > > -  if (conn->sockout >= 0 && conn->sockin != conn->sockout)
> > > -closesocket (conn->sockout);
> > > +  if (conn->sockout >= 0 && how == SHUT_WR) {
> > > +if (conn->sockin == conn->sockout)
> > > +  shutdown (conn->sockout, how);
> > > +else
> > > +  closesocket (conn->sockout);
> > > +conn->sockout = -1;
> > 
> > Don't we leak conn->sockin, if how == SHUT_WR && conn->sockin == 
> > conn->sockout?
> 
> No, because any call with how==SHUT_WR will eventually be followed by
> another call with how==SHUT_RDWR, and it is the latter call which
> handles closing sockin.

Got it, OK.

Series looks fine in that case.

Rich.

> > Rich.
> > 
> > > +  }
> > > +  else {
> > > +if (conn->sockin >= 0)
> > > +  closesocket (conn->sockin);
> > > +if (conn->sockout >= 0 && conn->sockin != conn->sockout)
> > > +  closesocket (conn->sockout);
> > > +  }
> > >  }
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH 4/4] blocksize-policy: Add blocksize-write-disconnect=N parameter

[Richard W.M. Jones]

sent()
> +  try:
> +h.pwrite(buf, 0)
> +assert expect_value == 0
> +  except nbd.Error as ex:
> +assert expect_value == ex.errnum
> +  if hasattr(h, \"stats_bytes_sent\"):
> +if expect_traffic:
> +  assert h.stats_bytes_sent() > start
> +else:
> +  assert h.stats_bytes_sent() == start
> +
> +h.set_strict_mode(0)  # Bypass client-side safety checks
> +# Beyond 64M
> +check(h, 64*1024*1024 + 1, errno.ERANGE, False)
> +check(h, 64*1024*1024 + 2, errno.ERANGE, False)
> +# Small reads
> +check(h, 1, errno.EINVAL)
> +check(h, 2, 0)
> +# Near 8M boundary
> +check(h, 8*1024*1024 - 2, 0)
> +check(h, 8*1024*1024 - 1, errno.EINVAL)
> +check(h, 8*1024*1024, 0)
> +check(h, 8*1024*1024 + 1, errno.EINVAL)
> +check(h, 8*1024*1024 + 2, errno.ENOMEM)
> +# Near 16M boundary
> +check(h, 16*1024*1024 - 2, errno.ENOMEM)
> +check(h, 16*1024*1024 - 1, errno.EINVAL)
> +check(h, 16*1024*1024, errno.ENOMEM)
> +check(h, 16*1024*1024 + 1, errno.EINVAL)
> +check(h, 16*1024*1024 + 2, errno.EINVAL)
> +# Near 32M boundary
> +check(h, 32*1024*1024 - 2, errno.EINVAL)
> +check(h, 32*1024*1024 - 1, errno.EINVAL)
> +check(h, 32*1024*1024, errno.EINVAL)
> +check(h, 32*1024*1024 + 1, errno.ENOTCONN)
> +assert h.aio_is_ready() is False
> +"'
> -- 

Reviewed-by: Richard W.M. Jones 

nbdkit-sh-plugin patches to follow?

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] [nbdkit PATCH 3/4] api: Add nbdkit_disconnect(int)

[Richard W.M. Jones]

onnect on write; the write and the
> +# pending read should still succeed, but second read attempt should fail.
> +nbdsh -u "nbd+unix:///?socket=$sock" -c '
> +import errno
> +
> +buf = nbd.Buffer(1)
> +c1 = h.aio_pread(buf, 1)
> +c2 = h.aio_pwrite(buf, 2)
> +h.poll(-1)
> +assert h.aio_peek_command_completed() == c2
> +h.aio_command_completed(c2)
> +c3 = h.aio_pread(buf, 3)
> +h.poll(-1)
> +assert h.aio_peek_command_completed() == c3
> +try:
> +  h.aio_command_completed(c3)
> +  assert False
> +except nbd.Error as ex:
> +  assert ex.errnum == errno.ESHUTDOWN
> +h.poll(-1)
> +assert h.aio_peek_command_completed() == c1
> +h.aio_command_completed(c1)
> +h.shutdown()
> +'
> +
> +# Non-empty export name does hard disconnect on write. The write and the
> +# pending read should fail with lost connection.
> +nbdsh -u "nbd+unix:///a?socket=$sock" -c '
> +import errno
> +
> +buf = nbd.Buffer(1)
> +c1 = h.aio_pread(buf, 1)
> +c2 = h.aio_pwrite(buf, 2)
> +while h.aio_in_flight() > 1:
> +  h.poll(-1)
> +assert h.aio_is_ready() is False
> +try:
> +  h.aio_command_completed(c1)
> +  assert False
> +except nbd.Error as ex:
> +  assert ex.errnum == errno.ENOTCONN
> +try:
> +  h.aio_command_completed(c2)
> +  assert False
> +except nbd.Error as ex:
> +  assert ex.errnum == errno.ENOTCONN
> +'
> +
> +# nbdkit should still be running
> +kill -s 0 $pid
> diff --git a/tests/test-disconnect-plugin.c b/tests/test-disconnect-plugin.c
> new file mode 100644
> index ..181b262f
> --- /dev/null
> +++ b/tests/test-disconnect-plugin.c
> @@ -0,0 +1,95 @@
> +/* nbdkit
> + * Copyright (C) 2013-2022 Red Hat Inc.
> + *
> + * 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.
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +static void
> +disconnect_unload (void)
> +{
> +  nbdkit_debug ("clean disconnect");
> +}
> +
> +static void *
> +disconnect_open (int readonly)
> +{
> +  return NBDKIT_HANDLE_NOT_NEEDED;
> +}
> +
> +static int64_t
> +disconnect_get_size (void *handle)
> +{
> +  return 1024*1024;
> +}
> +
> +#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
> +
> +/* Reads are delayed to show effect of disconnect on in-flight commands */
> +static int
> +disconnect_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
> +{
> +  memset (buf, 0, count);
> +  if (nbdkit_nanosleep (2, 0) == -1)
> +nbdkit_debug ("read delay ended early, returning success anyway");
> +  return 0;
> +}
> +
> +/* Writing causes a disconnect; export name determines severity. */
> +static int
> +disconnect_pwrite (void *handle, const void *buf, uint32_t count,
> +   uint64_t offset)
> +{
> +  const char *name = nbdkit_export_name ();
> +  bool hard = name && *name;
> +  nbdkit_debug ("%s disconnect triggered!", hard ? "hard" : "soft");
> +  nbdkit_disconnect (hard);
> +  /* Despite the disconnect, we still claim the write succeeded */
> +  return 0;
> +}
> +
> +static struct nbdkit_plugin plugin = {
> +  .name  = "disconnect",
> +  .version   = PACKAGE_VERSION,
> +  .unload= disconnect_unload,
> +  .open  = disconnect_open,
> +  .get_size  = disconnect_get_size,
> +  .pread = disconnect_pread,
> +  .pwrite= disconnect_pwrite,
> +};
> +
> +NBDKIT_REGISTER_PLUGIN(plugin)

Reviewed-by: Richard W.M. Jones 

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] [nbdkit PATCH 2/4] server: Give client EOF when we are done writing

[Richard W.M. Jones]

On Wed, Oct 26, 2022 at 05:18:00PM -0500, Eric Blake wrote:
> -  if (conn->sockin >= 0)
> -closesocket (conn->sockin);
> -  if (conn->sockout >= 0 && conn->sockin != conn->sockout)
> -closesocket (conn->sockout);
> +  if (conn->sockout >= 0 && how == SHUT_WR) {
> +if (conn->sockin == conn->sockout)
> +  shutdown (conn->sockout, how);
> +else
> +  closesocket (conn->sockout);
> +conn->sockout = -1;

Don't we leak conn->sockin, if how == SHUT_WR && conn->sockin == conn->sockout?

Rich.

> +  }
> +  else {
> +if (conn->sockin >= 0)
> +  closesocket (conn->sockin);
> +if (conn->sockout >= 0 && conn->sockin != conn->sockout)
> +  closesocket (conn->sockout);
> +  }
>  }
> diff --git a/server/crypto.c b/server/crypto.c
> index 1f605083..72486bf8 100644
> --- a/server/crypto.c
> +++ b/server/crypto.c
> @@ -412,7 +412,7 @@ crypto_send (const void *vbuf, size_t len, int flags)
>   * close, so this function ignores errors.
>   */
>  static void
> -crypto_close (void)
> +crypto_close (int how)
>  {
>GET_CONN;
>gnutls_session_t session = conn->crypto_session;
> @@ -420,17 +420,21 @@ crypto_close (void)
> 
>assert (session != NULL);
> 
> -  gnutls_transport_get_int2 (session, , );
> +  if (how == SHUT_WR)
> +gnutls_bye (session, GNUTLS_SHUT_WR);
> +  else {
> +gnutls_transport_get_int2 (session, , );
> 
> -  gnutls_bye (session, GNUTLS_SHUT_RDWR);
> +gnutls_bye (session, GNUTLS_SHUT_RDWR);
> 
> -  if (sockin >= 0)
> -closesocket (sockin);
> -  if (sockout >= 0 && sockin != sockout)
> -closesocket (sockout);
> +if (sockin >= 0)
> +  closesocket (sockin);
> +if (sockout >= 0 && sockin != sockout)
> +  closesocket (sockout);
> 
> -  gnutls_deinit (session);
> -  conn->crypto_session = NULL;
> +gnutls_deinit (session);
> +conn->crypto_session = NULL;
> +  }
>  }
> 
>  #ifdef WIN32
> -- 
> 2.37.3
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

-- 
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] [nbdkit PATCH 1/4] server: Switch connection status to enum

[Richard W.M. Jones]

sockin, count) < 0)
> -return connection_set_status (-1);
> +  skip_over_write_buffer (conn->sockin, count) < 0) {
> +connection_set_status (STATUS_DEAD);
> +return;
> +  }
>goto send_reply;
>  }
> 
> @@ -677,8 +684,10 @@ protocol_recv_request_send_reply (void)
>if (buf == NULL) {
>  error = ENOMEM;
>  if (cmd == NBD_CMD_WRITE &&
> -skip_over_write_buffer (conn->sockin, count) < 0)
> -  return connection_set_status (-1);
> +skip_over_write_buffer (conn->sockin, count) < 0) {
> +  connection_set_status (STATUS_DEAD);
> +  return;
> +}
>  goto send_reply;
>}
>  }
> @@ -702,13 +711,14 @@ protocol_recv_request_send_reply (void)
>}
>if (r == -1) {
>  nbdkit_error ("read data: %s: %m", name_of_nbd_cmd (cmd));
> -return connection_set_status (-1);
> +connection_set_status (STATUS_DEAD);
> +return;
>}
>  }
>}
> 
>/* Perform the request.  Only this part happens inside the request lock. */
> -  if (quit || !connection_get_status ()) {
> +  if (quit || connection_get_status () == STATUS_CLIENT_DONE) {
>  error = ESHUTDOWN;
>}
>else {
> @@ -720,8 +730,8 @@ protocol_recv_request_send_reply (void)
> 
>/* Send the reply packet. */
>   send_reply:
> -  if (connection_get_status () < 0)
> -return -1;
> +  if (connection_get_status () < STATUS_CLIENT_DONE)
> +return;
> 
>if (error != 0) {
>  /* Since we're about to send only the limited NBD_E* errno to the
> @@ -742,19 +752,14 @@ protocol_recv_request_send_reply (void)
>(cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) {
>  if (!error) {
>if (cmd == NBD_CMD_READ)
> -return send_structured_reply_read (request.handle, cmd,
> -   buf, count, offset);
> +send_structured_reply_read (request.handle, cmd, buf, count, offset);
>else /* NBD_CMD_BLOCK_STATUS */
> -return send_structured_reply_block_status (request.handle,
> -   cmd, flags,
> -   count, offset,
> -   extents);
> +send_structured_reply_block_status (request.handle, cmd, flags,
> +count, offset, extents);
>  }
>  else
> -  return send_structured_reply_error (request.handle, cmd, flags,
> -  error);
> +  send_structured_reply_error (request.handle, cmd, flags, error);
>}
>else
> -return send_simple_reply (request.handle, cmd, flags, buf, count,
> -  error);
> +send_simple_reply (request.handle, cmd, flags, buf, count, error);
>  }
> diff --git a/server/public.c b/server/public.c
> index 472ca623..6a9840bb 100644
> --- a/server/public.c
> +++ b/server/public.c
> @@ -727,7 +727,8 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
> */
>bool has_quit = quit;
>assert (has_quit ||
> -  (conn && conn->nworkers > 0 && connection_get_status () < 1) ||
> +  (conn && conn->nworkers > 0 &&
> +   connection_get_status () < STATUS_ACTIVE) ||
>(conn && (fds[2].revents & (POLLRDHUP | POLLHUP | POLLERR |
>POLLNVAL;
>if (has_quit)
> diff --git a/server/test-public.c b/server/test-public.c
> index 1cb759d1..1d83354f 100644
> --- a/server/test-public.c
> +++ b/server/test-public.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2018-2021 Red Hat Inc.
> + * Copyright (C) 2018-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -83,7 +83,7 @@ threadlocal_get_context (void)
>abort ();
>  }
> 
> -int
> +conn_status
>  connection_get_status (void)
>  {
>abort ();
> -- 
> 2.37.3
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

Reviewed-by: Richard W.M. Jones 

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 3/3] nbdsh: Allow -u interleaved with -c

[Richard W.M. Jones]

It's confusing having patches 1 & 3 separate.

I can't help thinking this whole series would be easier to understand
if instead of building a list of Python commands as strings, we built
a list of "instructions", where an instruction is a qualified union
which (in OCaml) would be:

type instruction =
  | Command of string  (* -c *)
  | URI of string  (* -u *)
  | OptMode(* --opt-mode *)
  etc

and then we'd run through the instructions in order at the end.  It
gets rid of the awkward quoting problems too.

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 1/3] nbdsh: Refactor handling of -u and friends

[Richard W.M. Jones]

On Wed, Oct 26, 2022 at 04:15:59PM -0500, Eric Blake wrote:
> Slightly rearrange the code so that we can reuse the -c framework for
> executing the code snippets for -u, --opt-mode, and --base-allocation.
> Slightly changes the error message when a URI is invalid (which
> requires revamping test-error.sh a bit to match), but otherwise no
> semantic change intended.
> ---
>  python/nbdsh.py  | 67 ++--
>  sh/test-error.sh | 37 ++
>  2 files changed, 52 insertions(+), 52 deletions(-)
> 
> diff --git a/python/nbdsh.py b/python/nbdsh.py
> index a98a9113..84ac84e2 100644
> --- a/python/nbdsh.py
> +++ b/python/nbdsh.py
> @@ -1,5 +1,5 @@
>  # NBD client library in userspace
> -# Copyright (C) 2013-2021 Red Hat Inc.
> +# Copyright (C) 2013-2022 Red Hat Inc.
>  #
>  # This library is free software; you can redistribute it and/or
>  # modify it under the terms of the GNU Lesser General Public
> @@ -35,12 +35,13 @@ def shell():
>   epilog=epilog)
>  short_options = []
>  long_options = []
> +shortcuts = 0
> 
>  parser.add_argument('--base-allocation', action='store_true',
>  help='request the "base:allocation" meta context')
>  long_options.append("--base-allocation")
> 
> -parser.add_argument('-c', '--command', action='append',
> +parser.add_argument('-c', '--command', action='append', default=[],
>  help="run a Python statement "
>  "(may be used multiple times)")
>  short_options.append("-c")
> @@ -98,10 +99,6 @@ def shell():
>  print("\n".join(long_options))
>  exit(0)
> 
> -# Create the banner and prompt.
> -banner = make_banner(args)
> -sys.ps1 = "nbd> "
> -
>  # If verbose, set LIBNBD_DEBUG=1
>  if args.verbose:
>  os.environ["LIBNBD_DEBUG"] = "1"
> @@ -110,42 +107,40 @@ def shell():
>  if not args.n:
>  h = nbd.NBD()
>  h.set_handle_name("nbdsh")
> +cmds = args.command
> 
>  # Set other attributes in the handle.
> +if args.uri is not None:
> +cmds.insert(0, 'h.connect_uri("%s")' % args.uri)

This allows commands to be injected if, for example, the nbdsh -u
parameter came from an untrusted source.

After going through probably hundreds of stackoverflow answer this
morning I cannot find if Python has an official way to escape this.
Both string.encode() and %r seem like potential candidates:

  >>> s = "\""
  >>> 'h.connect_uri(%s)' % s.encode('unicode-escape')
  'h.connect_uri(b\'"\')'
  >>> 'h.connect_uri(%r)' % s
  'h.connect_uri(\'"\')'

(Note the quoting displayed there is a little confusing because the
output has been quoted.)

> +shortcuts += 1
>  if args.base_allocation:
> -h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)
> +cmds.insert(0, 'h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)')
> +shortcuts += 1
>  if args.opt_mode:
> -h.set_opt_mode(True)
> +cmds.insert(0, 'h.set_opt_mode(True)')
> +shortcuts += 1
> 
> -# Parse the URI.
> -if args.uri is not None:
> -try:
> -h.connect_uri(args.uri)
> -except nbd.Error as ex:
> -print("nbdsh: unable to connect to uri '%s': %s" %
> -  (args.uri, ex.string), file=sys.stderr)
> -sys.exit(1)
> -
> -# If there are no -c or --command parameters, go interactive,
> -# otherwise we run the commands and exit.
> -if not args.command:
> -code.interact(banner=banner, local=locals(), exitmsg='')
> -else:
> -# https://stackoverflow.com/a/11754346
> -d = dict(locals(), **globals())
> -try:
> -for c in args.command:
> -if c != '-':
> -exec(c, d, d)
> -else:
> -exec(sys.stdin.read(), d, d)
> -except nbd.Error as ex:
> -if nbd.NBD().get_debug():
> -traceback.print_exc()
> +# Run all -c snippets, including any shortcuts we just added
> +# https://stackoverflow.com/a/11754346
> +d = dict(locals(), **globals())
> +try:
> +for c in args.command:
> +if c != '-':
> +exec(c, d, d)
>  else:
> -print("nbdsh: command line script failed: %s" % ex.string,
> -  file=sys.stderr)
> -sys.exit(1)
> +exec(sys.stdin.read(), d, d)
> +except nbd.Error as ex:
> +if nbd.NBD().get_debug():
> +traceback.print_exc()
> +else:
> +print("nbdsh: command line script failed: %s" % ex.string,
> +  file=sys.stderr)
> +sys.exit(1)
> +
> +# If there are no explicit -c or --command parameters, go interactive.
> +if len(args.command) - 

Re: [Libguestfs] distutils use in hivex libdnet libnbd (and more) [was: Re: Help needed triaging build failures without distutils]

[Richard W.M. Jones]

On Mon, Oct 24, 2022 at 06:00:31PM +0200, Miro Hrončok wrote:
> On 24. 10. 22 15:14, Richard W.M. Jones wrote:
> >Original code:
> >
> >   $ python3 -c 'import distutils.sysconfig; 
> > print(distutils.sysconfig.get_python_lib(1,0));'
> >   /usr/lib64/python3.11/site-packages
> >
> >Potential replacement:
> >
> >   $ python3 -c 'import sysconfig; print(sysconfig.get_path("platlib"));'
> >   /usr/local/lib64/python3.11/site-packages
> 
> That is a correct replacement. It will return the same value in
> rpmbuild environment.
> 
> >Maybe the original code was wrong, but I guess we don't want to
> >install site packages in /usr/local when running under RPM at least.
> 
> $ python3 -c 'import sysconfig; print(sysconfig.get_path("platlib"));'
> /usr/local/lib64/python3.11/site-packages
> 
> $ RPM_BUILD_ROOT=/ python3 -c 'import sysconfig;
> print(sysconfig.get_path("platlib"));'
> /usr/lib64/python3.11/site-packages

Ah ha!  Good one, thanks :-)

Here's the fix for libguestfs:
https://github.com/libguestfs/libguestfs/commit/26940f64a740676103b0ee49bf0ca5ac4e297841

I'll leave this fix to stew for a while and if it works I'll fix
various other projects in a similar way.

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] distutils use in hivex libdnet libnbd (and more) [was: Re: Help needed triaging build failures without distutils]

[Richard W.M. Jones]

[Sending 2nd email again since it didn't reach the Python-devel list first time]

On Tue, Oct 18, 2022 at 11:13:47AM +0100, Richard W.M. Jones wrote:
> On Tue, Oct 18, 2022 at 10:53:12AM +0100, Richard W.M. Jones wrote:
> >   https://github.com/libguestfs/libguestfs/blob/master/m4/guestfs-python.m4
> 
> Actually some help for how to replace it would be useful too as
> sysconfig is not an exact replacement for distutils.sysconfig.  In
> particular get_python_lib doesn't exist, and I can't find an exact
> replacement.
> 
> Original code:
> 
>   $ python3 -c 'import distutils.sysconfig; 
> print(distutils.sysconfig.get_python_lib(1,0));'
>   /usr/lib64/python3.11/site-packages
> 
> Potential replacement:
> 
>   $ python3 -c 'import sysconfig; print(sysconfig.get_path("platlib"));'
>   /usr/local/lib64/python3.11/site-packages
> 
> Maybe the original code was wrong, but I guess we don't want to
> install site packages in /usr/local when running under RPM at least.
> Note it's used here:
> 
> https://github.com/libguestfs/libguestfs/blob/master/python/Makefile.am#L50-L54
> 
> (We do not -- and cannot -- use ordinary Python install mechanisms.)
> 
> 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

-- 
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] distutils use in hivex libdnet libnbd (and more) [was: Re: Help needed triaging build failures without distutils]

[Richard W.M. Jones]

[Sending again since it didn't reach the Python-devel list first time]

On Tue, Oct 18, 2022 at 10:53:12AM +0100, Richard W.M. Jones wrote:
> On Tue, Oct 18, 2022 at 11:27:47AM +0200, Miro Hrončok wrote:
> > Hey Pythonistas.
> > 
> > The Python standard library distutils module will be removed from Python 
> > 3.12+
> > 
> > https://peps.python.org/pep-0632/
> 
> Annoying, but OK ...
> 
> > As preparatory work, we build all python packages in a Copr repository
> > with Python 3.11 sans distutils:
> > 
> > https://copr.fedorainfracloud.org/coprs/g/python/python-without-distutils/
> > 
> > I've rebuilt all the failed builds again and also in a control-group copr:
> > 
> > https://copr.fedorainfracloud.org/coprs/g/python/python-with-distutils/
> >
> > 250 packages that failed to build without distuils but succeeded
> > with distutils need to be examined and categorized into various
> > different groups:
> > 
> >  - package uses distutils only if sys.version_info < (3, 12)
> >  -- this is OK but still fails here
> >  - package uses distutils unconditionally and the package needs to be fixed
> >  - package uses another package that uses distutils unconditionally
> >   and the dependency needs to be fixed
> > 
> > I suspect most of the packages will fail to build with Python 3.12
> > (planned for Fedora 39, change proposal TBD). The python3-setutpools
> > package provides a distutils module [^1], so sometimes "simply"
> > adding BuildRequires: python3-setuptools might workaround the
> > problem.
> 
> ...
> 
> > rjones hivex libdnet libnbd
> 
> I think your testing methodology might have been wrong because we use
> distutils in other packages that I maintain, notably:
> 
>   https://github.com/libguestfs/libguestfs/blob/master/m4/guestfs-python.m4
> 
> I'm not sure what could have happened here.  The RPM would have failed
> to build if the Python bindings had been ./configure-d out.  The link
> shows libguestfs as "not build yet / Disabled":
> 
>   
> https://copr.fedorainfracloud.org/coprs/g/python/python-without-distutils/packages/?page=13
> 
> but then there's a build which succeeded:
> 
>   
> https://copr.fedorainfracloud.org/coprs/g/python/python-without-distutils/build/4906970/
> 
> and it has Python bindings.  (The logs of the build don't seem to be
> available.)
> 
> Anyway I'll see if I can fix this upstream as requested.
> 
> 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

-- 
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] [nbdkit PATCH 06/10] build: Only attempt to build vddk on Linux

[Richard W.M. Jones]

On Thu, Oct 20, 2022 at 11:09:29PM +0100, Richard W.M. Jones wrote:
> On Thu, Oct 20, 2022 at 03:32:05PM -0500, Eric Blake wrote:
> > When --enable/disable-vddk is not given to configure, our default was
> > to base on the current architecture.  But we know that we are
> > targeting a .so library built for Linux, so we can also gate things
> > based on the host OS.  And doing so means that vddk is no longer even
> > attempted on mingw, eliminating the need to explicitly enable/disable
> > it in our CI recipes.
> 
> It would theoretically be possible to get VDDK to work for the Windows
> port.  (In fact I believe someone asked for this).  However at the
> moment it's Linux only so this is fine.

https://gitlab.com/nbdkit/nbdkit/-/issues/3

Rich.

> Rich.
> 
> >  configure.ac | 6 +++---
> >  ci/build.sh  | 2 --
> >  2 files changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index d506fb51..000a7d67 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -1302,11 +1302,11 @@ AC_ARG_ENABLE([vddk],
> >   dnl newer versions which are supported only on x86-64.  Don't
> >   dnl compile on other platforms.
> >   AC_MSG_CHECKING([if the host CPU is compatible with VDDK])
> > - AS_IF([test "$host_cpu" = "x86_64"],[
> > -AC_MSG_RESULT([yes ($host_cpu)])
> > + AS_IF([test "$host_cpu" = "x86_64" && test "$host_os" = "linux-gnu"],[
> > +AC_MSG_RESULT([yes ($host)])
> >  enable_vddk=yes
> >   ],[
> > -AC_MSG_RESULT([no ($host_cpu)])
> > +AC_MSG_RESULT([no ($host)])
> >  enable_vddk=no
> >   ])
> >  ])
> > diff --git a/ci/build.sh b/ci/build.sh
> > index 7d31d5c5..241d0ef5 100755
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -122,14 +122,12 @@ main() {
> >  CONFIG_ARGS="\
> >  $CONFIG_ARGS
> >  --disable-ocaml
> > ---disable-vddk
> >  "
> >  ;;
> >  *)
> >  CONFIG_ARGS="\
> >  $CONFIG_ARGS
> >  --enable-ocaml
> > ---enable-vddk
> >  "
> >  ;;
> >  esac
> > -- 
> > 2.37.3
> > 
> > ___
> > Libguestfs mailing list
> > Libguestfs@redhat.com
> > https://listman.redhat.com/mailman/listinfo/libguestfs
> 
> -- 
> 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

-- 
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] [nbdkit PATCH 10/10] ci: Fix typo in last patch

[Richard W.M. Jones]

The series looks sensible, thanks for pushing!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH 06/10] build: Only attempt to build vddk on Linux

[Richard W.M. Jones]

On Thu, Oct 20, 2022 at 03:32:05PM -0500, Eric Blake wrote:
> When --enable/disable-vddk is not given to configure, our default was
> to base on the current architecture.  But we know that we are
> targeting a .so library built for Linux, so we can also gate things
> based on the host OS.  And doing so means that vddk is no longer even
> attempted on mingw, eliminating the need to explicitly enable/disable
> it in our CI recipes.

It would theoretically be possible to get VDDK to work for the Windows
port.  (In fact I believe someone asked for this).  However at the
moment it's Linux only so this is fine.

Rich.

>  configure.ac | 6 +++---
>  ci/build.sh  | 2 --
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index d506fb51..000a7d67 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1302,11 +1302,11 @@ AC_ARG_ENABLE([vddk],
>   dnl newer versions which are supported only on x86-64.  Don't
>   dnl compile on other platforms.
>   AC_MSG_CHECKING([if the host CPU is compatible with VDDK])
> - AS_IF([test "$host_cpu" = "x86_64"],[
> -AC_MSG_RESULT([yes ($host_cpu)])
> + AS_IF([test "$host_cpu" = "x86_64" && test "$host_os" = "linux-gnu"],[
> +AC_MSG_RESULT([yes ($host)])
>  enable_vddk=yes
>   ],[
> -AC_MSG_RESULT([no ($host_cpu)])
> +AC_MSG_RESULT([no ($host)])
>  enable_vddk=no
>   ])
>  ])
> diff --git a/ci/build.sh b/ci/build.sh
> index 7d31d5c5..241d0ef5 100755
> --- a/ci/build.sh
> +++ b/ci/build.sh
> @@ -122,14 +122,12 @@ main() {
>  CONFIG_ARGS="\
>  $CONFIG_ARGS
>  --disable-ocaml
> ---disable-vddk
>  "
>  ;;
>  *)
>  CONFIG_ARGS="\
>  $CONFIG_ARGS
>  --enable-ocaml
> ---enable-vddk
>  "
>  ;;
>  esac
> -- 
> 2.37.3
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

-- 
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] Why cant't use the pie link option when compiling libgustfs

[Richard W.M. Jones]

On Fri, Oct 21, 2022 at 12:10:43AM +0800, guiHua wrote:
> Hello  
>When I compile libgustfs, it will throw exception: undefined reference to
> 'caml_local_roots' when adding pie link option. 
> Why is that? The command is like this:
> ./configure CFLAGS="-O2 -fPIC" LDFLAGS="-pie -Wl".

Is that LDFLAGS actually valid?  You might try putting -pie into
CFLAGS, and I don't know if -Wl has any effect at all.

> I look forward to hearing from you.

I'm not certain why, but I can tell you that caml_local_roots is a
symbol in the OCaml runtime:

$ cat > hello.ml
print_endline "hello, world"
$ ocamlopt hello.ml -o hello
$ ./hello
hello, world
$ nm hello | grep caml_local_roots
0024 a domain_field_caml_local_roots

It's actually defined here:

https://github.com/ocaml/ocaml/blob/trunk/runtime/caml/memory.h#L238

(The "domain_field_" prefix is something to do with multicore, it used
to be a global variable, but now is stored per-thread.)

What distro is this?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] distutils use in hivex libdnet libnbd (and more) [was: Re: Help needed triaging build failures without distutils]

[Richard W.M. Jones]

On Tue, Oct 18, 2022 at 10:53:12AM +0100, Richard W.M. Jones wrote:
>   https://github.com/libguestfs/libguestfs/blob/master/m4/guestfs-python.m4

Actually some help for how to replace it would be useful too as
sysconfig is not an exact replacement for distutils.sysconfig.  In
particular get_python_lib doesn't exist, and I can't find an exact
replacement.

Original code:

  $ python3 -c 'import distutils.sysconfig; 
print(distutils.sysconfig.get_python_lib(1,0));'
  /usr/lib64/python3.11/site-packages

Potential replacement:

  $ python3 -c 'import sysconfig; print(sysconfig.get_path("platlib"));'
  /usr/local/lib64/python3.11/site-packages

Maybe the original code was wrong, but I guess we don't want to
install site packages in /usr/local when running under RPM at least.
Note it's used here:

https://github.com/libguestfs/libguestfs/blob/master/python/Makefile.am#L50-L54

(We do not -- and cannot -- use ordinary Python install mechanisms.)

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

[Libguestfs] distutils use in hivex libdnet libnbd (and more) [was: Re: Help needed triaging build failures without distutils]

[Richard W.M. Jones]

On Tue, Oct 18, 2022 at 11:27:47AM +0200, Miro Hrončok wrote:
> Hey Pythonistas.
> 
> The Python standard library distutils module will be removed from Python 3.12+
> 
> https://peps.python.org/pep-0632/

Annoying, but OK ...

> As preparatory work, we build all python packages in a Copr repository
> with Python 3.11 sans distutils:
> 
> https://copr.fedorainfracloud.org/coprs/g/python/python-without-distutils/
> 
> I've rebuilt all the failed builds again and also in a control-group copr:
> 
> https://copr.fedorainfracloud.org/coprs/g/python/python-with-distutils/
>
> 250 packages that failed to build without distuils but succeeded
> with distutils need to be examined and categorized into various
> different groups:
> 
>  - package uses distutils only if sys.version_info < (3, 12)
>  -- this is OK but still fails here
>  - package uses distutils unconditionally and the package needs to be fixed
>  - package uses another package that uses distutils unconditionally
>   and the dependency needs to be fixed
> 
> I suspect most of the packages will fail to build with Python 3.12
> (planned for Fedora 39, change proposal TBD). The python3-setutpools
> package provides a distutils module [^1], so sometimes "simply"
> adding BuildRequires: python3-setuptools might workaround the
> problem.

...

> rjones hivex libdnet libnbd

I think your testing methodology might have been wrong because we use
distutils in other packages that I maintain, notably:

  https://github.com/libguestfs/libguestfs/blob/master/m4/guestfs-python.m4

I'm not sure what could have happened here.  The RPM would have failed
to build if the Python bindings had been ./configure-d out.  The link
shows libguestfs as "not build yet / Disabled":

  
https://copr.fedorainfracloud.org/coprs/g/python/python-without-distutils/packages/?page=13

but then there's a build which succeeded:

  
https://copr.fedorainfracloud.org/coprs/g/python/python-without-distutils/build/4906970/

and it has Python bindings.  (The logs of the build don't seem to be
available.)

Anyway I'll see if I can fix this upstream as requested.

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] [nbdkit PATCH] rust: prevent dead_code warning

[Richard W.M. Jones]

Thanks - I'll push Thomas's patch shortly.  We can always revert it
later when the updated mockall package is released.

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] [nbdkit PATCH] rust: prevent dead_code warning

[Richard W.M. Jones]

On Fri, Oct 14, 2022 at 10:59:30PM +0200, Thomas Weißschuh wrote:
> On 2022-10-14 21:54+0100, Richard W.M. Jones wrote:
> > On Fri, Oct 14, 2022 at 10:42:30PM +0200, Thomas Weißschuh wrote:
> > > rustc 1.64.0 generates warnings for the mocked Server.
> > > This leads to a failure of test.sh.
> > > 
> > > ```
> > > warning: associated function `expect` is never used
> > >   --> tests/common/mod.rs:49:1
> > >|
> > > 49 | / mock!{
> > > 50 | | pub Server {}
> > > 51 | | impl Server for Server {
> > > 52 | | fn cache(, count: u32, offset: u64) -> Result<()>;
> > > ...  |
> > > 86 | | }
> > > 87 | | }
> > >| |_^
> > >|
> > >= note: `#[warn(dead_code)]` on by default
> > > ```
> > > ---
> > > 
> > > Note: This also affects the maintenance branches.
> > > 
> > >  plugins/rust/tests/common/mod.rs | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/plugins/rust/tests/common/mod.rs 
> > > b/plugins/rust/tests/common/mod.rs
> > > index 61c30464..de26c89f 100644
> > > --- a/plugins/rust/tests/common/mod.rs
> > > +++ b/plugins/rust/tests/common/mod.rs
> > > @@ -48,6 +48,7 @@ lazy_static! {
> > >  
> > >  mock!{
> > >  pub Server {}
> > > +#[allow(dead_code)]
> > >  impl Server for Server {
> > >  fn cache(, count: u32, offset: u64) -> Result<()>;
> > >  fn can_cache() -> Result;
> > > 
> > > base-commit: 20c2dc98b6bbde2f92e63d500d5e6015184bb105
> > 
> > Yes this has been bugging me as well, and probably we should put this
> > patch upstream.
> > 
> > However I will note that Alan submitted an issue about the underlying
> > issue:
> > 
> > https://listman.redhat.com/archives/libguestfs/2022-September/030074.html
> > https://github.com/asomers/mockall/issues/414
> > 
> > It's marked as fixed, but for some reason that didn't seem to fix the
> > tests - I'm still seeing the unused 'expect' here, even after
> > completely deleting the cargo cache.
> 
> I think the fixed version of mockall is just not released yet.
> 
> What is your guidance for packagers?
> Especially as this is also affecting the 1.32 branch.
> For now I am carrying the patch from the parent mail downstream.

I agree we should take your patch, just wanted to see if Alan had an
opinion on this first since he's maintaining the rust plugin.

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] [nbdkit PATCH] rust: prevent dead_code warning

[Richard W.M. Jones]

On Fri, Oct 14, 2022 at 10:42:30PM +0200, Thomas Weißschuh wrote:
> rustc 1.64.0 generates warnings for the mocked Server.
> This leads to a failure of test.sh.
> 
> ```
> warning: associated function `expect` is never used
>   --> tests/common/mod.rs:49:1
>|
> 49 | / mock!{
> 50 | | pub Server {}
> 51 | | impl Server for Server {
> 52 | | fn cache(, count: u32, offset: u64) -> Result<()>;
> ...  |
> 86 | | }
> 87 | | }
>| |_^
>|
>= note: `#[warn(dead_code)]` on by default
> ```
> ---
> 
> Note: This also affects the maintenance branches.
> 
>  plugins/rust/tests/common/mod.rs | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/plugins/rust/tests/common/mod.rs 
> b/plugins/rust/tests/common/mod.rs
> index 61c30464..de26c89f 100644
> --- a/plugins/rust/tests/common/mod.rs
> +++ b/plugins/rust/tests/common/mod.rs
> @@ -48,6 +48,7 @@ lazy_static! {
>  
>  mock!{
>  pub Server {}
> +#[allow(dead_code)]
>  impl Server for Server {
>  fn cache(, count: u32, offset: u64) -> Result<()>;
>  fn can_cache() -> Result;
> 
> base-commit: 20c2dc98b6bbde2f92e63d500d5e6015184bb105

Yes this has been bugging me as well, and probably we should put this
patch upstream.

However I will note that Alan submitted an issue about the underlying
issue:

https://listman.redhat.com/archives/libguestfs/2022-September/030074.html
https://github.com/asomers/mockall/issues/414

It's marked as fixed, but for some reason that didn't seem to fix the
tests - I'm still seeing the unused 'expect' here, even after
completely deleting the cargo cache.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] bug in gnutls_init()

[Richard W.M. Jones]

On Thu, Oct 13, 2022 at 02:04:34PM -0500, Eric Blake wrote:
> I just fixed a bug in nbdkit for incorrectly calling
> free(gnutls_session_t) after gnutls_init(, ...) fails:
> https://gitlab.com/nbdkit/nbdkit/-/commit/40faf3dfb20c06b9c5faa0a122607e3ae7c6202a
> 
> But in the process, I was browsing the source code to gnutls_init() to
> see why Coverity wasn't flagging free(opaque_type) as fishy, and found
> that there is a nasty lurking bug:
> 
> int gnutls_init(gnutls_session_t * session, unsigned int flags)
> {
>   int ret;
> 
>   FAIL_IF_LIB_ERROR;
> 
>   *session = gnutls_calloc(1, sizeof(struct gnutls_session_int));
> 
> Note that *session is left uninitialized if FAIL_IF_LIB_ERROR; causes
> an early return GNUTLS_E_LIB_IN_ERROR_STATE.  If a caller (properly)
> treats gnutls_session_t as an opaque type, and does not try to
> zero-initialize it (as there is no way to know that 0 is a safe value
> for an opaque type), then writing:
> 
>   gnutls_session_t session;
>   int err = gnutls_init (, GNUTLS_SERVER);
>   if (err < 0)
> gnutls_deinit (session);
> 
> is a bug waiting to happen, because it WILL cause gnutls_deinit() to
> attempt to dereference an uninitialized pointer if session remains
> uninitialized because of an earlier library error.

Thanks for fixing this - I will do some backports to the stable
branches later since this seems like an important fix.

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 | Failed pipeline for master | 018d55a8

[Richard W.M. Jones]

On Wed, Oct 12, 2022 at 02:00:21PM -0500, Eric Blake wrote:
> > Job #3163966643 ( https://gitlab.com/nbdkit/libnbd/-/jobs/3163966643/raw )
> > 
> > Stage: builds
> > Name: x86_64-opensuse-leap-153-prebuilt-env
> 
> This one is still failing because of a bug in gnutls; the log is
> reporting:
> 
> libnbd: debug: nbd1: nbd_connect_command: transition: 
> NEWSTYLE.OPT_STARTTLS.RECV_REPLY_PAYLOAD -> NEWSTYLE.OPT_STARTTLS.CHECK_REPLY
> free(): invalid pointer
> libnbd: debug: nbd1: nbd_connect_command: transition: 
> NEWSTYLE.OPT_STARTTLS.CHECK_REPLY -> NEWSTYLE.OPT_STARTTLS.TLS_HANDSHAKE_READ
> libnbd: debug: nbd1: nbd_connect_command: transition: 
> NEWSTYLE.OPT_STARTTLS.TLS_HANDSHAKE_READ -> DEAD
> libnbd: debug: nbd1: nbd_connect_command: leave: error="nbd_connect_command: 
> gnutls_handshake: Error in the pull function. (-1/1)"
> 
> That libc message about invalid free() is scary; I'm not yet sure
> whether it is a bug in opensuse-leap's gnutls package or something
> we're doing wrong in libnbd.

I had a look into this.  Unfortunately I only have OpenSUSE Tumbleweed
available.  It doesn't fail for me in Tumbleweed.  (It also doesn't
fail in the CI pipeline for Tumbleweed.)

So I guess this problem is somehow specific to nbdkit or gnutls in
OpenSUSE 15.3.

We can probably ignore this failure, under the assumption it is fixed
upstream.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH 2/2] info: Add --is tls, --can structured-reply

[Richard W.M. Jones]

Looks good, ACK series.

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] redhat-based EL9 distros not supported?

[Richard W.M. Jones]

The fix is upstream in 8858fc63e6 and will be in CentOS Stream soon.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] redhat-based EL9 distros not supported?

[Richard W.M. Jones]

On Mon, Oct 10, 2022 at 08:51:00AM +, Harry Bryson wrote:
> >> [Rocky9]$ virt-customize -a disk.qcow2 --firstboot-command /tmp/bootstrap
> >> [ 0.0] Examining the guest ...
> >> [ 17.1] Setting a random seed
> >> [ 17.3] Installing firstboot command: /tmp/bootstrap
> >> virt-customize: error: guest type rocky is not supported
> 
> >This is just a bug. For reference what versions of libguestfs &
> >guestfs-tools are you using?
> 
> Just the native EL9 versions
> $ rpm -q libguestfs guestfs-tools
> libguestfs-1.46.1-4.el9_0.alma.x86_64
> guestfs-tools-1.46.1-6.el9.0.1.x86_64
> (I am running on AlmaLinux9/Rocky9 as well)

What does Alma (as a guest image) return for inspect-get-distro?  As
far as I can tell we don't really support it in libguestfs so far.

Anyway the attached patches should fix the issue for Rocky guest
images, although it's not really long-term supportable to keep adding
distros like this, we'll have to think of a better way.

The bug is:
https://bugzilla.redhat.com/show_bug.cgi?id=2133443

> Not sure if this is the correct fix, but I test patched the distro matching 
> and it seemed to fix this.
> (Although virt-builder did not link cleanly - fPIE ?)

You probably need to configure with:

  ./configure CFLAGS="-O2 -fPIC -g"

(or alternatively just rebuild from the CentOS SRPM)

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
>From 85f3e4d084b71c4fac84d729f14c798001df2076 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" 
Date: Mon, 10 Oct 2022 13:59:00 +0100
Subject: [PATCH] common: customize: Support Rocky Linux

Reported-by: Harry Benson
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2133443
---
 mlcustomize/firstboot.ml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mlcustomize/firstboot.ml b/mlcustomize/firstboot.ml
index 5c7fd0d..0c76283 100644
--- a/mlcustomize/firstboot.ml
+++ b/mlcustomize/firstboot.ml
@@ -151,7 +151,8 @@ WantedBy=%s
 
   and install_sysvinit_service g root distro major =
 match distro with
-| "fedora"|"rhel"|"centos"|"scientificlinux"|"oraclelinux"|"redhat-based" 
->
+| "fedora"|"rhel"|"centos"|"scientificlinux"|"oraclelinux"|"rocky"|
+    "redhat-based" ->
   install_sysvinit_redhat g
 | "opensuse"|"sles"|"suse-based" ->
   install_sysvinit_suse g
-- 
2.37.0.rc2

>From ea0142b9906ee5844f58b13b72eb6150ee020b6a Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" 
Date: Mon, 10 Oct 2022 13:54:52 +0100
Subject: [PATCH] customize: Support Rocky Linux

Reported-by: Harry Benson
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2133443
---
 customize/hostname.ml| 3 ++-
 customize/password.ml| 3 ++-
 customize/random_seed.ml | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/customize/hostname.ml b/customize/hostname.ml
index df64a2dab4..fabba3cfd7 100644
--- a/customize/hostname.ml
+++ b/customize/hostname.ml
@@ -36,7 +36,8 @@ let rec set_hostname (g : Guestfs.guestfs) root hostname =
 update_etc_machine_info g hostname;
 true
 
-  | "linux", ("rhel"|"centos"|"scientificlinux"|"oraclelinux"|"redhat-based"), 
v
+  | "linux", ("rhel"|"centos"|"scientificlinux"|"oraclelinux"|"rocky"|
+  "redhat-based"), v
 when v >= 7 ->
 update_etc_hostname g hostname;
 update_etc_machine_info g hostname;
diff --git a/customize/password.ml b/customize/password.ml
index 608bf95dcf..b37b31fcdc 100644
--- a/customize/password.ml
+++ b/customize/password.ml
@@ -160,7 +160,8 @@ and default_crypto g root =
   let distro = g#inspect_get_distro root in
   let major = g#inspect_get_major_version root in
   match distro, major with
-  | ("rhel"|"centos"|"scientificlinux"|"oraclelinux"|"redhat-based"), v when v 
>= 9 ->
+  | ("rhel"|"centos"|"scientificlinux"|"oraclelinux"|"rocky"|
+ "redhat-based"), v when v >= 9 ->
 `YESCRYPT
   | ("rhel"|"centos"|"scientificlinux"|"oraclelinux"|"redhat-based"), v when v 
>= 6 ->
 `SHA512
diff --git a/customize/random_seed.ml b/customize/random_seed.ml
index f32d3194ea..2dcb700eaa 100644
--- a/customize/random_seed.ml
+++ b/customize/random_seed.ml
@@ -47,7 +47,8 @@ let rec set_random_seed (g : Guestfs.guestfs) root =
 let distro = g#inspect_get_distro root in
 let file =
   match typ, distro with
-  | "linux", 
("fedora"|"rhel"|"centos"|"scientificlinux"|"oraclelinux"|"redhat-based") ->
+  | "linux", ("fedora"|"rhel"|"centos"|"scientificlinux"|"oraclelinux"|
+  "rocky"|"redhat-based") ->
 Some "/var/lib/random-seed"
   | "linux", ("debian"|"ubuntu"|"kalilinux") ->
 Some "/var/lib/urandom/random-seed"
-- 
2.37.0.rc2

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

Re: [Libguestfs] [libnbd PATCH] RFC: fuzzing: Break up handshake into more steps

[Richard W.M. Jones]

FYI I restarted the fuzzing at commit c5a4042640 which includes this
patch.

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] redhat-based EL9 distros not supported?

[Richard W.M. Jones]

On Fri, Oct 07, 2022 at 04:33:01PM +, Harry Bryson wrote:
> Prior to Enterprise Linux 9, it seems some Red Hat based distros returned 
> "redhat-based" for inspect-get-distro
> This allowed things like "virt-customize --firstboot-command" to work as this 
> matched the supported distro list, e.g.
> 
> | "fedora"|"rhel"|"centos"|"scientificlinux"|"oraclelinux"|"redhat-based"
> 
> However, it seems some distros, e.g. Rocky9 and AlmaLinux9 are now returning 
> "rocky"/"almalinux" for inspect-get-distro
> When using virt-install and then virt-customize, I am seeing issues like the 
> following:
> 
> [Rocky9]$ virt-customize -a disk.qcow2 --firstboot-command /tmp/bootstrap
> [   0.0] Examining the guest ...
> [  17.1] Setting a random seed
> [  17.3] Installing firstboot command: /tmp/bootstrap
> virt-customize: error: guest type rocky is not supported

This is just a bug.  For reference what versions of libguestfs &
guestfs-tools are you using?

> Is there some way to return "redhat-based" again for inspect-get-distro or 
> does the distro list matching need to be updated?
> I possibly updated the OS database (osinfo-query os) to support newer OS - is 
> this related to this problem?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH] RFC: fuzzing: Break up handshake into more steps

[Richard W.M. Jones]

On Fri, Oct 07, 2022 at 07:37:19AM -0500, Eric Blake wrote:
> On Fri, Oct 07, 2022 at 10:22:57AM +0100, Richard W.M. Jones wrote:
> > On Thu, Oct 06, 2022 at 04:34:52PM -0500, Eric Blake wrote:
> > > Give the fuzzer a few more points to experiment with added branching
> > > by explicitly using opt mode.
> > > ---
> > > 
> > > I'm not quite sure whether the fuzzer is able to synthesize specific
> > > API calls from the client side; but if it can, letting the client
> > > specifically enter the NEGOTIATING state may allow the fuzzer to spot
> > > other nbd_opt_* API call chains that could provoke odd interactions,
> > > which would be completely missed when sticking with the default of
> > > skipping opt mode.
> > 
> > It's essentially looking for new paths through the code.  If the
> > change allows new libnbd paths to be explored then it will be
> > beneficial to fuzzing, if not then it'll make no difference.  I have
> > no objection to trying the patch anyway, so ACK.
> 
> Ok, in as 8592caba
> 
> Thinking about ways to expose even more code-paths, I wonder if we
> could tweak the client along the lines of:
> 
> if (rand () & 1)
>   nbd_set_handshake_flags (nbd, rand ());
> if (rand () & 1)
>   nbd_set_strict_mode (nbd, rand ());

Adding randomization to the fuzzer is a bad idea I'm afraid,
specifically called out in the docs:

https://aflplus.plus/docs/faq/ (search for "Stability")

> and so forth, to allow the fuzzer to explore different combinations of
> settings.

The fuzzer will explore different paths by presenting different
inputs.  In the case of libnbd, "input" means the network data that
normally libnbd would be reading from the NBD server.  As long as
variations in those replies (inputs) can cause libnbd to take
different paths then the fuzzer will eventually explore those paths.

> Another idea might be:
> 
> static void do_opt_structured_reply (void)
> { /* call nbd_opt_structured_reply() */ }
> static void do_opt_list_meta_context (void)
> { /* call nbd_opt_list_meta_context[_queries]() */ }
> ...
> void (*opts[])(void) = {
>   do_opt_structured_reply,
>   do_opt_list_meta_context,
>   ...
> };
> 
> for (i = rand () % 20; i > 0; i--)
>   opts[i % ARRAY_SIZE (opts)] ();
> 
> to play with different handshake sequences.

This won't work for the same reason.

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] RFC: fuzzing: Break up handshake into more steps

[Richard W.M. Jones]

On Thu, Oct 06, 2022 at 04:34:52PM -0500, Eric Blake wrote:
> Give the fuzzer a few more points to experiment with added branching
> by explicitly using opt mode.
> ---
> 
> I'm not quite sure whether the fuzzer is able to synthesize specific
> API calls from the client side; but if it can, letting the client
> specifically enter the NEGOTIATING state may allow the fuzzer to spot
> other nbd_opt_* API call chains that could provoke odd interactions,
> which would be completely missed when sticking with the default of
> skipping opt mode.

It's essentially looking for new paths through the code.  If the
change allows new libnbd paths to be explored then it will be
beneficial to fuzzing, if not then it'll make no difference.  I have
no objection to trying the patch anyway, so ACK.

Rich.

>  fuzzing/libnbd-fuzz-wrapper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c
> index 7e390558..e7cf7fe9 100644
> --- a/fuzzing/libnbd-fuzz-wrapper.c
> +++ b/fuzzing/libnbd-fuzz-wrapper.c
> @@ -200,7 +200,10 @@ client (int sock)
>nbd_add_meta_context (nbd, LIBNBD_CONTEXT_BASE_ALLOCATION);
> 
>/* This tests the handshake phase. */
> +  nbd_set_opt_mode (nbd, true);
>nbd_connect_socket (nbd, sock);
> +  nbd_opt_info (nbd);
> +  nbd_opt_go (nbd);
> 
>length = nbd_get_size (nbd);
> 
> -- 
> 2.37.3
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

-- 
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] Parameter 'encrypt.key-secret' is required for cipher for virt-ls

[Richard W.M. Jones]

On Thu, Oct 06, 2022 at 12:16:36PM +0200, Laszlo Ersek wrote:
> On 10/05/22 16:06, Do Re wrote:
> > Hello all,
> > 
> > background: One of my VM stopped working - on startup, I get the message
> > "No bootable device" in the virtual machine.
> > 
> > I would like to inspect the corresponding image with libguestfs-tools.
> > 
> > However, I don't know how to pass the encryption key to the tool. For
> > example:
> > 
> > virt-ls -a /opt/vm_witherror/machine1.qcow2 /
> > libguestfs: trace: set_verbose true
> > libguestfs: trace: set_verbose = 0
> > libguestfs: create: flags = 0, handle = 0x5645c40d6b00, program = virt-ls
> > libguestfs: trace: add_drive "/opt/vm_witherror/machine1.qcow2"
> > "readonly:true"
> > libguestfs: creating COW overlay to protect original drive content
> > libguestfs: trace: get_tmpdir
> > libguestfs: trace: get_tmpdir = "/tmp"
> > libguestfs: trace: disk_create "/tmp/libguestfsxECmri/overlay1.qcow2"
> > "qcow2" -1 "backingfile:/opt/vm_witherror/machine1.qcow2"
> > libguestfs: command: run: qemu-img
> > libguestfs: command: run: \ create
> > libguestfs: command: run: \ -f qcow2
> > libguestfs: command: run: \ -o backing_file=/opt/vm_witherror/machine1.qcow2
> > libguestfs: command: run: \ /tmp/libguestfsxECmri/overlay1.qcow2
> > qemu-img: /tmp/libguestfsxECmri/overlay1.qcow2: Parameter
> > 'encrypt.key-secret' is required for cipher
> > Could not open backing image.
> > libguestfs: error: qemu-img: /tmp/libguestfsxECmri/overlay1.qcow2:
> > qemu-img exited with error status 1, see debug messages above
> > libguestfs: trace: disk_create = -1 (error)
> > libguestfs: trace: add_drive = -1 (error)
> > libguestfs: trace: close
> > libguestfs: closing guestfs handle 0x5645c40d6b00 (state 0)
> > libguestfs: command: run: rm
> > libguestfs: command: run: \ -rf /tmp/libguestfsxECmri
> > 
> > 
> > Could you please provide an example on how to get such an encrypted disk
> > mounted?
> 
> My understanding is that libguestfs (and guestfs-tools) do not currently
> support the kind of encrypted disk where the encryption is implemented
> by QEMU, at the qcow2 layer.

I think the only sane way to do this at the moment is to open the
image first using qemu-nbd and then connect libguestfs to the NBD
socket.  In other words something like this:

$ qemu-nbd --object secret,id=sec0,data=secretpassword \
   --image-opts 
driver=qcow2,file.filename=machine1.qcow2,encrypt.format=luks,encrypt.key-secret=sec0
 \
   -t -k /tmp/socket &
$ guestfish --format=raw -a 'nbd+unix:///?socket=/tmp/socket' -i

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] api: Add nbd_[aio_]opt_starttls

[Richard W.M. Jones]

On Tue, Oct 04, 2022 at 07:51:18AM -0500, Eric Blake wrote:
> Very similar to the recent addition of nbd_opt_structured_reply, with
> the new nbd_opt_starttls() finally giving us fine-grained control over
> NBD_OPT_STARTTLS, and allowing productive handshaking with a server in
> SELECTIVETLS mode.
> 
> With this patch, it is now easy to reproduce the scenario of nbdkit's
> CVE-2021-3716, where a plaintext meddler-in-the-middle attacker could
> cause client denial of service when a --tls=on server failed to reset
> state after NBD_OPT_STARTTLS.  This also exposed the fact that nbdkit
> was not gracefully handling NBD_OPT_INFO from a plaintext client
> connecting to a --tls=require server; the testsuite is skipped unless
> using a fixed nbdkit (v1.33.2 or later).

I guess so.  It does seem complicated and a rather niche use case.
Perhaps we should just document the new API rather than bothering to
update the existing API call documentation, since almost certainly no
one using those APIs would ever need to think about this API?

Rich.

> ---
>  generator/API.ml |  98 +++--
>  generator/states-newstyle-opt-starttls.c |  40 --
>  generator/states-newstyle.c  |   3 +
>  lib/opt.c|  50 +++
>  tests/Makefile.am|   5 +
>  tests/opt-starttls.c | 166 +++
>  .gitignore   |   1 +
>  7 files changed, 343 insertions(+), 20 deletions(-)
>  create mode 100644 tests/opt-starttls.c
> 
> diff --git a/generator/API.ml b/generator/API.ml
> index 69849102..8301ec9f 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -599,7 +599,11 @@   "set_tls", {
>  =item C
> 
>  Disable TLS.  (The default setting, unless using L with
> -a URI that requires TLS)
> +a URI that requires TLS).
> +
> +This setting is also useful during integration testing when using
> +L and L for manual
> +control over when to request TLS negotiation.
> 
>  =item C
> 
> @@ -632,7 +636,8 @@   "set_tls", {
>  test whether this is the case with L.";
>  example = Some "examples/encryption.c";
>  see_also = [SectionLink "ENCRYPTION AND AUTHENTICATION";
> -Link "get_tls"; Link "get_tls_negotiated"];
> +Link "get_tls"; Link "get_tls_negotiated";
> +Link "opt_starttls"];
>};
> 
>"get_tls", {
> @@ -657,7 +662,7 @@   "get_tls_negotiated", {
>  After connecting you may call this to find out if the
>  connection is using TLS.
> 
> -This is only really useful if you set the TLS request mode
> +This is normally useful only if you set the TLS request mode
>  to C (see L), because in this
>  mode we try to use TLS but fall back to unencrypted if it was
>  not available.  This function will tell you if TLS was
> @@ -665,8 +670,14 @@   "get_tls_negotiated", {
> 
>  In C mode (the most secure) the connection
>  would have failed if TLS could not be negotiated, and in
> -C mode TLS is not tried.";
> -see_also = [Link "set_tls"; Link "get_tls"];
> +C mode TLS is not tried automatically.
> +
> +However, when doing manual integration testing of server
> +behavior, when you use L along with TLS
> +request mode C before triggering the TLS
> +handshake with L, then this will report
> +the result of that manual effort.";
> +see_also = [Link "set_tls"; Link "get_tls"; Link "opt_starttls"];
>};
> 
>"set_tls_certificates", {
> @@ -1092,11 +1103,12 @@   "set_opt_mode", {
>  newstyle server.  This setting has no effect when connecting to an
>  oldstyle server.
> 
> -Note that libnbd defaults to attempting
> +Note that libnbd defaults to attempting C and
>  C before letting you control remaining
> -negotiation steps; if you need control over this step as well,
> -first set L to false before
> -starting the connection attempt.
> +negotiation steps; if you need control over these steps as well,
> +first set L to C and
> +L to false before starting
> +the connection attempt.
> 
>  When option mode is enabled, you have fine-grained control over which
>  options are negotiated, compared to the default of the server
> @@ -1110,9 +1122,9 @@   "set_opt_mode", {
>  see_also = [Link "get_opt_mode"; Link "aio_is_negotiating";
>  Link "opt_abort"; Link "opt_go"; Link "opt_list";
>  Link "opt_info"; Link "opt_list_meta_context";
> -Link "opt_set_meta_context";
> +Link "opt_set_meta_context"; Link "opt_starttls";
>  Link "opt_structured_reply";
> -Link "set_request_structured_replies";
> +Link "set_tls"; Link "set_request_structured_replies";
>  Link "aio_connect"];
>};
> 
> @@ -1166,6 +1178,44 @@   "opt_abort", {
>  see_also = [Link "set_opt_mode"; Link "aio_opt_abort"; Link "opt_go"];
>};
> 
> +  "opt_starttls", {
> +default_call with
> +args = []; ret = RBool;
> +permitted_states = [ 

Re: [Libguestfs] Image Mounting Issue

[Richard W.M. Jones]

On Sat, Oct 01, 2022 at 02:07:30AM +, Bryce Zimmer wrote:
> See the attached output of libguestfs-test-tool.
> 
> I’m trying to mount an image to eve-ng VM using their instructions and ran 
> into
> an error.
> 
>  
> 
> These are the instructions I was following.
> 
> https://www.eve-ng.net/index.php/documentation/howtos/2822-2/
> 
>  
> 
> Please let me know if you have any questions or if I can help in any way.

qemu-system-x86_64: error: failed to set MSR 0x48f to 0x7f00036dfb
qemu-system-x86_64: /build/qemu-aXkhnW/qemu-4.2/target/i386/kvm.c:2691: 
kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.


It's a bug in qemu.  I'm not sure exactly which qemu bug but if you
search for "error: failed to set MSR 0x48f" you'll find a few
references which seem to be related to nested virt:

https://bugs.launchpad.net/qemu/+bug/1879646

https://communities.vmware.com/t5/ESXi-Discussions/ESXI-QEMU-Nested-Virtualization/td-p/2885260

You can work around it (although it'll be slow) using:

https://libguestfs.org/guestfs.3.html#force_tcg

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] RFC: generator: Mark APIs with allocator/deallocator semantics

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 10:42:01AM -0500, Eric Blake wrote:
> Modern GCC has two related attributes for functions returning a
> pointer:
> 
> __attribute__((__malloc__)) - this function returns a new pointer, not
> aliased to any existing pointer
> 
> __attribute__((__malloc__(fn,1))) - call fn(return_value) to avoid
> leaking memory allocated by this function
> 
> With those attributes, static analyzers can better detect when we pass
> the resulting pointer to the wrong deallocator, deallocate more than
> once, have a use after free, or otherwise leak the memory.  (Sadly, as
> of gcc 12.2.1, -fanalyzer still has a LOT of false positives, such as:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107100; since our code
> base triggers some of these, we can't quite rely on it yet).
> ---
>  lib/internal.h |  4 +++-
>  generator/C.ml | 24 +---
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index 0e17fbea..22f500b4 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -508,8 +508,10 @@ extern void nbd_internal_fork_safe_perror (const char *s)
>  extern char *nbd_internal_printable_buffer (const void *buf, size_t count)
>LIBNBD_ATTRIBUTE_NONNULL(1);
>  extern char *nbd_internal_printable_string (const char *str)
> +  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free)
>LIBNBD_ATTRIBUTE_NONNULL(1);
> -extern char *nbd_internal_printable_string_list (char **list);
> +extern char *nbd_internal_printable_string_list (char **list)
> +  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free);
> 
>  /* These are wrappers around socket(2) and socketpair(2).  They
>   * always set SOCK_CLOEXEC.  nbd_internal_socket can set SOCK_NONBLOCK
> diff --git a/generator/C.ml b/generator/C.ml
> index c74fae77..cfa2964c 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -244,6 +244,16 @@ let
>pr "extern ";
>print_call ?wrap ?closure_style name args optargs ret;
> 
> +  (* For RString, note that the caller must free() the argument.
> +   * Since this is used in a public header, we must use gcc's spelling
> +   * of __builtin_free in case bare free is defined as a macro.
> +   *)
> +  (match ret with
> +   | RString ->
> +  pr "\nLIBNBD_ATTRIBUTE_ALLOC_DEALLOC(__builtin_free)";
> +   | _ -> ()
> +  );
> +
>(* Output __attribute__((nonnull)) for the function parameters:
> * eg. struct nbd_handle *, int, char *
> * => [ true, false, true ]
> @@ -403,6 +413,13 @@ let
>pr "#endif\n";
>pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
>pr "\n";
> +  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 12 /* gcc >= 12.0 
> */\n";
> +  pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn) \\\n";
> +  pr "  __attribute__((__malloc__, __malloc__(fn, 1)))\n";
> +  pr "#else\n";
> +  pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn)\n";
> +  pr "#endif\n";
> +  pr "\n";
>pr "struct nbd_handle;\n";
>pr "\n";
>List.iter (
> @@ -433,12 +450,13 @@ let
>pr "#define %-40s %d\n" n i
>) constants;
>pr "\n";
> -  pr "extern struct nbd_handle *nbd_create (void);\n";
> -  pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
> -  pr "\n";
>pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL */\n";
>pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
>pr "\n";
> +  pr "extern struct nbd_handle *nbd_create (void)\n";
> +  pr "LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(nbd_close);\n";
> +  pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
> +  pr "\n";
>pr "extern const char *nbd_get_error (void);\n";
>pr "#define LIBNBD_HAVE_NBD_GET_ERROR 1\n";
>pr "\n";

ACK - worth a go, if it causes too many problems we can always
back it out later!

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] [p2v PATCH 2/2] dependencies.m4: add librsvg2

[Richard W.M. Jones]

For the series:

Reviewed-by: Richard W.M. Jones 

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 03:57:15PM +0200, Laszlo Ersek wrote:
> This is the same terrible "push" (or "registration") model (rather than
> the "pull") model that plagues systemd: if a kernel module is missing
> from the initrd that's needed for driving a device or a filesystem, the
> boot gets stuck without any indication of what *should* happen (i.e.
> what dependency is not being satisfied). There is no technical
> dependency whatsoever, it's just that an event does not occur that
> "might" otherwise "cause progress" from a global perspective.
> 
> The exact same silly pattern exists in UEFI, with PPI and protocol GUID
> dependencies. Very powerful as long as you are developing a particular
> module. Once you forget about it, *and* a particular protocol GUID is
> provided outside of your current repository (so you can't simply grep
> for it), heaven help you resolve the stuck boot (as, of course, it is
> not easy to all to get the set of those protocol GUIDs that *might* get
> the DXE dispatcher unstuck via their registration in the protocol database).
> 
> How is it better that the librsvg2 package, which is a *low level
> package*, includes a *semantic dependency* (basically: *knowledge*) of
> one of its high-profile comsumers (namely, gdk-pixbuf) -- a high level
> package --, and not the other way around? Since when is it a good idea
> to encode reverse dependencies in packages? The mind boggles.
> 
> All of this registration stuff is for the 5% more comfort of
> programmers, at the 90% more discomfort of sysadmins / users.

I think a lot of it comes from the endless quest to make smaller and
smaller container images, which IMHO is little to do with any real
technical problem and more to do with a kind of peacock's tail of
evolutionary battles.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 02:58:19PM +0200, Laszlo Ersek wrote:
> On 09/30/22 13:52, Daniel P. Berrangé wrote:
> > On Fri, Sep 30, 2022 at 12:56:40PM +0200, Laszlo Ersek wrote:
> >> On 09/30/22 11:46, Daniel P. Berrangé wrote:
> >>> On Fri, Sep 30, 2022 at 10:33:02AM +0100, Richard W.M. Jones wrote:
> >>>>
> >>>> Hmm, here's an interesting stackoverflow posting ...
> >>>>
> >>>> https://stackoverflow.com/questions/3301023/gtk-spinner-not-appearing
> >>>>
> >>>> The first point is "Make sure librsvg is installed".  librsvg is _not_
> >>>> installed in the ISO.  Laszlo, can you try building an ISO with this
> >>>> package explicitly added to the deps?
> >>>
> >>> Yes, that could be it.  The widget is rendered using CSS and the
> >>> Adwaita CSS rule references "process-working-symbolic" as the
> >>> icon, and that icon is only shipped in SVG format AFAICT.
> > 
> > snip
> > 
> >> I'm of the opinion that we should just get rid of metacity altogether,
> >> and use icewm. I don't know why metacity is not working, but I consider
> >> metacity a seriously substandard WM anyway, so let's just get rid of it?
> > 
> > Note, icewm doesn't exist in RHEL, only EPEL.
> 
> Correct, but I don't see that as a problem, considering that the last
> time we've officially "released" a p2v ISO, that was in the RHEL-7.6 (I
> think) timeframe, the infrastructure needed for building the ISO even on
> that major RHEL release does not exist any longer, and the only interim
> solution we have right now is me building the binary on my workstation
> and the ISO in a dedicated Fedora VM. For RHEL-9.2, it's theoretically
> possible that the Image Builder service can accommodate our needs, but
> then we're going to use custom repos anyway.
> 
> (I've just finished the commit message for the replacement, and yes I
> mentioned EPEL9 there.)

I think unfortunately this is going to be a problem - we can't really
depend on EPEL packages for supported RHEL products.

Can we make the WM optional?  ie. Change the default but keep the
possibility of still using metacity around.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 01:11:50PM +0100, Richard W.M. Jones wrote:
> icewm is available in RHEL 9.

Actually as Dan says, this isn't true.

I checked and it comes from EPEL:

# dnf install icewm
Updating Subscription Management repositories.
Unable to read consumer identity

This system is not registered with an entitlement server. You can use 
subscription-manager to register.

Last metadata expiration check: 2:11:05 ago on Fri 30 Sep 2022 11:06:17 BST.
Dependencies resolved.

 Package  Arch Version Repository  Size

Installing:
 icewmx86_64   2.9.9-1.el9 epel   1.3M
[...]


As Dan also said, the fix seems possibly because librsvg was installed
by some dependency.  Can you try the simpler solution of just adding
that dependency?

> I don't know if there's any reason to
> prefer metacity over icewm.  Usually when looking at virt-p2v
> dependencies, we tended to prefer, in order:
> 
>  - availability in RHEL

This is really required, and moving icewm into RHEL isn't
something that is easy to negotiate.

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] [v2v PATCH] convert_linux: include the BOCHS DRM driver in the initial ram disk

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 02:04:44PM +0200, Laszlo Ersek wrote:
> UEFI RHEL-7 guests cannot be successfully converted from VMWare without
> including the BOCHS DRM driver -- Plymouth ("rhgb") crashes during early
> boot in the converted domain.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2131123
> Signed-off-by: Laszlo Ersek 
> ---
>  convert/convert_linux.ml | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index b8e9ad15e22d..5bfdac5aa6d9 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -731,8 +731,13 @@ let convert (g : G.guestfs) source inspect 
> keep_serial_console _ =
>  match kernel.ki_initrd with
>  | None -> ()
>  | Some initrd ->
> -  (* Enable the basic virtio modules in the kernel. *)
> -  (* Also forcibly include the "xts" module; see RHBZ#1658126. *)
> +  (* Enable the basic virtio modules in the kernel.
> +   *
> +   * Also forcibly include the "xts" module; see RHBZ#1658126.
> +   *
> +   * Include the BOCHS DRM paravirt video driver; see RHBZ#2131123.  This
> +   * driver is known under two names -- "bochs-drm" and "bochs".
> +   *)
>let modules =
>  let modules =
>(* The order of modules here is deliberately the same as the
> @@ -743,7 +748,8 @@ let convert (g : G.guestfs) source inspect 
> keep_serial_console _ =
> *)
>List.filter (fun m -> List.mem m kernel.ki_modules)
>[ "virtio"; "virtio_ring"; "virtio_blk";
> -    "virtio_scsi"; "virtio_net"; "virtio_pci"; "xts" ] in
> +"virtio_scsi"; "virtio_net"; "virtio_pci"; "xts";
> +"bochs-drm"; "bochs" ] in

Reviewed-by: Richard W.M. Jones 

A much simpler change than I thought it would be :-)

Rich.

>  if modules <> [] then modules
>  else
>(* Fallback copied from old virt-v2v.  XXX Why not "ide"? *)
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs

-- 
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] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 01:01:09PM +0200, Laszlo Ersek wrote:
> Also I reckon Rich does not use metacity -- originally a gtk2-based
> window manager -- on his laptop, but the gnome shell. gnome shell could
> similarly eliminate the issue. (Assuming we agree that the issue is *in*
> metacity.)

XFCE.

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] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 12:56:40PM +0200, Laszlo Ersek wrote:
> On 09/30/22 11:46, Daniel P. Berrangé wrote:
> > On Fri, Sep 30, 2022 at 10:33:02AM +0100, Richard W.M. Jones wrote:
> >>
> >> Hmm, here's an interesting stackoverflow posting ...
> >>
> >> https://stackoverflow.com/questions/3301023/gtk-spinner-not-appearing
> >>
> >> The first point is "Make sure librsvg is installed".  librsvg is _not_
> >> installed in the ISO.  Laszlo, can you try building an ISO with this
> >> package explicitly added to the deps?
> >
> > Yes, that could be it.  The widget is rendered using CSS and the
> > Adwaita CSS rule references "process-working-symbolic" as the
> > icon, and that icon is only shipped in SVG format AFAICT.
> 
> (1) sorry, I've been in write-only mode for a long while now. That's why
> we've apparently arrived at nearly the same conclusions thus far,
> duplicating (triplicating?) our efforts. That said, I'm glad we did
> arrive at the same ideas.
> 
> (2) A number of surprises:
> 
> (2a) p2v already spells out icewm[-lite] as a depencency, just not when
> the VM boot disk image / ISO image is based on Fedora (more precisely:
> for SUSE). When building on Fedora, the WM included is Metacity instead
> (and the launch script starts metacity vs. icewm accordingly.)

It seems like SUSE developer Cédric Bosdonnat made that addition.  He
actually removed metacity and replaced it with icewm-lite
(commit cebcf47f6) and then made the change to the launch script to
select the right WM (commit 75c24c710).  However he didn't make any
corresponding change to Fedora so we now have both.

icewm is available in RHEL 9.  I don't know if there's any reason to
prefer metacity over icewm.  Usually when looking at virt-p2v
dependencies, we tended to prefer, in order:

 - availability in RHEL

 - small size

 - functionality

> (2b) There is a makefile dependency bug in virt-p2v; I'm going to post
> the patch for that.
> 
> > diff --git a/Makefile.am b/Makefile.am
> > index 02ff1bb2eebd..2881fc947b24 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -328,7 +328,7 @@ $(BLANK_DISK):
> > $(top_builddir)/run guestfish -N $@=part exit
> >
> >  virt-p2v.img: \
> > -   dependencies.m4 \
> > +   $(dependencies_files) \
> > issue \
> > launch-virt-p2v \
> > p2v.service \
> > @@ -373,7 +373,7 @@ stamp-test-virt-p2v-pxe-data-files: \
> > touch $@
> >
> >  test-virt-p2v-pxe.img: \
> > -   dependencies.m4 \
> > +   $(dependencies_files) \
> > issue \
> > launch-virt-p2v \
> > p2v.service \

Yes, seems like a mistake.

> Namely, even after I modified "dependencies.m4", the disk image did not
> get correctly regenerated for "run-virt-p2v-in-a-vm". While the image
> *was* regenerated, the intermediate (OS-filtered) deps files were not,
> and so the change I had made to "dependencies.m4" did not affect the
> actual package installation command line.

Is there another missing dep or did you track down the reason for that?

> (2c) I learned about metacity already being present when I launched an
> xterm window from the first dialog of p2v, and attempted to run icewm.
> IceWM complained that another window manager was already present; that
> was when I found metacity in the process list (pstree).
> 
> (2d) I started icewm with "icewm --replace" (as recommended by the error
> message from (2c)), and lo and behold, two changes had come into effect:
> 
> - the spinner started working

This is strange ..?  Did librsvg get installed at some point?

> - I actually got an actual Task Bar, where I could see all the windows:
>   virt-p2v, the GTK debug windows (plural), the XTerm window, and (in
>   the Tray area) even nm-applet! This is not a small improvement: QEMU
>   doesn't let me easily inject Alt-Tab, so switching between windows in
>   the VM environment has been a huge chore (made worse by the many GTK
>   debug windows).

Yup.

> (2e) That is, elaborating on the second bullet from (2d), not only is
> metacity (or else, our setup/usage of metacity) bugged enough to break
> the spinner, it even (i) does not offer a functional task bar, (b) does
> not show nm-applet -- there is no tray area!
> 
> I'm of the opinion that we should just get rid of metacity altogether,
> and use icewm. I don't know why metacity is not working, but I consider
> metacity a seriously substandard WM anyway, so let's just get rid of it?

Sure.  I don't think we have any particular affinity to metacity.  It
does however depen

Re: [Libguestfs] another GTK3 regression...

[Richard W.M. Jones]

Hmm, here's an interesting stackoverflow posting ...

https://stackoverflow.com/questions/3301023/gtk-spinner-not-appearing

The first point is "Make sure librsvg is installed".  librsvg is _not_
installed in the ISO.  Laszlo, can you try building an ISO with this
package explicitly added to the deps?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 10:04:27AM +0100, Daniel P. Berrangé wrote:
> As an idea to debug it more, try using the GTK inspector
> 
>$ GTK_DEBUG=interactive 
> 
> this will popup a second window, which lets you browse the
> widget hierarchy, view properties, etc. This might let you
> see something that's set in an incorrect way.

For future reference, inside the virt-p2v ISO environment I logged in
as root/p2v and edited /usr/bin/launch-virt-p2v, changing:

-   exec /usr/bin/virt-p2v --iso --colours
+   exec bash -c 'GTK_DEBUG=interactive /usr/bin/virt-p2v --iso --colours'

I then ran launch-virt-p2v from the text console.  This starts X11 and
does launches virt-p2v & the separate inspector window.

If you look at the attached screenshot, the inspector window (lower)
has the GtkSpinner widget in blue.  The widget name is greyed out
which makes it hard to read.  I guess that corresponds to the spinner
being hidden.

If you click Test Connection then the widget name becomes visible,
probably corresponding to the idle task being run and calling
start_spinner which shows the widget.  (The spinner itself still does
not show up or "spin").

When Test connection finishes, the widget name again goes back to
being greyed out, probably corresponding to stop_spinner being called.

A few other notes:

The GtkLabel below the blue bar in the screenshot is the spinner
message.  This is being displayed and updated properly.

If you click to the right of the "Style Classes" column in the
inspector, then the widget or box is flashed in blue briefly.  When
GtkSpinner is inactive nothing happens.  BUT (and it is difficult to
do the mouse gymnastics here) if you click "Test Connection" and while
that is running click to the right of the GtkSpinner, it does
highlight a small blue box (displaying nothing) to the left of the
spinner message.  So it seems that the spinner widget is occupying the
expected place on the screen, even if it's not displaying anything.

Clicking the second button along the top row of the inspector shows
details of the spinner.  There's a lot of stuff here.  For example
under "CSS nodes" I can see the -gtk-icon-transform CSS changing to
"rotate(XXX)" (with "XXX" rapidly increasing) as the spinner is
supposed to be animating, but again no animation is shown.

Under "Visual" there are various buttons.  If you enable "Show Graphic
Updates" then the GtkSpinner widget space flashes in red (again the
spinner itself does not display), so it seems to indicate that Gtk
"knows" that the spinner should be updating even if for some reason it
doesn't get rendered.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Richard W.M. Jones]

On Fri, Sep 30, 2022 at 08:55:02AM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 29, 2022 at 07:12:15PM +0100, Richard W.M. Jones wrote:
> > On Thu, Sep 29, 2022 at 04:47:34PM +0200, Laszlo Ersek wrote:
> > > http://lacos.interhost.hu/livecd-p2v-202209291608-gitc213ae00a337.iso
> > > 
> > > (built at c213ae00a337)
> > > 
> > > sha256: f3a149aeab0179213d74bb1eac30d5d6f807d4c9cf3a548667903d5434d5699a
> > 
> > No spinner!
> 
> BTW, is there any possibility your code is invoking GTK3 APIs
> from a thread != main GTK event loop thread ?  If so, that is
> a sure way to get non-deterministic wierd behaviour with GTK.

Maybe?  This is the code which is run when the "Test Connection"
button is clicked:

https://github.com/libguestfs/virt-p2v/blob/c213ae00a337cb04e63cbfe4fb4b3af4c003918f/gui.c#L404

It runs a thread (to test the connection using ssh) and that thread
creates an idle {job? event?} in the new thread.

However I documented it as:

 * Idle task called from C (but run on the
 * main thread) to start the spinner in the connection dialog.

Whether that is true or not and how I deduced that, I don't recall.

Rich.


> > While I remember, an annoying virt-p2v bug is that the keymap is
> > always set to the US locale (try XTerm -> localctl status).  I wonder
> > if it's easy to add a way to change the keyboard layout?  In
> > particular it doesn't work well if your password contains some
> > punctuation character which is mapped differently on US vs local
> > keyboards.
> 
> Assuming GTK3, you can use gsettings to change layout for apps
> 
> gsettings set org.gnome.desktop.input-sources sources 
> "[('"xkb"','"us"'),('"xkb"','"fr"'),('"xkb"','"it"')]"
> gsettings set org.gnome.desktop.input-sources current 1
> 
> this can be done dynamically on the fly too
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

-- 
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] another GTK3 regression...

[Richard W.M. Jones]

On Thu, Sep 29, 2022 at 04:47:34PM +0200, Laszlo Ersek wrote:
> http://lacos.interhost.hu/livecd-p2v-202209291608-gitc213ae00a337.iso
> 
> (built at c213ae00a337)
> 
> sha256: f3a149aeab0179213d74bb1eac30d5d6f807d4c9cf3a548667903d5434d5699a

No spinner!

While I remember, an annoying virt-p2v bug is that the keymap is
always set to the US locale (try XTerm -> localctl status).  I wonder
if it's easy to add a way to change the keyboard layout?  In
particular it doesn't work well if your password contains some
punctuation character which is mapped differently on US vs local
keyboards.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH nbdkit] rust: Drop box ptr

[Richard W.M. Jones]

On Thu, Sep 29, 2022 at 09:56:34AM -0600, alan somers wrote:
> This change is correct.  You can consider it reviewed by me and commit
> it.  As for the warning from Mockall, that's a bug in Mockall.  I'll
> fix it.

Thanks Alan - the drop() addition pushed as 6836e4c36.

Rich.

> On Thu, Sep 29, 2022 at 8:21 AM Richard W.M. Jones  wrote:
> >
> > Because of this recent change to rust, our close function warned that
> > we calculate Box::from_raw but never use it.  I added the suggested
> > call to drop() around the function.
> >
> > https://github.com/rust-lang/rust/pull/99270
> > ---
> >  plugins/rust/src/lib.rs | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs
> > index 69be36fb2..128334ef2 100644
> > --- a/plugins/rust/src/lib.rs
> > +++ b/plugins/rust/src/lib.rs
> > @@ -368,7 +368,7 @@ mod ffi {
> >
> >  pub(super) extern fn close(selfp: *mut c_void) {
> >  unsafe {
> > -Box::from_raw(selfp as *mut Box);
> > +drop(Box::from_raw(selfp as *mut Box));
> >  }
> >  }
> >
> > --
> > 2.37.0.rc2
> >

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters

[Richard W.M. Jones]

On Thu, Sep 29, 2022 at 09:08:26AM -0500, Eric Blake wrote:
> On Thu, Sep 29, 2022 at 02:42:05PM +0100, Richard W.M. Jones wrote:
> > > > +  (* Output __attribute__((nonnull)) for the function parameters:
> > > > +   * eg. struct nbd_handle *, int, char *
> > > > +   * => [ true, false, true ]
> > > > +   * => LIBNBD_ATTRIBUTE_NONNULL((1,3))
> > > > +   * => __attribute__((nonnull,(1,3)))
> > > 
> > > Style question. Do we want to REQUIRE the clients of this macro to
> > > pass in (), or would it be better to have a varargs format?
> > > 
> > > > +   *)
> > > > +  let nns : bool list =
> > > > +[ true ] (* struct nbd_handle * *)
> > > > +@ List.flatten (List.map arg_attr_nonnull args)
> > > > +@ List.flatten (List.map optarg_attr_nonnull optargs) in
> > > > +  let nns = List.mapi (fun i b -> (i+1, b)) nns in
> > > > +  let nns = filter_map (fun (i, b) -> if b then Some i else None) nns 
> > > > in
> > > > +  let nns : string list = List.map string_of_int nns in
> > > > +  pr "\nLIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns)
> > > 
> > > For generated code, it is just as easy to cope with either style (we
> > > can strip a layer of () if we want a varargs format).
> > > 
> ...
> > > > +  pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
> > > > +  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 12 /* gcc >= 
> > > > 12.0 */\n";
> > > > +  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ 
> > > > s))\n";
> > > 
> > > This definition is what requires us to pass in our own ().  That is,
> > > our end result is going to be one of:
> > > 
> > > __attribute__((__nonnull__(1) ))
> > > __attribute__((__nonnull__(1, 2) ))
> > > 
> > > but the difference is whether we must pass exactly one macro argument,
> > > and where that argument must include () even when there is only one
> > > parameter to be marked (what you coded):
> > > 
> > > LIBNBD_ATTRIBUTE_NONNULL((1))
> > > LIBNBD_ATTRIBUTE_NONNULL((1, 3))
> > > 
> > > vs. ease-of-use in supplying the () as part of the macro definition
> > > itself by using #define MACRO(...) and __VA_ARGS__:
> > > 
> > > LIBNBD_ATTRIBUTE_NONNULL(1)
> > > LIBNBD_ATTRIBUTE_NONNULL(1, 3)
> > 
> > I'm not sure I understand - what does the second definition look like?
> 
> Using a shorter name for testing:
> 
> $ cat foo.c
> #define my(...) __attribute__((__nonnull__(__VA_ARGS__)))
> extern void foo (char *a) my (1);
> extern void bar (char *a, char *b) my (1, 2);
> $ gcc -E foo.c
> # 0 "foo.c"
> # 0 ""
> # 0 ""
> # 1 "/usr/include/stdc-predef.h" 1 3 4
> # 0 "" 2
> # 1 "foo.c"
> 
> extern void foo (char *a) __attribute__((__nonnull__(1)));
> extern void bar (char *a, char *b) __attribute__((__nonnull__(1, 2)));
> $ gcc -o foo.o -c foo.c
> $ # compiled, so we satisfied gcc's attribute syntax
> 
> and similarly,
> #define LIBNBD_ATTRIBUTE_NONNULL(...) /* no-op */
> when disabling the macro.

Oh easier than I thought.  I'll push a fix for that, thanks.

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

[Libguestfs] [PATCH nbdkit] rust: Drop box ptr

[Richard W.M. Jones]

Because of this recent change to rust, our close function warned that
we calculate Box::from_raw but never use it.  I added the suggested
call to drop() around the function.

https://github.com/rust-lang/rust/pull/99270
---
 plugins/rust/src/lib.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/rust/src/lib.rs b/plugins/rust/src/lib.rs
index 69be36fb2..128334ef2 100644
--- a/plugins/rust/src/lib.rs
+++ b/plugins/rust/src/lib.rs
@@ -368,7 +368,7 @@ mod ffi {
 
 pub(super) extern fn close(selfp: *mut c_void) {
 unsafe {
-Box::from_raw(selfp as *mut Box);
+drop(Box::from_raw(selfp as *mut Box));
 }
 }
 
-- 
2.37.0.rc2

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

[Libguestfs] [PATCH nbdkit] rust: Drop box ptr

[Richard W.M. Jones]

This isn't a complete fix.  There's still this error when running the
tests.  I don't understand which expect function it's complaining
about.  One generated by the MockServer?

--
warning: the cargo feature `resolver` has been stabilized in the 1.51 release 
and is no longer necessary to be listed in the manifest
  See https://doc.rust-lang.org/cargo/reference/resolver.html#resolver-versions 
for more information about using this feature.
   Compiling nbdkit v0.2.0 (/home/rjones/d/nbdkit/plugins/rust)
error: associated function `expect` is never used
  --> tests/common/mod.rs:49:1
   |
49 | / mock!{
50 | | pub Server {}
51 | | impl Server for Server {
52 | | fn cache(, count: u32, offset: u64) -> Result<()>;
...  |
86 | | }
87 | | }
   | |_^
   |
   = note: `-D dead-code` implied by `-D warnings`

error: could not compile `nbdkit` due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `nbdkit` due to previous error
FAIL test.sh (exit status: 101)
--

Rich.


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

Re: [Libguestfs] [PATCH libnbd v3 0/6] Additional non-NULL warnings, checks and docs

[Richard W.M. Jones]

This series was pushed as 21b279a78e..e62a558f74

I guess there will still be some problems, and we may want to
rewrite the LIBNBD_ATTRIBUTE_NONNULL macro slightly, but we can
fix that lot up afterwards.

Thanks for everyone's help!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters

[Richard W.M. Jones]

On Thu, Sep 29, 2022 at 08:25:29AM -0500, Eric Blake wrote:
> On Wed, Sep 28, 2022 at 06:25:35PM +0100, Richard W.M. Jones wrote:
> > For API parameters that are pointers and must not be NULL, add the
> > appropriate GCC annotations.
> > 
> > Reviewed-by: Laszlo Ersek 
> > ---
> >  generator/C.ml  | 59 +++--
> >  tests/errors-connect-null.c |  4 +++
> >  2 files changed, 61 insertions(+), 2 deletions(-)
> 
> > @@ -216,7 +238,21 @@ let
> >  let print_fndecl ?wrap ?closure_style name args optargs ret =
> >pr "extern ";
> >print_call ?wrap ?closure_style name args optargs ret;
> > -  pr ";\n"
> > +
> > +  (* Output __attribute__((nonnull)) for the function parameters:
> > +   * eg. struct nbd_handle *, int, char *
> > +   * => [ true, false, true ]
> > +   * => LIBNBD_ATTRIBUTE_NONNULL((1,3))
> > +   * => __attribute__((nonnull,(1,3)))
> 
> Style question. Do we want to REQUIRE the clients of this macro to
> pass in (), or would it be better to have a varargs format?
> 
> > +   *)
> > +  let nns : bool list =
> > +[ true ] (* struct nbd_handle * *)
> > +@ List.flatten (List.map arg_attr_nonnull args)
> > +@ List.flatten (List.map optarg_attr_nonnull optargs) in
> > +  let nns = List.mapi (fun i b -> (i+1, b)) nns in
> > +  let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in
> > +  let nns : string list = List.map string_of_int nns in
> > +  pr "\nLIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns)
> 
> For generated code, it is just as easy to cope with either style (we
> can strip a layer of () if we want a varargs format).
> 
> >  
> >  let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
> >cbargs =
> > @@ -349,6 +385,19 @@ let
> >pr "extern \"C\" {\n";
> >pr "#endif\n";
> >pr "\n";
> > +  pr "#if defined(__GNUC__)\n";
> > +  pr "#define LIBNBD_GCC_VERSION \\\n";
> > +  pr "(__GNUC__ * 1 + __GNUC_MINOR__ * 100 + 
> > __GNUC_PATCHLEVEL__)\n";
> > +  pr "#endif\n";
> > +  pr "\n";
> > +  pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
> > +  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 12 /* gcc >= 12.0 
> > */\n";
> > +  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ 
> > s))\n";
> 
> This definition is what requires us to pass in our own ().  That is,
> our end result is going to be one of:
> 
> __attribute__((__nonnull__(1) ))
> __attribute__((__nonnull__(1, 2) ))
> 
> but the difference is whether we must pass exactly one macro argument,
> and where that argument must include () even when there is only one
> parameter to be marked (what you coded):
> 
> LIBNBD_ATTRIBUTE_NONNULL((1))
> LIBNBD_ATTRIBUTE_NONNULL((1, 3))
> 
> vs. ease-of-use in supplying the () as part of the macro definition
> itself by using #define MACRO(...) and __VA_ARGS__:
> 
> LIBNBD_ATTRIBUTE_NONNULL(1)
> LIBNBD_ATTRIBUTE_NONNULL(1, 3)

I'm not sure I understand - what does the second definition look like?

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 2/4] generator: Check that more parameters are not NULL

[Richard W.M. Jones]

On Thu, Sep 29, 2022 at 10:58:36AM +0100, Richard W.M. Jones wrote:
> It seems to be possible with phantom types & GADTs which are a very
> dark corner of OCaml and one which we shouldn't use, but here's how:
> 
> --
> 
> type nullable
> type not_nullable

Probably would have been better if I'd called those phantom types
"primitive" and "boxed" :-)

Rich.

> type _ arg =
> | Char   : not_nullable arg
> | Int: not_nullable arg
> | String : nullable arg
> | List   : nullable arg
> 
> (*
> # Char ;;
> - : not_nullable arg = Char
> # String ;;
> - : nullable arg = String
> *)
> 
> let only_for_nullables = function
>   | String -> "do something"
>   | List -> "do something else"
>   | not_nullable -> .
> (* val only_for_nullables : nullable arg -> string =  *)
> 
> --
> 
> Notes:
> 
> (1) The “| not_nullable -> .” line can be omitted.  It's just there to
> document the unreachable case, but (in this case) the compiler can
> infer this.
> 
> (2) If you omit either of the “| String“ or “| List“ cases then the
> compile will warn about incomplete matching, which is good.
> 
> Extending the example further, you can see how GADTs make the common
> case (second one below) possible, but harder to write:
> 
> --
> # let only_not_nullables = function | Char -> "char" | Int -> "int" ;;
> val only_not_nullables : not_nullable arg -> string = 
> 
> # let all : type a. a arg -> string = function
>   | Char -> "char"
>   | Int -> "int"
>   | String -> "string"
>   | List -> "list";;
> val all : 'a arg -> string = 
> --
> 
> 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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v2 2/4] generator: Check that more parameters are not NULL

[Richard W.M. Jones]

It seems to be possible with phantom types & GADTs which are a very
dark corner of OCaml and one which we shouldn't use, but here's how:

--

type nullable
type not_nullable

type _ arg =
| Char   : not_nullable arg
| Int: not_nullable arg
| String : nullable arg
| List   : nullable arg

(*
# Char ;;
- : not_nullable arg = Char
# String ;;
- : nullable arg = String
*)

let only_for_nullables = function
  | String -> "do something"
  | List -> "do something else"
  | not_nullable -> .
(* val only_for_nullables : nullable arg -> string =  *)

--

Notes:

(1) The “| not_nullable -> .” line can be omitted.  It's just there to
document the unreachable case, but (in this case) the compiler can
infer this.

(2) If you omit either of the “| String“ or “| List“ cases then the
compile will warn about incomplete matching, which is good.

Extending the example further, you can see how GADTs make the common
case (second one below) possible, but harder to write:

--
# let only_not_nullables = function | Char -> "char" | Int -> "int" ;;
val only_not_nullables : not_nullable arg -> string = 

# let all : type a. a arg -> string = function
  | Char -> "char"
  | Int -> "int"
  | String -> "string"
  | List -> "list";;
val all : 'a arg -> string = 
--

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 v3 3/6] lib/internal.h: Mark various internal functions with attribute((nonnull))

[Richard W.M. Jones]

On Wed, Sep 28, 2022 at 05:08:40PM -0500, Eric Blake wrote:
> On Wed, Sep 28, 2022 at 06:25:36PM +0100, Richard W.M. Jones wrote:
> > ---
> >  lib/internal.h | 75 +-
> >  1 file changed, 50 insertions(+), 25 deletions(-)
> 
> >  
> >  /* utils.c */
> > -extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp);
> > -extern int nbd_internal_copy_string_list (string_vector *v, char **in);
> > -extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv);
> > -extern int nbd_internal_set_querylist (struct nbd_handle *h, char 
> > **queries);
> > -extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t 
> > len);
> > -extern void nbd_internal_fork_safe_perror (const char *s);
> > -extern char *nbd_internal_printable_buffer (const void *buf, size_t count);
> > -extern char *nbd_internal_printable_string (const char *str);
> > +extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp)
> > +  LIBNBD_ATTRIBUTE_NONNULL((1, 3));
> > +extern int nbd_internal_copy_string_list (string_vector *v, char **in)
> > +  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
> > +extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv)
> > +  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
> > +extern int nbd_internal_set_querylist (struct nbd_handle *h, char 
> > **queries)
> > +  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
> 
> The ', 2' is wrong here, based on my review of 1/6.  This is one
> helper where I wanted NULL to mean "copy the implicit
> h->request_meta_contexts", and non-NULL to be "copy this explicit
> list, even if it is empty".

FYI I fixed this (removed the ",2") in my updated patch.  The function
allows queries == NULL as a valid input as before.

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 v3 1/6] lib/opt: Don't use NULL for an empty StringList

[Richard W.M. Jones]

How about this alternate to patch 1.

I have also adjusted a later patch so it no longer sets
attribute((nonnull)) on the queries parameter of
nbd_internal_set_querylist, so that function is unchanged
from before.

Also the tests pass.

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
>From bc0de6d378dd78da13210a315fd134f9d063b1ba Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" 
Date: Wed, 28 Sep 2022 18:01:40 +0100
Subject: [PATCH] lib/opt: Don't call nbd_unlocked_*_meta_context_queries with
 NULL

StringList parameters (char ** in C) will be marked with
__attribute__((nonnull)), including the internal nbd_unlocked_*
functions, so we cannot overload a new meaning with queries == NULL.

Add internal common functions that allow this instead.

Fixes: commit e33762a86cd5f064e5ef841520baa5fe7361d2c2
---
 lib/opt.c | 61 ---
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/lib/opt.c b/lib/opt.c
index 1b18920fdb..68850a2ee9 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -26,6 +26,21 @@
 
 #include "internal.h"
 
+static int opt_set_meta_context_queries (struct nbd_handle *h,
+ char **queries,
+ nbd_context_callback *context);
+static int opt_list_meta_context_queries (struct nbd_handle *h,
+  char **queries,
+  nbd_context_callback *context);
+static int aio_opt_set_meta_context_queries (struct nbd_handle *h,
+ char **queries,
+ nbd_context_callback *context,
+ nbd_completion_callback 
*complete);
+static int aio_opt_list_meta_context_queries (struct nbd_handle *h,
+  char **queries,
+  nbd_context_callback *context,
+  nbd_completion_callback 
*complete);
+
 /* Internal function which frees an option with callback. */
 void
 nbd_internal_free_option (struct nbd_handle *h)
@@ -224,7 +239,7 @@ int
 nbd_unlocked_opt_list_meta_context (struct nbd_handle *h,
 nbd_context_callback *context)
 {
-  return nbd_unlocked_opt_list_meta_context_queries (h, NULL, context);
+  return opt_list_meta_context_queries (h, NULL, context);
 }
 
 /* Issue NBD_OPT_LIST_META_CONTEXT and wait for the reply. */
@@ -232,6 +247,14 @@ int
 nbd_unlocked_opt_list_meta_context_queries (struct nbd_handle *h,
 char **queries,
 nbd_context_callback *context)
+{
+  return opt_list_meta_context_queries (h, queries, context);
+}
+
+static int
+opt_list_meta_context_queries (struct nbd_handle *h,
+   char **queries,
+   nbd_context_callback *context)
 {
   struct context_helper s = { .context = *context };
   nbd_context_callback l = { .callback = context_visitor, .user_data =  };
@@ -255,7 +278,7 @@ int
 nbd_unlocked_opt_set_meta_context (struct nbd_handle *h,
nbd_context_callback *context)
 {
-  return nbd_unlocked_opt_set_meta_context_queries (h, NULL, context);
+  return opt_set_meta_context_queries (h, NULL, context);
 }
 
 /* Issue NBD_OPT_SET_META_CONTEXT and wait for the reply. */
@@ -263,12 +286,20 @@ int
 nbd_unlocked_opt_set_meta_context_queries (struct nbd_handle *h,
char **queries,
nbd_context_callback *context)
+{
+  return opt_set_meta_context_queries (h, queries, context);
+}
+
+static int
+opt_set_meta_context_queries (struct nbd_handle *h,
+  char **queries,
+  nbd_context_callback *context)
 {
   struct context_helper s = { .context = *context };
   nbd_context_callback l = { .callback = context_visitor, .user_data =  };
   nbd_completion_callback c = { .callback = context_complete, .user_data =  
};
 
-  if (nbd_unlocked_aio_opt_set_meta_context_queries (h, queries, , ) == -1)
+  if (aio_opt_set_meta_context_queries (h, queries, , ) == -1)
 return -1;
 
   SET_CALLBACK_TO_NULL (*context);
@@ -371,8 +402,7 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle 
*h,
 nbd_context_callback *context,
 nbd_completion_callback *complete)
 {
-  return nbd_unlocked_aio_opt_list_meta_context_queries (h, NULL, context,
- complete);
+  return aio_opt_li

Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList

[Richard W.M. Jones]

OK now I remember what the problem was.

> @@ -255,7 +257,9 @@ int
>  nbd_unlocked_opt_set_meta_context (struct nbd_handle *h,
> nbd_context_callback *context)
>  {
> -  return nbd_unlocked_opt_set_meta_context_queries (h, NULL, context);

In this original code you're calling the internal unlocked version of
nbd_opt_set_meta_context_queries.  However the generator is creating a
prototype for the unlocked function and it adds the
attribute((nonnull)) annotation for it, something like:

  extern int nbd_unlocked_opt_set_meta_context_queries (...)
LIBNBD_ATTRIBUTE_NONNULL((1, 2));

This means that you cannot use queries == NULL here.

I think the generated annotation is correct, but we need a new
unannotated internal function that allows queries == NULL.

I'll try to come up with something.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList

[Richard W.M. Jones]

Let me try an alternate version of this.

The problem is that I was getting GCC warnings even before adding the
annotation to nbd_internal_set_querylist.  Somehow the annotation on
the public function "leaked" through to the internal function.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH libnbd v3 6/6] lib/utils.c: Assert that argv != NULL and add comments

[Richard W.M. Jones]

Change check into an assertion, and add detailed comments explaining
our assumptions.

Updates: commit d0fbb769286a97728b0d1358e7accc2eb708d795
Reviewed-by: Laszlo Ersek 
---
 lib/utils.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index b4043aa3bc..e538eeb4e6 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -79,11 +79,21 @@ nbd_internal_copy_string_list (string_vector *v, char **in)
 int
 nbd_internal_set_argv (struct nbd_handle *h, char **argv)
 {
-  /* FIXME: Do we want to assert(argv)? */
-  if (argv == NULL || argv[0] == NULL) {
+  /* This should never be NULL.  Normally the generator adds code to
+   * each StringList call in lib/api.c to check this and return an
+   * error.
+   */
+  assert (argv);
+
+  /* Because this function is only called from functions that take
+   * argv-style lists of strings (such as nbd_connect_command) we can
+   * check here that the command name is present.
+   */
+  if (argv[0] == NULL) {
 set_error (EINVAL, "missing command name in argv list");
 return -1;
   }
+
   if (nbd_internal_copy_string_list (>argv, argv) == -1) {
 set_error (errno, "realloc");
 return -1;
-- 
2.37.0.rc2

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

[Libguestfs] [PATCH libnbd v3 0/6] Additional non-NULL warnings, checks and docs

[Richard W.M. Jones]

v2:
https://listman.redhat.com/archives/libguestfs/2022-September/030014.html

I didn't think this would need a v3, but here we are.

The first patch (also a new patch) appears to fix a bug in Eric's
earlier series to do with meta queries.  It's not possible to call the
new APIs with queries == NULL, and this becomes obvious when you use
attribute((nonnull)) and enable GCC warnings.  I tried to fix this,
but two tests still fail for reasons I'm not clear about:

FAIL: opt-list-meta-queries
FAIL: opt-set-meta-queries

The third patch is also new, and extends attribute((nonnull))
annotations to many internal functions.  No actual errors found by
this, but it seems worth it to avoid future problems, assuming that
GCC won't start adding undefined behaviour.  I wonder aloud if we
should only enable attribute((nonnull)) for developer builds, ie. tie
it to ./configure --enable-gcc-warnings somehow.  (This series does
not do this.)

I added comments as suggested by Laszlo to patch 2, and
picked up his R-b's and Acks.

Rich.



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

[Libguestfs] [PATCH libnbd v3 5/6] generator: Document non-NULL behaviour of some parameters

[Richard W.M. Jones]

In the C API documentation mention the potential problems of calling
non-nullable parameters with NULL.  Usually an error is returned, but
warnings and worse might happen too.

Thanks: Eric Blake
Acked-by: Laszlo Ersek 
---
 docs/libnbd.pod | 18 ++
 generator/C.ml  | 21 +
 2 files changed, 39 insertions(+)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index eae9bb413b..170fc1fa07 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -387,8 +387,26 @@ The library ran out of memory while performing some 
operation.
 A request is too large, for example if you try to read too many bytes
 in a single L call.
 
+=item C
+
+A pointer parameter was C when it should be non-NULL.
+See the section below.
+
 =back
 
+=head2 Non-NULL parameters
+
+Almost all libnbd functions when called from C take one or more
+pointer parameters that must not be C.  For example, the handle
+parameter, strings and buffers should usually not be C.
+
+If a C is passed as one of these parameters, libnbd attempts to
+return an error with L returning C.
+
+However it may cause other compiler-related warnings and even
+undefined behaviour, so you should try to avoid this programming
+mistake.
+
 =head1 DEBUGGING MESSAGES
 
 Libnbd can print lots of debugging messages, useful if you have a
diff --git a/generator/C.ml b/generator/C.ml
index 2f22ad1e59..7d4bf7d509 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -107,6 +107,11 @@ let
   | UInt64 n -> [n]
   | UIntPtr n -> [n]
 
+let name_of_optarg =
+  function
+  | OClosure { cbname } -> cbname
+  | OFlags (n, _, _) -> n
+
 (* Map function arguments to __attribute__((nonnull)) annotations.
  * Because a single arg may map to multiple C parameters this
  * returns a list.  Note: true => add attribute nonnull.
@@ -980,6 +985,7 @@ let
 
   pr "=head1 ERRORS\n";
   pr "\n";
+
   if may_set_error then (
 let value = match errcode with
   | Some value -> value
@@ -993,6 +999,21 @@ let
 pr "This function does not fail.\n";
   pr "\n";
 
+  pr "The following parameters must not be NULL: C";
+  List.iter (
+fun arg ->
+  let nns = arg_attr_nonnull arg in
+  if List.mem true nns then pr ", C<%s>" (List.hd (name_of_arg arg))
+  ) args;
+  List.iter (
+fun arg ->
+  let nns = optarg_attr_nonnull arg in
+  if List.mem true nns then pr ", C<%s>" (name_of_optarg arg)
+  ) optargs;
+  pr ".\n";
+  pr "For more information see L.\n";
+  pr "\n";
+
   if permitted_states <> [] then (
 pr "=head1 HANDLE STATE\n";
 pr "\n";
-- 
2.37.0.rc2

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

[Libguestfs] [PATCH libnbd v3 2/6] generator: Add attribute((nonnull)) annotations to non-NULL parameters

[Richard W.M. Jones]

For API parameters that are pointers and must not be NULL, add the
appropriate GCC annotations.

Reviewed-by: Laszlo Ersek 
---
 generator/C.ml  | 59 +++--
 tests/errors-connect-null.c |  4 +++
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index 66498a7d88..6e4538fd34 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -107,6 +107,28 @@ let
   | UInt64 n -> [n]
   | UIntPtr n -> [n]
 
+(* Map function arguments to __attribute__((nonnull)) annotations.
+ * Because a single arg may map to multiple C parameters this
+ * returns a list.  Note: true => add attribute nonnull.
+ *)
+let arg_attr_nonnull =
+  function
+  (* BytesIn/Out are passed using a non-null pointer, and size_t *)
+  | BytesIn _ | BytesOut _
+  | BytesPersistIn _ | BytesPersistOut _ -> [ true; false ]
+  (* sockaddr is also non-null pointer, and length *)
+  | SockAddrAndLen (n, len) -> [ true; false ]
+  (* strings should be marked as non-null *)
+  | Path _ | String _ -> [ true ]
+  (* list of strings should be marked as non-null *)
+  | StringList n -> [ true ]
+  (* numeric and other non-pointer types are not able to be null *)
+  | Bool _ | Closure _ | Enum _ | Fd _ | Flags _
+  | Int _ | Int64 _ | SizeT _
+  | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ]
+
+let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ]
+
 let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
   ?closure_style args optargs =
   if parens then pr "(";
@@ -216,7 +238,21 @@ let
 let print_fndecl ?wrap ?closure_style name args optargs ret =
   pr "extern ";
   print_call ?wrap ?closure_style name args optargs ret;
-  pr ";\n"
+
+  (* Output __attribute__((nonnull)) for the function parameters:
+   * eg. struct nbd_handle *, int, char *
+   * => [ true, false, true ]
+   * => LIBNBD_ATTRIBUTE_NONNULL((1,3))
+   * => __attribute__((nonnull,(1,3)))
+   *)
+  let nns : bool list =
+[ true ] (* struct nbd_handle * *)
+@ List.flatten (List.map arg_attr_nonnull args)
+@ List.flatten (List.map optarg_attr_nonnull optargs) in
+  let nns = List.mapi (fun i b -> (i+1, b)) nns in
+  let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in
+  let nns : string list = List.map string_of_int nns in
+  pr "\nLIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat "," nns)
 
 let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
   cbargs =
@@ -349,6 +385,19 @@ let
   pr "extern \"C\" {\n";
   pr "#endif\n";
   pr "\n";
+  pr "#if defined(__GNUC__)\n";
+  pr "#define LIBNBD_GCC_VERSION \\\n";
+  pr "(__GNUC__ * 1 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)\n";
+  pr "#endif\n";
+  pr "\n";
+  pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
+  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 12 /* gcc >= 12.0 
*/\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__ s))\n";
+  pr "#else\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
+  pr "#endif\n";
+  pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
+  pr "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
   List.iter (
@@ -382,7 +431,7 @@ let
   pr "extern struct nbd_handle *nbd_create (void);\n";
   pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
   pr "\n";
-  pr "extern void nbd_close (struct nbd_handle *h);\n";
+  pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL */\n";
   pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
   pr "\n";
   pr "extern const char *nbd_get_error (void);\n";
@@ -770,6 +819,12 @@ let
   pr "\n";
   pr "#include \n";
   pr "\n";
+  pr "/* GCC will remove NULL checks from this file for any parameter\n";
+  pr " * annotated with attribute((nonnull)).  See RHBZ#1041336.  To\n";
+  pr " * avoid this, disable the attribute when including libnbd.h.\n";
+  pr " */\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
+  pr "\n";
   pr "#include \"libnbd.h\"\n";
   pr "#include \"internal.h\"\n";
   pr "\n";
diff --git a/tests/errors-connect-null.c b/tests/errors-connect-null.c
index 27b21f0f8f..b975181afb 100644
--- a/tests/errors-connect-null.c
+++ b/tests/errors-connect-null.c
@@ -27,6 +27,10 @@
 #include 
 #include 
 
+/* GCC will warn that we are passing NULL (or worse), so to do this
+ * test we must remove the header file attribute.
+ */
+#define LIBNBD_ATTRIBUTE_NONNULL(s)
 #include 
 
 static char *progname;
-- 
2.37.0.rc2

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

[Libguestfs] [PATCH libnbd v3 4/6] generator: Check that more parameters are not NULL

[Richard W.M. Jones]

We previously checked only that String parameters are not NULL,
returning an error + EFAULT if so.

However we did not check Bytes*, SockAddrAndLen, Path or StringList
parameters, also never NULL.  Be consistent about checks.

Thanks: Eric Blake for help and an earlier version of the patch
---
 generator/API.ml| 7 +--
 generator/C.ml  | 7 ++-
 tests/errors-connect-null.c | 2 +-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 6e3213ad26..69849102cf 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -3851,13 +3851,16 @@ let () =
 
 (* !may_set_error is incompatible with certain parameters because
  * we have to do a NULL-check on those which may return an error.
+ * Refer to generator/C.ml:generator_lib_api_c.
  *)
 | name, { args; may_set_error = false }
  when List.exists
 (function
- | Closure _ | Enum _ | Flags _ | String _ -> true
+ | Closure _ | Enum _ | Flags _
+ | BytesIn _ | BytesOut _ | BytesPersistIn _ | BytesPersistOut 
_
+ | SockAddrAndLen _ | Path _ | String _ | StringList _-> true
  | _ -> false) args ->
-   failwithf "%s: if args contains Closure/Enum/Flags/String parameter, 
may_set_error must be false" name
+   failwithf "%s: if args contains any non-null pointer parameter, 
may_set_error must be false" name
 
 (* !may_set_error is incompatible with certain optargs too.
  *)
diff --git a/generator/C.ml b/generator/C.ml
index 6e4538fd34..2f22ad1e59 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -620,7 +620,12 @@ let
  need_out_label := true
   | Flags (n, flags) ->
  print_flags_check n flags None
-  | String n ->
+  | BytesIn (n, _) | BytesOut (n, _)
+  | BytesPersistIn (n, _) | BytesPersistOut (n, _)
+  | SockAddrAndLen (n, _)
+  | Path n
+  | String n
+  | StringList n ->
  let value = match errcode with
| Some value -> value
| None -> assert false in
diff --git a/tests/errors-connect-null.c b/tests/errors-connect-null.c
index b975181afb..5313c4c394 100644
--- a/tests/errors-connect-null.c
+++ b/tests/errors-connect-null.c
@@ -78,7 +78,7 @@ main (int argc, char *argv[])
 exit (EXIT_FAILURE);
   }
   /* FIXME: If we add nonnull attributes, this might change to EFAULT */
-  check (EINVAL, "nbd_connect_command: ");
+  check (EFAULT, "nbd_connect_command: ");
 
   if (nbd_connect_command (nbd, (char **) cmd) != -1) {
 fprintf (stderr, "%s: test failed: "
-- 
2.37.0.rc2

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

[Libguestfs] [PATCH libnbd v3 3/6] lib/internal.h: Mark various internal functions with attribute((nonnull))

[Richard W.M. Jones]

---
 lib/internal.h | 75 +-
 1 file changed, 50 insertions(+), 25 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index 7896b1d5ca..fb15ada1c3 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -384,20 +384,27 @@ struct command {
   } while (0)
 
 /* aio.c */
-extern void nbd_internal_retire_and_free_command (struct command *);
+extern void nbd_internal_retire_and_free_command (struct command *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* connect.c */
-extern int nbd_internal_wait_until_connected (struct nbd_handle *h);
+extern int nbd_internal_wait_until_connected (struct nbd_handle *h)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* crypto.c */
-extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, 
struct socket *oldsock);
-extern bool nbd_internal_crypto_is_reading (struct nbd_handle *);
-extern int nbd_internal_crypto_handshake (struct nbd_handle *);
-extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *);
+extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, 
struct socket *oldsock)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern bool nbd_internal_crypto_is_reading (struct nbd_handle *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
+extern int nbd_internal_crypto_handshake (struct nbd_handle *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
+extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* debug.c */
 extern void nbd_internal_debug (struct nbd_handle *h, const char *context,
-const char *fs, ...);
+const char *fs, ...)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 3));
 #define debug(h, fs, ...)   \
   do {  \
 if_debug ((h))  \
@@ -410,9 +417,11 @@ extern void nbd_internal_debug (struct nbd_handle *h, 
const char *context,
   } while (0)
 
 /* errors.c */
-extern void nbd_internal_set_error_context (const char *context);
+extern void nbd_internal_set_error_context (const char *context)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern const char *nbd_internal_get_error_context (void);
-extern void nbd_internal_set_last_error (int errnum, char *error);
+extern void nbd_internal_set_last_error (int errnum, char *error)
+  LIBNBD_ATTRIBUTE_NONNULL((2));
 #define set_error(errnum, fs, ...)  \
   do {  \
 int _e = (errnum);  \
@@ -430,12 +439,15 @@ extern void nbd_internal_set_last_error (int errnum, char 
*error);
   } while (0)
 
 /* flags.c */
-extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h);
+extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern int nbd_internal_set_size_and_flags (struct nbd_handle *h,
 uint64_t exportsize,
-uint16_t eflags);
+uint16_t eflags)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min,
-uint32_t pref, uint32_t max);
+uint32_t pref, uint32_t max)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* is-state.c */
 extern bool nbd_internal_is_state_created (enum state state);
@@ -447,7 +459,8 @@ extern bool nbd_internal_is_state_dead (enum state state);
 extern bool nbd_internal_is_state_closed (enum state state);
 
 /* opt.c */
-extern void nbd_internal_free_option (struct nbd_handle *h);
+extern void nbd_internal_free_option (struct nbd_handle *h)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* protocol.c */
 extern int nbd_internal_errno_of_nbd_error (uint32_t error);
@@ -458,15 +471,18 @@ extern int64_t nbd_internal_command_common (struct 
nbd_handle *h,
 uint16_t flags, uint16_t type,
 uint64_t offset, uint64_t count,
 int count_err, void *data,
-struct command_cb *cb);
+struct command_cb *cb)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 
 /* socket.c */
 struct socket *nbd_internal_socket_create (int fd);
 
 /* states.c */
 extern void nbd_internal_abort_commands (struct nbd_handle *h,
- struct command **list);
-extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev);
+ struct command **list)
+  LIBNBD_ATTRIBUTE_NONNULL((1, 2));
+extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev)
+  LIBNBD_ATTRIBUTE_NONNULL((1));
 extern const char 

Re: [Libguestfs] [p2v PATCH 2/2] remove old GLIB2 / GTK3 compat code

[Richard W.M. Jones]

For the series:

Reviewed-by: Richard W.M. Jones 

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

[Libguestfs] [PATCH libnbd v3 1/6] lib/opt: Don't use NULL for an empty StringList

[Richard W.M. Jones]

StringList parameters (char ** in C) will be marked with
__attribute__((nonnull)).  To pass an empty list you have to use a
list containing a single NULL element, not a NULL pointer.

nbd_internal_set_querylist has also been adjusted.

Fixes: commit e33762a86cd5f064e5ef841520baa5fe7361d2c2
---
 generator/states-newstyle-opt-meta-context.c |  4 +++-
 lib/opt.c| 16 
 lib/utils.c  |  4 ++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/generator/states-newstyle-opt-meta-context.c 
b/generator/states-newstyle-opt-meta-context.c
index 46fee15be1..a60aa66f3b 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -65,12 +65,14 @@  NEWSTYLE.OPT_META_CONTEXT.START:
 }
   }
   if (opt != h->opt_current) {
+char *empty[] = { NULL };
+
 if (!h->request_meta || !h->structured_replies ||
 h->request_meta_contexts.len == 0) {
   SET_NEXT_STATE (%^OPT_GO.START);
   return 0;
 }
-if (nbd_internal_set_querylist (h, NULL) == -1) {
+if (nbd_internal_set_querylist (h, empty) == -1) {
   SET_NEXT_STATE (%.DEAD);
   return 0;
 }
diff --git a/lib/opt.c b/lib/opt.c
index 1b18920fdb..e323d7b1b0 100644
--- a/lib/opt.c
+++ b/lib/opt.c
@@ -224,7 +224,9 @@ int
 nbd_unlocked_opt_list_meta_context (struct nbd_handle *h,
 nbd_context_callback *context)
 {
-  return nbd_unlocked_opt_list_meta_context_queries (h, NULL, context);
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_opt_list_meta_context_queries (h, empty, context);
 }
 
 /* Issue NBD_OPT_LIST_META_CONTEXT and wait for the reply. */
@@ -255,7 +257,9 @@ int
 nbd_unlocked_opt_set_meta_context (struct nbd_handle *h,
nbd_context_callback *context)
 {
-  return nbd_unlocked_opt_set_meta_context_queries (h, NULL, context);
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_opt_set_meta_context_queries (h, empty, context);
 }
 
 /* Issue NBD_OPT_SET_META_CONTEXT and wait for the reply. */
@@ -371,7 +375,9 @@ nbd_unlocked_aio_opt_list_meta_context (struct nbd_handle 
*h,
 nbd_context_callback *context,
 nbd_completion_callback *complete)
 {
-  return nbd_unlocked_aio_opt_list_meta_context_queries (h, NULL, context,
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_aio_opt_list_meta_context_queries (h, empty, context,
  complete);
 }
 
@@ -407,7 +413,9 @@ nbd_unlocked_aio_opt_set_meta_context (struct nbd_handle *h,
nbd_context_callback *context,
nbd_completion_callback *complete)
 {
-  return nbd_unlocked_aio_opt_set_meta_context_queries (h, NULL, context,
+  char *empty[] = { NULL };
+
+  return nbd_unlocked_aio_opt_set_meta_context_queries (h, empty, context,
 complete);
 }
 
diff --git a/lib/utils.c b/lib/utils.c
index c1d633fea1..b4043aa3bc 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -100,7 +100,7 @@ nbd_internal_set_querylist (struct nbd_handle *h, char 
**queries)
 {
   size_t i;
 
-  if (queries) {
+  if (queries[0] != NULL /* non-empty list */) {
 if (nbd_internal_copy_string_list (>querylist, queries) == -1) {
   set_error (errno, "realloc");
   return -1;
@@ -109,7 +109,7 @@ nbd_internal_set_querylist (struct nbd_handle *h, char 
**queries)
 assert (h->querylist.len > 0);
 string_vector_remove (>querylist, h->querylist.len - 1);
   }
-  else {
+  else /* empty list */ {
 string_vector_reset (>querylist);
 for (i = 0; i < h->request_meta_contexts.len; ++i) {
   char *copy = strdup (h->request_meta_contexts.ptr[i]);
-- 
2.37.0.rc2

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