[libvirt PATCH] vircgroup: fix g_variant_new_parsed format string causing abort

[Pavel Hrdina]

The original code was incorrect and never tested because at the time of
implementing it the cgroup file `io.weight` was not available.

Resolves: https://issues.redhat.com/browse/RHEL-45185
Introduced-by: 9c1693eff427661616ce1bd2795688f87288a412
Signed-off-by: Pavel Hrdina 
---
 src/util/vircgroupv1.c | 2 +-
 src/util/vircgroupv2.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 77c7e209ce..c1f0562249 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1225,7 +1225,7 @@ virCgroupV1SetBlkioDeviceWeight(virCgroup *group,
 if (group->unitName) {
 GVariant *value = NULL;
 
-value = g_variant_new_parsed("[(%s, uint64 %u)]", path, weight);
+value = g_variant_new_parsed("[(%s, %t)]", path, weight);
 
 return virCgroupSetValueDBus(group->unitName, "BlockIODeviceWeight", 
value);
 } else {
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index db115e25f7..eaf5ae98f6 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -867,7 +867,7 @@ virCgroupV2SetBlkioDeviceWeight(virCgroup *group,
 if (group->unitName) {
 GVariant *value = NULL;
 
-value = g_variant_new_parsed("[(%s, uint64 %u)]", path, weight);
+value = g_variant_new_parsed("[(%s, %t)]", path, weight);
 
 return virCgroupSetValueDBus(group->unitName, "IODeviceWeight", value);
 } else {
-- 
2.45.2

Re: [PATCH] gitlab: fix codestyle CI job

[Pavel Hrdina]

On Fri, Jun 14, 2024 at 08:11:53PM +0100, Daniel P. Berrangé wrote:
> Jobs whose names start with a '.' as treated as templates, so
> not actually run in a pipeline.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH] tools: fix paths in PKI validation error messages

[Pavel Hrdina]

On Wed, Jun 12, 2024 at 11:54:54AM +0100, Daniel P. Berrangé wrote:
> A couple of paths passed in the error messages, didnt match the paths
> that were actually being tested.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  tools/virt-pki-validate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH] rpm: ensure -Werror is disabled for mingw builds

[Pavel Hrdina]

On Wed, Jun 05, 2024 at 01:47:15PM +0100, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé 
> ---
>  libvirt.spec.in | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 0f3c882f05..1d3240ee6f 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1485,8 +1485,9 @@ export SOURCE_DATE_EPOCH=$(stat --printf='%Y' 
> %{_specdir}/libvirt.spec)
>-Dtests=disabled \
>-Dudev=disabled \
>-Dwireshark_dissector=disabled \
> -  -Dyajl=disabled
> -%mingw_ninja
> +  -Dyajl=disabled \
> +  %{?enable_werror}
> +%mingw_ninja
>  %endif

I would note in commit message that this copies the behavior of
non-mingw build and it is disabled for non RHEL distro.

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH 1/3] meson: Improve default firewall backend configuration

[Pavel Hrdina]

On Tue, May 28, 2024 at 09:59:17AM -0700, Andrea Bolognani wrote:
> On Tue, May 28, 2024 at 12:50:51PM GMT, Laine Stump wrote:
> > On 5/28/24 12:31 PM, Pavel Hrdina wrote:
> > > On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
> > > > +  if (not firewall_backend_priority.contains('nftables') or
> > > > +  not firewall_backend_priority.contains('iptables') or
> > > > +  firewall_backend_priority.length() != 2)
> > >
> > > No need to have a check for specific values. Meson will already check if
> > > they are from the list of choices defined in meson_options.txt .
> >
> > But we don't just need to check that the values in the list are all valid
> > options; we also want to make sure that nobody has entered the same value
> > multiple times (which this ends up doing by making sure that there is at
> > least one entry for each valid value, and that the list is the same length
> > as the number of valid values).
> 
> Yes, that was exactly the idea.

True, that is not checked so we still need to duplicate the list here
that I wanted to avoid.

> > Or do we not care if someone repeats the same value? Maybe somebody wants to
> > include iptables support in the build, but not look for it automatically
> > (instead only use it if it's explicitly requested in network.conf). One way
> > of doing that would be to sent firewall_backend_priority = nftables,nftables
> >
> > (that does seem a bit obtuse; perhaps it would be better to allow limiting
> > the length of the option list to 1)
> 
> If that's something that we want to allow, then we should include
> explicit support for it rather than make it possible through obscure
> runes :)
> 
> I'm not sure we really need to bother, but I don't feel strongly
> either way so I could be persuaded to look into it. Perhaps as an
> after-release follow up, though?
> 
> > > > +option('firewall_backend_priority', type: 'array', choices: 
> > > > ['nftables', 'iptables'], description: 'firewall backends to try, 
> > > > preferred ones first')
> > >
> > > What about "order of firewall backends to try"? The part "preferred ones
> > > first" sounds strange to me.
> 
> Sure, that works too.

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH 3/3] rpm: Configure firewall backends explicitly

[Pavel Hrdina]

On Tue, May 28, 2024 at 05:49:21PM +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  libvirt.spec.in | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH 2/3] meson: Include firewall backend selection in summary

[Pavel Hrdina]

On Tue, May 28, 2024 at 05:49:20PM +0200, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  meson.build | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH 1/3] meson: Improve default firewall backend configuration

[Pavel Hrdina]

