[PATCH] docs, passt: Clarify some niche passt usage

[Martin Kletzander]

Change example logfile path and clarify how complicated all things passt
are.  I chose not to create the non-existing directory because it could
open a whole new can of worms.

Also explain missing `dev` attribute of ``

Resolves: https://issues.redhat.com/browse/RHEL-1833

Signed-off-by: Martin Kletzander 
---
 docs/formatdomain.rst | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 68f54ab3ed62..bc469e5f9f94 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -4902,14 +4902,23 @@ When the passt backend is used, the  
attribute
 ``logFile`` can be used to tell the passt process for this interface
 where to write its message log, and the  attribute ``dev``
 can tell it to use a particular host interface to derive the routes
-given to the guest for forwarding traffic upstream.
+given to the guest for forwarding traffic upstream.  Due to the design
+decisions of passt, if using SELinux, the log file is recommended to
+reside in the runtime directory of a user under which the passt
+process will run, most probably ``/run/user/$UID`` where ``$UID`` is
+the UID of the user, e.g. ``qemu``.  Beware that libvirt does not
+create this directory if it does not already exist to avoid possible,
+however unlikely, issues, especially since this logfile attribute is
+meant mostly for debugging.
 
 Additionally, when passt is used, multiple  elements
 can be added to forward incoming network traffic for the host to this
 guest interface. Each  must have a ``proto``
-attribute (set to ``tcp`` or ``udp``) and optional original
-``address`` (if not specified, then all incoming sessions to any host
-IP for the given proto/port(s) will be forwarded to the guest).
+attribute (set to ``tcp`` or ``udp``), optional original ``address``
+(if not specified, then all incoming sessions to any host IP for the
+given proto/port(s) will be forwarded to the guest), and an optional
+``dev`` attribute to limit the forwarded traffic to a specific host
+interface.
 
 The decision of which ports to forward is described with zero or more
  subelements of  (if there is no
@@ -4934,7 +4943,7 @@ ports **with the exception of some subset**.

  ...
  
-   
+   



@@ -4946,7 +4955,7 @@ ports **with the exception of some subset**.
  
  

-   
+   
  
  

-- 
2.41.0

[PATCH] conf, schema: Switch iothread/poll values to unsignedLong

[Martin Kletzander]

They represent nanoseconds, and we accept such values already.  Not that
anyone would use such values in the wild, but even one person testing
QEMU could put in a bigger value and will be bothered with validation
errors after every `virsh edit`.  Also add a test for it.

Resolves: https://issues.redhat.com/browse/RHEL-1717

Signed-off-by: Martin Kletzander 
---
 src/conf/schemas/domaincommon.rng  |  6 +++---
 tests/genericxml2xmlindata/iothreadids.xml | 23 ++
 tests/genericxml2xmltest.c |  2 ++
 3 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/iothreadids.xml

diff --git a/src/conf/schemas/domaincommon.rng 
b/src/conf/schemas/domaincommon.rng
index de3bd1c35c55..2f9ba31c0aec 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -876,17 +876,17 @@
 
   
 
-  
+  
 
   
   
 
-  
+  
 
   
   
 
-  
+  
 
   
 
diff --git a/tests/genericxml2xmlindata/iothreadids.xml 
b/tests/genericxml2xmlindata/iothreadids.xml
new file mode 100644
index ..671a4672958d
--- /dev/null
+++ b/tests/genericxml2xmlindata/iothreadids.xml
@@ -0,0 +1,23 @@
+
+  foo
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  1
+  
+
+  
+
+  
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+  
+
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 3501eadf5597..ce8073e85a30 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -249,6 +249,8 @@ mymain(void)
 DO_TEST("cpu-phys-bits-emulate");
 DO_TEST("cpu-phys-bits-passthrough");
 
+DO_TEST("iothreadids");
+
 virObjectUnref(caps);
 virObjectUnref(xmlopt);
 
-- 
2.41.0

Re: [libvirt PATCH 5/5] qemu: Implement support for vDPA block devices

[Jonathon Jongsma]

On 8/16/23 4:19 PM, Jonathon Jongsma wrote:

On 8/8/23 6:00 AM, Stefano Garzarella wrote:

On Mon, Aug 07, 2023 at 03:41:21PM +0200, Peter Krempa wrote:

On Thu, Aug 03, 2023 at 09:48:01 +0200, Stefano Garzarella wrote:
On Wed, Aug 2, 2023 at 10:33 PM Jonathon Jongsma 
 wrote:

> On 7/24/23 8:05 AM, Peter Krempa wrote:

[...]

> >
> > I've also noticed that using 'qcow2' format for the device 
doesn't work:

> >
> > error: internal error: process exited while connecting to 
monitor: 2023-07-24T12:54:15.818631Z qemu-system-x86_64: -blockdev 
{"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}: Could not read qcow2 header: Invalid argument

> >
> > If that is supposed to work, then qemu devs will probably need 
to know

> > about that, if that is not supposed to work, libvirt needs to add a
> > check, because the error doesn't tell much. It's also possible I've
> > messed up when formatting the image though, as didn't really try to
> > figure out what's happening.
> >
>
>
> That's a good question, and I don't actually know the answer. Were 
you
> using an actual vdpa block device for your tests or were you using 
the

> vdpa block simulator kernel module? How did you set it up? Adding
> Stefano to cc for his thoughts.

Yep, I would also like to understand how you initialized the device
with a qcow2 format.


Naively and originally I've simply used it as 'raw' at first and
formatted it from the guest OS. Then I've shut-down the VM and started
it back reconfiguring the image format as qcow2. This normally works
with real-file backed storage, and since the vdpa simulator seems to
persist the contents I supposed this would work.


Cool, I'll try that.
Can you try to reboot the VM, use it as `raw`, and read the qcow2 in the
vm from the guest OS?

Note: there could be some bugs in the simulator!




Theoretically, the best use case for vDPA block is that the backend
handles formats, for QEMU it should just be a virtio device, but being
a blockdev, we should be able to use formats anyway, so it should
work.


Yeah, ideally there will be no format driver in qemu used for these
devices (this is not yet the case, I'll need to fix libvirt to stop
using the 'raw' driver if not needed).

Here I'm more interested whether it is supposed to work, in which case
we want to allow using qcow2 as a format in libvirt, or it's not
supposed to work and we should forbid it before the user gets a
suboptimal error message such as now.


This is a good question. We certainly haven't tested it, because it's an
uncommon scenario, but as I said before, maybe it should work. I need to
check it better.





For now, waiting for real hardware, the only way to test vDPA block
support in QEMU is to use the simulator in the kernel or VDUSE.

With the kernel simulator we only have a 128 MB ramdisk available,
with VDUSE you can use QSD with any file:

$ modprobe -a vhost_vdpa vduse
$ qemu-storage-daemon \
    --blockdev 
file,filename=/path/to/image.qcow2,cache.direct=on,aio=native,node-name=file

\
    --blockdev qcow2,file=file,node-name=qcow2 \
    --export 
vduse-blk,id=vduse0,name=vduse0,num-queues=1,node-name=qcow2,writable=on


$ vdpa dev add name vduse0 mgmtdev vduse

Then you have a /dev/vhost-vdpa-X device that you can use with the
`virtio-blk-vhost-vdpa` blockdev (note: vduse requires QEMU with a
memory-backed with `share=on`), but using raw since the qcow2 is
handled by QSD.
Of course, we should be able to use raw file with QSD and qcow2 on
qemu (although it's not the optimal configuration), but I don't know
how to initialize a `virtio-blk-vhost-vdpa` blockdev with a qcow2
image :-(


With the above qemu storage daemon you should be able to do that by
simply dropping the qcow2 format driver and simply exposing a qcow2
formatted image. It similarly works with NBD:

I've formatted 2 qcow2 images:

# qemu-img create -f qcow2 /root/image1.qcow2 100M
# qemu-img create -f qcow2 /root/image2.qcow2 100M

And then exported them both via vduse and nbd without interpreting
qcow2, thus making the QSD into just a dumb storage device:

# qemu-storage-daemon \
  --blockdev 
file,filename=/root/image1.qcow2,cache.direct=on,aio=native,node-name=file1 \
  --export 
vduse-blk,id=vduse0,name=vduse0,num-queues=1,node-name=file1,writable=on \
  --blockdev 
file,filename=/root/image2.qcow2,cache.direct=on,aio=native,node-name=file2 \

  --nbd-server addr.type=unix,addr.path=/tmp/nbd.sock \
  --export nbd,id=nbd0,node-name=file2,writable=on,name=exportname


Cool! Thanks for sharing!



Now when I start a VM using the NBD export in qcow2 format:

   
 
 
   
 
 
 function='0x0'/>

   

The VM starts fine, but when using:

   
 
 
 
 function='0x0'/>

   

I get:

error: internal error: QEMU unexpectedly closed the monitor 
(vm='vdpa'): 2023-08-07T12:34:21.628520Z qemu-system-x86_64: 
-blockdev 

Re: [libvirt PATCH 01/33] ci: build.sh: Add variables from .gitlab-ci.yml

[Erik Skultety]

On Thu, Aug 31, 2023 at 05:45:05PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:09PM +0200, Erik Skultety wrote:
> > These are common variables we wish to use in containerized environments
> > both in GitLab and locally. Having these defined in a single place
> > rather than twice is highly preferable.
> > 
> > Signed-off-by: Erik Skultety 
> > ---
> >  ci/build.sh | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > diff --git a/ci/build.sh b/ci/build.sh
> > index ed9b1f4473..d5586f457c 100644
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -2,7 +2,12 @@
> >  
> >  cd "$CI_CONT_SRCDIR"
> >  
> > -export VIR_TEST_DEBUG=1
> > +export CCACHE_BASEDIR="$(pwd)"
> > +export CCACHE_DIR="$CCACHE_BASEDIR/ccache"
> > +export CCACHE_MAXSIZE="500M"
> > +export PATH="$CCACHE_WRAPPERSDIR:$PATH"
> > +export VIR_TEST_VERBOSE="1"
> > +export VIR_TEST_DEBUG="1"
> 
> These last two vars are good for CI, because in a non-interactive env we
> need the verbose / debug output upfront to stand a chance of diagnosing
> problems from logs after the fact
> 
> If I'm running a build locally with our ci/Makefile, I don't especially
> want it to be forced into verbose mode by default, as I have an interactive
> shell I can do that as & when needed myself.
> 
> Perhaps we can do
> 
>if ! test -t 1
>then
>   export VIR_TEST_VERBOSE="1"
>   export VIR_TEST_DEBUG="1"
>fi

This uncovered an interesting bug in the ci/helper + lcitool combo.
In lcitool we explicitly always ask for a TTY with --tty on podman/docker CLI.
If I drop the argument and interrupt the execution I'll get an unhandled
OSError exception from lcitool and a stacktrace from ci/helper which is nicely
interlaced with meson's own stacktrace if you interrupt the setup phase. I need
to look into lcitool whether there's an easy fix. The other temporary solution
until we incorporate the ci/helper logic into lcitool would be to set another
variable inside the temporary crafted script by ci/helper that would serve the
same purpose.

Erik

[PATCH] conf: Generate MAC address instead of keeping all zeroes

[Martin Kletzander]

When we parse  we keep that in memory
and pass it down to the hypervisor.  However, that MAC address is not
strictly valid as it is not marked as locally administered (bit 0x02)
but it is not even globally unique.  It is also used for loopback device
on Linux, for example.  And QEMU sees such MAC address just as "not
specified" and generates a new one that libvirt does not even know
about.  So to make the overall experience better we now generate it if
the supplied one is all clear.

Resolves: https://issues.redhat.com/browse/RHEL-974

Signed-off-by: Martin Kletzander 
---
 src/conf/domain_conf.c|  2 +-
 src/util/virmacaddr.c |  5 +
 src/util/virmacaddr.h |  1 +
 .../network-interface-mac-clear.xml   | 21 +++
 .../network-interface-mac-clear.xml   | 21 +++
 tests/genericxml2xmltest.c|  4 +++-
 6 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
 create mode 100644 tests/genericxml2xmloutdata/network-interface-mac-clear.xml

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bb4f1fdb948d..652bd09b21b8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
 return NULL;
 }
 
-if (!macaddr) {
+if (!macaddr || virMacAddrIsAllClear(>mac)) {
 virDomainNetGenerateMAC(xmlopt, >mac);
 def->mac_generated = true;
 }
diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
index 073f298b5b66..e06bb200fc68 100644
--- a/src/util/virmacaddr.c
+++ b/src/util/virmacaddr.c
@@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char 
s[VIR_MAC_BUFLEN])
 return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
 }
 
+bool virMacAddrIsAllClear(const virMacAddr *addr)
+{
+return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
+}
+
 void
 virMacAddrFree(virMacAddr *addr)
 {
diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
index f32b58805a61..7b9eb7443bd1 100644
--- a/src/util/virmacaddr.h
+++ b/src/util/virmacaddr.h
@@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
 bool virMacAddrIsUnicast(const virMacAddr *addr);
 bool virMacAddrIsMulticast(const virMacAddr *addr);
 bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
+bool virMacAddrIsAllClear(const virMacAddr *addr);
 void virMacAddrFree(virMacAddr *addr);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);
diff --git a/tests/genericxml2xmlindata/network-interface-mac-clear.xml 
b/tests/genericxml2xmlindata/network-interface-mac-clear.xml
new file mode 100644
index ..41beda8a79bb
--- /dev/null
+++ b/tests/genericxml2xmlindata/network-interface-mac-clear.xml
@@ -0,0 +1,21 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+
+  
+  
+
+  
+
diff --git a/tests/genericxml2xmloutdata/network-interface-mac-clear.xml 
b/tests/genericxml2xmloutdata/network-interface-mac-clear.xml
new file mode 100644
index ..a7935fa9f4de
--- /dev/null
+++ b/tests/genericxml2xmloutdata/network-interface-mac-clear.xml
@@ -0,0 +1,21 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+
+  
+  
+
+  
+
diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
index 3501eadf5597..bf160b7e0bef 100644
--- a/tests/genericxml2xmltest.c
+++ b/tests/genericxml2xmltest.c
@@ -187,6 +187,7 @@ mymain(void)
 DO_TEST("cpu-cache-disable");
 
 DO_TEST("network-interface-mac-check");
+DO_TEST_DIFFERENT("network-interface-mac-clear");
 
 DO_TEST_DIFFERENT("chardev-tcp");
 DO_TEST_FAIL_ACTIVE("chardev-tcp-missing-host");
@@ -255,4 +256,5 @@ mymain(void)
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
-VIR_TEST_MAIN(mymain)
+VIR_TEST_MAIN_PRELOAD(mymain,
+  VIR_TEST_MOCK("virrandom"))
-- 
2.42.0

[PATCH] tests: Remove unused symlink

[Martin Kletzander]

The test does not use VIR_TEST_DIFFERENT anyway, so it's probably a
leftover.

Signed-off-by: Martin Kletzander 
---
Pushed as trivial

 tests/genericxml2xmloutdata/device-backenddomain.xml | 1 -
 1 file changed, 1 deletion(-)
 delete mode 12 tests/genericxml2xmloutdata/device-backenddomain.xml

diff --git a/tests/genericxml2xmloutdata/device-backenddomain.xml 
b/tests/genericxml2xmloutdata/device-backenddomain.xml
deleted file mode 12
index f19471e3b50c..
--- a/tests/genericxml2xmloutdata/device-backenddomain.xml
+++ /dev/null
@@ -1 +0,0 @@
-../genericxml2xmlindata/device-backenddomain.xml
\ No newline at end of file
-- 
2.42.0

Re: [PATCH v2] NEWS: Announcing Network Metadata APIs

[Ján Tomko]

On a Wednesday in 2023, K Shiva Kiran wrote:

Ref to patchset implementing the above:
https://listman.redhat.com/archives/libvir-list/2023-August/241250.html

Signed-off-by: K Shiva Kiran 
---
This is a v2 of:
https://listman.redhat.com/archives/libvir-list/2023-August/241469.html
Diff to v1:
- Shortened the text and put all text under one section.

NEWS.rst | 11 +++
1 file changed, 11 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: Release of libvirt-9.7.0

[K Shiva Kiran]

This NEWS entry announcing the new metadata APIs for network object in 
version 9.7.0 seems to have been missed for the release.


Link to patch:
https://listman.redhat.com/archives/libvir-list/2023-August/241592.html

- Shiva

Re: [libvirt PATCH v2 1/6] qemu_snapshot: fix reverting external snapshot when not all disks are included

[Pavel Hrdina]

On Fri, Sep 01, 2023 at 11:10:43AM +0200, Peter Krempa wrote:
> On Fri, Sep 01, 2023 at 10:32:12 +0200, Pavel Hrdina wrote:
> > We need to skip all disks that have snapshot type other than 'external'.
> 
> Since the commit message is vague on the specific problem details ...
> 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_snapshot.c | 12 
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> > index d943281e35..ff85d56572 100644
> > --- a/src/qemu/qemu_snapshot.c
> > +++ b/src/qemu/qemu_snapshot.c
> > @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
> >  virDomainSnapshotDiskDef *snapdisk = >disks[i];
> >  virDomainDiskDef *domdisk = domdef->disks[i];
> >  
> > +if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> > +continue;
> 
> ... I remember you talking about the need to create overlays also for
> images which might have been excluded in given snapshot (the one you are
> reverting to) , but externally snapshotted in a later (still existing)
> one.
> 
> In such case not creating the overlay here would still invalidate the
> overlay of the next snapshot. Is that right?
> 
> I also remember that you mentioned that the actual problem is with empty
> cdroms, thus, did you rather want to use 'virStorageSourceIsEmpty' here?

That is done in virDomainSnapshotAlignDisks() called right before the
for loop in qemuSnapshotRevertExternalPrepare(), there we fill in the
temporary snapshot definition with new overlays. From that moment the
temporary snapshot definition contains all disks based on the VM
definition.

Looking at the code, when reverting to a snapshot we will always create
new overlays for all disks including readonly disks, not sure if that is
what we should do or no.

Pavel


signature.asc
Description: PGP signature

Release of libvirt-9.7.0

[Jiri Denemark]

The 9.7.0 release of both libvirt and libvirt-python is tagged and
signed tarballs and source RPMs are available at

https://download.libvirt.org/
https://download.libvirt.org/python/

Thanks everybody who helped with this release by sending patches,
reviewing, testing, or providing feedback. Your work is greatly
appreciated.

* New features

  * qemu: basic support for use of "VFIO variant" drivers

A VFIO variant driver is a device-specific driver that can
be used in place of the generic vfio-pci driver, and provides
extra functionality to support things like live migration of
guests with vfio-assigned devices. It can currently be used by:

1) setting ``managed='no'`` in the XML configuration for the device
2) pre-binding the variant driver using the ``--driver`` option of
   ``virsh nodedev-detach``.

