Re: [Libguestfs] [PATCH v2v 0/5] convert: Find out if Windows guest is expecting BIOS localtime or UTC

[Daniel P . Berrangé]

On Mon, Sep 25, 2023 at 04:26:59PM +0200, Alice Frosi wrote:
> Hi Rich,
> 
> On Mon, Sep 25, 2023 at 4:10 PM Richard W.M. Jones 
> wrote:
> 
> > [Alice: See patch 2]
> >
> 
> I'm not 100% sure about the source of this work. However, we had in
> KubeVirt people interested in using localtime with Windows [1]. Yes, I see
> that you pointed to that PR in patch 2. The problem with that PR is the
> migration. What happens if we migrate the VM to a host that is in another
> timezone? Is it going to break the application running inside the VM?

I would generally expect all hosts to be configured in the same timezone,
for any given single cloud deployment.

If you really are concerned about this problem though, libvirt lets you
set this explicitly eg

   

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Fwd: virt-v2v creating image that does not install guest agent on first boot

[Daniel P . Berrangé]

On Mon, Sep 25, 2023 at 01:14:40PM +0100, Richard W.M. Jones wrote:
> On Mon, Sep 25, 2023 at 01:09:15PM +0200, Lee Garrett wrote:
> > On 23.09.23 19:37, Laszlo Ersek wrote:
> > >On 9/22/23 16:47, Lee Garrett wrote:
> > >>On 22.09.23 14:54, Richard W.M. Jones wrote:
> > >>>On Fri, Sep 22, 2023 at 11:40:03AM +0100, Richard W.M. Jones wrote:
> > On Thu, Sep 21, 2023 at 07:47:52PM +0200, Lee Garrett wrote:
> > >On 21.09.23 19:43, Richard W.M. Jones wrote:
> > >>So this is probably another instance or variation of the timezone
> > >>formatting problem (of schtasks).  Which version of virt-v2v is this?
> > >>I want to check that you have a version with all the latest patches in
> > >>this area.
> > >
> > >It's 2.2.0-1 from Debian (12) bookworm. I've verified that it
> > >doesn't have any distro-specific patches.
> > >
> > >(https://salsa.debian.org/libvirt-team/virt-v2v/-/tree/debian/master/debian
> > >would have a patches/series file in this case)
> > 
> > The timezone fixes are:
> > 
> > commit 597d177567234c3a539098c423649781424eeb6f
> > Author: Laszlo Ersek 
> > Date:   Tue Mar 8 15:30:51 2022 +0100
> > 
> >   convert_windows: rewrite "configure_qemu_ga" script purely in
> > PowerShell
> > 
> > commit d9dc6c42ae64ba92993dbd9477f003ba73fcfa2f
> > Author: Richard W.M. Jones 
> > Date:   Fri Nov 12 08:47:55 2021 +
> > 
> >   convert/convert_windows.ml: Handle date formats with dots
> > instead of /
> > 
> > They are all included in >= 2.0
> > 
> > I wonder if 597d177567 has a subtle flaw, or if we introduced a bug
> > somewhere when refactoring this code later.
> > 
> > Lee: Do you have a theory about exactly what is wrong with the
> > schtasks date?  Like what was it supposed to be, assuming it was 120
> > seconds in the future from boot time, versus what it was set to:
> > 
> > >Firstboot-qemu-ga    9/21/2023 4:04:00 PM   Ready
> > 
> > Could a date or time field have not been swapped or been corrupted
> > in some predictable way?
> > >>>
> > >>>Or in even simpler terms, what is the time (and timezone) that
> > >>>this ^^^ machine was booted?
> > >>
> > >>I believe I have it figured out.
> > >>The guest local time is currently 7:08 AM (a few minutes after
> > >>firstboot/provisioning), pacific daylight time (UTC-7, though Windows
> > >>displays it as "UTC-08:00"). This is the timezone that the guest comes
> > >>configured with at first boot. The task is scheduled for 2:01 PM,
> > >>meaning it's scheduled to run ~7 hours in the future.
> > >>
> > >>So it seems like the task was meant to be scheduled for 2:01 PM UTC (=
> > >>7:01 AM PDT), but for some reason was scheduled for 2:01 PM *local time*.
> > >>
> > >> From what I can see, the host machine time zone is irrelevant (UTC+2).
> > >>
> > >>I don't know where the timezone mixup comes from, though. Running
> > >>`(get-date)` in the powershell at this point correctly returns the local
> > >>time (7:08 AM). I guess during injection the time is in UTC, and
> > >>schtasks.exe has no awareness of timezones?
> > >
> > >Right, I think there is a timezone disagreement between how we format the 
> > >timestamp and how schtasks.exe takes it.
> > >
> > >What matters here is the /ST (start time) flag.
> > >
> > >Today we have (in the common submodule):
> > >
> > >   add "$d = (get-date).AddSeconds(120)";
> > >   add "$dtfinfo = 
> > > [System.Globalization.DateTimeFormatInfo]::CurrentInfo";
> > >   add "$sdp = $dtfinfo.ShortDatePattern";
> > >   add "$sdp = $sdp -replace 'y+', ''";
> > >   add "$sdp = $sdp -replace 'M+', 'MM'";
> > >   add "$sdp = $sdp -replace 'd+', 'dd'";
> > >   add "schtasks.exe /Create /SC ONCE `";
> > >   add "  /ST $d.ToString('HH:mm') /SD $d.ToString($sdp) `";
> > >   add "  /RU SYSTEM /TN Firstboot-qemu-ga `";
> > >   add (sprintf "  /TR \"C:\\%s /forcerestart /qn /l+*vx C:\\%s.log\""
> > >  msi_path msi_path);
> > >
> > >Note that for the /ST option's argument, we only perform the following 
> > >steps:
> > >
> > >   $d = (get-date).AddSeconds(120)
> > >
> > >   /ST $d.ToString('HH:mm')
> > >
> > >This actually goes back to commit dc66e78fa37d ("windows: delay 
> > >installation of qemu-ga MSI", 2020-03-10). The timestamp massaging we've 
> > >since done only targeted the /SD (start date) option, not the start time 
> > >(/ST) one!
> > >
> > >So the problem may be that
> > >
> > >   (get-date).AddSeconds(120).ToString('HH:mm')
> > >
> > >formats the hour:minute timestamp in UTC (why though?), but the /ST option 
> > >takes hour:minute in local time.
> > >
> > >Interestingly, DateTime objects seem to have a "Kind" property, which may 
> > >be Utc, Local, or Unspec.
> > >
> > >https://learn.microsoft.com/en-us/dotnet/api/system.datetime.kind
> > >
> > >It seems to be used when converting from 

Re: [Libguestfs] regression: file does not understand the -S option

[Daniel P . Berrangé]

On Thu, Sep 21, 2023 at 12:25:21PM +0100, Richard W.M. Jones wrote:
> On Wed, Sep 20, 2023 at 11:42:55PM +0200, Olaf Hering wrote:
> > Recently a commit was added to call 'file -zSb' instead of 'file -zb'.
> > 
> > This causes a regression on Leap 15 (but not on Tumbleweed), because
> > file 5.32 does not understand the -S option.
> > 
> > How can this be fixed properly, to handle both cases either at runtime
> > or at buildtime?
> 
> The background to this was:
> 
>   https://github.com/libguestfs/libguestfs/issues/100
> 
> It took a while to work out what was going on in the original bug
> report, but it turned out that Arch (IIRC) enabled the seccomp feature
> in the 'file' command.  This filters what system calls 'file' is
> allowed to make, which strengthens security as 'file' is often run on
> untrusted inputs.
> 
> Unfortunately the seccomp rules for 'file' don't cope with running
> external programs (ie. 'file -z' which runs zcat).  We filed a bug to
> try to get that fixed:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=2148753
>   https://bugs.astron.com/view.php?id=406
> 
> but the fix to seccomp policy was rejected recently in both Fedora &
> upstream.

Their rationale in that bug makes no sense.

Not allowing 'clone+execve' etc is correct when '-z' is NOT specified
by the user. No argument there.

If '-z' is specified then adding clone+execve etc is the only way it
can work. They should apply a different seccomp filter for '-z' only
which includes clone+execve, etc.  Telling people to turn off seccomp
entirely in order to use '-z' is even worse for security than just
allowing clone+execve.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files

[Daniel P . Berrangé]

On Thu, Jul 27, 2023 at 11:07:07AM -0500, Eric Blake wrote:
> On Thu, Jul 27, 2023 at 04:00:25PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 26, 2023 at 12:50:03PM -0500, Eric Blake wrote:
> > > I ran:
> > >   gofmt -s -w $(git ls-files -- '**/*.go')
> > > 
> > > then touched up a few comments in test 590 where it mis-interpreted
> > > our intentions of having a single sentence that occupies more than 80
> > > columns.
> > 
> > Touching up manually isn't a good idea if you want to enforce
> > 'go fmt' compliance, as you'll want contributors to be able to
> > set their editors to automatically run 'go fmt' upon save and
> > submit as is.
> 
> Maybe I need to improve my wording here.  The existing format was
> ambiguous enough that gofmt picked a different layout than our
> original intent; manually tweaking was done to first get the comments
> in a format that better matched our original intent, at which point
> gofmt no longer mangled the comment.  Either way, the end result is
> still something that gofmt approves of.  And yes, the goal is that in
> the future, we will avoid commits where we add code to .go files that
> gofmt does not like.
> 
> > 
> > > Most of the changes are whitespace fixes (consistent use of TAB,
> > > altering the whitespace around operators), using preferred ordering
> > > for imports, and eliding excess syntax (unneeded () or ;).
> > > 
> > > This patch only adjusts files directly in git control.  Later patches
> > > will tackle (most) Go style problems in the generated files, then
> > > automate a way to ensure we don't regress.
> > 
> > libvirt-ci provides a container image for running & reporting
> > go fmt violations. Since you're using lcitool manifest, just
> > make this change
> > 
> > $ git diff ci/manifest.yml
> > diff --git a/ci/manifest.yml b/ci/manifest.yml
> > index f7b1daf..a5f3e66 100644
> > --- a/ci/manifest.yml
> > +++ b/ci/manifest.yml
> > @@ -6,6 +6,7 @@ gitlab:
> >project: libnbd
> >jobs:
> >  check-dco: false
> > +go-fmt: true
> >  
> >  targets:
> >alpine-315: x86_64
> > 
> > 
> > and re-generate, and it'll enforce style on *.go tree-wide.
> > 
> > libvirt-ci manifest also supports 'cargo-fmt' for rust, 'black' for
> > python, and 'clang-format' for C too, enabled in the same way.
> 
> Nice. That is shorter than my patch 7/7 approach.  It does two slight
> drawbacks, though:
> 
> - It only covers checked-in .go files, whereas my patch can also
> enforce formatting of generated .go files, if we want the generator to
> be complex enough to guarantee well-formatted output

Yep, only committed files.

> - failures are not detected at local 'make check' time, but rather
> delayed until a CI run. That's fine if your development model is pull
> request first; but a bit painful if it is commit email patches first
> and then see how CI fares.

You can do both - in libvirt we have meson test check style, but in CI
we have style checks split out into a separate job, as it makes it
nicer when browsing CI failures to separate reporting of functional
failures from style violations.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files

[Daniel P . Berrangé]

On Thu, Jul 27, 2023 at 09:50:01AM -0500, Eric Blake wrote:
> On Thu, Jul 27, 2023 at 01:52:00PM +0200, Laszlo Ersek wrote:
> > On 7/26/23 19:50, Eric Blake wrote:
> > > I ran:
> > >   gofmt -s -w $(git ls-files -- '**/*.go')
> > > 
> > > then touched up a few comments in test 590 where it mis-interpreted
> > > our intentions of having a single sentence that occupies more than 80
> > > columns.
> > > 
> > > Most of the changes are whitespace fixes (consistent use of TAB,
> > > altering the whitespace around operators), using preferred ordering
> > > for imports, and eliding excess syntax (unneeded () or ;).
> > > 
> > > This patch only adjusts files directly in git control.  Later patches
> > > will tackle (most) Go style problems in the generated files, then
> > > automate a way to ensure we don't regress.
> > > 
> > > Signed-off-by: Eric Blake 
> > > ---
> > > +++ b/golang/libnbd_620_stats_test.go
> > > @@ -124,16 +124,16 @@ func Test620Stats(t *testing.T) {
> > >   t.Fatalf("%s", err)
> > >   }
> > > 
> > > - if bs2 != bs1 + 28 {
> > > + if bs2 != bs1+28 {
> > >   t.Fatalf("unexpected value for bs2")
> > >   }
> 
> > > - if cr3 != cr2 + slop {
> > > + if cr3 != cr2+slop {
> > >   t.Fatalf("unexpected value for cr3")
> > >   }
> > >  }
> > 
> > I've done nearly nothing in Go, and generally the updates look
> > justified... but the changes to "libnbd_620_stats_test.go" are hideous.
> > Who on Earth considers
> > 
> >   if cr3 != cr2+slop
> > 
> > superior to
> > 
> >   if cr3 != cr2 + slop
> > 
> > ???
> > 
> > *shudder*
> 
> That's gofmt for you.  I was surprised by it too.
> 
> > 
> > Question in passing: if we wrote
> > 
> >   if cr3 != (cr2 + slop)
> > 
> > would gofmt contract that too to
> > 
> >   if cr3 != (cr2+slop)
> 
> Equally surprising, gofmt (at least from golang-bin-1.20.6-1.fc38)
> does NOT remove the spaces from that format, nor does it complain that
> the () are spurious.  Of course, future gofmt may change style again,
> but now we have a regression test in place to catch that.  And we may
> be faced with decisions down the road on how to pacify cases where
> gofmt itself changes styles from one release of the language to the
> next (if that happens, that would be a strong argument that we should
> remove gofmt from a gating 'make check' test, and instead incorporate
> it as an optional 'make dist' step - if we reach a point where there
> is no longer "the" canonical formatting, we shouls still favor
> tarballs created with "a" canoncial format).

snip

> as well as amending the commit message to mention the manual touchup
> needed to work around the (in our opinion, misguided) advice from Go.

IMHO that is defeating the point of using 'go fmt'. The value of 'go fmt'
is that developers & projects no longer need to endlessly debate style.
There is a consistent format across every project in the world that is
written in Go. The value of this consistency outweighs the negatives
of the few cases where formatting isn't the preference of individual
developers.

People will usually have their editors set to auto run 'go fmt' on
any '.go' file, and telling them to manually alter certain style
rules the project didn't like is an unecessary burden.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH 5/7] golang: Use 'gofmt' style recommendations on manual files

[Daniel P . Berrangé]

On Wed, Jul 26, 2023 at 12:50:03PM -0500, Eric Blake wrote:
> I ran:
>   gofmt -s -w $(git ls-files -- '**/*.go')
> 
> then touched up a few comments in test 590 where it mis-interpreted
> our intentions of having a single sentence that occupies more than 80
> columns.

Touching up manually isn't a good idea if you want to enforce
'go fmt' compliance, as you'll want contributors to be able to
set their editors to automatically run 'go fmt' upon save and
submit as is.

> Most of the changes are whitespace fixes (consistent use of TAB,
> altering the whitespace around operators), using preferred ordering
> for imports, and eliding excess syntax (unneeded () or ;).
> 
> This patch only adjusts files directly in git control.  Later patches
> will tackle (most) Go style problems in the generated files, then
> automate a way to ensure we don't regress.

libvirt-ci provides a container image for running & reporting
go fmt violations. Since you're using lcitool manifest, just
make this change

$ git diff ci/manifest.yml
diff --git a/ci/manifest.yml b/ci/manifest.yml
index f7b1daf..a5f3e66 100644
--- a/ci/manifest.yml
+++ b/ci/manifest.yml
@@ -6,6 +6,7 @@ gitlab:
   project: libnbd
   jobs:
 check-dco: false
+go-fmt: true
 
 targets:
   alpine-315: x86_64


and re-generate, and it'll enforce style on *.go tree-wide.

libvirt-ci manifest also supports 'cargo-fmt' for rust, 'black' for
python, and 'clang-format' for C too, enabled in the same way.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Proposal to delete the mirrors github.com/libguestfs/libnbd & /nbdkit

[Daniel P . Berrangé]

On Wed, Jun 07, 2023 at 07:18:46PM +0100, Richard W.M. Jones wrote:
> I don't want to actually link to them to avoid giving them link-karma,
> but the old repositories / now mirrors at:
> 
> github.com/libguestfs/libnbd
> github.com/libguestfs/nbdkit
> 
> stopped mirroring the true repositories:
> 
> https://gitlab.com/nbdkit/libnbd
> https://gitlab.com/nbdkit/nbdkit
> 
> some months ago.  It is apparently connected to github changing their
> host key.  We tried unsuccessfully to fix it, which I wasn't able to do.

I'm happy to have a look at that again if interested.

> I propose we just delete those mirrors instead.  Any objections?

I'm not sure deleting is a good idea wrt forks

  
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/what-happens-to-forks-when-a-repository-is-deleted-or-changes-visibility

  "When you delete a public repository, one of the existing
   public forks is chosen to be the new upstream repository.
   All other repositories are forked off of this new upstream
   and subsequent pull requests go to this new upstream
   repository."

I don't think we'd want some arbitrary fork to be "promoted" as the
upstream, as it'd give a very misleading impression.

It is probably better to change the description in the repo to alert
peolpe that it is stale, point them to gitlab, then mark it as
'archived' in github.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Wed, Mar 22, 2023 at 12:13:49PM +0100, Laszlo Ersek wrote:
> On 3/22/23 11:42, Laszlo Ersek wrote:
> 
> > Now the "podman build -f ci/containers/alpine-edge.Dockerfile -t
> > libnbd-alpine-edge" command is failing with a different error message --
> > the download completes, but the internal relinking etc fails due to
> > permission errors, which I don't understand. I've asked Martin for comments.
> >
> > Meanwhile, your other email (= just download the prebuilt container from
> > gitlab) could help!
> 
> Unfortunately, I got the same failure:
> 
> podman run -it --rm --userns=keep-id -v .:/repo:z -w /repo \
> registry.gitlab.com/nbdkit/libnbd/ci-alpine-edge:latest \
> bash
> 
> > Trying to pull registry.gitlab.com/nbdkit/libnbd/ci-alpine-edge:latest...
> > Getting image source signatures
> > Copying blob 88ecf269dec3 done
> > Copying blob 0ded2f83af0e done
> > Copying config a3b4bffb18 done
> > Writing manifest to image destination
> > Storing signatures
> > Error relocating /usr/lib/libreadline.so.8: RELRO protection failed: 
> > Permission denied
> > Error relocating /lib/ld-musl-x86_64.so.1: RELRO protection failed: 
> > Permission denied
> > Error relocating /usr/lib/libncursesw.so.6: RELRO protection failed: 
> > Permission denied
> > Error relocating /bin/bash: RELRO protection failed: Permission denied

This looks relevant:

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

and suggests

  restorecon -R ~/.local/share/containers/storage/overlay*

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Tue, Mar 21, 2023 at 03:56:22PM +0100, Laszlo Ersek wrote:
> On 3/21/23 15:05, Eric Blake wrote:
> > On Tue, Mar 21, 2023 at 07:04:59AM +0100, Laszlo Ersek wrote:
> >> On 3/20/23 20:41, Eric Blake wrote:
> >>> On Sun, Mar 19, 2023 at 10:41:37AM +0100, Laszlo Ersek wrote:
>  This is version 4 of the following sub-series:
> 
>  [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()
>  [libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe 
>  execvpe()
> 
> > ...
> >>>
> >>
> >> Series merged as commit range 742cbd8c7adc..0b7172b3cffa.
> >
> > I see you already fixed one pipeline failure due to some gcc versions
> > being more picky about function __attribute__ placement than others.
> > The remaining failures are with alpine Linux, where /bin/expr comes
> > from busybox instead of coreutils, and has the unfortunate quality of
> > having its behavior dependent on argv[0].
> 
> :)
> 
> I just wanted to announce on-list that I disabled
> "lib/test-fork-safe-execvpe.sh" on Alpine Linux because of this :)
> 
> >
> > Starting from a clean clone, I reproduced it locally with:
> >
> > $ git diff
> > diff --git i/lib/test-fork-safe-execvpe.sh w/lib/test-fork-safe-execvpe.sh
> > index 838bac9..4b3700c 100755
> > --- i/lib/test-fork-safe-execvpe.sh
> > +++ w/lib/test-fork-safe-execvpe.sh
> > @@ -18,7 +18,7 @@
> >
> >  . ../tests/functions.sh
> >
> > -set -e
> > +set -ex
> >
> >  # Determine the absolute pathname of the execvpe helper binary. The 
> > "realpath"
> >  # utility is not in POSIX, but Linux, FreeBSD and OpenBSD all have it.
> > @@ -155,7 +155,7 @@ success()
> >
> >  # Create a temporary directory and change the working directory to it.
> >  tmpd=$(mktemp -d)
> > -cleanup_fn rm -r -- "$tmpd"
> > +#cleanup_fn rm -r -- "$tmpd"
> >  cd "$tmpd"
> >
> >  # If the "file" parameter of execvpe() is an empty string, then we must 
> > fail --
> >
> > $ podman build -f ci/containers/alpine-edge.Dockerfile -t libnbd-alpine-edge
> > $ podman run -it --rm --userns=keep-id -v .:/repo:z -w /repo 
> > libnbd-alpine-edge bash
> > $ ./configure
> > $ make check
> > $ grep tmpd= lib/test-suite.log
> > + tmpd=/tmp/tmp.EMgKeF
> > $ /tmp/tmp.EMgKeF/bin/f 1 + 1
> > f: applet not found
> > 0b748c9fe495:~$
> >
> > So it looks like we need some way to work around busybox' insistance
> > that argv[0] determines which applet to run.
> 
> I couldn't come up with a reproducer like yours. I couldn't figure out
> how to *quickly* get an interactive Alpine Linux environment with the
> test failing, and I also couldn't figure out how to trigger "verbose"
> test runs on gitlab, without polluting the master branch with debug
> patches. (I tried forking the project in my own space on gitlab, and
> pushed a debug patch with just "set -x" onto a non-master branch *there*
> -- but CI didn't start in response to that.)
> 
> So, I only installed a new Alpine Linux virtual machine -- what a pain
> *that* was --, and investigated what /usr/bin/expr was. When I found it
> was a symlink to /bin/busybox, I started looking for Alpine Linux
> specific tweaks that could replace busybox (in this role) with a real
> binary executable "expr" utility.
> 
> I was relieved to find the following wiki article:
> 
>   https://wiki.alpinelinux.org/wiki/How_to_get_regular_stuff_working
> 
> which promised -- I thought anyways -- a real coreutils package.
> 
> Imagine my dismay when I found that, after installing coreutils with apk
> in the Alpine Linux VM, the symlink stayed in place, only its target
> binary changed from "/bin/busybox" to "coreutils". Well done, Alpine
> Linux, well done.
> 
> So, no, this mess (= Alpine Linux) is not salvageable. The
> "lib/test-fork-safe-execvpe.sh" script depends on "expr" being
> functional under the name "f". And yes I want "f" to be a
> single-character filename; otherwise the nice tabular formatting of the
> script falls apart (or produces overlong lines).
> 
> As last step, I learned about ci/skipped_tests, and used it to disable
> the test on alpine linux.
> 
> The latest pipeline passed:
> .
> 
> Either way, I wanted to highlight the following commits on the list:
> 
> 1  b29ff42e5d00 lib: account for realpath deficiency on some platforms
> 2  65631e5dfff5 lib/utils: try to placate attribute placement restriction 
> from gcc
> 3  4cae20ccefaf Revert "lib: account for realpath deficiency on some 
> platforms"
> 4  f5a065aa3a9c ci: skip "lib/test-fork-safe-execvpe.sh" on Alpine Linux

IIUC, that skips the test in CI, but if a developer or downstream user
runs 'make check' won't they still execute test-fork-safe-execvpe.sh ?
IOW, will the next release of libnbd have a broken test when it gets
imported into the Alpine distro ?


FWIW, in terms of CI I'd suggest that skipping Alpine is something that
is an especially bad idea. Alpine is the only distro in CI that uses
Musl instead of GLibc, and Alpine also does 

Re: [Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Tue, Mar 21, 2023 at 09:05:12AM -0500, Eric Blake wrote:
> 
> $ podman build -f ci/containers/alpine-edge.Dockerfile -t libnbd-alpine-edge

You usually shouldn't need this step when reproducing a problem
on CI. Instead just use the container that gitlab has published
under the project's registry

For any job eg

https://gitlab.com/nbdkit/libnbd/-/jobs/3973002673

you'll see at about line 7/8 in the log a line like

 Using Docker executor with image 
registry.gitlab.com/nbdkit/libnbd/ci-alpine-edge:latest

That image URL is accessible remotely on your local dev machine eg

  podman run -it registry.gitlab.com/nbdkit/libnbd/ci-alpine-edge:latest

You should only need to do 'podman build' if you're debugging a suspected
problem with the container image itself.

> $ podman run -it --rm --userns=keep-id -v .:/repo:z -w /repo 
> libnbd-alpine-edge bash
> $ ./configure
> $ make check
> $ grep tmpd= lib/test-suite.log 
> + tmpd=/tmp/tmp.EMgKeF
> $ /tmp/tmp.EMgKeF/bin/f 1 + 1
> f: applet not found
> 0b748c9fe495:~$ 
> 
> So it looks like we need some way to work around busybox' insistance
> that argv[0] determines which applet to run.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v4] lib/errors.c: Fix assert fail in exit path in multi-threaded code

[Daniel P . Berrangé]

On Thu, Mar 09, 2023 at 09:50:00AM +, Richard W.M. Jones wrote:
> When a highly multi-threaded program such as nbdcopy encounters an
> error, there is a race condition in the library which can cause an
> assertion failure and thus a core dump:
> 
> (1) An error occurs on one of the threads.  nbdcopy calls exit(3).
> 
> (2) In lib/errors.c, the destructor calls pthread_key_delete.
> 
> (3) Another thread which is still running also encounters an error,
> and inside libnbd the library calls set_error().
> 
> (4) The call to set_error() calls pthread_getspecific which returns
> NULL (since the key has already been destroyed in step (2)), and this
> causes us to call pthread_setspecific which returns EINVAL because
> glibc is able to detect invalid use of a pthread_key_t after it has
> been destroyed.  In any case, the error message is lost, and any
> subsequent call to nbd_get_error() will return NULL.
> 
> (5) We enter the %DEAD state, where there is an assertion that
> nbd_get_error() is not NULL.  This assertion is supposed to be for
> checking that the code called set_error() before entering the %DEAD
> state.  Although we did call set_error(), the error message was lost
> at step (4) and nbd_get_error() will always return NULL.  This
> assertion failure causes a crash.
> 
> There aren't any good ways to fix this, so I chose the same method as
> used by libvirt in this commit:
> 
> https://gitlab.com/libvirt/libvirt/-/commit/8e44e5593eb9b89fbc0b54fde15f130707a0d81e
> 
> (a) Use '-z nodelete' to prevent the library from being unloaded on
> dlclose().
> 
> (b) Do not call pthread_key_destroy (thus leaking the key).
> 
> (c) When threads exit they are still able to call free_errors_key
> because it is still present in memory.
> 
> Thanks: Daniel P. Berrangé, Eric Blake
> ---
>  configure.ac| 9 +
>  lib/Makefile.am | 1 +
>  lib/errors.c| 6 +-
>  3 files changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd v3] lib/errors.c: Fix assert fail in exit path in multi-threaded code

[Daniel P . Berrangé]

On Thu, Mar 09, 2023 at 08:44:51AM +, Richard W.M. Jones wrote:
> When a highly multi-threaded program such as nbdcopy encounters an
> error, there is a race condition in the library which can cause an
> assertion failure and thus a core dump:
> 
> (1) An error occurs on one of the threads.  nbdcopy calls exit(3).
> 
> (2) In lib/errors.c, the destructor calls pthread_key_delete.
> 
> (3) Another thread which is still running also encounters an error,
> and inside libnbd the library calls set_error().
> 
> (4) The call to set_error() calls pthread_getspecific which returns
> NULL (since the key has already been destroyed in step (2)), and this
> causes us to call pthread_setspecific which returns EINVAL because
> glibc is able to detect invalid use of a pthread_key_t after it has
> been destroyed.  In any case, the error message is lost, and any
> subsequent call to nbd_get_error() will return NULL.
> 
> (5) We enter the %DEAD state, where there is an assertion that
> nbd_get_error() is not NULL.  This assertion is supposed to be for
> checking that the code called set_error() before entering the %DEAD
> state.  Although we did call set_error(), the error message was lost
> at step (4) and nbd_get_error() will always return NULL.  This
> assertion failure causes a crash.
> 
> There aren't any good ways to fix this.  I chose to leak the
> pthread_key_t on the exit path.
> ---
>  lib/errors.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/errors.c b/lib/errors.c
> index 8b77650ef3..6fbfaacd34 100644
> --- a/lib/errors.c
> +++ b/lib/errors.c
> @@ -69,7 +69,11 @@ errors_key_destroy (void)
>  free (last_error->error);
>  free (last_error);
>}
> -  pthread_key_delete (errors_key);
> +
> +  /* We could do this, but that causes a race condition described here:
> +   * https://listman.redhat.com/archives/libguestfs/2023-March/031002.html
> +   */
> +  //pthread_key_delete (errors_key);
>  }

While I think this fixes the problem with pthread_setspecific crashing,
I believe this then opens apps up to the problem hit by libvirt with
destructors being run which don't exist in memory.

Consider a thread is running that has done something with libnbd which
caused allocate_last_error_on_demand() to run, which populates the
'errors_key' data for that thread. This thread is no longer doing anything
with libnbd, but the 'errors_key' data remains populated none the less
because that's just how thread locals work.

Now nothing at all is using libnbd.so, so some thread calls dlclose(),
causing libnbd.so to be unmapped from memory. Since we've not called
pthread_key_delete, the thread local still exists and, crucially, so
does its registered destructor function.