On Tue, May 28, 2024 at 05:49:19PM +0200, Andrea Bolognani wrote:
> The current implementation requires users to configure the
> preference as such:
> 
>   -Dfirewall_backend_default_1=iptables
>   -Dfirewall_backend_default_2=nftables
> 
> In addition to being more verbose than one would hope, there
> are several things that could go wrong.
> 
> First of all, meson performs no validation on the provided
> values, so mistakes will only be caught by the compiler.
> Additionally, it's entirely possible to provide nonsensical
> combinations, such as repeating the same value twice.
> 
> Change things so that the preference can now be configured
> as such:
> 
>   -Dfirewall_backend_priority=iptables,nftables
> 
> Checks have been added to prevent invalid values from being
> accepted.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  meson.build  | 16 +---
>  meson_options.txt|  3 +--
>  src/network/bridge_driver_conf.c |  6 +-
>  src/network/meson.build  |  6 --
>  src/network/network.conf.in  | 13 +++--
>  5 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index f67c3d2724..ed0e9686f8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1635,15 +1635,17 @@ endif
>  
>  if not get_option('driver_network').disabled() and conf.has('WITH_LIBVIRTD')
>conf.set('WITH_NETWORK', 1)
> -  firewall_backend_default_1 = get_option('firewall_backend_default_1')
> -  firewall_backend_default_conf = firewall_backend_default_1
> -  firewall_backend_default_1 = 'VIR_FIREWALL_BACKEND_' + 
> firewall_backend_default_1.to_upper()
> -  conf.set('FIREWALL_BACKEND_DEFAULT_1', firewall_backend_default_1)
>  
> -  firewall_backend_default_2 = get_option('firewall_backend_default_2')
> -  firewall_backend_default_2 = 'VIR_FIREWALL_BACKEND_' + 
> firewall_backend_default_2.to_upper()
> -  conf.set('FIREWALL_BACKEND_DEFAULT_2', firewall_backend_default_2)
> +  firewall_backend_priority = get_option('firewall_backend_priority')
> +  if (not firewall_backend_priority.contains('nftables') or
> +  not firewall_backend_priority.contains('iptables') or
> +  firewall_backend_priority.length() != 2)

No need to have a check for specific values. Meson will already check if
they are from the list of choices defined in meson_options.txt .

> +error('invalid value for firewall_backend_priority option')
> +  endif
>  
> +  conf.set('FIREWALL_BACKEND_PRIORITY_0', 'VIR_FIREWALL_BACKEND_' + 
> firewall_backend_priority[0].to_upper())
> +  conf.set('FIREWALL_BACKEND_PRIORITY_1', 'VIR_FIREWALL_BACKEND_' + 
> firewall_backend_priority[1].to_upper())
> +  conf.set('FIREWALL_BACKEND_PRIORITY_NUM', 
> firewall_backend_priority.length())
>  elif get_option('driver_network').enabled()
>error('libvirtd must be enabled to build the network driver')
>  endif
> diff --git a/meson_options.txt b/meson_options.txt
> index ad354a8668..8723d13231 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -115,8 +115,7 @@ option('dtrace', type: 'feature', value: 'auto', 
> description: 'use dtrace for st
>  option('firewalld', type: 'feature', value: 'auto', description: 'firewalld 
> support')
>  # dep:firewalld
>  option('firewalld_zone', type: 'feature', value: 'auto', description: 
> 'whether to install firewalld libvirt zone')
> -option('firewall_backend_default_1', type: 'string', value: 'nftables', 
> description: 'first firewall backend to try  when none is specified')
> -option('firewall_backend_default_2', type: 'string', value: 'iptables', 
> description: 'second firewall backend to try when none is specified (and 
> first is unavailable)')
> +option('firewall_backend_priority', type: 'array', choices: ['nftables', 
> 'iptables'], description: 'firewall backends to try, preferred ones first')

What about "order of firewall backends to try"? The part "preferred ones
first" sounds strange to me.

Otherwise looks good.

Pavel


signature.asc
Description: PGP signature

[libvirt PATCH] qemu_snapshot: fix memory leak when reverting external snapshot

[Pavel Hrdina]

The code cleaning up virStorageSource doesn't free data allocated by
virStorageSourceInit() so we need to call virStorageSourceDeinit()
explicitly.

Fixes: 8e664737813378d2a1bdeacc2ca8e942327e2cab
Resolves: https://issues.redhat.com/browse/RHEL-33044
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 09ec959f10..f5260c4a22 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2260,6 +2260,8 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm,
  snapdisk->src->path);
 }
 
+virStorageSourceDeinit(snapdisk->src);
+
 virDomainSnapshotDiskDefClear(snapdisk);
 }
 
@@ -2277,6 +2279,8 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm,
 VIR_WARN("Failed to remove snapshot image '%s'",
  snapdisk->src->path);
 }
+
+virStorageSourceDeinit(snapdisk->src);
 }
 }
 
-- 
2.45.1

Re: [PATCH v2 0/4] qemu: Substract isolcpus from all online affinity

[Pavel Hrdina]

On Tue, Apr 23, 2024 at 04:16:20PM +0200, Michal Privoznik wrote:
> v2 of:
> 
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/4V7OI5AEGYRN4GFQMQPIN4MYPJNK3NYJ/
> 
> diff to v1:
> - Don't error out on systems where /sys/devices/system/cpu/isolated is
>   unavailable.
> - Don't error out on systems where /sys/devices/system/cpu/isolated is
>   empty.
> 
> Both of these resulted in new patches.
> 
> Michal Prívozník (4):
>   virbitmap: Introduce virBitmapParseUnlimitedAllowEmpty()
>   virfile: Introduce virFileReadValueBitmapAllowEmpty()
>   virhostcpu: Introduce virHostCPUGetIsolated()
>   qemu: Substract isolcpus from all online affinity

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH v2 3/4] virhostcpu: Introduce virHostCPUGetIsolated()

[Pavel Hrdina]

On Tue, Apr 23, 2024 at 04:16:23PM +0200, Michal Privoznik wrote:
> This is a helper that parses /sys/devices/system/cpu/isolated
> into a virBitmap. It's going to be needed soon.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virhostcpu.c| 31 +++
>  src/util/virhostcpu.h|  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2c7e4b45d3..0f2d5db883 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2504,6 +2504,7 @@ virHostCPUGetCount;
>  virHostCPUGetCPUID;
>  virHostCPUGetHaltPollTime;
>  virHostCPUGetInfo;
> +virHostCPUGetIsolated;
>  virHostCPUGetKVMMaxVCPUs;
>  virHostCPUGetMap;
>  virHostCPUGetMicrocodeVersion;
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 01de69c0d1..b6d1db2302 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -1152,6 +1152,37 @@ virHostCPUGetAvailableCPUsBitmap(void)
>  }
>  
>  
> +/**
> + * virHostCPUGetIsolated:
> + * @isolated: returned bitmap of isolated CPUs
> + *
> + * Sets @isolated to point to a bitmap of isolated CPUs (e.g. those passed to
> + * isolcpus= kernel cmdline). If the file doesn't exist, @isolated is set to
> + * NULL and success is returned. If the file does exist but it's empty,
> + * @isolated is set to an empty bitmap an success is returned.

s/an success/and success/

> + *
> + * Returns: 0 on success,
> + * -1 otherwise (with error reported).
> + */
> +int
> +virHostCPUGetIsolated(virBitmap **isolated)
> +{
> +g_autoptr(virBitmap) bitmap = NULL;
> +int rc;
> +
> +rc = virFileReadValueBitmapAllowEmpty(, "%s/cpu/isolated", 
> SYSFS_SYSTEM_PATH);
> +if (rc == -2) {
> +*isolated = NULL;
> +return 0;
> +} else if (rc < 0) {
> +return -1;
> +}
> +
> +*isolated = g_steal_pointer();
> +return 0;
> +}


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 0/2] tests: Fix 'qemucapsprobe' and update caps for qemu-9.0 release