* Bug fixes

  * qemu: Various fixes to firmware selection

The changes made to firmware selection in libvirt 9.2.0 have unfortunately
introduced a number of regressions. All known issues in this area have now
been resolved.

Enjoy.

Jirka

Re: [libvirt PATCH] docs: remove archived projects from CI dashboard

[Peter Krempa]

On Fri, Sep 01, 2023 at 11:12:40 +0100, Daniel P. Berrangé wrote:
> When the projects were archived, their pipelines were all purged, and
> no new ones will be run. There is no point in continuing to display
> them on the CI dashboard.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/ci-dashboard.rst | 35 ---
>  1 file changed, 35 deletions(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 05/33] ci: build.sh: Add a wrapper function executing 'shell' commands

[Erik Skultety]

On Fri, Sep 01, 2023 at 10:46:06AM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 31, 2023 at 03:30:23PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
> > > This would normally be not needed at all, but the problem here is the
> > > Shell-in-YAML which GitLab interprets. It outputs every command that
> > > appears as a line in the 'script' segment in a color-coded fashion for
> > > easy identification of problems. Well, that useful feature is lost when
> > > there's indirection and one script calls into another in which case it
> > > would only output the respective script name which would make failure
> > > investigation harder. This simple helper tackles that by echoing the
> > > command to be run by any script/function with a color escape sequence
> > > so that we don't lose track of the *actual* shell commands being run as
> > > part of the GitLab job pipelines. An example of what the output then
> > > might look like:
> > > [RUN COMMAND]: 'meson compile -C build install-web'
> > 
> > FWIW, I happened to come across this
> > 
> >   https://docs.gitlab.com/ee/ci/jobs/index.html#custom-collapsible-sections
> > 
> > which lets scripts output messages in a special format that get
> > turned into collapsible sections when browsing logs in gitlab UI.
> > Not sure it is something we'd use in this specific patch, but
> > might be conceptually useful somewhere.
> 
> This actually works kinda nicely. I added this commit on top of
> yours:
> 
>   
> https://gitlab.com/berrange/libvirt/-/commit/29f123024c265262e70a409daa00c8634652a1b6
> 
> and an example pipeline is:
> 
>   https://gitlab.com/berrange/libvirt/-/jobs/4997176074
> 
> Notice there are three collapsible sections in the job logs
> 
>   "Running 'meson setup'"
>   "Running 'meson build'"
>   "Running 'meson test'"
> 
> collapsing earlier sections makes it somewhat easier to find the
> phase you're interested in, though its doesn't work so well when
> gitlab truncates the earlier part of the log that's over 500k.
> Not a problem for pipelines on master, but bad for forks.

Not bad. We could improve on it and go as far as print the actual command in
the section header and fallback to what I proposed in local environments.

Let's finish this series first and then you can refine and post your patch as a
follow up improvement.

Erik