The thread mentioned in the previous paragraph now terminates and this
causes the destructor function 'free_errors_key' to be run except
this function isn't mapped in memory any more. This will cause the
program to crash.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH libnbd] lib/errors.c: Fix assert fail in exit path in multi-threaded code

[Daniel P . Berrangé]

On Wed, Mar 08, 2023 at 03:22:29PM -0600, Eric Blake wrote:
> On Wed, Mar 08, 2023 at 08:29:41PM +, Richard W.M. Jones wrote:
> > When a highly multi-threaded program such as nbdcopy encounters an
> > error, there is a race condition in the library which can cause an
> > assertion failure and thus a core dump:
> > 
> > (1) An error occurs on one of the threads.  nbdcopy calls exit(3).
> > 
> > (2) In lib/errors.c, the destructor calls pthread_key_delete.
> >
> 
> Note - POSIX says that calling pthread_key_delete() can leak memory.
> If the reason we are calling pthread_key_delete() is because libnbd
> was dlopen'd and is now being dlclose'd, and an app repeatedly reloads
> libnbd, causes an error, and then unloads libnbd, this leak could grow
> to be rather sizeable.  But that is a rather unlikely scenario, and
> safely coordinating cleanup is hard when we consider both normal
> thread exits (where the key destructor runs if the key is not yet
> deleted) and module unloads (where pthread_key_delete skips key
> destructors).
> 
> > (3) Another thread which is still running also encounters an error,
> > and inside libnbd the library calls set_error().
> > 
> > (4) The call to set_error() calls pthread_getspecific which returns
> > NULL (since the key has already been destroyed in step (2)), and this
> 
> This is undefined behavior per POSIX.  It is not safe to call
> pthread_getspecific() on a destroyed key (even though glibc lets us
> get away with it by giving us EINVAL).
> 
> > causes us to call pthread_setspecific which returns EINVAL because
> > glibc is able to detect invalid use of a pthread_key_t after it has
> > been destroyed.  In any case, the error message is lost, and any
> > subsequent call to nbd_get_error() will return NULL.
> 
> Indeed - once we call pthread_key_delete() in step 2, ANY further use
> of the key is going to cause undefined behavior (hopefully EINVAL
> failures, but worse behavior is also possible).
> 
> One possible idea: have a non-thread-local global that starts life
> false, but which errors_free() atomically sets to true prior to
> calling pthread_key_delete().  All other functions which reference
> errors_key check that the global is still false before using
> pthread_{get,set}specific.
> 
> Another idea: have a counter of the number of threads that have set a
> thread-local error but not yet exited.  Increment the counter any time
> allocate_last_error_on_demand() says a new thread is triggering an
> error, and decrement the counter any time free_errors_key() calls the
> destructor at the end of a thread.  Only when all threads with an
> allocated error have exited will the counter reach zero, so only the
> last thread to clean up needs to call pthread_key_delete().  The
> counter might not reach zero before the process exits, but that's okay
> (if the reason the module is being unloaded is because the process is
> going away, cleanup is less important), although I don't know if
> valgrind would complain.  In fact, POSIX suggests:
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_key_delete.html
> "For an application to know that it is safe to delete a key, it has to
> know that all the threads that might potentially ever use the key do
> not attempt to use it again. For example, it could know this if all
> the client threads have called a cleanup procedure declaring that they
> are through with the module that is being shut down, perhaps by
> setting a reference count to zero."

IMHO the suggested ways to solve this problem are all complex enough
that I would not have much confidence in them even once implemented.

The use of pthread thread-data destructors is inherantly dangerous
when combined with dlclose, to the extent that the best course of
action is to simply forbid this scenario.

Libvirt hit this problem with pthread data destructors and dlclose()
many years ago and we merely blocked the functional problem by linking
libvirt with '-z nodelete'...

  commit 8e44e5593eb9b89fbc0b54fde15f130707a0d81e
  Author: Daniel P. Berrangé 
  Date:   Thu Sep 1 17:57:06 2011 +0100

Prevent crash from dlclose() of libvirt.so

When libvirt calls virInitialize it creates a thread local
for the virErrorPtr storage, and registers a callback to
cleanup memory when a thread exits. When libvirt is dlclose()d
or otherwise made non-resident, the callback function is
removed from memory, but the thread local may still exist
and if a thread later exists, it will invoke the callback
and SEGV. There may also be other thread locals with callbacks
pointing to libvirt code, so it is in general never safe to
unload libvirt.so from memory once initialized.

To allo

Re: [Libguestfs] [V2V PATCH 0/5] Bring support for virtio-scsi back to Windows

[Daniel P . Berrangé]

On Thu, Feb 23, 2023 at 11:43:38AM +0100, Laszlo Ersek wrote:
> On 2/22/23 19:20, Andrey Drobyshev wrote:
> > Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439
> > ("Remove guestcaps_block_type Virtio_SCSI") support for installing
> > virtio-scsi driver is missing in virt-v2v.  AFAIU plans and demands for
> > bringing this feature back have been out there for a while.  E.g. I've
> > found a corresponding issue which is still open [1].
> > 
> > The code in b28cd1dc, f0afc439 was removed due to removing the old in-place
> > support.  However, having the new in-place support present and bringing
> > this same code (partially) back with several additions and improvements,
> > I'm able to successfully convert and boot a Win guest with a virtio-scsi
> > disk controller.  So please consider the following implementation of
> > this feature.
> > 
> > [1] https://github.com/libguestfs/virt-v2v/issues/12
> 
> (Preamble: I'm 100% deferring to Rich on this, so take my comments for
> what they are worth.)
> 
> In my opinion, the argument made is weak. This cover letter does not say
> "why" -- it does not explain why virtio-blk is insufficient for *Virtuozzo*.
> 
> Second, reference [1] -- issue #12 -- doesn't sound too convincing. It
> writes, "opinionated qemu-based VMs that exclusively use UEFI and only
> virtio devices". "Opinionated" is the key word there. They're entitled
> to an opinion, they're not entitled to others conforming to their
> opinion. I happen to be opinionated as well, and I hold the opposite view.

I think that issue shouldn't have used the word 'opionated' as
it gives the wrong impression that the choice is somewhat
arbitrary and interchangable. I think there are rational reasons
why virtio-scsi is the better choice that they likely evaluated.

The main tradeoffs for virtio-blk vs virtio-scsi are outlined
by QEMU maintainers here:

  https://www.qemu.org/2021/01/19/virtio-blk-scsi-configuration/

TL;DR virtio-blk is preferred for maximum speed, while virtio-scsi
is preferred if you want to be able to add lots of disks free of
worrying about PCI slot availability.

I can totally understand why public clouds would pick to only
support virtio-scsi. The speed benefits of virtio-blk are likely
not relevant / visible to their customers because when you're
overcommiting hosts to serve many VM, the bottleneck is almost
certainly somewhere other than the guest disk device choice.

The ease of adding many disks is very interesting to public clouds
though, especially if the VM has many NICs already taking up PCI
slots. Getting into adding PCI bridges adds more complexity than
using SCSI. OpenStack maintainers have also considered preferring
virtio-scsi over virtio-blk for specifically this reason in the
past and might not have even used virtio-blk at all, if it were
not for the backcompat concerns.

So I'd say it is a reasonable desire to want to (optionally) emit
VMs that are setup for use of virtio-scsi instead of virtio-blk


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Tue, Feb 21, 2023 at 11:59:30PM +0100, Laszlo Ersek wrote:
> On 2/21/23 19:04, Daniel P. Berrangé wrote:
> 
> > AFAIK, libnbd/nbdkit haven't made a statement about what platforms
> > they aim to target. In my response I'm more or less assuming though
> > that you would only care about similar modern platforms to QEMU/libvirt,
> > and thus POSIX conformance would not be needed in all areas. Maybe
> > libnbd/nbdkit want to be more explicit about what they target as
> > platforms to make the portability requirements clear to contributors ?
> 
> libnbd's README.md requires
> 
> * Linux, FreeBSD or OpenBSD.
>   Other OSes may also work but we have only tested these three.
> * GCC or Clang
> * GNU make
> * bash
> * [...]
> 
> nbdkit's requires
> 
> * Linux, macOS, Windows, FreeBSD, OpenBSD or Haiku
> * GCC or Clang
> * bash
> * GNU make
> * [...]
> 
> To me, anything beyond Linux on those OS lists is entirely untestable
> *locally*, hence my reliance on POSIX. CI is a horrible way (compared to
> a published technical standard) to figure out whether each individual
> interface works as needed everywhere, even just across this small set of
> OSes. Having to look at multiple OS manual pages is just slightly less
> horrible (and I consider those less trustworthy than POSIX; see again
> the conflict between the linux man pages and the glibc documentation
> from GNU). The POSIX people have done *huge work* to save us that effort.

FWIW, my (bitter) experience from both libvirt and QEMU is that unless
you have CI validating it, you don't have portability to a platform, no
matter how much effort the developers put in complying with standards
and/or manual pages. The reality of bizarre/broken platform impls always
gets in the way of good intentions.

FWIW, in terms of testing locally, all those platforms are trivial,
with the exception of macOS. For Windows, mingw toolchain will get
you through all compilation portability problems which is 90% of the
work. While you can use WINE for some functional testing, a real
Windows VM is better.  For the *BSD / Haiku, a VM can be spun up
with KVM quite easily.

macOS is the real pain point because of its restrictive EULA
meaning you basically have to buy it. It makes it very much a second
class citizen for developers, unless they happen to have personal
apple hardware.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Tue, Feb 21, 2023 at 06:53:39PM +0100, Laszlo Ersek wrote:
> 
> More in general, this lesson tells me that POSIX is effectively
> irrelevant -- which is quite sad in itself; the bigger problem however
> is that *nothing replaces it*. If the one formal standard we have for
> portability does not reflect reality closely enough, and we need to rely
> on personal experience with various platforms, then we're back to where
> we were *before* POSIX. That is, having to check several separate
> documentation sets, and testing each API on every relevant platform in
> *each project* where the API is used. The idea is "ignore POSIX, care
> about Linux / modern systems only", but then it turns out those modern
> systems *do* differ sufficiently that extracting a common programming
> base *would* be useful. It's just that POSIX is not that common base;
> more precisely, there is no formalized, explicit common base. I guess
> "whatever passes CI" is the common base. That's... terrible, and it
> makes me seriously question if I want to program userspace in C at all.

FWIW, I wouldn't say that POSIX is irrelevant in general. If you
are trying to maximimse portability it is worth paying attention to.

Rather I'd say that maintainers of projects may be opinionated about
which platforms they wish to support, to eliminate the burden of caring
about platforms which have few if any users in the modern world.

In libvirt and QEMU context we set explicit platform support targets:

  https://libvirt.org/platforms.html
  https://www.qemu.org/docs/master/about/build-platforms.html

which effectively limit us to only care about actively developed
OS from the last ~4 years, and even then only fairly mainstream
stuff. We don't care about a hobbyist/toy UNIX OS. The burden is
on other OS to attain compatibility with mainstream modern OS,
not for apps to adapt to osbscure feature-poor platforms. 

With this attitude, we don't care about compliance with countless
obsolete vendor's UNIX platforms, and thus many of the edge cases
that POSIX worries about can be ignored. This frees the project
maintainers time to focus on work that benefits a broader set of
users.

>From this, libvirt/QEMU could both explicitly decide to not care
about any C compilers other than CLang/GCC.  Vendor compilers and
most especially MSVC are out of scope. CLang/GCC are able to support
any of the OS platforms we target. This frees us from caring about
ISO C standards, letting us use GNU extensions.


AFAIK, libnbd/nbdkit haven't made a statement about what platforms
they aim to target. In my response I'm more or less assuming though
that you would only care about similar modern platforms to QEMU/libvirt,
and thus POSIX conformance would not be needed in all areas. Maybe
libnbd/nbdkit want to be more explicit about what they target as
platforms to make the portability requirements clear to contributors ?


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Tue, Feb 21, 2023 at 05:03:23PM +0100, Laszlo Ersek wrote:
> On 2/21/23 13:08, Daniel P. Berrangé wrote:
> > On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
> >> execvp() [01] is powerful:
> >>
> >> - it performs PATH search [02] if necessary,
> >>
> >> - it falls back to executing the file with the shell as a shell
> >>   script in case the underlying execv() or execve() fails with
> >>   ENOEXEC.
> >>
> >> However, execvp() is not async-signal-safe [03], and so we shouldn't
> >> call it in a child process forked from a multi-threaded parent
> >> process [04], which the libnbd client application may well be.
> >
> > Is that actually a problem in the real world ?  There are various
> > things not technically listed as async-signal-safe by POSIX, which
> > all sane impls none the less make safe, or are practically safe in
> > any sane program using them.
> 
> By my count, it's virtually impossible to implement exec*p*() and
> friends without calling malloc() internally. That's the case even if you
> can get PATH in there without any problems. In turn, malloc() relies on
> libc-internal mutex(es) or other synchronization mechanisms, so that
> multiple threads can call malloc() / free() etc simultaneously, for
> managing the one shared address space of the process.
> 
> The problem is with calling fork() in a multi-threaded process. The
> child process created by fork() only has one thread -- the thread
> returning from fork(). All other threads simply "disappear", as viewed
> from the child process's perspective. This means that, if one of those
> "other" (disappearing) threads was in the middle of malloc(), holding
> for example a mutex, then in the child process, that mutex will appear
> as acquired, but no thread will own it -- so no thread will ever release
> it. Then, when the one thread returning from fork() in the child process
> calls exec*p*(), the malloc() call internal to exec*p*() will deadlock
> on that mutex.
> 
> Rich mentioned that libnbd had actually encountered a bug of this kind,
> just not specifically in exec*p*().
> 
> So the "async-signal-safe" term is a bit misleading here; I think what
> POSIX actually means is a kind of general reentrancy. Anyway, the POSIX
> language does use "async-signal-safe" when discussing this topic of
> threads, in the fork() specification.
> 
> Now, the exec*p*() manual at
> 
>   https://man7.org/linux/man-pages/man3/execvp.3.html
> 
> calls execlp(), execvp(), execvpe() "MT-Safe env", so you are right
> about at least Linux/glibc. I've (intentionally) not looked at the glibc
> implementation of execvp(), but I agree that, *if* we are happy with
> getenv() just because the linux/glibc manual calls it "MT-Safe env",
> *then* we could arguably be satisfied with execvp() as well.
> 
> I don't know if/how that applies to FreeBSD / OpenBSD though.
> 
>   https://man.freebsd.org/cgi/man.cgi?query=execvp
>   https://man.openbsd.org/execvp.3
> 
> These man pages do not seem to contain any of the strings "mt", "async",
> "signal", "safe", "thread".
> 
> > IIUC with execvp the risk would be that setenv makes any use of the
> > environment potentially unsafe,
> 
> to a smaller extent, yes
> 
> > and possibly execvp might use malloc which is also technically unsafe.
> 
> primarily this one, yes
> 
> > Both of these factors would technically make your replacement unsafe
> > too.
> 
> No, they wouldn't. That's the whole point. Both getenv() and malloc()
> are restricted to the _init API, which is called in the parent process,
> before fork(). The necessary information is prepared in a context
> structure, which the child only inherits. After fork(), the child calls
> a second API -- the effective execvpe() replacement -- which operates on
> the inherited context structure. The only functions called from this
> second API are write() and abort() -- indirectly, in case the new
> async-signal-safe assert() variant fails --, and execve(). All three of
> these functions are async-signal-safe.

Oh I missed that subtlety.

> 
> > Apps simply shouldn't ever call setenv once threads are created, and
> > malloc() is safe in any impl that is relevant these days.
> 
> FWIW, the linux/glibc manual states, in
> <https://man7.org/linux/man-pages/man2/fork.2.html>:
> 
> >*  After a fork() in a multithreaded program, the child can
> >   safely call only async-signal-safe functions (see
> >   signal-safety(7)) until such time as it calls ex

Re: [Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

[Daniel P . Berrangé]

On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
> execvp() [01] is powerful:
> 
> - it performs PATH search [02] if necessary,
> 
> - it falls back to executing the file with the shell as a shell script in
>   case the underlying execv() or execve() fails with ENOEXEC.
> 
> However, execvp() is not async-signal-safe [03], and so we shouldn't call
> it in a child process forked from a multi-threaded parent process [04],
> which the libnbd client application may well be.

Is that actually a problem in the real world ?  There are various
things not technically listed as async-signal-safe by POSIX, which
all sane impls none the less make safe, or are practically safe in
any sane program using them.

IIUC with execvp the risk would be that setenv makes any use of
the environment potentially unsafe, and possibly execvp might
use malloc which is also technically unsafe. Both of these
factors would technically make your replacement unsafe too.

Apps simply shouldn't ever call setenv once threads are created,
and malloc() is safe in any impl that is relevant these days.

> 
> Introduce three APIs for safely emulating execvp() with execve():
> 
> - nbd_internal_execvpe_init(): to be called in the parent process, before
>   fork(). Allocate and format candidate pathnames for execution with
>   execve(), based on PATH. Allocate a buffer for the longer command line
>   that the shell script fallback needs.
> 
> - nbd_internal_fork_safe_execvpe(): to be called in the child process. Try
>   the candidates formatted previously in sequence with execve(), as long
>   as execve() fails with errors related to pathname resolution /
>   inode-level file type / execution permission. Stop iterating on fatal
>   errors; if the fatal error is ENOEXEC, activate the shell script
>   fallback. As a small added feature, take the environment from an
>   explicit parameter (effectively imitating the GNU-specific execvpe()
>   function -- cf. commit dc64ac5cdd0b, "states: Use execvp instead of
>   execvpe", 2019-11-15).
> 
> - nbd_internal_execvpe_uninit(): to be called in the parent process, after
>   fork(). Release the resources allocated with
>   nbd_internal_execvpe_init().
> 
> The main idea with the above distribution of responsibilities is that any
> pre-scanning of PATH -- which was suggested by Eric Blake -- should not
> include activities that are performed by execve() anyway. I've considered
> and abandoned two approaches:
> 
> - During scanning (i.e., in nbd_internal_execvpe_init()), check each
>   candidate with access(X_OK) [05].
> 
>   I dropped this because the informative APPLICATION USAGE section of the
>   same specification [06] advises against it, saying "[a]n application
>   should instead attempt the action itself and handle the [EACCES] error
>   that occurs if the file is not accessible".
> 
> - During scanning, open each candidate with O_EXEC [07] until one open()
>   succeeds. Save the sole file descriptor for
>   nbd_internal_fork_safe_execvpe(). In the latter, call fexecve() [08],
>   which is free of all error conditions that necessitate iteration over
>   candidates. Still implement the ENOEXEC (shell script) fallback.
> 
>   I dropped this because (a) fexecve() is a quite recent interface
>   (highlighted by Eric); (b) for an application to open a file for
>   execution with fexecve(), it's more robust to pass O_EXEC to open() than
>   not to pass it, per APPLICATION USAGE [09], but on Linux/glibc, O_EXEC
>   does not seem supported, only O_PATH does [10].
> 
> Thus the chosen approach -- pre-generate filenames -- contains a  small
> TOCTTOU race (highlighted by Eric) after all, but it should be harmless.
> 
> Implementation-defined details:
> 
> - If PATH searching is necessary, but PATH is absent [01], then fall back
>   to confstr(_CS_PATH) [11], similarly to Linux/glibc execvpe() [12].
> 
>   If PATH is set but empty ("set to null") [02], or PATH is unset and
>   confstr(_CS_PATH) fails or returns no information or returns the empty
>   string, we fail the initial scanning (!) with ENOENT. This is consistent
>   with bash's behavior on Linux/glibc:
> 
>   $ PATH= ls
>   bash: ls: No such file or directory
> 
> Details chosen for unspecified behavior:
> 
> - Hardwire "/bin/sh" as the shell's pathname [01].
> 
> [01] https://pubs.opengroup.org/onlinepubs/9699919799/functions/execvp.html
> [02] 
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
> [03] 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03
> [04] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html
> [05] https://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html
> [06] 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html#tag_16_09_07
> [07] https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> [08] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html
> [09] 
> 

Re: [Libguestfs] Issue with downloading files whose path contains multi-byte utf-8 characters

[Daniel P . Berrangé]

On Mon, Feb 13, 2023 at 06:07:58PM +, Richard W.M. Jones wrote:
> On Sun, Feb 12, 2023 at 03:31:08PM +0200, Yonatan Shtarkman wrote:
> > Hey,
> > When downloading a file whose path contains multi-byte utf-8, libguestfs
> > sometimes crashes.
> > This reproduces when using python, and not when using guestfish.
> > 
> > Code to reproduce:
> > for i in range(2000):
> >     g.download ('/xxxó', '/tmp/1')
> 
> 'i' is not used inside the loop?  Or is this error intermittent?
> 
> > #0  raise (sig=) at ../sysdeps/unix/sysv/linux/raise.c:50
> > #1  0x77fac140 in  () at 
> > /lib/x86_64-linux-gnu/
> > libpthread.so.0
> > #2  0x76f77701 in _Py_INCREF (op=) at /usr/include/
> > python3.9/object.h:408
> > #3  guestfs_int_py_event_callback_wrapper
> >     (g=, flags=, array_len=0, array=0x0, 
> > buf_len=
> > 47, buf=0x113b8a0 "gs=0x0\r\ncommandrvf: udevadm --debug settle -E \303by",
> > event_handle=0, event=16, callback=0x72516600) at handle.c:137
> > #4  guestfs_int_py_event_callback_wrapper
> >     (g=, callback=0x72516600, event=16, event_handle=0,
> > flags=, buf=0x113b8a0 "gs=0x0\r\ncommandrvf: udevadm --debug
> > settle -E \303by", buf_len=47, array=0x0, array_len=0) at handle.c:104
> > #5  0x76e0076a in guestfs_int_call_callbacks_message (g=0xf31290, 
> > event
> > =16, buf=0x113b8a0 "gs=0x0\r\ncommandrvf: udevadm --debug settle -E \303by",
> > buf_len=47)
> >     at events.c:117
> > #6  0x76e1702e in guestfs_int_log_message_callback
> >     (g=g@entry=0xf31290, buf=0x113b8a0 "gs=0x0\r\ncommandrvf: udevadm 
> > --debug
> > settle -E \303by", len=len@entry=47) at proto.c:145
> > #7  0x76dfb759 in handle_log_message (g=g@entry=0xf31290, conn=
> > conn@entry=0x110e280) at conn-socket.c:395
> > #8  0x76dfbd63 in read_data (len=4, bufv=, connv=
> > , g=) at conn-socket.c:179
> > #9  read_data (g=0xf31290, connv=0x110e280, bufv=, len=4) at
> > conn-socket.c:142
> > #10 0x76e1764a in recv_from_daemon (buf_rtn=0x7fffd858, 
> > size_rtn=
> > 0x7fffd854, g=0xf31290) at proto.c:545
> > #11 guestfs_int_recv_from_daemon (g=g@entry=0xf31290, 
> > size_rtn=size_rtn@entry=
> > 0x7fffd854, buf_rtn=buf_rtn@entry=0x7fffd858) at proto.c:623
> > #12 0x76e17a5a in guestfs_int_recv
> >     (g=g@entry=0xf31290, fn=fn@entry=0x76e3b3e8 "download", 
> > hdr=hdr@entry=
> > 0x7fffd920, err=err@entry=0x7fffd8f0, xdrp=xdrp@entry=0x0, ret=
> > ret@entry=0x0)
> >     at proto.c:668
> > 
> > I debugged this issue and noticed that the appliance logs from commandrvf 
> > are
> > truncated, leading to parse failure (missing utf-8 additional bytes):
> > https://github.com/libguestfs/libguestfs/blob/master/python/handle.c#L134
> > UnicodeDecodeError: 'utf-8' codec can't decode byte 0x84 in position 0: 
> > invalid
> > start byte
> 
> So I thought we'd fixed this in:
> 
> https://github.com/libguestfs/libguestfs/commit/0ee02e0117527b86a31b2a88a14994ce7f15571f
> 
> This is specifically a Python API problem or would it affect
> the C API too?

The difference with any C API is that almost nothing at the C level will
be validating that the bytes are actually valid utf-8 sequences.

So the truncated data is unlikely to result in a fatal error. Python is
aggressively validating all bytes, and so you get a hard error from the
truncated UTF-8.  Other languages may vary, but I've not seen anything
that makes validation errors a failure in quite such an aggressive way
as python. The problems with decode exceptions have hit s many apps
using python over the past few years. Even worse if running in a C
locale as python will reject anything with 8th bit set as being outside
7-bit asciii, instead of being 8-bit clean in its stream handling.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH] lib: Choose q35 machine type for x86-64

[Daniel P . Berrangé]

On Thu, Feb 09, 2023 at 01:45:33PM +, Richard W.M. Jones wrote:
> This machine type is more modern than the older 'pc' type and as most
> qemu development is now focused there we expect it will perform and
> behave better.  In almost all respects this change should make no
> difference.

The key differences with q35 that have caused problems for
apps in the past

 - CDROMs must use 'sata' bus not 'ide'
 - PCI topology is completely different, so if you're
   trying to associate between the host side view of
   the config and the guest side view, your logic may
   need adapting
 - Hotplug of devices is limited to 1 device unless you
   pre-create a bunch of pcie-root-port devices

If you're already using 'virt' machine for aarch64 successfully
though, you should have already known about the last 2 points,
if they did indeed affect libguestfs.

The first point could conceivably affect the way to detect
devices, depending on what approach is used, but could equally
be a non issue.

I presume the libguestfs test suite would have complained if
there was any genuine problem with the change, since it is
pretty comphrensive.

> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2168578
> ---
>  lib/guestfs-internal.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
> index 07a2b9f617..4e9a103d78 100644
> --- a/lib/guestfs-internal.h
> +++ b/lib/guestfs-internal.h
> @@ -128,6 +128,9 @@ cleanup_mutex_unlock (pthread_mutex_t **ptr)
>  #define MAX_WINDOWS_EXPLORER_SIZE (4 * 1000 * 1000)
>  
>  /* Machine types. */
> +#if defined(__x86_64__)
> +#define MACHINE_TYPE "q35"
> +#endif
>  #ifdef __arm__
>  #define MACHINE_TYPE "virt"
>  #endif
> -- 
> 2.39.0
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
> 

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v] convert: linux: Require host cpu for all RHEL-alike >= 9

[Daniel P . Berrangé]

On Tue, Feb 07, 2023 at 10:20:14AM +, Richard W.M. Jones wrote:
> On Tue, Feb 07, 2023 at 08:56:06AM +0000, Daniel P. Berrangé wrote:
> > On Mon, Feb 06, 2023 at 12:49:41PM +, Richard W.M. Jones wrote:
> > >  (2) If the guest is RHEL family >= 9, use -cpu host / host-passthrough
> > > 
> > >  (3) Otherwise don't specify any -cpu or  section
> > > 
> > > The third option seems to cause qemu / libvirt to use qemu64.  However
> > > it has the advantage that the guest is migratable afterwards.
> > 
> > I vaguely recall in previous discussion that we decided not to
> > care if the guest is migratable or not, on the basis that wasn't
> > really domain knowledge of v2v nor an user request. Thus the
> > use of 'host-passthrough' rather than 'host-model' to give a
> > "perfect" match for the host, rather than approximation.
> 
> Discussion on the orignal patch:
> https://listman.redhat.com/archives/libguestfs/2022-April/thread.html#28711
> 
> Maybe we discussed migratability on IRC or on some previous thread,
> but all I can find there is the same commit message.  (OTOH I did
> review the patch and didn't query it, so ...)
> 
> I just posted a patch to change this from host-passthrough to
> host-model, because I think we don't need to hinder live migration if
> there's an easy way to do that.
> 
> > > There may be a case for changing this so:
> > > 
> > >  (a) Rule (2) is changed so we use host-model (for the libvirt target).
> > 
> > All hinges on whether users expect the output of v2v to be
> > migratable or not.
> > 
> > >  (b) Rule (3) is changed so we always specify a minimum CPU, eg.
> > >  for x86-64 guests, choose x86-64-v2 (is that an actual CPU model??)
> > 
> > In the case of x86-64-v2 it is the QEMU Nehalem model is a perfect
> > match, and is capable of running on both Intel and AMD hosts. This
> > is mostly luck though. For x86-64-v3 / -v4 ABIs there is no viable
> > model that will run on both AMD and Intel. So once there's a guest
> > OS that mandates -v3, we'll be back to wanting host-model/passthrough.
> 
> Is there any easy way to query libvirt to find out what is the best
> x86-64-vX compatible CPU available?  Sometimes we are connected to
> libvirt on the output side.

No, that's not a concept we expose. The x86-64-vX things are really
software ABIs aka the GCC -march / -mcpu flag arguments, and so
reporting about that kind of thing isn't especially in scope for
libvirt.

For QEMU we do however generate docs which show which CPUs can
support each x86 ABI - see the big colourful table here:

  https://www.qemu.org/docs/master/system/i386/cpu.html

and the script for that is here

  
https://gitlab.com/qemu-project/qemu/-/blob/master/scripts/cpu-x86-uarch-abi.py

I think it probably would be possible to query libvirt to ask for
the full expansion of the hypervisor host CPU featureset, because
ultimately the ABI is just declaring that features X, Y & Z must
exist as a minimum.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

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

[Daniel P . Berrangé]

On Tue, Feb 07, 2023 at 08:56:11AM +, Richard W.M. Jones wrote:
> 
> We worried about getenv's safety (lack of) for quite a while when we
> were writing libguestfs, which uses a lot more environment variables
> in many more places.  But we decided there was simply nothing we could
> do about it, and it was easier not to worry :-)

No one in their right mind will call setenv() after creating threads, and
if they do they deserve any breakage that happens :-) IMHO it is totally
reasonable to rely on getenv() being invariant, and declare any issues to
be someone else's problem to fix.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v] convert: linux: Require host cpu for all RHEL-alike >= 9

[Daniel P . Berrangé]