[Pavel Hrdina]

On Wed, Apr 24, 2024 at 10:27:22AM +0200, Peter Krempa wrote:
> Peter Krempa (2):
>   tests: qemucapsprobe: Fix construction of path to
> libqemucapsprobemock.so
>   qemucapabilitiestest: Update qemu capability dump for qemu-9.0 release
> 
>  .../caps_9.0.0_x86_64.replies | 50 ++-
>  .../caps_9.0.0_x86_64.xml |  4 +-
>  tests/qemucapsprobe.c |  2 +-
>  3 files changed, 7 insertions(+), 49 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH] vbox: Drop needless g_new0(..., 0) in vbox_snapshot_conf.c

[Pavel Hrdina]

On Fri, Apr 12, 2024 at 08:52:37PM +0200, Michal Privoznik wrote:
> clang on Fedora started to complain about some calls to g_new0()
> we're making in vbox_snapshot_conf.c. Specifically, we're passing
> zero as number of elements to allocate. And while usually SA
> tools are not clever, in this specific case clang is right.
> There are three cases where such call is made, but all of them
> later use VIR_EXPAND_N() to allocate more memory (if needed). But
> VIR_EXPAND_N() accepts a variable set to NULL happily.
> 
> Therefore, just drop those three calls to g_new0(..., 0) and let
> VIR_EXPAND_N() allocate memory.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/vbox/vbox_snapshot_conf.c | 6 --
>  1 file changed, 6 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH] qemucapabilitiestest: Update qemu-9.0 capabilities for RC1

[Pavel Hrdina]

On Tue, Apr 02, 2024 at 03:34:42PM +0200, Peter Krempa wrote:
> Update the capabilities to v9.0.0-rc1-55-g7fcf7575f3
> 
> Notable changes:
>  - Q35 machine now supports 4096 cpus
> 
>  - 'kvm-asyncpf-vmexit' cpu feature added
>  - 'x2apic' cpu feature is now migratable
> 
>  - LUKS detached header support added
>  - LUKS sm4 cipher alg support added
> 
>  - 'console' chardev backend type removed
>  - 'memory' chardev backend type deprecated
> 
>  - 'mapped-ram' migration capability added
>  - 'zero-page-detection' migration parameter added
> 
>  - 'acpi-generic-initiator' 'object' added
> 
>  - 'request-ebpf' QMP command added
> 
>  - 'legacy-reset', 'resettable-container', 'vhost-user-snd*' QOM types
>added
> 
>  - 'vdpa' property added for following device models:
> - virtio-balloon-pci
> - virtio-blk-pci
> - virtio-gpu-pci
> - virtio-iommu-pci
> - virtio-mem-pci
> - virtio-net-pci
> - virtio-scsi-pci
> 
>  - 'win2k-install-hack' property of 'ide-hd' added
>  - 'aw-bits', 'granule', properties of 'virtio-iommu-pci' added
>  - 'ebpf-rss-fds' property of 'virtio-net-pci' added
> 
> Signed-off-by: Peter Krempa 
> ---
>  .../domaincapsdata/qemu_9.0.0-q35.x86_64.xml  |2 +-
>  .../domaincapsdata/qemu_9.0.0-tcg.x86_64.xml  |1 +
>  .../caps_9.0.0_x86_64.replies | 4240 +
>  .../caps_9.0.0_x86_64.xml |   89 +-
>  ...host-model-fallback-tcg.x86_64-latest.args |2 +-
>  ...st-model-nofallback-tcg.x86_64-latest.args |2 +-
>  .../cpu-host-model-tcg.x86_64-latest.args |2 +-
>  7 files changed, 2212 insertions(+), 2126 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 1/1] gitignore: add cscope files to git ignore

[Pavel Hrdina]

On Mon, Mar 18, 2024 at 01:31:27PM +0100, Peter Krempa wrote:
> On Mon, Mar 18, 2024 at 13:14:54 +0100, Michal Prívozník wrote:
> > On 3/18/24 12:45, Denis V. Lunev wrote:
> > > On 3/18/24 11:42, Michal Prívozník wrote:
> > >> On 3/17/24 16:00, Denis V. Lunev wrote:
> > >>> Signed-off-by: Denis V. Lunev 
> > >>> ---
> > >>>   .gitignore | 1 +
> > >>>   1 file changed, 1 insertion(+)
> > >>>
> > >>> diff --git a/.gitignore b/.gitignore
> > >>> index 4695391342..44a9b446bd 100644
> > >>> --- a/.gitignore
> > >>> +++ b/.gitignore
> > >>> @@ -20,6 +20,7 @@ __pycache__/
> > >>>   /build/
> > >>>   /ci/scratch/
> > >>>   tags
> > >>> +cscope.*
> > >>>     # clangd related ignores
> > >>>   .clangd
> > >> Apparently, at some point in time this was here, but it was removed:
> > >>
> > >> https://gitlab.com/libvirt/libvirt/-/commit/8a63add283c310952b7df161b4b413e817468d01
> > >>
> > >> Michal
> > >>
> > > This is quite strange for me:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/.gitignore
> > > https://gitlab.com/qemu-project/qemu/-/blob/master/.gitignore?ref_type=heads
> > > 
> > > I do not see any obvious reason how big and extensive
> > > list of files in .gitignore could hurt us while it
> > > is obviously convenient.
> > 
> > Yeah, it feels a bit selective. I mean, we allow vim swap files to be in
> > .gitignore, we allow tags file to be there too (which strictly speaking
> > is a tooling helper file), but not cscope?
> 
> The reasoning to keep 'tags' ignored is because we ship .ctags, but
> generally tooling files should be ignored by "personal" .gitignores as
> we can't cover all of it especially if we don't configure it.