[libvirt PATCH] docs: remove archived projects from CI dashboard

[Daniel P . Berrangé]

When the projects were archived, their pipelines were all purged, and
no new ones will be run. There is no point in continuing to display
them on the CI dashboard.

Signed-off-by: Daniel P. Berrangé 
---
 docs/ci-dashboard.rst | 35 ---
 1 file changed, 35 deletions(-)

diff --git a/docs/ci-dashboard.rst b/docs/ci-dashboard.rst
index a7f4e71f96..50f39cd252 100644
--- a/docs/ci-dashboard.rst
+++ b/docs/ci-dashboard.rst
@@ -87,11 +87,6 @@ Object mappings
* - Project
  - Pipeline
 
-   * - libvirt-cim
- - .. image:: 
https://gitlab.com/libvirt/libvirt-cim/badges/master/pipeline.svg
-  :target: https://gitlab.com/libvirt/libvirt-cim/pipelines
-  :alt: libvirt-cim pipeline status
-
* - libvirt-dbus
  - .. image:: 
https://gitlab.com/libvirt/libvirt-dbus/badges/master/pipeline.svg
   :target: https://gitlab.com/libvirt/libvirt-dbus/pipelines
@@ -107,11 +102,6 @@ Object mappings
   :target: https://gitlab.com/libvirt/libvirt-go-xml-module/pipelines
   :alt: libvirt-go-xml-module pipeline status
 
-   * - libvirt-snmp
- - .. image:: 
https://gitlab.com/libvirt/libvirt-snmp/badges/master/pipeline.svg
-  :target: https://gitlab.com/libvirt/libvirt-snmp/pipelines
-  :alt: libvirt-snmp pipeline status
-
 
 Testing
 ---
@@ -149,16 +139,6 @@ Documentation / websites
* - Project
  - Pipeline
 
-   * - libvirt-publican
- - .. image:: 
https://gitlab.com/libvirt/libvirt-publican/badges/master/pipeline.svg
-  :target: https://gitlab.com/libvirt/libvirt-publican/pipelines
-  :alt: libvirt-publican pipeline status
-
-   * - libvirt-appdev-guide-python
- - .. image:: 
https://gitlab.com/libvirt/libvirt-appdev-guide-python/badges/master/pipeline.svg
-  :target: 
https://gitlab.com/libvirt/libvirt-appdev-guide-python/pipelines
-  :alt: libvirt-appdev-guide-python pipeline status
-
* - libvirt-wiki
  - .. image:: 
https://gitlab.com/libvirt/libvirt-wiki/badges/master/pipeline.svg
   :target: https://gitlab.com/libvirt/libvirt-wiki/pipelines
@@ -190,26 +170,11 @@ Miscellaneous
   :target: https://gitlab.com/libvirt/libvirt-console-proxy/pipelines
   :alt: libvirt-console-proxy pipeline status
 
-   * - libvirt-designer
- - .. image:: 
https://gitlab.com/libvirt/libvirt-designer/badges/master/pipeline.svg
-  :target: https://gitlab.com/libvirt/libvirt-designer/pipelines
-  :alt: libvirt-designer pipeline status
-
* - libvirt-devaddr
  - .. image:: 
https://gitlab.com/libvirt/libvirt-devaddr/badges/master/pipeline.svg
   :target: https://gitlab.com/libvirt/libvirt-devaddr/pipelines
   :alt: libvirt-devaddr pipeline status
 
-   * - libvirt-sandbox
- - .. image:: 
https://gitlab.com/libvirt/libvirt-sandbox/badges/master/pipeline.svg
-  :target: https://gitlab.com/libvirt/libvirt-sandbox/pipelines
-  :alt: libvirt-sandbox pipeline status
-
-   * - libvirt-sandbox-image
- - .. image:: 
https://gitlab.com/libvirt/libvirt-sandbox-image/badges/master/pipeline.svg
-  :target: https://gitlab.com/libvirt/libvirt-sandbox-image/pipelines
-  :alt: libvirt-sandbox-image pipeline status
-
* - libvirt-security-notice
  - .. image:: 
https://gitlab.com/libvirt/libvirt-security-notice/badges/master/pipeline.svg
   :target: https://gitlab.com/libvirt/libvirt-security-notice/pipelines
-- 
2.41.0

Re: Sunset libvirt-snmp?

[Erik Skultety]

On Fri, Sep 01, 2023 at 10:41:26AM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 01, 2023 at 11:39:34AM +0200, Erik Skultety wrote:
> > On Fri, Sep 01, 2023 at 02:15:59AM -0700, Andrea Bolognani wrote:
> > > On Thu, Aug 31, 2023 at 04:53:47PM +0100, Daniel P. Berrangé wrote:
> > > > I'm also strongly inclined to archive the following:
> > > >
> > > >   * libvirt-cim
> > > >   * libvirt-appdev-guide-python
> > > >   * libvirt-publican
> > > >   * libvirt-sandbox
> > > >   * libvirt-sandbox-image
> > > >   * libvirt-designer
> > > 
> > > Sounds good to me. It will save us some effort if we don't need to
> > > keep these repositories up to our standards (mainly in terms of CI)
> > > while they're seeing zero actual development.
> > > 
> > 
> > Fine by me. If you haven't done so yet, I'll unleash
> > https://gitlab.com/eskultety/gitlab_cleaner to clean the pipelines.
> 
> Ok, if you run the pipeline cleaner on those 6 repos, I'll purge the
> container images, then we toggle to archived.

Looks like we worked in parallel :), all of the above are now archived.

Erik

Re: [PATCH] virsh: Fix net-desc --config output

[Jiri Denemark]

On Thu, Aug 31, 2023 at 23:46:57 +0530, K Shiva Kiran wrote:
> Fixes the following bug:
> Command:  `net-desc --config [--title] my_network`
> Expected Output:  Title/Description of persistent config
> Output:   Title/Description of live config
> 
> This was caused due to the usage of a single `flags` variable in
> `virshGetNetworkDescription()` which ended up in a wrong enum being
> passed to `virNetworkGetMetadata()` (enum being that of LIVE instead of
> CONFIG).
> 
> Although the domain object has the same code, this didn't cause a problem
> there because the enum values of `VIR_DOMAIN_INACTIVE_XML` and
> `VIR_DOMAIN_METADATA_CONFIG` turn out to be the same (1 << 1), whereas
> they are not for network equivalent ones (1 << 0, 1 << 1).
> 
> Signed-off-by: K Shiva Kiran 
> ---
>  tools/virsh-network.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
...

Reviewed-by: Jiri Denemark 

Re: [libvirt PATCH 05/33] ci: build.sh: Add a wrapper function executing 'shell' commands

[Daniel P . Berrangé]

On Thu, Aug 31, 2023 at 03:30:23PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
> > This would normally be not needed at all, but the problem here is the
> > Shell-in-YAML which GitLab interprets. It outputs every command that
> > appears as a line in the 'script' segment in a color-coded fashion for
> > easy identification of problems. Well, that useful feature is lost when
> > there's indirection and one script calls into another in which case it
> > would only output the respective script name which would make failure
> > investigation harder. This simple helper tackles that by echoing the
> > command to be run by any script/function with a color escape sequence
> > so that we don't lose track of the *actual* shell commands being run as
> > part of the GitLab job pipelines. An example of what the output then
> > might look like:
> > [RUN COMMAND]: 'meson compile -C build install-web'
> 
> FWIW, I happened to come across this
> 
>   https://docs.gitlab.com/ee/ci/jobs/index.html#custom-collapsible-sections
> 
> which lets scripts output messages in a special format that get
> turned into collapsible sections when browsing logs in gitlab UI.
> Not sure it is something we'd use in this specific patch, but
> might be conceptually useful somewhere.

This actually works kinda nicely. I added this commit on top of
yours:

  
https://gitlab.com/berrange/libvirt/-/commit/29f123024c265262e70a409daa00c8634652a1b6

and an example pipeline is:

  https://gitlab.com/berrange/libvirt/-/jobs/4997176074

Notice there are three collapsible sections in the job logs

  "Running 'meson setup'"
  "Running 'meson build'"
  "Running 'meson test'"

collapsing earlier sections makes it somewhat easier to find the
phase you're interested in, though its doesn't work so well when
gitlab truncates the earlier part of the log that's over 500k.
Not a problem for pipelines on master, but bad for forks.


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

Re: [libvirt PATCH v2 6/6] Revert "capabilities: report full external snapshot support"

[Jiri Denemark]

On Fri, Sep 01, 2023 at 11:24:28 +0200, Peter Krempa wrote:
> On Fri, Sep 01, 2023 at 10:32:17 +0200, Pavel Hrdina wrote:
> > Reverting external snapshot for running VM doesn't work correctly so we
> > should not report this capability until it is fixed.
> > 
> > This reverts commit de71573bfec7f3acd22ec74794318de121716e21.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> 
> If Jirka decides to go with this alternative and cut the release before
> the fixes:
> 
> Reviewed-by: Peter Krempa 

Yeah, I think this is a better approach for this release. The
functionality can be properly fixed after the release.

Jirka

Re: Sunset libvirt-snmp?

[Daniel P . Berrangé]

On Fri, Sep 01, 2023 at 11:39:34AM +0200, Erik Skultety wrote:
> On Fri, Sep 01, 2023 at 02:15:59AM -0700, Andrea Bolognani wrote:
> > On Thu, Aug 31, 2023 at 04:53:47PM +0100, Daniel P. Berrangé wrote:
> > > I'm also strongly inclined to archive the following:
> > >
> > >   * libvirt-cim
> > >   * libvirt-appdev-guide-python
> > >   * libvirt-publican
> > >   * libvirt-sandbox
> > >   * libvirt-sandbox-image
> > >   * libvirt-designer
> > 
> > Sounds good to me. It will save us some effort if we don't need to
> > keep these repositories up to our standards (mainly in terms of CI)
> > while they're seeing zero actual development.
> > 
> 
> Fine by me. If you haven't done so yet, I'll unleash
> https://gitlab.com/eskultety/gitlab_cleaner to clean the pipelines.

Ok, if you run the pipeline cleaner on those 6 repos, I'll purge the
container images, then we toggle to archived.


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

Re: [libvirt PATCH v2 4/6] qemu_snapshot: correctly load the saved memory state file

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:15 +0200, Pavel Hrdina wrote:
> Original code assumed that the memory state file is only migration
> stream but it has additional metadata stored by libvirt. To correctly
> load the memory state file we need to reuse code that is used when
> restoring domain from saved image.
> 
> This duplicates some necessary parts of qemuSaveImageStartVM() because
> the external snapshot memory state is done by qemuSaveImageCreate().
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_snapshot.c | 115 +--
>  1 file changed, 97 insertions(+), 18 deletions(-)

[...]

