Re: [libvirt] [PATCH 0/2] libvirt-rust: Fixing bugs in Stream::{send, recv}

[Martin Kletzander]

On Thu, Sep 19, 2019 at 05:48:21AM +, Linus Färnstrand wrote:

Hi,

Fixing memory soundness bugs in Stream::recv, and other bugs in both
Stream::recv and Stream::send.

The method Stream::recv takes a size argument that it passes on as the nbytes
argument to virStreamRecv. The problem is that the corresponding data pointer
points to a local stack allocated array with a fixed size of 2048. So calling
Stream::recv with a size larger than 2048 can cause virStreamRecv to write
past the given buffer!

The library calls CStr::from_ptr(data) on the locally allocated buffer after
virStreamRecv returns. CStr::from_ptr will scan the memory from the given
pointer for the terminating null byte. There is no guarantee this buffer will
properly end with a null byte. The buffer is initialized with only nulls.
But since virStreamRecv can overwrite the entire buffer (and beyond!), there
is no guarantee there will be a null byte in the buffer at this point. As a
result, CStr::from_ptr can continue to read past the buffer. This can cause
either reading invalid memory, or it can cause the method to return a string
containing data from the stack way past the data written by virStreamRecv.
Heartbleed style issue.

The code does not check for the -2 error code in the return value of
virStreamRecv, treating it as a success. It looks like it will make it just
return an empty string as result. But a bug nonetheless.

After parsing the buffer as a C string in Stream::recv, it is lossily converted
into a Rust UTF-8 string with CStr::to_string_lossy. It can cause the data to
be modified before it is returned to the caller.
U+FFFD REPLACEMENT CHARACTER will be inserted on invalid UTF-8 codepoints.

The FFI bindings for both virStreamRecv and virStreamSend are wrong. They
specify the nbytes argument as c_int instead of size_t as the C library does.

I don't have access to SMTP on my dev machine. Hope ProtonMail does not
reformat this.



It's fine.  Just for a future reference, we prefer shallow formatting, basically
what I have is:

 git config format.thread shallow
 git config format.coverLetter auto

and then per-project:

 git config format.subjectprefix "libvirt-rust PATCH"


Have a great day
Linus Färnstrand

Linus Färnstrand (2):
 Fix bugs in Stream::recv
 Fix Stream::send

src/stream.rs | 42 +-
1 file changed, 21 insertions(+), 21 deletions(-)

--
2.21.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] libvirt-rust: Fix bugs in Stream::recv

[Martin Kletzander]

On Thu, Sep 19, 2019 at 05:48:37AM +, Linus Färnstrand wrote:

* pass same size to virStreamRecv as the buffer has allocated
* Handle -2 error case
* Fix FFI declaration to take size_t instead of c_uint
* Allow user to pass in buffer. To allow user to decide where to
 allocate it. And to be able to re-use the same buffer
* Don't try to treat binary data as a string

Signed-off-by: Linus Färnstrand 
---
src/stream.rs | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/stream.rs b/src/stream.rs
index 8333ee5..af6c8ec 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -18,6 +18,7 @@

extern crate libc;

+use std::convert::TryFrom;
use std::str;

use error::Error;
@@ -37,7 +38,7 @@ extern "C" {
 -> libc::c_int;
fn virStreamRecv(c: sys::virStreamPtr,
 data: *mut libc::c_char,
- nbytes: libc::c_uint)
+ nbytes: libc::size_t)
 -> libc::c_int;
fn virStreamFree(c: sys::virStreamPtr) -> libc::c_int;
fn virStreamAbort(c: sys::virStreamPtr) -> libc::c_int;
@@ -116,14 +117,14 @@ impl Stream {
}
}

-pub fn recv(&self, size: u32) -> Result {
-unsafe {
-let mut data: [libc::c_char; 2048] = ['\0' as i8; 2048];
-let ret = virStreamRecv(self.as_ptr(), data.as_mut_ptr(), size as 
libc::c_uint);
-if ret == -1 {
-return Err(Error::new());
-}
-return Ok(c_chars_to_string!(data.as_ptr()));
-}
+pub fn recv(&self, buf: &mut [u8]) -> Result {
+let ret = unsafe {
+virStreamRecv(
+self.as_ptr(),
+buf.as_mut_ptr() as *mut libc::c_char,
+buf.len(),
+)
+};
+usize::try_from(ret).map_err(|_| Error::new())


This way it is impossible to distinguish -1 and -2.  We could get a bit closer
to std::io::Read if -2 gets transformed to 0, as we haven't read anything.

Other than that it's good.


}
}
--
2.21.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] qemu_domain_address.c: use VIR_AUTOFREE() in strings

[Erik Skultety]

On Thu, Sep 19, 2019 at 05:04:46PM -0300, Daniel Henrique Barboza wrote:
> A few 'cleanup' labels gone after using VIR_AUTOFREE() in the
> strings.

s/in the strings/on the @addrStr variable

Reviewed-by: Erik Skultety 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] libvirt-rust Fix Stream::send

[Martin Kletzander]

On Thu, Sep 19, 2019 at 05:48:49AM +, Linus Färnstrand wrote:

* Handle the -2 error case
* Allow sending arbitrary byte array, not just UTF-8 strings
* Fix FFI declaration, takes size_t, not c_uint
* Return usize to be more idiomatic (type used for slice indexing)

Signed-off-by: Linus Färnstrand 
---
src/stream.rs | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/stream.rs b/src/stream.rs
index af6c8ec..2ca5d0b 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -34,7 +34,7 @@ pub mod sys {
extern "C" {
fn virStreamSend(c: sys::virStreamPtr,
 data: *const libc::c_char,
- nbytes: libc::c_uint)
+ nbytes: libc::size_t)
 -> libc::c_int;
fn virStreamRecv(c: sys::virStreamPtr,
 data: *mut libc::c_char,
@@ -105,16 +105,15 @@ impl Stream {
}
}

-pub fn send(&self, data: &str) -> Result {
-unsafe {
-let ret = virStreamSend(self.as_ptr(),
-string_to_c_chars!(data),
-data.len() as libc::c_uint);
-if ret == -1 {
-return Err(Error::new());
-}
-return Ok(ret as u32);
-}
+pub fn send(&self, data: &[u8]) -> Result {
+let ret = unsafe {
+virStreamSend(
+self.as_ptr(),
+data.as_ptr() as *mut libc::c_char,
+data.len()
+)
+};
+usize::try_from(ret).map_err(|_| Error::new())


I guess same as the comment form the first patch should be applied here, but we
might need to even change the return value so that it is properly distinguished
as here it has yet another meaning.  Are you basing this on some of your work on
top of libvirt-rust?  Is this the easy to use (semi-)properly?  If yes, we might
just as well keep it this way as it is still way better than what we have now.


}

pub fn recv(&self, buf: &mut [u8]) -> Result {
--
2.21.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v5 00/15] CPU Model Baseline and Comparison for s390x

[David Hildenbrand]

On 19.09.19 22:24, Collin Walling wrote:
> Note: since I've made some changes to a lot of these patches / split
> up some patches, I've decided to hold off on adding any r-b's in case
> there is a specific change that someone does not agree with.
> 
> Changelog:
> 
> - Properly refactored code from CPU model expansion function
> - Introduced a cleanup patch for CPU model expansion function
> - Introduced patches that modifies the refactored code to suit
> needs for baseline/comparison
> - CPU expansion function now accepts a virCPUDefPtr
> - Removed props parsing from CPU model comparison (they weren't
> used)
> - Cleaner error reporting when baselining/comparing with erroneous
> CPU models / features
> - Various cleanups based on feedback
> ___
> 
> To run these patches, execute the virsh hypervisor-cpu-compare or 
> hypervisor-cpu-baseline commands and pass an XML file describing one or 
> more CPU definition. You can use the definition from virsh domcapabilities 
> or from a guest XML. There is no need extract it from the file and place 
> it a new one, as the XML parser will look specifically for the CPU tags.
> ___
> 
> These patches hookup the virsh hypervisor-cpu-compare/baseline commands 
> for the s390x architecture. They take an XML file describing some CPU 
> definitions and passes the data to QEMU, where the actual CPU model 
> comparison / baseline calculation is handled (available since QEMU 2.8.5).
> 
> When baselining CPU models with the --features argument, s390x will report
> a full CPU model expansion.
> 
> Thanks.

Nice to see this make progress after all these years I integrated
support into QEMU :)

-- 

Thanks,

David / dhildenb

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Pavel Hrdina]

Meson build system is simple and quick compared to Autotools and it's
able to fully replace our Autotools usage.  There are few drawbacks as
it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
it's still evolving and the user base is not that large and there were
some tweaks required to achieve the same functionality.

However, there are benefits, the configure and build time is way shorter
and build definition files are more readable and easier to maintain.

There are some major changes with Meson build system:

- there is no syntax-check target, the syntax-check is part of Meson
  test suite but it's still possible to run it separately,

- Meson forces separation between source and build directories

Signed-off-by: Pavel Hrdina 
Tested-by: Ján Tomko 
---

Notes:
changes in v2:

- add -Werror if we are building from git
- fixed -Wframe-larger-than
- removed unrelated fix
- added comment for flake8 ignore warning
- added 'suite' labels 'syntax' and 'unit' for tests
- AUTHORS and libvirt-dbus.spec are generated only when building from 
git
- run.in is no longer executable, there is a helper script to fix 
permissions
  for the generated run script
- fixed include_directories for test executable, direct paths can be 
used
  since meson 0.50.0
- flake8 is optional as it was with autotools
- added meson version into spec file

 .gitignore  |   1 +
 AUTHORS.in  |   2 +-
 HACKING.md  |  24 ++--
 Makefile.am |  51 ---
 README.md   |  12 +-
 autogen.sh  |  52 ---
 configure.ac|  87 ---
 data/Makefile.am|  83 ---
 data/meson.build|  15 ++
 data/session/meson.build|   6 +
 data/system/meson.build |  18 +++
 docs/Makefile.am|  21 ---
 docs/meson.build|   8 ++
 libvirt-dbus.spec.in|   9 +-
 m4/manywarnings.m4  | 276 ---
 m4/virt-arg.m4  | 154 
 m4/virt-compile-pie.m4  |  35 -
 m4/virt-compile-warnings.m4 | 203 --
 m4/virt-linker-relro.m4 |  35 -
 m4/warnings.m4  |  79 --
 meson.build | 279 
 meson_options.txt   |   6 +
 run.in  |   4 +-
 src/Makefile.am |  66 -
 src/meson.build |  42 ++
 tests/Makefile.am   |  57 
 tests/meson.build   |  52 +++
 tools/fix-perm.sh   |   3 +
 tools/gen-authors.sh|   4 +
 29 files changed, 463 insertions(+), 1221 deletions(-)
 delete mode 100644 Makefile.am
 delete mode 100755 autogen.sh
 delete mode 100644 configure.ac
 delete mode 100644 data/Makefile.am
 create mode 100644 data/meson.build
 create mode 100644 data/session/meson.build
 create mode 100644 data/system/meson.build
 delete mode 100644 docs/Makefile.am
 create mode 100644 docs/meson.build
 delete mode 100644 m4/manywarnings.m4
 delete mode 100644 m4/virt-arg.m4
 delete mode 100644 m4/virt-compile-pie.m4
 delete mode 100644 m4/virt-compile-warnings.m4
 delete mode 100644 m4/virt-linker-relro.m4
 delete mode 100644 m4/warnings.m4
 create mode 100644 meson.build
 create mode 100644 meson_options.txt
 delete mode 100644 src/Makefile.am
 create mode 100644 src/meson.build
 delete mode 100644 tests/Makefile.am
 create mode 100644 tests/meson.build
 create mode 100755 tools/fix-perm.sh
 create mode 100755 tools/gen-authors.sh

diff --git a/.gitignore b/.gitignore
index b4abf66..7ed7554 100644
--- a/.gitignore
+++ b/.gitignore
@@ -19,6 +19,7 @@ vgcore.*
 /aclocal.m4
 /autom4te.cache/
 /build-aux/
+/build/
 /config.h
 /config.h.in
 /config.log
diff --git a/AUTHORS.in b/AUTHORS.in
index 52202ea..d5a486e 100644
--- a/AUTHORS.in
+++ b/AUTHORS.in
@@ -13,4 +13,4 @@ The primary maintainers of libvirt-dbus are:
 
 Patches have been received from:
 
-#contributorslist#
+@contributorslist@
diff --git a/HACKING.md b/HACKING.md
index 8903408..e90f136 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -16,32 +16,40 @@ Alternatively you can use one of the mirrors:
 Running from git repository
 ---
 
-  * The first step is to run autoreconf to create configure script:
+  * The first step is to run meson to create build directory:
 
 ```
-./autogen.sh
+meson build
 ```
 
 Now you can compile libvirt-dbus:
 
 ```
-make
+ninja -C build
 ```
 
 
-  * Before posting a patch, you should run tests and perform syntax-checking:
+  * Before posting a patch, you should run tests:
 
 ```
-make check
+ninja -C build test
 ```
 
-The test tool requires python3, python3-pytest and python3-dbus.
+The test tool requires python3, python3-pytest, python3-dbus and flake8.
+

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Daniel P . Berrangé]

On Fri, Sep 20, 2019 at 11:03:42AM +0200, Pavel Hrdina wrote:
> Meson build system is simple and quick compared to Autotools and it's
> able to fully replace our Autotools usage.  There are few drawbacks as
> it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
> it's still evolving and the user base is not that large and there were
> some tweaks required to achieve the same functionality.
> 
> However, there are benefits, the configure and build time is way shorter
> and build definition files are more readable and easier to maintain.
> 
> There are some major changes with Meson build system:
> 
> - there is no syntax-check target, the syntax-check is part of Meson
>   test suite but it's still possible to run it separately,
> 
> - Meson forces separation between source and build directories
> 
> Signed-off-by: Pavel Hrdina 
> Tested-by: Ján Tomko 
> ---
> 
> Notes:
> changes in v2:
> 
> - add -Werror if we are building from git
> - fixed -Wframe-larger-than
> - removed unrelated fix
> - added comment for flake8 ignore warning
> - added 'suite' labels 'syntax' and 'unit' for tests
> - AUTHORS and libvirt-dbus.spec are generated only when building from 
> git
> - run.in is no longer executable, there is a helper script to fix 
> permissions
>   for the generated run script
> - fixed include_directories for test executable, direct paths can be 
> used
>   since meson 0.50.0
> - flake8 is optional as it was with autotools
> - added meson version into spec file
> 
>  .gitignore  |   1 +
>  AUTHORS.in  |   2 +-
>  HACKING.md  |  24 ++--
>  Makefile.am |  51 ---
>  README.md   |  12 +-
>  autogen.sh  |  52 ---
>  configure.ac|  87 ---
>  data/Makefile.am|  83 ---
>  data/meson.build|  15 ++
>  data/session/meson.build|   6 +
>  data/system/meson.build |  18 +++
>  docs/Makefile.am|  21 ---
>  docs/meson.build|   8 ++
>  libvirt-dbus.spec.in|   9 +-
>  m4/manywarnings.m4  | 276 ---
>  m4/virt-arg.m4  | 154 
>  m4/virt-compile-pie.m4  |  35 -
>  m4/virt-compile-warnings.m4 | 203 --
>  m4/virt-linker-relro.m4 |  35 -
>  m4/warnings.m4  |  79 --
>  meson.build | 279 
>  meson_options.txt   |   6 +
>  run.in  |   4 +-
>  src/Makefile.am |  66 -
>  src/meson.build |  42 ++
>  tests/Makefile.am   |  57 
>  tests/meson.build   |  52 +++
>  tools/fix-perm.sh   |   3 +
>  tools/gen-authors.sh|   4 +
>  29 files changed, 463 insertions(+), 1221 deletions(-)
>  delete mode 100644 Makefile.am
>  delete mode 100755 autogen.sh
>  delete mode 100644 configure.ac
>  delete mode 100644 data/Makefile.am
>  create mode 100644 data/meson.build
>  create mode 100644 data/session/meson.build
>  create mode 100644 data/system/meson.build
>  delete mode 100644 docs/Makefile.am
>  create mode 100644 docs/meson.build
>  delete mode 100644 m4/manywarnings.m4
>  delete mode 100644 m4/virt-arg.m4
>  delete mode 100644 m4/virt-compile-pie.m4
>  delete mode 100644 m4/virt-compile-warnings.m4
>  delete mode 100644 m4/virt-linker-relro.m4
>  delete mode 100644 m4/warnings.m4
>  create mode 100644 meson.build
>  create mode 100644 meson_options.txt
>  delete mode 100644 src/Makefile.am
>  create mode 100644 src/meson.build
>  delete mode 100644 tests/Makefile.am
>  create mode 100644 tests/meson.build
>  create mode 100755 tools/fix-perm.sh
>  create mode 100755 tools/gen-authors.sh


> diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
> index 626e2da..3425e9e 100644
> --- a/libvirt-dbus.spec.in
> +++ b/libvirt-dbus.spec.in
> @@ -1,5 +1,6 @@
>  # -*- rpm-spec -*-
>  
> +%global meson_version @MESON_VERSION@
>  %global glib2_version @GLIB2_REQUIRED@
>  %global libvirt_version @LIBVIRT_REQUIRED@
>  %global libvirt_glib_version @LIBVIRT_GLIB_REQUIRED@
> @@ -14,7 +15,7 @@ URL: https://libvirt.org/
>  Source0: https://libvirt.org/sources/dbus/%{name}-%{version}.tar.xz
>  
>  BuildRequires: gcc
> -BuildRequires: libtool
> +BuildRequires: meson >= %{meson_version}
>  BuildRequires: glib2-devel >= %{glib2_version}
>  BuildRequires: libvirt-devel >= %{libvirt_version}
>  BuildRequires: libvirt-glib-devel >= %{libvirt_glib_version}
> @@ -35,11 +36,11 @@ This package provides D-Bus API for libvirt
>  %autosetup
>  
>  %build
> -%configure
> -%make_build
> +%meson
> +%meson_build
>  
>  %install
> -%make_install
> +%meson_install
>  
>  %pre
>  getent group %{system_user} >/dev/null || groupadd -r %{system_user}

Should this add a '%ch

Re: [libvirt] [PATCH 2/2] virpci, qemu_domain_address: adding virPCIDeviceSetAddress

[Erik Skultety]

On Thu, Sep 19, 2019 at 05:04:47PM -0300, Daniel Henrique Barboza wrote:
> A common operation in qemu_domain_address is comparing a
> virPCIDeviceAddress and assigning domain, bus, slot and function
> to a specific value. The former can be done with the existing
> virPCIDeviceAddressEqual() helper.
>
> A new virpci helper called virPCIDeviceSetAddress() was created to
> make it easier to assign the same domain/bus/slot/function of an
> already existent virPCIDeviceAddress. Using both in
> qemu_domain_address made the code tidier, since the object
> was already created to use virPCIDeviceAddressEqual().
>
> Signed-off-by: Daniel Henrique Barboza 
> ---
>
> I wasn't sure if this change was worth contributing it back,
> since it feels more like a 'look and feel' change. But
> since it made the code inside qemu_domain_address tidier,
> I decided to take a shot.

I actually think it does enhance readability, so why not. However...

[...]
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr 
> def,
>  /* Verify that first IDE and USB controllers (if any) is on the PIIX3, 
> fn 1 */
>  for (i = 0; i < def->ncontrollers; i++) {
>  virDomainControllerDefPtr cont = def->controllers[i];
> +virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0,
> +  .slot = 1, .function = 1};
> +virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0,
> +.slot = 1, .function = 2};
>
>  /* First IDE controller lives on the PIIX3 at slot=1, function=1 */
>  if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
>  cont->idx == 0) {
>  if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
> -if (cont->info.addr.pci.domain != 0 ||
> -cont->info.addr.pci.bus != 0 ||
> -cont->info.addr.pci.slot != 1 ||
> -cont->info.addr.pci.function != 1) {
> +if (!virPCIDeviceAddressEqual(&cont->info.addr.pci,
> +  &primaryIDEAddr)) {

I think this virPCIDeviceAddressEqual consolidation might be a separate patch
(matter of taste, feel free to disagree, but to me the changes are not strictly
related and can be easily split).

>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("Primary IDE controller must have PCI 
> address 0:0:1.1"));
> +   _("Primary IDE controller must have PCI "
> + "address 0:0:1.1"));

Unrelated...but it can stay...

>  return -1;
>  }
>  } else {
>  cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> -cont->info.addr.pci.domain = 0;
> -cont->info.addr.pci.bus = 0;
> -cont->info.addr.pci.slot = 1;
> -cont->info.addr.pci.function = 1;
> +virPCIDeviceSetAddress(&cont->info.addr.pci, 
> &primaryIDEAddr);

So, given the signature, it feels more like a Copy rather than Set. I imagine a
setter to enumerate the scalar types and the destination where the values
should end up in, in this regard, it's not a proper setter because the struct
has a few more elements that technically can be set, but if you created such a
setter I'd say we didn't gain anything in terms of readability at all,
especially when you could do:

/* provided that I'm interpreting 6.7.8.19 [1] of the C99 standard correctly,
 * everything else we're not using should be 0, otherwise we can't do this.
 * Alternatively, you'd need to reset the other members as well */
cont->info.addr.pci = primaryIDEAddr;

I'm not completely sold on the helper the way it is now, but I'm up for the idea
of the patch. Creating a proper Copy helper would bring the obvious issue of
needing to retain the original settings of .multi and zPCI to make it universal,
that's why I'm opting for the direct assignment, because all of the relevant
code in the patch should be x86 only and multifunction would be reset to 0,
which is also the default setting anyway, so that should play nicely as well.

[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [python PATCH 1/2] virDomainMemoryStats: include disk caches

[Pavel Hrdina]

Introduced in libvirt 4.6.0 by commit .

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

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/libvirt-override.c b/libvirt-override.c
index ff2cfdf..3310b54 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -409,6 +409,11 @@ libvirt_virDomainMemoryStats(PyObject *self 
ATTRIBUTE_UNUSED,
 key = libvirt_constcharPtrWrap("last_update");
 break;
 #endif /* LIBVIR_CHECK_VERSION(2, 1, 0) */
+#if LIBVIR_CHECK_VERSION(4, 6, 0)
+case VIR_DOMAIN_MEMORY_STAT_DISK_CACHES:
+key = libvirt_constcharPtrWrap("disk_caches");
+break;
+#endif /* LIBVIR_CHECK_VERSION(4, 6, 0) */
 default:
 continue;
 }
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [python PATCH 0/2] implement missing domain memory stats

[Pavel Hrdina]

Pavel Hrdina (2):
  virDomainMemoryStats: include disk caches
  virDomainMemoryStats: include hugetlb pgalloc and pgfail

 libvirt-override.c | 13 +
 1 file changed, 13 insertions(+)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [python PATCH 2/2] virDomainMemoryStats: include hugetlb pgalloc and pgfail

[Pavel Hrdina]

Introduced in libvirt 5.4.0 by commit .

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

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/libvirt-override.c b/libvirt-override.c
index 3310b54..c76c447 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -414,6 +414,14 @@ libvirt_virDomainMemoryStats(PyObject *self 
ATTRIBUTE_UNUSED,
 key = libvirt_constcharPtrWrap("disk_caches");
 break;
 #endif /* LIBVIR_CHECK_VERSION(4, 6, 0) */
+#if LIBVIR_CHECK_VERSION(5, 4, 0)
+case VIR_DOMAIN_MEMORY_STAT_HUGETLB_PGALLOC:
+key = libvirt_constcharPtrWrap("hugetlb_pgalloc");
+break;
+case VIR_DOMAIN_MEMORY_STAT_HUGETLB_PGFAIL:
+key = libvirt_constcharPtrWrap("hugetlb_pgfail");
+break;
+#endif /* LIBVIR_CHECK_VERSION(5, 4, 0) */
 default:
 continue;
 }
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Pavel Hrdina]

On Fri, Sep 20, 2019 at 10:14:00AM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 20, 2019 at 11:03:42AM +0200, Pavel Hrdina wrote:
> > Meson build system is simple and quick compared to Autotools and it's
> > able to fully replace our Autotools usage.  There are few drawbacks as
> > it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
> > it's still evolving and the user base is not that large and there were
> > some tweaks required to achieve the same functionality.
> > 
> > However, there are benefits, the configure and build time is way shorter
> > and build definition files are more readable and easier to maintain.
> > 
> > There are some major changes with Meson build system:
> > 
> > - there is no syntax-check target, the syntax-check is part of Meson
> >   test suite but it's still possible to run it separately,
> > 
> > - Meson forces separation between source and build directories
> > 
> > Signed-off-by: Pavel Hrdina 
> > Tested-by: Ján Tomko 
> > ---
> > 
> > Notes:
> > changes in v2:
> > 
> > - add -Werror if we are building from git
> > - fixed -Wframe-larger-than
> > - removed unrelated fix
> > - added comment for flake8 ignore warning
> > - added 'suite' labels 'syntax' and 'unit' for tests
> > - AUTHORS and libvirt-dbus.spec are generated only when building 
> > from git
> > - run.in is no longer executable, there is a helper script to fix 
> > permissions
> >   for the generated run script
> > - fixed include_directories for test executable, direct paths can 
> > be used
> >   since meson 0.50.0
> > - flake8 is optional as it was with autotools
> > - added meson version into spec file
> > 
> >  .gitignore  |   1 +
> >  AUTHORS.in  |   2 +-
> >  HACKING.md  |  24 ++--
> >  Makefile.am |  51 ---
> >  README.md   |  12 +-
> >  autogen.sh  |  52 ---
> >  configure.ac|  87 ---
> >  data/Makefile.am|  83 ---
> >  data/meson.build|  15 ++
> >  data/session/meson.build|   6 +
> >  data/system/meson.build |  18 +++
> >  docs/Makefile.am|  21 ---
> >  docs/meson.build|   8 ++
> >  libvirt-dbus.spec.in|   9 +-
> >  m4/manywarnings.m4  | 276 ---
> >  m4/virt-arg.m4  | 154 
> >  m4/virt-compile-pie.m4  |  35 -
> >  m4/virt-compile-warnings.m4 | 203 --
> >  m4/virt-linker-relro.m4 |  35 -
> >  m4/warnings.m4  |  79 --
> >  meson.build | 279 
> >  meson_options.txt   |   6 +
> >  run.in  |   4 +-
> >  src/Makefile.am |  66 -
> >  src/meson.build |  42 ++
> >  tests/Makefile.am   |  57 
> >  tests/meson.build   |  52 +++
> >  tools/fix-perm.sh   |   3 +
> >  tools/gen-authors.sh|   4 +
> >  29 files changed, 463 insertions(+), 1221 deletions(-)
> >  delete mode 100644 Makefile.am
> >  delete mode 100755 autogen.sh
> >  delete mode 100644 configure.ac
> >  delete mode 100644 data/Makefile.am
> >  create mode 100644 data/meson.build
> >  create mode 100644 data/session/meson.build
> >  create mode 100644 data/system/meson.build
> >  delete mode 100644 docs/Makefile.am
> >  create mode 100644 docs/meson.build
> >  delete mode 100644 m4/manywarnings.m4
> >  delete mode 100644 m4/virt-arg.m4
> >  delete mode 100644 m4/virt-compile-pie.m4
> >  delete mode 100644 m4/virt-compile-warnings.m4
> >  delete mode 100644 m4/virt-linker-relro.m4
> >  delete mode 100644 m4/warnings.m4
> >  create mode 100644 meson.build
> >  create mode 100644 meson_options.txt
> >  delete mode 100644 src/Makefile.am
> >  create mode 100644 src/meson.build
> >  delete mode 100644 tests/Makefile.am
> >  create mode 100644 tests/meson.build
> >  create mode 100755 tools/fix-perm.sh
> >  create mode 100755 tools/gen-authors.sh
> 
> 
> > diff --git a/libvirt-dbus.spec.in b/libvirt-dbus.spec.in
> > index 626e2da..3425e9e 100644
> > --- a/libvirt-dbus.spec.in
> > +++ b/libvirt-dbus.spec.in
> > @@ -1,5 +1,6 @@
> >  # -*- rpm-spec -*-
> >  
> > +%global meson_version @MESON_VERSION@
> >  %global glib2_version @GLIB2_REQUIRED@
> >  %global libvirt_version @LIBVIRT_REQUIRED@
> >  %global libvirt_glib_version @LIBVIRT_GLIB_REQUIRED@
> > @@ -14,7 +15,7 @@ URL: https://libvirt.org/
> >  Source0: https://libvirt.org/sources/dbus/%{name}-%{version}.tar.xz
> >  
> >  BuildRequires: gcc
> > -BuildRequires: libtool
> > +BuildRequires: meson >= %{meson_version}
> >  BuildRequires: glib2-devel >= %{glib2_version}
> >  BuildRequires: libvirt-devel >= %{libvirt_version}
> >  BuildRequires: libvirt-glib-devel >= %{libvirt_glib_version}
> > @@ -3

Re: [libvirt] [PATCH] security: AppArmor profile fixes for swtpm

[Martin Kletzander]

On Mon, Sep 16, 2019 at 03:27:25PM +0200, Chris Coulson wrote:

The AppArmor profile generated by virt-aa-helper is too strict for swtpm.
This change contains 2 small fixes:
- Relax append access to swtpm's log file to permit write access instead.
Append access is insufficient because the log is opened with O_CREAT.
- Permit swtpm to acquire a lock on its lock file.


Thanks for submitting this patch.  As written on our governance page [1], 
particularly under contributors [2], this project requires providing a 
Signed-off-by as an agreement with the Developer Certificate of Origin [3].

If you can resend the patches with that change (easily with git commit --amend, 
very easily with a silly command-line [4]) I will merge this patch.

Have a nice day,
Martin

[1] https://libvirt.org/governance.html
[2] https://libvirt.org/governance.html#contributors
[3] https://developercertificate.org/
[4] EDITOR='git interpret-trailers --in-place --trailer "Signed-off-by=..."' 
git commit --amend


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Daniel P . Berrangé]

On Fri, Sep 20, 2019 at 11:36:53AM +0200, Pavel Hrdina wrote:
> On Fri, Sep 20, 2019 at 10:14:00AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Sep 20, 2019 at 11:03:42AM +0200, Pavel Hrdina wrote:
> > > Meson build system is simple and quick compared to Autotools and it's
> > > able to fully replace our Autotools usage.  There are few drawbacks as
> > > it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
> > > it's still evolving and the user base is not that large and there were
> > > some tweaks required to achieve the same functionality.
> > > 
> > > However, there are benefits, the configure and build time is way shorter
> > > and build definition files are more readable and easier to maintain.
> > > 
> > > There are some major changes with Meson build system:
> > > 
> > > - there is no syntax-check target, the syntax-check is part of Meson
> > >   test suite but it's still possible to run it separately,
> > > 
> > > - Meson forces separation between source and build directories
> > > 
> > > Signed-off-by: Pavel Hrdina 
> > > Tested-by: Ján Tomko 


> > > @@ -35,11 +36,11 @@ This package provides D-Bus API for libvirt
> > >  %autosetup
> > >  
> > >  %build
> > > -%configure
> > > -%make_build
> > > +%meson
> > > +%meson_build
> > >  
> > >  %install
> > > -%make_install
> > > +%meson_install
> > >  
> > >  %pre
> > >  getent group %{system_user} >/dev/null || groupadd -r %{system_user}
> > 
> > Should this add a '%check' section to run '%meson_test' as sanity
> > check for the build.
> 
> I would rather do it as a followup patch where we wound have to add
> more build dependencies into our spec file (python3, python3-pytest,
> python3-dbus and python3-flake8).

Sure, fine with me.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH python] Custom impl for virConnectSetIdentity which can't be generated

[Daniel P . Berrangé]

ping, this would fix our broken CI...