Agreed, I see we have ignores for vim and emacs, but honestly these
could be removed as well because everyone can just edit
`.config/git/ignore` and put the files created by their favorite editor
into that file and it will work for every project you participate in.

Pavel

> I guess the reasoning for editor temp files is to prevent mistakes as
> those are basically to be present for everyone trying to edit the
> project.
> ___
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-le...@lists.libvirt.org


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 2/2] tests: mock __open_2()

[Pavel Hrdina]

On Thu, Mar 14, 2024 at 02:42:13PM +0100, Michal Prívozník wrote:
> On 3/14/24 13:10, Pavel Hrdina wrote:
> > On Thu, Mar 14, 2024 at 10:34:28AM +0100, Michal Privoznik wrote:
> >> As of commit [1] glibc may overwrite a call to open() with call
> >> to __open_2() (if only two arguments are provided and the code is
> >> compiled with clang). But since we are not mocking the latter our
> >> test suite is broken as tests try to access paths outside of our
> >> repo.
> >>
> >> 1: 
> >> https://sourceware.org/git/?p=glibc.git;a=commit;h=86889e22db329abac618c6a41f86c84657a15324
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>  meson.build  | 13 +
> >>  tests/nssmock.c  | 26 +
> >>  tests/qemusecuritymock.c | 24 +++
> >>  tests/vircgroupmock.c| 42 
> >>  tests/virfilewrapper.c   | 18 +
> >>  tests/virmock.h  |  4 
> >>  tests/virpcimock.c   | 17 ++--
> >>  tests/virtestmock.c  | 20 +++
> >>  tests/virusbmock.c   | 22 +
> >>  9 files changed, 175 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index 9842886bbb..b1b55b0d25 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -604,6 +604,12 @@ stat_functions = [
> >>  
> >>  functions += stat_functions
> >>  
> >> +open_functions = [
> >> +  '__open_2',
> >> +]
> >> +
> >> +functions += open_functions
> >> +
> >>  foreach function : functions
> >>if cc.has_function(function)
> >>  conf.set('WITH_@0@'.format(function.to_upper()), 1)
> >> @@ -618,6 +624,13 @@ foreach function : stat_functions
> >>  endforeach
> >>  
> >>  
> >> +foreach function : open_functions
> >> +  if cc.has_header_symbol('fcntl.h', function)
> >> +conf.set('WITH_@0@_DECL'.format(function.to_upper()), 1)
> >> +  endif
> >> +endforeach
> >> +
> >> +
> >>  # various header checks
> >>  
> >>  headers = [
> > 
> > [...]
> > 
> >> diff --git a/tests/virmock.h b/tests/virmock.h
> >> index 300ba17174..178d0a15f0 100644
> >> --- a/tests/virmock.h
> >> +++ b/tests/virmock.h
> >> @@ -27,6 +27,10 @@
> >>  
> >>  #include "internal.h"
> >>  
> >> +#ifndef WITH___OPEN_2_DECL
> >> +int __open_2 (const char *__path, int __oflag);
> >> +#endif
> > 
> > Do we really need this bit? Every single use of __open_2 is already
> > guarded by WITH__OPEN_2.
> 
> We do. We are not guaranteed that the conditions line up right
> everywhere for fcntl.h header file to provide that declaration. Yet, the
> symbol is available in the libc.so. For instance, ubuntu 22.04 with
> clang fails:
> 
> https://gitlab.com/MichalPrivoznik/libvirt/-/jobs/6394162037
> 
> Maybe we can switch all those WITH___OPEN_2 to WITH___OPEN_2_DECL? IOW -
> provide the mock only if conditions align just right for the declaration
> to exist. Let me see what our pipeline would think about that.
> 
> Michal

Ah I see, so the header file and actual library are not in sync on all
OSes. That's stupid but nothing we can do about.

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 2/2] tests: mock __open_2()

[Pavel Hrdina]

On Thu, Mar 14, 2024 at 10:34:28AM +0100, Michal Privoznik wrote:
> As of commit [1] glibc may overwrite a call to open() with call
> to __open_2() (if only two arguments are provided and the code is
> compiled with clang). But since we are not mocking the latter our
> test suite is broken as tests try to access paths outside of our
> repo.
> 
> 1: 
> https://sourceware.org/git/?p=glibc.git;a=commit;h=86889e22db329abac618c6a41f86c84657a15324
> Signed-off-by: Michal Privoznik 
> ---
>  meson.build  | 13 +
>  tests/nssmock.c  | 26 +
>  tests/qemusecuritymock.c | 24 +++
>  tests/vircgroupmock.c| 42 
>  tests/virfilewrapper.c   | 18 +
>  tests/virmock.h  |  4 
>  tests/virpcimock.c   | 17 ++--
>  tests/virtestmock.c  | 20 +++
>  tests/virusbmock.c   | 22 +
>  9 files changed, 175 insertions(+), 11 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 9842886bbb..b1b55b0d25 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -604,6 +604,12 @@ stat_functions = [
>  
>  functions += stat_functions
>  
> +open_functions = [
> +  '__open_2',
> +]
> +
> +functions += open_functions
> +
>  foreach function : functions
>if cc.has_function(function)
>  conf.set('WITH_@0@'.format(function.to_upper()), 1)
> @@ -618,6 +624,13 @@ foreach function : stat_functions
>  endforeach
>  
>  
> +foreach function : open_functions
> +  if cc.has_header_symbol('fcntl.h', function)
> +conf.set('WITH_@0@_DECL'.format(function.to_upper()), 1)
> +  endif
> +endforeach
> +
> +
>  # various header checks
>  
>  headers = [

[...]

> diff --git a/tests/virmock.h b/tests/virmock.h
> index 300ba17174..178d0a15f0 100644
> --- a/tests/virmock.h
> +++ b/tests/virmock.h
> @@ -27,6 +27,10 @@
>  
>  #include "internal.h"
>  
> +#ifndef WITH___OPEN_2_DECL
> +int __open_2 (const char *__path, int __oflag);
> +#endif

Do we really need this bit? Every single use of __open_2 is already
guarded by WITH__OPEN_2.

Pavel

>  #define VIR_MOCK_COUNT_ARGS(...) VIR_MOCK_ARG27(__VA_ARGS__, 26, 25, 24, 23, 
> 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1)
>  #define VIR_MOCK_ARG27(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, 
> _13, _14, _15, _16, _17, _18, _19, _20, _21, _22, _23, _24, _25, _26, _27, 
> ...) _27
>  #define VIR_MOCK_ARG_PASTE(a, b, ...) a##b(__VA_ARGS__)


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 1/2] virusbmock: Switch to VIR_MOCK_REAL_INIT()