>  /* No cookie means libvirt which saved the domain was too old to
>   * mess up the CPU definitions.
>   */
> @@ -2307,17 +2355,48 @@ qemuSnapshotRevertActive(virDomainObj *vm,
>  
>  rc = qemuProcessStart(snapshot->domain->conn, driver, vm,
>cookie ? cookie->cpu : NULL,

As noted in review of v1, we ought to use the cookie from the save image
or at least validate that this one is correct.

Re: [libvirt PATCH 6/7] qemu_snapshot: correctly load the saved memory state file

[Peter Krempa]

On Thu, Aug 31, 2023 at 16:55:05 +0200, Pavel Hrdina wrote:
> Original code assumed that the memory state file is only migration
> stream but it has additional metadata stored by libvirt. To correctly
> load the memory state file we need to reuse code that is used when
> restoring domain from saved image.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_snapshot.c | 78 
>  1 file changed, 47 insertions(+), 31 deletions(-)

This one is indeed cleaner. Note my comments about the save image cookie
though.

Reviewed-by: Peter Krempa 

Re: Sunset libvirt-snmp?

[Erik Skultety]

On Fri, Sep 01, 2023 at 02:15:59AM -0700, Andrea Bolognani wrote:
> On Thu, Aug 31, 2023 at 04:53:47PM +0100, Daniel P. Berrangé wrote:
> > I'm also strongly inclined to archive the following:
> >
> >   * libvirt-cim
> >   * libvirt-appdev-guide-python
> >   * libvirt-publican
> >   * libvirt-sandbox
> >   * libvirt-sandbox-image
> >   * libvirt-designer
> 
> Sounds good to me. It will save us some effort if we don't need to
> keep these repositories up to our standards (mainly in terms of CI)
> while they're seeing zero actual development.
> 

Fine by me. If you haven't done so yet, I'll unleash
https://gitlab.com/eskultety/gitlab_cleaner to clean the pipelines.

Erik

Re: [libvirt PATCH 4/7] qemuSaveImageStartProcess: make it possible to use without header

[Peter Krempa]

On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
> When used with internal snapshots there is no header to be used and no
> memory state to be decompressed.

This ...

> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_saveimage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 73115af42d..2538732901 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -573,7 +573,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>   * @fd: FD pointer of memory state file
>   * @path: path to memory state file
>   * @snapshot: snapshot to load when starting QEMU process or NULL
> - * @header: header from memory state file
> + * @header: header from memory state file or NULL
>   * @cookie: cookie from memory state file
>   * @asyncJob: type of asynchronous job
>   * @start_flags: flags to start QEMU process with

... here.

> @@ -605,7 +605,7 @@ qemuSaveImageStartProcess(virConnectPtr conn,
>  g_autofree char *errbuf = NULL;
>  int rc = 0;
>  
> -if ((header->version == 2) &&
> +if (header && (header->version == 2) &&
>  (header->compressed != QEMU_SAVE_FORMAT_RAW)) {
>  if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed)))
>  return -1;

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 3/7] qemuSaveImageStartProcess: add snapshot argument

[Peter Krempa]

On Thu, Aug 31, 2023 at 16:55:02 +0200, Pavel Hrdina wrote:
> When called from snapshot code we will need to pass snapshot object in
> order to make internal snapshots work correctly.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_saveimage.c | 6 --
>  src/qemu/qemu_saveimage.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)

> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 1eedc900b9..73115af42d 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -572,6 +572,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>   * @vm: domain object
>   * @fd: FD pointer of memory state file
>   * @path: path to memory state file
> + * @snapshot: snapshot to load when starting QEMU process or NULL

At this point the function comment must include what happens if you
provide it. And more specifically that user must not do that if they are
reloading an image.

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 2/7] qemuSaveImageStartProcess: allow setting reason for audit log

[Peter Krempa]

On Thu, Aug 31, 2023 at 16:55:01 +0200, Pavel Hrdina wrote:
> When called by snapshot code we will need to use different reason.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_saveimage.c | 6 --
>  src/qemu/qemu_saveimage.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 86f31d1820..1eedc900b9 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -576,6 +576,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>   * @cookie: cookie from memory state file
>   * @asyncJob: type of asynchronous job
>   * @start_flags: flags to start QEMU process with
> + * @reason: audit log reason

Put expected values into the comment.

>   * @started: boolean to store if QEMU process was started
>   *
>   * Start VM with existing memory state. Make sure that the stored memory 
> state


Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 1/7] qemu_saveimage: extract starting process to qemuSaveImageStartProcess

[Peter Krempa]