On Mon, Feb 06, 2023 at 12:49:41PM +, Richard W.M. Jones wrote:
> On Mon, Feb 06, 2023 at 12:37:30PM +0000, Daniel P. Berrangé wrote:
> > On Mon, Feb 06, 2023 at 12:19:40PM +, Richard W.M. Jones wrote:
> > > RHEL >= 9 and compatible distros like Rocky >= 9 will not boot using
> > > the default qemu CPU.  You will see an error at boot:
> > > 
> > >   Fatal glibc error: CPU does not support x86-64-v2
> > > 
> > > Instead you need to use -cpu host.
> > > 
> > > In commit f28757c6d1 ("convert_linux: set "gcaps_default_cpu = false"
> > > for x86_64 RHEL-9.0+ guests") we fixed this specifically for RHEL >= 9.
> > > 
> > > This commit extends the same fix to all RHEL family distros.
> > 
> > If v2v is going to use  host CPU for a bunch of distros, why not use
> > it for all distros ? What's the downside in -cpu host, instead of
> > the default qemu64 CPU ?
> > 
> > Even if the distro doesn't require x86_64-v2 ABI, they'll pretty much
> > all benefit from NOT using the default QEMU CPU model, if only for
> > the fact that they gain the 'aesni' feature which increases crypto
> > performance massively. 
> 
> I don't know what we should do, but the current code does this:
> 
>  (1) If a particular CPU model is specified by the source hypervisor
>  (eg VMware), use that.

Reasonable, if there's a match

>  (2) If the guest is RHEL family >= 9, use -cpu host / host-passthrough
> 
>  (3) Otherwise don't specify any -cpu or  section
> 
> The third option seems to cause qemu / libvirt to use qemu64.  However
> it has the advantage that the guest is migratable afterwards.

I vaguely recall in previous discussion that we decided not to
care if the guest is migratable or not, on the basis that wasn't
really domain knowledge of v2v nor an user request. Thus the
use of 'host-passthrough' rather than 'host-model' to give a
"perfect" match for the host, rather than approximation.


> There may be a case for changing this so:
> 
>  (a) Rule (2) is changed so we use host-model (for the libvirt target).

All hinges on whether users expect the output of v2v to be
migratable or not.

>  (b) Rule (3) is changed so we always specify a minimum CPU, eg.
>  for x86-64 guests, choose x86-64-v2 (is that an actual CPU model??)

In the case of x86-64-v2 it is the QEMU Nehalem model is a perfect
match, and is capable of running on both Intel and AMD hosts. This
is mostly luck though. For x86-64-v3 / -v4 ABIs there is no viable
model that will run on both AMD and Intel. So once there's a guest
OS that mandates -v3, we'll be back to wanting host-model/passthrough.

> (a) seems like a no-brainer.  Not sure why we chose host-passthrough
> instead of host-model?  The explanation in the commit message is not
> very convincing:
> https://github.com/libguestfs/virt-v2v/commit/a3e45b3ea5b45e682cb4b7ad3a5646586c6c7886
> 
> I think (b) is fraught because it is my understanding that there is no
> good choice for a minimum CPU since we don't know anything about the
> target hardware (eg. if it's Intel or AMD).  And on !x86-64 we really
> have no idea at all what's a good choice.

Yeah, I'd recommend against (b).

IMHO mgmt apps / provisioning tools should only ever choose host-model
or host-passthrough for KVM. Use of an explicit named CPU model should
be something left to admins to decide based on their global knowledge of
what machines they need to migrate across. Ignoring named models also
nicely avoids the problem of needing knowledge across all arches.

If you care about TCG, you could use 'maximum' instead of 'host-passthrough'.
They are identical in the case of KVM, and for TCG 'maximum' just gives you
all possible features.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v] convert: linux: Require host cpu for all RHEL-alike >= 9

[Daniel P . Berrangé]

On Mon, Feb 06, 2023 at 12:19:40PM +, Richard W.M. Jones wrote:
> RHEL >= 9 and compatible distros like Rocky >= 9 will not boot using
> the default qemu CPU.  You will see an error at boot:
> 
>   Fatal glibc error: CPU does not support x86-64-v2
> 
> Instead you need to use -cpu host.
> 
> In commit f28757c6d1 ("convert_linux: set "gcaps_default_cpu = false"
> for x86_64 RHEL-9.0+ guests") we fixed this specifically for RHEL >= 9.
> 
> This commit extends the same fix to all RHEL family distros.

If v2v is going to use  host CPU for a bunch of distros, why not use
it for all distros ? What's the downside in -cpu host, instead of
the default qemu64 CPU ?

Even if the distro doesn't require x86_64-v2 ABI, they'll pretty much
all benefit from NOT using the default QEMU CPU model, if only for
the fact that they gain the 'aesni' feature which increases crypto
performance massively. 

> 
> Updates: commit f28757c6d100060c65212ea55cfa59d308dcb850
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2166619
> Reported-by: Ming Xie
> Thanks: Laszlo Ersek
> ---
>  convert/convert_linux.ml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
> index e20cafa551..460cbff0ed 100644
> --- a/convert/convert_linux.ml
> +++ b/convert/convert_linux.ml
> @@ -201,9 +201,9 @@ let convert (g : G.guestfs) source inspect i_firmware 
> keep_serial_console _ =
>  
>  (* RHEL >= 9.0 on x86_64 requires the processor to support the 
> "x86-64-v2"
>   * microarchitecture level, which the default QEMU VCPU model does not
> - * satisfy.  Refer to RHBZ#2076013.
> + * satisfy.  Refer to RHBZ#2076013 RHBZ#2166619.
>   *)
> -let default_cpu_suffices = inspect.i_distro <> "rhel" ||
> +let default_cpu_suffices = family <> `RHEL_family ||
> inspect.i_arch <> "x86_64" ||
> inspect.i_major_version < 9 in
>  
> -- 
> 2.39.0
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
> 

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH 1/2] ci: Add opensuse-leap 15.4, skip TLS on 15.3

[Daniel P . Berrangé]

On Wed, Oct 19, 2022 at 09:46:12AM -0500, Eric Blake wrote:
> lcitool recently added support for Leap 15.4; meanwhile, Leap 15.3 has
> a version of nbdkit that tries to use @NBDKIT,SYSTEM as the TLS
> priority which then crashes in gnutls.  Explicitly testing our
> --without-gnutls configure coverage on that platform is thus a good
> idea; this required tweaking build.sh to learn another variables, and
> reordering to put CONFIGURE_OPTS (which the user can override at
> pipeline time) after our hard-coded CONFIG_ARGS.

Since you added support for $GNUTLS instead of setting CONFIGURE_OPTS
in manifest.yml, there was no need for the re-ordering of that
line. Still I guess the new order makes sense, as it allows override
of stuff set by CONFIG_ARGS should it be needed in future.

> 
> Thanks: Daniel P. Berrangé 
> ---
>  ci/build.sh| 14 +++--
>  ci/buildenv/opensuse-leap-154.sh   | 61 +
>  ci/containers/opensuse-leap-154.Dockerfile | 62 ++
>  ci/gitlab.yml  |  1 +
>  ci/gitlab/build-templates.yml  |  5 ++
>  ci/gitlab/builds.yml   | 20 +++
>  ci/gitlab/container-templates.yml  |  1 +
>  ci/gitlab/containers.yml   |  7 +++
>  ci/manifest.yml| 10 +++-
>  9 files changed, 176 insertions(+), 5 deletions(-)
>  create mode 100644 ci/buildenv/opensuse-leap-154.sh
>  create mode 100644 ci/containers/opensuse-leap-154.Dockerfile

Reviewed-by: Daniel P. Berrangé 


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [libnbd PATCH 2/2] ci: Add coverage of --without-libxml2

[Daniel P . Berrangé]

On Wed, Oct 19, 2022 at 09:46:13AM -0500, Eric Blake wrote:
> Although we generally like URI support, it is worth having explicit
> coverage in our CI that we can build without it for those who want
> minimal dependencies.  Similar to the previous patch, this is easiest
> to do with another build.sh variable.  Doing so found two additional
> tests that need skipping (comparable to how we already skipped similar
> tests in sh/).
> ---
>  ci/build.sh  |  8 +++-
>  ci/gitlab/builds.yml | 20 
>  ci/manifest.yml  |  5 +
>  dump/dump-data.sh|  1 +
>  dump/dump-pattern.sh |  1 +
>  5 files changed, 34 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd | Failed pipeline for master | 018d55a8

[Daniel P . Berrangé]

On Tue, Oct 18, 2022 at 02:59:37PM -0500, Eric Blake wrote:
> On Tue, Oct 18, 2022 at 09:10:24AM +0100, Daniel P. Berrangé wrote:
> > > > > 
> > > > > Indeed. Leap 15.4 and newer include the crypto-policies package. 
> > > > > Should the
> > > > > container move to a 15.4 base?
> > > > 
> > > > Yes, we need to add 15.4 to libvirt-ci facts database, given
> > > > the relative EOL dates.
> > > 
> > > I was about to do that today and see you've already taken care of it :-).
> > 
> > Opps, yes, I meant to reply to this mail to say so. When I looked, it
> > turned out to be trivial as no package changes were needed, so I just
> > submitted it.
> 
> I see the change in the libvirt-ci repo, but not how to apply it to
> the libnbd repo.  Running
> 
> lcitool manifest
> 
> updates a few .yml files, but does not do anything that obviously
> replaces opensuse-leap-153 with opensuse-leap-154, at least in the
> changes I saw.  But that could be because I'm unfamiliar with how the
> process is supposed to work.

The project local  ci/manifest.yml explicitly declares what OS targets
your project wants to perform CI against. So either add 15.4 there
in addition to 15.3, or replace 15.3 with 15.4 as you prefer.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd | Failed pipeline for master | 018d55a8

[Daniel P . Berrangé]

On Mon, Oct 17, 2022 at 03:22:17PM -0600, Jim Fehlig wrote:
> On 10/14/22 01:17, Daniel P. Berrangé wrote:
> > On Thu, Oct 13, 2022 at 03:02:51PM -0600, Jim Fehlig wrote:
> > > Hi Daniel,
> > > 
> > > Thanks for the detailed report!
> > > 
> > > On 10/13/22 03:33, Daniel P. Berrangé wrote:
> > > > On Thu, Oct 13, 2022 at 09:49:09AM +0100, Richard W.M. Jones wrote:
> > > > > 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.)
> > > > 
> > > > Anyone has access to the CI env.  Line 9 of the build log
> > > > shows the container env used:
> > > > 
> > > > Using docker image 
> > > > sha256:e4a8e52b0bbb712a544a90d21b21010daad8ab3e85a768cfea38571461ec85fc 
> > > > for registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153:latest with 
> > > > digest 
> > > > registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153@sha256:11179119130366bc340f0fe6d0c940fa904c5d3760a10e979296ffd6c8b28488
> > > >  ...
> > > > 
> > > > You just need to launch the same container, clone the git repo and
> > > > then run the build commands
> > > > 
> > > > IOW, on your local machine do:
> > > > 
> > > > $ podman run -it 
> > > > registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153:latestn
> > > > # git clone https://gitlab.com/nbdkit/libnbd
> > > > # cd libnbd
> > > > # autoreconf -if
> > > > # ./configure  --enable-gcc-warnings --with-gnutls --with-libxml2 
> > > > --enable-fuse --enable-ocaml --enable-python --enable-golang
> > > > 
> > > > # make -j 20
> > > > # cd tests
> > > > # ./connect-tls-psk
> > > > requires nbdkit --tls-verify-peer -U - null --run 'exit 0'
> > > > nbdkit: pattern: error: failed to set TLS session priority to 
> > > > @NBDKIT,SYSTEM:+ECDHE-PSK:+DHE-PSK:+PSK: The request is invalid.
> > > > nbd_connect_command: gnutls_handshake: Error in the push function. 
> > > > (-1/1)
> > > > 
> > > > What's interesting here is that this shows the real error
> > > > mesage about TLS sessino priority.
> > > > 
> > > > If you set MALLOC_CHECK=1, however, then we loose the useful
> > > > error message:
> > > > 
> > > ># MALLOC_CHECK_=1 MALLOC_PERTURB_=146  ./connect-tls-psk
> > > >requires nbdkit --tls-verify-peer -U - null --run 'exit 0'
> > > >free(): invalid pointer
> > > >nbd_connect_command: gnutls_handshake: Error in the pull function. 
> > > > (-1/1)
> > > > 
> > > > which was unfortunate for debuggability.
> > > > 
> > > > I confirmed it is nbdkit that is crashing and it appears to be
> > > > in gnutls code.
> > > > 
> > > > Looking at the image there is no /etc/crypto-policies directory,
> > > > and nor is there any 'crypto-policies' package available in the
> > > > distro.
> > > 
> > > Indeed. Leap 15.4 and newer include the crypto-policies package. Should 
> > > the
> > > container move to a 15.4 base?
> > 
> > Yes, we need to add 15.4 to libvirt-ci facts database, given
> > the relative EOL dates.
> 
> I was about to do that today and see you've already taken care of it :-).

Opps, yes, I meant to reply to this mail to say so. When I looked, it
turned out to be trivial as no package changes were needed, so I just
submitted it.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd | Failed pipeline for master | 018d55a8

[Daniel P . Berrangé]

On Thu, Oct 13, 2022 at 03:02:51PM -0600, Jim Fehlig wrote:
> Hi Daniel,
> 
> Thanks for the detailed report!
> 
> On 10/13/22 03:33, Daniel P. Berrangé wrote:
> > On Thu, Oct 13, 2022 at 09:49:09AM +0100, Richard W.M. Jones wrote:
> > > 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.)
> > 
> > Anyone has access to the CI env.  Line 9 of the build log
> > shows the container env used:
> > 
> > Using docker image 
> > sha256:e4a8e52b0bbb712a544a90d21b21010daad8ab3e85a768cfea38571461ec85fc for 
> > registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153:latest with digest 
> > registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153@sha256:11179119130366bc340f0fe6d0c940fa904c5d3760a10e979296ffd6c8b28488
> >  ...
> > 
> > You just need to launch the same container, clone the git repo and
> > then run the build commands
> > 
> > IOW, on your local machine do:
> > 
> >$ podman run -it 
> > registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153:latestn
> ># git clone https://gitlab.com/nbdkit/libnbd
> ># cd libnbd
> ># autoreconf -if
> ># ./configure  --enable-gcc-warnings --with-gnutls --with-libxml2 
> > --enable-fuse --enable-ocaml --enable-python --enable-golang
> > 
> ># make -j 20
> ># cd tests
> ># ./connect-tls-psk
> >requires nbdkit --tls-verify-peer -U - null --run 'exit 0'
> >nbdkit: pattern: error: failed to set TLS session priority to 
> > @NBDKIT,SYSTEM:+ECDHE-PSK:+DHE-PSK:+PSK: The request is invalid.
> >nbd_connect_command: gnutls_handshake: Error in the push function. (-1/1)
> > 
> > What's interesting here is that this shows the real error
> > mesage about TLS sessino priority.
> > 
> > If you set MALLOC_CHECK=1, however, then we loose the useful
> > error message:
> > 
> >   # MALLOC_CHECK_=1 MALLOC_PERTURB_=146  ./connect-tls-psk
> >   requires nbdkit --tls-verify-peer -U - null --run 'exit 0'
> >   free(): invalid pointer
> >   nbd_connect_command: gnutls_handshake: Error in the pull function. (-1/1)
> > 
> > which was unfortunate for debuggability.
> > 
> > I confirmed it is nbdkit that is crashing and it appears to be
> > in gnutls code.
> > 
> > Looking at the image there is no /etc/crypto-policies directory,
> > and nor is there any 'crypto-policies' package available in the
> > distro.
> 
> Indeed. Leap 15.4 and newer include the crypto-policies package. Should the
> container move to a 15.4 base?

Yes, we need to add 15.4 to libvirt-ci facts database, given
the relative EOL dates.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd | Failed pipeline for master | 018d55a8

[Daniel P . Berrangé]

On Thu, Oct 13, 2022 at 09:49:09AM +0100, Richard W.M. Jones wrote:
> 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.)

Anyone has access to the CI env.  Line 9 of the build log
shows the container env used:

Using docker image 
sha256:e4a8e52b0bbb712a544a90d21b21010daad8ab3e85a768cfea38571461ec85fc for 
registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153:latest with digest 
registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153@sha256:11179119130366bc340f0fe6d0c940fa904c5d3760a10e979296ffd6c8b28488
 ...

You just need to launch the same container, clone the git repo and
then run the build commands

IOW, on your local machine do:

  $ podman run -it 
registry.gitlab.com/nbdkit/libnbd/ci-opensuse-leap-153:latestn
  # git clone https://gitlab.com/nbdkit/libnbd
  # cd libnbd
  # autoreconf -if
  # ./configure  --enable-gcc-warnings --with-gnutls --with-libxml2 
--enable-fuse --enable-ocaml --enable-python --enable-golang

  # make -j 20
  # cd tests
  # ./connect-tls-psk 
  requires nbdkit --tls-verify-peer -U - null --run 'exit 0'
  nbdkit: pattern: error: failed to set TLS session priority to 
@NBDKIT,SYSTEM:+ECDHE-PSK:+DHE-PSK:+PSK: The request is invalid.
  nbd_connect_command: gnutls_handshake: Error in the push function. (-1/1)

What's interesting here is that this shows the real error
mesage about TLS sessino priority.

If you set MALLOC_CHECK=1, however, then we loose the useful
error message:

 # MALLOC_CHECK_=1 MALLOC_PERTURB_=146  ./connect-tls-psk 
 requires nbdkit --tls-verify-peer -U - null --run 'exit 0'
 free(): invalid pointer
 nbd_connect_command: gnutls_handshake: Error in the pull function. (-1/1)

which was unfortunate for debuggability.

I confirmed it is nbdkit that is crashing and it appears to be
in gnutls code.

Looking at the image there is no /etc/crypto-policies directory,
and nor is there any 'crypto-policies' package available in the
distro.

So they have mis-built  nbdkit in leap 15.3  with TLS priority
string of @NBDKIT,SYSTEM, despite not having support for that
in their distro.

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

Yep, broken nbdkit, compared by free() crash bug in gnutls
hiding the real error

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

In ci/manifest.yml set  'allow-failure: true' for 15.3, and
re-run   lcitool manifest.

Or disable gnutls build on 15.3 for CI purposes by passing --without-gnutls

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Fri, Sep 30, 2022 at 04:39:48PM +0200, Laszlo Ersek wrote:
> On 09/30/22 16:03, Daniel P. Berrangé wrote:
> 
> > There are a bunch of users who want you to fully express all the optional
> > deps, so they're guaranteed everything is installed by default. There are
> > another bunch of users who want everything to be optional so they can
> > make the most minimalist install profile in every conceivable scenario.
> > 
> > You can't win, no matter what, a bunch of people end up being unhappy
> > with the choices made.
> 
> Good point -- I read Rich's allusion to containers etc a minute ago, but
> I didn't fully get the point just then. Now it's clearer -- "my
> container provides a GTK3 application that doesn't have a spinner or any
> other use for SVG rendering, so I want to be able to exclude librsvg2".
> 
> > Over time Fedora and RHEL have tended more towards making everything
> > highly modular at the package level,  and then left the question of
> > default "bundles of packages" to the high level such as the installer
> > groups. Effectively it has been decided that if you're hand picking
> > packages, you need to accept the complexity and figure out all the
> > optional bits for your scenario.
> 
> There should be a middle of the road solution here; Suggests: or
> Recommends: directives, or even just plain comments, so that when I grep
> an upstream repository or a dist-git repository, I as a clueless human
> still be helped, without preventing the container folks from enjoying
> their minimalism.

Yeah the weak deps could help, but for whatever reason their usage
hasn't been widely adopted 


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Fri, Sep 30, 2022 at 03:57:15PM +0200, Laszlo Ersek wrote:
> On 09/30/22 15:35, Daniel P. Berrangé wrote:
> > On Fri, Sep 30, 2022 at 03:23:31PM +0200, Laszlo Ersek wrote:
> >> On 09/30/22 14:11, Richard W.M. Jones wrote:
> >>> On Fri, Sep 30, 2022 at 12:56:40PM +0200, Laszlo Ersek wrote:
> >>
> >>>> (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?
> >>
> >> So, "ldd `which icewm`" reports that icewm depends on librsvg-2.so.2;
> >> therefore it's not surprising that RPM generated the proper Requires:
> >> directive for the binary icewm package. I guess with Metacity, this is
> >> not the case -- which should be fine, if Metacity itself does not use
> >> librsvg2.
> >>
> >> So it looks like a package dependency bug somewhere in or around gtk3; I
> >> guess it has a kind of plugin architecture, and the SPEC file does not
> >> properly capture the (dynamic) dependency.
> >>
> >> Interestingly, in the usptream gtk repository, on the gtk-3-24 branch,
> >> the file ".gitlab-ci/fedora-gtk3.Dockerfile" spells out
> >> "librsvg2-common" and "librsvg2", from commit 81c4fa5d505ec (which is
> >> quite non-descript in this regard). So it is a *known* hidden dependency
> >> in a way. Sigh. Packaging bug then? :/
> > 
> > "its complicated" :-)
> > 
> > GTK doesn't directly know about any image formats. It is built on top
> > of GDK-Pixbuf which is a library that provides an API for loading images,
> > with plugins for various formats, most outsourced to other packages.
> > librsvg2 provides a plugin for GDK-Pixbuf for SVG files.
> > 
> > Meanwhile the usage of SVG files doesn't actually exist in GTK code either,
> > because GTK widget rendering is theme based, and both widget and icon themes
> > are separate. Even when the widget theme requests an icon, it doesn't 
> > specify
> > a format, because icon themes are pluggable too and it will look for icons
> > in whatever format happens to exist on disk.
> > 
> > The icon theme doesn't include a dep on librsvg2 either, because the icons'
> > package doesn't dictate what is used to load them.
> > 
> > IOW, it is a tragedy of a highly modular framework that nothing individually
> > needs/want to have a dep on librsvg2, but the combined work does need such
> > a dep.
> > 
> > In a typical Fedora/RHEL install, all the right bits will get installed
> > either because of deps elsewhere in the default "desktop" install profile
> > group, or because of comps groups listing librsvg.  p2v though is picking
> > a minimal set of individual packages and so misses the dep.
> 
> This is horrible. The librsvg2 package provides this file:

snip

> All of this registration stuff is for the 5% more comfort of
> programmers, at the 90% more discomfort of sysadmins / users.

Honestly as a packager you are stuck between a rock and a hard place.

There are a bunch of users who want you to fully express all the optional
deps, so they're guaranteed everything is installed by default. There are
another bunch of users who want everything to be optional so they can
make the most minimalist install profile in every conceivable scenario.

You can't win, no matter what, a bunch of people end up being unhappy
with the choices made.

Over time Fedora and RHEL have tended more towards making everything
highly modular at the package level,  and then left the question of
default "bundles of packages" to the high level such as the installer
groups. Effectively it has been decided that if you're hand picking
packages, you need to accept the complexity and figure out all the
optional bits for your scenario.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Fri, Sep 30, 2022 at 03:23:31PM +0200, Laszlo Ersek wrote:
> On 09/30/22 14:11, Richard W.M. Jones wrote:
> > On Fri, Sep 30, 2022 at 12:56:40PM +0200, Laszlo Ersek wrote:
> 
> >> (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?
> 
> So, "ldd `which icewm`" reports that icewm depends on librsvg-2.so.2;
> therefore it's not surprising that RPM generated the proper Requires:
> directive for the binary icewm package. I guess with Metacity, this is
> not the case -- which should be fine, if Metacity itself does not use
> librsvg2.
> 
> So it looks like a package dependency bug somewhere in or around gtk3; I
> guess it has a kind of plugin architecture, and the SPEC file does not
> properly capture the (dynamic) dependency.
> 
> Interestingly, in the usptream gtk repository, on the gtk-3-24 branch,
> the file ".gitlab-ci/fedora-gtk3.Dockerfile" spells out
> "librsvg2-common" and "librsvg2", from commit 81c4fa5d505ec (which is
> quite non-descript in this regard). So it is a *known* hidden dependency
> in a way. Sigh. Packaging bug then? :/

"its complicated" :-)

GTK doesn't directly know about any image formats. It is built on top
of GDK-Pixbuf which is a library that provides an API for loading images,
with plugins for various formats, most outsourced to other packages.
librsvg2 provides a plugin for GDK-Pixbuf for SVG files.

Meanwhile the usage of SVG files doesn't actually exist in GTK code either,
because GTK widget rendering is theme based, and both widget and icon themes
are separate. Even when the widget theme requests an icon, it doesn't specify
a format, because icon themes are pluggable too and it will look for icons
in whatever format happens to exist on disk.

The icon theme doesn't include a dep on librsvg2 either, because the icons'
package doesn't dictate what is used to load them.

IOW, it is a tragedy of a highly modular framework that nothing individually
needs/want to have a dep on librsvg2, but the combined work does need such
a dep.

In a typical Fedora/RHEL install, all the right bits will get installed
either because of deps elsewhere in the default "desktop" install profile
group, or because of comps groups listing librsvg.  p2v though is picking
a minimal set of individual packages and so misses the dep.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Fri, Sep 30, 2022 at 01:11:50PM +0100, Richard W.M. Jones 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.
> > 
> > (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:

Only in EPEL-9, not in base RHEL-9 AFAIK.:

# dnf list | grep icewm
icewm.x86_64  2.9.9-1.el9   
epel
...

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Fri, Sep 30, 2022 at 01:50:43PM +0200, Laszlo Ersek wrote:
> On 09/30/22 13:01, Laszlo Ersek wrote:
> 
> > ... meant to add: this has proved Daniel's point that the WM is
> > extremely important. For the record, I use IceWM locally, so when I
> > run virt-p2v "directly", on my workstation, and forward X11 over SSH,
> > the reason for me seeing the spinner in the GTK3 build *may be* that I
> > use IceWM locally. It's possible that, if I used metacity locally, the
> > spinner would not work for me even in this X11-over-SSH setup.
> > 
> > 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.)
> 
> So, for Rich, the spinner works, when metacity comes from Fedora 36.
> 
> Considering Koji:
> 
>   https://koji.fedoraproject.org/koji/packageinfo?packageID=143
> 
> Fedora 35 is based on upstream metacity 3.42.0, while Fedora 36 is on
> 3.44.0:
> 
>   https://koji.fedoraproject.org/koji/buildinfo?buildID=1853336
>   https://koji.fedoraproject.org/koji/buildinfo?buildID=1936492
> 
> (I've checked in the VM disk image -- it indeed contains
> "metacity-3.42.0-1.fc35".)
> 
> Now, the upstream commit range between 3.42.0 and 3.44.0 is really
> small:
> 
>   1  ba192a82b2e9 revert "ci: use ubuntu 21.04 image"
>   2  1e0b9611551b Update Croatian translation
>   3  503fca976051 Update Croatian translation
>   4  9ed3b103f6ad bump version to 3.43.1, update NEWS
>   5  1b052f6f8675 build: remove obsolete macros
>   6  b3ca2a83da79 bump version to 3.44.0, update NEWS
> 
> And the fedora package contains no downstream patches AFAICT.
> 
> So this does not explain why metacity works, when Rich builds the VM
> disk image from Fedora 36 :/

Does  librsvg2  exist in both cases - there could be a different
dependancy chain resulting in it not being installed in some cases.

Having looked at the for p2v and gtk, I don't think window manager
will affect the spinner, except by way of an indirect accident
such as changed install deps.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

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.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

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.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Fri, Sep 30, 2022 at 09:41:02AM +0100, Richard W.M. Jones wrote:
> 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.

The test_connection_thread code is using  g_idle_add to run all
GTK APIs, which results in it running in the main event loop
thread. That is the correct way to deal with GTK from multiple
threads.

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.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

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.

> 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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] another GTK3 regression...

[Daniel P . Berrangé]

On Wed, Sep 28, 2022 at 02:01:11PM +0200, Laszlo Ersek wrote:
> (this reproduces at commit 0687cea6a86e; IOW the regression is not from
> the recent GTK-related patches, but due to building p2v with GTK3. as
> opposed to GTK2)
> 
> In the first dialog, when the Test Connection button is clicked, a
> spinner is supposed to be shown to the left, while p2v communicates via
> v2v over ssh. This spinner is seen when running virt-p2v directly, but
> not when running virt-p2v from a VM (using either a Live CD, or the
> "run-virt-p2v-in-a-vm" makefile target).
> 
> This difference is not seen when virt-p2v is built with GTK2; in that
> case, *both* scenarios (direct & VM) show the spinner properly.
> 
> I don't have an idea what the cause is. I vaguely suspect it could be
> related to the windowing environment (window manager) in use. It pretty
> much looks like a GTK3 bug to me; whatever the windowing environment,
> the spinner should either be there or not. (It's possible that we have a
> virt-p2v bug, of course, but why isn't the symptom consistent then? GTK3
> should not have a dependency at all on the Window Manager.)

There certainly are dependancies on both the window manager, and the
desktop system (X11 vs wayland). There's a huge set of features that
widget toolkits can negotiate with window managers,  and each window
manager supports a different set of extensions, just one example
being support for client side window decorations. In theory GTK should
adapt to expose the same functionality to users whereever possible but
there are certainly edge cases where this doesn't work, and bugs will
certainly be present for more obscure combinations like X11 + any non
mainstream window manager.

It is also not surprising to see a differnce from GTK2 to 3, as it
radically changed under the hood, most notably with native Wayland
support.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 1/2] require GLIB2 >= 2.56 and GTK3 >= 3.22; enhance API compatibility

[Daniel P . Berrangé]

On Wed, Sep 28, 2022 at 02:06:49PM +0200, Laszlo Ersek wrote:
> For further simplifying virt-p2v's GTK API usage, require the GLIB2 and
> GTK3 versions that RHEL7 provides.
> 
> The MIN_REQUIRED= macros elicit warnings if we use an API that
> was already deprecated in said version of the given library.
> 
> The MAX_ALLOWED= macros make sure we don't unwittingly use an API
> that was introduced after our required minimum version.
> 
> (GDK is a layer below GTK, so using the GDK_VERSION_* macros ensures both
> GDK and GTK API compatibility.)
> 
> Suggested-by: Richard W.M. Jones 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Laszlo Ersek 
> ---
>  Makefile.am   | 4 
>  docs/p2v-building.pod | 6 +-
>  m4/p2v-libraries.m4   | 5 +++--
>  3 files changed, 12 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 7/7] gui.c: annotate GTK_INPUT_PURPOSE_PASSWORD with upstream GTK3 version

[Daniel P . Berrangé]

On Tue, Sep 27, 2022 at 06:27:08PM +0200, Laszlo Ersek wrote:
> On 09/27/22 17:59, Daniel P. Berrangé wrote:
> > On Tue, Sep 27, 2022 at 04:53:08PM +0200, Laszlo Ersek wrote:
> >> On 09/26/22 14:09, Daniel P. Berrangé wrote:
> >>> On Mon, Sep 26, 2022 at 12:59:21PM +0100, Richard W.M. Jones wrote:
> >>>>
> >>>> Apart from Dan's suggestions in patch 1, the series looks good to me.
> >>>>
> >>>> FWIW RHEL 7 (the earliest distro with PCRE 2) has glib2 2.56.1 &
> >>>> gtk3 3.22.30, so supporting any earlier versions also seems pointless,
> >>>> so that might be another thing to review.  We could make USE_POPOVERS
> >>>> unconditional, and make gui-gtk3-compat.h considerably less
> >>>> complicated.
> >>>
> >>> Say you pick 3.22 as your official min and check this with pkg-config
> >>> in configure, then you can further define
> >>>
> >>>   GDK_VERSION_MIN_REQUIRED=GDK_VERSION_3_22
> >>>   GDK_VERSION_MAX_ALLOWED=GDK_VERSION_3_22
> >>>
> >>> The former will give you warnings if you use any API that was already
> >>> deprecated in 3.22 - this is something that should be re-written to
> >>> use the recommended modern alternative API.
> >>
> >> Is "GDK" a muscle-memory typo (because "GDK" does exist)? Did you mean GTK?
> > 
> > It looks odd, but I really do mean GDK here. It affects both the GDK
> > and GTK   include files (GDK is a layer below GTK).
> 
> Right, I've found a diagram here:
> 
> https://en.wikipedia.org/wiki/GDK
> 
> What I didn't know was that the GTK and GDK versions were supposed to be
> in sync. In virt-p2v, we care about GTK (and maybe glib); I've not been
> aware that we care about APIs at the GDK level. So enforcing a GDK
> version seemed strange, as a proxy for the GTK version.

Practically speaking in terms of configure/meson checks it is sufficient
to merely check for GTK, as GTK's pkg-config file has a dependancy on
GDK's pkg-config file. It should be impossible end up with mis-matched
versions of GTK & GDK unless you've gone out of your way to intentionally
break their installation in some way.

It does mean you end up with the strange scenario, where you refer to
GTK for the pkg-config test, but for the API version checking #defines
I describe, you refer to GDK instead.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 7/7] gui.c: annotate GTK_INPUT_PURPOSE_PASSWORD with upstream GTK3 version

[Daniel P . Berrangé]

On Tue, Sep 27, 2022 at 04:53:08PM +0200, Laszlo Ersek wrote:
> On 09/26/22 14:09, Daniel P. Berrangé wrote:
> > On Mon, Sep 26, 2022 at 12:59:21PM +0100, Richard W.M. Jones wrote:
> >>
> >> Apart from Dan's suggestions in patch 1, the series looks good to me.
> >>
> >> FWIW RHEL 7 (the earliest distro with PCRE 2) has glib2 2.56.1 &
> >> gtk3 3.22.30, so supporting any earlier versions also seems pointless,
> >> so that might be another thing to review.  We could make USE_POPOVERS
> >> unconditional, and make gui-gtk3-compat.h considerably less
> >> complicated.
> > 
> > Say you pick 3.22 as your official min and check this with pkg-config
> > in configure, then you can further define
> > 
> >   GDK_VERSION_MIN_REQUIRED=GDK_VERSION_3_22
> >   GDK_VERSION_MAX_ALLOWED=GDK_VERSION_3_22
> > 
> > The former will give you warnings if you use any API that was already
> > deprecated in 3.22 - this is something that should be re-written to
> > use the recommended modern alternative API.
> 
> Is "GDK" a muscle-memory typo (because "GDK" does exist)? Did you mean GTK?

It looks odd, but I really do mean GDK here. It affects both the GDK
and GTK   include files (GDK is a layer below GTK).


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 7/7] gui.c: annotate GTK_INPUT_PURPOSE_PASSWORD with upstream GTK3 version

[Daniel P . Berrangé]

On Mon, Sep 26, 2022 at 12:59:21PM +0100, Richard W.M. Jones wrote:
> 
> Apart from Dan's suggestions in patch 1, the series looks good to me.
> 
> FWIW RHEL 7 (the earliest distro with PCRE 2) has glib2 2.56.1 &
> gtk3 3.22.30, so supporting any earlier versions also seems pointless,
> so that might be another thing to review.  We could make USE_POPOVERS
> unconditional, and make gui-gtk3-compat.h considerably less
> complicated.

Say you pick 3.22 as your official min and check this with pkg-config
in configure, then you can further define

  GDK_VERSION_MIN_REQUIRED=GDK_VERSION_3_22
  GDK_VERSION_MAX_ALLOWED=GDK_VERSION_3_22

The former will give you warnings if you use any API that was already
deprecated in 3.22 - this is something that should be re-written to
use the recommended modern alternative API.

The latter will give you warnings if you use any API that was introduced
after 3.22 - this is to prevent you accidentally introducing usage of
APIs newer than your min version

The same exists for glib via GLIB_VERSION_MIN_REQUIRED/MAX_ALLOWED.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 1/7] "shutdown_actions": suppress "missing initializer" warnings/errors

[Daniel P . Berrangé]

On Mon, Sep 26, 2022 at 10:18:06AM +0200, Laszlo Ersek wrote:
> gcc reports:
> 
> > gui.c:1795:3: error: missing initializer for field ‘padding’ of
> > ‘GActionEntry’ {aka ‘const struct _GActionEntry’}
> > [-Werror=missing-field-initializers]
> >  1795 |   { "shutdown", activate_action, NULL, NULL, NULL },
> >
> > gui.c:1796:3: error: missing initializer for field ‘padding’ of
> > ‘GActionEntry’ {aka ‘const struct _GActionEntry’}
> > [-Werror=missing-field-initializers]
> >  1796 |   { "reboot", activate_action, NULL, NULL, NULL },
> 
> I've found this only now because:
> 
> - this is the first time I'm building virt-p2v with GTK3,
> 
> - the "shutdown_actions" array depends on USE_POPOVERS which depends on
>   GTK3 being selected,
> 
> - the "missing-field-initializers" warning (treated as an error) has
>   recently been enabled via "-Wextra" in commit 391f9833d398 ("p2v-c.m4:
>   elicit a stricter set of warnings from gcc", 2022-09-23).
> 
> The C-language documentation for GActionEntry is silent on the "padding"
> array:
> 
>   https://docs.gtk.org/gio/struct.ActionEntry.html
> 
> However, the D-language docs expose it:
> 
>   https://api.gtkd.org/gio.c.types.GActionEntry.html
> 
> Provide the missing field initializer.
> 
> Signed-off-by: Laszlo Ersek 
> ---
>  gui.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gui.c b/gui.c
> index 49301d9a985b..4faaa710ed90 100644
> --- a/gui.c
> +++ b/gui.c
> @@ -1792,8 +1792,8 @@ static gboolean close_running_dialog (GtkWidget *w, 
> GdkEvent *event, gpointer da
>  
>  #ifdef USE_POPOVERS
>  static const GActionEntry shutdown_actions[] = {
> -  { "shutdown", activate_action, NULL, NULL, NULL },
> -  { "reboot", activate_action, NULL, NULL, NULL },
> +  { "shutdown", activate_action, NULL, NULL, NULL, { 0 } },
> +  { "reboot", activate_action, NULL, NULL, NULL, { 0 } },
>  };

Notice the header decl says 'padding' is private hence why it is
not documented.

struct _GActionEntry
{
  const gchar *name;

  void (* activate) (GSimpleAction *action,
 GVariant  *parameter,
 gpointer   user_data);

  const gchar *parameter_type;

  const gchar *state;

  void (* change_state) (GSimpleAction *action,
 GVariant  *value,
 gpointer   user_data);

  /*< private >*/
  gsize padding[3];
};


The purpose of this 'padding' entry is to reserve space in the struct
for future usage. Apps should never touch the padding field, since it
can change in future. ie that 3 element array can turn into three
separate public fields later, and then the compound initializer above
would be invalid.

The right way to declare this is using named initializers:

  { .name = "shutdown", .activate = activate_action },
  { .name = "reboot", .activate = activate_action },


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1

[Daniel P . Berrangé]

On Thu, Sep 08, 2022 at 02:36:15PM +0100, Richard W.M. Jones wrote:
> (Adding Dan for input)
> 
> On Thu, Sep 08, 2022 at 03:23:41PM +0200, Laszlo Ersek wrote:
> > On 09/08/22 10:03, Richard W.M. Jones wrote:
> > > On Mon, Sep 05, 2022 at 01:25:27PM +0200, Laszlo Ersek wrote:
> > >> +  "p2v.vcpu.dense_topo" => manual_entry->new(
> > >> +shortopt => "", # ignored for booleans
> > >> +description => "
> > >> +Copy the physical machine's CPU topology, densely populated, to the
> > >> +guest.  Disabled by default.  If disabled, the C setting
> > >> +takes effect.",
> > > 
> > > 
> > > I just realised I'm not completely sure what "densely populated"
> > > means here.  I think we should have a bit more explanation.
> > > 
> > > How about something like:
> > > 
> > >   "p2v.vcpu.dense_topo" => manual_entry->new(
> > > shortopt => "", # ignored for booleans
> > > description => "
> > > Copy the physical machine's complete CPU topology (sockets, cores and
> > > threads) to the guest.  Disabled by default.  If disabled, the
> > > C setting takes effect.",
> > > 
> > > (Which might also imply that we rename this something like
> > > "complete_topo" or "full_topo" but I'll leave that to you.)
> > 
> > By "dense", I meant to express that there are no gaps in the onlining of
> > the CPU topology.
> > 
> > Assume we have 2 sockets, 2 cores/socket, 2 theads/core. Assume CPU#1
> > (in socket#1) is hot-pluggable, but isn't currently plugged, only CPU#0
> > (in socket#0) is present -- making for 1*2*2 = 4 logical processors in
> > total. A physical machine may well boot like this. Then our topology is
> > 2*2*2, but we only have 4 logical processors, so the topology is not
> > densely populated. The language is supposed to express that in any such
> > case, we'll ignore the online / plugged / etc count, and we'll just grab
> > the static topology, and fully / densely populate it with logical
> > processors.
> > 
> > "Complete topology" does not express this. Sticking with the above
> > example, the topology is already complete on the physical machine (we
> > have full information about the levels of the hierarchy), but it's not
> > densely populated.
> > 
> > Another example would be 1 * 4 * 2 physical (a normal low-end machine by
> > today's standards), where the sysadmin disables (say) cores #1 and #2
> > using /sys/devices/system/cpu/cpu{1,2}/online. (I think this may even be
> > possible on the kernel command line, for whatever reason necessary.) In
> > this case, during conversion, if "dense_topo" is set, we carry over not
> > just the topology (= the 1 * 4 * 2 hierarchy), but we also densely
> > populate it (producing 8 logical processors in the conversion output,
> > disregarding the "gaps" on the source; i.e. that only 4 logical
> > processors were available on the physical machine originally.)
> > 
> > I considered "complete", and thought it didn't express my intent. "Full"
> > is so-so -- my problem is it seems to have two meanings; one is in fact
> > what I'm trying to say with "dense", but the other meaning is just
> > "complete", which I don't find good.
> > 
> > The choices p2v should offer are:
> > 
> > - Just carry over a flat VCPU count N --> this maps to a 1 socket * N
> > cores/socket * 1 thread / core topology, fully populated.
> > 
> > - Otherwise (i.e., when the dense_topo knob is enabled), convert the
> > original topology (S sockets * C/S cores/socket * T threads/core), *AND*
> > fully populate that topology (disregarding the original "online count"
> > on the physical machine, which may easily be less than the (S * C * T)
> > product.)
> 
> I think the "mot juste" has to express that we're trying to model as
> closely as possible the real physical topology.  (The denseness
> doesn't seem to be so important - are there many machines where CPUs
> are not online?  Can that even happen when virt-p2v is running?)
> 
> How about:
> 
> authentic_topo
> physical_topo
> accurate_topo
> 
> ...?
> 
> The patch is totally fine, we're just quibbling about the
> word "dense" :-)

Why not 'host_topo' since it is mirroring the host ?

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH 2/6] restrict vCPU topology to (a) fully populated physical, or (b) 1 * N * 1

[Daniel P . Berrangé]

On Mon, Sep 05, 2022 at 01:25:27PM +0200, Laszlo Ersek wrote:
> Currently, the CPU topology for the converted domain is determined as
> follows:
> 
> (1) main()  [main.c]
> 
> (2)   set_config_defaults() [main.c]
> vcpus <-- sysconf (_SC_NPROCESSORS_ONLN)
> get_cpu_config()[cpuid.c]
>   get_topology()[cpuid.c]
> sockets <-- lscpu (physical)
> cores   <-- lscpu (physical)
> threads <-- lscpu (physical)
> 
> (3)   update_config_from_kernel_cmdline()   [kernel-config.c]
> vcpus   <-- kernel cmdline
> sockets <-- kernel cmdline
> cores   <-- kernel cmdline
> threads <-- kernel cmdline
> 
> (4)   opt#1: kernel_conversion()[kernel.c]
> no change to topology info
>   opt#2: gui_conversion()   [gui.c]
> vcpus <-- GUI
> 
> (5) ...
>   start_conversion()[conversion.c]
> generate_physical_xml() [physical-xml.c]
>   format vcpus, sockets, cores, threads
> 
> In (2), we initialize the topology information from the physical machine.
> In (3), we override each property in isolation from the kernel command
> line (this is done for both kernel conversion and GUI conversion; in the
> former case, it is the only way for the user to influence each element of
> the topology). In (4), in case we picked GUI conversion, the VCPU number
> can be overridden yet another time on the GUI. In (5), we format the
> topology information into the domain XML.
> 
> The problem is that it's easy to create inconsistent topologies that
> libvirt complains about. One example is that in step (4) on the GUI, if
> the flat VCPU count is reduced by the user, it can result in sockets
> partially populated with cores or threads, or in cores partially populated
> with threads. Another example (even if the user does not touch the VCPU
> count) is a partially onlined CPU topology on the physical machine:
> _SC_NPROCESSORS_ONLN does not reflect any topology information, and if the
> flat number it returns is less than the (sockets * cores * threads)
> product from "lscpu", then the online logical processors will be
> "compressed to the left" by libvirt just the same as after the manual VCPU
> count reduction.
> 
> An over-complicated way to fix this would be the following:
> 
> - parse the output of "lscpu -p" (which is available since RHEL6 --
>   "lscpu" is altogether missing in RHEL5), retrieving online-ness combined
>   with full topology information
> 
> - expose complete topology info on the GUI
> 
> - format the complete topology information for libvirt.
> 
> A simpler way (which is what this patch series chooses) is to remove some
> flexibility from virt-p2v's configuration interface. Namely, only allow
> the user to choose betwen (a) copying the physical CPU topology, *fully
> populated*, (b) specifying the core count N for a "1 socket * N
> cores/socket * 1 thread/core" topology.
> 
> Option (a) eliminates the "partially onlined physical CPU topology"
> scenario; it produces a topology that exists in the physical world, and
> does not require p2v to maintain topology information more expressively
> than it currently does.
> 
> Option (b) eliminates any conflicts between the physical (or any
> user-configured) sockets:cores:threads hierarchy and a manually set
> logical processor count. Option (b) produces the kind of flat CPU topology
> that QEMU has defaulted to since version 6.2, with a simple "-smp" option.
> Option (b) preserves the simple p2v user interface for entering only a
> logical processor count.

I would further say that option (b) is the *only* one that makes
sense from a performance POV, if you are *not* doing 1:1 pCPU:vCPU
pinning.

The guest kernel scheduler will take different placement decisions
based on the topology, and if guest CPUIs are floating across
arbitrary host CPUs, those scheduler decisions are meaningless and
potentially even harmful to performance.

IOW, I'd say option (b) should be the default in the absence of any
explicit override from the user.

When libvirt integrates support for core scheduling, then it becomes
more reasonable to consider setting different topologies even when
not pinning.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd | Failed pipeline for master | 5bd16353

[Daniel P . Berrangé]

On Thu, Aug 11, 2022 at 10:01:23AM +0100, Richard W.M. Jones wrote:
> On Thu, Aug 11, 2022 at 07:42:17AM +, GitLab wrote:
> > Pipeline #610085241 triggered by ●   Richard W.M. Jones
> >had 3 failed jobs
> >   Failed jobs
> > ✖ builds x86_64-opensuse-leap-153
> 
> Weeks ago I worked with Jim Fehlig to update nbdkit from the
> intermediate development version they're using (1.29.4) which had an
> unfortunate memory corruption bug to a stable version.  However it
> still hasn't made it out either to OpenSUSE or to the container builds
> used in gitlab CI.
> 
> > ✖ builds x86_64-almalinux-8-clang
> 
> In copy-file-to-qcow2-compressed.sh:
> 
> ++ stat -c %s copy-file-to-qcow2-compressed.file1
> + size1=17039360
> ++ stat -c %s copy-file-to-qcow2-compressed.file2
> + size2=17039360
> + '[' 17039360 -ge 17039360 ']'
> + echo '/builds/nbdkit/libnbd/copy/copy-file-to-qcow2-compressed.sh: qcow2 
> compr
> ession did not make the file smaller'
> /builds/nbdkit/libnbd/copy/copy-file-to-qcow2-compressed.sh: qcow2 
> compression d
> id not make the file smaller
> 
> I see this occasionally from time to time and I'm unclear why it
> happens.  However Kevin Wolf recently suggested a better test for this
> - to use qemu-img check and look for compressed clusters.  See:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=2047660#c16
> 
> So I may replace this test entirely.
> 
> > ✖ buildsx86_64-freebsd-12
> 
> For some reason we're pulling in python27 and FreeBSD is giving
> a notice that it is deprecated.

It isn't pulling in python27.  The python27 is pre-existing install in
the image your using and when you run 'pkg install' for the build deps
something is causing the existing py27 install to be updated to a newer
version. Presumably this is something that the Cirrus CI folk have
pre-installed in their FreeBSD images.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [v2v PATCH] output/create_libvirt_xml: generate @check='none' CPU attribute

[Daniel P . Berrangé]

On Fri, Jul 22, 2022 at 01:49:21PM +0200, Laszlo Ersek wrote:
> On 07/22/22 11:50, Richard W.M. Jones wrote:
> > On Fri, Jul 22, 2022 at 10:42:48AM +0100, Daniel P. Berrangé wrote:
> >> On Fri, Jul 22, 2022 at 10:34:44AM +0100, Richard W.M. Jones wrote:
> >>> Sorry for the delayed response to this.  I see you've posted an
> >>> updated patch, so this is just a bit of FYI.
> >>>
> >>> I originally added CPU modelling in commit 11505e4b84 (March 2017):
> >>>
> >>> https://github.com/libguestfs/virt-v2v/commit/11505e4b84ce8d7eda4e2a275fdcecc5f2a3288d
> >>>
> >>> What we were actually trying to achieve here was to preserve the CPU
> >>> topology.  I believe the request came from Bill Helgerson who was
> >>> working on v2v in the proto-IMS product, and was working a lot with
> >>> customers.
> >>>
> >>> You can see in the code before the patch is applied we only modelled
> >>> the number of vCPUs.  Afterwards we have:
> >>>
> >>>  * number of vCPUs
> >>>  * vendor (eg. AMD)
> >>>  * model (eg. EPYC)
> >>>  * sockets
> >>>  * cores per socket
> >>>  * threads per core
> >>>
> >>> I think here only the first 1 and last 3 (#vCPUS, topology) are really
> >>> important.  I believe I added the vendor and model just because they
> >>> were there, without necessarily thinking too deeply about the
> >>> implications.
> >>>
> >>> As you covered in your email, what is the real meaning of converting a
> >>> source guest using eg AMD/EPYC with virt-v2v to some target?  Does it
> >>> mean that the target must be able to emulate all EPYC feature (likely
> >>> impossible if the target is Intel)?  I would say it's not that
> >>> important.  This isn't live migration, and almost all guests can be
> >>> booted interchangably on different x86_64 hardware.
> >>>
> >>> Is topology important?  I would say yes, or at least it's much more
> >>> important than vendor/model.  Workloads may expect not just a number
> >>> of vCPUs, but a particular layout, especially the larger and more
> >>> complex ones.
> >>
> >> In terms of topology, if you have NOT set pCPU:vCPU 1:1 pinning,
> >> then NEVER set threads > 1. There's a choice of sockets vs cores
> >> for non-pinned scenario, and generally I'd recommends 'cores'
> >> always because high core counts are common in real world, and
> >> 'sockets' mostly maxes out at 2/4 in real world (ignoring wierd
> >> high end hardware), also some OS restrict you based on sockets,
> >> but not cores. So IMHO the only compelling reason to use
> >> sockets > 1 is you want to have virtual NUMA topology, but
> >> even that's dubious unless pinning.
> >>
> >> If you have set pCPU:vCPU 1:1 pinning, then set topology to
> >> try to match what you've pinned to.
> >>
> >>> So ... my question now is, should we simply remove the vendor and
> >>> model fields completely?
> >>
> >> Removing 'model' is not a good idea, as you'll get the default
> >> CPU model.
> >>
> >> If you don't have to pick a particular CPU, then IMHO either
> >> use host-model or host-passthrough depending on whether you
> >> think live migration is important or not.
> > 
> > I mean remove them from virt-v2v's internal source model [confusing
> > terminology here - modelling the source != CPU model].  On targets
> > we'd choose something like cpu=host-model to get the best possible
> > migratable CPU.
> > 
> > The point is we're not copying the Intel / Nehalem, AMD / EPYC etc of
> > the guest from the source to the destination hypervisor.
> 
> I think producing host-passthrough indiscriminately on output (which we
> already do in the particular case only when the source does not specify
> a model and we know an OS does not run on qemu64) would be best. I don't
> think it would be a very difficult patch or patch set, but I dread the
> testing of it. :/
> 
> Let me go ahead and commit v2; and let's remember this discussion for
> the next time a CPU model related problem pops up. If switching to
> host-passthrough solves that problem then, we should implement it then.
> (And then ask QE to test it as comprehensively as they can...)

Remember, 'host-passthrough' is only possible with KVM, not TCG,
'host-model' works with both. If you have newish libvirt + QEMU
you can use 'maximum' which is equiv to 'host-pasthrough' on
KVM, or "all implemented features" on TCG.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [v2v PATCH] output/create_libvirt_xml: generate @check='none' CPU attribute

[Daniel P . Berrangé]

On Fri, Jul 22, 2022 at 10:34:44AM +0100, Richard W.M. Jones wrote:
> Sorry for the delayed response to this.  I see you've posted an
> updated patch, so this is just a bit of FYI.
> 
> I originally added CPU modelling in commit 11505e4b84 (March 2017):
> 
> https://github.com/libguestfs/virt-v2v/commit/11505e4b84ce8d7eda4e2a275fdcecc5f2a3288d
> 
> What we were actually trying to achieve here was to preserve the CPU
> topology.  I believe the request came from Bill Helgerson who was
> working on v2v in the proto-IMS product, and was working a lot with
> customers.
> 
> You can see in the code before the patch is applied we only modelled
> the number of vCPUs.  Afterwards we have:
> 
>  * number of vCPUs
>  * vendor (eg. AMD)
>  * model (eg. EPYC)
>  * sockets
>  * cores per socket
>  * threads per core
> 
> I think here only the first 1 and last 3 (#vCPUS, topology) are really
> important.  I believe I added the vendor and model just because they
> were there, without necessarily thinking too deeply about the
> implications.
> 
> As you covered in your email, what is the real meaning of converting a
> source guest using eg AMD/EPYC with virt-v2v to some target?  Does it
> mean that the target must be able to emulate all EPYC feature (likely
> impossible if the target is Intel)?  I would say it's not that
> important.  This isn't live migration, and almost all guests can be
> booted interchangably on different x86_64 hardware.
> 
> Is topology important?  I would say yes, or at least it's much more
> important than vendor/model.  Workloads may expect not just a number
> of vCPUs, but a particular layout, especially the larger and more
> complex ones.

In terms of topology, if you have NOT set pCPU:vCPU 1:1 pinning,
then NEVER set threads > 1. There's a choice of sockets vs cores
for non-pinned scenario, and generally I'd recommends 'cores'
always because high core counts are common in real world, and
'sockets' mostly maxes out at 2/4 in real world (ignoring wierd
high end hardware), also some OS restrict you based on sockets,
but not cores. So IMHO the only compelling reason to use
sockets > 1 is you want to have virtual NUMA topology, but
even that's dubious unless pinning.

If you have set pCPU:vCPU 1:1 pinning, then set topology to
try to match what you've pinned to.

> So ... my question now is, should we simply remove the vendor and
> model fields completely?

Removing 'model' is not a good idea, as you'll get the default
CPU model.

If you don't have to pick a particular CPU, then IMHO either
use host-model or host-passthrough depending on whether you
think live migration is important or not.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd golang failure on RISC-V

[Daniel P . Berrangé]

On Thu, Jun 09, 2022 at 05:33:03PM +0100, Richard W.M. Jones wrote:
> On Thu, Jun 09, 2022 at 07:22:45PM +0300, Nir Soffer wrote:
> > On Thu, Jun 9, 2022 at 6:48 PM Richard W.M. Jones  wrote:
> > > NB: this _does not_ address the other problem where GODEBUG=cgocheck=2
> > > complains about "fatal error: Go pointer stored into non-Go memory".
> > 
> > Do we keep go pointers in buffers allocated in C?
> 
> I guess we must do, but I'm not sure where.
> 
> However it's possible that golang on RISC-V is buggy, particularly the
> somewhat old version that we are still using.

Yeah, my money is on this being a bug in the go runtime, the signal
stuff in particular is a bit hairy in the go impl that I looked at.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd golang failure on RISC-V

[Daniel P . Berrangé]

On Thu, Jun 09, 2022 at 04:48:34PM +0100, Richard W.M. Jones wrote:
> On Thu, Jun 09, 2022 at 04:24:12PM +0100, Richard W.M. Jones wrote:
> > On Thu, Jun 09, 2022 at 03:20:02PM +0100, Daniel P. Berrangé wrote:
> > > > + go test -count=1 -v
> > > > === RUN   Test010Load
> > > > --- PASS: Test010Load (0.00s)
> > > > === RUN   TestAioBuffer
> > > > --- PASS: TestAioBuffer (0.00s)
> > > > === RUN   TestAioBufferFree
> > > > --- PASS: TestAioBufferFree (0.00s)
> > > > === RUN   TestAioBufferBytesAfterFree
> > > > SIGABRT: abort
> > > > PC=0x3fdf6f9bac m=0 sigcode=18446744073709551610
> > > 
> > > So suggesting TestAioBufferBytesAfterFree is as fault, but quite
> > > odd as that test case is trivial and whle it allocates some
> > > native memory it doesn't seem to write to it. Unless the problem
> > > happened in an earlier test case and we have delayed detection ?
> > > 
> > > I guess I'd try throwing darts at the wall by chopping out bits
> > > of test code to see what makes it disappear.
> > > 
> > > Perhaps also try swapping MakeAioBuffer with MakeAioBufferZero
> > > in case pre-existing data into the C.malloc()d block is confusing
> > > Go ? 
> > 
> > Interestingly if I remove libnbd_020_aio_buffer_test.go completely,
> > and disable GODEBUG, then the tests pass.  (Reproducer commands at end
> > of email).  So I guess at least one of the problems is confined to
> > this test and/or functions it calls in the main library.
> > Unfortunately this test is huge.
> > 
> > At your suggestion, replacing every MakeAioBuffer with
> > MakeAioBufferZero in that test, but it didn't help.  Also tried
> > replacing malloc -> calloc in the aio_buffer.go implementation which
> > didn't help.
> > 
> > I'll try some more random things ...
> 
> Adding a few printf's shows something interesting:
> 
> === RUN   TestAioBufferBytesAfterFree
> calling Free on 0x3fbc1882b0
> calling C.GoBytes on 0x3fbc1882b0
> SIGABRT: abort
> PC=0x3fe6aaebac m=0 sigcode=18446744073709551610
> 
> goroutine 21 [running]:
> gsignal
> :0
> abort
> :0
> runtime.throwException
> ../../../libgo/runtime/go-unwind.c:128
> runtime.unwindStack
> ../../../libgo/go/runtime/panic.go:535
> panic
> ../../../libgo/go/runtime/panic.go:750
> runtime.panicmem
> ../../../libgo/go/runtime/panic.go:210
> runtime.sigpanic
> ../../../libgo/go/runtime/signal_unix.go:634
> _wordcopy_fwd_aligned
> :0
> __GI_memmove
> :0
> runtime.stringtoslicebyte
> ../../../libgo/go/runtime/string.go:155
> __go_string_to_byte_array
> ../../../libgo/go/runtime/string.go:509
> _cgo_23192bdcbd72_Cfunc_GoBytes
> ./cgo-c-prolog-gccgo:46
> 
> This is a simple use after free because the Free function in
> aio_buffer.go frees the array and then the Bytes function attempts to
> copy b.Size bytes from the NULL pointer.

Well it isn't use-after-free, because we've cleared the
pointer we freed.

Rather we're simply trying to reference the NULL pointer,

> I didn't write this test so I'm not quite sure what it's trying to
> achieve.  It seems to be deliberately trying to cause a panic, but
> causes a segfault instead?  (And why only on RISC-V?)

IIUC, the kernel will map a page without read/write perms
at address 0x0 in userspace. Thus a NULL pointer reference
causes a SEGV to userspace. Golang tries to catch this
SEGV and turn it into a panic I assume.

Assuming the kernel isn't doing something wierd on Risc-V
with the userspace mapping at 0x0, then points to the
Golang SEGV handler on RISCV.

> 
>   func TestAioBufferBytesAfterFree(t *testing.T) {
> buf := MakeAioBuffer(uint(32))
> buf.Free()
> 
> defer func() {
> if r := recover(); r == nil {
> t.Fatal("Did not recover from panic calling Bytes() 
> after Free()")
> }
> }()
> 
> buf.Bytes()
>   }


> 
> Since this only happens on RISC-V I guess it might be something to do
> with the golang implementation on this architecture being unable to
> turn segfaults into panics.
> 
> Removing all three *AfterFree tests fixes the tests.
> 
> It seems a bit of an odd function however.  Wouldn't it be better to
> changes the Bytes function so that it tests if the pointer is NULL and
> panics?

In theory I guess both should be equivalent in terms of
semantics for the caller.


Also I feel like 'Free' ought to set 'b.Size = 0' after
it set 'b.P = nul'

Re: [Libguestfs] libnbd golang failure on RISC-V

[Daniel P . Berrangé]

On Thu, Jun 09, 2022 at 03:08:51PM +0100, Richard W.M. Jones wrote:
> On Thu, Jun 09, 2022 at 02:14:07PM +0100, Daniel P. Berrangé wrote:
> > If you can get the test to core dump, then plain old GDB core
> > dump could also be useful - might identify which specific API
> > is being called, which can help restrict the size of the haystack
> > for code review.
> 
> Eventually careful reading of this page revealed how to do this:
> https://pkg.go.dev/runtime
> 
> The results unfortunately don't seem very useful.  The stack trace
> appears to point to where the error was detected rather than where it
> was caused, but I've copied it below anyway.
> 
> Also attached is the failure in the test suite if you turn *off*
> cgocheck, maybe that is useful.


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

> + requires nbdkit --version
> + go test -count=1 -v
> === RUN   Test010Load
> --- PASS: Test010Load (0.00s)
> === RUN   TestAioBuffer
> --- PASS: TestAioBuffer (0.00s)
> === RUN   TestAioBufferFree
> --- PASS: TestAioBufferFree (0.00s)
> === RUN   TestAioBufferBytesAfterFree
> SIGABRT: abort
> PC=0x3fdf6f9bac m=0 sigcode=18446744073709551610

So suggesting TestAioBufferBytesAfterFree is as fault, but quite
odd as that test case is trivial and whle it allocates some
native memory it doesn't seem to write to it. Unless the problem
happened in an earlier test case and we have delayed detection ?

I guess I'd try throwing darts at the wall by chopping out bits
of test code to see what makes it disappear.

Perhaps also try swapping MakeAioBuffer with MakeAioBufferZero
in case pre-existing data into the C.malloc()d block is confusing
Go ? 

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd golang failure on RISC-V

[Daniel P . Berrangé]

On Thu, Jun 09, 2022 at 02:03:04PM +0100, Richard W.M. Jones wrote:
> make[2]: Entering directory '/home/rjones/d/libnbd/golang'
> perl /home/rjones/d/libnbd/podwrapper.pl --section=3 --man libnbd-golang.3 \
> --html ../html/libnbd-golang.3.html \
> libnbd-golang.pod
> /home/rjones/d/libnbd/run go build
> write of Go pointer 0x3fa8028000 to non-Go memory 0x3fd2c0fb20
> fatal error: Go pointer stored into non-Go memory
> 
> runtime stack:
> runtime_mstart
>   ../../../libgo/runtime/proc.c:593
> 
> goroutine 1 [running, locked to thread]:
> 
> 
> Not sure how I should diagnose this one further?  I wouldn't normally
> worry about RISC-V failures, but they may indicate some kind of arch
> generic problem that is just not exposed on x86.

It could be a bug in the riscv go runtime port, but I think
it is reasonable to consider a latent problem in libnbd code
as these kind of Go/non-Go pointer problems are often
extremely non-deterministic & hard to identify.

Setting GODEBUG=cgocheck=1   or GODEBUG=cgocheck=2  can sometimes
get more info.

If you can get the test to core dump, then plain old GDB core
dump could also be useful - might identify which specific API
is being called, which can help restrict the size of the haystack
for code review.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd: Failed to update to Fedora 36 & OpenSUSE Leap 15.3

[Daniel P . Berrangé]

On Mon, May 30, 2022 at 09:04:25AM +0100, Richard W.M. Jones wrote:
> On Mon, May 30, 2022 at 09:25:11AM +0200, Martin Kletzander wrote:
> > On Sun, May 29, 2022 at 11:22:05AM +0100, Richard W.M. Jones wrote:
> > >
> > >I added this commit and regenerated the CI files:
> > >
> > >https://gitlab.com/nbdkit/libnbd/-/commit/b6a98aacbe22d599f000d4d1c84c27081ec06957
> > >https://gitlab.com/nbdkit/libnbd/-/commit/2439fd5c7a07b314ce47728c6fbae16b9a26dcdb
> > >
> > >However apparently gitlab CI cannot create the Fedora 36 & OpenSUSE
> > >Leap 15.3 containers:
> > >
> > >https://gitlab.com/nbdkit/libnbd/-/jobs/2519037050
> > >https://gitlab.com/nbdkit/libnbd/-/jobs/2519037032
> > >
> > >with weird errors:
> > >
> > >ERROR: Job failed: failed to pull image 
> > >"registry.gitlab.com/nbdkit/libnbd/ci-fedora-36:latest" with specified 
> > >policies [always]: Error response from daemon: manifest for 
> > >registry.gitlab.com/nbdkit/libnbd/ci-fedora-36:latest not found: manifest 
> > >unknown: manifest unknown (manager.go:203:0s)
> > >
> > >I have no idea what this means.  Does something else need to be done
> > >to create those containers?
> > >
> > 
> > Looks like the containers were not built.  I suspect the following rule:
> > 
> > - if: '$CI_PROJECT_NAMESPACE == "nbdkit" && $CI_PIPELINE_SOURCE == 
> > "push" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH'
> 
> Just to check, CI_PROJECT_NAMESPACE refers to the top-level
> (https://gitlab.com/nbdkit) not the name of the project
> (https://gitlab.com/nbdkit/libnbd)?

Yes, CI_PROJECT_NAMESPACE is the first path component - either the
group name or the user name 


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] libnbd: Failed to update to Fedora 36 & OpenSUSE Leap 15.3

[Daniel P . Berrangé]

On Mon, May 30, 2022 at 09:25:11AM +0200, Martin Kletzander wrote:
> On Sun, May 29, 2022 at 11:22:05AM +0100, Richard W.M. Jones wrote:
> > 
> > I added this commit and regenerated the CI files:
> > 
> > https://gitlab.com/nbdkit/libnbd/-/commit/b6a98aacbe22d599f000d4d1c84c27081ec06957
> > https://gitlab.com/nbdkit/libnbd/-/commit/2439fd5c7a07b314ce47728c6fbae16b9a26dcdb
> > 
> > However apparently gitlab CI cannot create the Fedora 36 & OpenSUSE
> > Leap 15.3 containers:
> > 
> > https://gitlab.com/nbdkit/libnbd/-/jobs/2519037050
> > https://gitlab.com/nbdkit/libnbd/-/jobs/2519037032
> > 
> > with weird errors:
> > 
> > ERROR: Job failed: failed to pull image 
> > "registry.gitlab.com/nbdkit/libnbd/ci-fedora-36:latest" with specified 
> > policies [always]: Error response from daemon: manifest for 
> > registry.gitlab.com/nbdkit/libnbd/ci-fedora-36:latest not found: manifest 
> > unknown: manifest unknown (manager.go:203:0s)
> > 
> > I have no idea what this means.  Does something else need to be done
> > to create those containers?
> > 
> 
> Looks like the containers were not built.  I suspect the following rule:
> 
> - if: '$CI_PROJECT_NAMESPACE == "nbdkit" && $CI_PIPELINE_SOURCE == "push" 
> && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH'
> 
> unles CI_DEFAULT_BRANCH is already set.  I'm not sure who sets that
> (whether this is in the ci/cd settings or part of something else)
> because that was added not so long ago and I did not keep up with the
> changes.  If that is already set, then it might be that you need to push
> with an extra variable according to the comment in
> /ci/gitlab/container-templates.yml:

Any variable starting with 'CI_' prefix is something defined by
GitLab as standard. Not all variables are defined in all scenarios.
For example merge request related variables aren't defined in pipelines
triggered by branch pushes.

CI_DEFAULT_BRANCH is a variable always defined though, and its defaults
to 'master'.

IOW, this rule means that containers are not rebuilt in context of
the upstream project except on pushes to 'master'.

> #   - Push to default branch:
> #   -> rebuild if dockerfile changed, no cache
> #   - Otherwise
> #   -> rebuild if LIBVIRT_CI_CONTAINERS=1, no cache,
> #  to pick up new published distro packages or
> #  recover from deleted tag
> #
> # For forks
> #   - Always rebuild, with cache
> #
> 
> so you could try pushing such commits with something like:
> 
> git push -o ci.variable="LIBVIRT_CI_CONTAINERS=1"
> 
> but as I said I did not keep up with these changes and Dan will know for
> sure which one is needed.

If you pushed a commit which containers a dockerfile change it should
not have been required to set this variable, but yes, this variable
can force the build if unexpected problems arise.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [nbdkit PATCH] RFC: blocksize: Add test for sharding behavior

[Daniel P . Berrangé]

On Thu, May 26, 2022 at 09:58:50AM +0100, Richard W.M. Jones wrote:
> 
> Is there any way to do this without the literal sleeps?  Gitlab CI in
> particular appears to be very contended (I guess it runs in parallel
> on huge systems with vast numbers of unrelated containers).  I've seen
> threads being created that are so starved they never run at all even
> in tests running for many tens of seconds.

IIUC, GitLab uses GCE  "spot" VMs for its shared runners

  https://cloud.google.com/compute/docs/instances/spot

TL;DR; these are massively cheaper than normal VMs (60-91% cheaper),
but GCE might pre-empt the VM if it needs to reclaim resources for
more important VMs on the host.

IOW, we have to expect a (usually low level) of non-determinsitic
failures and stalls from the CI jobs using shared runners, and
be willing to hit the restart job button if a problem occurs.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH] always 'max' for the appliance CPU model on all targes except ppc

[Daniel P . Berrangé]

On Thu, May 26, 2022 at 10:10:02AM +0200, Andrew Jones wrote:
> On Wed, May 25, 2022 at 05:13:53PM +0100, Peter Maydell wrote:
> > On Wed, 25 May 2022 at 16:07, Laszlo Ersek  wrote:
> ...
> > >   Therefore it seems that starting with qemu-4.2, but strictly preceding
> > >   qemu-7.0, "-cpu max" and "-cpu host" are not "identical" when KVM is
> > >   enabled; "-cpu max" has more features. Because of that, I think there
> > >   are two options:
> > >
> > >   (a) This extra feature is actually harmless, so we should only update
> > >   the commit message (i.e., generally speaking, "-cpu max" has been
> > >   a superset of "-cpu host" on KVM and of "-cpu cortex-a57" on TCG).
> > >
> > >   (b) The feature actually presents a problem, and qemu in [v4.2.0,
> > >   v7.0.0) will not start when KVM accel and "-cpu max" are requested
> > >   simultaneously. In this case, I think the appliance needs to stick
> > >   with "-cpu host" on KVM.
> > 
> > I don't understand why you think these are the only two options. The
> > actual situation is:
> > 
> >  (c) -cpu max and -cpu host have always been identical on KVM,
> >  and this commit does not change that.
> >  There happens to have been a QOM property 'sve-max-vq' on 'max'
> >  that should not have existed there and that nobody can actually have
> >  been usefully setting, but now there isn't.
> >
> 
> Hi Peter,
> 
> I think I understand Laszlo's concern. If we advertise 'max' as
> effectively being an alias for 'host' when accel=kvm, then we
> should be able to take any given '-cpu max,...' command line
> parameter and replace it with '-cpu host,...' and have it still
> work. That's not the case, at least when sve-max-vq, and I think
> maybe also pauth-impdef, are used.
> 
> Also, if we did want max and host to be aliases for accel=kvm, then
> that implies we need max to work for all '-cpu host,...' with
> accel=tcg as well. And, in that case, we'd need max with TCG to
> "support" kvm-no-adjvtime and kvm-steal-time, which doesn't make
> much sense.
> 
> I think the "solution" is to not infer that max and host are
> effectively aliases allowing seamless transition from tcg to kvm
> and back. One may treat them as aliases when any additional
> properties provided are from the intersection of their supported
> properties, though.
> 
> In summary, if an appliance doesn't provide additional CPU properties
> or it selects only properties that intersect TCG and KVM, then,
> regarding the CPU, when 'max' is used, it can seamlessly change the
> accelerator. Otherwise, while the appliance can leave the CPU as 'max'
> in all cases, it will need to modify selected CPU properties depending
> on the accelerator.

We've never said that 'max' is the same for TCG and KVM, nor do
apps using it require/expect that to be the case.

It is simply intended to expose the maximum featureset available to
any given accelerator backend. On KVM "maximum featureset" is the
same as "host" as you can't expose more than what the hardware has,
while on TCG "maximum featureset" is the most that emulation supports.

The intent is/was that it serves as good CPU choice for apps which
maximises features available, without them needing to think.

Essentially every app should pick 'max' by default unless they need
to restrict features for sake of live migration compatibility, or
need to select a very specific model for some functional reason.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

[Libguestfs] [PATCH] always 'max' for the appliance CPU model on all targes except ppc

[Daniel P . Berrangé]

The current logic for selecting CPU model to use with the appliance
selects 'max' for all architectures except for ppc64le and aarch64.

On aarch64, it selects 'host' if KVM is available which is identical
to 'max'. For TCG it selects 'cortex-a57'. The 'max' model is a
superset of 'cortex-a57', so it is reasonable to use 'max' for TCG
too.

On ppc64, it selects no CPU model due to a historical bug where
using 'host' would break ability to fallback to TCG. Unfortunately
we can't use 'max' on PPC as this is actually an old G4 vintage
32-bit model, rather than a synonym for 'host' / all-TCG-features
as on other targets.

We can at least simplify the code to use 'max' in all scenarios
for appliance CPU model, and simply skip a CPU model for PPC.

Signed-off-by: Daniel P. Berrangé 
---
 docs/C_SOURCE_FILES|  1 -
 lib/Makefile.am|  1 -
 lib/appliance-cpu.c| 93 --
 lib/guestfs-internal.h |  3 --
 lib/launch-direct.c| 25 +---
 lib/launch-libvirt.c   | 40 --
 po/POTFILES|  1 -
 7 files changed, 27 insertions(+), 137 deletions(-)
 delete mode 100644 lib/appliance-cpu.c

diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES
index a367c93a0..a26487d99 100644
--- a/docs/C_SOURCE_FILES
+++ b/docs/C_SOURCE_FILES
@@ -283,7 +283,6 @@ lib/actions-6.c
 lib/actions-support.c
 lib/actions-variants.c
 lib/alloc.c
-lib/appliance-cpu.c
 lib/appliance-kcmdline.c
 lib/appliance-uefi.c
 lib/appliance.c
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 212bcb94a..40a4c9ac3 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -71,7 +71,6 @@ libguestfs_la_SOURCES = \
actions-variants.c \
alloc.c \
appliance.c \
-   appliance-cpu.c \
appliance-kcmdline.c \
appliance-uefi.c \
available.c \
diff --git a/lib/appliance-cpu.c b/lib/appliance-cpu.c
deleted file mode 100644
index 54ac6e2e3..0
--- a/lib/appliance-cpu.c
+++ /dev/null
@@ -1,93 +0,0 @@
-/* libguestfs
- * Copyright (C) 2009-2020 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
- */
-
-/**
- * The appliance choice of CPU model.
- */
-
-#include 
-
-#include 
-#include 
-
-#include "guestfs.h"
-#include "guestfs-internal.h"
-
-/**
- * Return the right CPU model to use as the qemu C<-cpu> parameter or
- * its equivalent in libvirt.  This returns:
- *
- * =over 4
- *
- * =item C<"host">
- *
- * The literal string C<"host"> means use C<-cpu host>.
- *
- * =item C<"max">
- *
- * The literal string C<"max"> means use C<-cpu max> (the best
- * possible).  This requires awkward translation for libvirt.
- *
- * =item some string
- *
- * Some string such as C<"cortex-a57"> means use C<-cpu cortex-a57>.
- *
- * =item C
- *
- * C means no C<-cpu> option at all.  Note returning C
- * does not indicate an error.
- *
- * =back
- *
- * This is made unnecessarily hard and fragile because of two stupid
- * choices in QEMU:
- *
- * =over 4
- *
- * =item *
- *
- * The default for C is to emulate a
- * C (WTF?).
- *
- * =item *
- *
- * We don't know for sure if KVM will work, but C<-cpu host> is broken
- * with TCG, so we almost always pass a broken C<-cpu> flag if KVM is
- * semi-broken in any way.
- *
- * =back
- */
-const char *
-guestfs_int_get_cpu_model (int kvm)
-{
-#if defined(__aarch64__)
-  /* With -M virt, the default -cpu is cortex-a15.  Stupid. */
-  if (kvm)
-return "host";
-  else
-return "cortex-a57";
-#elif defined(__powerpc64__)
-  /* See discussion in https://bugzilla.redhat.com/show_bug.cgi?id=1605071 */
-  return NULL;
-#else
-  /* On most architectures we can use "max" to get the best possible CPU.
-   * For recent qemu this should work even on TCG.
-   */
-  return "max";
-#endif
-}
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 16755cfb3..33037a26d 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -718,9 +718,6 @@ extern const char *guestfs_int_drive_protocol_to_string 
(enum drive_protocol pro
 /* appliance.c */
 extern int guestfs_int_build_appliance (guestfs_h *g, char **kernel, char 

Re: [Libguestfs] nbdkit worker thread shutdown

[Daniel P . Berrangé]

On Thu, May 05, 2022 at 02:24:12PM +0100, Nikolaus Rath wrote:
> On May 05 2022, "Richard W.M. Jones"  wrote:
> > On Thu, May 05, 2022 at 08:59:56AM +0100, Nikolaus Rath wrote:
> >> Hello,
> >> 
> >> When nbdkit calls a plugin's unload() method, is it guaranteed that all
> >> pending requests have been handled (and all worker threads exited)?
> >> 
> >> (I would have expected this to be the case, but I'm getting errors from
> >> threads accessing data that my unload() handler frees, so I wanted to
> >> confirm my assumption).
> >
> > .unload is literally called just before dlclose (see
> > server/backend.c:backend_unload) so if a thread in nbdkit was to
> > access something in your plugin after .unload then that would be quite
> > a serious bug in nbdkit.  It would be bound to crash because the code
> > and data has been unloaded from the process.  This implies too that
> > all connections would have been closed already.
> >
> > Normally the server has no view into your plugin except through the
> > pointers in the plugin structure, and it shouldn't be using those
> > after .unload.  Can you give us some clue as to what exactly nbdkit is
> > accessing in your plugin?
> 
> It *looks like* one of the nbdkit-started worker threads is still
> executing the plugin's read() handler at the time that a different
> thread is executing the plugin's unload() handler (background:
> https://github.com/archiecobbs/s3backer/issues/180).
> 
> However, this may not actually be the case. I wanted to confirm that
> this is not expected behavior before diving into this deeply.

Perhaps try running the program under GDB and capture the output
from 'thread apply all backtrace' when the assert fires. It should
give some insight into what threads are doing

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH nbdkit INCOMPLETE] readahead: Rewrite this filter so it prefetches using .cache

[Daniel P . Berrangé]

On Tue, Apr 19, 2022 at 04:05:50PM +0100, Richard W.M. Jones wrote:
> On Tue, Apr 19, 2022 at 04:31:44PM +0200, Laszlo Ersek wrote:
> > On 04/19/22 14:47, Richard W.M. Jones wrote:
> > > The previous readahead filter did not work well and we have stopped
> > > using it in virt-v2v.  However the concept is a good one if we can
> > > make it work.  This commit completely rethinks the filter.
> > 
> > I started responding to the cover letter, "what is the motivation for
> > this? furthermore, can we add this as a new filter instead, so
> > comparisons (back-and-forth) can be made easily"?
> 
> TBH the motivation is not very clear, but here are several current
> reasons:
> 
> (1) Virt-v2v does some random access conversion up front, but then
> spends most of its time sequentially copying, using originally
> “qemu-img convert” and now nbdcopy.  It was thought that
> nbdkit-readahead-filter could help in some way, but the current
> implementation does not help, and in fact had some pretty negative
> effects.
> 
> If we believe that nbdkit can help (which is not entirely clear), then
> fixing nbdkit-readahead-filter is worthwhile.
> 
> For virt-v2v it's often the case that the source is remote, over a
> relatively slow network.  On the one hand, prefetching would be good
> because it gets the data across the network and into the local cache.
> On the other hand, it's likely we can saturate most networks, so
> prefetching will make no difference or be net negative.

If we have a guest kernel involved, its possible that is already doing
its own readahead. Possibly doing it in nbdkit could be more effective
due to its running without needing any guest VM exits, but on the flip
side the kernel has more information about where it wants to readahead.
ie nbdkit has to assume reads are sequential, but the kernel could do
non-sequential readahead if it knows the data will be from a different
region of the file.

Similarly if you have something like qcow2 where physical clusters 
are non-sequential wrt guest clusters, having read-ahead below the
qcow2 layer is not likely effective.

> (2) Libvirt XML exposes the "readahead" setting of the qemu curl driver.
> https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms
> https://gitlab.com/qemu/qemu/-/blob/master/block/curl.c#L897
> 
> In order to convert libvirt to use nbdkit-curl-plugin it's therefore
> felt that we ought to have an equivalent feature in nbdkit, although
> another way to fix this would simply be to ignore this setting.

It isn't a requirement from libvirt's side. If nbdkit doesn't support
it, we'll simply report VIR_ERR_CONFIG_UNSUPPORTED if a readahead value
is set. In practice I suspect that scenario will affect no-one.

> (3) We have a readahead filter which definitely doesn't work, so we
> ought to either fix it or retire it.
> 
> (4) A feature of nbdkit is that you can write simple plugins (eg. no
> threads, no fancy features), and then layer filters on top to add some
> of those features (caching, logging, readahead), so that would be a
> reason to have a readahead filter.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v v2] lib: Use an ACL to allow qemu to access the v2v directory

[Daniel P . Berrangé]

On Tue, Mar 22, 2022 at 05:34:25PM +0100, Laszlo Ersek wrote:
> On 03/22/22 16:48, Richard W.M. Jones wrote:
> > When using the libvirt backend and running as root, libvirt will run
> > qemu as a non-root user (eg. qemu:qemu).  The v2v directory stores NBD
> > endpoints that qemu must be able to open and so we set the directory
> > to mode 0711.  Unfortunately this permits any non-root user to open
> > the sockets (since, by design, they have predictable names within the
> > directory).
> > 
> > So instead of using directory permissions, use an ACL which allows us
> > to precisely give access to the qemu user and no one else.
> > 
> > Reported-by: Xiaodai Wang
> > Thanks: Dr David Gilbert
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
> > ---
> >  lib/utils.ml | 34 +-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/utils.ml b/lib/utils.ml
> > index 757bc73c8e..48e6b6c82d 100644
> > --- a/lib/utils.ml
> > +++ b/lib/utils.ml
> > @@ -146,6 +146,26 @@ let backend_is_libvirt () =
> >let backend = fst (String.split ":" backend) in
> >backend = "libvirt"
> >  
> > +(* Get the local user that libvirt uses to run qemu when we are
> > + * running as root.  This is returned as an optional string
> > + * containing either the UID or username.
> > + * https://listman.redhat.com/archives/libguestfs/2022-March/028450.html
> > + *)
> > +let libvirt_qemu_uid () =
> > +  let conn = Libvirt.Connect.connect_readonly () in
> > +  let xml = Libvirt.Connect.get_capabilities conn in
> 
> Huh, I didn't know this existed already :)
> 
> > +  let doc = Xml.parse_memory xml in
> > +  let xpathctx = Xml.xpath_new_context doc in
> > +  let expr = "//secmodel[./model=\"dac\"]/baselabel[@type=\"kvm\"]/text()" 
> > in
> 
> So if you run "virsh capabilities", you get:
> 
> 
>   dac
>   0
>   +107:+107
>   +107:+107
> 
> 
> In case libvirt starts the domain with TCG, then I think the
> @type='qemu' filter should apply.

If you want to be perfectly correct you can distinguish KVM vs QEMU,
but in practice these are hardcoded to be identical for the 'dac'
driver. They only differ for the 'selinux' secmodel driver today.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH v2v] lib: Use an ACL to allow qemu to access the v2v directory

[Daniel P . Berrangé]

On Tue, Mar 22, 2022 at 02:35:54PM +, Richard W.M. Jones wrote:
> For fuller explanation see:
> https://bugzilla.redhat.com/show_bug.cgi?id=2066773#c1
> 
> I'm not very happy with this patch for a few reasons:
> 
>  - Does every distro use "qemu" as the user that runs qemu?

Not sure, but you can query this from libvirt

# virsh capabilities  | xmllint -xpath 
'//secmodel[./model="dac"]/baselabel[@type="kvm"]'  -
+107:+107

The base level here is the label that any files must have in order
to be writable by QEMU, using a default process label.

In the case of the 'dac' model this is a UID:GID pair (+ indicates
numeric ID, as opposed to a username with all numbers).

NB, this doesn't apply if you're overriding the default label to
use a distinct UID per VM, but I assume v2v isn't doing that and
controls its own VMs

>  - Having to run an external process (not a big deal, but a bit clumsy)

In theory libacl gives you programmatic API for this.

>  - Aren't ACLs actually deprecated?

Not that I know of.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [p2v PATCH] build: remove gnulib

[Daniel P . Berrangé]

On Mon, Mar 21, 2022 at 04:16:18PM +0100, Laszlo Ersek wrote:
> It turns out that all we need from gnulib @ 253f29d8b391 is xstrtoull(),
> ignore_value(), and assure(), when building on Fedora 35 anyway.

I guess the question is what OS distros is p2v targetting ?

With very rare exceptions, Fedora has never really needed much of the
platform portability stuff from gnulib.

The primary benefit from gnulib comes if needing to target FreeBSD,
macOS, Windows or somewhat old Linux like older RHEL versions, fixing
their many flaws to make them operate more like modern Fedora would.

> Constructing this patch must be the most arbitrary "programming" I've ever
> done. It started with capturing the output of "gnulib-tool" (invoked
> through "autogen.sh" -> "bootstrap"), then trimming it as much as
> possible, guided by libguestfs commit 0f54df53d26e ("build: Remove
> gnulib.", 2021-04-08), then filling in any new gaps.
> 
> (The "manywarnings" functionality falls victim to this patch as well -- if
> that change was good enough for libguestfs, then so should it be for
> virt-p2v.)

That need not be the case. It is pretty trivial to use "manywarnings"
functionality in isolation from anything else related to gnulib, as
it is nicely self contained. Just copy the manywarnings.m4 and
warnings.m4 files out of gnulib.git into your local m4/ directory.

We did this for libvirt and quite a few other virt userspace projects
before we eventually switched to using meson.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Running lcitool in nbdkit

[Daniel P . Berrangé]

On Mon, Mar 14, 2022 at 10:41:05AM +, Richard W.M. Jones wrote:
> I rebased Martin's tree[1] on top of libvirt-ci head, and reran
> lcitool.  There are several places where I changed the generated files
> in nbdkit.git and where those changes are overwritten (I didn't
> realise those files were generated).  I think I can fix most of them.
> 
> But a couple of larger problems I can see ...
> 
> (a) lcitool wants to make this change:
> 
> diff --git a/ci/containers/fedora-rawhide.Dockerfile 
> b/ci/containers/fedora-rawhide.Dockerfile
> index 7e0a8d25..d4409f87 100644
> --- a/ci/containers/fedora-rawhide.Dockerfile
> +++ b/ci/containers/fedora-rawhide.Dockerfile
> @@ -6,10 +6,7 @@
>  
>  FROM registry.fedoraproject.org/fedora:rawhide
>  
> -RUN dnf update -y && \
> -dnf install 'dnf-command(config-manager)' -y && \
> -dnf config-manager -y 
> --add-repo=http://dl.fedoraproject.org/pub/alt/rawhide-kernel-nodebug/fedora-rawhide-kernel-nodebug.repo
>  && \
> -dnf update -y --nogpgcheck fedora-gpg-keys && \
> +RUN dnf update -y --nogpgcheck fedora-gpg-keys && \
> 
> 
> I added "dnf install 'dnf-command(config-manager)'" in:
> https://gitlab.com/nbdkit/nbdkit/-/commit/e359fab8c682436faff914591b5ff42e29e77b9c
> because the dnf config-manager command doesn't work.
> 
> The dnf config-manager command in turn was added earlier in Martin's
> original commit.  I don't know if Martin added that manually or if an
> earlier version of lcitool generated it, but it is necessary in order
> to use the nodebug kernel, and using the nodebug kernel is necessary
> to avoid performance problems, timeouts and out of memory problems
> when using libguestfs on Rawhide.

Ok, that need for an extra special repo for rawhide no-debug kerenl
isn't something lcitool knows how to cope with right now. So must
be been manually editted.

> (b) lcitool wants to remove my perl conditionalization changes:
> 
> @@ -312,11 +309,10 @@ mingw32-fedora-35:
>  - mingw32-fedora-35-container
>allow_failure: false
>variables:
> -NAME: fedora-35
>  CROSS: mingw32
> -PERL: skip
> -RUST: skip
>  GOLANG: skip
> +NAME: fedora-35
> +RUST: skip
> 
> which I added here:
> 
> https://gitlab.com/nbdkit/nbdkit/-/commit/414ee371ee2b1a12c6d45466d041322b247723eb
> 
> I'm not clear where I should have added those.  The rust
> conditionalization stuff doesn't seem to be part of lcitool so I guess
> it's generated from another file somewhere.
> 
> Be nice if original files were called "file.in" or something like that
> so we can tell what files are generated and what are originals.

The only manually editted file is ci/manifest.yml. Everything else in
the ci/ directory (the ci/gitlab.yml, ci/containers/* and ci/cirrus/*)
is auto-generated from what's in the manifest.yml and shouldn't be
touched.

So you'll need to add the PERL=skip variable to manifest.yml

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Running lcitool in nbdkit

[Daniel P . Berrangé]

On Mon, Mar 14, 2022 at 10:14:37AM +, Richard W.M. Jones wrote:
> On Mon, Mar 14, 2022 at 10:01:46AM +, Richard W.M. Jones wrote:
> > Martin - I guess you must have created that nbdkit.yml file at some
> > point, but I can't find it in your fork.
> 
> Found it:
> 
> https://gitlab.com/nertpinx/libvirt-ci/-/commits/_nbdkit2
> 
> I guess we're still missing almalinux support in libvirt-ci though.
> 
> $ ../libvirt-ci/lcitool manifest ci/manifest.yml
> [ERROR]: An unexpected error occurred
> Traceback (most recent call last):
>   File "/home/rjones/d/libvirt-ci/guests/lcitool/lcitool/manifest.py", line 
> 87, in _normalize
> facts = inventory.target_facts[target]
> KeyError: 'almalinux-8'

Current upstream has almalinux-8, so I think this nbdkit2 branch is just
a little outdated.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Running lcitool in nbdkit

[Daniel P . Berrangé]

On Mon, Mar 14, 2022 at 10:01:46AM +, Richard W.M. Jones wrote:
> On Mon, Mar 14, 2022 at 09:18:04AM +0000, Daniel P. Berrangé wrote:
> > On Sat, Mar 12, 2022 at 10:50:21AM +, Richard W.M. Jones wrote:
> > > Am I doing this wrong?  [see log below]  This is using the upstream
> > > libvirt-ci checked out just now from gitlab, but it seems as if nbdkit
> > > has to be listed as a project under that repo.
> > > 
> > > "./lcitool projects" shows libnbd but not nbdkit.
> > 
> > Indeed, there's only libnbd there. Presumably nbdkit is something
> > Martin hasn't got around to adding ?
> > 
> > With recentish changes though there's no longer any need to add
> > projects directly to libvirt-ci.git. They can be kept locally
> > in the project eg
> > 
> >nbdkit.git/ci/projects/nbdkit.yml
> > 
> > then use  'lcitool --data-dir ci' and it'll find the nbdkit.yml
> > when searching for projects.
> > 
> > libvirt-ci.git will only need modifying it distro package
> > mappings need additions/changes.
> 
> I copied 
> libvirt-ci.git/guests/lcitool/lcitool/ansible/vars/projects/libnbd.yml
> to ci/projects/nbdkit.yml.  I'll probably have to adjust the packages
> in that file, but it did allow me to re-run the command above and
> regenerate the templates.
> 
> Martin - I guess you must have created that nbdkit.yml file at some
> point, but I can't find it in your fork.
> 
> > > A second question, since the libvirt-ci project does a bunch of stuff
> > > with ansible & containers, regenerating the files like this doesn't
> > > require ansible & containers?  (I don't want to run those on my
> > > development machine.)
> > 
> > ansible is only used when managing VMs, no need for it with containers.
> 
> I meant would the "manifest" subcommand do anything weird (ie. not
> just updating the templates), but it seems fine.

Yep, the manifest command doesn't do anything with VMs or ansible.

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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] Running lcitool in nbdkit

[Daniel P . Berrangé]

On Sat, Mar 12, 2022 at 10:50:21AM +, Richard W.M. Jones wrote:
> Am I doing this wrong?  [see log below]  This is using the upstream
> libvirt-ci checked out just now from gitlab, but it seems as if nbdkit
> has to be listed as a project under that repo.
> 
> "./lcitool projects" shows libnbd but not nbdkit.

Indeed, there's only libnbd there. Presumably nbdkit is something
Martin hasn't got around to adding ?

With recentish changes though there's no longer any need to add
projects directly to libvirt-ci.git. They can be kept locally
in the project eg

   nbdkit.git/ci/projects/nbdkit.yml

then use  'lcitool --data-dir ci' and it'll find the nbdkit.yml
when searching for projects.

libvirt-ci.git will only need modifying it distro package
mappings need additions/changes.

> A second question, since the libvirt-ci project does a bunch of stuff
> with ansible & containers, regenerating the files like this doesn't
> require ansible & containers?  (I don't want to run those on my
> development machine.)

ansible is only used when managing VMs, no need for it with containers.


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 :|
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

[Daniel P . Berrangé]

On Wed, Jan 12, 2022 at 03:14:43PM +0100, Laszlo Ersek wrote:
> On 01/12/22 12:26, Daniel P. Berrangé wrote:
> > On Tue, Jan 11, 2022 at 11:22:56AM +, Richard W.M. Jones wrote:
> >> On Tue, Jan 11, 2022 at 12:15:13PM +0100, Laszlo Ersek wrote:
> >>> By the way, we have more "offenders" left:
> >>>
> >>> - three in "bundled/libvirt-ocaml/libvirt_c_common.c":
> >>
> >> I'm quite sure this happens in a lot of places.
> >>
> >> BTW libvirt-ocaml is a separate project, so fixes should go to:
> >>
> >> https://libvirt.org/git/?p=libvirt-ocaml.git;a=summary
> >>
> >> (and really we should remove all bundled code - I can't recall why it
> >> was added, but it's not needed now.)
> > 
> > NB that's a read-only mirror, the primary repo is:
> > 
> >   https://gitlab.com/libvirt/libvirt-ocaml/
> > 
> > and takes patches via merge requests
> 
> What is the status of
> "contrib/0001-Add-Libvirt.Domain.get_cpu_stats_total.patch"?
> 
> It is affected by the issue, but I totally don't want to touch it if the
> patch is not actually applied. It is a patch from Hu Tao
> , from 2012, which got added in 2018 (commit
> 669cbc5a0ce1), but not actually merged. I can't see any machinery within
> the project that would apply it.
> 
> My recommendation would be to just delete this directory (contrib). I
> certainly can't do anyting about this patch (prove it right or wrong,
> test it, etc). On the other hand, fixing some instances of
> 
>   Store_field (block, fieldnr, caml_copy_... (...));
> 
> but not the instance in this explicit patch, feels wrong.
> 
> If we can't drop the "contrib" directory, I'll abandon this effort.

I don't see any benefit in keeping this patch in the contrib
directory. Either it is correct and should be applied properly,
or it is incorrect and should be dropped. Leaving it in contrib
isn't helping anyone. It will be present in git history  if anyone
wants to get it back later.


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

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

Re: [Libguestfs] [v2v PATCH] convert/libosinfo-c.c: turn caml_copy_*() return blocks into root values

[Daniel P . Berrangé]

On Tue, Jan 11, 2022 at 11:22:56AM +, Richard W.M. Jones wrote:
> On Tue, Jan 11, 2022 at 12:15:13PM +0100, Laszlo Ersek wrote:
> > By the way, we have more "offenders" left:
> > 
> > - three in "bundled/libvirt-ocaml/libvirt_c_common.c":
> 
> I'm quite sure this happens in a lot of places.
> 
> BTW libvirt-ocaml is a separate project, so fixes should go to:
> 
> https://libvirt.org/git/?p=libvirt-ocaml.git;a=summary
> 
> (and really we should remove all bundled code - I can't recall why it
> was added, but it's not needed now.)

NB that's a read-only mirror, the primary repo is:

  https://gitlab.com/libvirt/libvirt-ocaml/

and takes patches via merge requests


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

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

Re: [Libguestfs] [PATCH 0/3] resolve conflict between manual and libvirt-assigned PCI addresses

[Daniel P . Berrangé]

On Tue, Jan 04, 2022 at 02:02:57PM +, Richard W.M. Jones wrote:
> On Tue, Jan 04, 2022 at 10:33:14AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Dec 23, 2021 at 11:36:58AM +0100, Laszlo Ersek wrote:
> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034160
> > > 
> > > The first patch extends our current  hack, moving the
> > > virtio-net-pci device to slot 0x1E, where it is very unlikely to
> > > conflict with any libvirt-assigned PCI address.
> > 
> > Remind me why we still need to use  ? A need
> > for this obviously reflects a failure of libvirt to address the
> > libguestfs requirements. Can we fix the root cause here with proper
> > XML support for whatever feature is missing. 
> 
> Laszlo actually fixed this properly in the case we care about (modern
> libvirt), if you look at the 3rd patch which went upstream:
> 
> https://github.com/libguestfs/libguestfs/commit/5858c2cf6c24b3776e3867eafd9d86a1f4912d9c
> 
> Ignoring the old libvirt case, the only reason we still need
>  is to allow us to set TMPDIR accurately.
> (Otherwise if you recall we can get a TMPDIR from an unrelated run of
> libguestfs that happens to share the same session libvirtd).

Hmm, right so that's originally because of the use of 'snapshot=on'
but IIUC you no longer use that and instead create overlays explicitly.

It is still possible QEMU creates other temporary files, and for
that matter so could libvirtd, but the qemu:arg only affects the
former.

I guess we can't set TMPDIR before calling virConnectOpen because
while that would correctly influence the spawned libvirtd, it
would also influence anything else in the process using libguestfs
in other threads.

This is painful :-( Basically would need  ability to pass a
custom TMPDIR for libvirtd into the virConnectOpen call, via
a URI parameter.

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

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

Re: [Libguestfs] [PATCH 0/3] resolve conflict between manual and libvirt-assigned PCI addresses

[Daniel P . Berrangé]

On Thu, Dec 23, 2021 at 11:36:58AM +0100, Laszlo Ersek wrote:
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034160
> 
> The first patch extends our current  hack, moving the
> virtio-net-pci device to slot 0x1E, where it is very unlikely to
> conflict with any libvirt-assigned PCI address.

Remind me why we still need to use  ? A need
for this obviously reflects a failure of libvirt to address the
libguestfs requirements. Can we fix the root cause here with proper
XML support for whatever feature is missing. 

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

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

Re: [Libguestfs] [v2v PATCH v2] convert_linux: translate the first CD-ROM's references in boot conf files

[Daniel P . Berrangé]

On Fri, Dec 17, 2021 at 10:31:40AM +, Richard W.M. Jones wrote:
> On Fri, Dec 17, 2021 at 10:21:39AM +, Richard W.M. Jones wrote:
> > So we need to keep things going for them even if RHEL doesn't
> > necessarily support their use case.  That means making conversions of
> > RHEL 5/6 work on RHEL 9
> 
> I mean "on RHEL 9 with Q35", my assumption being that either RHEL 9
> has already dropped i440fx, or that using i440fx will produce some big
> scary deprecation warnings (not sure which is going to happen).

i440fx will exist for the purpose of incoming migrations from RHEL-8
because OpenStack still used i440fx for the lifetime of 8.

It should not be used for newly provisioned VMs on 9, and is likely
to be marked as deprecated when querying from QEMU with QMP.


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

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

Re: [Libguestfs] [PATCH libnbd CI] ci: Drop Fedora 33, add Fedora 35

[Daniel P . Berrangé]

On Mon, Dec 13, 2021 at 03:17:56PM +, Richard W.M. Jones wrote:
> On Mon, Dec 13, 2021 at 04:07:27PM +0100, Martin Kletzander wrote:
> > On Mon, Dec 13, 2021 at 02:40:40PM +, Richard W.M. Jones wrote:
> > >
> > >I pushed this (commit 202d0ecad) in the hope it might fix the tests.
> > >However Martin indicates that the error message might be nothing to do
> > >with the Fedora version.  Let's see.
> > >
> > 
> > It looks like everything went swimmingly the next time around even
> > before this patch:
> > 
> > https://gitlab.com/nbdkit/libnbd/-/pipelines/428406657
> 
> Yes, it does look like a temporary docker pull problem.
> 
> > so it clear wasn't anything with Fedora 33, although updating to F35 is
> > definitely worth doing.
> 
> Yup.
> 
> > It did not happen to be before here with the containers for libnbd.  I
> > guess it can happen any time if there is an issue on the remote side
> > when pushing a container for example.
> > 
> > However I am facing few issues with nbdkit CI that I am still trying to
> > finish and on and off working on it.  The issue there is that the tests
> > time out because of all the qemu invocations, especially with TCG.  I am
> > trying to think about what tests to skip and maybe split the load
> > between different distros.
> 
> So you can disable the libguestfs-based tests if TCG is too slow:
> 
> https://src.fedoraproject.org/rpms/nbdkit/blob/rawhide/f/nbdkit.spec#_695
> 
> We probably ought to formalise that into a configure flag, but at the
> moment the above trick should be reliable because we use the same in
> Fedora builds on slow arches.
> 
> Also re nbdkit CI, it turns out Windows builds were broken for a while
> (I fixed it a few days ago).  Would be nice to CI this!

Whuen using libvirt-ci manifest to generate gitlab config and dockerfiles,
it is pretty trivial to include mingw builds in the matrix.

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

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

Re: [Libguestfs] Minimum OCaml compiler version (2021/2022 edition)

[Daniel P . Berrangé]

On Tue, Nov 09, 2021 at 09:04:40AM +, Richard W.M. Jones wrote:
> Previously:
> https://listman.redhat.com/archives/libguestfs/2020-March/msg00063.html
> https://listman.redhat.com/archives/libguestfs/2017-September/msg00203.html
> 
> Our current minimum version across projects is 4.03.
> 
> We still use "noalloc" in a few places which causes this warning:
> 
>   ocamlopt.opt -warn-error +A-3 -c NBDKit.ml -o NBDKit.cmx
>   File "NBDKit.ml", line 155, characters 0-70:
>   155 | external set_name : string -> unit = "ocaml_nbdkit_set_name" "noalloc"
> ^^
>   Alert deprecated: [@@noalloc] should be used instead of "noalloc"
> 
> I just noticed now that this change was made in 4.03 -- I will go
> ahead and fix this everywhere today.
> 
> Should we move to a newer minimum version?  If we moved to 4.07 then
> we could also get rid of the warnings about Pervasives (replaced by
> Stdlib), eg:
> 
>   File "std_utils.ml", line 329, characters 26-44:
>   329 | let sort_uniq ?(cmp = Pervasives.compare) xs =
>   ^^
>   Alert deprecated: module Stdlib.Pervasives
>   Use Stdlib instead.
> 
> However 4.07 was only released in 2018, and this would mean removing
> RHEL 7 compatibility (officially -- it's sort of unofficially not
> supported already).  More irritatingly, FreeBSD is stuck on 4.05.

Note both libvirt and QEMU have explicitly dropped RHEL-7
as a supported build platform[1], under the rule that platforms
are dropped 2 years after the new major version is released.

It has been > 2 years since RHEL-8 GA, so they dropped RHEL-7.

IOW even if new libguestfs can build on RHEL-7, you'll be
stuck with old libvirt and QEMU.

> Here are the common distros and versions (< 4.07 marked with '*'):
> 
>   Arch (Extra) OCaml 4.12
>   Debian stableOCaml 4.11
>   Debian testing   OCaml 4.11
>   Fedora 31OCaml 4.08
>   Fedora 35OCaml 4.12
>   FreeBSD (ports)  OCaml 4.05  *
>   OpenSUSE OCaml 4.13
>   RHEL 7   OCaml 4.05  *
>   RHEL 8   OCaml 4.07
>   RHEL 9   OCaml 4.11
>   Ubuntu 16.04 OCaml 4.02  *

Ubuntu 16.04 is explicitly dropped by both QEMU and libvirt too
a long time ago. It falls under both the 2 year cut off rule,
an the maximum of two major releases in concurrently rules.

NB, QEMU/libvirt only look at Ubuntu LTS releases for cutoffs

>   Ubuntu 18.04 OCaml 4.05  *
>   Ubuntu 20.04 OCaml 4.08
>   Ubuntu 21.04 OCaml 4.11
> 
> And here are the release dates of the OCaml compiler:
> 
>   OCaml versionRelease date
>   4.02 2014-08
>   4.03 2016-04
>   4.04 2016-11
>   4.05 2017-07
>   4.06 2017-11
>   4.07 2018-07
>   4.08 2019-06
>   4.09 2019-09
>   4.10 2020-02
>   4.11 2020-08
>   4.12 2021-02
>   4.13 2021-09

Regards,
Daniel

[1] https://www.qemu.org/docs/master/about/build-platforms.html
https://libvirt.org/platforms.html

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

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

Re: [Libguestfs] [virt-v2v RFC wave 2 03/10] convert/windows_virtio: restrict the warning with virtio-win.iso absent

[Daniel P . Berrangé]

On Tue, Nov 09, 2021 at 11:55:32AM +0100, Laszlo Ersek wrote:
> I'm asking now because these simplifications look technically possible
> even before I start investigating the "OVF video device" topic. I expect
> the latter to turn into an infinite mess, so if I can (or should) tack
> the cirrus cleanup patches to the end of my series, I figure I'd like to
> do that first.

On the QEMU side, assuming you don't care about guest OS dating back
from 1995, Cirrus should never be used under any circumstance [1].
Anywhere that might have used Cirrus should be switched to use VGA. So
if v2v does have any Cirrus related usage, I'd recommend to swap that
out as a priority.

Regards,
Daniel

[1] https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/
https://www.kraxel.org/blog/2019/09/display-devices-in-qemu/#tldr
-- 
|: 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 :|

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

Re: [Libguestfs] [PATCH libnbd v2 1/2] golang: Changes test license to LPGL

[Daniel P . Berrangé]

On Mon, Nov 01, 2021 at 11:30:07AM +, Richard W.M. Jones wrote:
> On Mon, Nov 01, 2021 at 09:32:37AM +0000, Daniel P. Berrangé wrote:
> > On Sun, Oct 31, 2021 at 06:20:05PM +, Richard W.M. Jones wrote:
> > > On Sun, Oct 31, 2021 at 06:59:32PM +0200, Nir Soffer wrote:
> > > > Having different license for the tests complicates everyone life for no
> > > > benefit. Change the license to LGPL2+ like the rest of the library.
> > > > 
> > > > Related discussion:
> > > > https://listman.redhat.com/archives/libguestfs/2021-October/msg00196.html
> > > > 
> > > > Signed-off-by: Nir Soffer 
> > > 
> > > I agree with this change.
> > > 
> > > Since this involves some code that I wrote originally, can you add:
> > > 
> > >   Signed-off-by: Richard W.M. Jones 
> > > 
> > > However I'm not the only author here (by quite a lot!):
> > > 
> > > $ git shortlog -s golang/src/libguestfs.org/libnbd/
> > > 13  Eric Blake
> > >  2  Nir Soffer
> > >  4  Richard W.M. Jones
> > > 
> > > so could be a good idea to ask Eric specifically for permission too
> > > (even though in both cases the © is likely assigned to Red Hat as our
> > > employer).
> > 
> > Looking at libnbd.git as a whole I see
> > 
> >   1 Author: anson <83398016+anson...@users.noreply.github.com>
> >   1 Author: Chris Lamb 
> > 324 Author: Eric Blake 
> >   1 Author: Laszlo Ersek 
> >  52 Author: Martin Kletzander 
> >   5 Author: Nir Soffer 
> >  33 Author: Nir Soffer 
> >   2 Author: Pino Toscano 
> > 936 Author: Richard W.M. Jones 
> > 
> > 
> > only two patches from non-Red Hat people, and both of those patches
> > are trivial bug fixes so could be said to be non-copyrigtable changes.
> > With this in mind, I would suggest that it is viable to remove all the
> > license headers across the codebase and add SPDX tags in their place.
> 
> While viable, we did discuss this already and decided against it for
> now -- we'll watch what qemu & libvirt are doing.

QEMU / libvirt aren't liekly to adopt SPDX for their main repos because
they both have a huge variety of copyright holders, so it is not viable
to get permission to replace the license headers with SPDX tags. The
kernel adopted SPDX despite not getting permission from all copyright
holders, but they went through some kind of auditing process with legal
oversight to have confidence that is is OK. I don't know enough about
the fine details to be able to suggest copying that approach.

libnbd is fortunately not in this situation because it is essentially
all (C) Red Hat.

IOW, don't wait for libvirt/QEMU - decide it for libnbd on its own
merits.

FWIW, I'm using SPDX tags for any new projects I create from scratch
going forward e.g. https://gitlab.com/bichon-project/bichon/


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

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

Re: [Libguestfs] [PATCH libnbd v2 1/2] golang: Changes test license to LPGL

[Daniel P . Berrangé]

On Sun, Oct 31, 2021 at 06:20:05PM +, Richard W.M. Jones wrote:
> On Sun, Oct 31, 2021 at 06:59:32PM +0200, Nir Soffer wrote:
> > Having different license for the tests complicates everyone life for no
> > benefit. Change the license to LGPL2+ like the rest of the library.
> > 
> > Related discussion:
> > https://listman.redhat.com/archives/libguestfs/2021-October/msg00196.html
> > 
> > Signed-off-by: Nir Soffer 
> 
> I agree with this change.
> 
> Since this involves some code that I wrote originally, can you add:
> 
>   Signed-off-by: Richard W.M. Jones 
> 
> However I'm not the only author here (by quite a lot!):
> 
> $ git shortlog -s golang/src/libguestfs.org/libnbd/
> 13  Eric Blake
>  2  Nir Soffer
>  4  Richard W.M. Jones
> 
> so could be a good idea to ask Eric specifically for permission too
> (even though in both cases the © is likely assigned to Red Hat as our
> employer).

Looking at libnbd.git as a whole I see

  1 Author: anson <83398016+anson...@users.noreply.github.com>
  1 Author: Chris Lamb 
324 Author: Eric Blake 
  1 Author: Laszlo Ersek 
 52 Author: Martin Kletzander 
  5 Author: Nir Soffer 
 33 Author: Nir Soffer 
  2 Author: Pino Toscano 
936 Author: Richard W.M. Jones 


only two patches from non-Red Hat people, and both of those patches
are trivial bug fixes so could be said to be non-copyrigtable changes.
With this in mind, I would suggest that it is viable to remove all the
license headers across the codebase and add SPDX tags in their place.

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

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

Re: [Libguestfs] [PATCH 1/9] qapi: New special feature flag "unstable"

[Daniel P . Berrangé]

On Tue, Oct 26, 2021 at 05:15:10PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote:
> >> Kevin Wolf  writes:
> >> 
> >> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> >> >> By convention, names starting with "x-" are experimental.  The parts
> >> >> of external interfaces so named may be withdrawn or changed
> >> >> incompatibly in future releases.
> >> >> 
> >> >> Drawback: promoting something from experimental to stable involves a
> >> >> name change.  Client code needs to be updated.
> >> >> 
> >> >> Moreover, the convention is not universally observed:
> >> >> 
> >> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
> >> >>   Looks accidental, but it's ABI since 4.2.
> >> >> 
> >> >> * QOM types "memory-backend-file", "memory-backend-memfd",
> >> >>   "memory-backend-ram", and "memory-backend-epc" have a property
> >> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
> >> >>   stable despite its name.
> >> >> 
> >> >> We could document these exceptions, but documentation helps only
> >> >> humans.  We want to recognize "unstable" in code, like "deprecated".
> >> >> 
> >> >> Replace the convention by a new special feature flag "unstable".  It
> >> >> will be recognized by the QAPI generator, like the existing feature
> >> >> flag "deprecated", and unlike regular feature flags.
> >> >> 
> >> >> This commit updates documentation and prepares tests.  The next commit
> >> >> updates the QAPI schema.  The remaining patches update the QAPI
> >> >> generator and wire up -compat policy checking.
> >> >> 
> >> >> Signed-off-by: Markus Armbruster 
> >> >
> >> > Obviously, replacing the old convention gets rid of the old drawbacks,
> >> > but adds a new one: While using x- makes it very obvious for a human
> >> > user that this is an unstable feature, a feature flag in the schema will
> >> > almost certainly go unnoticed in manual use.
> >> 
> >> I thought about this, but neglected to put it in writing.  My bad.
> >> 
> >> Manual use of unstable interfaces is mostly fine.  Human users can adapt
> >> to changing interfaces.  HMP works that way.
> >> 
> >> Management applications are better off with a feature flag than with a
> >> naming convention we sometimes ignore.
> >
> > We will sometimes ignore/forget the feature flag too though, so I'm
> > not convinced there's much difference there.
> 
> -compat unstable-input=reject,unstable-output=hide should help you stay
> on the straight & narrow :)

That's from the pov of the mgmt app. I meant from the POV of QEMU
maintainers forgetting to add "unstable" flag, just as they might
forget to add a "x-" prefix.


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

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

Re: [Libguestfs] [PATCH 1/9] qapi: New special feature flag "unstable"

[Daniel P . Berrangé]

On Tue, Oct 26, 2021 at 11:37:19AM +0200, Markus Armbruster wrote:
> Kevin Wolf  writes:
> 
> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> >> By convention, names starting with "x-" are experimental.  The parts
> >> of external interfaces so named may be withdrawn or changed
> >> incompatibly in future releases.
> >> 
> >> Drawback: promoting something from experimental to stable involves a
> >> name change.  Client code needs to be updated.
> >> 
> >> Moreover, the convention is not universally observed:
> >> 
> >> * QOM type "input-barrier" has properties "x-origin", "y-origin".
> >>   Looks accidental, but it's ABI since 4.2.
> >> 
> >> * QOM types "memory-backend-file", "memory-backend-memfd",
> >>   "memory-backend-ram", and "memory-backend-epc" have a property
> >>   "x-use-canonical-path-for-ramblock-id" that is documented to be
> >>   stable despite its name.
> >> 
> >> We could document these exceptions, but documentation helps only
> >> humans.  We want to recognize "unstable" in code, like "deprecated".
> >> 
> >> Replace the convention by a new special feature flag "unstable".  It
> >> will be recognized by the QAPI generator, like the existing feature
> >> flag "deprecated", and unlike regular feature flags.
> >> 
> >> This commit updates documentation and prepares tests.  The next commit
> >> updates the QAPI schema.  The remaining patches update the QAPI
> >> generator and wire up -compat policy checking.
> >> 
> >> Signed-off-by: Markus Armbruster 
> >
> > Obviously, replacing the old convention gets rid of the old drawbacks,
> > but adds a new one: While using x- makes it very obvious for a human
> > user that this is an unstable feature, a feature flag in the schema will
> > almost certainly go unnoticed in manual use.
> 
> I thought about this, but neglected to put it in writing.  My bad.
> 
> Manual use of unstable interfaces is mostly fine.  Human users can adapt
> to changing interfaces.  HMP works that way.
> 
> Management applications are better off with a feature flag than with a
> naming convention we sometimes ignore.

We will sometimes ignore/forget the feature flag too though, so I'm
not convinced there's much difference there.

> If we want to keep "unstable" obvious to the humans who write such
> programs, we can continue to require "x-", in addition to the feature
> flag.  We pay for it with renames, and the risk of forgetting to rename
> in time (which is what got us the awkward stable
> "x-use-canonical-path-for-ramblock-id").  Tradeoff.  I chose not to, but
> if y'all think we should...

IMHO the renames arguably make life easier for mgmt apps, as they
only need to check for the name without the x- prefix, and they
know they won't be accidentally using the fature from an older
QEMU where it was unstable because the older QEMU had a different
name they won't be checking for.

We can just as easily forget to remove the "unstable" feature
flag, as forget to rename.

The plus point about the feature flag is that it is aligned with
the "deprecated" feature flag, and potentially aligned with a
future "insecure" feature flag.

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

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

Re: [Libguestfs] [PATCH 1/9] qapi: New special feature flag "unstable"

[Daniel P . Berrangé]

On Tue, Oct 26, 2021 at 10:22:15AM +0100, Dr. David Alan Gilbert wrote:
> * Kevin Wolf (kw...@redhat.com) wrote:
> > Am 25.10.2021 um 07:25 hat Markus Armbruster geschrieben:
> > > By convention, names starting with "x-" are experimental.  The parts
> > > of external interfaces so named may be withdrawn or changed
> > > incompatibly in future releases.
> > > 
> > > Drawback: promoting something from experimental to stable involves a
> > > name change.  Client code needs to be updated.
> > > 
> > > Moreover, the convention is not universally observed:
> > > 
> > > * QOM type "input-barrier" has properties "x-origin", "y-origin".
> > >   Looks accidental, but it's ABI since 4.2.
> > > 
> > > * QOM types "memory-backend-file", "memory-backend-memfd",
> > >   "memory-backend-ram", and "memory-backend-epc" have a property
> > >   "x-use-canonical-path-for-ramblock-id" that is documented to be
> > >   stable despite its name.
> > > 
> > > We could document these exceptions, but documentation helps only
> > > humans.  We want to recognize "unstable" in code, like "deprecated".
> > > 
> > > Replace the convention by a new special feature flag "unstable".  It
> > > will be recognized by the QAPI generator, like the existing feature
> > > flag "deprecated", and unlike regular feature flags.
> > > 
> > > This commit updates documentation and prepares tests.  The next commit
> > > updates the QAPI schema.  The remaining patches update the QAPI
> > > generator and wire up -compat policy checking.
> > > 
> > > Signed-off-by: Markus Armbruster 
> > 
> > Obviously, replacing the old convention gets rid of the old drawbacks,
> > but adds a new one: While using x- makes it very obvious for a human
> > user that this is an unstable feature, a feature flag in the schema will
> > almost certainly go unnoticed in manual use.
> 
> Agreed, I'd keep the x- as well.
> 
> Having said that, the x- represents a few different things (that we
> don't currently distinguish):
>   - experimental
>   - for internal use
>   - for debugging/human use

All of those usage scenarios have the same implication though:

   Command/data format is liable to change in incompatible ways,
   or be deleted, with no prior warning.

I don't think we need to distinguish the use cases - some commands
may belong to two or three of those use cases. All that matters is
that they're considered "unstable" from an API compat POV.

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

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

Re: [Libguestfs] Publishing the libnbd go module

[Daniel P . Berrangé]

On Mon, Oct 25, 2021 at 05:22:28PM +0300, Nir Soffer wrote:
> On Mon, Oct 25, 2021 at 5:01 PM Richard W.M. Jones  wrote:
> >
> > [Adding Matthew]
> >
> > On Mon, Oct 25, 2021 at 04:45:03PM +0300, Nir Soffer wrote:
> > > I'm playing with libnbd go module, planning to use it in a new command[1]
> > >
> > > The biggest obstacle for me is that the module is not published in a way 
> > > that
> > > Go developers expect.
> > >
> > > The module is listed in:
> > > https://pkg.go.dev/github.com/libguestfs/libnbd/golang
> > >
> > > But the module actually lives in:
> > > https://github.com/libguestfs/libnbd/tree/master/golang/src/libguestfs.org/libnbd
> > >
> > > So the pkg.go.dev page is broken, .e.g no there is no documation or 
> > > license, and
> > > the suggested import is wrong.
> > >
> > > The module name is "libguestfs.org/libnbd". But if you try to use it,
> > > for example
> > > in the (improved) example from libnbd-golang.pod:
> > >
> > > $ cat test.go
> > > package main
> > >
> > > import "fmt"
> > > import "libguestfs.org/libnbd"
> > >
> > > func main() {
> > > h, err := libnbd.Create()
> > > if err != nil {
> > > panic(err)
> > > }
> > > defer h.Close()
> > > uri := "nbd://localhost"
> > > err = h.ConnectUri(uri)
> > > if err != nil {
> > > panic(err)
> > > }
> > > size, err := h.GetSize()
> > > if err != nil {
> > > panic(err)
> > > }
> > > fmt.Printf("size of %s = %d\n", uri, size)
> > > }
> > >
> > > $ go mod init example/test
> > > go: creating new go.mod: module example/test
> > > go: to add module requirements and sums:
> > > go mod tidy
> > >
> > > $ go mod tidy
> > > go: finding module for package libguestfs.org/libnbd
> > > example/test imports
> > > libguestfs.org/libnbd: cannot find module providing package
> > > libguestfs.org/libnbd: unrecognized import path
> > > "libguestfs.org/libnbd": reading
> > > https://libguestfs.org/libnbd?go-get=1: 404 Not Found
> >
> > That website is entirely static so if it involves fetching stuff from
> > there it's probably not going to work.
> 
> So the import path should be:
> 
> gitlab.com/libnbd/golang/src/libguestfs.org/libnbd
> 
> Since gitlab (or github) already supports go tools.

Depends if you want applications to be susceptible to future changes
in where the code is located. A libguestfs.org/libnbd import would
mean you can move git hosting without impacting apps.

It is fairly simple to configure - you just need the web address

  https://libguestfs.org/libnbd

to serve up an HTML document that contains a magic  tag in
the page header

https://gitlab.com/path/to/real/git/repo/location"/>


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

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

Re: [Libguestfs] Publishing the libnbd go module

[Daniel P . Berrangé]

On Mon, Oct 25, 2021 at 04:45:03PM +0300, Nir Soffer wrote:
> I'm playing with libnbd go module, planning to use it in a new command[1]
> 
> The biggest obstacle for me is that the module is not published in a way that
> Go developers expect.
> 
> The module is listed in:
> https://pkg.go.dev/github.com/libguestfs/libnbd/golang
> 
> But the module actually lives in:
> https://github.com/libguestfs/libnbd/tree/master/golang/src/libguestfs.org/libnbd
> 
> So the pkg.go.dev page is broken, .e.g no there is no documation or license, 
> and
> the suggested import is wrong.
> 
> The module name is "libguestfs.org/libnbd". But if you try to use it,
> for example
> in the (improved) example from libnbd-golang.pod:
> 
> $ cat test.go
> package main
> 
> import "fmt"
> import "libguestfs.org/libnbd"
> 
> func main() {
> h, err := libnbd.Create()
> if err != nil {
> panic(err)
> }
> defer h.Close()
> uri := "nbd://localhost"
> err = h.ConnectUri(uri)
> if err != nil {
> panic(err)
> }
> size, err := h.GetSize()
> if err != nil {
> panic(err)
> }
> fmt.Printf("size of %s = %d\n", uri, size)
> }
> 
> $ go mod init example/test
> go: creating new go.mod: module example/test
> go: to add module requirements and sums:
> go mod tidy
> 
> $ go mod tidy
> go: finding module for package libguestfs.org/libnbd
> example/test imports
> libguestfs.org/libnbd: cannot find module providing package
> libguestfs.org/libnbd: unrecognized import path
> "libguestfs.org/libnbd": reading
> https://libguestfs.org/libnbd?go-get=1: 404 Not Found
> 
> If we use libguestfs.org, https://libguestfs.org/libnbd?go-get=1
> should return the expected
> metadata instead of 404.
> 
> But even if "libguestfs.org/libnbd" would work, we cannot use the
> module from the source
> since the source is missing the generated files (wrappers.go, binding.go, 
> ...).
> 
> It looks like the only ways to use the module are:
> 
> - Vendor the go code from the tarball.
> 
> I did not try this since I don't want to copy libnbd into source into
> my project.
> 
> - Clone and build libnbd locally, and replace libguestfs.org with the
> path to your local source
> 
> $ go mod edit -replace
> libguestfs.org/libnbd=../../src/libnbd/golang/src/libguestfs.org/libnbd
> $ go mod tidy
> go: found libguestfs.org/libnbd in libguestfs.org/libnbd
> v0.0.0-0001010100-
> 
> $ cat go.mod
> module example/test
> 
> go 1.16
> 
> replace libguestfs.org/libnbd =>
> ../../src/libnbd/golang/src/libguestfs.org/libnbd
> 
> require libguestfs.org/libnbd v0.0.0-0001010100-
> 
> 
> But the version is wrong - it should be v1.10.0.
> I think the issue is missing tag:
> https://golang.org/ref/mod#vcs-version
> 
> If a module is defined in a subdirectory within the repository,
> that is, the module subdirectory
> portion of the module path is not empty, then each tag name must
> be prefixed with the module
> subdirectory, followed by a slash. For example, the module
> golang.org/x/tools/gopls is defined
> in the gopls subdirectory of the repository with root path
> golang.org/x/tools. The version v0.4.0
> of that module must have the tag named gopls/v0.4.0 in that repository.
> 
> So the linbd project needs a tag like:
> golang/src/libguestfs.org/libnbd/v1.10.0
> 
> Removing the "src/libguestfs.org" directories will clean things up:
> golang/libnbd/v1.10.0

Go modules are a bit annoying in that they're effectively designed
around the use of semver for versioning. Anything from v2.
onwards means you need to put the module code in sub-directory
named after the major version number. This means all your apps
need to update their imports any time you change major version.

In libvirt's go modules we had to re-create the git repos with
new tags based on semver instead of calver, to get out of the
ever changing sub-directory trap.

libnbd is lucky that it is still on v1., so isn't in the
sub-dir naming trap yet.

I feel like as a general rule life is simpler if the Go code is
all in a dedicated repo, completely separate from any other
non-Go code, and any auto-generated code is committed too, and
wired up such that "go gen" will trigger whatever command is
needed to re-generate.

Assuming you don't want to change libnbd build system at all
though, you could have a separate git repo where you import
the generated Go binding at time of each release. It'll still
get a little odd when people want to submit bugs/patches, as
they will need to know to submit to the main repo, not this
import-only repo.

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

___
Libguestfs mailing list
Libguestfs@redhat.com

Re: [Libguestfs] translating CD-ROM device paths from i440fx to Q35 in virt-v2v (was: test-v2v-cdrom: update the CD-ROM's bus to SATA in the converted domain)

[Daniel P . Berrangé]

On Thu, Sep 30, 2021 at 01:12:39PM +0200, Laszlo Ersek wrote:
> On 09/29/21 21:22, Richard W.M. Jones wrote:
> Please correct me if I'm wrong: at the moment, I believe virt-v2v parses
> and manipulates the following elements and attributes in the domain XML:
> 
>   
>   
>   
>   
>   ^^^   ^^^
> 
> My understanding is however that the target/@dev attribute is mostly
> irrelevant:
> 
> https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms
> 
> The dev attribute indicates the "logical" device name. The actual
> device name specified is not guaranteed to map to the device name in
> the guest OS. Treat it as a device ordering hint. [...]

I won't say it is irrelevant.  Functionally @dev is absolutely still
important, as it influences how the disk is attached to the VM.

Rather I would say that the @dev attribute is misleading to users,
because they mistakenly think it provides a guarantee that the disk
will appear with this name inside the guest.

> What actually matters is the target/@bus attribute, in combination with
> the sibling element . Such as:
> 
>   
>  ^^^
>   
> ^   ^   ^
> 
>   
>  ^^^
>   
> ^   ^   ^
> 
>   
>  ^^^
>   
> ^   ^   ^
> 
>   
>  ^^^
>   
> ^   ^   ^
> 
> So, target/@dev should be mostly ignored; what matters is the following
> tuple:
> 
>   (target/@bus, address/@controller, address/@bus, address/@unit)

Yes, the  is what libvirt internally drivers all configuration
off, but in practice application developers almost never use the 
element directly.  They will just give target/@dev and libvirt will use
that to automatically populate an  element, in order to reliably
fixate the guest ABI thereafter.

> 
> Extracting just the tuples:
> 
>   (ide, 0, 0, 0)
>   (ide, 0, 0, 1)
>   (ide, 0, 1, 0)
>   (ide, 0, 1, 1)
> 
> The first two components of each tuple -- i.e., (ide, 0) -- refer to the
> following IDE controller:
> 
> 
> ^^ ^
>function='0x1'/>
> 
> 
> and then the rest of the components, such as (0, 0), (0, 1), (1, 0), (1,
> 1), identify the disk on that IDE controller.

Yes, that's correct for IDE.

> (Side comment: the PCI location of the (first) IDE controller is fixed
> in QEMU; if one tries to change it, libvirt complains: "Primary IDE
> controller must have PCI address 0:0:1.1".)

Yep, its defined by the QEMU machine type and we just have to accept
that for IDE/SATA.

> Inside the guest, /dev/hd* nodes don't even exist, so it's unlikely that
> /etc/fstab would refer to them. /etc/fstab can however refer to symlinks
> under "/dev/disk/by-id" (for example):
> 
>   lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM1 -> 
> ../../sr0
>   lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM2 -> 
> ../../sr1
>   lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM3 -> 
> ../../sr2
>   lrwxrwxrwx. 1 root root  9 Sep 30 11:54 ata-QEMU_DVD-ROM_QM4 -> 
> ../../sr3
> 
> Furthermore, we have pseudo-files (directories) such as:
> 
>   
> /sys/devices/pci:00/:00:01.1/ata1/host0/target0:0:0/0:0:0:0/block/sr0
>   
> /sys/devices/pci:00/:00:01.1/ata1/host0/target0:0:1/0:0:1:0/block/sr1
>   
> /sys/devices/pci:00/:00:01.1/ata2/host1/target1:0:0/1:0:0:0/block/sr2
>   
> /sys/devices/pci:00/:00:01.1/ata2/host1/target1:0:1/1:0:1:0/block/sr3
>   ^   ^
> 
> So in order to map a device path from the original guest's "/etc/fstab",
> such as "/dev/disk/by-id/ata-QEMU_DVD-ROM_QM3", to the original
> domain XML's  element, we have to do the following in the "source"
> appliance:
> 
> NODE=$(realpath /dev/disk/by-id/ata-QEMU_DVD-ROM_QM3)
> # -> /dev/sr2
> NODE=${NODE#/dev/}
> # -> sr2
> DEVPATH=$(ls -d 
> /sys/devices/pci:00/:00:01.1/ata?/host?/target?:0:?/?:0:?:0/block/$NODE)
> # -> 
> /sys/devices/pci:00/:00:01.1/ata2/host1/target1:0:0/1:0:0:0/block/sr2
> 
> And then map the "1:0:0:0" pathname component from $DEVPATH to:
> 
>   
>   
>^^^
>[1]:0:0:0  1:0:[0]:0
> 
> in the original domain XML. This tells us under what device node the
> original guest sees the host-side file ( element).

Yes, to map from the guest to the libvirt XML, you need to be working
in terms of the hardware buses/addresses.


> So, assuming we mapped the original (i440fx)
> "/dev/disk/by-id/ata-QEMU_DVD-ROM_QM3" guest device path to some
>  element (= host-side file) 

Re: [Libguestfs] [libnbd PATCH] ci: Adopt libvirt-ci's manifest approach

[Daniel P . Berrangé]

On Thu, Sep 23, 2021 at 04:40:59PM +0200, Martin Kletzander wrote:
> This makes it a bit easier to write .gitlab-ci.yml as it inherits most of what
> can be generated from the ci/gitlab.yml and deprecates the ci/refresh script.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  .gitlab-ci.yml| 270 +---
>  ci/README.rst |   7 +-
>  ci/cirrus/freebsd-12.vars |   2 +-
>  ci/cirrus/freebsd-13.vars |   2 +-
>  ci/cirrus/freebsd-current.vars|   2 +-
>  ci/cirrus/macos-11.vars   |   2 +-
>  ci/containers/alpine-314.Dockerfile   |   2 +-
>  ci/containers/alpine-edge.Dockerfile  |   2 +-
>  ci/containers/centos-8.Dockerfile |   2 +-
>  ci/containers/centos-stream-8.Dockerfile  |   2 +-
>  ci/containers/debian-10-cross-i686.Dockerfile |   2 +-
>  ci/containers/debian-10.Dockerfile|   2 +-
>  ci/containers/debian-11-cross-i686.Dockerfile |   2 +-
>  ci/containers/debian-11.Dockerfile|   2 +-
>  ci/containers/debian-sid.Dockerfile   |   2 +-
>  ci/containers/fedora-33.Dockerfile|   2 +-
>  ci/containers/fedora-34-cross-i686.Dockerfile |   0
>  ci/containers/fedora-34.Dockerfile|   2 +-
>  ci/containers/fedora-rawhide.Dockerfile   |   2 +-
>  ci/containers/opensuse-leap-152.Dockerfile|   2 +-
>  ci/containers/opensuse-tumbleweed.Dockerfile  |   2 +-
>  ci/containers/ubuntu-1804.Dockerfile  |   2 +-
>  ci/containers/ubuntu-2004.Dockerfile  |   2 +-
>  ci/gitlab.yml | 413 ++
>  ci/manifest.yml   |  72 +++
>  ci/refresh|  33 --
>  26 files changed, 520 insertions(+), 315 deletions(-)
>  delete mode 100644 ci/containers/fedora-34-cross-i686.Dockerfile
>  create mode 100644 ci/gitlab.yml
>  create mode 100644 ci/manifest.yml
>  delete mode 100755 ci/refresh

> diff --git a/ci/manifest.yml b/ci/manifest.yml
> new file mode 100644
> index ..2439ed01eea2
> --- /dev/null
> +++ b/ci/manifest.yml
> @@ -0,0 +1,72 @@
> +projects:
> +  - libnbd
> +
> +gitlab:
> +  namespace: nbdkit
> +  project: libnbd
> +  jobs:
> +check-dco: false
> +
> +targets:
> +  alpine-314: x86_64
> +
> +  alpine-edge: x86_64
> +
> +  centos-8:
> +jobs:
> +  - arch: x86_64
> +
> +  - arch: x86_64
> +suffix: -clang
> +variables:
> +  CC: clang
> +
> +  centos-stream-8: x86_64
> +
> +  debian-10:
> +jobs:
> +  - arch: x86_64
> +  - arch: i686
> +
> +  debian-11:
> +jobs:
> +  - arch: x86_64
> +  - arch: i686
> +
> +  debian-sid: x86_64
> +
> +  fedora-33: x86_64
> +
> +  fedora-34: x86_64
> +
> +  fedora-rawhide:
> +jobs:
> +  - arch: x86_64
> +
> +  - arch: x86_64
> +suffix: -clang
> +variables:
> +  CC: clang
> +
> +  freebsd-12: x86_64
> +
> +  freebsd-13: x86_64
> +
> +  freebsd-current: x86_64
> +
> +  opensuse-leap-152: x86_64
> +
> +  opensuse-tumbleweed: x86_64
> +
> +  macos-11:
> +jobs:
> +  - arch: x86_64
> +variables:
> +  PATH_EXTRA: 
> /usr/local/opt/ccache/libexec:/usr/local/opt/gettext/bin:/usr/local/opt/libpcap/bin:/usr/local/opt/libxslt/bin:/usr/local/opt/rpcgen/bin
> +  PKG_CONFIG_PATH: 
> /usr/local/opt/curl/lib/pkgconfig:/usr/local/opt/libpcap/lib/pkgconfig:/usr/local/opt/libxml2/lib/pkgconfig:/usr/local/opt/ncurses/lib/pkgconfig:/usr/local/opt/readline/lib/pkgconfig
> +
> +  ubuntu-1804: x86_64
> +
> +  ubuntu-2004:
> +jobs:
> +  - arch: x86_64

Can be simplified like the ubuntu-1804 def

Reviewed-by: Daniel P. Berrangé 



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

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

Re: [Libguestfs] [PATCH nbdinfo v2 1/3] common/utils: Add function to convert sizes to human-readable

[Daniel P . Berrangé]

On Mon, Sep 20, 2021 at 05:39:59PM +0100, Richard W.M. Jones wrote:
> On Mon, Sep 20, 2021 at 06:25:34PM +0200, Laszlo Ersek wrote:
> > On 09/20/21 13:04, Richard W.M. Jones wrote:
> > > For example 1024 is returned as "1K".
> > > 
> > > This does not attempt to handle decimals or SI units.  If the number
> > > isn't some multiple of a power of 1024 then it is returned as bytes (a
> > > flag is available to indicate this).
> > > 
> > > I looked at both the gnulib and qemu versions of this function.  The
> > > gnulib version is not under a license which is compatible with libnbd
> > > and is also really complicated, although it does handle fractions and
> > > SI units.  The qemu version is essentially just frexp + sprintf and
> > > doesn't attempt to convert to the human-readable version reversibly.
> > > ---
> > >  .gitignore |  1 +
> > >  common/utils/Makefile.am   | 10 +++-
> > >  common/utils/human-size.c  | 54 +
> > >  common/utils/human-size.h  | 49 +++
> > >  common/utils/test-human-size.c | 89 ++
> > >  5 files changed, 201 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/.gitignore b/.gitignore
> > > index 2aa1fd99..5fc59677 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -31,6 +31,7 @@ Makefile.in
> > >  /bash/nbdcopy
> > >  /bash/nbdfuse
> > >  /bash/nbdinfo
> > > +/common/utils/test-human-size
> > >  /common/utils/test-vector
> > >  /compile
> > >  /config.cache
> > > diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am
> > > index 1ca4a370..b273ada1 100644
> > > --- a/common/utils/Makefile.am
> > > +++ b/common/utils/Makefile.am
> > > @@ -34,6 +34,8 @@ include $(top_srcdir)/common-rules.mk
> > >  noinst_LTLIBRARIES = libutils.la
> > >  
> > >  libutils_la_SOURCES = \
> > > + human-size.c \
> > > + human-size.h \
> > >   vector.c \
> > >   vector.h \
> > >   version.c \
> > > @@ -50,8 +52,12 @@ libutils_la_LIBADD = \
> > >  
> > >  # Unit tests.
> > >  
> > > -TESTS = test-vector
> > > -check_PROGRAMS = test-vector
> > > +TESTS = test-human-size test-vector
> > > +check_PROGRAMS = test-human-size test-vector
> > > +
> > > +test_human_size_SOURCES = test-human-size.c human-size.c human-size.h
> > > +test_human_size_CPPFLAGS = -I$(srcdir)
> > > +test_human_size_CFLAGS = $(WARNINGS_CFLAGS)
> > >  
> > >  test_vector_SOURCES = test-vector.c vector.c vector.h
> > >  test_vector_CPPFLAGS = -I$(srcdir)
> > > diff --git a/common/utils/human-size.c b/common/utils/human-size.c
> > > new file mode 100644
> > > index ..772f2489
> > > --- /dev/null
> > > +++ b/common/utils/human-size.c
> > > @@ -0,0 +1,54 @@
> > > +/* nbd client library in userspace
> > > + * Copyright (C) 2020-2021 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
> > > + */
> > 
> > (1) General question: should we adopt "SPDX-License-Identifier"s? They
> > are more succinct.
> 
> I'd usually follow what qemu or libvirt are doing, and as far as I can
> see they are not using these.

It is a non-trivial undertaking for any established project with
multiple copyright holders.

The license header is generally not something you are permitted to
change generally unless you are the copyright holder, or have the
copy holders' agreement.  Despite this, we can see the kernel did
such a switch, replacing license headers with SPDX tags. My
understanding though, is that there was work done behind scenes
with legal input before they actually merged the patches, as a
means to justify this change.

On any newly written project I'd certainly use SPDX, but IMHO
for existing projects it isn't viable unless we see clear legal
advice explaining a process to follow that makes it acceptable
to replace license text with SPDX.

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

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

Re: [Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

[Daniel P . Berrangé]

On Mon, Sep 20, 2021 at 05:11:38PM +0200, Laszlo Ersek wrote:
> On 09/20/21 12:37, Daniel P. Berrangé wrote:
> > On Mon, Sep 20, 2021 at 07:23:35AM +0200, Laszlo Ersek wrote:
> >> It turns out that a C-language array containing C-language pointers can
> >> only be allocated in C:
> >>
> >> https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
> >>
> >> Rewrite the "arg_string_list" and "free_string_list" functions almost
> >> entirely in C.
> >>
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>  generator/golang.ml | 46 -
> >>  1 file changed, 37 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/generator/golang.ml b/generator/golang.ml
> >> index d328abe4ed08..5db478e8a494 100644
> >> --- a/generator/golang.ml
> >> +++ b/generator/golang.ml
> >> @@ -52,6 +52,39 @@ _go_guestfs_create_flags (unsigned flags)
> >>  {
> >>  return guestfs_create_flags (flags);
> >>  }
> >> +
> >> +//
> >> +// A C-language array of C-language pointers needs to be allocated in C.
> >> +// 
> >> https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
> >> +//
> >> +typedef char *pChar;
> >> +
> >> +pChar *allocStringArray (size_t nmemb)
> >> +{
> >> +pChar *array;
> >> +
> >> +array = malloc (sizeof (pChar) * (nmemb + 1));
> >> +array[nmemb] = NULL;
> >> +return array;
> >> +}
> >> +
> >> +void storeString (pChar *array, size_t idx, pChar string)
> >> +{
> >> +array[idx] = string;
> >> +}
> >> +
> >> +void freeStringArray (pChar *array)
> >> +{
> >> +pChar *position;
> >> +pChar string;
> >> +
> >> +position = array;
> >> +while ((string = *position) != NULL) {
> >> +  free (string);
> >> +  position++;
> >> +}
> >> +free (array);
> >> +}
> >>  */
> >>  import \"C\"
> >>  
> >> @@ -148,12 +181,11 @@ func (g *Guestfs) Close () *GuestfsError {
> >>   * C strings and golang []string.
> >>   */
> >>  func arg_string_list (xs []string) **C.char {
> >> -r := make ([]*C.char, 1 + len (xs))
> >> +r := C.allocStringArray (C.size_t (len (xs)))
> >>  for i, x := range xs {
> >> -r[i] = C.CString (x)
> >> +C.storeString (r, C.size_t (i), C.CString (x));
> >>  }
> >> -r[len (xs)] = nil
> >> -return [0]
> >> +return (**C.char) (r)
> >>  }
> > 
> > This could be done purely in Go without helper functions
> > 
> >   func array_elem(arr **C.char, idx int) **C.char {
> >  return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
> > (unsafe.Sizeof(arr) * uintptr(idx
> >   }
> > 
> >   func array_size(arr **C.char, len int) C.size_t {
> > return C.size_t(unsafe.Sizeof(*arr) * (1 + uintptr(len)))
> >   }
> > 
> >   func arg_string_list2(xs []string) **C.char {
> > var r **C.char
> > r = (**C.char)(C.malloc(array_size(len(xs
> > for i, x := range xs {
> > str := array_elem(r, i)
> > *str = C.CString(x)
> > }
> > str := array_elem(r, len(xs))
> > *str = nil
> > return r
> >   }
> 
> I'm a total noob in Go and I find the syntax very difficult to read. In
> addition to that, I don't have the slightest idea about the Go runtime.
> (What I do know however is that the #cgo parser is not a full-blown C
> language parser; for example it does not accept "sizeof variable"
> without parens, plus see <https://github.com/golang/go/issues/14210> for
> another example.) The end result is that I wanted to get out of Go-land
> as quickly and as *simply* as possible (meaning the syntax of calling C
> code), and then to do the actual business in C. This is the Go binding
> of a C library, so I feel this approach is justified.

I can understand that POV if you're most comfortable being C developer.
>From my POV maintaining Go bindings though, I think that the desirable
to use Go code to the greatest extent possible. I'd only use C wrapper
functions if it was the only solution - eg where you need 2 consequetive
C API calls in the same thread for T

Re: [Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

[Daniel P . Berrangé]

On Mon, Sep 20, 2021 at 05:23:30PM +0200, Laszlo Ersek wrote:
> On 09/20/21 14:33, Daniel P. Berrangé wrote:
> > On Mon, Sep 20, 2021 at 12:03:51PM +0100, Richard W.M. Jones wrote:
> >> On Mon, Sep 20, 2021 at 11:37:02AM +0100, Daniel P. Berrangé wrote:
> >>> What distro / go version do you see this on, as I can't reproduce
> >>> this pointer problem with a standalone demo app ?
> >>
> >> For me this started to happen after upgrading to
> >> golang-bin-1.17-2.fc36.x86_64 in Rawhide.  It also caused this error:
> > 
> > Hmm, I still cant reproduce the problem that Laszlo is fixing
> > 
> > $ cat str.c
> > 
> > #include 
> > 
> > void foo(char **str) {
> >   for (int i = 0; str[i] != NULL; i++) {
> > fprintf(stderr, "%d: %s (0x%p)\n", i, str[i], str[i]);
> >   }
> > }
> > 
> > $ cat str.go
> > package main
> > 
> > /*
> > #cgo LDFLAGS: -L/home/berrange/t/lib -lstr
> > 
> > #include 
> > 
> > extern void foo(char **str);
> > 
> > */
> > import "C"
> > 
> > import (
> > "fmt"
> > "unsafe"
> > )
> > 
> > func array_elem(arr **C.char, idx int) **C.char {
> > return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
> > (unsafe.Sizeof(arr) * uintptr(idx
> > }
> > 
> > func arg_string_list1(xs []string) **C.char {
> > r := make([]*C.char, 1+len(xs))
> > for i, x := range xs {
> > r[i] = C.CString(x)
> > }
> > r[len(xs)] = nil
> > return [0]
> > }
> > 
> > func arg_string_list2(xs []string) **C.char {
> > var r **C.char
> > r = (**C.char)(C.malloc(C.size_t(unsafe.Sizeof(*r) * (1 + 
> > uintptr(len(xs))
> > for i, x := range xs {
> > str := array_elem(r, i)
> > *str = C.CString(x)
> > }
> > str := array_elem(r, len(xs))
> > *str = nil
> > return r
> > }
> > 
> > func free_string_list(argv **C.char) {
> > for i := 0; ; i++ {
> > str := (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(argv)) +
> > (unsafe.Sizeof(*argv) * uintptr(i
> > if *str == nil {
> > break
> > }
> > fmt.Printf("%x\n", *str)
> > C.free(unsafe.Pointer(*str))
> > }
> > }
> > 
> > func bar(str []string) {
> > cstr1 := arg_string_list1(str)
> > defer free_string_list(cstr1)
> > C.foo(cstr1)
> > cstr2 := arg_string_list2(str)
> > defer free_string_list(cstr2)
> > C.foo(cstr2)
> > }
> > 
> > func main() {
> > bar([]string{"hello", "world"})
> > }
> > 
> > 
> > My interpretation is that arg_string_list1 impl here should have
> > raised the error that Laszlo reports, but both impls work fine
> 
> Can you create a new structure type, make the C function take the structure 
> (or a pointer to the structure), and in the structure, make the field have 
> this type:
> 
>   char * const * str;
> 
> Because this is the scenario where the libguestfs test suite fails (panics). 
> The libguestfs test suite has a *different* case that does match your example 
> directly, and *that* case works in the libguestfs test suite flawlessly. The 
> panic surfaces only in the "char*const* embedded in struct" case. (I assume 
> "const" makes no difference, but who knows!)

Oh, that makes sense, because you have a Go pointer to the storage for
the struct, and then the 'const *const *str' field is initialized with
a Go pointer returned from the arg_string_list().

You're allowed to pass a Go pointer to C via CGo, but the memory that
points to is not allowed to contained further Go pointers. So the struct
fields must strictly use a C pointer.


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

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

Re: [Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

[Daniel P . Berrangé]

On Mon, Sep 20, 2021 at 12:03:51PM +0100, Richard W.M. Jones wrote:
> On Mon, Sep 20, 2021 at 11:37:02AM +0100, Daniel P. Berrangé wrote:
> > What distro / go version do you see this on, as I can't reproduce
> > this pointer problem with a standalone demo app ?
> 
> For me this started to happen after upgrading to
> golang-bin-1.17-2.fc36.x86_64 in Rawhide.  It also caused this error:

Hmm, I still cant reproduce the problem that Laszlo is fixing

$ cat str.c

#include 

void foo(char **str) {
  for (int i = 0; str[i] != NULL; i++) {
fprintf(stderr, "%d: %s (0x%p)\n", i, str[i], str[i]);
  }
}

$ cat str.go
package main

/*
#cgo LDFLAGS: -L/home/berrange/t/lib -lstr

#include 

extern void foo(char **str);

*/
import "C"

import (
"fmt"
"unsafe"
)

func array_elem(arr **C.char, idx int) **C.char {
return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
(unsafe.Sizeof(arr) * uintptr(idx
}

func arg_string_list1(xs []string) **C.char {
r := make([]*C.char, 1+len(xs))
for i, x := range xs {
r[i] = C.CString(x)
}
r[len(xs)] = nil
return [0]
}

func arg_string_list2(xs []string) **C.char {
var r **C.char
r = (**C.char)(C.malloc(C.size_t(unsafe.Sizeof(*r) * (1 + 
uintptr(len(xs))
for i, x := range xs {
str := array_elem(r, i)
*str = C.CString(x)
}
str := array_elem(r, len(xs))
*str = nil
return r
}

func free_string_list(argv **C.char) {
for i := 0; ; i++ {
str := (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(argv)) +
(unsafe.Sizeof(*argv) * uintptr(i
if *str == nil {
break
}
fmt.Printf("%x\n", *str)
C.free(unsafe.Pointer(*str))
}
}

func bar(str []string) {
cstr1 := arg_string_list1(str)
defer free_string_list(cstr1)
C.foo(cstr1)
cstr2 := arg_string_list2(str)
defer free_string_list(cstr2)
C.foo(cstr2)
}

func main() {
bar([]string{"hello", "world"})
}


My interpretation is that arg_string_list1 impl here should have
raised the error that Laszlo reports, but both impls work fine

$ gcc -fPIC -c -o str.o str.c
$ gcc -shared -o libstr.so str.o

$ go version
go version go1.17 linux/amd64
$ go build -o str str.go
$ LD_LIBRARY_PATH=/home/berrange/t/lib  ./str
0: hello (0x0x1d68970)
1: world (0x0x1d68990)
0: hello (0x0x1d689d0)
1: world (0x0x1d689f0)
1d689d0
1d689f0
1d68970
1d68990



Is my test scearnio there representative of what the failing test
case is doing ?  Or is perhaps the C function calling back into
the Go code ?

The reason I'm curious is that the current code for arrays here
matches what libvirt-go-module currently uses in some places, so
I'm wondering if that needs fixing too.


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

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

Re: [Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

[Daniel P . Berrangé]

On Mon, Sep 20, 2021 at 07:23:35AM +0200, Laszlo Ersek wrote:
> The current "arg_string_list" and "free_string_list" implementations go
> back to commit b6f01f32606d ("Add Go (language) bindings.", 2013-07-03).
> There are two problems with them:
> 
> - "free_string_list" doesn't actually free anything,

Doh.

> 
> - at the *first* such g.Internal_test() call site that passes an
>   Ostringlist member inside the Optargs argument, namely:
> 
> > g.Internal_test ("abc",
> >  string_addr ("def"),
> >  []string{},
> >  false,
> >  0,
> >  0,
> >  "123",
> >  "456",
> >  []byte{'a', 'b', 'c', '\000', 'a', 'b', 'c'},
> >  _test{Ostringlist_is_set: true,
> >Ostringlist: []string{}
> >   }
> > )
> 
>   the "golang/run-bindtests" case crashes:
> 
> > panic: runtime error: cgo argument has Go pointer to Go pointer
> >
> > goroutine 1 [running]:
> > libguestfs.org/guestfs.(*Guestfs).Internal_test.func7(0xc18180,
> > 0xadfb60, 0xadfb80, 0xc10048, 0x0, 0x0, 0x0, 0xae3e10, 0xae3e30,
> > 0xade3a0, ...)
> > golang/src/libguestfs.org/guestfs/guestfs.go:6729 +0xa9
> > libguestfs.org/guestfs.(*Guestfs).Internal_test(0xc18180, 0x4ee3a5,
> > 0x3, 0xc61be8, 0xc61af8, 0x0, 0x0, 0xc61a00, 0x0, 0x0, ...)
> > golang/src/libguestfs.org/guestfs/guestfs.go:6729 +0x3c9
> > main.main()
> > golang/bindtests/bindtests.go:77 +0x151e
> > exit status 2
> > FAIL run-bindtests (exit status: 1)


What distro / go version do you see this on, as I can't reproduce
this pointer problem with a standalone demo app ?



> 
> It turns out that a C-language array containing C-language pointers can
> only be allocated in C:
> 
> https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
> 
> Rewrite the "arg_string_list" and "free_string_list" functions almost
> entirely in C.
> 
> Signed-off-by: Laszlo Ersek 
> ---
>  generator/golang.ml | 46 -
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/generator/golang.ml b/generator/golang.ml
> index d328abe4ed08..5db478e8a494 100644
> --- a/generator/golang.ml
> +++ b/generator/golang.ml
> @@ -52,6 +52,39 @@ _go_guestfs_create_flags (unsigned flags)
>  {
>  return guestfs_create_flags (flags);
>  }
> +
> +//
> +// A C-language array of C-language pointers needs to be allocated in C.
> +// 
> https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
> +//
> +typedef char *pChar;
> +
> +pChar *allocStringArray (size_t nmemb)
> +{
> +pChar *array;
> +
> +array = malloc (sizeof (pChar) * (nmemb + 1));
> +array[nmemb] = NULL;
> +return array;
> +}
> +
> +void storeString (pChar *array, size_t idx, pChar string)
> +{
> +array[idx] = string;
> +}
> +
> +void freeStringArray (pChar *array)
> +{
> +pChar *position;
> +pChar string;
> +
> +position = array;
> +while ((string = *position) != NULL) {
> +  free (string);
> +  position++;
> +}
> +free (array);
> +}
>  */
>  import \"C\"
>  
> @@ -148,12 +181,11 @@ func (g *Guestfs) Close () *GuestfsError {
>   * C strings and golang []string.
>   */
>  func arg_string_list (xs []string) **C.char {
> -r := make ([]*C.char, 1 + len (xs))
> +r := C.allocStringArray (C.size_t (len (xs)))
>  for i, x := range xs {
> -r[i] = C.CString (x)
> +C.storeString (r, C.size_t (i), C.CString (x));
>  }
> -r[len (xs)] = nil
> -return [0]
> +return (**C.char) (r)
>  }

This could be done purely in Go without helper functions

  func array_elem(arr **C.char, idx int) **C.char {
 return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
(unsafe.Sizeof(arr) * uintptr(idx
  }

  func array_size(arr **C.char, len int) C.size_t {
return C.size_t(unsafe.Sizeof(*arr) * (1 + uintptr(len)))
  }

  func arg_string_list2(xs []string) **C.char {
var r **C.char
r = (**C.char)(C.malloc(array_size(len(xs
for i, x := range xs {
str := array_elem(r, i)
*str = C.CString(x)
}
str := array_elem(r, len(xs))
*str = nil
return r
  }


>  
>  func count_string_list (argv **C.char) int {
> @@ -167,11 +199,7 @@ func count_string_list (argv **C.char) int {
>  }
>  
>  func free_string_list (argv **C.char) {
> -for *argv != nil {
> -//C.free (*argv)
> -argv = (**C.char) (unsafe.Pointer (uintptr (unsafe.Pointer (argv)) +
> -   unsafe.Sizeof (*argv)))
> -}
> +C.freeStringArray ((*C.pChar) (argv))
>  }

This can be done entirely in Go

 func 

Re: [Libguestfs] [PATCH] lib: direct: Remove use of sga

[Daniel P . Berrangé]

On Thu, Sep 09, 2021 at 12:35:12PM +0100, Richard W.M. Jones wrote:
> On Thu, Sep 09, 2021 at 01:11:09PM +0200, Laszlo Ersek wrote:
> > How does libguestfs deal with different QEMU versions / QEMU feature
> > deprecation? Is there some kind of feature detection?
> 
> Kind of, but it's only grepping the output of -help, -device \?,
> and/or looking at QMP output of "query-qmp-schema" and "query-kvm".
> 
> The code is in:
> 
>   https://github.com/libguestfs/libguestfs/blob/master/lib/qemu.c
> 
> As qemu has settled down over time and libguestfs uses only a
> well-established set of features, this kind of feature detection has
> become less and less interesting.  (So quite unlike libvirt where
> they're always trying to catch up with the latest qemu features).  At
> some point we might just say "use qemu >= VERSION" and have done with it.
> 
> > For example, why would it be less good to implement the change as follows:
> > 
> >   if (g->verbose) {
> > if (guestfs_int_qemu_supports_device(...) {
> >   arg ("-device", "sga");
> > } else {
> > #if defined(__i386__) || defined(__x86_64__)
> >   arg ("-machine", "graphics=off");
> > #endif
> > }
> 
> I think in this case it's my understanding that the "new" (from RHEL 7!)
> seabios graphics feature is just better than SGA, and so we might as
> well use it all the time even if SGA is available.  That was my
> understanding from talking to Gerd anyhow.

IIUC, functionally they should be no difference.

The "better" comes from the seabios impl being written in C
while the sgabios impl is 2500 lines of asm :-)


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

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

Re: [Libguestfs] [PATCH] lib: direct: Remove use of sga

[Daniel P . Berrangé]

On Wed, Sep 08, 2021 at 04:42:05PM +0100, Richard W.M. Jones wrote:
> sga (or "sgabios" or "Serial Graphics Adapter") is an option ROM for
> seabios which directs output to the serial adapter.  This is very
> useful for debugging BIOS problems during boot.
> 
> RHEL wants to deprecate this feature (in fact, they just deprecated it
> without telling us).  However there is an equivalent feature now in
> seabios which can be enabled using either -nographic or
> -machine graphics=off

FWIW, this is available since SeaBIOS 1.11, bundled with
QEMU 2.11.0 which is the minimum libvirt supports these
days. I presume libguestfs min version exceeds that ?

> This commit removes sga and enables -machine graphics=off in the
> direct backend.
> 
> (We cannot do the same for the libvirt backend because libvirt has no
> feature to implement this yet).

You likely won't need any changes to libvirt side. Once I fully confirm
that it has no impact on live migration, we'll transparentely update
libvirt to use the new syntax.


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

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

Re: [Libguestfs] Fwd: libnbd | Failed pipeline for master | 2e381ac2

[Daniel P . Berrangé]

On Fri, Aug 27, 2021 at 11:09:13AM +0100, Richard W.M. Jones wrote:
> >From the log:
> https://gitlab.com/nbdkit/libnbd/-/jobs/1540375264
> 
> opensuse zypper seems to be failing with a recoverable error:
> 
> Resolving package dependencies...
> Problem: the to be installed glib2-devel-2.68.3-4.1.x86_64 requires 
> 'libglib-2_0-0 = 2.68.3', but this requirement cannot be provided
>   not installable providers: libglib-2_0-0-2.68.3-4.1.i586[repo-oss]
>libglib-2_0-0-2.68.3-4.1.x86_64[repo-oss]
>  Solution 1: downgrade of libglib-2_0-0-2.68.4-1.1.x86_64 to 
> libglib-2_0-0-2.68.3-4.1.x86_64
>  Solution 2: do not install glib2-devel-2.68.3-4.1.x86_64
>  Solution 3: break glib2-devel-2.68.3-4.1.x86_64 by ignoring some of its 
> dependencies
> Choose from above solutions by number or cancel [1/2/3/c/d/?] (c): c
> 
> I wonder if this is something that libvirt-ci should handle better?
> 
> I found this thread which seems to indicate there's no way to force
> zypper to pick an option:
> https://www.linuxquestions.org/questions/suse-opensuse-60/how-to-pre-select-solutions-1-or-2-in-autoyast-xml-config-file-4175688006/
> which is kind of annoying.  But maybe --force-resolution is worth a try?

I've not tried that option, but I will say that openSuse Tumbleweed
seems to hit these broken dependancy problems fairly often.  I'm
increasingly inclined to say it is too unstable to be worth having
in CI, unless it is marked "allow_failure: true" so it doesn't
block the pipeline


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

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

Re: [Libguestfs] [PATCH] docs: Link to protocol security considerations in uri docs

[Daniel P . Berrangé]

On Wed, Aug 18, 2021 at 11:02:48AM -0500, Eric Blake wrote:
> On Mon, Aug 16, 2021 at 05:25:02PM +0200, Wouter Verhelst wrote:
> > > As a followup, I got this reply from Hanno Böck on oss-security:
> > > 
> > > https://www.openwall.com/lists/oss-security/2021/08/11/8
> > > | The buffering vulnerabilities we found are in STARTTLS implementations
> > > | that have the expectation to enforce a secure connection, but suffer
> > > | from various vulnerabilities in the implementation.
> > > 
> > > One of the reasons that SMTP and IMAP were particularly problematic in
> > > implementations is that they are line-based protocols, with
> > > arbitrary-sized packets where the length isn't known until the \n is
> > > reached.  Both clients and servers have to choose whether to read one
> > > character at a time (painfully slow) or read ahead into a buffer and
> > > then processing what is in the buffer.  Many of the CVEs raised were
> > > in regards to mishandling such buffers, such as acting on
> > > previously-buffered plaintext even after the switch to encryption.
> > > 
> > > Thankfully, the NBD protocol has a much more structured handshake
> > > (while different NBD_OPT commands can have different payload lenghts,
> > > at least the header of each packet designates the length in advance,
> > > reducing the need for read-ahead buffering), as well as documentation
> > > that the NBD_OPT_ phase is a lockstep algorithm (neither client nor
> > > server should be building up a buffer of more than one
> > > command/response).
> > > 
> > > Another aspect of the SMTP/IMAP security holes came from incorrectly
> > > carrying state across the transition to encryption (making a false
> > > assumption that the answer made in plaintext will be the same when
> > > encrypted).  Unfortunately, I have realized that the NBD spec has one
> > > bit of state (NBD_OPT_SET_META_CONTEXT) where it is currently silent
> > > on how to handle that state across a transition to encrypted.  So I
> > > plan on posting a followup patch that matches the actual
> > > implementation of nbdkit in opportunistic mode (qemu-nbd does not
> > > offer opportunistic mode, and nbd-server does not suport
> > > NBD_OPT_SET_META_CONTEXT): a server in opportunistic mode MUST treat
> > > the NBD_OPT_STARTTLS command as wiping out any earlier
> > > NBD_OPT_SET_META_CONTEXT.
> > 
> > This all makes sense, thanks.
> 
> Dan Berrangé and I thought about some more potential future problems:
> right now, even with FORCEDTLS mode (in both client and server), we
> have NO way to validate that the initial NBD_FLAG_[C_] bits advertised
> between client and server were not tampered with by a MitM during the
> plaintext portion of the exchange.  The flags field is 16 bits sent
> from the server, and 16 bits mirrored back by the client, to determine
> which protocol features will be in use the remainder of the
> connection.  Right now, exactly two of those bits are defined:
> 
> NBD_FLAG_FIXED_NEWSTYLE - the spec is clear that NBD_OPT_STARTTLS
> should not be sent unless this bit was negotiated.  Thus, both client
> and server will be sending the bit set, and absence of the bit will be
> evidence of a MitM attempting a downgrade attack, and there's nothing
> further to worry about in the protocol.  Once STARTTLS is executed, we
> already know this capability was available, so we don't need a way to
> re-verify it while encrypted.
> 
> NBD_FLAG_NO_ZEROES - this bit controls how the server will respond to
> NBD_OPT_EXPORT_NAME.  A mismatch between this bit (whether the MitM
> strips or adds the bit) will only be observable if the client uses
> NBD_OPT_EXPORT_NAME, but all clients that use STARTTLS are already
> encouraged to use NBD_OPT_GO.  We may want to tighten the security
> portion of the spec to make this recommendation even stronger (that
> is, any client that wants to ensure there is no MitM downgrade attack
> MUST use NBD_OPT_GO rather than NBD_OPT_EXPORT_NAME; and all servers
> that support TLS MUST support NBD_OPT_GO), but once a client uses the
> modern export selection code, we no longer care about mismatches in
> this bit, and therefore we don't need a way to re-verify it while
> encrypted.
> 
> However, I'm also worried about future extensions.  For example, we
> have been considering the addition of a way for clients to make 64-bit
> requests (basically, allowing NBD_OPT_WRITE_ZEROES to become a
> single-transaction bulk-zero for an export larger than 4G).  If the
> way this extension is haggled between client and server utilizes only
> a new NBD_FLAG_*, then we have introduced a potential security hole,
> because we are back in the situation of having a MitM flip bits prior
> to STARTTLS so that client and server do not agree on which protocol
> changes to use.  We can avoid this by adding a way to validate which
> capability bits are actually common between client and server via a
> new NBD_OPT_ sent after STARTTLS.  But rather than needing a way to
> re-verify 

Re: [Libguestfs] [PATCH] point users to Libera Chat rather than FreeNode

[Daniel P . Berrangé]

On Thu, May 27, 2021 at 01:00:55PM +0100, Richard W.M. Jones wrote:
> On Thu, May 27, 2021 at 09:52:58AM +0100, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  docs/guestfs-faq.pod  | 2 +-
> >  website/index.html.in | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/docs/guestfs-faq.pod b/docs/guestfs-faq.pod
> > index bea609591..15b1247b0 100644
> > --- a/docs/guestfs-faq.pod
> > +++ b/docs/guestfs-faq.pod
> > @@ -108,7 +108,7 @@ There is a mailing list, mainly for development, but 
> > users are also
> >  welcome to ask questions about libguestfs and the virt tools:
> >  L<https://www.redhat.com/mailman/listinfo/libguestfs>
> >  
> > -You can also talk to us on IRC channel C<#libguestfs> on FreeNode.
> > +You can also talk to us on IRC channel C<#guestfs> on Libera Chat.
> >  We're not always around, so please stay in the channel after asking
> >  your question and someone will get back to you.
> >  
> > diff --git a/website/index.html.in b/website/index.html.in
> > index f469c5eeb..7453129d6 100644
> > --- a/website/index.html.in
> > +++ b/website/index.html.in
> > @@ -55,8 +55,8 @@ guestfish --ro -i -a disk.img
> >  
> >  Join us on
> >  the http://www.redhat.com/mailman/listinfo/libguestfs;>libguestfs
> > -mailing list, or on IRC channel #libguestfs
> > -on http://freenode.net/;>FreeNode.
> > +mailing list, or on IRC channel #guestfs
> > +on https://libera.chat/;>Libera Chat.
> >  
> 
> Thanks Dan, I've pushed it.

This doesn't seem to have gone live on the actual website.

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

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

Re: [Libguestfs] [libnbd PATCH] nbdsh: Simplify the script

[Daniel P . Berrangé]

On Tue, Jun 22, 2021 at 10:29:14PM +0300, Nir Soffer wrote:
> On Tue, Jun 22, 2021 at 9:20 PM Martin Kletzander  wrote:
> >
> > There is no need for any hacks if we just do what execution of the module 
> > would
> > have done.
> >
> > Signed-off-by: Martin Kletzander 
> > ---
> >  sh/nbdsh.in | 12 
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/sh/nbdsh.in b/sh/nbdsh.in
> > index d10f0c1b6b26..f66e2918d304 100644
> > --- a/sh/nbdsh.in
> > +++ b/sh/nbdsh.in
> > @@ -1,4 +1,4 @@
> > -#!/bin/sh -
> > +#!/usr/bin/env @PYTHON@
> 
> Do we really care about specific @PYTHON@?
> 
> If not we can use:
> 
> #!/usr/bin/python3

That is not reliably portable, especially to non-Linux platforms
like *BSD as it isn't always in /usr/bin


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

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

Re: [Libguestfs] [libnbd PATCH] ci: Enable python code style

[Daniel P . Berrangé]

On Wed, Jun 23, 2021 at 12:31:14PM +0100, Richard W.M. Jones wrote:
> On Tue, Jun 22, 2021 at 03:38:13PM +0200, Martin Kletzander wrote:
> > Signed-off-by: Martin Kletzander 
> > ---
> >  ci/build.sh | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/ci/build.sh b/ci/build.sh
> > index 7d62a84a5d4b..4ea3fec7d512 100755
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -8,6 +8,7 @@ main() {
> >  autoreconf -if
> >  
> >  CONFIG_ARGS="\
> > +--enable-python-code-style \
> >  --enable-gcc-warnings \
> >  --enable-fuse \
> >  --enable-ocaml \
> 
> It's OK as long as "someone" is going to chase up and fix the new
> failures whenever flake8 changes, or we could do something like what
> Nir suggests and pin a version of flake8.

This isn't all that much different from GCC where new releases
inevitably trigger new compiler warnings.

As long as you have a nightly CI job that is running Fedora
rawhide, you'll detect the problems quicky and be able to
fix or ignorelist them before it has a negative impact.

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

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

[Libguestfs] [PATCH nbdkit] plugins/ssh: remove pointless code fetching SHA1 fingerprint

[Daniel P . Berrangé]

The result of calling ssh_get_publickey_hash() is never used in the
code, simply being freed on all exit paths. It appears this was
copied from the libssh docs example code, where the fingerprint
was indeed printed on the console.

The ssh_session_is_known_server() call will validate against any
fingerprint stored in the $HOME/.ssh/known_hosts file. The hashes
in this file will use the algorithm configured for the openssh
client, which will usually be SHA256 in modern OS.

Signed-off-by: Daniel P. Berrangé 
---
 plugins/ssh/ssh.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
index 994e9834..6d18f18d 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -228,14 +228,6 @@ do_verify_remote_host (struct ssh_handle *h)
 nbdkit_error ("could not get server public key");
 return -1;
   }
-  rc = ssh_get_publickey_hash (srv_pubkey,
-   SSH_PUBLICKEY_HASH_SHA1,
-   , );
-  ssh_key_free (srv_pubkey);
-  if (rc < 0) {
-nbdkit_error ("could not get server public key SHA1 hash");
-return -1;
-  }
 
   state = ssh_session_is_known_server (h->session);
   switch (state) {
@@ -245,13 +237,11 @@ do_verify_remote_host (struct ssh_handle *h)
 
   case SSH_KNOWN_HOSTS_CHANGED:
 nbdkit_error ("host key for server changed");
-ssh_clean_pubkey_hash ();
 return -1;
 
   case SSH_KNOWN_HOSTS_OTHER:
 nbdkit_error ("host key for server was not found "
   "but another type of key exists");
-ssh_clean_pubkey_hash ();
 return -1;
 
   case SSH_KNOWN_HOSTS_NOT_FOUND:
@@ -259,22 +249,18 @@ do_verify_remote_host (struct ssh_handle *h)
  * host key is set up before using nbdkit so we error out here.
  */
 nbdkit_error ("could not find known_hosts file");
-ssh_clean_pubkey_hash ();
 return -1;
 
   case SSH_KNOWN_HOSTS_UNKNOWN:
 nbdkit_error ("host key is unknown, you must use ssh first "
   "and accept the host key");
-ssh_clean_pubkey_hash ();
 return -1;
 
   case SSH_KNOWN_HOSTS_ERROR:
 nbdkit_error ("known hosts error: %s", ssh_get_error (h->session));
-ssh_clean_pubkey_hash ();
 return -1;
   }
 
-  ssh_clean_pubkey_hash ();
   return 0;
 }
 
-- 
2.31.1

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

Re: [Libguestfs] [PATCH nbdkit] Set visibility to default for exported symbols

[Daniel P . Berrangé]

On Wed, Jun 02, 2021 at 09:05:26PM +0100, Richard W.M. Jones wrote:
> In plugins we expect the plugin_init function to be exported as well
> as *_debug_* variables.  In the server various nbdkit_* functions
> should be exported.
> 
> If you compile nbdkit with -fvisibility=hidden some of these symbols
> are not exported at all even though the linker script says they should
> be.  This option can be useful as it allows the compiler to generate
> simpler and hence faster code, and it's required for Clang CFI.
> 
> Set the visibility to "default" for all symbols we expect to be
> exported.  In theory this should also be required for all the nbdkit_*
> functions exported by the server, but I found that it is not needed by
> either GCC or Clang.

This is kinda wierd, as it feels like we're duplicating information
that is already available in the linker script, but I guess the clue
is in the name of the latter. The visibility option is processed by
the compiler, while the linker script is processed by the linker
and thus too late for the compiler to use.

It is annoying to have to maintain both a .syms script and the source
level annotations. Perhaps there is value in a script that looks for
the NBDKIT_DLL_PUBLIC annotations and uses them to auto-generated a
.syms file for the linker ?

> ---
>  include/nbdkit-common.h  |  6 +-
>  include/nbdkit-filter.h  |  1 +
>  include/nbdkit-plugin.h  |  1 +
>  tests/Makefile.am|  1 +
>  plugins/ocaml/bindings.c | 26 +-
>  plugins/ocaml/plugin.c   | 14 +++---
>  tests/dummy-vddk.c   | 30 --
>  7 files changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
> index 51a6264c..f43ebe59 100644
> --- a/include/nbdkit-common.h
> +++ b/include/nbdkit-common.h
> @@ -83,10 +83,14 @@ extern "C" {
>  #define NBDKIT_EXTENT_ZERO(1<<1) /* Same as NBD_STATE_ZERO */
>  
>  #ifndef WIN32
> -#define NBDKIT_EXTERN_DECL(ret, fn, args) extern ret fn args
> +#define NBDKIT_EXTERN_DECL(ret, fn, args)   \
> +  __attribute__((__visibility__("default")))\
> +  extern ret fn args
> +#define NBDKIT_DLL_PUBLIC __attribute__((__visibility__("default")))
>  #else
>  #define NBDKIT_EXTERN_DECL(ret, fn, args) \
>extern __declspec(dllexport) ret fn args
> +#define NBDKIT_DLL_PUBLIC __declspec(dllimport)
>  #endif
>  
>  NBDKIT_EXTERN_DECL (void, nbdkit_error,
> diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
> index 633d34e7..8b587d70 100644
> --- a/include/nbdkit-filter.h
> +++ b/include/nbdkit-filter.h
> @@ -263,6 +263,7 @@ struct nbdkit_filter {
>  
>  #define NBDKIT_REGISTER_FILTER(filter)  \
>NBDKIT_CXX_LANG_C \
> +  NBDKIT_DLL_PUBLIC \
>struct nbdkit_filter *\
>filter_init (void)\
>{ \
> diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
> index 611987c7..59ec11b6 100644
> --- a/include/nbdkit-plugin.h
> +++ b/include/nbdkit-plugin.h
> @@ -154,6 +154,7 @@ NBDKIT_EXTERN_DECL (int, nbdkit_is_tls, (void));
>  
>  #define NBDKIT_REGISTER_PLUGIN(plugin)  \
>NBDKIT_CXX_LANG_C \
> +  NBDKIT_DLL_PUBLIC \
>struct nbdkit_plugin *\
>plugin_init (void)\
>{ \
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index ae801290..acb74fa8 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -1027,6 +1027,7 @@ libvixDiskLib_la_SOURCES = \
>   $(NULL)
>  libvixDiskLib_la_CPPFLAGS = \
>   -I$(top_srcdir)/plugins/vddk \
> + -I$(top_srcdir)/include \
>   $(NULL)
>  libvixDiskLib_la_CXXFLAGS = $(WARNINGS_CFLAGS)
>  # For use of the -rpath option, see:
> diff --git a/plugins/ocaml/bindings.c b/plugins/ocaml/bindings.c
> index 493b147b..a6d57084 100644
> --- a/plugins/ocaml/bindings.c
> +++ b/plugins/ocaml/bindings.c
> @@ -51,7 +51,7 @@
>  /* Bindings for miscellaneous nbdkit_* utility functions. */
>  
>  /* NB: noalloc function. */
> -value
> +NBDKIT_DLL_PUBLIC value
>  ocaml_nbdkit_set_error (value nv)
>  {
>int err;
> @@ -77,7 +77,7 @@ ocaml_nbdkit_set_error (value nv)
>return Val_unit;
>  }
>  
> -value
> +NBDKIT_DLL_PUBLIC value
>  ocaml_nbdkit_parse_size (value strv)
>  {
>CAMLparam1 (strv);
> @@ -92,7 +92,7 @@ ocaml_nbdkit_parse_size (value strv)
>CAMLreturn (rv);
>  }
>  
> -value
> +NBDKIT_DLL_PUBLIC value
>  ocaml_nbdkit_parse_bool (value strv)
>  {
>CAMLparam1 

[Libguestfs] [PATCH] point users to Libera Chat rather than FreeNode

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 docs/guestfs-faq.pod  | 2 +-
 website/index.html.in | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/guestfs-faq.pod b/docs/guestfs-faq.pod
index bea609591..15b1247b0 100644
--- a/docs/guestfs-faq.pod
+++ b/docs/guestfs-faq.pod
@@ -108,7 +108,7 @@ There is a mailing list, mainly for development, but users 
are also
 welcome to ask questions about libguestfs and the virt tools:
 L<https://www.redhat.com/mailman/listinfo/libguestfs>
 
-You can also talk to us on IRC channel C<#libguestfs> on FreeNode.
+You can also talk to us on IRC channel C<#guestfs> on Libera Chat.
 We're not always around, so please stay in the channel after asking
 your question and someone will get back to you.
 
diff --git a/website/index.html.in b/website/index.html.in
index f469c5eeb..7453129d6 100644
--- a/website/index.html.in
+++ b/website/index.html.in
@@ -55,8 +55,8 @@ guestfish --ro -i -a disk.img
 
 Join us on
 the http://www.redhat.com/mailman/listinfo/libguestfs;>libguestfs
-mailing list, or on IRC channel #libguestfs
-on http://freenode.net/;>FreeNode.
+mailing list, or on IRC channel #guestfs
+on https://libera.chat/;>Libera Chat.
 
 
 
-- 
2.31.1

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