[Pavel Hrdina]

On Thu, Mar 14, 2024 at 10:34:27AM +0100, Michal Privoznik wrote:
> Since virusbmock was written 10 years ago, back when we didn't
> have virmock.h and its helpers, it open codes symbol resolution
> (VIR_MOCK_REAL_INIT). With a bit of cleanup (e.g. renaming
> realopen to real_open and so on) it can use virmock.h provided
> macros.
> 
> And while at it, drop include of virusb.h - there is no
> compelling reason for it include the file. The mock just
> redirects paths passed to open()/opendir().
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/virusbmock.c | 20 
>  1 file changed, 8 insertions(+), 12 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH] NEWS: Update for release

[Pavel Hrdina]

On Fri, Mar 01, 2024 at 09:45:30AM +0100, Peter Krempa wrote:
> Mention improvement of virt-admin, and fixes for the VPD xml and disk
> migration port bug.
> 
> Signed-off-by: Peter Krempa 
> ---
>  NEWS.rst | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/NEWS.rst b/NEWS.rst
> index d270b2397d..4a71c7d3f0 100644
> --- a/NEWS.rst
> +++ b/NEWS.rst
> @@ -52,7 +52,7 @@ v10.1.0 (unreleased)
> 
>  * **Improvements**
> 
> -* nodedev: Add ability to update persistent mediated devices by defining them
> +  * nodedev: Add ability to update persistent mediated devices by defining 
> them
> 
>  Existing persistent mediated devices can now also be updated by
>  ``virNodeDeviceDefineXML()`` as long as parent and UUID remain unchanged.
> @@ -66,6 +66,12 @@ v10.1.0 (unreleased)
>  Secrets with  were left unable to be checked for 
> in
>  the access driver, i.e. in ACL rules. Missing code was provided.
> 
> +  * virt-admin: Notify users to use explicit URI if connection fails
> +
> +``virt-admin`` doesn't try to guess the URI of the daemon to manage so a
> +failure to connect may be confusing for users if modular daemons are 
> used.
> +Add a hint to use the URI of te dameon to manage.

s/te dameon/the daemon/

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 0/9] Fix broken HTML in API docs

[Pavel Hrdina]

On Thu, Feb 29, 2024 at 03:58:14PM +0100, Peter Krempa wrote:
> While checking API docs after recent migration to gitlab-pages I've
> noticed that the footer is not properly rendered.
> 
> A deeper dig showed that the issue is that empty  elements, while
> formatted by the XML output version of XSLT conversion is turned into
> the non-pair empty variant. This contradicts the HTML standard which we
> declare to use for our pages and thus is mis-parsed into dom, where
> every empty div is treated as a start of a div, thus nesting everything.
> 
> This series fixes the above and also many other errors pointed out by
> the HTML validator at:
> 
> https://validator.w3.org/
> 
> Compare:
> 
>  https://libvirt.org/html/libvirt-libvirt-domain.html
> 
>  vs
> 
>  
> https://pipo.sk.gitlab.io/-/libvirt/-/jobs/6288946946/artifacts/website/html/libvirt-libvirt-domain.html
> 
> and the validation:
> 
>   
> https://validator.w3.org/nu/?doc=https%3A%2F%2Flibvirt.org%2Fhtml%2Flibvirt-libvirt-domain.html
> 
>   vs
> 
>   
> https://validator.w3.org/nu/?doc=https%3A%2F%2Fpipo.sk.gitlab.io%2F-%2Flibvirt%2F-%2Fjobs%2F6288946946%2Fartifacts%2Fwebsite%2Fhtml%2Flibvirt-libvirt-domain.html
> 
> Also non-API pages validation:
> 
>  https://validator.w3.org/nu/?doc=https%3A%2F%2Flibvirt.org%2Fdocs.html
> 
>  vs
> 
>  
> https://validator.w3.org/nu/?doc=https%3A%2F%2Fpipo.sk.gitlab.io%2F-%2Flibvirt%2F-%2Fjobs%2F6288946946%2Fartifacts%2Fwebsite%2Fdocs.html
> 
> Peter Krempa (9):
>   docs: site: Don't generate '   docs: page: Add 'lang="en"' for all HTML output documents
>   docs: page: Fix declaration of main javascript source
>   docs: index: Fix import of blog planet javascript
>   docs: newapi: Don't generate empty  in template for ACL
> permissions
>   docs: newapi: Avoid empty s when there is no description
>   docs: newapi: Avoid table where every row has an cell with 'colspan'
>   docs: newapi: Properly skip ACL entries if empty
>   docs: newapi: Fix generation of type definition tables
> 
>  docs/index.rst  |   2 +-
>  docs/newapi.xsl | 158 ++++++--
>  docs/page.xsl   |   7 ++-
>  docs/site.xsl   |   5 +-
>  4 files changed, 93 insertions(+), 79 deletions(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH 2/2] qemu_snapshot: correctly update metadata when deleting external snapshot with multiple branches

[Pavel Hrdina]

XML metadata for snapshot contains only single list of disk overlays
from the moment when the snapshot was taken. When user creates multiple
branches of snapshots the parent snapshot will still list only the
original disk overlays. This may cause an issue in a specific scenario:

 s1
  |
  +- s2
  +- s3 (active)

For this snapshot topology when we delete s2 metadata for s1 are not
updated. Now when we delete s1 the code operated with incorrect
overlays from s1 metadata in order to update s3 metadata resulting in no
changes to s3 metadata.

Now when user tries to delete s3 it fails with following error:

error: Failed to delete snapshot s3
error: operation failed: snapshot VM disk source and parent disk source are 
not the same

