Re: [PATCH RESEND v3 8/8] libvirt.spec: drop no longer required build dependencies

[Neal Gompa]

On Tue, May 25, 2021 at 11:34 AM Andrea Bolognani  wrote:
>
> On Tue, May 25, 2021 at 05:06:41PM +0200, Michal Prívozník wrote:
> > On 5/25/21 12:00 PM, Andrea Bolognani wrote:
> > > On Tue, May 25, 2021 at 10:00:47AM +0200, Pavel Hrdina wrote:
> > >> -# For storage wiping with different algorithms
> > >> -BuildRequires: scrub
> > >
> > > Turns out we're actually missing the runtime dependency on scrub! Can
> > > you please take care of addressing that issue in a separate patch?
> >
> > Are we? scrub is needed if and only if you want to pass a special
> > algorithm to virStorageVolWipePattern(). Does that justify a runtime
> > dependency? We fail gracefully if scrub isn't installed. We aren't even
> > requiring qemu binary for daemon-driver-qemu package.
> >
> > Wasn't the whole idea to drop dependencies?
>
> The point was to drop **build** dependencies, specifically
> problematic ones such as ZFS. When it comes to runtime dependencies,
> our RPMs generally try to be as featureful as possible, so adding a
> dependency on scrub is the right thing to do IMO.
>

Why is ZFS problematic? The zfs-fuse package is totally fine for us to
depend on...


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH v4] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

Am Tue, 25 May 2021 23:40:59 +0200
schrieb Olaf Hering :

> In case of error the objects are disposed by libxl_domain_config_dispose.

I just realized this is not always true, unless this change is applied on top.
If libxlMakeNic fails, libxl_domain_config_dispose can not release anything.
With this additional change each successfully initialized object can be fully 
disposed.

Olaf

--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1523,13 +1523,14 @@ libxlMakeNicList(virDomainDef *def,  
libxl_domain_config *d_config)
 libxl_device_nic *x_nics;
 size_t i, nvnics = 0;
 