On Thu, Aug 31, 2023 at 16:55:00 +0200, Pavel Hrdina wrote:
> Part of qemuSaveImageStartVM() function will be used when reverting
> external snapshots. To avoid duplicating code and logic extract the
> shared bits into separate function.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_saveimage.c | 103 ++
>  src/qemu/qemu_saveimage.h |  12 +
>  2 files changed, 83 insertions(+), 32 deletions(-)
> 
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 41310d6a9a..86f31d1820 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -565,42 +565,46 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>  return ret;
>  }
>  
> +/**
> + * qemuSaveImageStartProcess:
> + * @conn: connection object
> + * @driver: qemu driver object
> + * @vm: domain object
> + * @fd: FD pointer of memory state file
> + * @path: path to memory state file
> + * @header: header from memory state file
> + * @cookie: cookie from memory state file
> + * @asyncJob: type of asynchronous job
> + * @start_flags: flags to start QEMU process with
> + * @started: boolean to store if QEMU process was started
> + *
> + * Start VM with existing memory state. Make sure that the stored memory 
> state
> + * is correctly decompressed so it can be loaded by QEMU process.

In subsequent patches which add the ability to conditionally load the
save image or not load it and load an internal snapshot, you never
update this comment so it will not be known to the reader that the
function gained additional modes.

Please address that in the individual commits or separately at the end.

I don't mind if you also mention it here right away before subsequent
patches.


> + *
> + * Returns 0 on success, -1 on error.
> + */
>  int
> -qemuSaveImageStartVM(virConnectPtr conn,
> - virQEMUDriver *driver,
> - virDomainObj *vm,
> - int *fd,
> - virQEMUSaveData *data,
> - const char *path,
> - bool start_paused,
> - bool reset_nvram,
> - virDomainAsyncJob asyncJob)
> +qemuSaveImageStartProcess(virConnectPtr conn,
> +  virQEMUDriver *driver,
> +  virDomainObj *vm,
> +  int *fd,
> +  const char *path,
> +  virQEMUSaveHeader *header,
> +  qemuDomainSaveCookie *cookie,
> +  virDomainAsyncJob asyncJob,
> +  unsigned int start_flags,
> +  bool *started)
>  {
>  qemuDomainObjPrivate *priv = vm->privateData;
> -int ret = -1;
> -bool started = false;
> -virObjectEvent *event;
>  VIR_AUTOCLOSE intermediatefd = -1;
>  g_autoptr(virCommand) cmd = NULL;
>  g_autofree char *errbuf = NULL;
> -g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> -virQEMUSaveHeader *header = >header;
> -g_autoptr(qemuDomainSaveCookie) cookie = NULL;
>  int rc = 0;
> -unsigned int start_flags = VIR_QEMU_PROCESS_START_PAUSED |
> -VIR_QEMU_PROCESS_START_GEN_VMID;
> -
> -if (reset_nvram)
> -start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM;
> -
> -if (virSaveCookieParseString(data->cookie, (virObject **),
> - 
> virDomainXMLOptionGetSaveCookie(driver->xmlopt)) < 0)
> -goto cleanup;
>  
>  if ((header->version == 2) &&
>  (header->compressed != QEMU_SAVE_FORMAT_RAW)) {
>  if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed)))
> -goto cleanup;
> +return -1;
>  
>  intermediatefd = *fd;
>  *fd = -1;
> @@ -613,7 +617,7 @@ qemuSaveImageStartVM(virConnectPtr conn,
>  if (virCommandRunAsync(cmd, NULL) < 0) {
>  *fd = intermediatefd;
>  intermediatefd = -1;
> -goto cleanup;
> +return -1;
>  }
>  }
>  
> @@ -622,7 +626,7 @@ qemuSaveImageStartVM(virConnectPtr conn,
>   */
>  if (cookie &&
>  qemuDomainFixupCPUs(vm, >cpu) < 0)
> -goto cleanup;
> +return -1;
>  
>  if (cookie && !cookie->slirpHelper)
>  priv->disableSlirp = true;
> @@ -631,12 +635,12 @@ qemuSaveImageStartVM(virConnectPtr conn,
>   asyncJob, "stdio", *fd, path, NULL,
>   VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
>   start_flags) == 0)
> -started = true;
> +*started = true;
>  
>  if (intermediatefd != -1) {
>  virErrorPtr orig_err = NULL;
>  
> -if (!started) {
> +if (!*started) {
>  /* if there was an error setting up qemu, the intermediate
>   * process will wait forever to write to stdout, so we
>   * must manually kill it and ignore any 

Re: [libvirt PATCH 00/33] ci: Unify the GitLab CI jobs with local executions && adopt lcitool container executions

[Daniel P . Berrangé]

On Fri, Sep 01, 2023 at 11:21:04AM +0200, Erik Skultety wrote:
> On Thu, Aug 31, 2023 at 05:17:13PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
> > > Technically a v2 of:
> > > https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
> > > 
> > > However, the approach here is slightly different and what that series said
> > > about migration to lcitool container executions as a replacement for
> > > ci/Makefile is actually done here. One of the core problems of the above
> > > pointed out in review was that more Shell logic was introduced including 
> > > CLI
> > > parsing, conditional executions, etc. which we fought hard to get rid of 
> > > in the
> > > past. I reworked the Shell functions quite a bit and dropped whatever 
> > > extra
> > > Shell logic the original series added.
> > > Obviously we can't get rid of Shell completely because of .gitlab-ci.yml 
> > > and so
> > > I merely extracted the recipes into functions which are then sourced as
> > > ci/build.sh and executed. Now, that on its own would hide the actual 
> > > commands
> > > being run in the GitLab job log, so before any command is actually 
> > > executed, it
> > > is formatted with a color sequence so we don't miss that information as 
> > > that
> > > would be a regression to the status quo.
> > > 
> > > Lastly, this series then takes the effort inside the ci/build.sh script 
> > > and
> > > basically mirrors whatever GitLab would do to run a job inside a local
> > > container which is executed by lcitool (yes, we already have that 
> > > capability).
> > > 
> > > Please give this a try and I'm already looking forward to comments as I'd 
> > > like
> > > to expand this effort to local VM executions running the TCK integration 
> > > tests,
> > > so this series is quite important in that regard.
> > 
> > Do you have a gitlab branch with this contnt somewhere. When i tried to
> > apply the patches to current git, it was unhappy on the 3rd patch
> > 
> > $ git am -3 ~/cibuild
> > Applying: ci: build.sh: Add variables from .gitlab-ci.yml
> > Applying: ci: build.sh: Add GIT_ROOT env helper variable
> > Applying: ci: build.sh: Don't mention that MESON_ARGS are available via CLI
> > error: sha1 information is lacking or useless (ci/build.sh).
> > error: could not build fake ancestor
> > Patch failed at 0003 ci: build.sh: Don't mention that MESON_ARGS are 
> > available via CLI
> > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> 
> Yeah, I had a different merge conflict locally, but I guess both related to
> b68d253c4603a041cb993fa133ba25e610a78929 not having been pushed yet and the
> time I posted the series.

Yeah, I eventually figured out b68d253c4603a041cb993fa133ba25e610a78929 was
the problem and applied your series before that commit to test 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 :|

Re: [libvirt PATCH v2 6/6] Revert "capabilities: report full external snapshot support"

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:17 +0200, Pavel Hrdina wrote:
> Reverting external snapshot for running VM doesn't work correctly so we
> should not report this capability until it is fixed.
> 
> This reverts commit de71573bfec7f3acd22ec74794318de121716e21.
> 
> Signed-off-by: Pavel Hrdina 
> ---

If Jirka decides to go with this alternative and cut the release before
the fixes:

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH v2 5/6] NEWS: document support for reverting external snapshots

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:16 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  NEWS.rst | 8 
>  1 file changed, 8 insertions(+)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH v2 4/6] qemu_snapshot: correctly load the saved memory state file

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:15 +0200, Pavel Hrdina wrote:
> Original code assumed that the memory state file is only migration
> stream but it has additional metadata stored by libvirt. To correctly
> load the memory state file we need to reuse code that is used when
> restoring domain from saved image.
> 
> This duplicates some necessary parts of qemuSaveImageStartVM() because
> the external snapshot memory state is done by qemuSaveImageCreate().
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_snapshot.c | 115 +--
>  1 file changed, 97 insertions(+), 18 deletions(-)

Hmmm, in the end you were right. This version is more verbose and in
fact doesn't end up having two code paths for external and internal but
both call qemuProcessStart which still does everything together.

I'll re-visit v1. Since my main concern is not aleviated by this one and
I think v1 can be made tolerable by improving some function
documentation.

Regardless of the above:

Reviewed-by: Peter Krempa 

as I went through this pathc already.

Re: [libvirt PATCH 00/33] ci: Unify the GitLab CI jobs with local executions && adopt lcitool container executions

[Erik Skultety]

On Thu, Aug 31, 2023 at 05:17:13PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
> > Technically a v2 of:
> > https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
> > 
> > However, the approach here is slightly different and what that series said
> > about migration to lcitool container executions as a replacement for
> > ci/Makefile is actually done here. One of the core problems of the above
> > pointed out in review was that more Shell logic was introduced including CLI
> > parsing, conditional executions, etc. which we fought hard to get rid of in 
> > the
> > past. I reworked the Shell functions quite a bit and dropped whatever extra
> > Shell logic the original series added.
> > Obviously we can't get rid of Shell completely because of .gitlab-ci.yml 
> > and so
> > I merely extracted the recipes into functions which are then sourced as
> > ci/build.sh and executed. Now, that on its own would hide the actual 
> > commands
> > being run in the GitLab job log, so before any command is actually 
> > executed, it
> > is formatted with a color sequence so we don't miss that information as that
> > would be a regression to the status quo.
> > 
> > Lastly, this series then takes the effort inside the ci/build.sh script and
> > basically mirrors whatever GitLab would do to run a job inside a local
> > container which is executed by lcitool (yes, we already have that 
> > capability).
> > 
> > Please give this a try and I'm already looking forward to comments as I'd 
> > like
> > to expand this effort to local VM executions running the TCK integration 
> > tests,
> > so this series is quite important in that regard.
> 
> Do you have a gitlab branch with this contnt somewhere. When i tried to
> apply the patches to current git, it was unhappy on the 3rd patch
> 
> $ git am -3 ~/cibuild
> Applying: ci: build.sh: Add variables from .gitlab-ci.yml
> Applying: ci: build.sh: Add GIT_ROOT env helper variable
> Applying: ci: build.sh: Don't mention that MESON_ARGS are available via CLI
> error: sha1 information is lacking or useless (ci/build.sh).
> error: could not build fake ancestor
> Patch failed at 0003 ci: build.sh: Don't mention that MESON_ARGS are 
> available via CLI
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".

Yeah, I had a different merge conflict locally, but I guess both related to
b68d253c4603a041cb993fa133ba25e610a78929 not having been pushed yet and the
time I posted the series.

Here's the fixed branch:
https://gitlab.com/eskultety/libvirt/-/commits/helper-lcitool

And here's a pipeline (with the fix posted for patch 11):
https://gitlab.com/eskultety/libvirt/-/pipelines/988871113

Erik

Re: [libvirt PATCH 05/33] ci: build.sh: Add a wrapper function executing 'shell' commands

[Daniel P . Berrangé]

On Fri, Sep 01, 2023 at 11:16:09AM +0200, Erik Skultety wrote:
> On Fri, Sep 01, 2023 at 10:10:55AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
> > > This would normally be not needed at all, but the problem here is the
> > > Shell-in-YAML which GitLab interprets. It outputs every command that
> > > appears as a line in the 'script' segment in a color-coded fashion for
> > > easy identification of problems. Well, that useful feature is lost when
> > > there's indirection and one script calls into another in which case it
> > > would only output the respective script name which would make failure
> > > investigation harder. This simple helper tackles that by echoing the
> > > command to be run by any script/function with a color escape sequence
> > > so that we don't lose track of the *actual* shell commands being run as
> > > part of the GitLab job pipelines. An example of what the output then
> > > might look like:
> > > [RUN COMMAND]: 'meson compile -C build install-web'
> > > 
> > > Signed-off-by: Erik Skultety 
> > > ---
> > >  ci/build.sh | 6 ++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/ci/build.sh b/ci/build.sh
> > > index 02ff1a8388..d4fbf0ab37 100644
> > > --- a/ci/build.sh
> > > +++ b/ci/build.sh
> > > @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS 
> > > || \
> > >  
> > >  ninja -C build $NINJA_ARGS
> > >  
> > > +run_cmd() {
> > > +local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces
> > > +
> > > +printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD"
> > > +}
> > 
> > I think we sould just get rid of the $CMD env variable in the caller
> > entirely and pass in arguments individual. eg so this method becomes
> > 
> >  run_cmd() {
> >  printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$*"
> >  $@
> >  }
> > 
> > Then in the callers instead of
> > 
> > local CMD="meson compile -C build $BUILD_ARGS"
> > run_cmd "$CMD"
> > 
> > We get
> > 
> > run_cmd meson compile -C build "$BUILD_ARGS"
> > 
> > this would have avoided the bug you just posted a fix for where
> > we set 'local CMD' but forget the actual 'run_cmd' call.
> 
> My only complaint is, again, readability - this particular example is fine, it
> would IMO become a mess with commands taking several arguments which would not
> fit onto a single line. I don't expect these functions to change much, so 
> while
> you're absolutely right about preventing bugs like that, I think having some
> reasonable readability (shell--) would be a good tradeoff.

A line continuation isn't that hard to add for comamnds which get too
long.

Avoiding the intermediate variable is also more robust by eliminating
two levels of extra quoting. Quoting/expansion problems are one of the
more painful aspects of working in shell, so I think its good to cull
extra variables where practical.

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

Re: [libvirt PATCH 11/33] ci: build.sh: Add a wrapper function over the 'rpmbuild' job

[Daniel P . Berrangé]

On Fri, Sep 01, 2023 at 09:29:13AM +0200, Erik Skultety wrote:
> On Thu, Aug 31, 2023 at 05:55:54PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
> > > This helper is a shell function transcript of its original GitLab CI
> > > counterpart.
> > > 
> > > Signed-off-by: Erik Skultety 
> > > ---
> > >  ci/build.sh | 11 +++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/ci/build.sh b/ci/build.sh
> > > index 6990f2d171..30f4712e4b 100644
> > > --- a/ci/build.sh
> > > +++ b/ci/build.sh
> > > @@ -94,3 +94,14 @@ run_potfile() {
> > >  run_meson_setup
> > >  run_build
> > >  }
> > > +
> > > +run_rpmbuild() {
> > > +local CMD="rpmbuild \
> > > + --clean \
> > > + --nodeps \
> > > + --define "_without_mingw 1" \
> > > + -ta build/meson-dist/libvirt-*.tar.xz"
> > > +
> > > +run_meson_setup
> > 
> > Redundant here as implied by run_dist
> 
> While I can drop all these redundant setup calls, the reason why I put them
> there was simply readability. Let's face it Shell isn't a particularly nice
> structured language to look at especially when it comes to functions, so 
> rather
> than having everyone go and look what run_dist/run_build, etc. do, I instead
> opted for transparent naming and explicit function calls, IOW so that when you
> look at run_rpmbuild you immediately know we run meson setup followed by a
> project build, etc., but then if you call run_dist alone in a local 
> interactive
> environment then run_dist would also call meson_setup because it can't go
> without it, so hence the redundancy in all the functions.
> If you still feel like it's undesirable even in ^this case, I will drop the
> calls.

In practice I don't think users need to worry about what each
functions runs, because your implementation has ensured that
every single cmd is "self contained" and will run any dependancies
is has. Given that I think it is overkill to have the transitive
expansion of every dependancy, just the immediate dependency is
sufficient.

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

Re: [libvirt PATCH 05/33] ci: build.sh: Add a wrapper function executing 'shell' commands

[Erik Skultety]

On Fri, Sep 01, 2023 at 10:10:55AM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
> > This would normally be not needed at all, but the problem here is the
> > Shell-in-YAML which GitLab interprets. It outputs every command that
> > appears as a line in the 'script' segment in a color-coded fashion for
> > easy identification of problems. Well, that useful feature is lost when
> > there's indirection and one script calls into another in which case it
> > would only output the respective script name which would make failure
> > investigation harder. This simple helper tackles that by echoing the
> > command to be run by any script/function with a color escape sequence
> > so that we don't lose track of the *actual* shell commands being run as
> > part of the GitLab job pipelines. An example of what the output then
> > might look like:
> > [RUN COMMAND]: 'meson compile -C build install-web'
> > 
> > Signed-off-by: Erik Skultety 
> > ---
> >  ci/build.sh | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/ci/build.sh b/ci/build.sh
> > index 02ff1a8388..d4fbf0ab37 100644
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
> >  
> >  ninja -C build $NINJA_ARGS
> >  
> > +run_cmd() {
> > +local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces
> > +
> > +printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD"
> > +}
> 
> I think we sould just get rid of the $CMD env variable in the caller
> entirely and pass in arguments individual. eg so this method becomes
> 
>  run_cmd() {
>  printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$*"
>  $@
>  }
> 
> Then in the callers instead of
> 
> local CMD="meson compile -C build $BUILD_ARGS"
> run_cmd "$CMD"
> 
> We get
> 
> run_cmd meson compile -C build "$BUILD_ARGS"
> 
> this would have avoided the bug you just posted a fix for where
> we set 'local CMD' but forget the actual 'run_cmd' call.

My only complaint is, again, readability - this particular example is fine, it
would IMO become a mess with commands taking several arguments which would not
fit onto a single line. I don't expect these functions to change much, so while
you're absolutely right about preventing bugs like that, I think having some
reasonable readability (shell--) would be a good tradeoff.

Erik

Re: Sunset libvirt-snmp?

[Andrea Bolognani]

On Thu, Aug 31, 2023 at 04:53:47PM +0100, Daniel P. Berrangé wrote:
> I'm also strongly inclined to archive the following:
>
>   * libvirt-cim
>   * libvirt-appdev-guide-python
>   * libvirt-publican
>   * libvirt-sandbox
>   * libvirt-sandbox-image
>   * libvirt-designer

Sounds good to me. It will save us some effort if we don't need to
keep these repositories up to our standards (mainly in terms of CI)
while they're seeing zero actual development.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 3/6] qemu_saveimage: export qemuSaveImageGetCompressionCommand

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:14 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_saveimage.c | 2 +-
>  src/qemu/qemu_saveimage.h | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)

Also squashable into the previous commit since you need both for the
same reason.

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH v2 2/6] qemu_saveimage: export virQEMUSaveFormat enum

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:13 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_saveimage.c | 17 -
>  src/qemu/qemu_saveimage.h | 17 +
>  2 files changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Peter Krempa 

Re: [libvirt PATCH 05/33] ci: build.sh: Add a wrapper function executing 'shell' commands

[Daniel P . Berrangé]

On Fri, Aug 25, 2023 at 07:55:13PM +0200, Erik Skultety wrote:
> This would normally be not needed at all, but the problem here is the
> Shell-in-YAML which GitLab interprets. It outputs every command that
> appears as a line in the 'script' segment in a color-coded fashion for
> easy identification of problems. Well, that useful feature is lost when
> there's indirection and one script calls into another in which case it
> would only output the respective script name which would make failure
> investigation harder. This simple helper tackles that by echoing the
> command to be run by any script/function with a color escape sequence
> so that we don't lose track of the *actual* shell commands being run as
> part of the GitLab job pipelines. An example of what the output then
> might look like:
> [RUN COMMAND]: 'meson compile -C build install-web'
> 
> Signed-off-by: Erik Skultety 
> ---
>  ci/build.sh | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/ci/build.sh b/ci/build.sh
> index 02ff1a8388..d4fbf0ab37 100644
> --- a/ci/build.sh
> +++ b/ci/build.sh
> @@ -25,6 +25,12 @@ meson setup build --werror -Dsystem=true $MESON_ARGS || \
>  
>  ninja -C build $NINJA_ARGS
>  
> +run_cmd() {
> +local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces
> +
> +printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$CMD"; eval "$CMD"
> +}

I think we sould just get rid of the $CMD env variable in the caller
entirely and pass in arguments individual. eg so this method becomes

 run_cmd() {
 printf "\e[93m[RUN COMMAND]: '%s'\e[0m\n" "$*"
 $@
 }

Then in the callers instead of

local CMD="meson compile -C build $BUILD_ARGS"
run_cmd "$CMD"

We get

run_cmd meson compile -C build "$BUILD_ARGS"

this would have avoided the bug you just posted a fix for where
we set 'local CMD' but forget the actual 'run_cmd' call.


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

Re: [libvirt PATCH v2 1/6] qemu_snapshot: fix reverting external snapshot when not all disks are included

[Peter Krempa]

On Fri, Sep 01, 2023 at 10:32:12 +0200, Pavel Hrdina wrote:
> We need to skip all disks that have snapshot type other than 'external'.

Since the commit message is vague on the specific problem details ...

> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_snapshot.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index d943281e35..ff85d56572 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
>  virDomainSnapshotDiskDef *snapdisk = >disks[i];
>  virDomainDiskDef *domdisk = domdef->disks[i];
>  
> +if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> +continue;

... I remember you talking about the need to create overlays also for
images which might have been excluded in given snapshot (the one you are
reverting to) , but externally snapshotted in a later (still existing)
one.

In such case not creating the overlay here would still invalidate the
overlay of the next snapshot. Is that right?

I also remember that you mentioned that the actual problem is with empty
cdroms, thus, did you rather want to use 'virStorageSourceIsEmpty' here?

Re: [libvirt PATCH 11/33] ci: build.sh: Add a wrapper function over the 'rpmbuild' job

[Erik Skultety]

On Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
> This helper is a shell function transcript of its original GitLab CI
> counterpart.
> 
> Signed-off-by: Erik Skultety 
> ---
>  ci/build.sh | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/ci/build.sh b/ci/build.sh
> index 6990f2d171..30f4712e4b 100644
> --- a/ci/build.sh
> +++ b/ci/build.sh
> @@ -94,3 +94,14 @@ run_potfile() {
>  run_meson_setup
>  run_build
>  }
> +
> +run_rpmbuild() {
> +local CMD="rpmbuild \
> + --clean \
> + --nodeps \
> + --define "_without_mingw 1" \
> + -ta build/meson-dist/libvirt-*.tar.xz"
> +
> +run_meson_setup
> +run_dist
> +}
> -- 
> 2.41.0
> 

Consider the following squashed in:

diff --git a/ci/build.sh b/ci/build.sh
index 5eef53f8d1..b990f5eeac 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -87,6 +87,7 @@ run_rpmbuild() {

 run_meson_setup
 run_dist
+run_cmd "$CMD"
 }


AND


diff --git a/ci/build.sh b/ci/build.sh
index b990f5eeac..a45bc2b110 100644
--- a/ci/build.sh
+++ b/ci/build.sh
@@ -82,7 +82,7 @@ run_rpmbuild() {
 local CMD="rpmbuild \
  --clean \
  --nodeps \
- --define "_without_mingw 1" \
+ --define '_without_mingw 1' \
  -ta build/meson-dist/libvirt-*.tar.xz"

 run_meson_setup


Regards,
Erik

[libvirt PATCH v2 2/6] qemu_saveimage: export virQEMUSaveFormat enum

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_saveimage.c | 17 -
 src/qemu/qemu_saveimage.h | 17 +
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 41310d6a9a..eca47171c2 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -37,23 +37,6 @@
 
 VIR_LOG_INIT("qemu.qemu_saveimage");
 
-typedef enum {
-QEMU_SAVE_FORMAT_RAW = 0,
-QEMU_SAVE_FORMAT_GZIP = 1,
-QEMU_SAVE_FORMAT_BZIP2 = 2,
-/*
- * Deprecated by xz and never used as part of a release
- * QEMU_SAVE_FORMAT_LZMA
- */
-QEMU_SAVE_FORMAT_XZ = 3,
-QEMU_SAVE_FORMAT_LZOP = 4,
-/* Note: add new members only at the end.
-   These values are used in the on-disk format.
-   Do not change or re-use numbers. */
-
-QEMU_SAVE_FORMAT_LAST
-} virQEMUSaveFormat;
-
 VIR_ENUM_DECL(qemuSaveCompression);
 VIR_ENUM_IMPL(qemuSaveCompression,
   QEMU_SAVE_FORMAT_LAST,
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 30cf4b1ee0..6892e6764f 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -32,6 +32,23 @@
 
 G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL));
 
+typedef enum {
+QEMU_SAVE_FORMAT_RAW = 0,
+QEMU_SAVE_FORMAT_GZIP = 1,
+QEMU_SAVE_FORMAT_BZIP2 = 2,
+/*
+ * Deprecated by xz and never used as part of a release
+ * QEMU_SAVE_FORMAT_LZMA
+ */
+QEMU_SAVE_FORMAT_XZ = 3,
+QEMU_SAVE_FORMAT_LZOP = 4,
+/* Note: add new members only at the end.
+   These values are used in the on-disk format.
+   Do not change or re-use numbers. */
+
+QEMU_SAVE_FORMAT_LAST
+} virQEMUSaveFormat;
+
 typedef struct _virQEMUSaveHeader virQEMUSaveHeader;
 struct _virQEMUSaveHeader {
 char magic[sizeof(QEMU_SAVE_MAGIC)-1];
-- 
2.41.0

[libvirt PATCH v2 4/6] qemu_snapshot: correctly load the saved memory state file

[Pavel Hrdina]

Original code assumed that the memory state file is only migration
stream but it has additional metadata stored by libvirt. To correctly
load the memory state file we need to reuse code that is used when
restoring domain from saved image.

This duplicates some necessary parts of qemuSaveImageStartVM() because
the external snapshot memory state is done by qemuSaveImageCreate().

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 115 +--
 1 file changed, 97 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index ff85d56572..538da6570a 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2004,6 +2004,22 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm,
 }
 
 
+typedef struct _qemuSnapshotRevertMemoryData {
+int fd;
+char *path;
+virQEMUSaveData *data;
+} qemuSnapshotRevertMemoryData;
+
+
+static void
+qemuSnapshotClearRevertMemoryData(qemuSnapshotRevertMemoryData *memdata)
+{
+VIR_FORCE_CLOSE(memdata->fd);
+virQEMUSaveDataFree(memdata->data);
+}
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuSnapshotRevertMemoryData, 
qemuSnapshotClearRevertMemoryData);
+
+
 /**
  * qemuSnapshotRevertExternalPrepare:
  * @vm: domain object
@@ -2011,15 +2027,13 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm,
  * @snap: snapshot object we are reverting to
  * @config: live domain definition
  * @inactiveConfig: offline domain definition
- * memsnapFD: pointer to store memory state file FD or NULL
- * memsnapPath: pointer to store memory state file path or NULL
+ * @memdata: struct with data to load memory state
  *
  * Prepare new temporary snapshot definition @tmpsnapdef that will
  * be used while creating new overlay files after reverting to snapshot
  * @snap. In case we are reverting to snapshot with memory state it will
- * open it and pass FD via @memsnapFD and path to the file via
- * @memsnapPath, caller is responsible for freeing both @memsnapFD and
- * memsnapPath.
+ * open it and store necessary data in @memdata. Caller is responsible
+ * to clear the data by using qemuSnapshotClearRevertMemoryData().
  *
  * Returns 0 in success, -1 on error.
  */