On Mon, Sep 16, 2019 at 05:37:43PM +0100, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé 
> ---
>  generator.py   |  1 +
>  libvirt-override.c | 66 ++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/generator.py b/generator.py
> index 433c1f2..913dab8 100755
> --- a/generator.py
> +++ b/generator.py
> @@ -489,6 +489,7 @@ skip_impl = (
>  'virDomainGetDiskErrors',
>  'virNodeGetMemoryParameters',
>  'virNodeSetMemoryParameters',
> +'virConnectSetIdentity',
>  'virNodeGetCPUMap',
>  'virDomainMigrate3',
>  'virDomainMigrateToURI3',
> diff --git a/libvirt-override.c b/libvirt-override.c
> index ff2cfdf..33f20d1 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -10209,6 +10209,69 @@ libvirt_virDomainGetGuestInfo(PyObject *self 
> ATTRIBUTE_UNUSED,
>  }
>  #endif /* LIBVIR_CHECK_VERSION(5, 7, 0) */
>  
> +
> +#if LIBVIR_CHECK_VERSION(5, 8, 0)
> +static virPyTypedParamsHint virPyConnectSetIdentityParams[] = {
> +{ VIR_CONNECT_IDENTITY_USER_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_UNIX_USER_ID, VIR_TYPED_PARAM_ULLONG },
> +{ VIR_CONNECT_IDENTITY_GROUP_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_UNIX_GROUP_ID, VIR_TYPED_PARAM_ULLONG },
> +{ VIR_CONNECT_IDENTITY_PROCESS_ID, VIR_TYPED_PARAM_LLONG },
> +{ VIR_CONNECT_IDENTITY_PROCESS_TIME, VIR_TYPED_PARAM_ULLONG },
> +{ VIR_CONNECT_IDENTITY_SASL_USER_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, VIR_TYPED_PARAM_STRING },
> +};
> +
> +static PyObject *
> +libvirt_virConnectSetIdentity(PyObject *self ATTRIBUTE_UNUSED,
> +  PyObject *args)
> +{
> +virConnectPtr conn;
> +PyObject *pyobj_conn, *dict;
> +PyObject *ret = NULL;
> +int i_retval;
> +int nparams = 0;
> +Py_ssize_t size = 0;
> +unsigned int flags;
> +virTypedParameterPtr params = NULL, new_params = NULL;
> +
> +if (!PyArg_ParseTuple(args,
> +  (char *)"OOI:virConnectSetIdentity",
> +  &pyobj_conn, &dict, &flags))
> +return NULL;
> +conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
> +
> +if (!PyDict_Check(dict)) {
> +PyErr_Format(PyExc_TypeError, "migration params must be a 
> dictionary");
> +return NULL;
> +}
> +
> +if (virPyDictToTypedParams(dict, ¶ms, &nparams,
> +   virPyConnectSetIdentityParams,
> +   
> VIR_N_ELEMENTS(virPyConnectSetIdentityParams)) < 0) {
> +return NULL;
> +}
> +
> +LIBVIRT_BEGIN_ALLOW_THREADS;
> +i_retval = virConnectSetIdentity(conn, new_params, size, flags);
> +LIBVIRT_END_ALLOW_THREADS;
> +
> +if (i_retval < 0) {
> +ret = VIR_PY_INT_FAIL;
> +goto cleanup;
> +}
> +
> +ret = VIR_PY_INT_SUCCESS;
> +
> + cleanup:
> +virTypedParamsFree(params, nparams);
> +virTypedParamsFree(new_params, size);
> +return ret;
> +}
> +#endif /* LIBVIR_CHECK_VERSION(5, 8, 0) */
> +
> +
>  /
>   *   *
>   *   The registration stuff  *
> @@ -10467,6 +10530,9 @@ static PyMethodDef libvirtMethods[] = {
>  #if LIBVIR_CHECK_VERSION(5, 7, 0)
>  {(char *) "virDomainGetGuestInfo", libvirt_virDomainGetGuestInfo, 
> METH_VARARGS, NULL},
>  #endif /* LIBVIR_CHECK_VERSION(5, 7, 0) */
> +#if LIBVIR_CHECK_VERSION(5, 8, 0)
> +{(char *) "virConnectSetIdentity", libvirt_virConnectSetIdentity, 
> METH_VARARGS, NULL},
> +#endif /* LIBVIR_CHECK_VERSION(5, 8, 0) */
>  {NULL, NULL, 0, NULL}
>  };
>  
> -- 
> 2.21.0
> 

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH python] Custom impl for virConnectSetIdentity which can't be generated

[Pavel Hrdina]

On Mon, Sep 16, 2019 at 05:37:43PM +0100, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé 
> ---
>  generator.py   |  1 +
>  libvirt-override.c | 66 ++
>  2 files changed, 67 insertions(+)

This is missing XML description in libvirt-override-api.xml.

> diff --git a/generator.py b/generator.py
> index 433c1f2..913dab8 100755
> --- a/generator.py
> +++ b/generator.py
> @@ -489,6 +489,7 @@ skip_impl = (
>  'virDomainGetDiskErrors',
>  'virNodeGetMemoryParameters',
>  'virNodeSetMemoryParameters',
> +'virConnectSetIdentity',
>  'virNodeGetCPUMap',
>  'virDomainMigrate3',
>  'virDomainMigrateToURI3',
> diff --git a/libvirt-override.c b/libvirt-override.c
> index ff2cfdf..33f20d1 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -10209,6 +10209,69 @@ libvirt_virDomainGetGuestInfo(PyObject *self 
> ATTRIBUTE_UNUSED,
>  }
>  #endif /* LIBVIR_CHECK_VERSION(5, 7, 0) */
>  
> +
> +#if LIBVIR_CHECK_VERSION(5, 8, 0)
> +static virPyTypedParamsHint virPyConnectSetIdentityParams[] = {
> +{ VIR_CONNECT_IDENTITY_USER_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_UNIX_USER_ID, VIR_TYPED_PARAM_ULLONG },
> +{ VIR_CONNECT_IDENTITY_GROUP_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_UNIX_GROUP_ID, VIR_TYPED_PARAM_ULLONG },
> +{ VIR_CONNECT_IDENTITY_PROCESS_ID, VIR_TYPED_PARAM_LLONG },
> +{ VIR_CONNECT_IDENTITY_PROCESS_TIME, VIR_TYPED_PARAM_ULLONG },
> +{ VIR_CONNECT_IDENTITY_SASL_USER_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME, VIR_TYPED_PARAM_STRING },
> +{ VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, VIR_TYPED_PARAM_STRING },
> +};
> +
> +static PyObject *
> +libvirt_virConnectSetIdentity(PyObject *self ATTRIBUTE_UNUSED,
> +  PyObject *args)
> +{
> +virConnectPtr conn;
> +PyObject *pyobj_conn, *dict;
> +PyObject *ret = NULL;
> +int i_retval;
> +int nparams = 0;
> +Py_ssize_t size = 0;
> +unsigned int flags;
> +virTypedParameterPtr params = NULL, new_params = NULL;
> +
> +if (!PyArg_ParseTuple(args,
> +  (char *)"OOI:virConnectSetIdentity",
> +  &pyobj_conn, &dict, &flags))
> +return NULL;
> +conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
> +
> +if (!PyDict_Check(dict)) {
> +PyErr_Format(PyExc_TypeError, "migration params must be a 
> dictionary");
> +return NULL;
> +}
> +
> +if (virPyDictToTypedParams(dict, ¶ms, &nparams,
> +   virPyConnectSetIdentityParams,
> +   
> VIR_N_ELEMENTS(virPyConnectSetIdentityParams)) < 0) {
> +return NULL;
> +}
> +
> +LIBVIRT_BEGIN_ALLOW_THREADS;
> +i_retval = virConnectSetIdentity(conn, new_params, size, flags);

This is strange, new_params are NULL as they are not used at all,
instead you are filling params.  It feels like new_params and size are
not required at all.

Otherwise looks good.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH python v2] Custom impl for virConnectSetIdentity which can't be generated

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 generator.py |  1 +
 libvirt-override-api.xml |  7 +
 libvirt-override.c   | 64 
 3 files changed, 72 insertions(+)

diff --git a/generator.py b/generator.py
index 433c1f2..913dab8 100755
--- a/generator.py
+++ b/generator.py
@@ -489,6 +489,7 @@ skip_impl = (
 'virDomainGetDiskErrors',
 'virNodeGetMemoryParameters',
 'virNodeSetMemoryParameters',
+'virConnectSetIdentity',
 'virNodeGetCPUMap',
 'virDomainMigrate3',
 'virDomainMigrateToURI3',
diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
index b2a6259..7a0d4c5 100644
--- a/libvirt-override-api.xml
+++ b/libvirt-override-api.xml
@@ -796,5 +796,12 @@
   
   
 
+
+  Override the default identity information associated with the 
connection.
+  
+  
+  
+  
+
   
 
diff --git a/libvirt-override.c b/libvirt-override.c
index ff2cfdf..72edca9 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -10209,6 +10209,67 @@ libvirt_virDomainGetGuestInfo(PyObject *self 
ATTRIBUTE_UNUSED,
 }
 #endif /* LIBVIR_CHECK_VERSION(5, 7, 0) */
 
+
+#if LIBVIR_CHECK_VERSION(5, 8, 0)
+static virPyTypedParamsHint virPyConnectSetIdentityParams[] = {
+{ VIR_CONNECT_IDENTITY_USER_NAME, VIR_TYPED_PARAM_STRING },
+{ VIR_CONNECT_IDENTITY_UNIX_USER_ID, VIR_TYPED_PARAM_ULLONG },
+{ VIR_CONNECT_IDENTITY_GROUP_NAME, VIR_TYPED_PARAM_STRING },
+{ VIR_CONNECT_IDENTITY_UNIX_GROUP_ID, VIR_TYPED_PARAM_ULLONG },
+{ VIR_CONNECT_IDENTITY_PROCESS_ID, VIR_TYPED_PARAM_LLONG },
+{ VIR_CONNECT_IDENTITY_PROCESS_TIME, VIR_TYPED_PARAM_ULLONG },
+{ VIR_CONNECT_IDENTITY_SASL_USER_NAME, VIR_TYPED_PARAM_STRING },
+{ VIR_CONNECT_IDENTITY_X509_DISTINGUISHED_NAME, VIR_TYPED_PARAM_STRING },
+{ VIR_CONNECT_IDENTITY_SELINUX_CONTEXT, VIR_TYPED_PARAM_STRING },
+};
+
+static PyObject *
+libvirt_virConnectSetIdentity(PyObject *self ATTRIBUTE_UNUSED,
+  PyObject *args)
+{
+virConnectPtr conn;
+PyObject *pyobj_conn, *dict;
+PyObject *ret = NULL;
+int i_retval;
+int nparams = 0;
+unsigned int flags;
+virTypedParameterPtr params = NULL;
+
+if (!PyArg_ParseTuple(args,
+  (char *)"OOI:virConnectSetIdentity",
+  &pyobj_conn, &dict, &flags))
+return NULL;
+conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
+
+if (!PyDict_Check(dict)) {
+PyErr_Format(PyExc_TypeError, "migration params must be a dictionary");
+return NULL;
+}
+
+if (virPyDictToTypedParams(dict, ¶ms, &nparams,
+   virPyConnectSetIdentityParams,
+   VIR_N_ELEMENTS(virPyConnectSetIdentityParams)) 
< 0) {
+return NULL;
+}
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+i_retval = virConnectSetIdentity(conn, params, nparams, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (i_retval < 0) {
+ret = VIR_PY_INT_FAIL;
+goto cleanup;
+}
+
+ret = VIR_PY_INT_SUCCESS;
+
+ cleanup:
+virTypedParamsFree(params, nparams);
+return ret;
+}
+#endif /* LIBVIR_CHECK_VERSION(5, 8, 0) */
+
+
 /
  * *
  * The registration stuff  *
@@ -10467,6 +10528,9 @@ static PyMethodDef libvirtMethods[] = {
 #if LIBVIR_CHECK_VERSION(5, 7, 0)
 {(char *) "virDomainGetGuestInfo", libvirt_virDomainGetGuestInfo, 
METH_VARARGS, NULL},
 #endif /* LIBVIR_CHECK_VERSION(5, 7, 0) */
+#if LIBVIR_CHECK_VERSION(5, 8, 0)
+{(char *) "virConnectSetIdentity", libvirt_virConnectSetIdentity, 
METH_VARARGS, NULL},
+#endif /* LIBVIR_CHECK_VERSION(5, 8, 0) */
 {NULL, NULL, 0, NULL}
 };
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [python PATCH 2/2] virDomainMemoryStats: include hugetlb pgalloc and pgfail

[Daniel P . Berrangé]

On Fri, Sep 20, 2019 at 11:33:11AM +0200, Pavel Hrdina wrote:
> Introduced in libvirt 5.4.0 by commit .
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1683516
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt-override.c | 8 
>  1 file changed, 8 insertions(+)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [python PATCH 1/2] virDomainMemoryStats: include disk caches

[Daniel P . Berrangé]

On Fri, Sep 20, 2019 at 11:33:10AM +0200, Pavel Hrdina wrote:
> Introduced in libvirt 4.6.0 by commit .
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1683516
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  libvirt-override.c | 5 +
>  1 file changed, 5 insertions(+)

Reviewed-by: Daniel P. Berrangé 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH python v2] Custom impl for virConnectSetIdentity which can't be generated

[Pavel Hrdina]

On Fri, Sep 20, 2019 at 11:28:40AM +0100, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé 
> ---
>  generator.py |  1 +
>  libvirt-override-api.xml |  7 +
>  libvirt-override.c   | 64 
>  3 files changed, 72 insertions(+)

Reviewed-by: Pavel Hrdina 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Fabiano Fidêncio]

On Fri, Sep 20, 2019 at 11:05 AM Pavel Hrdina  wrote:
>
> Meson build system is simple and quick compared to Autotools and it's
> able to fully replace our Autotools usage.  There are few drawbacks as
> it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
> it's still evolving and the user base is not that large and there were
> some tweaks required to achieve the same functionality.
>
> However, there are benefits, the configure and build time is way shorter
> and build definition files are more readable and easier to maintain.
>
> There are some major changes with Meson build system:
>
> - there is no syntax-check target, the syntax-check is part of Meson
>   test suite but it's still possible to run it separately,
>
> - Meson forces separation between source and build directories
>
> Signed-off-by: Pavel Hrdina 
> Tested-by: Ján Tomko 
> ---
>
> Notes:
> changes in v2:
>
> - add -Werror if we are building from git
> - fixed -Wframe-larger-than
> - removed unrelated fix
> - added comment for flake8 ignore warning
> - added 'suite' labels 'syntax' and 'unit' for tests
> - AUTHORS and libvirt-dbus.spec are generated only when building from 
> git

Why? Not opposed to this decision, just want to understand the reason for that.

> - run.in is no longer executable, there is a helper script to fix 
> permissions
>   for the generated run script
> - fixed include_directories for test executable, direct paths can be 
> used
>   since meson 0.50.0

By using 0.50.0 you're dropping support on:
- Debian 10;
- Ubuntu 19.04;

Personally, I would stick to 0.49.0.

> - flake8 is optional as it was with autotools
> - added meson version into spec file
>
>  .gitignore  |   1 +
>  AUTHORS.in  |   2 +-
>  HACKING.md  |  24 ++--
>  Makefile.am |  51 ---
>  README.md   |  12 +-
>  autogen.sh  |  52 ---
>  configure.ac|  87 ---
>  data/Makefile.am|  83 ---
>  data/meson.build|  15 ++
>  data/session/meson.build|   6 +
>  data/system/meson.build |  18 +++
>  docs/Makefile.am|  21 ---
>  docs/meson.build|   8 ++
>  libvirt-dbus.spec.in|   9 +-
>  m4/manywarnings.m4  | 276 ---
>  m4/virt-arg.m4  | 154 
>  m4/virt-compile-pie.m4  |  35 -
>  m4/virt-compile-warnings.m4 | 203 --
>  m4/virt-linker-relro.m4 |  35 -
>  m4/warnings.m4  |  79 --
>  meson.build | 279 
>  meson_options.txt   |   6 +
>  run.in  |   4 +-
>  src/Makefile.am |  66 -
>  src/meson.build |  42 ++
>  tests/Makefile.am   |  57 
>  tests/meson.build   |  52 +++
>  tools/fix-perm.sh   |   3 +
>  tools/gen-authors.sh|   4 +
>  29 files changed, 463 insertions(+), 1221 deletions(-)
>  delete mode 100644 Makefile.am
>  delete mode 100755 autogen.sh
>  delete mode 100644 configure.ac
>  delete mode 100644 data/Makefile.am
>  create mode 100644 data/meson.build
>  create mode 100644 data/session/meson.build
>  create mode 100644 data/system/meson.build
>  delete mode 100644 docs/Makefile.am
>  create mode 100644 docs/meson.build
>  delete mode 100644 m4/manywarnings.m4
>  delete mode 100644 m4/virt-arg.m4
>  delete mode 100644 m4/virt-compile-pie.m4
>  delete mode 100644 m4/virt-compile-warnings.m4
>  delete mode 100644 m4/virt-linker-relro.m4
>  delete mode 100644 m4/warnings.m4
>  create mode 100644 meson.build
>  create mode 100644 meson_options.txt
>  delete mode 100644 src/Makefile.am
>  create mode 100644 src/meson.build
>  delete mode 100644 tests/Makefile.am
>  create mode 100644 tests/meson.build
>  create mode 100755 tools/fix-perm.sh
>  create mode 100755 tools/gen-authors.sh
>
> diff --git a/.gitignore b/.gitignore
> index b4abf66..7ed7554 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -19,6 +19,7 @@ vgcore.*
>  /aclocal.m4
>  /autom4te.cache/
>  /build-aux/
> +/build/
>  /config.h
>  /config.h.in
>  /config.log
> diff --git a/AUTHORS.in b/AUTHORS.in
> index 52202ea..d5a486e 100644
> --- a/AUTHORS.in
> +++ b/AUTHORS.in
> @@ -13,4 +13,4 @@ The primary maintainers of libvirt-dbus are:
>
>  Patches have been received from:
>
> -#contributorslist#
> +@contributorslist@
> diff --git a/HACKING.md b/HACKING.md
> index 8903408..e90f136 100644
> --- a/HACKING.md
> +++ b/HACKING.md
> @@ -16,32 +16,40 @@ Alternatively you can use one of the mirrors:
>  Running from git repository
>  ---
>
> -  * The first step is to run autoreconf to create configure script:
> +  * The first step is to run meson to create build directory:
>
>  ``

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Fabiano Fidêncio]

On Fri, Sep 20, 2019 at 1:28 PM Fabiano Fidêncio  wrote:
>
> On Fri, Sep 20, 2019 at 11:05 AM Pavel Hrdina  wrote:
> >
> > Meson build system is simple and quick compared to Autotools and it's
> > able to fully replace our Autotools usage.  There are few drawbacks as
> > it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
> > it's still evolving and the user base is not that large and there were
> > some tweaks required to achieve the same functionality.
> >
> > However, there are benefits, the configure and build time is way shorter
> > and build definition files are more readable and easier to maintain.
> >
> > There are some major changes with Meson build system:
> >
> > - there is no syntax-check target, the syntax-check is part of Meson
> >   test suite but it's still possible to run it separately,
> >
> > - Meson forces separation between source and build directories
> >
> > Signed-off-by: Pavel Hrdina 
> > Tested-by: Ján Tomko 
> > ---
> >
> > Notes:
> > changes in v2:
> >
> > - add -Werror if we are building from git
> > - fixed -Wframe-larger-than
> > - removed unrelated fix
> > - added comment for flake8 ignore warning
> > - added 'suite' labels 'syntax' and 'unit' for tests
> > - AUTHORS and libvirt-dbus.spec are generated only when building 
> > from git
>
> Why? Not opposed to this decision, just want to understand the reason for 
> that.
>
> > - run.in is no longer executable, there is a helper script to fix 
> > permissions
> >   for the generated run script
> > - fixed include_directories for test executable, direct paths can 
> > be used
> >   since meson 0.50.0
>
> By using 0.50.0 you're dropping support on:
> - Debian 10;
> - Ubuntu 19.04;
>
> Personally, I would stick to 0.49.0.

So, let me see if I properly understood your comment.
You mean that before you were requiring 0.50.0 and now, because you
changed the way to deal with the include_directories, you require
0.49.0. Is it?
If so, just ignore my comment in the previous email.

Best Regards,
-- 
Fabiano Fidêncio

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] docs: kbase: Add a section explaining how to verify SEV from the guest

[Erik Skultety]

Commit 50dfabbb59 forgot to add this important bit on how to check that
all the changes to the XML actually worked.
---
 docs/kbase/launch_security_sev.html.in | 12 
 1 file changed, 12 insertions(+)

diff --git a/docs/kbase/launch_security_sev.html.in 
b/docs/kbase/launch_security_sev.html.in
index 923bb52b25..4b8e06ccc1 100644
--- a/docs/kbase/launch_security_sev.html.in
+++ b/docs/kbase/launch_security_sev.html.in
@@ -349,6 +349,18 @@ EOF
   ...
 

+Checking SEV from within the guest
+
+After making the necessary adjustments discussed in
+Configuration, the VM should now boot
+successfully with SEV enabled. You can then verify that the guest
+enabled with SEV by running:
+
+
+
+# dmesg | grep -i sev
+AMD Secure Encrypted Virtualization (SEV) active
+
 Limitations
 
 Currently, the boot disk cannot be of type virtio-blk, instead, 
virtio-scsi
--
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] remote: fix enablement of IP networking in virtproxyd

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/Makefile.inc.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index e05ae64169..e02c20d47c 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -237,7 +237,7 @@ virtproxyd_CFLAGS = \
$(REMOTE_DAEMON_CFLAGS) \
-DSOCK_PREFIX="\"libvirt\"" \
-DDAEMON_NAME="\"virtproxyd\"" \
-   -DENABLE_IP \
+   -DWITH_IP \
-DVIRTPROXYD \
$(NULL)
 virtproxyd_LDFLAGS = $(REMOTE_DAEMON_LD_FLAGS)
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/7] conf: Drop pointless 'domain' argument from virDomainSnapshotRedefinePrep