-x_nics = g_new0(libxl_device_nic, nnics);
+d_config->nics = x_nics = g_new0(libxl_device_nic, nnics);
 
 for (i = 0; i < nnics; i++) {
 if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
 continue;
 
 libxl_device_nic_init(_nics[nvnics]);
+d_config->num_nics = nvnics + 1;
 if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
 return -1;
 /*
@@ -1544,8 +1545,6 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
 }
 
 VIR_SHRINK_N(x_nics, nnics, nnics - nvnics);
-d_config->nics = x_nics;
-d_config->num_nics = nvnics;
 
 return 0;
 }


pgp2mzaQmza8D.pgp
Description: Digitale Signatur von OpenPGP

[PATCH v4] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

libxl objects are supposed to be initialized and disposed.
Adjust libxlMakeNic to use an already initialized object, it is owned by
the caller.

Adjust libxlMakeNicList to initialize the list of objects, before they
are filled by libxlMakeNic. In case of error the objects are disposed
by libxl_domain_config_dispose.

Signed-off-by: Olaf Hering 
---
 src/libxl/libxl_conf.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2ecbcf6911..c672bafbe9 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1335,8 +1335,6 @@ libxlMakeNic(virDomainDef *def,
 return -1;
 }
 
-libxl_device_nic_init(x_nic);
-
 virMacAddrGetRaw(_nic->mac, x_nic->mac);
 
 /*
@@ -1531,8 +1529,9 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
 if (virDomainNetGetActualType(l_nics[i]) == 
VIR_DOMAIN_NET_TYPE_HOSTDEV)
 continue;
 
+libxl_device_nic_init(_nics[nvnics]);
 if (libxlMakeNic(def, l_nics[i], _nics[nvnics], false))
-goto error;
+return -1;
 /*
  * The devid (at least right now) will not get initialized by
  * libxl in the setup case but is required for starting the
@@ -1549,12 +1548,6 @@ libxlMakeNicList(virDomainDef *def,  libxl_domain_config 
*d_config)
 d_config->num_nics = nvnics;
 
 return 0;
-
- error:
-for (i = 0; i < nnics; i++)
-libxl_device_nic_dispose(_nics[i]);
-VIR_FREE(x_nics);
-return -1;
 }
 
 int

Re: [libvirt PATCH 0/4] Support autostart for mediated devices

[Daniel P . Berrangé]

On Tue, May 25, 2021 at 11:36:59AM -0500, Jonathon Jongsma wrote:
> On Tue, 2021-05-25 at 11:20 +0100, Daniel P. Berrangé wrote:
> > On Fri, May 14, 2021 at 04:28:57PM -0500, Jonathon Jongsma wrote:
> > > This is a short patch series based on an initial patch from Boris
> > > Fiuczynski
> > > that I massaged a little bit and added a few additional patches.
> > > 
> > > This allows you to define mediated devices in libvirt which are
> > > started
> > > automatically at boot or parent device plugin.
> > 
> > Sorry I missed review of this series before it was merged, but I
> > think
> > this design aproach is seriously wrong. This is exposing autostart as
> > an XML property, but in all other parts of libvirt we expose
> > autostart
> > as an explicit API. I think we need to make the implemnentation
> > consistent with the rest of libvirt.
> 
> Hmm, you're right. Sorry I didn't catch that. Should we revert these
> patches for now then? I'll try to come up with a new implementation.

Several of the patches are still useful. Only the 2nd patch and some
files in the 3rd are impacted I think.

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

Recommended volume permissions (being created for vagrant-libvirt via fog-libvirt)

[Darragh Bailey]

Hi,

A request has come up recently in vagrant-libvirt about changing the
permissions used for the VM volume image file.

Currently there is a backing image file uploaded that gets 744 as the file
permissions, and then the VM domain is created using this as the backing
file for any changes. The file containing the changes for the VM gets 600,
so accessing what is contained is limited to libvirt and thus to those that
can connect to libvirt.

The request is to change this to be 744, it appears to have been triggered
due to a desire to try and use virt-v2v to create a portable XML and export
the disks.

However I'm a little hesitant as in general I would default to more secure
rather than less secure to avoid creating security concerns down the line.
Even though vagrant-libvirt is typically used for development, it wouldn't
surprise me to see it being used on CI build infrastructure and given the
shared nature of that, making things less secure may cause issues for some
users. Of course working out who would be impacted is virtually impossible
without making the change and seeing who is concerned. And that might be
several months down the line before it's raised.

Rather than just merging this, wondering if there are any security
guidelines on the file permissions for VM image files? That or something
that can outline the risks, or even clarify that it's unnecessary to worry
about?

--
Darragh Bailey
"Nothing is foolproof to a sufficiently talented fool"

Re: [libvirt PATCH 0/4] Support autostart for mediated devices

[Jonathon Jongsma]

On Tue, 2021-05-25 at 11:20 +0100, Daniel P. Berrangé wrote:
> On Fri, May 14, 2021 at 04:28:57PM -0500, Jonathon Jongsma wrote:
> > This is a short patch series based on an initial patch from Boris
> > Fiuczynski
> > that I massaged a little bit and added a few additional patches.
> > 
> > This allows you to define mediated devices in libvirt which are
> > started
> > automatically at boot or parent device plugin.
> 
> Sorry I missed review of this series before it was merged, but I
> think
> this design aproach is seriously wrong. This is exposing autostart as
> an XML property, but in all other parts of libvirt we expose
> autostart
> as an explicit API. I think we need to make the implemnentation
> consistent with the rest of libvirt.
> 
> 
> Regards,
> Daniel


Hmm, you're right. Sorry I didn't catch that. Should we revert these
patches for now then? I'll try to come up with a new implementation.

Jonathon

Re: [PATCH] meson.build: Fix the -Wvla-larger-than flag

[Daniel P . Berrangé]

On Tue, May 25, 2021 at 06:02:47PM +0200, Thomas Huth wrote:
> It's "...-than=..." and not "...-then=...".
> 
> Fixes: 8dd259d0c6 ("meson: add manywarnings")
> Signed-off-by: Thomas Huth 
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 4f23f9104e..8ee0416700 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -390,7 +390,7 @@ cc_flags += [
>'-Wvariadic-macros',
>'-Wvector-operation-performance',
>'-Wvla',
> -  '-Wvla-larger-then=4031',
> +  '-Wvla-larger-than=4031',

IIUC,  -Wvla-larger-than=NNN weakens the check done by -Wvla such that
it only complains about a subset of vlas that are unbounded or larger
than the noted size.

Given that we had this typo, we were only ever using -Wvla. Since we
compile without warnings, we evidentally don't have any vlas that
trigger with -Wvla at all.

So by that rationale I think -Wvla-larger-than=NNN is redundant and
can be removed entirely


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

[PATCH] meson.build: Fix the -Wvla-larger-than flag

[Thomas Huth]

It's "...-than=..." and not "...-then=...".

Fixes: 8dd259d0c6 ("meson: add manywarnings")
Signed-off-by: Thomas Huth 
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 4f23f9104e..8ee0416700 100644
--- a/meson.build
+++ b/meson.build
@@ -390,7 +390,7 @@ cc_flags += [
   '-Wvariadic-macros',
   '-Wvector-operation-performance',
   '-Wvla',
-  '-Wvla-larger-then=4031',
+  '-Wvla-larger-than=4031',
   '-Wvolatile-register-var',
   '-Wwrite-strings',
 ]
-- 
2.27.0

Re: [PATCH RESEND v3 8/8] libvirt.spec: drop no longer required build dependencies

[Andrea Bolognani]

On Tue, May 25, 2021 at 05:06:41PM +0200, Michal Prívozník wrote:
> On 5/25/21 12:00 PM, Andrea Bolognani wrote:
> > On Tue, May 25, 2021 at 10:00:47AM +0200, Pavel Hrdina wrote:
> >> -# For storage wiping with different algorithms
> >> -BuildRequires: scrub
> >
> > Turns out we're actually missing the runtime dependency on scrub! Can
> > you please take care of addressing that issue in a separate patch?
>
> Are we? scrub is needed if and only if you want to pass a special
> algorithm to virStorageVolWipePattern(). Does that justify a runtime
> dependency? We fail gracefully if scrub isn't installed. We aren't even
> requiring qemu binary for daemon-driver-qemu package.
>
> Wasn't the whole idea to drop dependencies?

The point was to drop **build** dependencies, specifically
problematic ones such as ZFS. When it comes to runtime dependencies,
our RPMs generally try to be as featureful as possible, so adding a
dependency on scrub is the right thing to do IMO.

-- 
Andrea Bolognani / Red Hat / Virtualization

Entering freeze for libvirt-7.4.0

[Jiri Denemark]

I have just tagged v7.4.0-rc1 in the repository and pushed signed
tarballs and source RPMs to https://libvirt.org/sources/

Please give the release candidate some testing and in case you find a
serious issue which should have a fix in the upcoming release, feel
free to reply to this thread to make sure the issue is more visible.

If you have not done so yet, please update NEWS.rst to document any
significant change you made since the last release.

Thanks,

Jirka

Re: [PATCH RESEND v3 8/8] libvirt.spec: drop no longer required build dependencies

[Michal Prívozník]

On 5/25/21 12:00 PM, Andrea Bolognani wrote:
> On Tue, May 25, 2021 at 10:00:47AM +0200, Pavel Hrdina wrote:
>> -# For mount/umount in FS driver
>> -BuildRequires: util-linux
> 
> This BuildRequires is duplicated, so please drop the other instance
> as well.
> 
>> -# For storage wiping with different algorithms
>> -BuildRequires: scrub
> 
> Turns out we're actually missing the runtime dependency on scrub! Can
> you please take care of addressing that issue in a separate patch?

Are we? scrub is needed if and only if you want to pass a special
algorithm to virStorageVolWipePattern(). Does that justify a runtime
dependency? We fail gracefully if scrub isn't installed. We aren't even
requiring qemu binary for daemon-driver-qemu package.

Wasn't the whole idea to drop dependencies?

Michal

availability of cluster_size support (re. [libvirt PATCH v2 0/2] storage_driver: add support for cluster_size QCOW2 option)

[Geoff Macartney1]

Hello all,

 

If I understand correctly, the addition of cluster_size support [1] will be available in libvirt 7.4.0, expected to be included in RHEL-AV-8.5.0.

 

I know "Red Hat does not generally disclose future release schedules" but is it possible to know roughly when this is due for release?

 

Kind regards

Geoff

 

 

[1] https://listman.redhat.com/archives/libvir-list/2021-May/msg00620.html

Re: RFC: Qemu backup interface plans

[Vladimir Sementsov-Ogievskiy]

25.05.2021 11:50, Max Reitz wrote:

On 19.05.21 08:11, Vladimir Sementsov-Ogievskiy wrote:

18.05.2021 19:39, Max Reitz wrote:


[...]


On 17.05.21 14:07, Vladimir Sementsov-Ogievskiy wrote:


[...]


Not also, that there is another benefit of such thing: we'll implement this 
callback in qcow2 driver, so that backup will read clusters not in guest 
cluster order, but in host cluster order, to read more sequentially, which 
should bring better performance on rotating disks.


I’m not exactly sure how you envision this to work, but block_status also 
already gives you the host offset in *map.



But block-status doesn't give a possibility to read sequentially. For this, 
user should call block-status several times until the whole disk covered, then 
sort the segments by host offset. I wonder, could it be implemented as some 
iterator, like

read_iter = bdrv_get_sequential_read_iter(source)

while (extents = bdrv_read_next(read_iter)):
   for ext in extents:
 start_writing_task(target, ext.offset, ext.bytes, ext.qiov)

where bdrv_read_next will read guest data in host-cluster-sequence..


How would you implement this, though?


I don't know :) That's why I wrote "I wonder".. Anyway I have enough work with 
all previous steps.


qcow2 doesn’t have a reverse mapping either, so it too would need to read all 
L2 table entries and sort them, wouldn’t it?



Hmm, yes. With current qcow2, it seems to be the only way. And we'll be limited 
by memory, so probably, read several L2 tables, do sort, return sorted extents, 
read next bunch of L2 tables and so on.

And then, I agree, we can just implement with help of current block_status() 
implementation.

Or probably we can implement reverse mapping in qcow2 as extension. But I doubt 
that it worth the complexity.. Still, it depends on how much extra IO will it 
cost (almost nothing, as should work through qcow2 cache) and how much 
performance benefit will it bring (no idea, it should be measured).

--
Best regards,
Vladimir

Re: [PATCH v3] libxl: adjust handling of libxl_device_nic objects

[Olaf Hering]

Am Fri, 21 May 2021 10:49:15 -0600
schrieb Jim Fehlig :

> There's actually a subtle bug with this logic change.

Well, the devid index number used to depend on "nnics", now it would depend on 
"nvnics". That might be an incompatible change indeed.

Olaf 


pgpsGmVto0jvm.pgp
Description: Digitale Signatur von OpenPGP

[PATCH] spec: Fix %endif indentation

[Michal Privoznik]

In recent commit f772c1fd2a a misaligned %endif sneaked in which
upsets syntax-check. Align it properly.

Signed-off-by: Michal Privoznik 
---

Pushed as trivial and build breaker fix.

 libvirt.spec.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index cdca7d8db6..7920599498 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -73,7 +73,7 @@
 %else
 %ifnarch %{arches_qemu_kvm}
 %define with_storage_gluster 0
-   %endif
+%endif
 %endif
 %endif
 
-- 
2.26.3

Re: [PATCH for 7.4.0 v2 0/3] Avoid double indentation of element

[Michal Prívozník]

On 5/25/21 12:12 PM, Pavel Hrdina wrote:
> On Tue, May 25, 2021 at 11:57:39AM +0200, Michal Prívozník wrote:
>> On 5/25/21 11:48 AM, Michal Privoznik wrote:
>>>
>>
>> Meanwhile, this was fixed in libxml2's upstream:
>>
>> https://gitlab.gnome.org/GNOME/libxml2/-/commit/13ad8736d294536da4cbcd70a96b0a2fbf47070c
>>
>> So I guess there won't be any regression with any upstream release of
>> libxml2. But as I pointed out in v1, some distros are known to already
>> backport the problematic patch (Fedora and Gentoo) so the question is
>> how likely they are to backport the fix too. I still think it's worth
>> fixing on our side.
> 
> I'm not sure if it's worth fixing, on one hand libvirt tests would be
> broken, on the other hand running libvirt tests could detect the broken
> backport and help others to create bug reports to backport the fix as
> well.

I hear you but also, if we want to freeze anytime soon we should have
green pipeline :-)

And also, we already have precedence of working around broken libraries
in our code. Remember glib and all the fun we were having?

> 
> Anyway, for the code
> 
> Reviewed-by: Pavel Hrdina 
> 

Pushed, thank you.

Michal

[libvirt PATCH] docs: introduce stubs for new libvirt Go packages

[Daniel P . Berrangé]

Currently we expose libvirt Go packages at

  libvirt.org/libvirt-go
  libvirt.org/libvirt-go-xml

These packages have not supported Go modules historically and when we
tried to introduce modules, we hit the problem that we're not using
semver for versioning.

The only way around this is to introduce new packages under a different
namespace, that will have the exact same code, but be tagged with a
different version numbering scheme.

This change proposes:

  libvirt.org/go/libvirt
  libvirt.org/go/libvirtxml

Note the hyphen is removed so that the import basename matches the
Go package name.

Signed-off-by: Daniel P. Berrangé 
---

The corresponding repos exist

  https://gitlab.com/libvirt/libvirt-go-module
  https://gitlab.com/libvirt/libvirt-go-xml-module

but I'm leaving them empty until we've completed this release, then
I'll populate with content matching 7.4.0, minus the existing tags.
This change should also wait until after the release.

 docs/go/libvirt.rst| 13 +++
 docs/go/libvirtxml.rst | 10 
 docs/go/meson.build| 53 ++
 docs/meson.build   |  1 +
 4 files changed, 77 insertions(+)
 create mode 100644 docs/go/libvirt.rst
 create mode 100644 docs/go/libvirtxml.rst
 create mode 100644 docs/go/meson.build

diff --git a/docs/go/libvirt.rst b/docs/go/libvirt.rst
new file mode 100644
index 00..46250691c1
--- /dev/null
+++ b/docs/go/libvirt.rst
@@ -0,0 +1,13 @@
+===
+Libvirt Go Language API
+===
+
+The `Go `__ package ``libvirt.org/go/libvirt`` provides
+`CGo `__ binding from the OS native Libvirt API.
+
+In general the Go representation is a direct 1-1 mapping from native API
+concepts to Go, so the native API documentation should serve as a reference
+for most behaviour.
+
+For details of Go specific behaviour consult the
+`Go package documentation `__.
diff --git a/docs/go/libvirtxml.rst b/docs/go/libvirtxml.rst
new file mode 100644
index 00..7583433f56
--- /dev/null
+++ b/docs/go/libvirtxml.rst
@@ -0,0 +1,10 @@
+==
+Libvirt Go XML parsing API
+==
+
+The `Go `__ package ``libvirt.org/go/libvirtxml`` provides
+annotated Go struct definitions for parsing (and formatting) XML documents used
+with libvirt APIs.
+
+For details of Go specific behaviour consult the
+`Go package documentation `__.
diff --git a/docs/go/meson.build b/docs/go/meson.build
new file mode 100644
index 00..99fa1b074a
--- /dev/null
+++ b/docs/go/meson.build
@@ -0,0 +1,53 @@
+docs_go_files = [
+  'libvirt',
+  'libvirtxml',
+]
+
+html_xslt_gen_xslt = subsite_xsl
+html_xslt_gen_install_dir = docs_html_dir / 'go'
+html_xslt_gen = []
+
+foreach name : docs_go_files
+  rst_file = '@0@.rst'.format(name)
+
+  html_xslt_gen += {
+'name': name,
+'file': docs_rst2html_gen.process(rst_file),
+'source': 'docs/go' / rst_file,
+  }
+endforeach
+
+# keep the XSLT processing code block in sync with docs/meson.build
+
+# --- begin of XSLT processing ---
+
+foreach data : html_xslt_gen
+  html_filename = data['name'] + '.html'
+
+  html_file = custom_target(
+html_filename,
+input: data.get('file', data['name'] + '.html.in'),
+output: html_filename,
+command: [
+  xsltproc_prog,
+  '--stringparam', 'pagesrc', data.get('source', ''),
+  '--stringparam', 'builddir', meson.build_root(),
+  '--stringparam', 'timestamp', docs_timestamp,
+  '--nonet',
+  html_xslt_gen_xslt,
+  '@INPUT@',
+],
+depends: data.get('depends', []),
+depend_files: [ page_xsl ],
+capture: true,
+install: true,
+install_dir: html_xslt_gen_install_dir,
+  )
+
+  install_web_deps += html_file
+  install_web_files += html_file.full_path() + ':' + html_xslt_gen_install_dir
+endforeach
+
+html_xslt_gen = []
+
+# --- end of XSLT processing ---
diff --git a/docs/meson.build b/docs/meson.build
index f550629d8e..d7afdbd323 100644
--- a/docs/meson.build
+++ b/docs/meson.build
@@ -296,6 +296,7 @@ html_xslt_gen = []
 # --- end of XSLT processing ---
 
 subdir('fonts')
+subdir('go')
 subdir('html')
 subdir('internals')
 subdir('js')
-- 
2.31.1

Re: [libvirt PATCH 0/4] Support autostart for mediated devices

[Daniel P . Berrangé]

On Fri, May 14, 2021 at 04:28:57PM -0500, Jonathon Jongsma wrote:
> This is a short patch series based on an initial patch from Boris Fiuczynski
> that I massaged a little bit and added a few additional patches.
> 
> This allows you to define mediated devices in libvirt which are started
> automatically at boot or parent device plugin.

Sorry I missed review of this series before it was merged, but I think
this design aproach is seriously wrong. This is exposing autostart as
an XML property, but in all other parts of libvirt we expose autostart
as an explicit API. I think we need to make the implemnentation
consistent with the rest of libvirt.


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: [PATCH for 7.4.0 v2 0/3] Avoid double indentation of element

[Pavel Hrdina]

On Tue, May 25, 2021 at 11:57:39AM +0200, Michal Prívozník wrote:
> On 5/25/21 11:48 AM, Michal Privoznik wrote:
> > 
> 
> Meanwhile, this was fixed in libxml2's upstream:
> 
> https://gitlab.gnome.org/GNOME/libxml2/-/commit/13ad8736d294536da4cbcd70a96b0a2fbf47070c
> 
> So I guess there won't be any regression with any upstream release of
> libxml2. But as I pointed out in v1, some distros are known to already
> backport the problematic patch (Fedora and Gentoo) so the question is
> how likely they are to backport the fix too. I still think it's worth
> fixing on our side.

I'm not sure if it's worth fixing, on one hand libvirt tests would be
broken, on the other hand running libvirt tests could detect the broken
backport and help others to create bug reports to backport the fix as
well.

Anyway, for the code

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature

Re: [PATCH RESEND v3 8/8] libvirt.spec: drop no longer required build dependencies

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:47AM +0200, Pavel Hrdina wrote:
> -# For mount/umount in FS driver
> -BuildRequires: util-linux

This BuildRequires is duplicated, so please drop the other instance
as well.

> -# For storage wiping with different algorithms
> -BuildRequires: scrub

Turns out we're actually missing the runtime dependency on scrub! Can
you please take care of addressing that issue in a separate patch?

With the second BuildRequires on util-linux removed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH for 7.4.0 v2 0/3] Avoid double indentation of element

[Michal Prívozník]

On 5/25/21 11:48 AM, Michal Privoznik wrote:
> 

Meanwhile, this was fixed in libxml2's upstream:

https://gitlab.gnome.org/GNOME/libxml2/-/commit/13ad8736d294536da4cbcd70a96b0a2fbf47070c

So I guess there won't be any regression with any upstream release of
libxml2. But as I pointed out in v1, some distros are known to already
backport the problematic patch (Fedora and Gentoo) so the question is
how likely they are to backport the fix too. I still think it's worth
fixing on our side.

Michal

Re: [PATCH RESEND v3 7/8] meson: optional_programs should be used only for building libvirt

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:46AM +0200, Pavel Hrdina wrote:
> Drop code that creates defines with program paths and update the comment
> to reflect current usage of optional_programs.
>
> Signed-off-by: Pavel Hrdina 
> ---
>  meson.build | 10 +-
>  1 file changed, 1 insertion(+), 9 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v3 6/8] meson: use runtime binaries to only resolve features with "auto" value

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:45AM +0200, Pavel Hrdina wrote:
> This way meson will try to do the right thing by default but we will
> allow users to change this behavior by using -Dname=enabled. This comes
> with two benefits compared to the previous behavior:
>
>   - no need to install the binaries if developers would like to check
> that the code compiles correctly
>
>   - package maintainers can drop some build dependencies
>
> Signed-off-by: Pavel Hrdina 
> ---
>  meson.build | 102 
>  1 file changed, 63 insertions(+), 39 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v3 4/8] meson: move iscsiadm check into storage_iscsi condition

[Andrea Bolognani]

On Tue, May 25, 2021 at 02:36:04AM -0700, Andrea Bolognani wrote:
> On Tue, May 25, 2021 at 10:00:43AM +0200, Pavel Hrdina wrote:
> > +  if not get_option('storage_iscsi').disabled()
> > +iscsi_enable = true
> > +iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
> > libvirt_sbin_path)
> > +
> > +if not iscsiadm_prog.found()
> > +  if get_option('storage_iscsi').enabled()
> > +error('We need iscsiadm for iSCSI storage driver')
> > +  else
> > +iscsi_enable = false
> > +  endif
> > +endif
>
> I believe this could be rewritten as
>
>   if not get_option('storage_iscsi').disabled()
> iscsi_enable = true
> iscsiadm_prog = find_program('iscsiadm', required: 
> get_option('storage_iscsi'), dirs: libvirt_sbin_path)
>
> if not iscsiadm_prog.found()
>   iscsi_enable = false
> endif
>
> I see this pattern used elsewhere in the file and it's more concise
> while achieving the same result AFAICT.

I just realized that you rewrite this check again in 6/8, so no
matter what it looks like in this commit

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH v2 3/3] virxml: Avoid double indentation of element

[Michal Privoznik]

There was a recent change in libxml2 that caused a trouble for
us. To us,  in domain or network XMLs are just opaque
value where management application can store whatever data it
finds fit. At XML parser/formatter level, we just make a copy of
the element during parsing and then format it back. For
formatting we use xmlNodeDump() which allows caller to specify
level of indentation. Previously, the indentation was not
applied onto the very first line, but as of v2.9.12-2-g85b1792e
libxml2 is applying indentation also on the first line.

This does not work well with out virBuffer because as soon as we
call virBufferAsprintf() to append  element,
virBufferAsprintf() will apply another level of indentation.

Instead of version checking, let's skip any indentation added by
libxml2 before virBufferAsprintf() is called.

Note, the problem is only when telling xmlNodeDump() to use
indentation, i.e. level argument is not zero. Therefore,
virXMLNodeToString() which also calls xmlNodeDump() is safe as it
passes zero.

Tested-by: Bjoern Walk 
Signed-off-by: Michal Privoznik 
---
 src/util/virxml.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index 91c6f6b02e..4360b15486 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -1723,6 +1723,7 @@ virXMLFormatMetadata(virBuffer *buf,
  xmlNodePtr metadata)
 {
 g_autoptr(xmlBuffer) xmlbuf = NULL;
+const char *xmlbufContent = NULL;
 int oldIndentTreeOutput = xmlIndentTreeOutput;
 
 if (!metadata)
@@ -1745,7 +1746,12 @@ virXMLFormatMetadata(virBuffer *buf,
 return -1;
 }
 
-virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
+/* After libxml2-v2.9.12-2-g85b1792e even the first line is indented.
+ * But virBufferAsprintf() also adds indentation. Skip one of them. */
+xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
+virSkipSpaces();
+
+virBufferAsprintf(buf, "%s\n", xmlbufContent);
 xmlIndentTreeOutput = oldIndentTreeOutput;
 
 return 0;
-- 
2.26.3

[PATCH v2 2/3] virxml: Report error if virXMLFormatMetadata() fails

[Michal Privoznik]

I guess this is more of an academic problem, because if
 content was problematic we would have caught the
error during parsing. Anyway, as is this function returns -1
without any error reported. Fix it by reporting one.

Signed-off-by: Michal Privoznik 
---
 src/util/virxml.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/virxml.c b/src/util/virxml.c
index 062a5402f6..91c6f6b02e 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -1740,6 +1740,8 @@ virXMLFormatMetadata(virBuffer *buf,
 if (xmlNodeDump(xmlbuf, metadata->doc, metadata,
 virBufferGetIndent(buf) / 2, 1) < 0) {
 xmlIndentTreeOutput = oldIndentTreeOutput;
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Unable to format metadata element"));
 return -1;
 }
 
-- 
2.26.3

[PATCH v2 1/3] virxml: Introduce and use virXMLFormatMetadata()

[Michal Privoznik]

So far, we have to places where we format  into XMLs:
domain and network. Bot places share the same code. Move it into
a helper function and just call it from those places.

Signed-off-by: Michal Privoznik 
---
 src/conf/domain_conf.c   | 23 ++---
 src/conf/network_conf.c  | 23 ++---
 src/libvirt_private.syms |  1 +
 src/util/virxml.c| 43 
 src/util/virxml.h|  3 +++
 5 files changed, 51 insertions(+), 42 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4f78b7b43d..413c44ac61 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27806,27 +27806,8 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
*def,
 virBufferEscapeString(buf, "%s\n",
   def->description);
 
-if (def->metadata) {
-g_autoptr(xmlBuffer) xmlbuf = NULL;
-int oldIndentTreeOutput = xmlIndentTreeOutput;
-
-/* Indentation on output requires that we previously set
- * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2
- * spaces per level of indentation of intermediate elements,
- * but no leading indentation before the starting element.
- * Thankfully, libxml maps what looks like globals into
- * thread-local uses, so we are thread-safe.  */
-xmlIndentTreeOutput = 1;
-xmlbuf = virXMLBufferCreate();
-
-if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
-virBufferGetIndent(buf) / 2, 1) < 0) {
-xmlIndentTreeOutput = oldIndentTreeOutput;
-return -1;
-}
-virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
-xmlIndentTreeOutput = oldIndentTreeOutput;
-}
+if (virXMLFormatMetadata(buf, def->metadata) < 0)
+return -1;
 
 if (virDomainDefHasMemoryHotplug(def)) {
 virBufferAsprintf(buf,
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index a9eadff29c..b10ff5c7a8 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2486,27 +2486,8 @@ virNetworkDefFormatBuf(virBuffer *buf,
 virUUIDFormat(uuid, uuidstr);
 virBufferAsprintf(buf, "%s\n", uuidstr);
 
-if (def->metadata) {
-g_autoptr(xmlBuffer) xmlbuf = NULL;
-int oldIndentTreeOutput = xmlIndentTreeOutput;
-
-/* Indentation on output requires that we previously set
- * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2
- * spaces per level of indentation of intermediate elements,
- * but no leading indentation before the starting element.
- * Thankfully, libxml maps what looks like globals into
- * thread-local uses, so we are thread-safe.  */
-xmlIndentTreeOutput = 1;
-xmlbuf = virXMLBufferCreate();
-
-if (xmlNodeDump(xmlbuf, def->metadata->doc, def->metadata,
-virBufferGetIndent(buf) / 2, 1) < 0) {
-xmlIndentTreeOutput = oldIndentTreeOutput;
-return -1;
-}
-virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
-xmlIndentTreeOutput = oldIndentTreeOutput;
-}
+if (virXMLFormatMetadata(buf, def->metadata) < 0)
+return -1;
 
 if (def->forward.type != VIR_NETWORK_FORWARD_NONE) {
 const char *dev = NULL;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e1aef5267e..37515f80ec 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3555,6 +3555,7 @@ virXMLCheckIllegalChars;
 virXMLExtractNamespaceXML;
 virXMLFormatElement;
 virXMLFormatElementEmpty;
+virXMLFormatMetadata;
 virXMLNewNode;
 virXMLNodeContentString;
 virXMLNodeNameEqual;
diff --git a/src/util/virxml.c b/src/util/virxml.c
index 8dcece704a..062a5402f6 100644
--- a/src/util/virxml.c
+++ b/src/util/virxml.c
@@ -1707,6 +1707,49 @@ virXMLFormatElement(virBuffer *buf,
 }
 
 
+/**
+ * virXMLFormatMetadata:
+ * @buf: the parent buffer where the element will be placed
+ * @metadata: pointer to metadata node
+ *
+ * Helper to format metadata element. If @metadata is NULL then
+ * this function is a NOP.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise.
+ */
+int
+virXMLFormatMetadata(virBuffer *buf,
+ xmlNodePtr metadata)
+{
+g_autoptr(xmlBuffer) xmlbuf = NULL;
+int oldIndentTreeOutput = xmlIndentTreeOutput;
+
+if (!metadata)
+return 0;
+
+/* Indentation on output requires that we previously set
+ * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2
+ * spaces per level of indentation of intermediate elements,
+ * but no leading indentation before the starting element.
+ * Thankfully, libxml maps what looks like globals into
+ * thread-local uses, so we are thread-safe.  */
+xmlIndentTreeOutput = 1;
+xmlbuf = virXMLBufferCreate();
+
+if (xmlNodeDump(xmlbuf, metadata->doc, metadata,
+

[PATCH for 7.4.0 v2 0/3] Avoid double indentation of element

[Michal Privoznik]

v2 of:

https://listman.redhat.com/archives/libvir-list/2021-May/msg00767.html

diff to v1:
- Deduplicated the code per Pavel's suggestion

Michal Prívozník (3):
  virxml: Introduce and use virXMLFormatMetadata()
  virxml: Report error if virXMLFormatMetadata() fails
  virxml: Avoid double indentation of  element

 src/conf/domain_conf.c   | 23 ++
 src/conf/network_conf.c  | 23 ++
 src/libvirt_private.syms |  1 +
 src/util/virxml.c| 51 
 src/util/virxml.h|  3 +++
 5 files changed, 59 insertions(+), 42 deletions(-)

-- 
2.26.3

Re: [PATCH RESEND v3 5/8] meson: stop setting runtime binaries defines during compilation

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:44AM +0200, Pavel Hrdina wrote:
> Technically the location of these binaries may be different when
> compiling libvirt or running it. This will allow users to change $PATH
> to use different binaries as well.
>
> Signed-off-by: Pavel Hrdina 
> ---
>  meson.build| 41 --
>  src/bhyve/bhyve_command.c  |  4 +++
>  src/locking/lock_driver_lockd.c| 12 ++--
>  src/storage/storage_backend_logical.c  | 13 
>  src/storage/storage_backend_sheepdog.c |  2 ++
>  src/storage/storage_backend_zfs.c  |  3 ++
>  src/storage/storage_util.h |  6 
>  src/util/virnuma.c |  1 +
>  8 files changed, 36 insertions(+), 46 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v3 4/8] meson: move iscsiadm check into storage_iscsi condition

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:43AM +0200, Pavel Hrdina wrote:
> +  if not get_option('storage_iscsi').disabled()
> +iscsi_enable = true
> +iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
> libvirt_sbin_path)
> +
> +if not iscsiadm_prog.found()
> +  if get_option('storage_iscsi').enabled()
> +error('We need iscsiadm for iSCSI storage driver')
> +  else
> +iscsi_enable = false
> +  endif
> +endif

I believe this could be rewritten as

  if not get_option('storage_iscsi').disabled()
iscsi_enable = true
iscsiadm_prog = find_program('iscsiadm', required:
get_option('storage_iscsi'), dirs: libvirt_sbin_path)

if not iscsiadm_prog.found()
  iscsi_enable = false
endif

I see this pattern used elsewhere in the file and it's more concise
while achieving the same result AFAICT.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH for 7.4.0] conf: Avoid double indentation of element

[Bjoern Walk]

Bjoern Walk  [2021-05-25, 11:01AM +0200]:
> Michal Privoznik  [2021-05-25, 09:32AM +0200]:
> > There was a recent change in libxml2 that caused a trouble for
> > us. To us,  in domain or network XMLs are just opaque
> > value where management application can store whatever data it
> > finds fit. At XML parser/formatter level, we just make a copy of
> > the element during parsing and then format it back. For
> > formatting we use xmlNodeDump() which allows caller to specify
> > level of indentation. Previously, the indentation was not
> > applied onto the very first line, but as of v2.9.12-2-g85b1792e
> > libxml2 is applying indentation also on the first line.
> > 
> > This does not work well with out virBuffer because as soon as we
> > call virBufferAsprintf() to append  element,
> > virBufferAsprintf() will apply another level of indentation.
> > 
> > Instead of version checking, let's skip any indentation added by
> > libxml2 before virBufferAsprintf() is called.
> > 
> > Note, the problem is only when telling xmlNodeDump() to use
> > indentation, i.e. level argument is not zero. Therefore,
> > virXMLNodeToString() which also calls xmlNodeDump() is safe as it
> > passes zero.
> > 
> > Signed-off-by: Michal Privoznik 
> 
> Tested-by: Bjoern Walk 
> 
> But like Daniel said, this now breaks for older libxml2 versions.

Ah, I also misread this. All is fine with both libxml2 v2.9.12 and
earlier.

-- 
IBM Systems
Linux on Z & KVM Development
--
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


signature.asc
Description: PGP signature

Re: [PATCH RESEND v3 3/8] meson: drop check for runtime binary dependencies

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:42AM +0200, Pavel Hrdina wrote:
> +#define EBTABLES_PATH "ebtables"
> +#define IPTABLES_PATH "iptables"
> +#define IP6TABLES_PATH "ip6tables"

These are no longer paths, so you should rename them accordingly in a
preparatory patch.

> +#define IP_PATH "ip"

Same here.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH for 7.4.0] conf: Avoid double indentation of element

[Daniel P . Berrangé]

On Tue, May 25, 2021 at 10:52:49AM +0200, Michal Prívozník wrote:
> On 5/25/21 10:47 AM, Daniel P. Berrangé wrote:
> > On Tue, May 25, 2021 at 09:32:35AM +0200, Michal Privoznik wrote:
> >> There was a recent change in libxml2 that caused a trouble for
> >> us. To us,  in domain or network XMLs are just opaque
> >> value where management application can store whatever data it
> >> finds fit. At XML parser/formatter level, we just make a copy of
> >> the element during parsing and then format it back. For
> >> formatting we use xmlNodeDump() which allows caller to specify
> >> level of indentation. Previously, the indentation was not
> >> applied onto the very first line, but as of v2.9.12-2-g85b1792e
> >> libxml2 is applying indentation also on the first line.
> >>
> >> This does not work well with out virBuffer because as soon as we
> >> call virBufferAsprintf() to append  element,
> >> virBufferAsprintf() will apply another level of indentation.
> >>
> >> Instead of version checking, let's skip any indentation added by
> >> libxml2 before virBufferAsprintf() is called.
> >>
> >> Note, the problem is only when telling xmlNodeDump() to use
> >> indentation, i.e. level argument is not zero. Therefore,
> >> virXMLNodeToString() which also calls xmlNodeDump() is safe as it
> >> passes zero.
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>
> >> I've raised this issue with libxml2 team:
> >>
> >> https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a37f92472e8de5f3f
> >>
> >> but I'm not sure it'll be fixed. Unfortunately, the patch that's causing
> >> us trouble is being backported all over the place, because it's
> >> supposedly fixing a regression.
> >>
> >>  src/conf/domain_conf.c  | 9 -
> >>  src/conf/network_conf.c | 9 -
> >>  2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> index 4f78b7b43d..84a8c269be 100644
> >> --- a/src/conf/domain_conf.c
> >> +++ b/src/conf/domain_conf.c
> >> @@ -27808,6 +27808,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
> >> *def,
> >>  
> >>  if (def->metadata) {
> >>  g_autoptr(xmlBuffer) xmlbuf = NULL;
> >> +const char *xmlbufContent = NULL;
> >>  int oldIndentTreeOutput = xmlIndentTreeOutput;
> >>  
> >>  /* Indentation on output requires that we previously set
> >> @@ -27824,7 +27825,13 @@ 
> >> virDomainDefFormatInternalSetRootName(virDomainDef *def,
> >>  xmlIndentTreeOutput = oldIndentTreeOutput;
> >>  return -1;
> >>  }
> >> -virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
> >> +
> >> +/* After libxml2-v2.9.12-2-g85b1792e even the first line is 
> >> indented.
> >> + * But virBufferAsprintf() also adds indentation. Skip one of 
> >> them. */
> >> +xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
> >> +virSkipSpaces();
> >> +
> >> +virBufferAsprintf(buf, "%s\n", xmlbufContent);
> > 
> > Won't this mean that indent is now wrong for the older libxml ?
> 
> No, because the older did not put any spaces at the beginning. I mean,
> with older libxml we get:
> 
>   xmlbufContent = "\nblah blah ...";
> 
> with new new libxl we get:
> 
>   xmlbufContent = "  \n blah blah ...";
> 
> Hence virSkipSpaces() which drops those leading spaces.

Ah, you're stripping the spaces that libxml added, rather than the
spaces that virBuffer added.


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: [PATCH RESEND v3 2/8] qemu_conf: use virFindFileInPathFull for runtime binaries

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:41AM +0200, Pavel Hrdina wrote:
> Following patches will stop detecting the full path during compilation
> so we will need to do it here.
>
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_conf.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH RESEND v3 1/8] virfile: introduce virFindFileInPathFull()

[Andrea Bolognani]

On Tue, May 25, 2021 at 10:00:40AM +0200, Pavel Hrdina wrote:
> Extend virFindFileInPath to search in custom extra paths as well. Some
> binaries that libvirt needs are not usually in $PATH so we need to have
> a way to look for these as well.
>
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt_private.syms |  2 +-
>  src/util/virfile.c   | 16 ++--
>  src/util/virfile.h   |  5 -
>  tests/testutilsqemu.c|  3 ++-
>  tests/virfirewallmock.c  |  3 ++-
>  5 files changed, 23 insertions(+), 6 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH for 7.4.0] conf: Avoid double indentation of element

[Bjoern Walk]

Michal Privoznik  [2021-05-25, 09:32AM +0200]:
> There was a recent change in libxml2 that caused a trouble for
> us. To us,  in domain or network XMLs are just opaque
> value where management application can store whatever data it
> finds fit. At XML parser/formatter level, we just make a copy of
> the element during parsing and then format it back. For
> formatting we use xmlNodeDump() which allows caller to specify
> level of indentation. Previously, the indentation was not
> applied onto the very first line, but as of v2.9.12-2-g85b1792e
> libxml2 is applying indentation also on the first line.
> 
> This does not work well with out virBuffer because as soon as we
> call virBufferAsprintf() to append  element,
> virBufferAsprintf() will apply another level of indentation.
> 
> Instead of version checking, let's skip any indentation added by
> libxml2 before virBufferAsprintf() is called.
> 
> Note, the problem is only when telling xmlNodeDump() to use
> indentation, i.e. level argument is not zero. Therefore,
> virXMLNodeToString() which also calls xmlNodeDump() is safe as it
> passes zero.
> 
> Signed-off-by: Michal Privoznik 

Tested-by: Bjoern Walk 

But like Daniel said, this now breaks for older libxml2 versions.


signature.asc
Description: PGP signature

Re: [PATCH for 7.4.0] conf: Avoid double indentation of element

[Pavel Hrdina]

On Tue, May 25, 2021 at 09:32:35AM +0200, Michal Privoznik wrote:
> There was a recent change in libxml2 that caused a trouble for
> us. To us,  in domain or network XMLs are just opaque
> value where management application can store whatever data it
> finds fit. At XML parser/formatter level, we just make a copy of
> the element during parsing and then format it back. For
> formatting we use xmlNodeDump() which allows caller to specify
> level of indentation. Previously, the indentation was not
> applied onto the very first line, but as of v2.9.12-2-g85b1792e
> libxml2 is applying indentation also on the first line.
> 
> This does not work well with out virBuffer because as soon as we
> call virBufferAsprintf() to append  element,
> virBufferAsprintf() will apply another level of indentation.
> 
> Instead of version checking, let's skip any indentation added by
> libxml2 before virBufferAsprintf() is called.
> 
> Note, the problem is only when telling xmlNodeDump() to use
> indentation, i.e. level argument is not zero. Therefore,
> virXMLNodeToString() which also calls xmlNodeDump() is safe as it
> passes zero.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> I've raised this issue with libxml2 team:
> 
> https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a37f92472e8de5f3f
> 
> but I'm not sure it'll be fixed. Unfortunately, the patch that's causing
> us trouble is being backported all over the place, because it's
> supposedly fixing a regression.
> 
>  src/conf/domain_conf.c  | 9 -
>  src/conf/network_conf.c | 9 -
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4f78b7b43d..84a8c269be 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -27808,6 +27808,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
> *def,
>  
>  if (def->metadata) {
>  g_autoptr(xmlBuffer) xmlbuf = NULL;
> +const char *xmlbufContent = NULL;
>  int oldIndentTreeOutput = xmlIndentTreeOutput;
>  
>  /* Indentation on output requires that we previously set
> @@ -27824,7 +27825,13 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
> *def,
>  xmlIndentTreeOutput = oldIndentTreeOutput;
>  return -1;
>  }
> -virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
> +
> +/* After libxml2-v2.9.12-2-g85b1792e even the first line is indented.
> + * But virBufferAsprintf() also adds indentation. Skip one of them. 
> */
> +xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
> +virSkipSpaces();
> +
> +virBufferAsprintf(buf, "%s\n", xmlbufContent);
>  xmlIndentTreeOutput = oldIndentTreeOutput;
>  }
>  
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index a9eadff29c..20c6dc091a 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2488,6 +2488,7 @@ virNetworkDefFormatBuf(virBuffer *buf,
>  
>  if (def->metadata) {
>  g_autoptr(xmlBuffer) xmlbuf = NULL;
> +const char *xmlbufContent = NULL;
>  int oldIndentTreeOutput = xmlIndentTreeOutput;
>  
>  /* Indentation on output requires that we previously set
> @@ -2504,7 +2505,13 @@ virNetworkDefFormatBuf(virBuffer *buf,
>  xmlIndentTreeOutput = oldIndentTreeOutput;
>  return -1;
>  }
> -virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
> +
> +/* After libxml2-v2.9.12-2-g85b1792e even the first line is indented.
> + * But virBufferAsprintf() also adds indentation. Skip one of them. 
> */
> +xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
> +virSkipSpaces();
> +
> +virBufferAsprintf(buf, "%s\n", xmlbufContent);

Please create a helper function and put that workaround there instead of
having it open-coded at two different places.

Otherwise looks good.

Pavel

>  xmlIndentTreeOutput = oldIndentTreeOutput;
>  }
>  
> -- 
> 2.26.3
> 


signature.asc
Description: PGP signature

Re: [PATCH for 7.4.0] conf: Avoid double indentation of element

[Michal Prívozník]

On 5/25/21 10:47 AM, Daniel P. Berrangé wrote:
> On Tue, May 25, 2021 at 09:32:35AM +0200, Michal Privoznik wrote:
>> There was a recent change in libxml2 that caused a trouble for
>> us. To us,  in domain or network XMLs are just opaque
>> value where management application can store whatever data it
>> finds fit. At XML parser/formatter level, we just make a copy of
>> the element during parsing and then format it back. For
>> formatting we use xmlNodeDump() which allows caller to specify
>> level of indentation. Previously, the indentation was not
>> applied onto the very first line, but as of v2.9.12-2-g85b1792e
>> libxml2 is applying indentation also on the first line.
>>
>> This does not work well with out virBuffer because as soon as we
>> call virBufferAsprintf() to append  element,
>> virBufferAsprintf() will apply another level of indentation.
>>
>> Instead of version checking, let's skip any indentation added by
>> libxml2 before virBufferAsprintf() is called.
>>
>> Note, the problem is only when telling xmlNodeDump() to use
>> indentation, i.e. level argument is not zero. Therefore,
>> virXMLNodeToString() which also calls xmlNodeDump() is safe as it
>> passes zero.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> I've raised this issue with libxml2 team:
>>
>> https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a37f92472e8de5f3f
>>
>> but I'm not sure it'll be fixed. Unfortunately, the patch that's causing
>> us trouble is being backported all over the place, because it's
>> supposedly fixing a regression.
>>
>>  src/conf/domain_conf.c  | 9 -
>>  src/conf/network_conf.c | 9 -
>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 4f78b7b43d..84a8c269be 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -27808,6 +27808,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
>> *def,
>>  
>>  if (def->metadata) {
>>  g_autoptr(xmlBuffer) xmlbuf = NULL;
>> +const char *xmlbufContent = NULL;
>>  int oldIndentTreeOutput = xmlIndentTreeOutput;
>>  
>>  /* Indentation on output requires that we previously set
>> @@ -27824,7 +27825,13 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
>> *def,
>>  xmlIndentTreeOutput = oldIndentTreeOutput;
>>  return -1;
>>  }
>> -virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
>> +
>> +/* After libxml2-v2.9.12-2-g85b1792e even the first line is 
>> indented.
>> + * But virBufferAsprintf() also adds indentation. Skip one of them. 
>> */
>> +xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
>> +virSkipSpaces();
>> +
>> +virBufferAsprintf(buf, "%s\n", xmlbufContent);
> 
> Won't this mean that indent is now wrong for the older libxml ?

No, because the older did not put any spaces at the beginning. I mean,
with older libxml we get:

  xmlbufContent = "\nblah blah ...";

with new new libxl we get:

  xmlbufContent = "  \n blah blah ...";

Hence virSkipSpaces() which drops those leading spaces.

Michal

Re: RFC: Qemu backup interface plans

[Max Reitz]

On 19.05.21 08:11, Vladimir Sementsov-Ogievskiy wrote:

18.05.2021 19:39, Max Reitz wrote:


[...]


On 17.05.21 14:07, Vladimir Sementsov-Ogievskiy wrote:


[...]

Not also, that there is another benefit of such thing: we'll 
implement this callback in qcow2 driver, so that backup will read 
clusters not in guest cluster order, but in host cluster order, to 
read more sequentially, which should bring better performance on 
rotating disks.


I’m not exactly sure how you envision this to work, but block_status 
also already gives you the host offset in *map.




But block-status doesn't give a possibility to read sequentially. For 
this, user should call block-status several times until the whole disk 
covered, then sort the segments by host offset. I wonder, could it be 
implemented as some iterator, like


read_iter = bdrv_get_sequential_read_iter(source)

while (extents = bdrv_read_next(read_iter)):
   for ext in extents:
     start_writing_task(target, ext.offset, ext.bytes, ext.qiov)

where bdrv_read_next will read guest data in host-cluster-sequence..


How would you implement this, though?  qcow2 doesn’t have a reverse 
mapping either, so it too would need to read all L2 table entries and 
sort them, wouldn’t it?


Max

Re: [PATCH for 7.4.0] conf: Avoid double indentation of element

[Daniel P . Berrangé]

On Tue, May 25, 2021 at 09:32:35AM +0200, Michal Privoznik wrote:
> There was a recent change in libxml2 that caused a trouble for
> us. To us,  in domain or network XMLs are just opaque
> value where management application can store whatever data it
> finds fit. At XML parser/formatter level, we just make a copy of
> the element during parsing and then format it back. For
> formatting we use xmlNodeDump() which allows caller to specify
> level of indentation. Previously, the indentation was not
> applied onto the very first line, but as of v2.9.12-2-g85b1792e
> libxml2 is applying indentation also on the first line.
> 
> This does not work well with out virBuffer because as soon as we
> call virBufferAsprintf() to append  element,
> virBufferAsprintf() will apply another level of indentation.
> 
> Instead of version checking, let's skip any indentation added by
> libxml2 before virBufferAsprintf() is called.
> 
> Note, the problem is only when telling xmlNodeDump() to use
> indentation, i.e. level argument is not zero. Therefore,
> virXMLNodeToString() which also calls xmlNodeDump() is safe as it
> passes zero.
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> I've raised this issue with libxml2 team:
> 
> https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a37f92472e8de5f3f
> 
> but I'm not sure it'll be fixed. Unfortunately, the patch that's causing
> us trouble is being backported all over the place, because it's
> supposedly fixing a regression.
> 
>  src/conf/domain_conf.c  | 9 -
>  src/conf/network_conf.c | 9 -
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4f78b7b43d..84a8c269be 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -27808,6 +27808,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
> *def,
>  
>  if (def->metadata) {
>  g_autoptr(xmlBuffer) xmlbuf = NULL;
> +const char *xmlbufContent = NULL;
>  int oldIndentTreeOutput = xmlIndentTreeOutput;
>  
>  /* Indentation on output requires that we previously set
> @@ -27824,7 +27825,13 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
> *def,
>  xmlIndentTreeOutput = oldIndentTreeOutput;
>  return -1;
>  }
> -virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
> +
> +/* After libxml2-v2.9.12-2-g85b1792e even the first line is indented.
> + * But virBufferAsprintf() also adds indentation. Skip one of them. 
> */
> +xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
> +virSkipSpaces();
> +
> +virBufferAsprintf(buf, "%s\n", xmlbufContent);

Won't this mean that indent is now wrong for the older libxml ?


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 0/4] adjust max memlock limit when hotplugging a vDPA device

[Michal Prívozník]

On 5/21/21 9:11 PM, Laine Stump wrote:
> see patch 4 for the why.
> 
> Found during testing of https://bugzilla.redhat.com/1939776
> 
> Laine Stump (4):
>   qemu_hotplug.c: don't skip cleanup on failures of
> qemuDomainAttachNetDevice
>   conf: new function virDomainNetRemoveByObj()
>   qemu_hotplug.c: add net devices to the domain list earlier
>   qemu: adjust the maxmemlock limit when hotplugging a vDPA device
> 
>  src/conf/domain_conf.c   | 16 
>  src/conf/domain_conf.h   |  1 +
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_hotplug.c  | 41 +++-
>  4 files changed, 50 insertions(+), 9 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal

[PATCH RESEND v3 2/8] qemu_conf: use virFindFileInPathFull for runtime binaries

[Pavel Hrdina]

Following patches will stop detecting the full path during compilation
so we will need to do it here.

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_conf.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 916a3d36ee..437b3ce2be 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -109,6 +109,12 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool 
privileged,
   const char *root)
 {
 g_autoptr(virQEMUDriverConfig) cfg = NULL;
+const char *qemu_extra_paths[] = {
+"/usr/libexec",
+"/usr/lib/qemu",
+"/usr/lib",
+NULL,
+};
 
 if (virQEMUConfigInitialize() < 0)
 return NULL;
@@ -267,10 +273,14 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool 
privileged,
 return NULL;
 }
 
-cfg->bridgeHelperName = g_strdup(QEMU_BRIDGE_HELPER);
-cfg->prHelperName = g_strdup(QEMU_PR_HELPER);
-cfg->slirpHelperName = g_strdup(QEMU_SLIRP_HELPER);
-cfg->dbusDaemonName = g_strdup(QEMU_DBUS_DAEMON);
+cfg->bridgeHelperName = virFindFileInPathFull(QEMU_BRIDGE_HELPER,
+  (GStrv) qemu_extra_paths);
+cfg->prHelperName = virFindFileInPathFull(QEMU_PR_HELPER,
+  (GStrv) qemu_extra_paths);
+cfg->slirpHelperName = virFindFileInPathFull(QEMU_SLIRP_HELPER,
+ (GStrv) qemu_extra_paths);
+cfg->dbusDaemonName = virFindFileInPathFull(QEMU_DBUS_DAEMON,
+(GStrv) qemu_extra_paths);
 
 cfg->securityDefaultConfined = true;
 cfg->securityRequireConfined = false;
-- 
2.31.1

[PATCH RESEND v3 6/8] meson: use runtime binaries to only resolve features with "auto" value

[Pavel Hrdina]

This way meson will try to do the right thing by default but we will
allow users to change this behavior by using -Dname=enabled. This comes
with two benefits compared to the previous behavior:

  - no need to install the binaries if developers would like to check
that the code compiles correctly

  - package maintainers can drop some build dependencies

Signed-off-by: Pavel Hrdina 
---
 meson.build | 102 
 1 file changed, 63 insertions(+), 39 deletions(-)

diff --git a/meson.build b/meson.build
index 2900b6e1a8..8e8d31953d 100644
--- a/meson.build
+++ b/meson.build
@@ -977,12 +977,6 @@ endif
 
 libparted_version = '1.8.0'
 libparted_dep = dependency('libparted', version: '>=' + libparted_version, 
required: false)
-if libparted_dep.found()
-  parted_prog = find_program('parted', required: false, dirs: 
libvirt_sbin_path)
-  if not parted_prog.found()
-libparted_dep = dependency('', required: false)
-  endif
-endif
 
 libpcap_version = '1.5.0'
 if not get_option('libpcap').disabled()
@@ -1420,11 +1414,17 @@ if not get_option('driver_libvirtd').disabled()
 endif
 
 if not get_option('driver_bhyve').disabled() and host_machine.system() == 
'freebsd'
-  bhyve_prog = find_program('bhyve', required: get_option('driver_bhyve'))
-  bhyvectl_prog = find_program('bhyvectl', required: 
get_option('driver_bhyve'))
-  bhyveload_prog = find_program('bhyveload', required: 
get_option('driver_bhyve'))
+  bhyve_enable = true
 
-  if bhyve_prog.found() and bhyvectl_prog.found() and bhyveload_prog.found()
+  if get_option('driver_bhyve').auto()
+bhyve_prog = find_program('bhyve', required: false)
+bhyvectl_prog = find_program('bhyvectl', required: false)
+bhyveload_prog = find_program('bhyveload', required: false)
+
+bhyve_enable = bhyve_prog.found() and bhyvectl_prog.found() and 
bhyveload_prog.found()
+  endif
+
+  if bhyve_enable
 conf.set('WITH_BHYVE', 1)
   endif
 elif get_option('driver_bhyve').enabled()
@@ -1680,8 +1680,17 @@ if conf.has('WITH_LIBVIRTD')
   endif
 
   if not get_option('storage_disk').disabled() and devmapper_dep.found() and 
libparted_dep.found()
-use_storage = true
-conf.set('WITH_STORAGE_DISK', 1)
+disk_enable = true
+
+if get_option('storage_disk').auto()
+  parted_prog = find_program('parted', required: false, dirs: 
libvirt_sbin_path)
+  disk_enable = parted_prog.found()
+endif
+
+if disk_enable
+  use_storage = true
+  conf.set('WITH_STORAGE_DISK', 1)
+endif
   elif get_option('storage_disk').enabled()
 error('You must install libparted and libdevmapper to compile libvirt with 
disk storage driver')
   endif
@@ -1702,10 +1711,10 @@ if conf.has('WITH_LIBVIRTD')
   endif
 endif
 
-if fs_enable
-  mount_prog = find_program('mount', required: get_option('storage_fs'), 
dirs: libvirt_sbin_path)
-  umount_prog = find_program('umount', required: get_option('storage_fs'), 
dirs: libvirt_sbin_path)
-  mkfs_prog = find_program('mkfs', required: get_option('storage_fs'), 
dirs: libvirt_sbin_path)
+if fs_enable and get_option('storage_fs').auto()
+  mount_prog = find_program('mount', required: false, dirs: 
libvirt_sbin_path)
+  umount_prog = find_program('umount', required: false, dirs: 
libvirt_sbin_path)
+  mkfs_prog = find_program('mkfs', required: false, dirs: 
libvirt_sbin_path)
 
   if not mount_prog.found() or not umount_prog.found() or not 
mkfs_prog.found()
 fs_enable = false
@@ -1727,14 +1736,11 @@ if conf.has('WITH_LIBVIRTD')
 
   if not get_option('storage_iscsi').disabled()
 iscsi_enable = true
-iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
libvirt_sbin_path)
 
-if not iscsiadm_prog.found()
-  if get_option('storage_iscsi').enabled()
-error('We need iscsiadm for iSCSI storage driver')
-  else
-iscsi_enable = false
-  endif
+if get_option('storage_iscsi').auto()
+  iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
libvirt_sbin_path)
+
+  iscsi_enable = iscsiadm_prog.found()
 endif
 
 if iscsi_enable
@@ -1758,12 +1764,15 @@ if conf.has('WITH_LIBVIRTD')
   'lvchange', 'vgchange', 'vgscan',
   'pvs', 'vgs', 'lvs',
 ]
-foreach name : lvm_progs
-  lvm_prog = find_program(name, required: get_option('storage_lvm'), dirs: 
libvirt_sbin_path)
-  if not lvm_prog.found()
-lvm_enable = false
-  endif
-endforeach
+
+if get_option('storage_lvm').auto()
+  foreach name : lvm_progs
+lvm_prog = find_program(name, required: false, dirs: libvirt_sbin_path)
+if not lvm_prog.found()
+  lvm_enable = false
+endif
+  endforeach
+endif
 
 if lvm_enable
   use_storage = true
@@ -1791,9 +1800,15 @@ if conf.has('WITH_LIBVIRTD')
   endif
 
   if not get_option('storage_sheepdog').disabled()
-sheepdogcli_prog = find_program('dog', required: 

[PATCH RESEND v3 4/8] meson: move iscsiadm check into storage_iscsi condition

[Pavel Hrdina]

This requires to define the binary name in code because we compile
src/util/viriscsi.c unconditionally.

Signed-off-by: Pavel Hrdina 
---
 meson.build | 22 --
 src/util/viriscsi.h |  2 ++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index ed4afd2052..7eeaa689f9 100644
--- a/meson.build
+++ b/meson.build
@@ -824,7 +824,6 @@ endforeach
 optional_programs = [
   'augparse',
   'flake8',
-  'iscsiadm',
   'pdwtags',
 ]
 
@@ -1742,11 +1741,22 @@ if conf.has('WITH_LIBVIRTD')
 error('Need glusterfs (libgfapi) for gluster storage driver')
   endif
 
-  if not get_option('storage_iscsi').disabled() and iscsiadm_prog.found()
-use_storage = true
-conf.set('WITH_STORAGE_ISCSI', 1)
-  elif get_option('storage_iscsi').enabled()
-error('We need iscsiadm for iSCSI storage driver')
+  if not get_option('storage_iscsi').disabled()
+iscsi_enable = true
+iscsiadm_prog = find_program('iscsiadm', required: false, dirs: 
libvirt_sbin_path)
+
+if not iscsiadm_prog.found()
+  if get_option('storage_iscsi').enabled()
+error('We need iscsiadm for iSCSI storage driver')
+  else
+iscsi_enable = false
+  endif
+endif
+
+if iscsi_enable
+  use_storage = true
+  conf.set('WITH_STORAGE_ISCSI', 1)
+endif
   endif
 
   if not get_option('storage_iscsi_direct').disabled() and libiscsi_dep.found()
diff --git a/src/util/viriscsi.h b/src/util/viriscsi.h
index 6d17d139eb..fa7ff280c2 100644
--- a/src/util/viriscsi.h
+++ b/src/util/viriscsi.h
@@ -23,6 +23,8 @@
 
 #include "internal.h"
 
+#define ISCSIADM "iscsiadm"
+
 char *
 virISCSIGetSession(const char *devpath,
bool probe)
-- 
2.31.1

[PATCH RESEND v3 7/8] meson: optional_programs should be used only for building libvirt

[Pavel Hrdina]

Drop code that creates defines with program paths and update the comment
to reflect current usage of optional_programs.

Signed-off-by: Pavel Hrdina 
---
 meson.build | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/meson.build b/meson.build
index 8e8d31953d..1c0de2f408 100644
--- a/meson.build
+++ b/meson.build
@@ -819,7 +819,7 @@ foreach item : required_programs_groups
 endforeach
 
 
-# optional programs
+# optional programs used while building libvirt
 
 optional_programs = [
   'augparse',
@@ -830,14 +830,6 @@ optional_programs = [
 foreach name : optional_programs
   prog = find_program(name, required: false, dirs: libvirt_sbin_path)
   varname = name.underscorify()
-  if prog.found()
-prog_path = prog.path()
-  else
-prog_path = name
-  endif
-
-  conf.set_quoted(varname.to_upper(), prog_path)
-  conf.set_quoted('@0@_PATH'.format(varname.to_upper()), prog_path)
   set_variable('@0@_prog'.format(varname), prog)
 endforeach
 
-- 
2.31.1

[PATCH RESEND v3 8/8] libvirt.spec: drop no longer required build dependencies

[Pavel Hrdina]

These are no longer required to build libvirt as they are used during
compilation only by meson to detect if some "auto" features should be
enabled or not but in spec file we explicitly enable/disable all libvirt
features.

Signed-off-by: Pavel Hrdina 
---
 libvirt.spec.in | 31 ---
 1 file changed, 31 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f421828d16..a49cd58e38 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -260,10 +260,6 @@ BuildRequires: sanlock-devel >= 2.4
 BuildRequires: libpcap-devel >= 1.5.0
 BuildRequires: libnl3-devel
 BuildRequires: libselinux-devel
-BuildRequires: dnsmasq >= 2.41
-BuildRequires: iptables
-BuildRequires: radvd
-BuildRequires: ebtables
 BuildRequires: module-init-tools
 BuildRequires: cyrus-sasl-devel
 BuildRequires: polkit >= 0.112
@@ -272,13 +268,7 @@ BuildRequires: util-linux
 %if %{with_qemu}
 # For managing ACLs
 BuildRequires: libacl-devel
-# From QEMU RPMs
-BuildRequires: /usr/bin/qemu-img
 %endif
-# For LVM drivers
-BuildRequires: lvm2
-# For pool type=iscsi
-BuildRequires: iscsi-initiator-utils
 %if %{with_storage_iscsi_direct}
 # For pool type=iscsi-direct
 BuildRequires: libiscsi-devel
@@ -297,15 +287,6 @@ BuildRequires: librbd-devel
 BuildRequires: glusterfs-api-devel >= 3.4.1
 BuildRequires: glusterfs-devel >= 3.4.1
 %endif
-%if %{with_storage_sheepdog}
-BuildRequires: sheepdog
-%endif
-%if %{with_storage_zfs}
-# Support any conforming implementation of zfs. On stock Fedora
-# this is zfs-fuse, but could be zfsonlinux upstream RPMs
-BuildRequires: /sbin/zfs
-BuildRequires: /sbin/zpool
-%endif
 %if %{with_numactl}
 # For QEMU/LXC numa info
 BuildRequires: numactl-devel
@@ -330,21 +311,9 @@ BuildRequires: audit-libs-devel
 # we need /usr/sbin/dtrace
 BuildRequires: systemtap-sdt-devel
 
-# For mount/umount in FS driver
-BuildRequires: util-linux
-# For showmount in FS driver (netfs discovery)
-BuildRequires: nfs-utils
-
 # Fedora build root suckage
 BuildRequires: gawk
 
-# For storage wiping with different algorithms
-BuildRequires: scrub
-
-%if %{with_numad}
-BuildRequires: numad
-%endif
-
 %if %{with_wireshark}
 BuildRequires: wireshark-devel
 %endif
-- 
2.31.1

[PATCH RESEND v3 5/8] meson: stop setting runtime binaries defines during compilation

[Pavel Hrdina]

Technically the location of these binaries may be different when
compiling libvirt or running it. This will allow users to change $PATH
to use different binaries as well.

Signed-off-by: Pavel Hrdina 
---
 meson.build| 41 --
 src/bhyve/bhyve_command.c  |  4 +++
 src/locking/lock_driver_lockd.c| 12 ++--
 src/storage/storage_backend_logical.c  | 13 
 src/storage/storage_backend_sheepdog.c |  2 ++
 src/storage/storage_backend_zfs.c  |  3 ++
 src/storage/storage_util.h |  6 
 src/util/virnuma.c |  1 +
 8 files changed, 36 insertions(+), 46 deletions(-)

diff --git a/meson.build b/meson.build
index 7eeaa689f9..2900b6e1a8 100644
--- a/meson.build
+++ b/meson.build
@@ -979,9 +979,7 @@ libparted_version = '1.8.0'
 libparted_dep = dependency('libparted', version: '>=' + libparted_version, 
required: false)
 if libparted_dep.found()
   parted_prog = find_program('parted', required: false, dirs: 
libvirt_sbin_path)
-  if parted_prog.found()
-conf.set_quoted('PARTED', parted_prog.path())
-  else
+  if not parted_prog.found()
 libparted_dep = dependency('', required: false)
   endif
 endif
@@ -1428,9 +1426,6 @@ if not get_option('driver_bhyve').disabled() and 
host_machine.system() == 'freeb
 
   if bhyve_prog.found() and bhyvectl_prog.found() and bhyveload_prog.found()
 conf.set('WITH_BHYVE', 1)
-conf.set_quoted('BHYVE', bhyve_prog.path())
-conf.set_quoted('BHYVECTL', bhyvectl_prog.path())
-conf.set_quoted('BHYVELOAD', bhyveload_prog.path())
   endif
 elif get_option('driver_bhyve').enabled()
   error('The bhyve driver cannot be enabled')
@@ -1719,18 +1714,7 @@ if conf.has('WITH_LIBVIRTD')
 
 if fs_enable
   use_storage = true
-
   conf.set('WITH_STORAGE_FS', 1)
-  conf.set_quoted('MOUNT', mount_prog.path())
-  conf.set_quoted('UMOUNT', umount_prog.path())
-  conf.set_quoted('MKFS', mkfs_prog.path())
-
-  showmount_prog = find_program('showmount', required: false, dirs: 
libvirt_sbin_path)
-  showmount_path = ''
-  if showmount_prog.found()
-showmount_path = showmount_prog.path()
-  endif
-  conf.set_quoted('SHOWMOUNT', showmount_path)
 endif
   endif
 
@@ -1775,11 +1759,8 @@ if conf.has('WITH_LIBVIRTD')
   'pvs', 'vgs', 'lvs',
 ]
 foreach name : lvm_progs
-  set_variable(
-'@0@_prog'.format(name),
-find_program(name, required: get_option('storage_lvm'), dirs: 
libvirt_sbin_path)
-  )
-  if not get_variable('@0@_prog'.format(name)).found()
+  lvm_prog = find_program(name, required: get_option('storage_lvm'), dirs: 
libvirt_sbin_path)
+  if not lvm_prog.found()
 lvm_enable = false
   endif
 endforeach
@@ -1787,10 +1768,6 @@ if conf.has('WITH_LIBVIRTD')
 if lvm_enable
   use_storage = true
   conf.set('WITH_STORAGE_LVM', 1)
-
-  foreach name : lvm_progs
-conf.set_quoted(name.to_upper(), 
get_variable('@0@_prog'.format(name)).path())
-  endforeach
 endif
   endif
 
@@ -1819,7 +1796,6 @@ if conf.has('WITH_LIBVIRTD')
 if sheepdogcli_prog.found()
   use_storage = true
   conf.set('WITH_STORAGE_SHEEPDOG', 1)
-  conf.set_quoted('SHEEPDOGCLI', sheepdogcli_prog.path())
 endif
   endif
 
@@ -1842,11 +1818,8 @@ if conf.has('WITH_LIBVIRTD')
   if not get_option('storage_zfs').disabled()
 zfs_enable = true
 foreach name : ['zfs', 'zpool']
-  set_variable(
-'@0@_prog'.format(name),
-find_program(name, required: get_option('storage_zfs'), dirs: 
libvirt_sbin_path)
-  )
-  if not get_variable('@0@_prog'.format(name)).found()
+  zfs_prog = find_program(name, required: get_option('storage_zfs'), dirs: 
libvirt_sbin_path)
+  if not zfs_prog.found()
 zfs_enable = false
   endif
 endforeach
@@ -1854,9 +1827,6 @@ if conf.has('WITH_LIBVIRTD')
 if zfs_enable
   use_storage = true
   conf.set('WITH_STORAGE_ZFS', 1)
-  foreach name : ['zfs', 'zpool']
-conf.set_quoted(name.to_upper(), 
get_variable('@0@_prog'.format(name)).path())
-  endforeach
 endif
   endif
 endif
@@ -1970,7 +1940,6 @@ if not get_option('numad').disabled() and 
numactl_dep.found()
   numad_prog = find_program('numad', required: get_option('numad'), dirs: 
libvirt_sbin_path)
   if numad_prog.found()
 conf.set('WITH_NUMAD', 1)
-conf.set_quoted('NUMAD', numad_prog.path())
   endif
 elif get_option('numad').enabled()
   error('You must have numactl enabled for numad support.')
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index ab9d3026cc..71293e039f 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -37,6 +37,10 @@
 
 #define VIR_FROM_THIS VIR_FROM_BHYVE
 
+#define BHYVE "bhyve"
+#define BHYVECTL "bhyvectl"
+#define BHYVELOAD "bhyveload"
+
 VIR_LOG_INIT("bhyve.bhyve_command");
 
 static int
diff --git 

[PATCH RESEND v3 1/8] virfile: introduce virFindFileInPathFull()

[Pavel Hrdina]

Extend virFindFileInPath to search in custom extra paths as well. Some
binaries that libvirt needs are not usually in $PATH so we need to have
a way to look for these as well.

Signed-off-by: Pavel Hrdina 
---
 src/libvirt_private.syms |  2 +-
 src/util/virfile.c   | 16 ++--
 src/util/virfile.h   |  5 -
 tests/testutilsqemu.c|  3 ++-
 tests/virfirewallmock.c  |  3 ++-
 5 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e1aef5267e..2ab43d5ffb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2259,7 +2259,7 @@ virFileWrapperFdClose;
 virFileWrapperFdFree;
 virFileWrapperFdNew;
 virFileWriteStr;
-virFindFileInPath;
+virFindFileInPathFull;
 
 
 # util/virfilecache.h
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 03a7725dd3..7922fda2e5 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1654,13 +1654,15 @@ virFileIsLink(const char *linkpath)
 }
 
 /*
- * Finds a requested executable file in the PATH env. e.g.:
+ * Finds a requested executable file in the paths provided by @extra_paths
+ * argument or in PATH env. e.g.:
  * "qemu-img" will return "/usr/bin/qemu-img"
  *
  * You must free the result
  */
 char *
-virFindFileInPath(const char *file)
+virFindFileInPathFull(const char *file,
+  const GStrv extra_paths)
 {
 const char *origpath = NULL;
 g_auto(GStrv) paths = NULL;
@@ -1692,6 +1694,16 @@ virFindFileInPath(const char *file)
 return abspath;
 }
 
+/* First search in paths provided by caller.
+ */
+if (extra_paths) {
+for (pathiter = extra_paths; *pathiter; pathiter++) {
+g_autofree char *fullpath = g_strdup_printf("%s/%s", *pathiter, 
file);
+if (virFileIsExecutable(fullpath))
+return g_steal_pointer();
+}
+}
+
 /* copy PATH env so we can tweak it */
 origpath = getenv("PATH");
 if (!origpath)
diff --git a/src/util/virfile.h b/src/util/virfile.h
index b9f7b1766f..eee27c2efc 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -184,9 +184,12 @@ int virFileResolveAllLinks(const char *linkpath,
 int virFileIsLink(const char *linkpath)
 ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT;
 
-char *virFindFileInPath(const char *file)
+char *virFindFileInPathFull(const char *file,
+const GStrv extra_paths)
 G_GNUC_NO_INLINE;
 
+#define virFindFileInPath(file) virFindFileInPathFull(file, NULL)
+
 char *virFileFindResource(const char *filename,
   const char *builddir,
   const char *installdir)
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 1444abc401..1ceceef4a7 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -118,7 +118,8 @@ static const char *qemu_default_ram_id[VIR_ARCH_LAST] = {
 };
 
 char *
-virFindFileInPath(const char *file)
+virFindFileInPathFull(const char *file,
+  const GStrv extra_args G_GNUC_UNUSED)
 {
 if (g_str_has_prefix(file, "qemu-system") ||
 g_str_equal(file, "qemu-kvm")) {
diff --git a/tests/virfirewallmock.c b/tests/virfirewallmock.c
index 6b096701c9..a047e56bd9 100644
--- a/tests/virfirewallmock.c
+++ b/tests/virfirewallmock.c
@@ -20,7 +20,8 @@
 #include "virfile.h"
 
 char *
-virFindFileInPath(const char *file)
+virFindFileInPathFull(const char *file,
+  const GStrv extra_args G_GNUC_UNUSED)
 {
 if (file &&
 (g_strrstr(file, "ebtables") ||
-- 
2.31.1

[PATCH RESEND v3 0/8] cleanup meson checks for runtime binaries

[Pavel Hrdina]

Recent attempt to add a lot of meson options to specify different
runtime paths motivated me enough to cleanup this from meson.
 
Changes in v3:   
- some patches were already pushed   
- removed patch that moved virFindFileInPath from testutilsqemu.c
 
Changes in v2:   
- split and rework patch 16/17 to address review comments
- added a new patch to cleanup libvirt.spec.in file

Pavel Hrdina (8):
  virfile: introduce virFindFileInPathFull()
  qemu_conf: use virFindFileInPathFull for runtime binaries
  meson: drop check for runtime binary dependencies
  meson: move iscsiadm check into storage_iscsi condition
  meson: stop setting runtime binaries defines during compilation
  meson: use runtime binaries to only resolve features with "auto" value
  meson: optional_programs should be used only for building libvirt
  libvirt.spec: drop no longer required build dependencies

 libvirt.spec.in|  31 
 meson.build| 214 +
 src/bhyve/bhyve_command.c  |   4 +
 src/libvirt_private.syms   |   2 +-
 src/locking/lock_driver_lockd.c|  12 +-
 src/network/bridge_driver.c|   2 +
 src/node_device/node_device_driver.c   |   2 +
 src/qemu/qemu_conf.c   |  23 ++-
 src/storage/storage_backend_logical.c  |  13 ++
 src/storage/storage_backend_sheepdog.c |   2 +
 src/storage/storage_backend_zfs.c  |   3 +
 src/storage/storage_util.c |   2 +
 src/storage/storage_util.h |   6 +
 src/util/virdnsmasq.c  |   1 +
 src/util/virfile.c |  16 +-
 src/util/virfile.h |   5 +-
 src/util/virfirewall.h |   4 +
 src/util/viriscsi.h|   2 +
 src/util/virkmod.h |   3 +
 src/util/virnetdevbandwidth.h  |   2 +
 src/util/virnetdevip.c |   2 +
 src/util/virnetdevmidonet.c|   2 +
 src/util/virnetdevopenvswitch.c|   2 +
 src/util/virnuma.c |   1 +
 src/util/virsysinfo.c  |   1 +
 src/util/virutil.c |   2 +
 tests/testutilsqemu.c  |   3 +-
 tests/virfirewallmock.c|   3 +-
 28 files changed, 173 insertions(+), 192 deletions(-)

-- 
2.31.1

[PATCH RESEND v3 3/8] meson: drop check for runtime binary dependencies

[Pavel Hrdina]

These binaries are used only during runtime so technically there is no
need to check for them while compiling libvirt.

Usually the location is the same while compiling and running but it may
not be true. In addition they are not strictly required to compile the
code so this way developers don't have to install it or create fake
binaries in order to compile the code.

Signed-off-by: Pavel Hrdina 
---
 meson.build  | 63 
 src/network/bridge_driver.c  |  2 +
 src/node_device/node_device_driver.c |  2 +
 src/qemu/qemu_conf.c |  5 +++
 src/storage/storage_util.c   |  2 +
 src/util/virdnsmasq.c|  1 +
 src/util/virfirewall.h   |  4 ++
 src/util/virkmod.h   |  3 ++
 src/util/virnetdevbandwidth.h|  2 +
 src/util/virnetdevip.c   |  2 +
 src/util/virnetdevmidonet.c  |  2 +
 src/util/virnetdevopenvswitch.c  |  2 +
 src/util/virsysinfo.c|  1 +
 src/util/virutil.c   |  2 +
 14 files changed, 30 insertions(+), 63 deletions(-)

diff --git a/meson.build b/meson.build
index 4f23f9104e..ed4afd2052 100644
--- a/meson.build
+++ b/meson.build
@@ -823,24 +823,9 @@ endforeach
 
 optional_programs = [
   'augparse',
-  'dmidecode',
-  'dnsmasq',
-  'ebtables',
   'flake8',
-  'ip',
-  'ip6tables',
-  'iptables',
   'iscsiadm',
-  'mdevctl',
-  'mm-ctl',
-  'modprobe',
-  'ovs-vsctl',
   'pdwtags',
-  'radvd',
-  'rmmod',
-  'scrub',
-  'tc',
-  'udevadm',
 ]
 
 foreach name : optional_programs
@@ -1646,54 +1631,6 @@ if not get_option('driver_qemu').disabled()
 endif
 conf.set_quoted('QEMU_USER', qemu_user)
 conf.set_quoted('QEMU_GROUP', qemu_group)
-
-qemu_bridge_prog = find_program(
-  'qemu-bridge-helper',
-  dirs: [ '/usr/libexec', '/usr/lib/qemu', '/usr/lib' ],
-  required: false
-)
-if qemu_bridge_prog.found()
-  qemu_bridge_path = qemu_bridge_prog.path()
-else
-  qemu_bridge_path = '/usr/libexec/qemu-bridge-helper'
-endif
-conf.set_quoted('QEMU_BRIDGE_HELPER', qemu_bridge_path)
-
-qemu_pr_prog = find_program(
-  'qemu-pr-helper',
-  dirs: [ '/usr/bin', '/usr/libexec' ],
-  required: false
-)
-if qemu_pr_prog.found()
-  qemu_pr_path = qemu_pr_prog.path()
-else
-  qemu_pr_path = '/usr/bin/qemu-pr-helper'
-endif
-conf.set_quoted('QEMU_PR_HELPER', qemu_pr_path)
-
-qemu_slirp_prog = find_program(
-  'slirp-helper',
-  dirs: [ '/usr/bin', '/usr/libexec' ],
-  required: false
-)
-if qemu_slirp_prog.found()
-  qemu_slirp_path = qemu_slirp_prog.path()
-else
-  qemu_slirp_path = '/usr/bin/slirp-helper'
-endif
-conf.set_quoted('QEMU_SLIRP_HELPER', qemu_slirp_path)
-
-qemu_dbus_daemon_prog = find_program(
-  'dbus-daemon',
-  dirs: [ '/usr/bin', '/usr/libexec' ],
-  required: false
-)
-if qemu_dbus_daemon_prog.found()
-  qemu_dbus_daemon_path = qemu_dbus_daemon_prog.path()
-else
-  qemu_dbus_daemon_path = '/usr/bin/dbus-daemon'
-endif
-conf.set_quoted('QEMU_DBUS_DAEMON', qemu_dbus_daemon_path)
   endif
 endif
 
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a711b34c48..81d0c06ec5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -74,6 +74,8 @@
 #define VIR_FROM_THIS VIR_FROM_NETWORK
 #define MAX_BRIDGE_ID 256
 
+#define RADVD "radvd"
+
 static virMutex bridgeNameValidateMutex = VIR_MUTEX_INITIALIZER;
 
 /**
diff --git a/src/node_device/node_device_driver.c 
b/src/node_device/node_device_driver.c
index a9f605715b..72edbc3fa0 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -48,6 +48,8 @@
 
 VIR_LOG_INIT("node_device.node_device_driver");
 
+#define MDEVCTL "mdevctl"
+
 virNodeDeviceDriverState *driver;
 
 
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 437b3ce2be..d228504d8c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -55,6 +55,11 @@
 
 VIR_LOG_INIT("qemu.qemu_conf");
 
+#define QEMU_BRIDGE_HELPER "qemu-bridge-helper"
+#define QEMU_PR_HELPER "qemu-pr-helper"
+#define QEMU_SLIRP_HELPER "slirp-helper"
+#define QEMU_DBUS_DAEMON "dbus-daemon"
+
 /* These are only defaults, they can be changed now in qemu.conf and
  * explicitly specified port is checked against these two (makes
  * sense to limit the values).
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index a7c9355bf9..b43ea7b757 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -85,6 +85,8 @@ VIR_LOG_INIT("storage.storage_util");
 # define S_IRWXUGO (S_IRWXU | S_IRWXG | S_IRWXO)
 #endif
 
+#define SCRUB "scrub"
+
 /* virStorageBackendNamespaceInit:
  * @poolType: virStoragePoolType
  * @xmlns: Storage Pool specific namespace callback methods
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index 

[PATCH for 7.4.0] conf: Avoid double indentation of element

[Michal Privoznik]

There was a recent change in libxml2 that caused a trouble for
us. To us,  in domain or network XMLs are just opaque
value where management application can store whatever data it
finds fit. At XML parser/formatter level, we just make a copy of
the element during parsing and then format it back. For
formatting we use xmlNodeDump() which allows caller to specify
level of indentation. Previously, the indentation was not
applied onto the very first line, but as of v2.9.12-2-g85b1792e
libxml2 is applying indentation also on the first line.

This does not work well with out virBuffer because as soon as we
call virBufferAsprintf() to append  element,
virBufferAsprintf() will apply another level of indentation.

Instead of version checking, let's skip any indentation added by
libxml2 before virBufferAsprintf() is called.

Note, the problem is only when telling xmlNodeDump() to use
indentation, i.e. level argument is not zero. Therefore,
virXMLNodeToString() which also calls xmlNodeDump() is safe as it
passes zero.

Signed-off-by: Michal Privoznik 
---

I've raised this issue with libxml2 team:

https://gitlab.gnome.org/GNOME/libxml2/-/commit/85b1792e37b131e7a51af98a37f92472e8de5f3f

but I'm not sure it'll be fixed. Unfortunately, the patch that's causing
us trouble is being backported all over the place, because it's
supposedly fixing a regression.

 src/conf/domain_conf.c  | 9 -
 src/conf/network_conf.c | 9 -
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4f78b7b43d..84a8c269be 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27808,6 +27808,7 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
 
 if (def->metadata) {
 g_autoptr(xmlBuffer) xmlbuf = NULL;
+const char *xmlbufContent = NULL;
 int oldIndentTreeOutput = xmlIndentTreeOutput;
 
 /* Indentation on output requires that we previously set
@@ -27824,7 +27825,13 @@ virDomainDefFormatInternalSetRootName(virDomainDef 
*def,
 xmlIndentTreeOutput = oldIndentTreeOutput;
 return -1;
 }
-virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
+
+/* After libxml2-v2.9.12-2-g85b1792e even the first line is indented.
+ * But virBufferAsprintf() also adds indentation. Skip one of them. */
+xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
+virSkipSpaces();
+
+virBufferAsprintf(buf, "%s\n", xmlbufContent);
 xmlIndentTreeOutput = oldIndentTreeOutput;
 }
 
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index a9eadff29c..20c6dc091a 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2488,6 +2488,7 @@ virNetworkDefFormatBuf(virBuffer *buf,
 
 if (def->metadata) {
 g_autoptr(xmlBuffer) xmlbuf = NULL;
+const char *xmlbufContent = NULL;
 int oldIndentTreeOutput = xmlIndentTreeOutput;
 
 /* Indentation on output requires that we previously set
@@ -2504,7 +2505,13 @@ virNetworkDefFormatBuf(virBuffer *buf,
 xmlIndentTreeOutput = oldIndentTreeOutput;
 return -1;
 }
-virBufferAsprintf(buf, "%s\n", (char *) xmlBufferContent(xmlbuf));
+
+/* After libxml2-v2.9.12-2-g85b1792e even the first line is indented.
+ * But virBufferAsprintf() also adds indentation. Skip one of them. */
+xmlbufContent = (const char *) xmlBufferContent(xmlbuf);
+virSkipSpaces();
+
+virBufferAsprintf(buf, "%s\n", xmlbufContent);
 xmlIndentTreeOutput = oldIndentTreeOutput;
 }
 
-- 
2.26.3