@@ -2029,8 +2043,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
   virDomainMomentObj *snap,
   virDomainDef *config,
   virDomainDef *inactiveConfig,
-  int *memsnapFD,
-  char **memsnapPath)
+  qemuSnapshotRevertMemoryData *memdata)
 {
 size_t i;
 bool active = virDomainObjIsActive(vm);
@@ -2065,12 +2078,20 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
 return -1;
 }
 
-if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) {
+if (memdata && snapdef->memorysnapshotfile) {
 virQEMUDriver *driver = ((qemuDomainObjPrivate *) 
vm->privateData)->driver;
-g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virDomainDef) savedef = NULL;
 
-*memsnapPath = snapdef->memorysnapshotfile;
-*memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, 
NULL);
+memdata->path = snapdef->memorysnapshotfile;
+memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path,
+, >data,
+false, NULL, false, false);
+
+if (memdata->fd < 0)
+return -1;
+
+if  (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt))
+return -1;
 }
 
 return 0;
@@ -2254,13 +2275,16 @@ qemuSnapshotRevertActive(virDomainObj *vm,
 virObjectEvent *event = NULL;
 virObjectEvent *event2 = NULL;
 virDomainMomentObj *loadSnap = NULL;
-VIR_AUTOCLOSE memsnapFD = -1;
-char *memsnapPath = NULL;
 int detail;
 bool defined = false;
 qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie;
 int rc;
 g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL;
+g_auto(qemuSnapshotRevertMemoryData) memdata = { -1, NULL, NULL };
+g_autoptr(virCommand) cmd = NULL;
+g_autofree char *errbuf = NULL;
+VIR_AUTOCLOSE intermediatefd = -1;
+int memrc = 0;
 
 start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
 
@@ -2284,7 +2308,7 @@ qemuSnapshotRevertActive(virDomainObj *vm,
 
 if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap,
   *config, *inactiveConfig,
-  , ) < 0) {
+  ) < 0) {
 return -1;
 }
 } else {
@@ -2298,6 +2322,30 @@ qemuSnapshotRevertActive(virDomainObj *vm,
 
 virDomainObjAssignDef(vm, config, true, NULL);
 
+if (virDomainSnapshotIsExternal(snap) && memdata.data) {
+virQEMUSaveHeader 

[libvirt PATCH v2 5/6] NEWS: document support for reverting external snapshots

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 NEWS.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index e40c8ac259..a3be76d6cc 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -28,6 +28,14 @@ v9.7.0 (unreleased)
 2) pre-binding the variant driver using the ``--driver`` option of
``virsh nodedev-detach``.
 
+  * QEMU: implement reverting external snapshots
+
+Reverting external snapshots is now possible using the existing API
+``virDomainSnapshotRevert()``. Management application can check host
+capabilities for  element within the list of
+guest features to see if the current libvirt supports both deleting
+and reverting external snapshots.
+
 * **Improvements**
 
 * **Bug fixes**
-- 
2.41.0

[libvirt PATCH v2 3/6] qemu_saveimage: export qemuSaveImageGetCompressionCommand

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_saveimage.c | 2 +-
 src/qemu/qemu_saveimage.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index eca47171c2..44ab263144 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -207,7 +207,7 @@ virQEMUSaveDataFinish(virQEMUSaveData *data,
 }
 
 
-static virCommand *
+virCommand *
 qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression)
 {
 virCommand *ret = NULL;
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 6892e6764f..510576baa2 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -105,6 +105,9 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
bool use_raw_on_fail)
 ATTRIBUTE_NONNULL(2);
 
+virCommand *
+qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression);
+
 int
 qemuSaveImageCreate(virQEMUDriver *driver,
 virDomainObj *vm,
-- 
2.41.0

[libvirt PATCH v2 6/6] Revert "capabilities: report full external snapshot support"

[Pavel Hrdina]

Reverting external snapshot for running VM doesn't work correctly so we
should not report this capability until it is fixed.

This reverts commit de71573bfec7f3acd22ec74794318de121716e21.

Signed-off-by: Pavel Hrdina 
---
 docs/formatcaps.rst| 7 ---
 src/conf/capabilities.c| 1 -
 src/conf/capabilities.h| 1 -
 src/conf/schemas/capability.rng| 5 -
 src/qemu/qemu_capabilities.c   | 1 -
 tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml | 1 -
 tests/qemucaps2xmloutdata/caps.aarch64.xml | 1 -
 tests/qemucaps2xmloutdata/caps.ppc.xml | 1 -
 tests/qemucaps2xmloutdata/caps.ppc64.xml   | 1 -
 tests/qemucaps2xmloutdata/caps.riscv64.xml | 1 -
 tests/qemucaps2xmloutdata/caps.s390x.xml   | 1 -
 tests/qemucaps2xmloutdata/caps.sparc.xml   | 1 -
 tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml  | 1 -
 tests/qemucaps2xmloutdata/caps.x86_64.xml  | 1 -
 14 files changed, 24 deletions(-)

diff --git a/docs/formatcaps.rst b/docs/formatcaps.rst
index 95502c511f..bb8bc663d2 100644
--- a/docs/formatcaps.rst
+++ b/docs/formatcaps.rst
@@ -134,12 +134,6 @@ The  element will typically wrap up the 
following elements:
   creating external disk snapshots is supported. If absent, creating 
external
   snapshots may still be supported, but it requires attempting the API and
   checking for an error to find out for sure. :since:`Since 1.2.3`
-   ``externalSnapshot``
-  If this element is present, the hypervisor supports deleting and
-  reverting external snapshots including memory state. Support for creation
-  of external snapshots is reported via the ``disksnapshot`` feature flag.
-  Management applications can now switch from internal snapshots to 
external
-  snapshots. :since:`Since 9.7.0`
 
 Examples
 
@@ -324,7 +318,6 @@ capabilities enabled in the chip and BIOS you will see:
 
 
 
-
   
 
 
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 34f04cb7d3..56768ce6e0 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -508,7 +508,6 @@ static const struct virCapsGuestFeatureInfo 
virCapsGuestFeatureInfos[VIR_CAPS_GU
 [VIR_CAPS_GUEST_FEATURE_TYPE_DEVICEBOOT] = { "deviceboot", false },
 [VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT] = { "disksnapshot", true },
 [VIR_CAPS_GUEST_FEATURE_TYPE_HAP] = { "hap", true },
-[VIR_CAPS_GUEST_FEATURE_TYPE_EXTERNAL_SNAPSHOT] = { "externalSnapshot", 
false },
 };
 
 
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index 9eaf6e2807..c78e3e52fa 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -40,7 +40,6 @@ typedef enum {
 VIR_CAPS_GUEST_FEATURE_TYPE_DEVICEBOOT,
 VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,
 VIR_CAPS_GUEST_FEATURE_TYPE_HAP,
-VIR_CAPS_GUEST_FEATURE_TYPE_EXTERNAL_SNAPSHOT,
 
 VIR_CAPS_GUEST_FEATURE_TYPE_LAST
 } virCapsGuestFeatureType;
diff --git a/src/conf/schemas/capability.rng b/src/conf/schemas/capability.rng
index b1968df258..83b414961a 100644
--- a/src/conf/schemas/capability.rng
+++ b/src/conf/schemas/capability.rng
@@ -493,11 +493,6 @@
 
   
 
-
-  
-
-  
-
   
 
   
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 05cc11218a..40eacf0f4d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1121,7 +1121,6 @@ virQEMUCapsInitGuestFromBinary(virCaps *caps,
 virCapabilitiesAddGuestFeature(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DEVICEBOOT);
 virCapabilitiesAddGuestFeatureWithToggle(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT,
  true, false);
-virCapabilitiesAddGuestFeature(guest, 
VIR_CAPS_GUEST_FEATURE_TYPE_EXTERNAL_SNAPSHOT);
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG)) {
 virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU,
diff --git a/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml 
b/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml
index b9a5b5a1d6..b53a886b90 100644
--- a/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml
+++ b/tests/qemucaps2xmloutdata/caps.aarch64+hvf.xml
@@ -21,7 +21,6 @@
   
   
   
-  
 
   
 
diff --git a/tests/qemucaps2xmloutdata/caps.aarch64.xml 
b/tests/qemucaps2xmloutdata/caps.aarch64.xml
index 61512ed740..5dca6d3102 100644
--- a/tests/qemucaps2xmloutdata/caps.aarch64.xml
+++ b/tests/qemucaps2xmloutdata/caps.aarch64.xml
@@ -21,7 +21,6 @@
   
   
   
-  
 
   
 
diff --git a/tests/qemucaps2xmloutdata/caps.ppc.xml 
b/tests/qemucaps2xmloutdata/caps.ppc.xml
index 86d6b85ae0..e7ba391795 100644
--- a/tests/qemucaps2xmloutdata/caps.ppc.xml
+++ b/tests/qemucaps2xmloutdata/caps.ppc.xml
@@ -19,7 +19,6 @@
   
   
   
-  
 
   
 
diff --git 

[libvirt PATCH v2 1/6] qemu_snapshot: fix reverting external snapshot when not all disks are included

[Pavel Hrdina]

We need to skip all disks that have snapshot type other than 'external'.

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index d943281e35..ff85d56572 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm,
 virDomainSnapshotDiskDef *snapdisk = >disks[i];
 virDomainDiskDef *domdisk = domdef->disks[i];
 
+if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+
 if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) 