[Peter Krempa]

'vm' is passed in which contains the definition which contains the UUID
so we don't need another parameter for this.

Signed-off-by: Peter Krempa 
---
 src/conf/snapshot_conf.c | 5 ++---
 src/conf/snapshot_conf.h | 3 +--
 src/qemu/qemu_driver.c   | 2 +-
 src/test/test_driver.c   | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 61c807a71f..96ad8ca953 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -986,8 +986,7 @@ virDomainSnapshotIsExternal(virDomainMomentObjPtr snap)
 }

 int
-virDomainSnapshotRedefinePrep(virDomainPtr domain,
-  virDomainObjPtr vm,
+virDomainSnapshotRedefinePrep(virDomainObjPtr vm,
   virDomainSnapshotDefPtr *defptr,
   virDomainMomentObjPtr *snap,
   virDomainXMLOptionPtr xmlopt,
@@ -1006,7 +1005,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
 if (other)
 otherdef = virDomainSnapshotObjGetDef(other);
 check_if_stolen = other && otherdef->parent.dom;
-if (virDomainSnapshotRedefineValidate(def, domain->uuid, other, xmlopt,
+if (virDomainSnapshotRedefineValidate(def, vm->def->uuid, other, xmlopt,
   flags) < 0) {
 /* revert any stealing of the snapshot domain definition */
 if (check_if_stolen && def->parent.dom && !otherdef->parent.dom)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 216726fc16..17d614a7e1 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -129,8 +129,7 @@ int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr 
snapshot,
 bool virDomainSnapshotDefIsExternal(virDomainSnapshotDefPtr def);
 bool virDomainSnapshotIsExternal(virDomainMomentObjPtr snap);

-int virDomainSnapshotRedefinePrep(virDomainPtr domain,
-  virDomainObjPtr vm,
+int virDomainSnapshotRedefinePrep(virDomainObjPtr vm,
   virDomainSnapshotDefPtr *def,
   virDomainMomentObjPtr *snap,
   virDomainXMLOptionPtr xmlopt,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ad14b864f7..69467c21f6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15985,7 +15985,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
 qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);

 if (redefine) {
-if (virDomainSnapshotRedefinePrep(domain, vm, &def, &snap,
+if (virDomainSnapshotRedefinePrep(vm, &def, &snap,
   driver->xmlopt,
   &update_current, flags) < 0)
 goto endjob;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index dafd8c8daa..19b8158ed6 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -8612,7 +8612,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
 goto cleanup;

 if (redefine) {
-if (virDomainSnapshotRedefinePrep(domain, vm, &def, &snap,
+if (virDomainSnapshotRedefinePrep(vm, &def, &snap,
   privconn->xmlopt,
   &update_current, flags) < 0)
 goto cleanup;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/7] conf: Drop pointless 'domain' argument from virDomainCheckpointRedefinePrep

[Peter Krempa]

'vm' is passed in which contains the definition which contains the UUID
so we don't need another parameter for this.

Signed-off-by: Peter Krempa 
---
 src/conf/checkpoint_conf.c | 7 +++
 src/conf/checkpoint_conf.h | 3 +--
 src/qemu/qemu_driver.c | 2 +-
 src/test/test_driver.c | 2 +-
 4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index 5c998267aa..a83863adcc 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -532,8 +532,7 @@ virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def,


 int
-virDomainCheckpointRedefinePrep(virDomainPtr domain,
-virDomainObjPtr vm,
+virDomainCheckpointRedefinePrep(virDomainObjPtr vm,
 virDomainCheckpointDefPtr *defptr,
 virDomainMomentObjPtr *chk,
 virDomainXMLOptionPtr xmlopt,
@@ -544,13 +543,13 @@ virDomainCheckpointRedefinePrep(virDomainPtr domain,
 virDomainMomentObjPtr other = NULL;
 virDomainCheckpointDefPtr otherdef = NULL;

-virUUIDFormat(domain->uuid, uuidstr);
+virUUIDFormat(vm->def->uuid, uuidstr);

 if (virDomainCheckpointCheckCycles(vm->checkpoints, def, vm->def->name) < 
0)
 return -1;

 if (!def->parent.dom ||
-memcmp(def->parent.dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
+memcmp(def->parent.dom->uuid, vm->def->uuid, VIR_UUID_BUFLEN)) {
 virReportError(VIR_ERR_INVALID_ARG,
_("definition for checkpoint %s must use uuid %s"),
def->parent.name, uuidstr);
diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
index 47ff69eb4d..2be041ff56 100644
--- a/src/conf/checkpoint_conf.h
+++ b/src/conf/checkpoint_conf.h
@@ -90,8 +90,7 @@ virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def,
 int
 virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr checkpoint);

-int virDomainCheckpointRedefinePrep(virDomainPtr domain,
-virDomainObjPtr vm,
+int virDomainCheckpointRedefinePrep(virDomainObjPtr vm,
 virDomainCheckpointDefPtr *def,
 virDomainMomentObjPtr *checkpoint,
 virDomainXMLOptionPtr xmlopt,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 21ab58e4ec..ad14b864f7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17313,7 +17313,7 @@ qemuDomainCheckpointCreateXML(virDomainPtr domain,
 goto cleanup;

 if (redefine) {
-if (virDomainCheckpointRedefinePrep(domain, vm, &def, &chk,
+if (virDomainCheckpointRedefinePrep(vm, &def, &chk,
 driver->xmlopt,
 &update_current) < 0)
 goto endjob;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 9df7840390..dafd8c8daa 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -9073,7 +9073,7 @@ testDomainCheckpointCreateXML(virDomainPtr domain,
 goto cleanup;

 if (redefine) {
-if (virDomainCheckpointRedefinePrep(domain, vm, &def, &chk,
+if (virDomainCheckpointRedefinePrep(vm, &def, &chk,
 privconn->xmlopt,
 &update_current) < 0)
 goto cleanup;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/7] qemu: Move, rename and export qemuDomObjFromDomain

[Peter Krempa]

Move it to qemu_domain.c and rename it to qemuDomainObjFromDomain. This
will allow reusing it after splitting out checkpoint code from
qemu_driver.c.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_domain.c |  31 +
 src/qemu/qemu_domain.h |   1 +
 src/qemu/qemu_driver.c | 303 +++--
 3 files changed, 169 insertions(+), 166 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ad4db7e881..f733198ea9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -119,6 +119,37 @@ VIR_ENUM_IMPL(qemuDomainNamespace,
   "mount",
 );

+
+/**
+ * qemuDomainObjFromDomain:
+ * @domain: Domain pointer that has to be looked up
+ *
+ * This function looks up @domain and returns the appropriate virDomainObjPtr
+ * that has to be released by calling virDomainObjEndAPI().
+ *
+ * Returns the domain object with incremented reference counter which is locked
+ * on success, NULL otherwise.
+ */
+virDomainObjPtr
+qemuDomainObjFromDomain(virDomainPtr domain)
+{
+virDomainObjPtr vm;
+virQEMUDriverPtr driver = domain->conn->privateData;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+vm = virDomainObjListFindByUUID(driver->domains, domain->uuid);
+if (!vm) {
+virUUIDFormat(domain->uuid, uuidstr);
+virReportError(VIR_ERR_NO_DOMAIN,
+   _("no domain with matching uuid '%s' (%s)"),
+   uuidstr, domain->name);
+return NULL;
+}
+
+return vm;
+}
+
+
 struct _qemuDomainLogContext {
 virObject parent;

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f53ea146e1..63d780e79b 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -581,6 +581,7 @@ struct _qemuDomainXmlNsDef {
 char **capsdel;
 };

+virDomainObjPtr qemuDomainObjFromDomain(virDomainPtr domain);

 qemuDomainSaveCookiePtr qemuDomainSaveCookieNew(virDomainObjPtr vm);

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0753904472..21ab58e4ec 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -161,42 +161,13 @@ static int qemuARPGetInterfaces(virDomainObjPtr vm,

 static virQEMUDriverPtr qemu_driver;

-/**
- * qemuDomObjFromDomain:
- * @domain: Domain pointer that has to be looked up
- *
- * This function looks up @domain and returns the appropriate virDomainObjPtr
- * that has to be released by calling virDomainObjEndAPI().
- *
- * Returns the domain object with incremented reference counter which is locked
- * on success, NULL otherwise.
- */
-static virDomainObjPtr
-qemuDomObjFromDomain(virDomainPtr domain)
-{
-virDomainObjPtr vm;
-virQEMUDriverPtr driver = domain->conn->privateData;
-char uuidstr[VIR_UUID_STRING_BUFLEN];
-
-vm = virDomainObjListFindByUUID(driver->domains, domain->uuid);
-if (!vm) {
-virUUIDFormat(domain->uuid, uuidstr);
-virReportError(VIR_ERR_NO_DOMAIN,
-   _("no domain with matching uuid '%s' (%s)"),
-   uuidstr, domain->name);
-return NULL;
-}
-
-return vm;
-}
-
 /* Looks up the domain object from snapshot and unlocks the
  * driver. The returned domain object is locked and ref'd and the
  * caller must call virDomainObjEndAPI() on it. */
 static virDomainObjPtr
 qemuDomObjFromSnapshot(virDomainSnapshotPtr snapshot)
 {
-return qemuDomObjFromDomain(snapshot->domain);
+return qemuDomainObjFromDomain(snapshot->domain);
 }


@@ -230,7 +201,7 @@ qemuSnapObjFromSnapshot(virDomainObjPtr vm,
 static virDomainObjPtr
 qemuDomObjFromCheckpoint(virDomainCheckpointPtr checkpoint)
 {
-return qemuDomObjFromDomain(checkpoint->domain);
+return qemuDomainObjFromDomain(checkpoint->domain);
 }


@@ -1742,7 +1713,7 @@ static int qemuDomainIsActive(virDomainPtr dom)
 virDomainObjPtr obj;
 int ret = -1;

-if (!(obj = qemuDomObjFromDomain(dom)))
+if (!(obj = qemuDomainObjFromDomain(dom)))
 goto cleanup;

 if (virDomainIsActiveEnsureACL(dom->conn, obj->def) < 0)
@@ -1760,7 +1731,7 @@ static int qemuDomainIsPersistent(virDomainPtr dom)
 virDomainObjPtr obj;
 int ret = -1;

-if (!(obj = qemuDomObjFromDomain(dom)))
+if (!(obj = qemuDomainObjFromDomain(dom)))
 goto cleanup;

 if (virDomainIsPersistentEnsureACL(dom->conn, obj->def) < 0)
@@ -1778,7 +1749,7 @@ static int qemuDomainIsUpdated(virDomainPtr dom)
 virDomainObjPtr obj;
 int ret = -1;

-if (!(obj = qemuDomObjFromDomain(dom)))
+if (!(obj = qemuDomainObjFromDomain(dom)))
 goto cleanup;

 if (virDomainIsUpdatedEnsureACL(dom->conn, obj->def) < 0)
@@ -1958,7 +1929,7 @@ static int qemuDomainSuspend(virDomainPtr dom)
 int state;
 virQEMUDriverConfigPtr cfg = NULL;

-if (!(vm = qemuDomObjFromDomain(dom)))
+if (!(vm = qemuDomainObjFromDomain(dom)))
 return -1;

 if (virDomainSuspendEnsureACL(dom->conn, vm->def) < 0)
@@ -2013,7 +1984,7 @@ static int qemuDomainResume(virDomainPtr dom)

[libvirt] [PATCH 4/7] qemu: driver: Remove misplaced qemuDomainObjEndJob in qemuDomainCheckpointGetXMLDesc

[Peter Krempa]

The code that gets the job to refresh disk sizes was not merged yet so
remove this artifact.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 69467c21f6..485b38ebac 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17547,9 +17547,6 @@ qemuDomainCheckpointGetXMLDesc(virDomainCheckpointPtr 
checkpoint,
 xml = virDomainCheckpointDefFormat(chkdef, driver->caps, driver->xmlopt,
format_flags);

-if (flags & VIR_DOMAIN_CHECKPOINT_XML_SIZE)
-qemuDomainObjEndJob(driver, vm);
-
  cleanup:
 virDomainObjEndAPI(&vm);
 return xml;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 6/7] qemu: domain: Move checkpoint related code to qemu_checkpoint.c

[Peter Krempa]

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_checkpoint.c | 185 ++---
 src/qemu/qemu_checkpoint.h |   5 +
 src/qemu/qemu_domain.c | 162 +---
 src/qemu/qemu_domain.h |  15 ---
 4 files changed, 180 insertions(+), 187 deletions(-)

diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index 9e9af76871..22f0ffb26e 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -76,6 +76,169 @@ qemuCheckpointObjFromCheckpoint(virDomainObjPtr vm,
 }


+static int
+qemuCheckpointWriteMetadata(virDomainObjPtr vm,
+virDomainMomentObjPtr checkpoint,
+virCapsPtr caps,
+virDomainXMLOptionPtr xmlopt,
+const char *checkpointDir)
+{
+unsigned int flags = VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE;
+virDomainCheckpointDefPtr def = virDomainCheckpointObjGetDef(checkpoint);
+VIR_AUTOFREE(char *) newxml = NULL;
+VIR_AUTOFREE(char *) chkDir = NULL;
+VIR_AUTOFREE(char *) chkFile = NULL;
+
+newxml = virDomainCheckpointDefFormat(def, caps, xmlopt, flags);
+if (newxml == NULL)
+return -1;
+
+if (virAsprintf(&chkDir, "%s/%s", checkpointDir, vm->def->name) < 0)
+return -1;
+if (virFileMakePath(chkDir) < 0) {
+virReportSystemError(errno, _("cannot create checkpoint directory 
'%s'"),
+ chkDir);
+return -1;
+}
+
+if (virAsprintf(&chkFile, "%s/%s.xml", chkDir, def->parent.name) < 0)
+return -1;
+
+return virXMLSaveFile(chkFile, NULL, "checkpoint-edit", newxml);
+}
+
+
+static int
+qemuDomainCheckpointDiscard(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+virDomainMomentObjPtr chk,
+bool update_parent,
+bool metadata_only)
+{
+virDomainMomentObjPtr parent = NULL;
+virDomainMomentObjPtr moment;
+virDomainCheckpointDefPtr parentdef = NULL;
+size_t i, j;
+VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
+VIR_AUTOFREE(char *) chkFile = NULL;
+
+if (!metadata_only && !virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("cannot remove checkpoint from inactive domain"));
+return -1;
+}
+
+if (virAsprintf(&chkFile, "%s/%s/%s.xml", cfg->checkpointDir,
+vm->def->name, chk->def->name) < 0)
+return -1;
+
+if (!metadata_only) {
+qemuDomainObjPrivatePtr priv = vm->privateData;
+bool success = true;
+bool search_parents;
+virDomainCheckpointDefPtr chkdef = virDomainCheckpointObjGetDef(chk);
+
+qemuDomainObjEnterMonitor(driver, vm);
+parent = virDomainCheckpointFindByName(vm->checkpoints,
+   chk->def->parent_name);
+for (i = 0; i < chkdef->ndisks; i++) {
+virDomainCheckpointDiskDef *disk = &chkdef->disks[i];
+const char *node;
+
+if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
+continue;
+
+node = qemuDomainDiskNodeFormatLookup(vm, disk->name);
+/* If any ancestor checkpoint has a bitmap for the same
+ * disk, then this bitmap must be merged to the
+ * ancestor. */
+search_parents = true;
+for (moment = parent;
+ search_parents && moment;
+ moment = virDomainCheckpointFindByName(vm->checkpoints,
+
parentdef->parent.parent_name)) {
+parentdef = virDomainCheckpointObjGetDef(moment);
+for (j = 0; j < parentdef->ndisks; j++) {
+virDomainCheckpointDiskDef *disk2;
+VIR_AUTOPTR(virJSONValue) arr = NULL;
+
+disk2 = &parentdef->disks[j];
+if (STRNEQ(disk->name, disk2->name) ||
+disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
+continue;
+search_parents = false;
+
+arr = virJSONValueNewArray();
+if (!arr ||
+virJSONValueArrayAppendString(arr, disk->bitmap) < 0) {
+success = false;
+break;
+}
+if (chk == virDomainCheckpointGetCurrent(vm->checkpoints) 
&&
+qemuMonitorEnableBitmap(priv->mon, node,
+disk2->bitmap) < 0) {
+success = false;
+break;
+}
+if (qemuMonitorMergeBitmaps(priv->mon, node,
+disk2->bitmap, &arr) < 0) {
+

[libvirt] [PATCH 0/7] qemu: checkpoints: Collect most code in a single place

[Peter Krempa]

The checkpoint code is quite complex and was dispersed in many places.
Refactor it to be in one new separate file.

I also plan to do the same to the snapshot code once this is dealt with.

Additionally aggregating all the code in one place will allow
refactoring and reuse in the incremental backup implementation.

Peter Krempa (7):
  qemu: Move, rename and export qemuDomObjFromDomain
  conf: Drop pointless 'domain' argument from
virDomainCheckpointRedefinePrep
  conf: Drop pointless 'domain' argument from
virDomainSnapshotRedefinePrep
  qemu: driver: Remove misplaced qemuDomainObjEndJob in
qemuDomainCheckpointGetXMLDesc
  qemu: driver: Move checkpoint-related code to qemu_checkpoint.c
  qemu: domain: Move checkpoint related code to qemu_checkpoint.c
  qemu: driver: Don't pull in qemu_monitor_json.h directly

 src/conf/checkpoint_conf.c |   7 +-
 src/conf/checkpoint_conf.h |   3 +-
 src/conf/snapshot_conf.c   |   5 +-
 src/conf/snapshot_conf.h   |   3 +-
 src/qemu/Makefile.inc.am   |   2 +
 src/qemu/qemu_checkpoint.c | 646 ++
 src/qemu/qemu_checkpoint.h |  55 +++
 src/qemu/qemu_domain.c | 193 ++-
 src/qemu/qemu_domain.h |  16 +-
 src/qemu/qemu_driver.c | 686 -
 src/test/test_driver.c |   4 +-
 11 files changed, 887 insertions(+), 733 deletions(-)
 create mode 100644 src/qemu/qemu_checkpoint.c
 create mode 100644 src/qemu/qemu_checkpoint.h

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 7/7] qemu: driver: Don't pull in qemu_monitor_json.h directly

[Peter Krempa]

There's nothing that uses it directly now. Also not allowing direct use
will promote our layering.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 89a0b0963d..186b6ed528 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -47,7 +47,6 @@
 #include "qemu_hostdev.h"
 #include "qemu_hotplug.h"
 #include "qemu_monitor.h"
-#include "qemu_monitor_json.h"
 #include "qemu_process.h"
 #include "qemu_migration.h"
 #include "qemu_migration_params.h"
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/7] qemu: driver: Move checkpoint-related code to qemu_checkpoint.c

[Peter Krempa]

Move all extensive functions to a new file so that we don't just pile
everything in the common files. This obviously isn't possible with
straight code movement as we still need stubs in qemu_driver.c

Additionally some functions e.g. for looking up a checkpoint by name
were so short that moving the impl didn't make sense.

Note that in the move the new file also doesn't use
virQEMUMomentReparent but rather an stripped down copy. As I plan to
split out snapshot code into a separate file the unification doesn't
make sense any more.

Signed-off-by: Peter Krempa 
---
 src/qemu/Makefile.inc.am   |   2 +
 src/qemu/qemu_checkpoint.c | 483 +
 src/qemu/qemu_checkpoint.h |  50 
 src/qemu/qemu_driver.c | 379 +
 4 files changed, 540 insertions(+), 374 deletions(-)
 create mode 100644 src/qemu/qemu_checkpoint.c
 create mode 100644 src/qemu/qemu_checkpoint.h

diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 48fd0332ec..71fb07b638 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -62,6 +62,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_slirp.h \
qemu/qemu_tpm.c \
qemu/qemu_tpm.h \
+   qemu/qemu_checkpoint.c \
+   qemu/qemu_checkpoint.h \
$(NULL)


diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
new file mode 100644
index 00..9e9af76871
--- /dev/null
+++ b/src/qemu/qemu_checkpoint.c
@@ -0,0 +1,483 @@
+/*
+ * qemu_checkpoint.c: checkpoint related implementation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+
+#include "qemu_checkpoint.h"
+#include "qemu_capabilities.h"
+#include "qemu_monitor.h"
+#include "qemu_monitor_json.h"
+#include "qemu_domain.h"
+
+#include "virerror.h"
+#include "virlog.h"
+#include "datatypes.h"
+#include "viralloc.h"
+#include "domain_conf.h"
+#include "libvirt_internal.h"
+#include "virxml.h"
+#include "virstring.h"
+#include "virdomaincheckpointobjlist.h"
+#include "virdomainsnapshotobjlist.h"
+
+#define VIR_FROM_THIS VIR_FROM_QEMU
+
+VIR_LOG_INIT("qemu.qemu_checkpoint");
+
+/* Looks up the domain object from checkpoint and unlocks the
+ * driver. The returned domain object is locked and ref'd and the
+ * caller must call virDomainObjEndAPI() on it. */
+virDomainObjPtr
+qemuDomObjFromCheckpoint(virDomainCheckpointPtr checkpoint)
+{
+return qemuDomainObjFromDomain(checkpoint->domain);
+}
+
+
+/* Looks up checkpoint object from VM and name */
+virDomainMomentObjPtr
+qemuCheckpointObjFromName(virDomainObjPtr vm,
+  const char *name)
+{
+virDomainMomentObjPtr chk = NULL;
+chk = virDomainCheckpointFindByName(vm->checkpoints, name);
+if (!chk)
+virReportError(VIR_ERR_NO_DOMAIN_CHECKPOINT,
+   _("no domain checkpoint with matching name '%s'"),
+   name);
+
+return chk;
+}
+
+
+/* Looks up checkpoint object from VM and checkpointPtr */
+virDomainMomentObjPtr
+qemuCheckpointObjFromCheckpoint(virDomainObjPtr vm,
+virDomainCheckpointPtr checkpoint)
+{
+return qemuCheckpointObjFromName(vm, checkpoint->name);
+}
+
+
+/* Called inside job lock */
+static int
+qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr caps,
+virDomainObjPtr vm,
+virDomainCheckpointDefPtr def)
+{
+int ret = -1;
+size_t i;
+char *xml = NULL;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+/* Easiest way to clone inactive portion of vm->def is via
+ * conversion in and back out of xml.  */
+if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps,
+vm->def, priv->origCPU,
+true, true)) ||
+!(def->parent.dom = virDomainDefParseString(xml, caps, driver->xmlopt,
+priv->qemuCaps,
+
VIR_DOMAIN_DEF_PARSE_INACTIVE |
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
+goto cleanup;
+
+if (virDomainCheckpointAlignDisks(def) < 0)
+goto cleanup;
+
+for (i = 0; i < def->ndisks; i++) {
+virDomainCheckpointDiskDefPtr disk = &def-

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Pavel Hrdina]

On Fri, Sep 20, 2019 at 01:28:28PM +0200, Fabiano Fidêncio wrote:
> On Fri, Sep 20, 2019 at 11:05 AM Pavel Hrdina  wrote:
> >
> > Meson build system is simple and quick compared to Autotools and it's
> > able to fully replace our Autotools usage.  There are few drawbacks as
> > it's a fairly new build system, it requires Python 3.5 and Ninja 1.5.0,
> > it's still evolving and the user base is not that large and there were
> > some tweaks required to achieve the same functionality.
> >
> > However, there are benefits, the configure and build time is way shorter
> > and build definition files are more readable and easier to maintain.
> >
> > There are some major changes with Meson build system:
> >
> > - there is no syntax-check target, the syntax-check is part of Meson
> >   test suite but it's still possible to run it separately,
> >
> > - Meson forces separation between source and build directories
> >
> > Signed-off-by: Pavel Hrdina 
> > Tested-by: Ján Tomko 
> > ---
> >
> > Notes:
> > changes in v2:
> >
> > - add -Werror if we are building from git
> > - fixed -Wframe-larger-than
> > - removed unrelated fix
> > - added comment for flake8 ignore warning
> > - added 'suite' labels 'syntax' and 'unit' for tests
> > - AUTHORS and libvirt-dbus.spec are generated only when building 
> > from git
> 
> Why? Not opposed to this decision, just want to understand the reason for 
> that.

This is what was done by Autotools, at least for AUTHORS.  If you are
compiling libvirt-dbus from tarball you don't have git so you will not
be able to generate AUTHORS.  For the spec file my idea was that there
is no need to regenerate it from tarball as that one is usually used as
it is.

> > - run.in is no longer executable, there is a helper script to fix 
> > permissions
> >   for the generated run script
> > - fixed include_directories for test executable, direct paths can 
> > be used
> >   since meson 0.50.0
> 
> By using 0.50.0 you're dropping support on:
> - Debian 10;
> - Ubuntu 19.04;
> 
> Personally, I would stick to 0.49.0.

Ignored per following email :).

> > - flake8 is optional as it was with autotools
> > - added meson version into spec file

[...]

> > --- a/autogen.sh
> > +++ /dev/null
> > @@ -1,52 +0,0 @@
> > -#!/bin/sh
> > -# Run this to generate all the initial makefiles, etc.
> > -
> > -set -e
> > -srcdir=`dirname $0`
> > -test -z "$srcdir" && srcdir=.
> > -
> > -THEDIR=`pwd`
> > -cd $srcdir
> > -
> > -DIE=0
> > -
> > -for prog in autoreconf automake autoconf libtoolize
> > -do
> > -($prog --version) < /dev/null > /dev/null 2>&1 || {
> > -echo
> > -echo "You must have $prog installed to compile libvirt-dbus."
> > -DIE=1
> > -}
> > -done
> > -
> > -if test "$DIE" -eq 1; then
> > -exit 1
> > -fi
> > -
> > -if test -z "$*"; then
> > -echo "I am going to run ./configure with no args - if you "
> > -echo "wish to pass any extra arguments to it, please specify them 
> > on "
> > -echo "the $0 command line."
> > -fi
> > -
> > -mkdir -p build-aux
> > -autoreconf -if
> > -
> > -cd $THEDIR
> > -
> > -if test "x$1" = "x--system"; then
> > -shift
> > -prefix=/usr
> > -libdir=$prefix/lib
> > -sysconfdir=/etc
> > -localstatedir=/var
> > -if [ -d /usr/lib64 ]; then
> > -  libdir=$prefix/lib64
> > -fi
> > -EXTRA_ARGS="--prefix=$prefix --sysconfdir=$sysconfdir 
> > --localstatedir=$localstatedir --libdir=$libdir"
> > -fi
> 
> --system ended up being remove here but never added back.
> AFAIU that's a pattern followed by libvirt* projects and, if that's
> the case, may be worth to adding it back.

Good point, should have mentioned it in the commit message.
The --system option is usually used only by libvirt developers if they
need to run libvirt from git with system configuration files, I doubt
that anyone is actually using it with libvirt-dbus as there is probably
no need at all.

> > -
> > -$srcdir/configure $EXTRA_ARGS "$@" && {
> > -echo
> > -echo "Now type 'make' to compile libvirt-dbus."
> > -}

[...]

> > diff --git a/meson.build b/meson.build
> > new file mode 100644
> > index 000..30eeebe
> > --- /dev/null
> > +++ b/meson.build
> > @@ -0,0 +1,279 @@
> > +project(
> > +'libvirt-dbus', 'c',
> > +version: '1.4.0',
> > +license: 'LGPLv2+',
> > +meson_version: '>= 0.49.0',
> 
> In case you're really going for 0.50.0, this needs to be bumped ...
> 
> > +default_options: [
> > +'buildtype=debugoptimized',
> > +'c_std=gnu99',
> > +],
> > +)
> > +
> > +
> > +conf = configuration_data()
> > +conf.set('MESON_VERSION', '0.49.0')
> 
> ... and this one as well ...
> 
> > +conf.set('PACKAGE', meson.project_name())
> > +conf.set('VERSION', meson.project_version())
> > +conf.set('build_root', meson.build_root())
> > +conf.set('sbindir', get_option('sbindir'))
> > +conf.set(

Re: [libvirt] [dbus PATCH v2] build: convert to Meson/Ninja build system

[Fabiano Fidêncio]

[snip]

>
> > In general, it looks good and works as expected.
> > I will add my "Reviewed-by: " after we discuss the points raised.
> >
> > Another thing, please, let's sync to have the libvirt-jenkins-ci work
> > done and merged before this one gets merged.
>
> Works for me, thanks for review, I'll fix the dist script to use a shell
> script.
>

Reviewed-by: Fabiano Fidêncio 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] remote: fix enablement of IP networking in virtproxyd

[Andrea Bolognani]

On Fri, 2019-09-20 at 13:12 +0100, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/remote/Makefile.inc.am | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Fixes b7ed8ce98112.

  Reviewed-by: Andrea Bolognani 

In a separate patch, you might want to s/CUT_ENABLE_IP/CUT_WITH_IP/g
as well, for consistency.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] virpci, qemu_domain_address: adding virPCIDeviceSetAddress

[Daniel Henrique Barboza]

On 9/20/19 6:25 AM, Erik Skultety wrote:

On Thu, Sep 19, 2019 at 05:04:47PM -0300, Daniel Henrique Barboza wrote:

A common operation in qemu_domain_address is comparing a
virPCIDeviceAddress and assigning domain, bus, slot and function
to a specific value. The former can be done with the existing
virPCIDeviceAddressEqual() helper.

A new virpci helper called virPCIDeviceSetAddress() was created to
make it easier to assign the same domain/bus/slot/function of an
already existent virPCIDeviceAddress. Using both in
qemu_domain_address made the code tidier, since the object
was already created to use virPCIDeviceAddressEqual().

Signed-off-by: Daniel Henrique Barboza 
---

I wasn't sure if this change was worth contributing it back,
since it feels more like a 'look and feel' change. But
since it made the code inside qemu_domain_address tidier,
I decided to take a shot.

I actually think it does enhance readability, so why not. However...

[...]

--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr 
def,
  /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 
1 */
  for (i = 0; i < def->ncontrollers; i++) {
  virDomainControllerDefPtr cont = def->controllers[i];
+virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0,
+  .slot = 1, .function = 1};
+virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0,
+.slot = 1, .function = 2};

  /* First IDE controller lives on the PIIX3 at slot=1, function=1 */
  if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
  cont->idx == 0) {
  if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
-if (cont->info.addr.pci.domain != 0 ||
-cont->info.addr.pci.bus != 0 ||
-cont->info.addr.pci.slot != 1 ||
-cont->info.addr.pci.function != 1) {
+if (!virPCIDeviceAddressEqual(&cont->info.addr.pci,
+  &primaryIDEAddr)) {

I think this virPCIDeviceAddressEqual consolidation might be a separate patch
(matter of taste, feel free to disagree, but to me the changes are not strictly
related and can be easily split).


  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Primary IDE controller must have PCI address 
0:0:1.1"));
+   _("Primary IDE controller must have PCI "
+ "address 0:0:1.1"));

Unrelated...but it can stay...


  return -1;
  }
  } else {
  cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
-cont->info.addr.pci.domain = 0;
-cont->info.addr.pci.bus = 0;
-cont->info.addr.pci.slot = 1;
-cont->info.addr.pci.function = 1;
+virPCIDeviceSetAddress(&cont->info.addr.pci, &primaryIDEAddr);

So, given the signature, it feels more like a Copy rather than Set. I imagine a
setter to enumerate the scalar types and the destination where the values
should end up in, in this regard, it's not a proper setter because the struct
has a few more elements that technically can be set, but if you created such a
setter I'd say we didn't gain anything in terms of readability at all,
especially when you could do:

/* provided that I'm interpreting 6.7.8.19 [1] of the C99 standard correctly,
  * everything else we're not using should be 0, otherwise we can't do this.
  * Alternatively, you'd need to reset the other members as well */
cont->info.addr.pci = primaryIDEAddr;


That was my first idea before trying to come out with this "partial setter"
so to speak - since we're setting only domain/bus/slot/function, I wanted
to avoid setting the other values as well.

But now that I thought bit more about the context I believe the direct
assignment here is fine indeed. Here's a snippet of the code I'm
changing:


    if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
    (code to check if address is equal to X ...)

    } else {
 /* assign address to X */


AddressIsPresent checks for info->type=TYPE_PCI and if 
addr.pci{domain|bus|slot}
is not zero*. It didn't check if multi,ext and zpci was filled, but the 
current
code doesn't care much either (otherwise the existing code would be 
setting those
as well). My conclusion is that, at this point, these other attributes 
are either

irrelevant or will be set up to later on, thus the default 0 values are fine
here.

I'll re-send this dude with direct assignment and without this extra setter
function I came up with inside vircpi.c. Thanks for the review!



DHB






I'm not completely sold on the helper the way it is now, but I'm up for

[libvirt] [PATCH v2 0/2] a few cleanups in qemu_domain_address.c

[Daniel Henrique Barboza]

Second round of these cleanups after the reviews from
Eric.


changes from v1:
- fixed the commit message of patch 1
- ditched the virPCIAddressSet() approach - use direct
assignment instead


v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00823.html 

Daniel Henrique Barboza (2):
  qemu_domain_address.c: use VIR_AUTOFREE() in strings
  qemu_domain_address: use virPCIDeviceAddressEqual() in conditionals

 src/qemu/qemu_domain_address.c | 125 +++--
 1 file changed, 55 insertions(+), 70 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/2] qemu_domain_address.c: use VIR_AUTOFREE() in strings

[Daniel Henrique Barboza]

A few 'cleanup' labels gone after using VIR_AUTOFREE() on the
@addrStr variable.

Reviewed-by: Erik Skultety 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain_address.c | 69 +++---
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 216ba6235e..e20481607f 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1524,12 +1524,11 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
  * inappropriate address types.
  */
 if (!info->pciConnectFlags) {
-char *addrStr = virPCIDeviceAddressAsString(&info->addr.pci);
+VIR_AUTOFREE(char *) addrStr = 
virPCIDeviceAddressAsString(&info->addr.pci);
 
 VIR_WARN("qemuDomainDeviceCalculatePCIConnectFlags() thinks that the "
  "device with PCI address %s should not have a PCI address",
  addrStr ? addrStr : "(unknown)");
-VIR_FREE(addrStr);
 
 info->pciConnectFlags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE;
 }
@@ -1720,11 +1719,10 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr 
def,
   virQEMUCapsPtr qemuCaps,
   virDomainPCIAddressSetPtr addrs)
 {
-int ret = -1;
 size_t i;
 virPCIDeviceAddress tmp_addr;
 bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
-char *addrStr = NULL;
+VIR_AUTOFREE(char *) addrStr = NULL;
 virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE
   | VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
 
@@ -1742,7 +1740,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
 cont->info.addr.pci.function != 1) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Primary IDE controller must have PCI 
address 0:0:1.1"));
-goto cleanup;
+return -1;
 }
 } else {
 cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
@@ -1762,7 +1760,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
 cont->info.addr.pci.function != 2) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("PIIX3 USB controller at index 0 must 
have PCI address 0:0:1.2"));
-goto cleanup;
+return -1;
 }
 } else {
 cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
@@ -1777,7 +1775,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
 }
 if (addrs->nbuses &&
 virDomainPCIAddressReserveAddr(addrs, &cont->info.addr.pci, flags, 
0) < 0)
-goto cleanup;
+return -1;
 }
 
 /* Implicit PIIX3 devices living on slot 1 not handled above */
@@ -1786,11 +1784,11 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr 
def,
 tmp_addr.slot = 1;
 /* ISA Bridge at 00:01.0 */
 if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0)
-goto cleanup;
+return -1;
 /* Bridge at 00:01.3 */
 tmp_addr.function = 3;
 if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0)
-goto cleanup;
+return -1;
 }
 
 if (def->nvideos > 0 &&
@@ -1808,26 +1806,26 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr 
def,
 tmp_addr.slot = 2;
 
 if (!(addrStr = virPCIDeviceAddressAsString(&tmp_addr)))
-goto cleanup;
+return -1;
 if (!virDomainPCIAddressValidate(addrs, &tmp_addr,
  addrStr, flags, true))
-goto cleanup;
+return -1;
 
 if (virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) {
 if (qemuDeviceVideoUsable) {
 if (qemuDomainPCIAddressReserveNextAddr(addrs,
 
&primaryVideo->info) < 0) {
-goto cleanup;
+return -1;
 }
 } else {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("PCI address 0:0:2.0 is in use, "
  "QEMU needs it for primary video"));
-goto cleanup;
+return -1;
 }
 } else {
 if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) 
< 0)
-goto cleanup;
+return -1;
 primaryVideo->info.addr.pci = tmp_addr;
 primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
 }
@@ -1838,7

[libvirt] [PATCH v2 2/2] qemu_domain_address: use virPCIDeviceAddressEqual() in conditionals

[Daniel Henrique Barboza]

A common operation in qemu_domain_address is comparing a
virPCIDeviceAddress and assigning domain, bus, slot and function
to a specific value. The former can be done with the existing
virPCIDeviceAddressEqual() helper, as long as we provide
a virPCIDeviceAddress to compare it to.

The later can be done by direct assignment of the now existing
virPCIDeviceAddress struct. The defined values of domain, bus,
slot and function will be assigned to info->addr.pci, the other
values are zeroed (which happens to be their default values too).
It's also worth noticing that all these assignments are being
conditioned by virDeviceInfoPCIAddressIsPresent() calls, thus it's
sensible to discard any non-zero values that might happen to exist
in @cont->info.addr, if we settled beforehand that @cont->info.addr
is not present or bogus.

Suggested-by: Erik Skultety 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain_address.c | 56 +++---
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e20481607f..4f9ae1f37d 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr 
def,
 /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 
1 */
 for (i = 0; i < def->ncontrollers; i++) {
 virDomainControllerDefPtr cont = def->controllers[i];
+virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0,
+  .slot = 1, .function = 1};
+virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0,
+.slot = 1, .function = 2};
 
 /* First IDE controller lives on the PIIX3 at slot=1, function=1 */
 if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE &&
 cont->idx == 0) {
 if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
-if (cont->info.addr.pci.domain != 0 ||
-cont->info.addr.pci.bus != 0 ||
-cont->info.addr.pci.slot != 1 ||
-cont->info.addr.pci.function != 1) {
+if (!virPCIDeviceAddressEqual(&cont->info.addr.pci,
+  &primaryIDEAddr)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Primary IDE controller must have PCI 
address 0:0:1.1"));
+   _("Primary IDE controller must have PCI "
+ "address 0:0:1.1"));
 return -1;
 }
 } else {
 cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
-cont->info.addr.pci.domain = 0;
-cont->info.addr.pci.bus = 0;
-cont->info.addr.pci.slot = 1;
-cont->info.addr.pci.function = 1;
+cont->info.addr.pci = primaryIDEAddr;
 }
 } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
cont->idx == 0 &&
(cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI 
||
 cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT)) {
 if (virDeviceInfoPCIAddressIsPresent(&cont->info)) {
-if (cont->info.addr.pci.domain != 0 ||
-cont->info.addr.pci.bus != 0 ||
-cont->info.addr.pci.slot != 1 ||
-cont->info.addr.pci.function != 2) {
+if (!virPCIDeviceAddressEqual(&cont->info.addr.pci,
+  &piix3USBAddr)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("PIIX3 USB controller at index 0 must 
have PCI address 0:0:1.2"));
+   _("PIIX3 USB controller at index 0 must "
+ "have PCI address 0:0:1.2"));
 return -1;
 }
 } else {
 cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
-cont->info.addr.pci.domain = 0;
-cont->info.addr.pci.bus = 0;
-cont->info.addr.pci.slot = 1;
-cont->info.addr.pci.function = 2;
+cont->info.addr.pci = piix3USBAddr;
 }
 } else {
 /* this controller is not skipped in qemuDomainCollectPCIAddress */
@@ -1800,6 +1796,8 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
  * at slot 2.
  */
 virDomainVideoDefPtr primaryVideo = def->videos[0];
+virPCIDeviceAddress primaryCardAddr = {.domain = 0, .bus = 0,
+   .slot = 2, .function = 0};
 
 if (virDeviceInfoPCIAddressIsWanted(&primaryVideo->info)) {
 mem

[libvirt] [PATCH] conf: reattach interface taps to correct bridge on restart

[Laine Stump]

When the bridge re-attach handling was moved out of the network driver
and into the hypervisor driver (commit b806a60e) as a part of the
refactor to split the network driver into a separate daemon, the check
was accidentally changed to only check for type='bridge'. The check for
type in this case needs to check for type='network' as well.

(at the time we thought that type='network' and type='bridge' could be
conflated for interface actual type, but this turned out to be too
problematic to do).

Signed-off-by: Laine Stump 
---

(NB: I thought I remembered seeing a bugzilla report go by about this
regression, but couldn't find it in any of the places I frequent
(upstream, Fedora, or RHEL) If someone can locate it and wants to let
me know the BZ#, I can add it to the commit message and update the
report).

(This fixes the reconnect of taps to their bridges, but the count
maintained in the network object still isn't being updated in these
cases. I've tried removing the !virUUIDIsValid() check in this same
chunk, along with preserving the original port uuid if it's already
valid in virDomainNetDefActualToNetworkPort(), and that results in
fixing the usage count for type='network' when it's a libvirt-managed
bridge or a macvtap passthrough pool, but leads to errors in other
cases.)


 src/conf/domain_conf.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 848c831330..24223bceb2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30971,13 +30971,16 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
virDomainDefPtr dom,
virDomainNetDefPtr iface)
 {
+virDomainNetType actualType = virDomainNetGetActualType(iface);
+
 if (!virUUIDIsValid(iface->data.network.portid)) {
 if (virDomainNetCreatePort(conn, dom, iface,
VIR_NETWORK_PORT_CREATE_RECLAIM) < 0)
 return;
 }
 
-if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
+actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
 /*
  * NB: we can't notify the guest of any MTU change anyway,
  * so there is no point in trying to learn the actualMTU
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] docs: attempt to document the general libvirt dev strategy

[Daniel P . Berrangé]

There are various ideas / plans floating around for future libvirt work,
some of which is actively in progress. Historically we've never captured
this kind of information anywhere, except in mailing list discussions.
In particular guidelines in hacking.html.in don't appear until a policy
is actively applied.

This patch attempts to fill the documentation gap, by creating a new
"strategy" page which outlines the general vision for some notable
future changes. The key thing to note is that none of the stuff on this
page is guaranteed, plans may change as new information arises. IOW this
is a "best guess" as to the desired future.

This doc has focused on three areas, related to the topic of language
usage / consolidation

 - Use of non-C languages for the library, daemons or helper tools
 - Replacement of autotools with meson
 - Use of RST and Sphinx for documentation (website + man pages)

Signed-off-by: Daniel P. Berrangé 
---
 docs/docs.html.in |   3 +
 docs/strategy.html.in | 143 ++
 2 files changed, 146 insertions(+)
 create mode 100644 docs/strategy.html.in

diff --git a/docs/docs.html.in b/docs/docs.html.in
index c1741c89b4..6cf09f51bc 100644
--- a/docs/docs.html.in
+++ b/docs/docs.html.in
@@ -128,6 +128,9 @@
 Contributor guidelines
 General hacking guidelines for contributors
 
+Project strategy
+Sets a vision for future direction & technical choices
+
 Bug reports
 How and where to report bugs and request features
 
diff --git a/docs/strategy.html.in b/docs/strategy.html.in
new file mode 100644
index 00..41bbebb8f9
--- /dev/null
+++ b/docs/strategy.html.in
@@ -0,0 +1,143 @@
+
+
+http://www.w3.org/1999/xhtml";>
+  
+Libvirt Project Strategy
+
+
+  This document attempts to outline the libvirt project strategy for
+  the near future. Think of this is a high level vision or todo list
+  setting the direction for the project & its developers to take
+
+
+Language consolidation
+
+
+  At time of writing libvirt uses the following languages
+
+
+
+  C
+  The core libvirt library, daemons, and helper tools are all written
+in the C language.
+  Python
+  Various supporting build/test scripts are written in Python, with
+compatibility for Python 2 and 3.
+  Perl
+  Various supporting build/test scripts are written in Python. It is
+also used for many syntax-check inline rules
+  Shell
+  The autoconf configure script, is essentially shell script. Shell
+is also used for some simple build/test scripts. At runtime libvirt
+avoids shell except when using SSH tunnels to a remote host
+  XSLT
+  The website documentation is subsituted into templates using XSLT
+scripts. The API documentation is also autogenerated from an XML
+description using XSLT
+  HTML
+  The website documentation is all written in plain HTML. Some HTML
+is also auto-generated for API documentation
+  M4
+  The autoconf configure script uses a large number of M4 macros to
+generate its content
+  make
+  The core build system uses the traditional GNU make recipes
+  automake
+  The make recipes make use of automake's make language extensions
+which are then turned into regular make rules
+  awk/sed
+  A number of the syntax-check inline rules involve use of awk/sed
+scripts
+  POD
+  The command line manual pages are typically written in Perl's POD
+format, and converted to troff
+
+
+
+  The wide range of languages used present a knowledge burden for
+  developers involved in libvirt, especially when there are multiple
+  languages all used in the same problem spaces. This is most notable
+  in the build system which uses a combination of shell, m4, make,
+  automake, awk, sed, perl and python, with debugging requiring
+  understanding of the interactions between many languages. The
+  relative popularity of the languages has implications for how easily
+  the project can attract new contributors, and the quality of the code
+  they'll be able to submit. This is most notable when comparing Perl
+  to Python, as since the start of the libvirt project, the popularity
+  and knowledge of Perl has declined, while Python has risen.
+
+
+  The C language has served libvirt well over the years, but its age shows
+  giving rise to limitations which negatively impact the project in terms
+  of code quality, reliability, and efficiency of development. Most notably
+  its lack of memory safety means that many code bugs become trivially
+  exploitable security flaws or denial of service. The lack of a high
+  level portable runtime, resulting in alot of effort being spent to
+  ensure cross platform portability. The modern languages Rust and Go

Re: [libvirt] [PATCH] conf: reattach interface taps to correct bridge on restart

[Daniel P . Berrangé]

On Fri, Sep 20, 2019 at 10:06:35AM -0400, Laine Stump wrote:
> When the bridge re-attach handling was moved out of the network driver
> and into the hypervisor driver (commit b806a60e) as a part of the
> refactor to split the network driver into a separate daemon, the check
> was accidentally changed to only check for type='bridge'. The check for
> type in this case needs to check for type='network' as well.
> 
> (at the time we thought that type='network' and type='bridge' could be
> conflated for interface actual type, but this turned out to be too
> problematic to do).
> 
> Signed-off-by: Laine Stump 

Reviewed-by: Daniel P. Berrangé 

> (This fixes the reconnect of taps to their bridges, but the count
> maintained in the network object still isn't being updated in these
> cases. I've tried removing the !virUUIDIsValid() check in this same
> chunk, along with preserving the original port uuid if it's already
> valid in virDomainNetDefActualToNetworkPort(), and that results in
> fixing the usage count for type='network' when it's a libvirt-managed
> bridge or a macvtap passthrough pool, but leads to errors in other
> cases.)

IIUC, we need to do is use something like

   bool reclaim = false;
   if (!virUUIDIsValid(portid)) {
 reclaim = true;
   } else {
 port = virNetworkLookupPortByUUID(net, portid);
 if (port == NULL)
   reclaim = true;
 else
   virObjectUnref(port)
   }

   if (reclaim)
  virDomainNetCreatePort...

> 
> 
>  src/conf/domain_conf.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 848c831330..24223bceb2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30971,13 +30971,16 @@ virDomainNetNotifyActualDevice(virConnectPtr conn,
> virDomainDefPtr dom,
> virDomainNetDefPtr iface)
>  {
> +virDomainNetType actualType = virDomainNetGetActualType(iface);
> +
>  if (!virUUIDIsValid(iface->data.network.portid)) {
>  if (virDomainNetCreatePort(conn, dom, iface,
> VIR_NETWORK_PORT_CREATE_RECLAIM) < 0)
>  return;
>  }
>  
> -if (virDomainNetGetActualType(iface) == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> +actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>  /*
>   * NB: we can't notify the guest of any MTU change anyway,
>   * so there is no point in trying to learn the actualMTU
> -- 
> 2.21.0
> 

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] docs: attempt to document the general libvirt dev strategy

[Peter Krempa]

On Fri, Sep 20, 2019 at 15:12:01 +0100, Daniel Berrange wrote:
> There are various ideas / plans floating around for future libvirt work,
> some of which is actively in progress. Historically we've never captured
> this kind of information anywhere, except in mailing list discussions.
> In particular guidelines in hacking.html.in don't appear until a policy
> is actively applied.
> 
> This patch attempts to fill the documentation gap, by creating a new
> "strategy" page which outlines the general vision for some notable
> future changes. The key thing to note is that none of the stuff on this
> page is guaranteed, plans may change as new information arises. IOW this
> is a "best guess" as to the desired future.
> 
> This doc has focused on three areas, related to the topic of language
> usage / consolidation
> 
>  - Use of non-C languages for the library, daemons or helper tools
>  - Replacement of autotools with meson
>  - Use of RST and Sphinx for documentation (website + man pages)
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/docs.html.in |   3 +
>  docs/strategy.html.in | 143 ++
>  2 files changed, 146 insertions(+)
>  create mode 100644 docs/strategy.html.in

[...]

> +
> +  The C language has served libvirt well over the years, but its age 
> shows
> +  giving rise to limitations which negatively impact the project in terms
> +  of code quality, reliability, and efficiency of development. Most 
> notably
> +  its lack of memory safety means that many code bugs become trivially
> +  exploitable security flaws or denial of service. The lack of a high
> +  level portable runtime, resulting in alot of effort being spent to
> +  ensure cross platform portability. The modern languages Rust and Go
> +  provide viable options for low level systems programming, in a way that
> +  was not practical with other common languages such as Python and Java.
> +  There is thus a desire to make use of either Rust or Go, or more likely
> +  a combination of both, to incrementally replace existing use of C, and
> +  also for greenfield development.

Please emphasise that senseless rewrite of old code, while being "memory
safe", may introduce new logic bugs which are hard to find in many cases.

It should be well justified that rewriting some functionality has clear
maintenance and upkeep benefits even when we consider the cost logic
bugs which will eventually be introduced in any refactor. This goes for
any rewrite.



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] docs: attempt to document the general libvirt dev strategy

[Andrea Bolognani]

On Fri, 2019-09-20 at 15:12 +0100, Daniel P. Berrangé wrote:
[...]
> +  This document attempts to outline the libvirt project strategy for
> +  the near future. Think of this is a high level vision or todo list

s/is a high/as a high/

> +  setting the direction for the project & its developers to take

Missing period at the end of the sentence.

Also:

  strategy.html.in:10: parser error : xmlParseEntityRef: no name
setting the direction for the project & its developers to take
  ^

I would just use "and" anyway.

[...]
> +  Perl
> +  Various supporting build/test scripts are written in Python. It is

s/Python/Perl/

[...]
> +  XSLT
> +  The website documentation is subsituted into templates using XSLT
> +scripts.

s/subsituted/substituted/

But I would probably reword it along the lines of

  The website uses XSLT for its templating system.

[...]
> +  automake
> +  The make recipes make use of automake's make language extensions
> +which are then turned into regular make rules

Let's try to avoid using "make" every other word. How about something
like

  automake extends the capabilities of GNU make by introducing a
  declarative syntax that can be used to describe recipes

[...]
> +  POD
> +  The command line manual pages are typically written in Perl's POD
> +format, and converted to troff

  strategy.html.in:53: parser error : Opening and ending tag mismatch: dd line 
52 and dt
format, and converted to troff
   ^

[...]
> +  The C language has served libvirt well over the years, but its age 
> shows
> +  giving rise to limitations which negatively impact the project in terms
> +  of code quality, reliability, and efficiency of development. Most 
> notably
> +  its lack of memory safety means that many code bugs become trivially
> +  exploitable security flaws or denial of service. The lack of a high
> +  level portable runtime, resulting in alot of effort being spent to

s/alot/a lot/

> +  ensure cross platform portability. The modern languages Rust and Go
> +  provide viable options for low level systems programming, in a way that
> +  was not practical with other common languages such as Python and Java.
> +  There is thus a desire to make use of either Rust or Go, or more likely
> +  a combination of both, to incrementally replace existing use of C, and
> +  also for greenfield development.

I would drop the "or more likely a combination of both" because I'm
hoping that we'll be able to standardize on a single one: using a
mix of both would once again increase the burden placed upon
contributors, which is the very problem we're trying to address.

Perhaps we're going to end up with both after all, but I would not
include language encouraging this.

[...]
> +  C
> +  The core libvirt library, daemons, and helper tools are all written
> +in the C language.

This reads to me as to say that all of the above are implemented
using C only, which contradicts the points below. I would also single
out the public API as the one part that will need to remain in C. So
perhaps something like

  The core library, daemons and helper tools will continue to be
  written mostly in C for the foreseeable future. Even as other
  programming languages are introduced, libvirt will continue to
  offer a stable C API/ABI.

> +  Rust
> +  Parts of the core libvirt library / daemon are to be able to 
> leverage
> +Rust to replace C. (TBD vs Go)
> +  Go
> +  Parts of the core libvirt library / daemon are to be able to 
> leverage
> +Rust to replace C. (TBD vs Rust). Helper tools are able to be written
> +in Go.

There's no reason Rust could be used for the library and daemons but
not be suitable for helper tools. I'd just have a single section for
the two languages, something like

  Rust and Go

  These languages should be usable as an alternative to C in all the
  areas listed above: new code should favor them over C, and existing
  C code may be ported after carefully considering the pros and cons
  of doing so.

[...]
> +  RST

I would s/RST/reStructuredText/ at least in this first occurrence.

[...]
> +  Some notable points from the above. Whether the core library / daemons
> +  will use Rust or Go internally is still to be decided based on more
> +  detailed evaluation of their pros and cons, to identify the best fit,
> +  as the need to link & embed this functionality in other processes has

  strategy.html.in:114: parser error : xmlParseEntityRef: no name
as the need to link & embed this functionality in other processes has
^

> +  complex interactions both at a technical and non-technical level. For
> +  standalone helper tools, Go is a good fit for rapid development as
> +  there are no concerns around in

Re: [libvirt] [PATCH] docs: attempt to document the general libvirt dev strategy

[Pavel Hrdina]

On Fri, Sep 20, 2019 at 03:12:01PM +0100, Daniel P. Berrangé wrote:
> There are various ideas / plans floating around for future libvirt work,
> some of which is actively in progress. Historically we've never captured
> this kind of information anywhere, except in mailing list discussions.
> In particular guidelines in hacking.html.in don't appear until a policy
> is actively applied.
> 
> This patch attempts to fill the documentation gap, by creating a new
> "strategy" page which outlines the general vision for some notable
> future changes. The key thing to note is that none of the stuff on this
> page is guaranteed, plans may change as new information arises. IOW this
> is a "best guess" as to the desired future.
> 
> This doc has focused on three areas, related to the topic of language
> usage / consolidation
> 
>  - Use of non-C languages for the library, daemons or helper tools
>  - Replacement of autotools with meson
>  - Use of RST and Sphinx for documentation (website + man pages)
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/docs.html.in |   3 +
>  docs/strategy.html.in | 143 ++
>  2 files changed, 146 insertions(+)
>  create mode 100644 docs/strategy.html.in

[...]

> +
> +  The meson build system is written in Python, at time of writing,
> +  requiring version 3.4 or later. This directly informs the choice to use
> +  Python 3.4 as the language for all supporting build scripts, 
> re-inforcing
> +  the other benefits of Python, over Perl, Shell, M4, automake, etc.
> +

In addition to all comments, Meson since version 0.45.0 requires
Python 3.5 and we will most likely use at least 0.49.0.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 02/20] util: ignore EACCESS in virDirOpenIfExists

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Whether a directory exists or is not readable shouldn't make a big
diffence.

This removes errors when firmare or vhost-user config is looked up
from a user session libvirt if /etc/libvirt is not readable.

Signed-off-by: Marc-André Lureau 
---
  src/util/virfile.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)



In some cases this makes sense, in others it isn't clear. For example:

[:~] $ chmod 000 .config/libvirt/storage/
[:~] $ libvirtd
...
2019-09-20 19:45:13.691+: 327223: error : virDirOpenInternal:2846 : 
cannot open directory '/home/crobinso/.config/libvirt/storage': 
Permission denied
2019-09-20 19:45:13.691+: 327223: error : virStateInitialize:656 : 
Initialization of storage state driver failed: cannot open directory 
'/home/crobinso/.config/libvirt/storage': Permission denied
2019-09-20 19:45:13.691+: 327223: error : daemonRunStateInit:790 : 
Driver state initialization failed


After the patches, this case doesn't explicitly fail, but the driver 
won't report any existing storage pools so it causes silent issues. I 
think erroring incase permissions are wrong is better, because libvirt 
doesn't have any code to attempt to fix that situation, unlike for 
missing directory


Not sure if it's realistic for this case to happen but I noticed it 
through code inspection.


Since this only applies to your particular case in the code in one area, 
you can do (untested)


if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) {
/* descriptive comment */
if (virLastErrorIsSystemErrno(EACCES)) {
virResetLastError();
return 0;
}
return -1;
}



diff --git a/src/util/virfile.c b/src/util/virfile.c
index bb844c64e5..4d1fe50efc 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2855,7 +2855,8 @@ virFileRemove(const char *path,
  #endif /* WIN32 */
  
  static int

-virDirOpenInternal(DIR **dirp, const char *name, bool ignoreENOENT, bool quiet)
+virDirOpenInternal(DIR **dirp, const char *name,
+   bool ignoreENOENT, bool ignoreEACCESS, bool quiet)
  {
  *dirp = opendir(name); /* exempt from syntax-check */
  if (!*dirp) {
@@ -2864,6 +2865,8 @@ virDirOpenInternal(DIR **dirp, const char *name, bool 
ignoreENOENT, bool quiet)
  
  if (ignoreENOENT && errno == ENOENT)

  return 0;
+if (ignoreEACCESS && errno == EACCES)
+return 0;
  virReportSystemError(errno, _("cannot open directory '%s'"), name);
  return -1;
  }
@@ -2881,7 +2884,7 @@ virDirOpenInternal(DIR **dirp, const char *name, bool 
ignoreENOENT, bool quiet)
  int
  virDirOpen(DIR **dirp, const char *name)
  {
-return virDirOpenInternal(dirp, name, false, false);
+return virDirOpenInternal(dirp, name, false, false, false);
  }
  
  /**

@@ -2890,13 +2893,13 @@ virDirOpen(DIR **dirp, const char *name)
   * @name: path of the directory
   *
   * Returns 1 on success.
- * If opendir returns ENOENT, 0 is returned without reporting an error.
+ * If opendir returns ENOENT or EACCES, 0 is returned without reporting an 
error.
   * On other errors, -1 is returned and an error is reported.
   */
  int
  virDirOpenIfExists(DIR **dirp, const char *name)
  {
-return virDirOpenInternal(dirp, name, true, false);
+return virDirOpenInternal(dirp, name, true, true, false);
  }
  
  /**

@@ -2912,7 +2915,7 @@ virDirOpenIfExists(DIR **dirp, const char *name)
  int
  virDirOpenQuiet(DIR **dirp, const char *name)
  {
-return virDirOpenInternal(dirp, name, false, true);
+return virDirOpenInternal(dirp, name, false, false, true);
  }
  
  /**





- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 05/20] qemu-cgroup: allow accel rendernode access

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
Reviewed-by: Ján Tomko 
---
  src/qemu/qemu_cgroup.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index ecd96efb0a..eb6f993d8e 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -503,6 +503,25 @@ qemuSetupGraphicsCgroup(virDomainObjPtr vm,
  }
  
  
+static int

+qemuSetupVideoAccelCgroup(virDomainObjPtr vm,
+  virDomainVideoAccelDefPtr def)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int ret;
+
+if (!def->rendernode ||
+!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
+return 0;
+
+ret = virCgroupAllowDevicePath(priv->cgroup, def->rendernode,
+   VIR_CGROUP_DEVICE_RW, false);
+virDomainAuditCgroupPath(vm, priv->cgroup, "allow", def->rendernode,
+ "rw", ret);
+return ret;
+}
+
+
  static int
  qemuSetupBlkioCgroup(virDomainObjPtr vm)
  {
@@ -803,6 +822,11 @@ qemuSetupDevicesCgroup(virDomainObjPtr vm)
  goto cleanup;
  }
  
+for (i = 0; i < vm->def->nvideos; i++) {

+if (qemuSetupVideoAccelCgroup(vm, vm->def->videos[i]->accel) < 0)
+goto cleanup;
+}
+
  for (i = 0; i < vm->def->ninputs; i++) {
  if (qemuSetupInputCgroup(vm, vm->def->inputs[i]) < 0)
  goto cleanup;



->accel can be NULL here otherwise we crash like I mentioned yesterday. 
All the other functions here act on a device as a whole so I think this 
should be qemuSetupVideoCgroup while you are here


FWIW after fixing this the VM starts, but I see this in the logs with 
stock Fedora 31 qemu 4.1, not sure if it's indicative of a qemu error or 
something missing on libvirt side. The VM gets to the initrd load stage 
but then graphic output stops updating


2019-09-20T20:06:51.238950Z qemu-system-x86_64: Failed initializing 
vhost-user memory map, consider using -object memory-backend-file share=on
2019-09-20T20:06:51.238971Z qemu-system-x86_64: vhost_set_mem_table 
failed: Resource temporarily unavailable (11)

2019-09-20T20:06:51.238976Z qemu-system-x86_64: Error start vhost dev

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 08/20] qemu: validate virtio-gpu with vhost-user

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Check qemu capability, and accept 3d acceleration. 3d acceleration
support is checked when looking for a suitable vhost-user helper.

Signed-off-by: Marc-André Lureau 
---
  src/qemu/qemu_process.c  | 57 +++-
  tests/qemuxml2argvtest.c |  3 +--
  2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 955ba4de4c..463b783966 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5266,34 +5266,43 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm,
  for (i = 0; i < vm->def->nvideos; i++) {
  video = vm->def->videos[i];
  
-if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&

- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
- video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW))) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("this QEMU does not support '%s' video device"),
-   virDomainVideoTypeToString(video->type));
-return -1;
-}
-
-if (video->accel) {
-if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
-(video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_VIRGL))) {
+if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) {
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_GPU)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("this QEMU does not support 'vhost-user' video 
device"));
+return -1;
+}
+} else {
+if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+ video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW))) {
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("%s 3d acceleration is not supported"),
+   _("this QEMU does not support '%s' video 
device"),
 virDomainVideoTypeToString(video->type));
  return -1;
  }
+
+if (video->accel) {
+if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
+(video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_VIRGL))) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("%s 3d acceleration is not supported"),
+   virDomainVideoTypeToString(video->type));
+return -1;
+}
+}
  }
  }



The quantity of indented code says to me this function should be split 
up, but I think all this should be moved to Validate time anyways, so 
it's something for the future


Reviewed-by: Cole Robinson 

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 09/20] qemu: restrict 'virgl=' option to non-vhostuser video type

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

vhost-user device doesn't have a virgl option, it is passed to the
vhost-user-gpu helper process instead.

Signed-off-by: Marc-André Lureau 
---
  src/qemu/qemu_command.c| 9 ++---
  tests/qemuxml2argvdata/virtio-options.args | 3 +--
  2 files changed, 7 insertions(+), 5 deletions(-)



Reviewed-by: Cole Robinson 

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 10/20] qemu: add vhost-user helpers

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Add qemuVhostUserFetchConfigs() to discover vhost-user helpers.

qemuVhostUserFillDomainGPU() will find the first matching GPU helper
with the required capabilities and set the associated
vhost_user_binary.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Cole Robinson 

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 11/20] qemu: add qemuSecurityStartVhostUserGPU helper

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

See function documentation. Used in a following patch.

Signed-off-by: Marc-André Lureau 
---
  src/qemu/qemu_security.c | 40 
  src/qemu/qemu_security.h |  6 ++
  2 files changed, 46 insertions(+)



Reviewed-by: Cole Robinson 

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 12/20] conf: add privateData to virDomainVideoDef

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
  src/conf/domain_conf.c | 26 +-
  src/conf/domain_conf.h |  7 +--
  2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9e1c8babf8..e99e52dc5e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2820,13 +2820,19 @@ void virDomainShmemDefFree(virDomainShmemDefPtr def)
  
  
  virDomainVideoDefPtr

-virDomainVideoDefNew(void)
+virDomainVideoDefNew(virDomainXMLOptionPtr xmlopt)
  {
  virDomainVideoDefPtr def;
  
  if (VIR_ALLOC(def) < 0)

  return NULL;
  
+if (xmlopt && xmlopt->privateData.videoNew &&

+!(def->privateData = xmlopt->privateData.videoNew())) {
+VIR_FREE(def);
+return NULL;
+}
+
  def->heads = 1;
  return def;
  }
@@ -2858,6 +2864,7 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
  return;
  
  virDomainVideoDefClear(def);

+virObjectUnref(def->privateData);
  VIR_FREE(def);
  }
  
@@ -5681,7 +5688,8 @@ virDomainDefPostParseVideo(virDomainDefPtr def,
  
  static int

  virDomainDefPostParseCommon(virDomainDefPtr def,
-struct virDomainDefPostParseDeviceIteratorData 
*data)
+struct virDomainDefPostParseDeviceIteratorData 
*data,
+virDomainXMLOptionPtr xmlopt)
  {
  size_t i;
  
@@ -5716,7 +5724,7 @@ virDomainDefPostParseCommon(virDomainDefPtr def,

  if (virDomainDefPostParseTimer(def) < 0)
  return -1;
  
-if (virDomainDefAddImplicitDevices(def) < 0)

+if (virDomainDefAddImplicitDevices(def, xmlopt) < 0)
  return -1;
  
  if (virDomainDefPostParseVideo(def, data) < 0)

@@ -5842,7 +5850,7 @@ virDomainDefPostParse(virDomainDefPtr def,
  if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
  goto cleanup;
  
-if ((ret = virDomainDefPostParseCommon(def, &data)) < 0)

+if ((ret = virDomainDefPostParseCommon(def, &data, xmlopt)) < 0)
  goto cleanup;
  
  if (xmlopt->config.assignAddressesCallback) {

@@ -15450,7 +15458,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
  VIR_AUTOFREE(char *) vgamem = NULL;
  VIR_AUTOFREE(char *) primary = NULL;
  
-if (!(def = virDomainVideoDefNew()))

+if (!(def = virDomainVideoDefNew(xmlopt)))
  return NULL;
  
  ctxt->node = node;

@@ -23771,7 +23779,7 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
  }
  
  static int

-virDomainDefAddImplicitVideo(virDomainDefPtr def)
+virDomainDefAddImplicitVideo(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt)


newline inbetween the arguments here, and all other similar locations
in src/vz there's a usage of virDomainDefAddImplicitDevices which will 
need xmlopt passed in too. Otherwise:


Reviewed-by: Cole Robinson 

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 13/20] qemu: add qemuDomainVideoPrivate

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
  src/qemu/qemu_domain.c | 38 ++
  src/qemu/qemu_domain.h | 12 
  2 files changed, 50 insertions(+)


Reviewed-by: Cole Robinson 

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 14/20] qemu: add vhost-user-gpu helper unit

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Similar to the qemu_tpm.c, add a unit with a few functions to
start/stop and setup the cgroup of the external vhost-user-gpu
process. See function documentation.

The vhost-user connection fd is set on qemuDomainVideoPrivate struct.

Signed-off-by: Marc-André Lureau 
---
  src/qemu/Makefile.inc.am   |   2 +
  src/qemu/qemu_domain.c |   5 +-
  src/qemu/qemu_domain.h |   2 +-
  src/qemu/qemu_vhost_user_gpu.c | 276 +
  src/qemu/qemu_vhost_user_gpu.h |  49 ++
  5 files changed, 332 insertions(+), 2 deletions(-)
  create mode 100644 src/qemu/qemu_vhost_user_gpu.c
  create mode 100644 src/qemu/qemu_vhost_user_gpu.h

diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 449550e64b..8040bf9366 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -66,6 +66,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_tpm.h \
qemu/qemu_vhost_user.c \
qemu/qemu_vhost_user.h \
+   qemu/qemu_vhost_user_gpu.c \
+   qemu/qemu_vhost_user_gpu.h \
$(NULL)
  
  
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c

index 9437694940..ab923aa917 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1341,8 +1341,11 @@ qemuDomainVideoPrivateNew(void)
  
  
  static void

-qemuDomainVideoPrivateDispose(void *obj ATTRIBUTE_UNUSED)
+qemuDomainVideoPrivateDispose(void *obj)
  {
+qemuDomainVideoPrivatePtr priv = obj;
+
+VIR_FORCE_CLOSE(priv->vhost_user_fd);
  }
  
  
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h

index 89bd77b3c0..8afb4993d3 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -509,7 +509,7 @@ typedef qemuDomainVideoPrivate *qemuDomainVideoPrivatePtr;
  struct _qemuDomainVideoPrivate {
  virObject parent;
  
-bool tmp_to_remove;

+int vhost_user_fd;
  };
  
  
diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c

new file mode 100644
index 00..b259b58434
--- /dev/null
+++ b/src/qemu/qemu_vhost_user_gpu.c
@@ -0,0 +1,276 @@
+/*
+ * qemu_vhost_user_gpu.c: QEMU vhost-user GPU support
+ *
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "qemu_vhost_user_gpu.h"
+#include "qemu_vhost_user.h"
+#include "qemu_extdevice.h"
+
+#include "conf/domain_conf.h"
+#include "configmake.h"
+#include "vircommand.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virutil.h"
+#include "virfile.h"
+#include "virstring.h"
+#include "virtime.h"
+#include "virpidfile.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("qemu.vhost_user_gpu");
+
+
+static char *
+qemuVhostUserGPUCreatePidFilename(const char *stateDir,
+  const char *shortName,
+  const char *alias)
+{
+VIR_AUTOFREE(char *) devicename = NULL;
+
+if (virAsprintf(&devicename, "%s-%s-vhost-user-gpu", shortName, alias) < 0)
+return NULL;
+
+return virPidFileBuildPath(stateDir, devicename);
+}
+
+
+/*
+ * qemuVhostUserGPUGetPid:
+ * @binpath: path of executable associated with the pidfile
+ * @stateDir: the directory where vhost-user-gpu writes the pidfile into
+ * @shortName: short name of the domain
+ * @alias: video device alias
+ * @pid: pointer to pid
+ *
+ * Return -errno upon error, or zero on successful reading of the pidfile.
+ * If the PID was not still alive, zero will be returned, and @pid will be
+ * set to -1;
+ */
+static int
+qemuVhostUserGPUGetPid(const char *binPath,
+   const char *stateDir,
+   const char *shortName,
+   const char *alias,
+   pid_t *pid)
+{
+VIR_AUTOFREE(char *) pidfile = NULL;
+
+pidfile = qemuVhostUserGPUCreatePidFilename(stateDir, shortName, alias);
+if (!pidfile)
+return -ENOMEM;
+
+return virPidFileReadPathIfAlive(pidfile, pid, binPath);
+}
+
+
+int qemuExtVhostUserGPUPrepareDomain(virQEMUDriverPtr driver,
+ virDomainVideoDefPtr video)
+{
+return qemuVhostUserFillDomainGPU(driver, video);
+}
+
+
+/*
+ * qemuExtVhostUserGPUStart:
+ * @driver: QEMU driver
+ * @vm: the VM domain
+ * @video: the video device
+ *
+ * Start the

Re: [libvirt] [PATCH v4 20/20] tests: add vhost-user-gpu xml2argv tests

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lur...@redhat.com wrote:

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
  ...host-user-gpu-secondary.x86_64-latest.args | 43 +
  .../vhost-user-gpu-secondary.xml  | 46 +++
  .../vhost-user-vga.x86_64-latest.args | 40 
  tests/qemuxml2argvdata/vhost-user-vga.xml | 42 +
  tests/qemuxml2argvtest.c  |  3 ++
  5 files changed, 174 insertions(+)
  create mode 100644 
tests/qemuxml2argvdata/vhost-user-gpu-secondary.x86_64-latest.args
  create mode 100644 tests/qemuxml2argvdata/vhost-user-gpu-secondary.xml
  create mode 100644 tests/qemuxml2argvdata/vhost-user-vga.x86_64-latest.args
  create mode 100644 tests/qemuxml2argvdata/vhost-user-vga.xml


For 15-20:

Reviewed-by: Cole Robinson 

git log says you have commit access, so I think this series is fine to 
push with:


- patch #2 split out and changed+submitted separately
- Jano's comments addressed
- the cgroup crasher fixed. just ignore my comment about the function rename
- Send a follow up patch to fix the error reporting in qemu_vhost_user_gpu.c

As for testing the rendernode allocation, virHostGetDRMRenderNode is 
mocked in tests/qemuxml2argvmock.c, and the test case 
./tests/qemuxml2argvdata/graphics-spice-gl-auto-rendernode.xml triggers 
that mocking, so maybe something similar can be done for vhost-user


- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list