For the actual deletion there is a code to figure out the correct disk
source but it was not used to update metadata as well. Due to reasons
how block commit in libvirt works we need to create a copy of that disk
source in order to have it available when updating metadata as the
original source will be freed at that point.

Resolves: https://issues.redhat.com/browse/RHEL-26276
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 34 +-
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index be089a31db..09ec959f10 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2639,6 +2639,8 @@ typedef struct _qemuSnapshotDeleteExternalData {
 virDomainSnapshotDiskDef *snapDisk; /* snapshot disk definition */
 virDomainDiskDef *domDisk; /* VM disk definition */
 virStorageSource *diskSrc; /* source of disk we are deleting */
+virStorageSource *diskSrcMetadata; /* copy of diskSrc to be used when 
updating
+  metadata because diskSrc is freed */
 virDomainMomentObj *parentSnap;
 virDomainDiskDef *parentDomDisk; /* disk definition from snapshot metadata 
*/
 virStorageSource *parentDiskSrc; /* backing disk source of the @diskSrc */
@@ -2657,6 +2659,7 @@ 
qemuSnapshotDeleteExternalDataFree(qemuSnapshotDeleteExternalData *data)
 if (!data)
 return;
 
+virObjectUnref(data->diskSrcMetadata);
 virObjectUnref(data->job);
 g_slist_free_full(data->disksWithBacking, g_free);
 
@@ -2893,6 +2896,8 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
 if (!data->diskSrc)
 return -1;
 
+data->diskSrcMetadata = virStorageSourceCopy(data->diskSrc, 
false);
+
 if (!virStorageSourceIsSameLocation(data->diskSrc, 
snapDiskSrc)) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("VM disk source and snapshot disk source 
are not the same"));
@@ -3049,6 +3054,7 @@ typedef struct _qemuSnapshotUpdateDisksData 
qemuSnapshotUpdateDisksData;
 struct _qemuSnapshotUpdateDisksData {
 virDomainMomentObj *snap;
 virDomainObj *vm;
+GSList *externalData;
 int error;
 };
 
@@ -3078,7 +3084,8 @@ static int
 qemuSnapshotUpdateDisksSingle(virDomainMomentObj *snap,
   virDomainDef *def,
   virDomainDef *parentDef,
-  virDomainSnapshotDiskDef *snapDisk)
+  virDomainSnapshotDiskDef *snapDisk,
+  virStorageSource *diskSrc)
 {
 virDomainDiskDef *disk = NULL;
 
@@ -3091,7 +3098,7 @@ qemuSnapshotUpdateDisksSingle(virDomainMomentObj *snap,
 if (!(parentDisk = qemuDomainDiskByName(parentDef, snapDisk->name)))
 return -1;
 
-if (virStorageSourceIsSameLocation(snapDisk->src, disk->src)) {
+if (virStorageSourceIsSameLocation(diskSrc, disk->src)) {
 virObjectUnref(disk->src);
 disk->src = virStorageSourceCopy(parentDisk->src, false);
 }
@@ -3102,7 +3109,7 @@ qemuSnapshotUpdateDisksSingle(virDomainMomentObj *snap,
 virStorageSource *next = disk->src->backingStore;
 
 while (next) {
-if (virStorageSourceIsSameLocation(snapDisk->src, next)) {
+if (virStorageSourceIsSameLocation(diskSrc, next)) {
 cur->backingStore = next->backingStore;
 next->backingStore = NULL;
 virObjectUnref(next);
@@ -3128,17 +3135,15 @@ qemuSnapshotDeleteUpdateDisks(void *payload,
 qemuDomainObjPrivate *priv = data->vm->privateData;
 virQEMUDriver *driver = priv->driver;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(data->snap);
-ssize_t i;
+GSList *cur = NULL;
 
-for (i = 0; i < snapdef->ndisks; i++) {
-virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
-
- 

[libvirt PATCH 1/2] qemu_snapshot: call qemuSnapshotDeleteUpdateDisks only for external snapshots

[Pavel Hrdina]

Calling this function when deleting internal snapshot isn't required
because with internal snapshots all changes are done within the file
itself so there is no file deletion and no need to update snapshot
metadata.

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 0cac0c4146..be089a31db 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -3537,14 +3537,16 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm,
 if (rep.err < 0)
 ret = -1;
 
-data.snap = snap;
-data.vm = vm;
-data.error = 0;
-virDomainMomentForEachDescendant(snap,
- qemuSnapshotDeleteUpdateDisks,
- );
-if (data.error < 0)
-ret = -1;
+if (virDomainSnapshotIsExternal(snap)) {
+data.snap = snap;
+data.vm = vm;
+data.error = 0;
+virDomainMomentForEachDescendant(snap,
+ qemuSnapshotDeleteUpdateDisks,
+ );
+if (data.error < 0)
+ret = -1;
+}
 
 virDomainMomentMoveChildren(snap, snap->parent);
 }
-- 
2.43.2
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH 0/2] correctly update metadata when deleting external snapshot

[Pavel Hrdina]

Pavel Hrdina (2):
  qemu_snapshot: call qemuSnapshotDeleteUpdateDisks only for external
snapshots
  qemu_snapshot: correctly update metadata when deleting external
snapshot with multiple branches

 src/qemu/qemu_snapshot.c | 52 
 1 file changed, 31 insertions(+), 21 deletions(-)

-- 
2.43.2
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PUSHED PATCH] spec: Explicitly disable userfaultfd_sysctl for mingw

[Pavel Hrdina]

On Wed, Feb 14, 2024 at 01:31:29PM +0100, Peter Krempa wrote:
> On Tue, Feb 13, 2024 at 19:16:00 +0100, Jiri Denemark wrote:
> > The %meson* macros pass --auto-features=enabled to enable all "auto"
> > features, which means we have to explicitly disable them.
> 
> This is not the first time we've hit this. I really think the only sane
> option for --auto-features= for distributions is to use 'disabled' and
> explicitly enable what they need.

This is our own fault as Jano already pointed out here [1] we have the
option '--auto-features=enabled' in our own spec file for mingw [2].

Pavel

[1] 

[2] 



signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH] qemu_snapshot: allow reverting to external disk only snapshot

[Pavel Hrdina]

When snapshot is created with disk-only flag it is always external
snapshot without memory state. Historically when there was not support
to revert external snapshots this produced error message.

error: Failed to revert snapshot s1
error: internal error: Invalid target domain state 'disk-snapshot'. 
Refusing snapshot reversion

Now we can simply consider this as reverting to offline snapshot as the
possible damage to file system is already done at the point of snapshot
creation.

Resolves: https://issues.redhat.com/browse/RHEL-21549
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 0cac0c4146..7964f70553 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2606,6 +2606,7 @@ qemuSnapshotRevert(virDomainObj *vm,
 case VIR_DOMAIN_SNAPSHOT_SHUTDOWN:
 case VIR_DOMAIN_SNAPSHOT_SHUTOFF:
 case VIR_DOMAIN_SNAPSHOT_CRASHED:
+case VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT:
 ret = qemuSnapshotRevertInactive(vm, snapshot, snap,
  driver, cfg,
  ,
@@ -2617,8 +2618,6 @@ qemuSnapshotRevert(virDomainObj *vm,
_("qemu doesn't support reversion of snapshot taken in 
PMSUSPENDED state"));
 goto endjob;
 
-case VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT:
-/* Rejected earlier as an external snapshot */
 case VIR_DOMAIN_SNAPSHOT_NOSTATE:
 case VIR_DOMAIN_SNAPSHOT_BLOCKED:
 case VIR_DOMAIN_SNAPSHOT_LAST:
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH] qemu_snapshot: fix detection if non-leaf snapshot isn't in active chain

[Pavel Hrdina]

The condition was completely wrong. As per the comment for function
virDomainMomentIsAncestor() it checks that the first argument is
descendant of the second argument.

Consider the following snapshot tree for VM:

  s1
|
+- s2
|   |
|   +- s3
|
+- s4
|
+- s5 (current)

When deleting s2 with the original code we checked if
virDomainMomentIsAncestor(s2, s5) which would return false basically for
any snapshot as s5 is leaf snapshot so no children.

When deleting s2 with fixed code we check if
virDomainMomentIsAncestor(s5, s2) which still returns false but when
deleting s4 it will correctly return true.

Resolves: https://issues.redhat.com/browse/RHEL-23212
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 73ff533827..af5f995b0d 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -3815,7 +3815,7 @@ qemuSnapshotDeleteValidate(virDomainObj *vm,
 }
 
 if (snap != current && snap->nchildren != 0 &&
-virDomainMomentIsAncestor(snap, current)) {
+!virDomainMomentIsAncestor(current, snap)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("deletion of non-leaf external snapshot that is 
not in active chain is not supported"));
 return -1;
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH 2/2] qemu_snapshot: create: don't require disk-only flag for offline external snapshot

[Pavel Hrdina]

Historically creating offline external snapshot required disk-only flag
as well. Now when user requests new snapshot for offline VM and at least
one disk is specified to use external snapshot we will no longer require
disk-only flag as all other not specified disk will use external
snapshots as well.

Resolves: https://issues.redhat.com/browse/RHEL-22797
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_snapshot.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index ab7b47319b..d089f70d4e 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1577,15 +1577,25 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm,
 
 
 static bool
-qemuSnapshotCreateUseExternal(virDomainSnapshotDef *def,
+qemuSnapshotCreateUseExternal(virDomainObj *vm,
+  virDomainSnapshotDef *def,
   unsigned int flags)
 {
+ssize_t i;
+
 if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)
 return true;
 
 if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
 return true;
 
+if (!virDomainObjIsActive(vm)) {
+for (i = 0; i < def->ndisks; i++) {
+if (def->disks[i].snapshot == 
VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+return true;
+}
+}
+
 return false;
 }
 
@@ -1618,7 +1628,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm,
 return -1;
 }
 
-if (qemuSnapshotCreateUseExternal(def, flags)) {
+if (qemuSnapshotCreateUseExternal(vm, def, flags)) {
 align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
 def->state = virDomainObjGetState(vm, NULL);
 
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH 1/2] qemu_snapshot: create: refactor external snapshot detection

[Pavel Hrdina]

Introduce new function qemuSnapshotCreateUseExternal() that will return
true if we will use external snapshots as default location.

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 73ff533827..ab7b47319b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1576,6 +1576,20 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm,
 }
 
 
+static bool
+qemuSnapshotCreateUseExternal(virDomainSnapshotDef *def,
+  unsigned int flags)
+{
+if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY)
+return true;
+
+if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+return true;
+
+return false;
+}
+
+
 static int
 qemuSnapshotCreateAlignDisks(virDomainObj *vm,
  virDomainSnapshotDef *def,
@@ -1584,7 +1598,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm,
 {
 g_autofree char *xml = NULL;
 qemuDomainObjPrivate *priv = vm->privateData;
-virDomainSnapshotLocation align_location = 
VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
+virDomainSnapshotLocation align_location = 
VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT;
 
 /* Easiest way to clone inactive portion of vm->def is via
  * conversion in and back out of xml.  */
@@ -1604,17 +1618,19 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm,
 return -1;
 }
 
-if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
+if (qemuSnapshotCreateUseExternal(def, flags)) {
 align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
-if (virDomainObjIsActive(vm))
-def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT;
-else
-def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
-def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO;
-} else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
 def->state = virDomainObjGetState(vm, NULL);
-align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+
+if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
+if (virDomainObjIsActive(vm))
+def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT;
+else
+def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
+def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO;
+}
 } else {
+align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
 def->state = virDomainObjGetState(vm, NULL);
 
 if (virDomainObjIsActive(vm) &&
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH 0/2] don't require disk-only flag when creating external snapshots

[Pavel Hrdina]

Pavel Hrdina (2):
  qemu_snapshot: create: refactor external snapshot detection
  qemu_snapshot: create: don't require disk-only flag for offline
external snapshot

 src/qemu/qemu_snapshot.c | 44 
 1 file changed, 35 insertions(+), 9 deletions(-)

-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [libvirt PATCH 0/9] rpm: Tweak dependencies

[Pavel Hrdina]

On Fri, Dec 01, 2023 at 06:20:57PM +0100, Andrea Bolognani wrote:
> Test pipeline: https://gitlab.com/abologna/libvirt/-/pipelines/1092535621
> lcitool changes: https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/447
> 
> Andrea Bolognani (9):
>   rpm: Drop MinGW BuildRequires on libgcrypt/libgpg-error
>   meson: Stop looking for udevadm at build time
>   meson: Stop looking for scrub at build time
>   meson: Stop looking for passt at build time
>   rpm: Add Requires on scrub
>   rpm: Drop BuildDepends on scrub
>   rpm: Drop BuildDepends on passt
>   ci: Refresh generated files
>   ci: Stop passing --nodeps to rpmbuild

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH for 9.9.0] qemuProcessStartWithMemoryState: Don't start qemu with '-loadvm SNAP' and '-incoming defer' together

[Pavel Hrdina]

On Fri, Dec 01, 2023 at 10:41:59AM +0100, Peter Krempa wrote:
> A bug in qemuProcessStartWithMemoryState caused that we would start qemu
> with '-loadvm SNAP' and '-incoming defer' together.  qemu doesn't expect
> that and crashes on an assertion failure [1].
> 
> [1]: https://issues.redhat.com/browse/RHEL-16782
> 
> Fixes: 8a88d3e5860881f430e528d3e5e8d6455ded4d1d
> Resolves: https://issues.redhat.com/browse/RHEL-17841
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_process.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH] qemu_process: fix crash in qemuSaveImageDecompressionStart

[Pavel Hrdina]

Commit changing the code to allow passing NULL as @data into
qemuSaveImageDecompressionStart() was not correct as it left the
original call into the function as well.

Introduced-by: 2f3e582a1ac1008eba8d43c751cdba8712dd1614
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2247754
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_process.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1ef032dbd2..b9267d8699 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8208,9 +8208,6 @@ qemuProcessStartWithMemoryState(virConnectPtr conn,
 }
 }
 
-if (qemuSaveImageDecompressionStart(data, fd, , , 
) < 0)
-return -1;
-
 /* No cookie means libvirt which saved the domain was too old to mess up
  * the CPU definitions.
  */
-- 
2.41.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[libvirt PATCH] qemu_snapshot: fix reverting to inactive snapshot

[Pavel Hrdina]

When reverting to inactive snapshot updating the domain definition needs
to happen after the new overlays are created otherwise qemu-img will
correctly fail with error:

Trying to create an image with the same filename as the backing file

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 1962ba4027..5fc0b82e79 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2157,13 +2157,20 @@ qemuSnapshotRevertExternalInactive(virDomainObj *vm,
 {
 virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver;
 g_autoptr(virBitmap) created = NULL;
+int ret = -1;
 
 created = virBitmapNew(tmpsnapdef->ndisks);
 
+if (qemuSnapshotCreateQcow2Files(driver, domdef, tmpsnapdef, created) < 0)
+goto cleanup;
+
 if (qemuSnapshotDomainDefUpdateDisk(domdef, tmpsnapdef, false) < 0)
-return -1;
+goto cleanup;
 
-if (qemuSnapshotCreateQcow2Files(driver, domdef, tmpsnapdef, created) < 0) 
{
+ret = 0;
+
+ cleanup:
+if (ret < 0 && created) {
 ssize_t bit = -1;
 virErrorPtr err = NULL;
 
@@ -2180,11 +2187,9 @@ qemuSnapshotRevertExternalInactive(virDomainObj *vm,
 }
 
 virErrorRestore();
-
-return -1;
 }
 
-return 0;
+return ret;
 }
 
 
-- 
2.41.0

[libvirt PATCH] qemu_snapshot: fix snapshot deletion that had multiple children

[Pavel Hrdina]

When we revert to non-leaf snapshot and create new branch or branches
the overlay in snapshot metadata is no longer usable as a disk source
for deletion of that snapshot. We need to use other places to figure out
the correct storage source.

Fixes: https://gitlab.com/libvirt/libvirt/-/issues/534

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

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 1962ba4027..ee06e72b11 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2748,6 +2748,44 @@ qemuSnapshotGetDisksWithBackingStore(virDomainObj *vm,
 }
 
 
+/**
+ * qemuSnapshotExternalGetSnapDiskSrc:
+ * @vm: domain object
+ * @snap: snapshot object
+ * @snapDisk: disk definition from snapshost
+ *
+ * Try to get actual disk source for @snapDisk as the source stored in
+ * snapshot metadata is not always the correct source we need to work with.
+ * This happens mainly after reverting to non-leaf snapshot and creating
+ * new branch with new snapshot.
+ *
+ * Returns disk source on success, NULL on error.
+ */
+static virStorageSource *
+qemuSnapshotExternalGetSnapDiskSrc(virDomainObj *vm,
+   virDomainMomentObj *snap,
+   virDomainSnapshotDiskDef *snapDisk)
+{
+virDomainDiskDef *disk = NULL;
+
+/* Should never happen when deleting external snapshot as for now we do
+ * not support this specific case for now. */
+if (snap->nchildren > 1)
+return snapDisk->src;
+
+if (snap->first_child) {
+disk = qemuDomainDiskByName(snap->first_child->def->dom, 
snapDisk->name);
+} else if (virDomainSnapshotGetCurrent(vm->snapshots) == snap) {
+disk = qemuDomainDiskByName(vm->def, snapDisk->name);
+}
+
+if (disk)
+return disk->src;
+
+return snapDisk->src;
+}
+
+
 /**
  * qemuSnapshotDeleteExternalPrepareData:
  * @vm: domain object
@@ -2802,18 +2840,22 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
 }
 
 if (data->merge) {
+virStorageSource *snapDiskSrc = NULL;
+
 data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name);
 if (!data->domDisk)
 return -1;
 
+snapDiskSrc = qemuSnapshotExternalGetSnapDiskSrc(vm, snap, 
data->snapDisk);
+
 if (virDomainObjIsActive(vm)) {
 data->diskSrc = 
virStorageSourceChainLookupBySource(data->domDisk->src,
-
data->snapDisk->src,
+
snapDiskSrc,
 
>prevDiskSrc);
 if (!data->diskSrc)
 return -1;
 
-if (!virStorageSourceIsSameLocation(data->diskSrc, 
data->snapDisk->src)) {
+if (!virStorageSourceIsSameLocation(data->diskSrc, 
snapDiskSrc)) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("VM disk source and snapshot disk source 
are not the same"));
 return -1;
-- 
2.41.0