< 0)
 return -1;
 }
@@ -2098,6 +2101,9 @@ qemuSnapshotRevertExternalActive(virDomainObj *vm,
 return -1;
 
 for (i = 0; i < tmpsnapdef->ndisks; i++) {
+if (tmpsnapdef->disks[i].snapshot != 
VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+
 if (qemuSnapshotDiskPrepareOne(snapctxt,
vm->def->disks[i],
tmpsnapdef->disks + i,
@@ -2188,6 +2194,9 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm,
 for (i = 0; i < curdef->nrevertdisks; i++) {
 virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]);
 
+if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+
 if (virStorageSourceInit(snapdisk->src) < 0 ||
 virStorageSourceUnlink(snapdisk->src) < 0) {
 VIR_WARN("Failed to remove snapshot image '%s'",
@@ -2203,6 +2212,9 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm,
 for (i = 0; i < curdef->ndisks; i++) {
 virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]);
 
+if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+continue;
+
 if (virStorageSourceInit(snapdisk->src) < 0 ||
 virStorageSourceUnlink(snapdisk->src) < 0) {
 VIR_WARN("Failed to remove snapshot image '%s'",
-- 
2.41.0

[libvirt PATCH v2 0/6] external snapshot revert fixes

[Pavel Hrdina]

This fixes reverting external snapshots to not error out in cases where
it should work and makes it correctly load the memory state when
reverting to snapshot of running VM.

Last patch is not part of the fix, it is alternative solution to remove
the capability as we are close to releasing libvirt 9.7.0 in case the
code fixing reverting snapshots is considered too complex.

This v2 limits the impact only to snapshot code and no longer changes
anything in qemu_saveimage except for exporting one function and one
enum.

Pavel Hrdina (6):
  qemu_snapshot: fix reverting external snapshot when not all disks are
included
  qemu_saveimage: export virQEMUSaveFormat enum
  qemu_saveimage: export qemuSaveImageGetCompressionCommand
  qemu_snapshot: correctly load the saved memory state file
  NEWS: document support for reverting external snapshots
  Revert "capabilities: report full external snapshot support"

 NEWS.rst  |   8 ++
 docs/formatcaps.rst   |   7 -
 src/conf/capabilities.c   |   1 -
 src/conf/capabilities.h   |   1 -
 src/conf/schemas/capability.rng   |   5 -
 src/qemu/qemu_capabilities.c  |   1 -
 src/qemu/qemu_saveimage.c |  19 +--
 src/qemu/qemu_saveimage.h |  20 +++
 src/qemu/qemu_snapshot.c  | 127 +++---
 .../qemucaps2xmloutdata/caps.aarch64+hvf.xml  |   1 -
 tests/qemucaps2xmloutdata/caps.aarch64.xml|   1 -
 tests/qemucaps2xmloutdata/caps.ppc.xml|   1 -
 tests/qemucaps2xmloutdata/caps.ppc64.xml  |   1 -
 tests/qemucaps2xmloutdata/caps.riscv64.xml|   1 -
 tests/qemucaps2xmloutdata/caps.s390x.xml  |   1 -
 tests/qemucaps2xmloutdata/caps.sparc.xml  |   1 -
 tests/qemucaps2xmloutdata/caps.x86_64+hvf.xml |   1 -
 tests/qemucaps2xmloutdata/caps.x86_64.xml |   1 -
 18 files changed, 138 insertions(+), 60 deletions(-)

-- 
2.41.0

Re: [libvirt PATCH 00/33] ci: Unify the GitLab CI jobs with local executions && adopt lcitool container executions

[Erik Skultety]

On Thu, Aug 31, 2023 at 06:28:16PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
> > Technically a v2 of:
> > https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
> > 
> > However, the approach here is slightly different and what that series said
> > about migration to lcitool container executions as a replacement for
> > ci/Makefile is actually done here. One of the core problems of the above
> > pointed out in review was that more Shell logic was introduced including CLI
> > parsing, conditional executions, etc. which we fought hard to get rid of in 
> > the
> > past. I reworked the Shell functions quite a bit and dropped whatever extra
> > Shell logic the original series added.
> > Obviously we can't get rid of Shell completely because of .gitlab-ci.yml 
> > and so
> > I merely extracted the recipes into functions which are then sourced as
> > ci/build.sh and executed. Now, that on its own would hide the actual 
> > commands
> > being run in the GitLab job log, so before any command is actually 
> > executed, it
> > is formatted with a color sequence so we don't miss that information as that
> > would be a regression to the status quo.
> > 
> > Lastly, this series then takes the effort inside the ci/build.sh script and
> > basically mirrors whatever GitLab would do to run a job inside a local
> > container which is executed by lcitool (yes, we already have that 
> > capability).
> > 
> > Please give this a try and I'm already looking forward to comments as I'd 
> > like
> > to expand this effort to local VM executions running the TCK integration 
> > tests,
> > so this series is quite important in that regard.
> 
> Overall I'm fine with what's proposed here.
> 
> Two general thoughts
> 
>  * ci/Makefile appears pretty much redundant - ci/helper can provide
>the same level of functionality AFAICT, and it'd be nice to kill
>an outstanding usage of 'make' given our goal to standardize on
>meson + python

Huh, the fact that removal of Makefile isn't part of this series is a mistake
on my side - I worked on this on 2 parallel branches trying out 2 slightly
different approaches. I did drop it on one branch but not the other which I
ultimately decided to go with. Since I'll be sending a v2, I'll add that patch.

> 
>  * ci/helper looks almost entirely independent of libvirt, aside
>from the list of 'choices' for the --job arg, and the --namespace
>arg default value, it would work with any virt project we have if
>the project created its own ci/build.sh file
> 
>Can we fold all its logic into lcitool, and just have that as
>the entrypoint ? In ci/manifest.yml we can get the project
>namespace, and we could possibly just extra the commands by
>crudely regex matching 'ci/build.sh' content against the
>pattern '^run_.*\(\)$ {'

Technically we could. Extracting the code and injecting it into lcitool is not
a problem, in fact, it would be quite straight forward. The problem is
designing a CLI interface that would make sense for the use case without
breaking the existing one too much. Ideally by introducing just a bunch of
optional args which I don't think is very realistic. Since that will require
thorough thinking and designing I did not want to dive right into that as I
wasn't even sure whether I'd be able to push this conversion through upstream.

> 
> 
> The removal of ci/Makefil feels like it could be done in this series,
> but its fine if the ci/helper suggestion is left as separate future
> work.

Yeah, like I said above, incorporating ci/helper into lcitool is likely going
to be again one of the bigger overhauls so that will be a project on its own.

Thanks for the comments, much appreciated.

Erik

Re: [libvirt PATCH 4/7] qemuSaveImageStartProcess: make it possible to use without header

[Peter Krempa]

On Thu, Aug 31, 2023 at 17:59:27 +0200, Pavel Hrdina wrote:
> On Thu, Aug 31, 2023 at 05:43:35PM +0200, Peter Krempa wrote:
> > On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
> > > When used with internal snapshots there is no header to be used and no
> > > memory state to be decompressed.
> > 
> > I didn't yet have a look at the rest, but this made me curious. What are
> > you actually doing with this with internal snapshots?
> > 
> > There in fact isn't a save image at all with internal snapshots as the
> > memory image is stored in the qcow2 image (in a different section than
> > the data -> thus also the name internal) so I'm not exactly sure what
> > you are refering to here in the commit message.
> 
> All of that is correct. In PATCH 6/7 this new function is called from
> qemuSnapshotRevertActive unconditionally. And it will handle reverting
> internal and external snapshots and do the correct thing based on what
> arguments are passed to it.
> 
> In case of internal snapshots we were calling qemuProcessStart and
> passing virDomainMomentObj. To avoid code duplication this parameter
> was introduced in PATCH 3/7 to this new helper so it can start QEMU
> process when reverting to internal snapshot as well as when reverting to
> external snapshots.

Given how complex snapshots are I don't think it would make it any
cleaner if a single function is called that has two similar but
semantically very distinct behaviours and the reader has to understand
that it's based on the value of the arguments which behaviour is chosen.

Thus if you have both function calls and a condition selecting one of
them it will be easier for readers.

Re: [libvirt PATCH 14/33] ci: build.sh: Drop direct invocation of meson/ninja commands

[Erik Skultety]

On Thu, Aug 31, 2023 at 05:59:26PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:22PM +0200, Erik Skultety wrote:
> > We've moved all invocations to the respective helper function which
> > we'll execute both from gitlab CI jobs and local environments so we
> > don't need to have them on the global level as it would also not work
> > with "sourcing" this file to populate the environment with function
> > definitions.
> > 
> > Signed-off-by: Erik Skultety 
> > ---
> >  ci/build.sh | 5 -
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/ci/build.sh b/ci/build.sh
> > index 133952f706..b075c49af3 100644
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -18,11 +18,6 @@ GIT_ROOT="$(git rev-parse --show-toplevel)"
> >  
> >  MESON_ARGS="$MESON_ARGS $MESON_OPTS"
> >  
> > -meson setup build --werror -Dsystem=true $MESON_ARGS || \
> > -(cat build/meson-logs/meson-log.txt && exit 1)
> > -
> > -ninja -C build $NINJA_ARGS
> > -
> >  run_cmd() {
> >  local CMD="$(echo $CMD | tr -s ' ')" # truncate any additional spaces
> >
> 
> Now we drop immediate invokation at time of execution, I wonder if the
> build.sh name is a little mis-leading.
> 
> Might be better renamed to 'functions.sh' or 'commands.sh' perhaps, so
> the name doesn't suggest that it actually builds stuff, merely that it
> supplies some shell logic ?

Fair enough, didn't like the name either, but I wanted to keep at least
something we had around :).

Erik

Re: [libvirt PATCH 11/33] ci: build.sh: Add a wrapper function over the 'rpmbuild' job

[Erik Skultety]

On Thu, Aug 31, 2023 at 05:55:54PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:19PM +0200, Erik Skultety wrote:
> > This helper is a shell function transcript of its original GitLab CI
> > counterpart.
> > 
> > Signed-off-by: Erik Skultety 
> > ---
> >  ci/build.sh | 11 +++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/ci/build.sh b/ci/build.sh
> > index 6990f2d171..30f4712e4b 100644
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -94,3 +94,14 @@ run_potfile() {
> >  run_meson_setup
> >  run_build
> >  }
> > +
> > +run_rpmbuild() {
> > +local CMD="rpmbuild \
> > + --clean \
> > + --nodeps \
> > + --define "_without_mingw 1" \
> > + -ta build/meson-dist/libvirt-*.tar.xz"
> > +
> > +run_meson_setup
> 
> Redundant here as implied by run_dist

While I can drop all these redundant setup calls, the reason why I put them
there was simply readability. Let's face it Shell isn't a particularly nice
structured language to look at especially when it comes to functions, so rather
than having everyone go and look what run_dist/run_build, etc. do, I instead
opted for transparent naming and explicit function calls, IOW so that when you
look at run_rpmbuild you immediately know we run meson setup followed by a
project build, etc., but then if you call run_dist alone in a local interactive
environment then run_dist would also call meson_setup because it can't go
without it, so hence the redundancy in all the functions.
If you still feel like it's undesirable even in ^this case, I will drop the
calls.

Erik

Re: [libvirt PATCH 10/33] ci: build.sh: Add a wrapper function over the 'potfile' job

[Erik Skultety]

On Thu, Aug 31, 2023 at 05:55:21PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:18PM +0200, Erik Skultety wrote:
> > This helper is a shell function transcript of its original GitLab CI
> > counterpart. There's one notable difference such that we pass '-j1' to
> > the meson compile command otherwise we'd have to execute the 'run_build'
> > function twice, passing 'libvirt-pot-dep' and 'libvirt-pot' targets
> > in a serial manner.
> > 
> > Signed-off-by: Erik Skultety 
> > ---
> >  ci/build.sh | 12 
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/ci/build.sh b/ci/build.sh
> > index b2891a0c0f..6990f2d171 100644
> > --- a/ci/build.sh
> > +++ b/ci/build.sh
> > @@ -82,3 +82,15 @@ run_codestyle() {
> >  run_build
> >  run_test
> >  }
> > +
> > +run_potfile() {
> > +# since meson would run jobs for each of the following target in 
> > parallel,
> > +# we'd have dependency issues such that one target might depend on a
> > +# generated file which hasn't been generated yet by the other target, 
> > hence
> > +# we limit potfile job to a single build job (luckily potfile build has
> > +# negligible performance impact)
> > +BUILD_ARGS="-j1 libvirt-pot-dep libvirt-pot"
> > +
> > +run_meson_setup
> > +run_build
> 
> run_meson_setup is redundant here too as implied by run_build
> 
> Rather than -j1 could we instead invoke 'run_build' twice eg:
> 
>   BUILD_ARGS=libvirt-pot-dep run_build
>   BUILD_ARGS=libvirt-pot run_build

There was a reason why I did it that way, but I can't remember what it was.
Unless I manage to remember, I'll revert back to what you suggest.

Erik