Re: [PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Laine Stump]

On 2/24/20 1:58 PM, Gaurav Agrawal wrote:

From: GAURAV AGRAWAL 

Signed-off-by: Gaurav Agrawal 
---
  src/network/bridge_driver_linux.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)



Yay! It applied properly, so you have all the pieces of patch submission 
properly in line! :-)



(Welcome to the neighborhood, BTW.)




diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c
index 7bbde5c6a9..fde33b5d38 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -22,6 +22,7 @@
  #include 
  
  #include "viralloc.h"

+#include "virerror.h"
  #include "virfile.h"
  #include "viriptables.h"
  #include "virstring.h"
@@ -53,7 +54,7 @@ static void networkSetupPrivateChains(void)
  if (rc < 0) {
  VIR_DEBUG("Failed to create global IPv4 chains: %s",
virGetLastErrorMessage());
-errInitV4 = virSaveLastError();
+virErrorPreserveLast(&errInitV4);
  virResetLastError();
  } else {
  virFreeError(errInitV4);
@@ -70,7 +71,7 @@ static void networkSetupPrivateChains(void)
  if (rc < 0) {
  VIR_DEBUG("Failed to create global IPv6 chains: %s",
virGetLastErrorMessage());
-errInitV6 = virSaveLastError();
+virErrorPreserveLast(&errInitV6);
  virResetLastError();
  } else {
  virFreeError(errInitV6);



For anyone who didn't notice, this patch is for one of the "bite sized 
tasks" here:


https://wiki.libvirt.org/page/BiteSizedTasks#More_conversions_to_virErrorPreserveLast.2FvirErrorRestore


This is a very strange case of virSaveLastError() / virSetError() to 
pick - usually they are in pairs within the same function, but in this 
case when the original error occurs during the driver init, it is 
squanched away in the static errInitV[46] with virSaveLastError(), and 
later, in a completely different context, whenever something tries to 
add a firewall rule of the given type, *then* it sets the error (with 
virSetError()) to what was earlier encountered during init.



Your patch has replaced the virSaveLastError() of the earlier part with 
virErrorPreserveLast(), but hasn't replaced the virSetError() of the 
later part (which is down in networkAddFirewallRules()) with 
virErrorRestore().



I can make those two minor changes and push if you like, or you're free 
to send again with --subject-prefix="PATCHv2". (Or, maybe an even better 
idea - you could expand the patch to replace all the other uses of 
virSaveLastError()/virSetError()). The choice is yours :-)



(Oh, and BTW, no need to Cc: crobinso - he'll see it anyway)

[libvirt PATCH] node_device: hal: include virutil.h

[Ján Tomko]

Signed-off-by: Ján Tomko 
Fixes: b11e8cccdd5163727fd4cecda0076ac2b63fe32d
---
Pushed as a build fix.
Also re-ran configure on my FreeBSD guest to pick up HAL.
 src/node_device/node_device_hal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/node_device/node_device_hal.c 
b/src/node_device/node_device_hal.c
index c3ca310bb7..a48b4ffcd1 100644
--- a/src/node_device/node_device_hal.c
+++ b/src/node_device/node_device_hal.c
@@ -38,6 +38,7 @@
 #include "virlog.h"
 #include "virdbus.h"
 #include "virstring.h"
+#include "virutil.h"
 
 #include "configmake.h"
 
-- 
2.24.1

Re: [libvirt PATCH 0/7] tests: libxl: clean up test mocking

[Jim Fehlig]

On 2/22/20 7:25 AM, Ján Tomko wrote:

Refactor libxlDriverConfigNew to remove the need
for mocking virFilePath and add libxlDomainGetEmulatorType
to the mock to remove the need to invoke a binary
for nearly every domain we parse

Ján Tomko (7):
   testutilsxen: error out on initialization failure
   libxl: conf: move default keepalive settings to libxlDriverConfigNew
   libxl: StateInitialize: use g_autofree
   libxl: split out DriverConfigInit out of DriverConfigNew
   libxl: do not mock virFileMakePath
   tests: link the libxl tests with libxltestdriver.la
   tests: libxl: do not run the emulator

  src/libxl/libxl_capabilities.h |  3 +-
  src/libxl/libxl_conf.c | 85 ++
  src/libxl/libxl_conf.h |  2 +
  src/libxl/libxl_driver.c   |  7 +--
  tests/Makefile.am  |  9 ++--
  tests/libxlmock.c  | 18 +++
  tests/testutilsxen.c   |  9 +++-
  7 files changed, 75 insertions(+), 58 deletions(-)



For the series

Reviewed-by: Jim Fehlig 

Regards,
Jim

Re: [libvirt PATCH 7/7] tests: libxl: do not run the emulator

[Jim Fehlig]

On 2/22/20 7:25 AM, Ján Tomko wrote:

Ever since commit c5a00350 the libxl parser invokes the emulator
to probe which device model to use.

Commit b90c4b5 introduced a workaround that used a stable path
which was very likely to result in the answer matching the default.
However the test is still affected by the host state and the binary
gets invoked if present.

Mock the libxlDomainGetEmulatorType function to stop wasting CPU
cycles every time a 'make check' is run on a system with xen installed.

For example xlconfigtest gets faster by 90 %

Signed-off-by: Ján Tomko 
Fixes: b90c4b5f505698d600303c5b4f03f5d229b329dd
---
  src/libxl/libxl_capabilities.h | 3 ++-
  tests/libxlmock.c  | 7 +++
  2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_capabilities.h b/src/libxl/libxl_capabilities.h
index 9a7c8bf636..9efb836429 100644
--- a/src/libxl/libxl_capabilities.h
+++ b/src/libxl/libxl_capabilities.h
@@ -50,4 +50,5 @@ libxlMakeDomainCapabilities(virDomainCapsPtr domCaps,
  size_t nfirmwares);
  
  int

-libxlDomainGetEmulatorType(const virDomainDef *def);
+libxlDomainGetEmulatorType(const virDomainDef *def)
+G_GNUC_NO_INLINE;


Ah, I think the lack of this change caused me grief in the past while fiddling 
with the mocking.


Regards,
Jim


diff --git a/tests/libxlmock.c b/tests/libxlmock.c
index 60e6b78129..a36ca135f6 100644
--- a/tests/libxlmock.c
+++ b/tests/libxlmock.c
@@ -30,6 +30,7 @@
  
  # include "virfile.h"

  # include "virsocket.h"
+# include "libxl/libxl_capabilities.h"
  
  VIR_MOCK_IMPL_RET_VOID(xs_daemon_open,

 struct xs_handle *)
@@ -123,4 +124,10 @@ VIR_MOCK_IMPL_RET_ARGS(stat, int,
  return real_stat(path, sb);
  }
  
+int

+libxlDomainGetEmulatorType(const virDomainDef *def G_GNUC_UNUSED)
+{
+return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+}
+
  #endif /* WITH_LIBXL && WITH_YAJL */

[PATCH] virt-aa-helper: Fix build by including virutil.h

[Jim Fehlig]

Commit fb01e1a44d missed including virutil.h, causing the following
compilation error

../../src/security/virt-aa-helper.c:1055:43: error: implicit declaration of
function 'virHostGetDRMRenderNode' [-Werror=implicit-function-declaration]
1055 | char *defaultRenderNode = virHostGetDRMRenderNode();

Signed-off-by: Jim Fehlig 
---

Pushing under the build-breaker rule.

 src/security/virt-aa-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 6f36652c7c..b6f58efdea 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -41,6 +41,7 @@
 #include "virxml.h"
 #include "viruuid.h"
 #include "virusb.h"
+#include "virutil.h"
 #include "virpci.h"
 #include "virfile.h"
 #include "configmake.h"
-- 
2.25.0

Re: [PATCH] build: stop running aclocal manually

[Ján Tomko]

On Mon, Feb 24, 2020 at 05:50:17PM +, Daniel P. Berrangé wrote:

The autoreconf script will already run aclocal for us,
so there's no need todo that ahead of time.


s/todo/to do/



Signed-off-by: Daniel P. Berrangé 
---
autogen.sh | 1 -
1 file changed, 1 deletion(-)



Neat, that's 3.5 s for free.

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH] bhyve: utils: use relative path for virclosecallbacks.h

[Ján Tomko]

When moving virclosecallbacks to src/hypervisor, I did not
adjust all the possible includes in Makefiles.

Use a path relative to src to fix the build.

Signed-off-by: Ján Tomko 
Fixes: 25c29ac2f5842a7d48d9f9619317f68acf5d9995
---
Pushed as a build fix.

 src/bhyve/bhyve_utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
index 8dda6062b5..f3e80b6121 100644
--- a/src/bhyve/bhyve_utils.h
+++ b/src/bhyve/bhyve_utils.h
@@ -26,7 +26,7 @@
 #include "configmake.h"
 #include "virdomainobjlist.h"
 #include "virthread.h"
-#include "virclosecallbacks.h"
+#include "hypervisor/virclosecallbacks.h"
 #include "virportallocator.h"
 
 #define BHYVE_AUTOSTART_DIRSYSCONFDIR "/libvirt/bhyve/autostart"
-- 
2.24.1

Re: [libvirt PATCH 0/8] Reduce usage of virutil.h (include epistles)

[Daniel Henrique Barboza]

On 2/23/20 8:25 PM, Ján Tomko wrote:

With the introduction of virenum.h and GLib,
the need for including this file diminishes.

Remove its transitive inclusion from almost
anywhere to discourage its use as a generic
helper dump. The remaining functions should
be repatriated into other files if possible.

Ján Tomko (8):
   Remove virutil.h where possible
   tests: include unistd.h instead of virutil.h
   util: vircgroup: include unistd.h rather than virutil.h
   util: virportallocator: add includes
   tools: virt-host-validate: move virutil.h include
   Include unistd.h where used
   virsh: include virutil.h where used
   Remove virutil.h from all header files



All patches:


Reviewed-by: Daniel Henrique Barboza 




  src/access/viraccessperm.h| 1 -
  src/bhyve/bhyve_domain.c  | 1 +
  src/bhyve/bhyve_driver.c  | 1 +
  src/conf/capabilities.c   | 1 +
  src/conf/cpu_conf.h   | 1 -
  src/conf/device_conf.h| 1 -
  src/conf/domain_conf.c| 1 +
  src/conf/interface_conf.h | 1 -
  src/conf/node_device_conf.h   | 1 -
  src/conf/node_device_util.c   | 1 +
  src/conf/numa_conf.h  | 1 -
  src/conf/secret_conf.h| 1 -
  src/conf/storage_conf.c   | 1 +
  src/conf/virnetworkportdef.c  | 1 +
  src/conf/virnwfilterbindingobj.c  | 1 +
  src/esx/esx_vi.c  | 1 +
  src/hypervisor/domain_cgroup.c| 2 ++
  src/interface/interface_backend_netcf.c   | 1 +
  src/interface/interface_backend_udev.c| 1 +
  src/libvirt-domain.c  | 1 +
  src/libxl/libxl_driver.c  | 1 +
  src/libxl/libxl_migration.c   | 1 +
  src/locking/lock_driver_lockd.c   | 1 +
  src/locking/lock_driver_sanlock.c | 1 +
  src/logging/log_handler.c | 1 +
  src/lxc/lxc_cgroup.c  | 1 +
  src/lxc/lxc_conf.c| 2 ++
  src/lxc/lxc_container.c   | 1 +
  src/lxc/lxc_controller.c  | 1 +
  src/lxc/lxc_domain.c  | 1 -
  src/lxc/lxc_driver.c  | 1 +
  src/lxc/lxc_fuse.c| 2 ++
  src/lxc/lxc_native.c  | 1 +
  src/lxc/lxc_process.c | 1 +
  src/network/bridge_driver.c   | 1 +
  src/network/leaseshelper.c| 1 +
  src/node_device/node_device_driver.c  | 1 +
  src/node_device/node_device_udev.c| 1 +
  src/nwfilter/nwfilter_ebiptables_driver.c | 1 +
  src/openvz/openvz_conf.c  | 1 +
  src/openvz/openvz_driver.c| 1 +
  src/openvz/openvz_util.c  | 2 ++
  src/qemu/qemu_agent.c | 1 +
  src/qemu/qemu_alias.c | 1 +
  src/qemu/qemu_capabilities.c  | 1 +
  src/qemu/qemu_cgroup.c| 1 +
  src/qemu/qemu_command.c   | 1 +
  src/qemu/qemu_conf.c  | 1 +
  src/qemu/qemu_domain.c| 1 +
  src/qemu/qemu_driver.c| 1 +
  src/qemu/qemu_hostdev.c   | 1 +
  src/qemu/qemu_interop_config.c| 1 +
  src/qemu/qemu_migration.c | 1 +
  src/qemu/qemu_monitor.c   | 1 +
  src/qemu/qemu_process.c   | 1 +
  src/qemu/qemu_shim.c  | 1 +
  src/qemu/qemu_vhost_user.c| 1 +
  src/qemu/qemu_vhost_user_gpu.c| 1 -
  src/remote/remote_driver.c| 1 +
  src/rpc/virnetlibsshsession.c | 1 -
  src/rpc/virnetsshsession.c| 1 -
  src/secret/secret_driver.c| 1 +
  src/storage/storage_backend_disk.c| 1 +
  src/storage/storage_backend_iscsi.c   | 1 +
  src/storage/storage_backend_logical.c | 1 +
  src/storage/storage_backend_mpath.c   | 1 +
  src/storage/storage_driver.c  | 1 +
  src/storage/storage_file_fs.c | 1 +
  src/storage/storage_util.c| 1 +
  src/test/test_driver.c| 1 +
  src/util/iohelper.c   | 1 -
  src/util/vircgroup.h  | 1 -
  src/util/vircgroupv1.c| 1 +
  src/util/vircgroupv2.c| 1 +
  src/util/virconf.h| 1 -
  src/util/virerror.c   | 1 -
  src/util/virfirewall.c| 1 -
  src/util/virgic.c | 1 -
  src/util/virgic.h | 1 -
  src/util/virkeycode.h | 1 -
  src/util/virmdev.h| 1 -
  src/util/virmodule.c  | 1 +
  src/util/virnetdevbandwidth.c | 1 -
  src/util/virnetdevbridge.c| 1 -
  src/util/virnetdevip.c| 1 -
  src/util/v

[PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Gaurav Agrawal]

From: GAURAV AGRAWAL 

Signed-off-by: Gaurav Agrawal 
---
 src/network/bridge_driver_linux.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/network/bridge_driver_linux.c 
b/src/network/bridge_driver_linux.c
index 7bbde5c6a9..fde33b5d38 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -22,6 +22,7 @@
 #include 
 
 #include "viralloc.h"
+#include "virerror.h"
 #include "virfile.h"
 #include "viriptables.h"
 #include "virstring.h"
@@ -53,7 +54,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv4 chains: %s",
   virGetLastErrorMessage());
-errInitV4 = virSaveLastError();
+virErrorPreserveLast(&errInitV4);
 virResetLastError();
 } else {
 virFreeError(errInitV4);
@@ -70,7 +71,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv6 chains: %s",
   virGetLastErrorMessage());
-errInitV6 = virSaveLastError();
+virErrorPreserveLast(&errInitV6);
 virResetLastError();
 } else {
 virFreeError(errInitV6);
-- 
2.24.1

Re: [PATCH v1 10/12] add hostdev handling for bhyve

[Ryan Moeller]

> Can you split this patch up into a few pieces.
>
> We generally want the changes to the XML parser/formatter to
> be separate from any driver code, as the first part.
>
> Then I'd suggest a separate patch for the PCI and the SCSI
> hostdev support in bhyve.

> For the conf stuff we'll need a docs update in docs/formatdomain.html.in
> at the same time as this parser additions.

> If you can provide a little detail in the commit message on why the current
> SCSI hostdev stuff doesn't work for FreeBSD that'd be useful too.

Can do.

> I didn't see anything in the code is keeping track of in-use PCI devices.
> Does something else in FreeBSD guarantee that you won't have bad stuff
> happening if the PCI device is attempted to be assigned to 2 guests ?
>
> Also is it required to manually detach the host OS driver first, or
> is that automatic ?  This ties into which 'managed=no|yes' attribute
> choices you should permit for the hostdev.

There is not automatic management for passing through devices. It is
configured by a list of PCI devices that are to be reserved for
passthrough at boot. The vmm module will attach the listed devices to
the ppt driver before something else can try to take them.

If the same device is assigned to multiple VMs then the bhyve command
will error with an appropriate message when trying to start VMs after
the first one takes the device. This seems fine to me. I have several
VMs configured to use the same devices but only can run one at a time.
This is for testing drivers in different stable branches, on different
operating systems, etc.

I could imagine some future extension of this work to enable libvirt
to automatically configure devices for passthrough at next boot
perhaps, but that is more of a convenience than a necessity. Right now
passthrough can only be configured by manually putting the bhyve
command line args in the config, so this change is already a definite
improvement in terms of leveraging libvirt's facilities.

-- 
Ryan Moeller
iXsystems, Inc.
OS Developer
Email: r...@ixsystems.com

[libvirt PATCH] docs: add page describing the libvirt daemons

[Daniel P . Berrangé]

Now that we have more than just the libvirtd daemon, we should be
explaining to users what they are all for & important aspects of their
configuration.

Signed-off-by: Daniel P. Berrangé 
---
 docs/daemons.rst  | 682 ++
 docs/docs.html.in |   3 +
 2 files changed, 685 insertions(+)
 create mode 100644 docs/daemons.rst

diff --git a/docs/daemons.rst b/docs/daemons.rst
new file mode 100644
index 00..a74b228025
--- /dev/null
+++ b/docs/daemons.rst
@@ -0,0 +1,682 @@
+===
+Libvirt Daemons
+===
+
+.. contents::
+
+A libvirt deployment for accessing one of the stateful drivers will require
+one or more daemons to be deployed on the virtualization host. There are a
+number of ways the daemons can be configured which will be outlined in this
+page.
+
+Architectural options
+=
+
+Monolithic vs modular daemons
+-
+
+Traditionally libvirt provided a single monolithic daemon called ``libvirtd``
+which exposed support for all the stateful drivers, both primary hypervisor
+drivers and secondary supporting drivers. It also enables secure remote access
+from clients running off host.
+
+Work is underway for the monolithic daemon to be replaced by a new set of
+modular daemons ``virt${DRIVER}d``, each one servicing a single stateful
+driver. A further ``virtproxyd`` daemon will provide secure remote access, as
+well as backcompatibility for clients using the UNIX socket path of the
+monolithic daemon.
+
+The change to modular daemons should not affect API functionality used by
+management applications. It will, however, have an impact on host provisioning
+tools since there are new systemd services and configuration files to be
+managed.
+
+Currently both monolithic and modular daemons are built by default, but the RPC
+client still prefers connecting to the monolithic daemon. It is intended to
+switch the RPC client to prefer the modular daemons in the near future. At
+least 1 year after this switch (but not more than 2 years), the monolithic
+daemon will be deleted entirely.
+
+Operating modes
+---
+
+The libvirt daemons, whether monolithic or modular, can often operate in two
+modes
+
+* *System mode* - the daemon is running as the root user account, enabling
+  access to its full range of functionality. A read-write connection to
+  daemons in system mode **typically implies privileges equivalent to having
+  a root shell**. Suitable `authentication mechanisms `__ **must
+  be enabled** to secure it against untrustworthy clients/users.
+
+* *Session mode* - the daemon is running as any non-root user account,
+  providing access to a more restricted range of functionality. Only client
+  apps/users running under **the same UID are permitted to connect**, thus a
+  connection does not imply any elevation of privileges.
+
+  Not all drivers support session mode and as such the corresponding
+  modular daemon may not support running in this mode
+
+
+Monolithic driver daemon
+
+
+The monolithic daemon is known as ``libvirtd`` and has historically been the
+default in libvirt. It is configured via the file 
``/etc/libvirt/libvirtd.conf``
+
+
+Monolithic sockets
+--
+
+When running in system mode, ``libvirtd`` exposes three UNIX domain sockets, 
and
+optionally, one or two TCP sockets
+
+* ``/var/run/libvirt/libvirt-sock`` - the primary socket for accessing libvirt
+  APIs, with full read-write privileges. A connection to this socket gives the
+  client privileges that are equivalent to having a root shell. This is the
+  socket that most management applications connect to by default.
+
+* ``/var/run/libvirt/libvirt-sock-ro`` - the secondary socket for accessing
+  libvirt APIs, with limited read-only privileges. A connection to this socket
+  gives the ability to query the existance of objects and monitor some aspects
+  of their operation. This is the socket that most management applications
+  connect to when requesting read only mode. Typically this is what a
+  monitoring app would use.
+
+* ``/var/run/libvirt/libvirt-admin-sock`` - the administrative socket for
+  controlling operation of the daemon itself (as opposed to drivers it is
+  running). This can be used to dynamically reconfigure some aspects of the
+  daemon and monitor/control connected clients.
+
+* ``TCP 16509`` - the non-TLS socket for remotely accessing the libvirt APIs,
+  with full read-write privileges. A connection to this socket gives the
+  client privileges that are equivalent to having a root shell. Since it does
+  not use TLS, an `authentication mechanism `__ that provides
+  encryption must be used. Only the GSSAPI/Kerberos mechanism is capable of
+  satisfying this requirement. In general applications should not use this
+  socket except for debugging in a development/test environment.
+
+* ``TCP 16514`` - the TLS socket for remotely accessing the libvirt APIs,
+  with

Re: [PATCH v1 00/12] Bhyve driver improvements

[Ryan Moeller]

Awesome, thank you!

I'll try to address the remaining feedback when I get a chance.
This work isn't exactly my primary job function so it's been more of a
nights and weekends labor of love kind of thing.

Thanks again for taking the time to review and push these patches!

On Mon, Feb 24, 2020 at 1:07 PM Daniel P. Berrangé  wrote:
>
> On Mon, Feb 24, 2020 at 01:46:12AM -0500, Ryan Moeller wrote:
> > Rebased and updated from previous patch set to address feedback:
> >
> >  * Tried to match local convention for subjects where obvious
> >  * Split patch 01 into two patches, with updated messages
> >  * Use g_autofree to fix use after free in conf/virnetworkobj
> >  * Add missing newline in one of the tests args files
> >  * Fix failing schema tests after schema change
> >
> > gmake check now reports no failing tests on FreeBSD for each patch.
> >
> > Ryan Moeller (12):
> >   bhyve: process: remove unneeded header
> >   conf: fix use after free
> >   bhyve: process: don't bother seeking to end of log
> >   bhyve: monitor: Make bhyveMonitor a virClass
> >   bhyve: monitor: refactor register/unregister
> >   bhyve: add hooks
> >   bhyve: add reboot support
> >   bhyve: command: refactor virBhyveProcessBuildBhyveCmd
> >   bhyve: parse_command: slot,bus,func -> bus,slot,func
>
> Thanks for the contribution, I've pushed these patches now
> since they were independant of the remaining three.
>
>
> >   add hostdev handling for bhyve
> >   bhyve: command: enable booting from hostdevs
> >   Allow PCI functions up to 255 for PCI ARI
>
> 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 :|
>


-- 
Ryan Moeller
iXsystems, Inc.
OS Developer
Email: r...@ixsystems.com

Re: [PATCH v1 00/12] Bhyve driver improvements

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:12AM -0500, Ryan Moeller wrote:
> Rebased and updated from previous patch set to address feedback:
> 
>  * Tried to match local convention for subjects where obvious
>  * Split patch 01 into two patches, with updated messages
>  * Use g_autofree to fix use after free in conf/virnetworkobj
>  * Add missing newline in one of the tests args files
>  * Fix failing schema tests after schema change
> 
> gmake check now reports no failing tests on FreeBSD for each patch.
> 
> Ryan Moeller (12):
>   bhyve: process: remove unneeded header
>   conf: fix use after free
>   bhyve: process: don't bother seeking to end of log
>   bhyve: monitor: Make bhyveMonitor a virClass
>   bhyve: monitor: refactor register/unregister
>   bhyve: add hooks
>   bhyve: add reboot support
>   bhyve: command: refactor virBhyveProcessBuildBhyveCmd
>   bhyve: parse_command: slot,bus,func -> bus,slot,func

Thanks for the contribution, I've pushed these patches now
since they were independant of the remaining three.


>   add hostdev handling for bhyve
>   bhyve: command: enable booting from hostdevs
>   Allow PCI functions up to 255 for PCI ARI

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

network: bridge_driver: Use new helpers for storing libvirt errors

[Gaurav Agrawal]

>From c2028d3b27e20eb0d15a553139d2c987325d977e Mon Sep 17 00:00:00 2001
From: Gaurav Agrawal 
Date: Mon, 24 Feb 2020 22:49:21 +0530
Subject: [PATCH] network: bridge_driver: Use new helpers for storing libvirt
 errors

Signed-off-by: Gaurav Agrawal 
---
 src/network/bridge_driver_linux.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/network/bridge_driver_linux.c
b/src/network/bridge_driver_linux.c
index 7bbde5c6a9..fde33b5d38 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -22,6 +22,7 @@
 #include 

 #include "viralloc.h"
+#include "virerror.h"
 #include "virfile.h"
 #include "viriptables.h"
 #include "virstring.h"
@@ -53,7 +54,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv4 chains: %s",
   virGetLastErrorMessage());
-errInitV4 = virSaveLastError();
+virErrorPreserveLast(&errInitV4);
 virResetLastError();
 } else {
 virFreeError(errInitV4);
@@ -70,7 +71,7 @@ static void networkSetupPrivateChains(void)
 if (rc < 0) {
 VIR_DEBUG("Failed to create global IPv6 chains: %s",
   virGetLastErrorMessage());
-errInitV6 = virSaveLastError();
+virErrorPreserveLast(&errInitV6);
 virResetLastError();
 } else {
 virFreeError(errInitV6);
-- 
2.24.1

Re: [PATCH v1 11/12] bhyve: command: enable booting from hostdevs

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:23AM -0500, Ryan Moeller wrote:
> It is possible to boot from virtio-scsi with UEFI-devel, for example.
> 
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_command.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 9f4030f6a3..048c26733a 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -993,11 +993,8 @@ virBhyveGetBootDisk(virDomainDefPtr def)
>  virDomainDiskDefPtr match = NULL;
>  int boot_dev = -1;
>  
> -if (def->ndisks < 1) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -   _("Domain should have at least one disk defined"));
> +if (def->ndisks == 0)
>  return NULL;
> -}

This doesn't look like correct logic to me. Most code paths
which return NULL will report an error, so having one code
path which doesn't is pretty unusual practice.

Also the caller of this method  still assumes a NULL return
value indicates an error AFAICS.

>  
>  if (def->os.nBootDevs > 1) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -- 
> 2.24.1
> 
> 

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

Re: [PATCH v1 10/12] add hostdev handling for bhyve

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:22AM -0500, Ryan Moeller wrote:
> Handle PCI passthrough and virtio-scsi using hostdev devices.
> 
> Example PCI passthrough:
> domain xml snippet
> ```
> 
>   
> 
> 
>   
> 
>   
> 
>  function='0x00'/>
>   
> 
> ```
> loader.conf snippet
> ```
> vmm_load="YES"
> pptdevs="6/2/0"
> ```
> 
> Example SCSI passthrough:
> domain xml snippet
> ```
> 
>   
>   
> 
> ```
> ctl.conf snippet
> ```
> portal-group "pg0" {
> discovery-auth-group "no-authentication"
> listen "127.0.0.1"
> }
> 
> target iqn.2020-01.com.example:target0 {
> auth-group "no-authentication"
> portal-group "pg0"
> port ioctl/5/0
> 
> lun 0 { path "/dev/zvol/storage/lun0" }
> lun 1 { path "/dev/zvol/storage/lun1" }
> lun 2 { path "/dev/zvol/storage/lun2" }
> lun 3 { path "/dev/zvol/storage/lun3" }
> }
> ```

Can you split this patch up into a few pieces.

We generally want the changes to the XML parser/formatter to
be separate from any driver code, as the first part.

Then I'd suggest a separate patch for the PCI and the SCSI
hostdev support in bhyve.

> 
> Signed-off-by: Ryan Moeller 
> ---
>  docs/schemas/domaincommon.rng |  30 
>  src/bhyve/bhyve_capabilities.c|  14 ++
>  src/bhyve/bhyve_capabilities.h|   1 +
>  src/bhyve/bhyve_command.c | 121 
>  src/bhyve/bhyve_parse_command.c   |  90 
>  src/conf/domain_audit.c   |   5 +
>  src/conf/domain_conf.c| 131 ++
>  src/conf/domain_conf.h|  29 +++-

For the conf stuff we'll need a docs update in docs/formatdomain.html.in
at the same time as this parser additions.

If you can provide a little detail in the commit message on why the current
SCSI hostdev stuff doesn't work for FreeBSD that'd be useful too.

>  src/conf/virconftypes.h   |   3 +
>  src/qemu/qemu_command.c   |   2 +
>  src/qemu/qemu_domain.c|   5 +
>  src/qemu/qemu_hostdev.c   |   1 +
>  src/qemu/qemu_hotplug.c   |   2 +
>  src/qemu/qemu_migration.c |   1 +
>  src/security/security_apparmor.c  |   1 +
>  src/security/security_dac.c   |  28 
>  src/security/security_selinux.c   |   8 ++
>  .../bhyveargv2xml-passthru.args   |   8 ++
>  .../bhyveargv2xml-passthru.xml|  26 
>  .../bhyveargv2xml-virtio-scsi.args|   9 ++
>  .../bhyveargv2xml-virtio-scsi.xml |  20 +++
>  tests/bhyveargv2xmltest.c |   2 +
[>  .../bhyvexml2argv-passthru.args   |  11 ++
>  .../bhyvexml2argv-passthru.ldargs |   1 +
>  .../bhyvexml2argv-passthru.xml|  22 +++
>  .../bhyvexml2argv-virtio-scsi.args|   9 ++
>  .../bhyvexml2argv-virtio-scsi.ldargs  |   1 +
>  .../bhyvexml2argv-virtio-scsi.xml |  21 +++
>  tests/bhyvexml2argvtest.c |   4 +-
>  .../bhyvexml2xmlout-passthru.xml  |  29 
>  .../bhyvexml2xmlout-virtio-scsi.xml   |  23 +++
>  tests/bhyvexml2xmltest.c  |   2 +
>  32 files changed, 658 insertions(+), 2 deletions(-)
>  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-passthru.args
>  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-passthru.xml
>  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-virtio-scsi.args
>  create mode 100644 tests/bhyveargv2xmldata/bhyveargv2xml-virtio-scsi.xml
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-passthru.args
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-passthru.ldargs
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-passthru.xml
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-virtio-scsi.args
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-virtio-scsi.ldargs
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-virtio-scsi.xml
>  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-passthru.xml
>  create mode 100644 tests/bhyvexml2xmloutdata/bhyvexml2xmlout-virtio-scsi.xml

I didn't see anything in the code is keeping track of in-use PCI devices.
Does something else in FreeBSD guarantee that you won't have bad stuff
happening if the PCI device is attempted to be assigned to 2 guests ?

Also is it required to manually detach the host OS driver first, or
is that automatic ?  This ties into which 'managed=no|yes' attribute
choices you should permit for the hostdev.


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

Re: [libvirt PATCH 0/3] Remove usage of virHexToBin (glib chronicles)

[Laine Stump]

On 2/23/20 3:20 PM, Ján Tomko wrote:

Prefer g_ascii_xdigit_value

Ján Tomko (3):
   util: uuid: remove use of virHexToBin
   Remove all use of virHexToBin
   util: remove virHexToBin

  src/libvirt_private.syms |  1 -
  src/util/virbitmap.c |  3 +--
  src/util/virmacaddr.c|  5 ++---
  src/util/virutil.c   | 15 ---
  src/util/virutil.h   |  2 --
  src/util/viruuid.c   | 11 +--
  src/vmx/vmx.c|  4 ++--
  7 files changed, 10 insertions(+), 31 deletions(-)



Hmmpph. I meant to respond to 0/3 with my


Reviewed-by: Laine Stump 


not just to the first patch.

[PATCH] build: stop running aclocal manually

[Daniel P . Berrangé]

The autoreconf script will already run aclocal for us,
so there's no need todo that ahead of time.

Signed-off-by: Daniel P. Berrangé 
---
 autogen.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/autogen.sh b/autogen.sh
index 671dd63eb6..4e1bbceb0a 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -15,7 +15,6 @@ cd "$srcdir"
 
 git submodule update --init || exit 1
 
-aclocal --install || exit 1
 autoreconf --verbose --force --install || exit 1
 
 if test "x$1" = "x--system"; then
-- 
2.24.1

Re: [PATCH 3/3] qemu-img: Deprecate use of -b without -F

[Peter Krempa]

On Mon, Feb 24, 2020 at 10:08:31 -0600, Eric Blake wrote:
> On 2/24/20 5:38 AM, Peter Krempa wrote:
> > On Sat, Feb 22, 2020 at 05:23:41 -0600, Eric Blake wrote:
> > > Creating an image that requires format probing of the backing image is
> > > inherently unsafe (we've had several CVEs over the years based on
> > > probes leaking information to the guest on a subsequent boot).  If our
> > > probing algorithm ever changes, or if other tools like libvirt
> > > determine a different probe result than we do, then subsequent use of
> > > that backing file under a different format will present corrupted data
> > > to the guest.  Start a deprecation clock so that future qemu-img can
> > > refuse to create unsafe backing chains that would rely on probing.
> > > 
> > > However, there is one time where probing is safe: when we first create
> > > an image, no guest has yet used the new image, so as long as we record
> > > what we probed, all future uses of the image will see the same data -
> > 
> > I disagree. If you are creating an overlay on top of an existing image
> > it's not safe to probe the format any more generally. (obviously you'd
> > have to trust the image and express the trust somehow)
> > 
> > The image may have been used in a VM as raw and that means that the VM
> > might have recorded a valid qcow2 header into it. Creating the overlay
> > with probing would legitimize this.
> > 
> > Let's assume we have a malicious image written by the guest but we
> > simulate it by:
> > 
> 
> > b) Now with this patchset:
> > 
> > $ ./qemu-img create -f qcow2 -b /tmp/malicious /tmp/post-patch.qcow2
> > qemu-img: warning: Deprecated use of non-raw backing file without explicit 
> > backing format, using detected format of qcow2
> > Formatting '/tmp/post-patch.qcow2', fmt=qcow2 size=2560 
> > backing_file=/tmp/malicious backing_fmt=qcow2 cluster_size=65536 
> > lazy_refcounts=off refcount_bits=16
> 
> > 
> > You now get a warning, but "backing file format" is now recorded in the
> > overlay. Now this is WAY worse than it was before. The overlay now
> > legitimizes the format recorded by the malicious guest which circumvents
> > libvirt's protections. The warning is very easy to miss, and if you run
> > it in scripts you might never get to see it. We can't allow that.
> 
> Good point.  I'll respin this series where v2 never writes the implicit
> format except for a raw image (because probing raw is not only safe to
> record, but also prevents the guest from ever changing that probe, and the
> real risk we are interested in preventing is when a formerly raw image later
> probes as non-raw).
> 
> > 
> > 
> > > so the code now records the probe results as if the user had passed
> > > -F.  When this happens, it is unconditionally safe to record a probe
> > > of 'raw', but any other probe is still worth warning the user in case
> > 
> > While it's safe I don't think it should be encouraged. IMO -F should be
> > made mandatory with -b.
> 
> Making it mandatory will require the completion of the deprecation period.
> For 5.0 and 5.1, the best we can do is the warning, but for 5.2 (assuming v2
> of this series is acceptable), it WILL become a hard error.

Yes, that's fair. I just wanted to point out that the warning and later
error should be reported also if raw is probed. I'm okay with recording
raw into the overlay even now.

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

On Mon, Feb 24, 2020 at 14:24:15 +, Daniel Berrange wrote:
> On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
> > On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:

[...]

> > I'll reiterate the historical state of the problem because I think it's
> > important:
> > 
> > Pre-blockdev:
> >   - we internally assumed that if the image format of an backing image
> > was not present in the overlay it is 'raw'
> >   - this influenced security labelling but not actually how qemu viewed
> > or probed the file. If it was qcow2 probed as qcow2 qemu opened it
> > as qcow2 possibly even including the backing file if selinux or
> > other mechanism didn't prevent it.
> > 
> > post-blockdev:
> >   - the assumption of 'raw' would now be expressed into the qemu
> > configuration. This assumption turned into data corruption since we
> > no longer allowed qemu to probe the format and forced it as raw.
> >   - fix was to always require the format to be recorded in the overlay
> >   - this made users unhappy who neglected to record the format into the
> > overlay when creating it manually
> 
> So the key problem we have is that with -blockdev we are always explicitly
> telling QEMU what the backing file is for every image.
> 
> Can we fix this to have the exact same behaviour as before by *not* telling
> QEMU anything about the backing file when using -blockdev, if there is no
> well defined backing format present. ie, use -blockdev, but let QEMU probe
> just as it did in non-blockdev days.
> 
> Would there be any downsides to this that did not already exist in the
> non-blockdev days ?

We can, but the price is that:
1) we won't allow blockjobs and anything blockdev-related because node
name would be out of our control. This was possible in pre-blockdev era.
2) we will lose control of actually telling qemu to NOT open the backing
file in that case. Distros using only unix permission still have
arbitrary file access under permissions of the qemu process.
3) weird special-case code, because we need to keep some metadata about
the image to do security labelling

> I don't think we can solve the regressions in behaviour of backing files
> by doing probing of the backing files in libvirt, because that only works
> for the case where libvirt can actually open the file. ie a local file on
> disk. We don't have logic for opening backing files on RBD, GlusterFS,
> iSCSI, HTTP, SSH, etc, and nor do we want todo that.

Now we are back in the teritory where we actually do match what would
happen with previously. We don't specify these on the command line with
ehaviour matching what's described above, with the caveats as above.

I kept this behaviour because we couldn't do better. This is in place
even now if the last introspectable image has valid format specified.

We can reconsider how to approach this but ideally separately.

> So to me it looks like the only viable option is to not specify the
> backing file info to QEMU at all.
> 
> > Now this adds an interresting dimension to this problem. If libvirt
> > forces the users to specify the image format, and the users don't know
> > it they will probe. So we are basically making this a problem of
> > somebody else. [2] As you can see in that patch, it uses 'qemu-img'
> > anyways and also additionally actually allows the chain to continue
> > deeper! [3]
> 
> Yeah, this is a really bad situation given the difficulty in safely
> using qemu-img, without also breaking valid usage.
> 
> We don't want to push this off to apps
> 
> > This boils down to whether we want to accept some possibility of image
> > corruption in trade for avoiding regression of behaviour in the secure
> > cases as well as management apps and users not having to re-invent when
> > probing an image is actually safe.
> 
> I feel like the risk of image corruption is pretty minor. Our probing
> handles all normal cases the same way as QEMU and newly introduced
> image formats are rare.

Well, in this case I'm actually for re-considering the original patch
discussed here. It uses image-format-probing code from libvirt, to allow
the most common cases which were forbidden in a safe way. This means
that as long as we can probe the image and the probed image does not
have a backing file we allow the startup.

It restores previous behaviour for valid cases including blockjobs,
correctly revokes invalid cases (existing chain after image wihtout
format, images impossible to introspect), is limited to the backing
store walking code so can be contained and the price is doing the image
format detection using libvirt's code.

Re: [PATCH v1 09/12] bhyve: parse_command: slot, bus, func -> bus, slot, func

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:21AM -0500, Ryan Moeller wrote:
> This *is* a no-op, but there was a period of sickening dread while
> auditing to be sure that no actual confusion between bus and slot had
> occurred. I hope to avoid that by following the conventional order.
> 
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_parse_command.c | 34 -
>  1 file changed, 17 insertions(+), 17 deletions(-)

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

Re: [PATCH v1 08/12] bhyve: command: refactor virBhyveProcessBuildBhyveCmd

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:20AM -0500, Ryan Moeller wrote:
> Reduce the complexity by isolating loop bodies in separate functions.
> 
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_command.c | 115 ++
>  1 file changed, 67 insertions(+), 48 deletions(-)

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

Re: [PATCH v1 06/12] bhyve: add hooks

[Ryan Moeller]

Will do, thanks!

On Mon, Feb 24, 2020 at 12:01 PM Daniel P. Berrangé  wrote:
>
> On Mon, Feb 24, 2020 at 01:46:18AM -0500, Ryan Moeller wrote:
> > Signed-off-by: Ryan Moeller 
> > ---
> >  src/bhyve/bhyve_process.c | 33 +
> >  src/util/virhook.c| 15 +++
> >  src/util/virhook.h| 11 +++
> >  3 files changed, 59 insertions(+)
>
> You'll also want to update the docs in docs/hooks.html.in.
> I'll take this patch as is though, so can you send the docs
> as a followup patch afterwards.
>
> 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 :|
>


-- 
Ryan Moeller
iXsystems, Inc.
OS Developer
Email: r...@ixsystems.com

Re: [PATCH v1 07/12] bhyve: add reboot support

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:19AM -0500, Ryan Moeller wrote:
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_driver.c  | 30 ++
>  src/bhyve/bhyve_monitor.c | 19 +-
>  src/bhyve/bhyve_monitor.h |  2 ++
>  src/bhyve/bhyve_process.c | 52 ---
>  src/bhyve/bhyve_process.h |  3 +++
>  5 files changed, 85 insertions(+), 21 deletions(-)

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

Re: [PATCH v1 06/12] bhyve: add hooks

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:18AM -0500, Ryan Moeller wrote:
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_process.c | 33 +
>  src/util/virhook.c| 15 +++
>  src/util/virhook.h| 11 +++
>  3 files changed, 59 insertions(+)

You'll also want to update the docs in docs/hooks.html.in.
I'll take this patch as is though, so can you send the docs
as a followup patch afterwards.

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

Re: [PATCH v1 05/12] bhyve: monitor: refactor register/unregister

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:17AM -0500, Ryan Moeller wrote:
> Pull the code for registering and unregistering a bhyve monitor object
> into separate functions to improve code clarity.
> 
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_monitor.c | 50 ++-
>  1 file changed, 33 insertions(+), 17 deletions(-)

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

Re: [PATCH v1 04/12] bhyve: monitor: Make bhyveMonitor a virClass

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:16AM -0500, Ryan Moeller wrote:
> This makes lifecycle management a bit easier thanks to ref counting, and
> it is closer to what the qemu driver does.

FWIW, if you want todo any more conversions in the future, note that
we're intending to replace virObject with GObject.  eg as illustrated
in:

  commit 16121a88a7ef933220bcf9eff6575367278a06f7
  Author: Daniel P. Berrangé 
  Date:   Thu Sep 19 15:38:03 2019 +0100

util: convert virIdentity class to use GObject

> 
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_monitor.c | 116 +-
>  1 file changed, 76 insertions(+), 40 deletions(-)

None the less, I'm fine taking this conversion to virObject
as is, since it is better than what currently exists.

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

Re: [PATCH v1 02/12] conf: fix use after free

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:14AM -0500, Ryan Moeller wrote:
> Signed-off-by: Ryan Moeller 
> ---
>  src/conf/virnetworkobj.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)

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

Re: [PATCH v1 03/12] bhyve: process: don't bother seeking to end of log

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:15AM -0500, Ryan Moeller wrote:
> The file is opened O_APPEND.
> 
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_process.c | 5 -
>  1 file changed, 5 deletions(-)

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

Re: [PATCH v1 01/12] bhyve: process: remove unneeded header

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:46:13AM -0500, Ryan Moeller wrote:
> Signed-off-by: Ryan Moeller 
> ---
>  src/bhyve/bhyve_process.c | 1 -
>  1 file changed, 1 deletion(-)

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

Re: [PATCH 0/3] Tighten qemu-img rules on missing backing format

[Eric Blake]

On 2/24/20 5:01 AM, Peter Krempa wrote:

On Sat, Feb 22, 2020 at 05:23:38 -0600, Eric Blake wrote:

In the past, we have had CVEs caused by qemu probing one image type
when an image started out as another but the guest was able to modify
content.  The solution to those CVEs was to encode backing format
information into qcow2, to ensure that once we make a decision, we
don't have to probe any further.  However, we failed to enforce this
at the time.  And now that libvirt is switching to -blockdev, it has
come back to bite us: with -block, libvirt had no easy way (other than


s/-block/-drive/ ?


Whoops, yes.




json:{} pseudoprotocol) to force a backing file, but with -blockdev,


"json:{}" is basically -blockdev with extra steps. Old -drive usage
didn't have any way to do that apart from rewriting the image. Which is
basically the same since json:{} also needs to be recorded in the image
to take effect.


Discussed in my other reply (it looks like I'll need to distinguish 
between json: pointing to just a protocol as the backing layer, vs. the 
more typical json: pointing to a format as the backing layer).





libvirt HAS to use blockdev-open on the backing chain and supply a
backing format there, and thus has to probe images.  If libvirt ever
probes differently than qemu, we are back to the potential
guest-visible data corruption or potential host CVEs.


As I've elaborated in [1] I disagree with the host CVE part. The
insecure part is not probing the format itself, but probing format AND
using the backing file of the image if we probed format.

I agree that mis-probing format leads to data corruption though.


Your argument that there are other means at hand to prevent CVE when 
probing does occur is valid, however, my point is that CVEs due to 
probing were historically possible if the rest of the toolchain is not 
careful.  It is more of a burden-shifting problem: when qemu is not 
preventing probing, then other applications like libvirt have to take on 
extra measures to ensure libvirt does not have a CVE (the fact that we 
haven't had any in a few years is a good sign that we are at least aware 
of the problem and have worked hard to remain safe, even if it has 
required duplicated effort across multiple tools); whereas if qemu takes 
a hard-line stance on probing (which is the goal of the deprecation 
introduced in this series), now qemu has ensured safety whether or not 
the other layers also take measures to avoid any CVE.





It's time to deprecate images without backing formats.  This patch
series does two things: 1. record an implicit backing format where one
is learned (although sadly, not all qemu-img commands are able to
learn a format), 2. warn to the user any time a probe had ambiguous
results or a backing format is omitted from an image.  All previous
images without a backing format are still usable, but hopefully the
warnings (along with libvirt's complaints about images without a
backing format) help us pinpoint remaining applications that are


It is not a warning in libvirt though. We just refuse it now because we
don't do probing. Previously we allowed qemu to probe the format and the
only thing that prevented host CVEs was if the host used selinux or any
other security approach which would prevent opening the backing file.


creating images on their own without recording a backing format.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] qemu-img: Deprecate use of -b without -F

[Eric Blake]

On 2/24/20 5:38 AM, Peter Krempa wrote:

On Sat, Feb 22, 2020 at 05:23:41 -0600, Eric Blake wrote:

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot).  If our
probing algorithm ever changes, or if other tools like libvirt
determine a different probe result than we do, then subsequent use of
that backing file under a different format will present corrupted data
to the guest.  Start a deprecation clock so that future qemu-img can
refuse to create unsafe backing chains that would rely on probing.

However, there is one time where probing is safe: when we first create
an image, no guest has yet used the new image, so as long as we record
what we probed, all future uses of the image will see the same data -


I disagree. If you are creating an overlay on top of an existing image
it's not safe to probe the format any more generally. (obviously you'd
have to trust the image and express the trust somehow)

The image may have been used in a VM as raw and that means that the VM
might have recorded a valid qcow2 header into it. Creating the overlay
with probing would legitimize this.

Let's assume we have a malicious image written by the guest but we
simulate it by:




b) Now with this patchset:

$ ./qemu-img create -f qcow2 -b /tmp/malicious /tmp/post-patch.qcow2
qemu-img: warning: Deprecated use of non-raw backing file without explicit 
backing format, using detected format of qcow2
Formatting '/tmp/post-patch.qcow2', fmt=qcow2 size=2560 
backing_file=/tmp/malicious backing_fmt=qcow2 cluster_size=65536 
lazy_refcounts=off refcount_bits=16




You now get a warning, but "backing file format" is now recorded in the
overlay. Now this is WAY worse than it was before. The overlay now
legitimizes the format recorded by the malicious guest which circumvents
libvirt's protections. The warning is very easy to miss, and if you run
it in scripts you might never get to see it. We can't allow that.


Good point.  I'll respin this series where v2 never writes the implicit 
format except for a raw image (because probing raw is not only safe to 
record, but also prevents the guest from ever changing that probe, and 
the real risk we are interested in preventing is when a formerly raw 
image later probes as non-raw).






so the code now records the probe results as if the user had passed
-F.  When this happens, it is unconditionally safe to record a probe
of 'raw', but any other probe is still worth warning the user in case


While it's safe I don't think it should be encouraged. IMO -F should be
made mandatory with -b.


Making it mandatory will require the completion of the deprecation 
period.  For 5.0 and 5.1, the best we can do is the warning, but for 5.2 
(assuming v2 of this series is acceptable), it WILL become a hard error.





our probe differed from their expectations.  Similarly, if the backing
file name uses the json: psuedo-protocol, the backing name includes
the format.


Not necessarily. The backing store string can be e.g.:

$ ./qemu-img create -f qcow1 -b 
'json:{"driver":"file","filename":"/tmp/malicious"}' /tmp/json.qcow2
Formatting '/tmp/json.qcow1', fmt=qcow2 size=197120 
backing_file=json:{"driver":"file",,"filename":"/tmp/malicious"} 
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img info /tmp/json.qcow1
image: /tmp/json.qcow1
file format: qcow1
virtual size: 191 KiB (197120 bytes)
disk size: 195 KiB
cluster_size: 65535
backing file: json:{"driver":"file","filename":"/tmp/malicious"}
Format specific information:
 compat: 0.1
 lazy refcounts: false
 refcount bits: 15
 corrupt: false

Now this has the old semantics but we didn't even get the warning. But
at least the backing file format is not written into the overlay.


Hmm.  json:{"driver":"qcow2",...} encodes the format, but your argument 
is that json:{"driver":"file",...} encodes only the protocol but not the 
format.  We want the warning when there is no format, but with json:, it 
becomes harder to tell if a format is present or not.  I'll have to 
think about what to do in that case.




+++ b/block.c
@@ -6013,6 +6013,15 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
"Could not open backing image to determine 
size.\n");
  goto out;
  } else {
+if (!backing_fmt && !strstart(backing_file, "json:", NULL)) {
+backing_fmt = bs->drv->format_name;
+qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, backing_fmt, NULL);


We must never write the detected format into the overlay. Not even when
we print a warning. This can legitimize a malicious file if the user
mises the warning.



Point taken. v2 will address this differently.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [libvirt] [PATCH 0/8] Second take on slirp-helper & dbus-vmstate

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
>
>
> Sorry for late review. I will reply to individual patches with suggested
> changes. I have them locally as a fixup patches, so I can squash them
> and resend (keeping your authorship of course). You can also find them
> on my branch (if you want to test them):
>
>https://github.com/zippy2/libvirt/commits/qemu_dbus_review

thanks for the review and the patches, I'll send a new version soon.

Re: [PATCH 2/8] qemu-conf: add configurable dbus-daemon location

[Marc-André Lureau]

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   m4/virt-driver-qemu.m4 | 6 ++
> >   src/qemu/libvirtd_qemu.aug | 1 +
> >   src/qemu/qemu.conf | 3 +++
> >   src/qemu/qemu_conf.c   | 5 +
> >   src/qemu/qemu_conf.h   | 1 +
> >   src/qemu/test_libvirtd_qemu.aug.in | 1 +
> >   6 files changed, 17 insertions(+)
> >
>
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index b62dd1df52..e1fea390c7 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -228,6 +228,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> > privileged)
> >   cfg->bridgeHelperName = g_strdup(QEMU_BRIDGE_HELPER);
> >   cfg->prHelperName = g_strdup(QEMU_PR_HELPER);
> >   cfg->slirpHelperName = g_strdup(QEMU_SLIRP_HELPER);
> > +cfg->slirpHelperName = g_strdup(QEMU_DBUS_DAEMON);
>
> Oops, s/slirpHelperName/dbusDaemonName/  :-D
>

right, fixed

Re: [PATCH 7/8] qemu: add dbus-vmstate helper migration support

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > Helper processes may have their state migrated with QEMU data stream
> > thanks to the QEMU "dbus-vmstate".
> >
> > libvirt maintains the list of helpers to be migrated. The
> > "dbus-vmstate" is added when required, and given the list of helper
> > Ids that must be migrated, on save & load sides.
> >
> > See also:
> > https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus-vmstate.rst
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   src/qemu/qemu_alias.c|  7 +++
> >   src/qemu/qemu_alias.h|  2 +
> >   src/qemu/qemu_command.c  | 62 +++
> >   src/qemu/qemu_command.h  |  3 ++
> >   src/qemu/qemu_dbus.c | 14 ++
> >   src/qemu/qemu_dbus.h |  4 ++
> >   src/qemu/qemu_domain.c   | 10 +
> >   src/qemu/qemu_domain.h   |  5 +++
> >   src/qemu/qemu_hotplug.c  | 82 
> >   src/qemu/qemu_hotplug.h  |  8 
> >   src/qemu/qemu_migration.c| 51 ++
> >   src/qemu/qemu_monitor.c  | 21 +
> >   src/qemu/qemu_monitor.h  |  3 ++
> >   src/qemu/qemu_monitor_json.c | 15 +++
> >   src/qemu/qemu_monitor_json.h |  5 +++
> >   15 files changed, 292 insertions(+)
> >
>
>
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 71d0bb0879..8c281f3a70 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1125,10 +1125,18 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver,
> > bool remote,
> > unsigned int flags)
> >   {
> > +qemuDomainObjPrivatePtr priv = vm->privateData;
> >   int nsnapshots;
> >   int pauseReason;
> >   size_t i;
> >
> > +if (virStringListLength((const char **)priv->dbusVMStateIds) &&
> > +!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
> > +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > +   _("cannot migrate this domain without dbus-vmstate 
> > support"));
> > +return false;
> > +}
> > +
>
> This should be done in the if(!OFFLINE) a few lines below. IIUC, vmstate
> is runtime thing, and when doing offline migration (e.g. just copying
> over disks and XMLs), no qemu is involved and thus no vmstate matters.

Right, thanks


>
> >   /* perform these checks only when migrating to remote hosts */
> >   if (remote) {
> >   nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0);
>
> Michal
>

Re: [PATCH 8/8] qemu-slirp: register helper for migration

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > When the helper supports DBus, connect it to the bus and set its ID.
> >
> > If the helper supports migration, register its ID to the list of
> > dbus-vmstate ID to migrate, and specify --dbus-incoming when
> > restoring the VM.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   src/qemu/qemu_slirp.c | 37 +
> >   1 file changed, 37 insertions(+)
> >
> > diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
> > index 8e001f0d10..48fc0a68c2 100644
> > --- a/src/qemu/qemu_slirp.c
> > +++ b/src/qemu/qemu_slirp.c
> > @@ -18,6 +18,7 @@
> >
> >   #include 
> >
> > +#include "qemu_dbus.h"
> >   #include "qemu_extdevice.h"
> >   #include "qemu_security.h"
> >   #include "qemu_slirp.h"
> > @@ -202,6 +203,16 @@ qemuSlirpGetFD(qemuSlirpPtr slirp)
> >   }
> >
> >
> > +static char *
> > +qemuSlirpGetDBusVMStateId(virDomainNetDefPtr net)
> > +{
> > +char macstr[VIR_MAC_STRING_BUFLEN] = "";
> > +
> > +/* can't use alias, because it's not stable across restarts */
> > +return g_strdup_printf("slirp-%s", virMacAddrFormat(&net->mac, 
> > macstr));
> > +}
> > +
> > +
> >   void
> >   qemuSlirpStop(qemuSlirpPtr slirp,
> > virDomainObjPtr vm,
> > @@ -209,11 +220,14 @@ qemuSlirpStop(qemuSlirpPtr slirp,
> > virDomainNetDefPtr net)
> >   {
> >   g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> > +g_autofree char *id = qemuSlirpGetDBusVMStateId(net);
> >   g_autofree char *pidfile = NULL;
> >   virErrorPtr orig_err;
> >   pid_t pid;
> >   int rc;
> >
> > +qemuDBusVMStateRemove(vm, id);
> > +
> >   if (!(pidfile = qemuSlirpCreatePidFilename(cfg, vm->def, 
> > net->info.alias))) {
> >   VIR_WARN("Unable to construct slirp pidfile path");
> >   return;
> > @@ -310,6 +324,29 @@ qemuSlirpStart(qemuSlirpPtr slirp,
> >   }
> >   }
> >
> > +if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_ADDRESS)) {
> > +g_autofree char *id = qemuSlirpGetDBusVMStateId(net);
> > +g_autofree char *dbus_addr = qemuDBusGetAddress(driver, vm);
> > +
> > +if (qemuDBusStart(driver, vm) < 0)
> > +return -1;
> > +
> > +virCommandAddArgFormat(cmd, "--dbus-id=%s", id);
> > +
> > +virCommandAddArgFormat(cmd, "--dbus-address=%s", dbus_addr);
> > +
> > +if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) {
> > +if (qemuDBusVMStateAdd(vm, id) < 0) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Failed to register slirp migration"));
> > +return -1;
> > +}
> > +if (incoming) {
> > +virCommandAddArg(cmd, "--dbus-incoming");
> > +}
>
> 'make syntax-check' complains here.


thanks again for the fix!

Re: [PATCH 3/8] qemu-conf: add dbusStateDir

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > Location of DBus daemon state configuration, socket, pid...
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   src/qemu/qemu_conf.c | 4 
> >   src/qemu/qemu_conf.h | 1 +
> >   2 files changed, 5 insertions(+)
> >
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index e1fea390c7..ade12dac6c 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> > privileged)
> >
> >   cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", 
> > LOCALSTATEDIR);
> >
> > +cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", 
> > LOCALSTATEDIR);
> > +
> >   cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", 
> > LOCALSTATEDIR);
> >   cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir);
> >   cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir);
> > @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> > privileged)
> >   cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
> >
> >   cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir);
> > +cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
> >
> >   cfg->configBaseDir = virGetUserConfigDirectory();
>
> Instead of doing practically the same in two branches, you can achieve
> the same result with just one line if you put the call just below
> cfg->slirpStateDir init.
>
> However, do we need this to be in a special directory instead of per VM
> private directory? The way I implemented PR helper was that the socket
> it creates and to which qemu connects is stored under vm->priv->libDir
> which is initialized in qemuDomainSetPrivatePaths() to:
>
>$cfg->libDir/domain-$shortName/
>
> This way you wouldn't need to care about domain's shortname in the next
> patch.

Makes sense. I think I based this on slirpStateDir code. Any reason
it's not using vm->priv->libdir too?

Re: [PATCH 1/8] qemu: remove dbus-vmstate code

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > This code was based on a per-helper instance and peer-to-peer
> > connections. The code that landed in qemu master for v5.0 is relying
> > on a single instance and DBus bus.
> >
> > Instead of trying to adapt the existing dbus-vmstate code, let's
> > remove it and resubmit. That should make reviewing easier.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   src/qemu/Makefile.inc.am  |   2 -
> >   src/qemu/qemu_alias.c |  16 -
> >   src/qemu/qemu_alias.h |   3 -
> >   src/qemu/qemu_command.c   |  83 -
> >   src/qemu/qemu_command.h   |   3 -
> >   src/qemu/qemu_dbus.c  |  94 
> >   src/qemu/qemu_dbus.h  |  42 -
> >   src/qemu/qemu_domain.c|  13 
> >   src/qemu/qemu_domain.h|   1 -
> >   src/qemu/qemu_extdevice.c |   4 +-
> >   src/qemu/qemu_hotplug.c   |  83 +
> >   src/qemu/qemu_hotplug.h   |  11 
> >   src/qemu/qemu_migration.c |   8 ---
> >   src/qemu/qemu_slirp.c | 125 +-
> >   src/qemu/qemu_slirp.h |   4 +-
> >   15 files changed, 7 insertions(+), 485 deletions(-)
> >   delete mode 100644 src/qemu/qemu_dbus.c
> >   delete mode 100644 src/qemu/qemu_dbus.h
>
> You missed po/POTFILES.in:
>
> @SRCDIR@/src/qemu/qemu_dbus.c

yes, thanks

Re: [PATCH 4/8] qemu: add a DBus daemon helper unit

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > Add a unit to start & stop a private dbus-daemon.
> >
> > The daemon is meant to be started on demand, and associated with a
> > QEMU process. It should be stopped when the QEMU process is stopped.
> >
> > The current policy is permissive like a session bus. Stricter
> > policies can be added later, following recommendations from:
> > https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus.rst
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   src/qemu/Makefile.inc.am |   4 +
> >   src/qemu/qemu_dbus.c | 299 +++
> >   src/qemu/qemu_dbus.h |  40 ++
> >   src/qemu/qemu_domain.c   |   2 +
> >   src/qemu/qemu_domain.h   |   3 +
> >   tests/Makefile.am|   1 +
> >   6 files changed, 349 insertions(+)
> >   create mode 100644 src/qemu/qemu_dbus.c
> >   create mode 100644 src/qemu/qemu_dbus.h
> >
> > diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
> > index 028ab9043c..833638ec3c 100644
> > --- a/src/qemu/Makefile.inc.am
> > +++ b/src/qemu/Makefile.inc.am
> > @@ -69,6 +69,8 @@ QEMU_DRIVER_SOURCES = \
> >   qemu/qemu_checkpoint.h \
> >   qemu/qemu_backup.c \
> >   qemu/qemu_backup.h \
> > + qemu/qemu_dbus.c \
> > + qemu/qemu_dbus.h \
>
> These can be added where they were just a moment ago ;-)
>

yep

> >   $(NULL)
> >
> >
> > @@ -93,6 +95,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
> >   $(LIBNL_CFLAGS) \
> >   $(SELINUX_CFLAGS) \
> >   $(XDR_CFLAGS) \
> > + $(DBUS_CFLAGS) \
> >   -I$(srcdir)/access \
> >   -I$(builddir)/access \
> >   -I$(srcdir)/conf \
> > @@ -105,6 +108,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
> >   $(GNUTLS_LIBS) \
> >   $(LIBNL_LIBS) \
> >   $(SELINUX_LIBS) \
> > + $(DBUS_LIBS) \
> >   $(LIBXML_LIBS) \
> >   $(NULL)
> >   libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
> > diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
> > new file mode 100644
> > index 00..9c8a03c947
> > --- /dev/null
> > +++ b/src/qemu/qemu_dbus.c
> > @@ -0,0 +1,299 @@
> > +/*
> > + * qemu_dbus.c: QEMU dbus daemon
> > + *
> > + * 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_extdevice.h"
> > +#include "qemu_dbus.h"
> > +#include "qemu_security.h"
> > +
> > +#include "viralloc.h"
> > +#include "virlog.h"
> > +#include "virstring.h"
> > +#include "virtime.h"
> > +#include "virpidfile.h"
> > +
> > +#define VIR_FROM_THIS VIR_FROM_NONE
> > +
> > +VIR_LOG_INIT("qemu.dbus");
> > +
> > +
> > +int
> > +qemuDBusPrepareHost(virQEMUDriverPtr driver)
> > +{
> > +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> > +
> > +return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
> > +VIR_DIR_CREATE_ALLOW_EXIST);
> > +}
> > +
> > +
> > +static char *
> > +qemuDBusCreatePidFilename(const char *stateDir,
> > +  const char *shortName)
> > +{
> > +g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
> > +
> > +return virPidFileBuildPath(stateDir, name);
>
> Instead of having each caller pass cfg->dbusStateDir, we can accept cfg
> here and deref ourselves.

sure

>
> > +}
> > +
> > +
> > +static char *
> > +qemuDBusCreateFilename(const char *stateDir,
> > +   const char *shortName,
> > +   const char *ext)
> > +{
> > +g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
> > +
> > +return virFileBuildPath(stateDir, name,  ext);
> > +}
> > +
> > +
> > +static char *
> > +qemuDBusCreateSocketPath(virQEMUDriverConfigPtr cfg,
> > + const char *shortName)
> > +{
> > +return qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".sock");
> > +}
> > +
>
> I'd introduce qemuDBusCreateConfPath() so that nobody else has to call
> qemuDBusCreateFilename() again.

ok

>
> > +
> > +char *
> > +qemuDBusGetAddress(virQEMUDriverPtr driver,
> > +   virDomainObjPtr vm)
> > +{
> > +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> > +g_autofree char *shortName = virDomainDefGetShor

Re: [PATCH 5/8] domain: save/restore the state of dbus-daemon running

[Marc-André Lureau]

Hi

On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik  wrote:
>
> On 1/14/20 2:46 PM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> >
> > This avoids trying to start a dbus-daemon when its already running.
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >   src/qemu/qemu_domain.c | 5 +
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 7722a53c62..dda3cb781f 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -2916,6 +2916,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
> > 
> > virDomainChrTypeToString(priv->monConfig->type));
> >   }
> >
> > +if (priv->dbusDaemonRunning)
> > +virBufferAddLit(buf, "\n");
> > +
> >   if (priv->namespaces) {
> >   ssize_t ns = -1;
> >
> > @@ -3697,6 +3700,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
> >   goto error;
> >   }
> >
> > +priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", 
> > ctxt) > 0;
> > +
> >   if ((node = virXPathNode("./namespaces", ctxt))) {
> >   xmlNodePtr next;
> >
> >
>
> I'd push these a bit down - closer to PR daemon and slirp so that they
> are grouped together.

Well, as we introduce DBus bus for the VM, it would be a foundation
for IPC/multi-process communication, not specific to slirp. So I'd
leave it near the top.

Re: [libvirt PATCH 1/3] util: uuid: remove use of virHexToBin

[Laine Stump]

On 2/23/20 3:20 PM, Ján Tomko wrote:

Prefer g_ascii_xdigit_value to virHexToBin.

Check the return value of the function and
remove the g_ascii_isxdigit calls, since
they're done anyway internally.

Signed-off-by: Ján Tomko 



Reviewed-by: Laine Stump 


(The two functions behave differently in the case of being passed 
invalid hex-ascii characters, but all our uses check for valid input 
prior to calling, so that difference doesn't matter).




---
  src/util/viruuid.c | 11 +--
  1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/util/viruuid.c b/src/util/viruuid.c
index c8ee59beee..908b09945d 100644
--- a/src/util/viruuid.c
+++ b/src/util/viruuid.c
@@ -29,7 +29,6 @@
  #include 
  
  #include "internal.h"

-#include "virutil.h"
  #include "virerror.h"
  #include "virlog.h"
  #include "viralloc.h"
@@ -105,6 +104,7 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid)
  cur++;
  
  for (i = 0; i < VIR_UUID_BUFLEN;) {

+int val;
  uuid[i] = 0;
  if (*cur == 0)
  return -1;
@@ -112,16 +112,15 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid)
  cur++;
  continue;
  }
-if (!g_ascii_isxdigit(*cur))
+if ((val = g_ascii_xdigit_value(*cur)) < 0)
  return -1;
-uuid[i] = virHexToBin(*cur);
-uuid[i] *= 16;
+uuid[i] = 16 * val;
  cur++;
  if (*cur == 0)
  return -1;
-if (!g_ascii_isxdigit(*cur))
+if ((val = g_ascii_xdigit_value(*cur)) < 0)
  return -1;
-uuid[i] += virHexToBin(*cur);
+uuid[i] += val;
  i++;
  cur++;
  }

Re: [libvirt PATCH 3/4] virhostdev: move to src/hypervisor

[Michal Privoznik]

On 2/23/20 3:16 PM, Ján Tomko wrote:

This module depends on domain_conf and is used directly by various
hypervisor drivers.

Move it to src/hypervisor.

Signed-off-by: Ján Tomko 
---
  build-aux/syntax-check.mk |  2 +-
  po/POTFILES.in|  2 +-
  src/hypervisor/Makefile.inc.am|  2 +
  src/{util => hypervisor}/virhostdev.c |  0
  src/{util => hypervisor}/virhostdev.h |  0
  src/libvirt_private.syms  | 60 +--
  src/libxl/Makefile.inc.am |  1 +
  src/util/Makefile.inc.am  |  2 -
  tests/Makefile.am |  1 +
  9 files changed, 36 insertions(+), 34 deletions(-)
  rename src/{util => hypervisor}/virhostdev.c (100%)
  rename src/{util => hypervisor}/virhostdev.h (100%)


Missed aa-helper:

diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am
index 823d80c5dd..5f2f4c8e2d 100644
--- a/src/security/Makefile.inc.am
+++ b/src/security/Makefile.inc.am
@@ -100,6 +100,7 @@ virt_aa_helper_LDADD += libvirt_probes.lo
 endif WITH_DTRACE_PROBES
 virt_aa_helper_CFLAGS = \
-I$(srcdir)/conf \
+   -I$(top_srcdir)/src/hypervisor \
-I$(srcdir)/security \
$(AM_CFLAGS) \
$(PIE_CFLAGS) \


Michal

Re: [libvirt PATCH 0/4] Do not depend on conf/ in util/

[Michal Privoznik]

On 2/23/20 3:16 PM, Ján Tomko wrote:



Ján Tomko (4):
   syntax-check: inclusion rule for src/hypervisor
   conf: move virHostdevIs functions
   virhostdev: move to src/hypervisor
   virclosecallbacks: move to src/hypervisor

  build-aux/syntax-check.mk|  5 +-
  po/POTFILES.in   |  4 +-
  src/bhyve/Makefile.inc.am|  1 +
  src/conf/domain_conf.c   | 44 ++-
  src/conf/domain_conf.h   | 10 +++
  src/hypervisor/Makefile.inc.am   |  4 +
  src/{util => hypervisor}/virclosecallbacks.c |  0
  src/{util => hypervisor}/virclosecallbacks.h |  0
  src/{util => hypervisor}/virhostdev.c| 43 --
  src/{util => hypervisor}/virhostdev.h|  9 ---
  src/libvirt_private.syms | 83 ++--
  src/libxl/Makefile.inc.am|  1 +
  src/util/Makefile.inc.am |  4 -
  tests/Makefile.am|  1 +
  14 files changed, 105 insertions(+), 104 deletions(-)
  rename src/{util => hypervisor}/virclosecallbacks.c (100%)
  rename src/{util => hypervisor}/virclosecallbacks.h (100%)
  rename src/{util => hypervisor}/virhostdev.c (98%)
  rename src/{util => hypervisor}/virhostdev.h (97%)



Reviewed-by: Michal Privoznik 

Michal

[libvirt PATCH] ci: Fix handling of $PKG_CONFIG_LIBDIR

[Andrea Bolognani]

There are two environment variables that are baked into our
cross-compilation container images at build time, $CONFIGURE_OPTS
and $PKG_CONFIG_LIBDIR: the former contain the options necessary
to convince configure to perform a cross build rather than a
native one, and the latter is necessary so that pkg-config will
locate the .pc files for MinGW libraries. Container images that
are not intended for cross-compilation will not have either one
defined.

The problem is that, while an empty $CONFIGURE_OPTS is completely
harmless, setting $PKG_CONFIG_LIBDIR to an emtpy value will
result in pkg-config not looking in its default search path, thus
not finding any library, and subsequently breaking native builds.

To work around this issue, only pass $PKG_CONFIG_LIBDIR to sudo
when the value is set in the calling environment.

Fixes: 71517ae4db35c4dcc6c358d60d3a6d5da0615d39
Signed-off-by: Andrea Bolognani 
---
Pushed as a CI fix.

 ci/Makefile | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ci/Makefile b/ci/Makefile
index 03799924b4..577b130d2f 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -216,12 +216,15 @@ ci-run-command@%: ci-prepare-tree
$(CI_ENGINE) run $(CI_ENGINE_ARGS) $(CI_IMAGE_PREFIX)$*$(CI_IMAGE_TAG) \
/bin/bash -c ' \
$(CI_USER_HOME)/prepare || exit 1; \
+   if test "$$PKG_CONFIG_LIBDIR"; then \
+   pkgconfig_env="PKG_CONFIG_LIBDIR=$$PKG_CONFIG_LIBDIR"; \
+   fi; \
sudo \
  --login \
  --user="#$(CI_UID)" \
  --group="#$(CI_GID)" \
  CONFIGURE_OPTS="$$CONFIGURE_OPTS" \
- PKG_CONFIG_LIBDIR="$$PKG_CONFIG_LIBDIR" \
+ $$pkgconfig_env \
  CI_CONT_SRCDIR="$(CI_CONT_SRCDIR)" \
  CI_CONT_BUILDDIR="$(CI_CONT_BUILDDIR)" \
  CI_SMP="$(CI_SMP)" \
-- 
2.24.1

[PATCH v4 3/5] lxc: Replacing default strings definitions by g_autofree statement

[Julio Faracco]

There are a lots of strings being handled inside some LXC functions.
They can be moved to g_autofree to avoid declaring a return value to get
proper code cleanups. This commit is changing functions from
lxc_{controller,cgroup,fuse} only.

Signed-off-by: Julio Faracco 
---
 src/lxc/lxc_cgroup.c | 15 +++
 src/lxc/lxc_controller.c | 96 ++--
 src/lxc/lxc_fuse.c   | 23 +++---
 3 files changed, 44 insertions(+), 90 deletions(-)

diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 997a5c3dfa..d29b65092a 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -54,8 +54,7 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
virCgroupPtr cgroup,
virBitmapPtr nodemask)
 {
-int ret = -1;
-char *mask = NULL;
+g_autofree char *mask = NULL;
 virDomainNumatuneMemMode mode;
 
 if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO &&
@@ -66,21 +65,17 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
 
 if (virDomainNumatuneGetMode(def->numa, -1, &mode) < 0 ||
 mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) {
-ret = 0;
-goto cleanup;
+return 0;
 }
 
 if (virDomainNumatuneMaybeFormatNodeset(def->numa, nodemask,
 &mask, -1) < 0)
-goto cleanup;
+return -1;
 
 if (mask && virCgroupSetCpusetMems(cgroup, mask) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
- cleanup:
-VIR_FREE(mask);
-return ret;
+return 0;
 }
 
 
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 518967ee83..c580b17f5f 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -802,8 +802,7 @@ static int 
virLXCControllerGetNumadAdvice(virLXCControllerPtr ctrl,
   virBitmapPtr *mask)
 {
 virBitmapPtr nodemask = NULL;
-char *nodeset = NULL;
-int ret = -1;
+g_autofree char *nodeset = NULL;
 
 /* Get the advisory nodeset from numad if 'placement' of
  * either  or  is 'auto'.
@@ -812,20 +811,17 @@ static int 
virLXCControllerGetNumadAdvice(virLXCControllerPtr ctrl,
 nodeset = 
virNumaGetAutoPlacementAdvice(virDomainDefGetVcpus(ctrl->def),
 ctrl->def->mem.cur_balloon);
 if (!nodeset)
-goto cleanup;
+return -1;
 
 VIR_DEBUG("Nodeset returned from numad: %s", nodeset);
 
 if (virBitmapParse(nodeset, &nodemask, VIR_DOMAIN_CPUMASK_LEN) < 0)
-goto cleanup;
+return -1;
 }
 
-ret = 0;
 *mask = nodemask;
 
- cleanup:
-VIR_FREE(nodeset);
-return ret;
+return 0;
 }
 
 
@@ -1434,9 +1430,8 @@ virLXCControllerSetupUsernsMap(virDomainIdMapEntryPtr map,
  */
 static int virLXCControllerSetupUserns(virLXCControllerPtr ctrl)
 {
-char *uid_map = NULL;
-char *gid_map = NULL;
-int ret = -1;
+g_autofree char *uid_map = NULL;
+g_autofree char *gid_map = NULL;
 
 /* User namespace is disabled for container */
 if (ctrl->def->idmap.nuidmap == 0) {
@@ -1450,28 +1445,23 @@ static int 
virLXCControllerSetupUserns(virLXCControllerPtr ctrl)
 if (virLXCControllerSetupUsernsMap(ctrl->def->idmap.uidmap,
ctrl->def->idmap.nuidmap,
uid_map) < 0)
-goto cleanup;
+return -1;
 
 gid_map = g_strdup_printf("/proc/%d/gid_map", ctrl->initpid);
 
 if (virLXCControllerSetupUsernsMap(ctrl->def->idmap.gidmap,
ctrl->def->idmap.ngidmap,
gid_map) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
- cleanup:
-VIR_FREE(uid_map);
-VIR_FREE(gid_map);
-return ret;
+return 0;
 }
 
 static int virLXCControllerSetupDev(virLXCControllerPtr ctrl)
 {
-char *mount_options = NULL;
-char *opts = NULL;
-char *dev = NULL;
-int ret = -1;
+g_autofree char *mount_options = NULL;
+g_autofree char *opts = NULL;
+g_autofree char *dev = NULL;
 
 VIR_DEBUG("Setting up /dev/ for container");
 
@@ -1488,24 +1478,18 @@ static int virLXCControllerSetupDev(virLXCControllerPtr 
ctrl)
 opts = g_strdup_printf("mode=755,size=65536%s", mount_options);
 
 if (virFileSetupDev(dev, opts) < 0)
-goto cleanup;
+return -1;
 
 if (lxcContainerChown(ctrl->def, dev) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
- cleanup:
-VIR_FREE(opts);
-VIR_FREE(mount_options);
-VIR_FREE(dev);
-return ret;
+return 0;
 }
 
 static int virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
 {
 size_t i;
-int ret = -1;
-char *path = NULL;
+g_autofree char *path = NULL;
 const struct {
 int maj;
 int min;
@@ -1521,7 +1505,7 @@ stati

[PATCH v4 5/5] lxc: Count max VCPUs based on cpuset.cpus in native config

[Julio Faracco]

Native config files sometimes can setup cpuset.cpus to pin some CPUs.
Before this, LXC was using a fixed number of 1 VCPU. After this commit,
XML definition will generate a dynamic number of VCPUs based on that
cgroup attribute.

Signed-off-by: Julio Faracco 
---
 src/lxc/lxc_container.c   | 23 ++
 src/lxc/lxc_container.h   |  2 ++
 src/lxc/lxc_native.c  | 24 +--
 .../lxcconf2xml-cpusettune.xml|  2 +-
 4 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 88e27f3060..c5788e5c32 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -2487,3 +2487,26 @@ int lxcContainerChown(virDomainDefPtr def, const char 
*path)
 
 return 0;
 }
+
+
+int lxcContainerGetMaxCpusInCpuset(const char *cpuset)
+{
+const char *c = cpuset;
+int max_cpu = 0;
+
+while (c) {
+int a, b, ret;
+
+ret = sscanf(c, "%d-%d", &a, &b);
+if (ret == 1)
+max_cpu++;
+else if (ret == 2)
+max_cpu +=  a > b ? a - b + 1 : b - a + 1;
+
+if (!(c = strchr(c+1, ',')))
+break;
+c++;
+}
+
+return max_cpu;
+}
diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
index 94a6c5309c..6f112e0667 100644
--- a/src/lxc/lxc_container.h
+++ b/src/lxc/lxc_container.h
@@ -63,3 +63,5 @@ virArch lxcContainerGetAlt32bitArch(virArch arch);
 int lxcContainerChown(virDomainDefPtr def, const char *path);
 
 bool lxcIsBasicMountLocation(const char *path);
+
+int lxcContainerGetMaxCpusInCpuset(const char *cpuset);
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
index 02d2bf33e4..409bf00bd2 100644
--- a/src/lxc/lxc_native.c
+++ b/src/lxc/lxc_native.c
@@ -993,6 +993,24 @@ lxcSetCpusetTune(virDomainDefPtr def, virConfPtr 
properties)
 return 0;
 }
 
+
+static int
+lxcGetVCpuMax(virConfPtr properties)
+{
+g_autofree char *value = NULL;
+int vcpumax = 1;
+
+if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
+  &value) > 0) {
+vcpumax = lxcContainerGetMaxCpusInCpuset(value);
+if (vcpumax > 0)
+return vcpumax;
+}
+
+return vcpumax;
+}
+
+
 static int
 lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr value, void *data)
 {
@@ -1132,6 +1150,7 @@ lxcParseConfigString(const char *config,
 virDomainDefPtr vmdef = NULL;
 g_autoptr(virConf) properties = NULL;
 g_autofree char *value = NULL;
+int vcpumax;
 
 if (!(properties = virConfReadString(config, VIR_CONF_FLAG_LXC_FORMAT)))
 return NULL;
@@ -1155,10 +1174,11 @@ lxcParseConfigString(const char *config,
 
 /* Value not handled by the LXC driver, setting to
  * minimum required to make XML parsing pass */
-if (virDomainDefSetVcpusMax(vmdef, 1, xmlopt) < 0)
+vcpumax = lxcGetVCpuMax(properties);
+if (virDomainDefSetVcpusMax(vmdef, vcpumax, xmlopt) < 0)
 goto error;
 
-if (virDomainDefSetVcpus(vmdef, 1) < 0)
+if (virDomainDefSetVcpus(vmdef, vcpumax) < 0)
 goto error;
 
 vmdef->nfss = 0;
diff --git a/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml 
b/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml
index 6df089d00f..a1fec12d9b 100644
--- a/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml
+++ b/tests/lxcconf2xmldata/lxcconf2xml-cpusettune.xml
@@ -3,7 +3,7 @@
   c7a5fdbd-edaf-9455-926a-d65c16db1809
   65536
   65536
-  1
+  5
   
 
   
-- 
2.20.1

[PATCH v4 2/5] lxc: Add HPET device into allowed devices

[Julio Faracco]

This commit is related to RTC timer device too. HPET is being shared
from host device through `localtime` clock. This timer is available
creating a new timer using `hpet` name.

Signed-off-by: Julio Faracco 
---
 docs/formatdomain.html.in |  2 +-
 src/lxc/lxc_cgroup.c  | 17 +
 src/lxc/lxc_controller.c  | 33 +
 3 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 5598bf41b4..8571db89dc 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2464,7 +2464,7 @@
 The name attribute selects which timer is
 being modified, and can be one of
 "platform" (currently unsupported),
-"hpet" (libxl, xen, qemu), "kvmclock" (qemu),
+"hpet" (libxl, xen, qemu, lxc), "kvmclock" (qemu),
 "pit" (qemu), "rtc" (qemu, lxc), "tsc" (libxl, qemu -
 since 3.2.0), "hypervclock"
 (qemu - since 1.2.2) or
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 6a103055a4..997a5c3dfa 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -344,20 +344,19 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
 for (i = 0; i < def->clock.ntimers; i++) {
 virDomainTimerDefPtr timer = def->clock.timers[i];
 
+if (!timer->present)
+break;
+
 switch ((virDomainTimerNameType)timer->name) {
 case VIR_DOMAIN_TIMER_NAME_PLATFORM:
 case VIR_DOMAIN_TIMER_NAME_TSC:
 case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
 case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
 case VIR_DOMAIN_TIMER_NAME_PIT:
-case VIR_DOMAIN_TIMER_NAME_HPET:
 case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
 case VIR_DOMAIN_TIMER_NAME_LAST:
 break;
 case VIR_DOMAIN_TIMER_NAME_RTC:
-if (!timer->present)
-break;
-
 if (virFileExists("/dev/rtc")) {
 if (virCgroupAllowDevicePath(cgroup, "/dev/rtc",
  VIR_CGROUP_DEVICE_READ,
@@ -367,6 +366,16 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
 VIR_DEBUG("Ignoring non-existent device /dev/rtc");
 }
 break;
+case VIR_DOMAIN_TIMER_NAME_HPET:
+if (virFileExists("/dev/hpet")) {
+if (virCgroupAllowDevicePath(cgroup, "/dev/hpet",
+ VIR_CGROUP_DEVICE_READ,
+ false) < 0)
+return -1;
+} else {
+VIR_DEBUG("Ignoring non-existent device /dev/hpet");
+}
+break;
 }
 }
 } else {
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index eba6bfe0bf..518967ee83 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1566,13 +1566,15 @@ virLXCControllerSetupTimers(virLXCControllerPtr ctrl)
 dev_t dev;
 virDomainTimerDefPtr timer = def->clock.timers[i];
 
+if (!timer->present)
+continue;
+
 switch ((virDomainTimerNameType)timer->name) {
 case VIR_DOMAIN_TIMER_NAME_PLATFORM:
 case VIR_DOMAIN_TIMER_NAME_TSC:
 case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
 case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
 case VIR_DOMAIN_TIMER_NAME_PIT:
-case VIR_DOMAIN_TIMER_NAME_HPET:
 case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
 case VIR_DOMAIN_TIMER_NAME_LAST:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1580,9 +1582,6 @@ virLXCControllerSetupTimers(virLXCControllerPtr ctrl)
virDomainTimerNameTypeToString(timer->name));
 return -1;
 case VIR_DOMAIN_TIMER_NAME_RTC:
-if (!timer->present)
-break;
-
 if (stat("/dev/rtc", &sb) < 0) {
 if (errno == EACCES)
 return -1;
@@ -1605,6 +1604,32 @@ virLXCControllerSetupTimers(virLXCControllerPtr ctrl)
 return -1;
 }
 
+if (lxcContainerChown(ctrl->def, path) < 0)
+return -1;
+break;
+case VIR_DOMAIN_TIMER_NAME_HPET:
+if (stat("/dev/hpet", &sb) < 0) {
+if (errno == EACCES)
+return -1;
+
+virReportSystemError(errno,
+ _("Path '%s' is not accessible"),
+ path);
+return -1;
+}
+
+path = g_strdup_printf("/%s/%s.dev/%s", LXC_STATE_DIR,
+   ctrl->def->name, "/hpet");
+
+dev = makedev(major(sb.st_rdev), minor(sb.st_rdev));
+if (mknod(path, S_IFCHR, dev) < 

[PATCH v4 4/5] lxc: Implement virtual /proc/cpuinfo via LXC fuse

[Julio Faracco]

This commit tries to fix lots of issues related to LXC VCPUs. One of
them is related to /proc/cpuinfo content. If only 1 VCPU is set, LXC
containers will show all CPUs available for host. The second one is
related to CPU share, if an user set only 1 VCPU, the container/process
will use all available CPUs. (This is not the case when `cpuset`
attribute is declared). So, this commit adds a virtual cpuinfo based on
VCPU mapping and it automatically limits the CPU usage according VCPU
count.

Example (now):
LXC container - 8 CPUS with 2 VCPU:
lxc-root# stress --cpu 8

On host machine, only CPU 0 and 1 have 100% usage.

Signed-off-by: Julio Faracco 
---
 src/lxc/lxc_cgroup.c| 31 ++
 src/lxc/lxc_container.c | 39 ++---
 src/lxc/lxc_fuse.c  | 95 ++---
 3 files changed, 145 insertions(+), 20 deletions(-)

diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index d29b65092a..912a252473 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -50,6 +50,34 @@ static int virLXCCgroupSetupCpuTune(virDomainDefPtr def,
 }
 
 
+static int virLXCCgroupSetupVcpuAuto(virDomainDefPtr def,
+ virCgroupPtr cgroup)
+{
+size_t i;
+int vcpumax;
+virBuffer buffer = VIR_BUFFER_INITIALIZER;
+virBufferPtr cpuset = &buffer;
+
+vcpumax = virDomainDefGetVcpusMax(def);
+for (i = 0; i < vcpumax; i++) {
+virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(def, i);
+/* Cgroup is smart enough to convert numbers separated
+ * by comma into ranges. Example: "0,1,2,5," -> "0-2,5".
+ * Libvirt does not need to process it here. */
+if (vcpu)
+virBufferAsprintf(cpuset, "%zu,", i);
+}
+if (virCgroupSetCpusetCpus(cgroup,
+   virBufferCurrentContent(cpuset)) < 0) {
+virBufferFreeAndReset(cpuset);
+return -1;
+}
+
+virBufferFreeAndReset(cpuset);
+return 0;
+}
+
+
 static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
virCgroupPtr cgroup,
virBitmapPtr nodemask)
@@ -61,6 +89,9 @@ static int virLXCCgroupSetupCpusetTune(virDomainDefPtr def,
 def->cpumask &&
 virCgroupSetupCpusetCpus(cgroup, def->cpumask) < 0) {
 return -1;
+} else {
+/* auto mode for VCPU limits */
+virLXCCgroupSetupVcpuAuto(def, cgroup);
 }
 
 if (virDomainNumatuneGetMode(def->numa, -1, &mode) < 0 ||
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 41efe43a14..88e27f3060 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -997,8 +997,8 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
 static int lxcContainerMountProcFuse(virDomainDefPtr def,
  const char *stateDir)
 {
-int ret;
-char *meminfo_path = NULL;
+g_autofree char *meminfo_path = NULL;
+g_autofree char *cpuinfo_path = NULL;
 
 VIR_DEBUG("Mount /proc/meminfo stateDir=%s", stateDir);
 
@@ -1006,15 +1006,29 @@ static int lxcContainerMountProcFuse(virDomainDefPtr 
def,
stateDir,
def->name);
 
-if ((ret = mount(meminfo_path, "/proc/meminfo",
- NULL, MS_BIND, NULL)) < 0) {
+if (mount(meminfo_path, "/proc/meminfo",
+ NULL, MS_BIND, NULL) < 0) {
 virReportSystemError(errno,
  _("Failed to mount %s on /proc/meminfo"),
  meminfo_path);
+return -1;
 }
 
-VIR_FREE(meminfo_path);
-return ret;
+VIR_DEBUG("Mount /proc/cpuinfo stateDir=%s", stateDir);
+
+cpuinfo_path = g_strdup_printf("/.oldroot/%s/%s.fuse/cpuinfo",
+   stateDir,
+   def->name);
+
+if (mount(cpuinfo_path, "/proc/cpuinfo",
+ NULL, MS_BIND, NULL) < 0) {
+virReportSystemError(errno,
+ _("Failed to mount %s on /proc/cpuinfo"),
+ cpuinfo_path);
+return -1;
+}
+
+return 0;
 }
 #else
 static int lxcContainerMountProcFuse(virDomainDefPtr def G_GNUC_UNUSED,
@@ -1027,8 +1041,7 @@ static int lxcContainerMountProcFuse(virDomainDefPtr def 
G_GNUC_UNUSED,
 static int lxcContainerMountFSDev(virDomainDefPtr def,
   const char *stateDir)
 {
-int ret = -1;
-char *path = NULL;
+g_autofree char *path = NULL;
 int flags = def->idmap.nuidmap ? MS_BIND : MS_MOVE;
 
 VIR_DEBUG("Mount /dev/ stateDir=%s", stateDir);
@@ -1038,7 +1051,7 @@ static int lxcContainerMountFSDev(virDomainDefPtr def,
 if (virFileMakePath("/dev") < 0) {
 virReportSystemError(errno, "%s",
  _("Cannot create /dev"));
-goto cleanup;
+return -1;
 }
 
 VIR_DEBUG("T

[PATCH v4 1/5] lxc: Add Real Time Clock device into allowed devices

[Julio Faracco]

This commit share host Real Time Clock device (rtc) into LXC containers
to support hardware clock. This should be available setting up a `rtc`
timer under clock section. Since this option is not emulated, it should
be available only for `localtime` clock. This option should be readonly
due to security reasons.

Before:
root# hwclock --verbose
hwclock from util-linux 2.32.1
System Time: 1581877557.598365
Trying to open: /dev/rtc0
Trying to open: /dev/rtc
Trying to open: /dev/misc/rtc
No usable clock interface found.
hwclock: Cannot access the Hardware Clock via any known method.

Now:
root# hwclock
2020-02-16 18:23:55.374134+00:00
root# hwclock -w
hwclock: ioctl(RTC_SET_TIME) to /dev/rtc to set the time failed:
Permission denied

Signed-off-by: Julio Faracco 
---
 docs/formatdomain.html.in |  2 +-
 src/lxc/lxc_cgroup.c  | 36 +
 src/lxc/lxc_controller.c  | 68 +++
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4fef2a0a97..5598bf41b4 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2465,7 +2465,7 @@
 being modified, and can be one of
 "platform" (currently unsupported),
 "hpet" (libxl, xen, qemu), "kvmclock" (qemu),
-"pit" (qemu), "rtc" (qemu), "tsc" (libxl, qemu -
+"pit" (qemu), "rtc" (qemu, lxc), "tsc" (libxl, qemu -
 since 3.2.0), "hypervclock"
 (qemu - since 1.2.2) or
 "armvtimer" (qemu - since 6.1.0).
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index 4ebe5ef467..6a103055a4 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -337,6 +337,42 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
  VIR_CGROUP_DEVICE_RWM) < 0)
 return -1;
 
+VIR_DEBUG("Allowing timers char devices");
+
+/* Sync'ed with Host clock */
+if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME) {
+for (i = 0; i < def->clock.ntimers; i++) {
+virDomainTimerDefPtr timer = def->clock.timers[i];
+
+switch ((virDomainTimerNameType)timer->name) {
+case VIR_DOMAIN_TIMER_NAME_PLATFORM:
+case VIR_DOMAIN_TIMER_NAME_TSC:
+case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
+case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
+case VIR_DOMAIN_TIMER_NAME_PIT:
+case VIR_DOMAIN_TIMER_NAME_HPET:
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
+case VIR_DOMAIN_TIMER_NAME_LAST:
+break;
+case VIR_DOMAIN_TIMER_NAME_RTC:
+if (!timer->present)
+break;
+
+if (virFileExists("/dev/rtc")) {
+if (virCgroupAllowDevicePath(cgroup, "/dev/rtc",
+ VIR_CGROUP_DEVICE_READ,
+ false) < 0)
+return -1;
+} else {
+VIR_DEBUG("Ignoring non-existent device /dev/rtc");
+}
+break;
+}
+}
+} else {
+VIR_DEBUG("Ignoring non-localtime clock");
+}
+
 VIR_DEBUG("Device whitelist complete");
 
 return 0;
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index c3dec0859c..eba6bfe0bf 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1550,6 +1550,71 @@ static int 
virLXCControllerPopulateDevices(virLXCControllerPtr ctrl)
 }
 
 
+static int
+virLXCControllerSetupTimers(virLXCControllerPtr ctrl)
+{
+g_autofree char *path = NULL;
+size_t i;
+struct stat sb;
+virDomainDefPtr def = ctrl->def;
+
+/* Not sync'ed with Host clock */
+if (def->clock.offset != VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME)
+return 0;
+
+for (i = 0; i < def->clock.ntimers; i++) {
+dev_t dev;
+virDomainTimerDefPtr timer = def->clock.timers[i];
+
+switch ((virDomainTimerNameType)timer->name) {
+case VIR_DOMAIN_TIMER_NAME_PLATFORM:
+case VIR_DOMAIN_TIMER_NAME_TSC:
+case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
+case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
+case VIR_DOMAIN_TIMER_NAME_PIT:
+case VIR_DOMAIN_TIMER_NAME_HPET:
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
+case VIR_DOMAIN_TIMER_NAME_LAST:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unsupported timer type (name) '%s'"),
+   virDomainTimerNameTypeToString(timer->name));
+return -1;
+case VIR_DOMAIN_TIMER_NAME_RTC:
+if (!timer->present)
+break;
+
+if (stat("/dev/rtc", &sb) < 0) {
+if (errno == EACCES)
+return -1;
+
+virReportSystemError(errno,
+ 

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 02:34:16PM +0100, Peter Krempa wrote:
> On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
> > [adding qemu]
> 
> Adding Daniel as he objected to qemu-img.
> 
> > 
> > On 2/19/20 12:57 PM, Peter Krempa wrote:
> 
> [...]
> 
> > > Additionally I think that we could just get rid of the copy of the image
> > > detection copy in libvirt and replace it by invocation of qemu-img. That
> > > way we could avoid any discrepancies in the detection process in the
> > > first place.
> > 
> > Now there's an interesting thought.  Since data corruption occurs when there
> > is disagreement about which mode to use, getting libvirt out of the probing
> > business by deferring all decisions to qemu-img info is a smart move - if
> > qemu says an image probes as qcow2 (in an environment where probing is
> > safe), then libvirt passing an explicit qcow2 to qemu for guest usage (in an
> > environment where probing is not safe) will at least see the same
> > guest-visible data.  Less code to maintain in libvirt, and no chance for a
> > mismatch between the two projects on which format a probe should result in.
> 
> I raised the use of qemu-img to Daniel and he disagreed with use of
> qemu-img in libvirt for doing the probing on multiple reasons:
>  - qemu-img instantiates many data structures relevant to the format so
>it has a huge attack surface

This was the most critical reason why we have this code in libvirt
in the first place.

NB, we need to be sure we are comparing the same things between
libvirt and QEMU when we discuss "probing".

What we're talking about by probing in libvirt context is

  - Detect the image format
  - Detect the image virtual size
  - Detect the image physical size
  - Detect the image backing store location
  - Detect the image backing store format
  - Detect the image encryption usage

In QEMU 'format probing' (as impl by bdrv_probe in QEMU's block layer)
only covers the very first point, 'detect the image format'. All the
other information can only be acquired by opening the image (bdrv_open
in QEMU's block layer).

The issue is that bdrv_open does waay more than we desire here,
because it is serving the broader purpose of allowing QEMU to actually
use the image. qcow2 is probably the worst case example, as it has
to parse the image and setup data structures for l1, l2 tables,
refcount tables, snapshots, and initialize the encryption layer if
present.

It is known that this code is vulnerable to maliciously created
qcow2 images. This resulted in OpenStack being vulnerable to
CVE-2015-5162 https://bugs.launchpad.net/ossa/+bug/1449062

It isn't possible to do anything to avoid this risk if you are
invoking qemu-img on untrustworthy images. The best you can do
is to mitigate the effects by placing memory/CPU ulimits on
the qemu-img process. Determining these limits then introduces
a new problem, as you have to pick a limit which is low enough
to avoid DoS, while large enough to allow all valid usage.

Since mitigating CVE-2015-5162 OpenStack has faced this problem
with users reporting that the limits it set were breaking valid
usage, as so had to increase the limits, which increases the
DoS impact.  Then there's also the pain that OSP suffered when
QEMU introduced mandatory locking which broke all existing
usage of 'qemu-img info' when a VM was running.

Of course when you launch QEMU later, it is susceptible to the
DoS in the system emulator, but this is mitigated by fact that
upfront probing is going to reject some malicious images. If
some bad images do get past, then it will be dealt with by the
mgmt apps normal monitoring of a running VM resource usage
and/or cgroups limits.

The libvirt probing code is designed to do the minimal work
needed to get the information we require. Of course there may
be bugs in libvirt's code, but it is much more straightforward
for us to analyse & understand risks, as most of the problematic
code that QEMU has simply doesn't exist in libvirt.

>  - performance of spawning extra processes would be way worse

Yes, this was the second motivation for having this code in libvirt
originally. The QEMU VM startup case wasn't the target, but rather
storage pools code. When we start a storage pool with 100's of images,
the time to spawn 100's of instances of qemu-img adds up very quickly.
Even if qemu-img had exactly the same minimalist code as libvirt's
current probing logic it would still be worse. The overhead of process
startup vs the time spent probing the image is a poor ratio, such that
process startup/exec time dominates.


The third reason why libvirt has this code is because historically
the error reporting from qemu-img has been quite unhelpful - many
errors just end up being a generic EINVAL error message. Things have
improved over time, but error reporting is always a challenge when
spawning external commands to do work.


The fourth reason why libvirt has this image file detection code is
that it is used by non-QEMU drivers

[PATCH v4 0/5] lxc: Add VCPU features for LXC

[Julio Faracco]

This series cover a lots of functionalities to LXC VCPUs. It enables
sharing some timer devices between host and LXC guest using `timer`
settings. It still has other improvements related to VCPU and LXC such
as virtual cpuinfo content based on VCPU settings and some better
resource limits. Each patch has the description of the problem and what
it is trying to fix.

v1-v2: Add Daniel's comments and some cleanups.
v2-v3: Remove dependency from patch 4 and 5.
v3-v4: Missing cpuinfo file from Fuse Getattr handler.

Julio Faracco (5):
  lxc: Add Real Time Clock device into allowed devices
  lxc: Add HPET device into allowed devices
  lxc: Replacing default strings definitions by g_autofree statement
  lxc: Implement virtual /proc/cpuinfo via LXC fuse
  lxc: Count max VCPUs based on cpuset.cpus in native config

 docs/formatdomain.html.in |   4 +-
 src/lxc/lxc_cgroup.c  |  91 -
 src/lxc/lxc_container.c   |  62 --
 src/lxc/lxc_container.h   |   2 +
 src/lxc/lxc_controller.c  | 187 --
 src/lxc/lxc_fuse.c| 114 +--
 src/lxc/lxc_native.c  |  24 ++-
 .../lxcconf2xml-cpusettune.xml|   2 +-
 8 files changed, 374 insertions(+), 112 deletions(-)

-- 
2.20.1

Re: [PATCH] apparmor: allow to call vhost-user-gpu

[Christian Ehrhardt]

On Fri, Feb 14, 2020 at 10:19 PM Jim Fehlig  wrote:

> On 2/14/20 1:14 PM, Christian Ehrhardt wrote:
> >
> >
> > On Fri, Feb 14, 2020 at 6:00 PM Jim Fehlig  > > wrote:
> >
> > On 2/13/20 4:32 AM, Christian Ehrhardt wrote:
> >  > Configuring vhost-user-gpu like:
> >  >  
> >  >
> >  >
> >  >  
> >  > Triggers an apparmor denial like:
> >  >  apparmor="DENIED" operation="exec" profile="libvirtd"
> >  >  name="/usr/lib/qemu/vhost-user-gpu" pid=888257
> comm="libvirtd"
> >  >  requested_mask="x" denied_mask="x" fsuid=0 ouid=0
> >  >
> >  > This helper is provided by qemu for vhost-user-gpu and thereby
> being
> >  > in the same path as qemu_bridge_helper. Due to that adding a rule
> allowing
> >  > to call uses the same path list.
> >
> > Does the vhost-usr-gpu helper need a profile to restrict its access,
> similar to
> > the bridge helper?
> >
> > Hi Jim,
> > Yes - we can later on add one, as soon as someone did the work to trace
> all the
> > things that will be needed.
> > I had no full setup - and I'm not sure about the multitude of potential
> > configurations - so I didn't go that far.
> > I didn't have that yet, but if anyone has please just add a follow on
> patch.
> >
> > The P in PUx allows that someone defines an external profile to guard it
> - and
> > it would be used, but without one existing the U allows it to fall back
> to
> > unconfined.
>
> Nod. That's reasonable behavior in the absence of an internal profile.
>
> > If/Once we add an internal profile like we do for bridge helper P can be
> changed
> > to C, but as I said I have no useful profile and no full setup to
> test&train one
> > at the moment.
>
> With a bit of searching I could probably find a setup, but getting access
> is
> another thing. In the meantime your patch is a fine alternative IMO
>
> Reviewed-by: Jim Fehlig 
>

Thanks, pushed with the Reviewed-by added.


> Regards,
> Jim
>
> >  >
> >  > Signed-off-by: Christian Ehrhardt <
> christian.ehrha...@canonical.com
> > >
> >  > ---
> >  >   src/security/apparmor/usr.sbin.libvirtd.in
> >  | 1 +
> >  >   1 file changed, 1 insertion(+)
> >  >
> >  > diff --git a/src/security/apparmor/usr.sbin.libvirtd.in
> >  b/src/security/apparmor/
> usr.sbin.libvirtd.in
> > 
> >  > index b384b7213b..1e137039e9 100644
> >  > --- a/src/security/apparmor/usr.sbin.libvirtd.in
> > 
> >  > +++ b/src/security/apparmor/usr.sbin.libvirtd.in
> > 
> >  > @@ -86,6 +86,7 @@ profile libvirtd @sbindir@/libvirtd
> > flags=(attach_disconnected) {
> >  > /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
> >  > /usr/{lib,lib64}/xen/bin/* Ux,
> >  > /usr/lib/xen-*/bin/libxl-save-helper PUx,
> >  > +  /usr/{lib,lib64,lib/qemu,libexec}/vhost-user-gpu PUx,
> >  >
> >  > # Required by
> nwfilter_ebiptables_driver.c:ebiptablesWriteToTempFile() to
> >  > # read and run an ebtables script.
> >  >
> >
> >
> >
> > --
> > Christian Ehrhardt
> > Staff Engineer, Ubuntu Server
> > Canonical Ltd
>
>

-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH 8/9] virStorageFileGetMetadataRecurse: Allow format probing under special circumstances

[Peter Krempa]

On Wed, Feb 19, 2020 at 13:12:53 -0600, Eric Blake wrote:
> [adding qemu]

Adding Daniel as he objected to qemu-img.

> 
> On 2/19/20 12:57 PM, Peter Krempa wrote:

[...]

> > Additionally I think that we could just get rid of the copy of the image
> > detection copy in libvirt and replace it by invocation of qemu-img. That
> > way we could avoid any discrepancies in the detection process in the
> > first place.
> 
> Now there's an interesting thought.  Since data corruption occurs when there
> is disagreement about which mode to use, getting libvirt out of the probing
> business by deferring all decisions to qemu-img info is a smart move - if
> qemu says an image probes as qcow2 (in an environment where probing is
> safe), then libvirt passing an explicit qcow2 to qemu for guest usage (in an
> environment where probing is not safe) will at least see the same
> guest-visible data.  Less code to maintain in libvirt, and no chance for a
> mismatch between the two projects on which format a probe should result in.

I raised the use of qemu-img to Daniel and he disagreed with use of
qemu-img in libvirt for doing the probing on multiple reasons:
 - qemu-img instantiates many data structures relevant to the format so
   it has a huge attack surface
 - performance of spawning extra processes would be way worse

While at least from the point of view of VM startup both can be
challenged this adds a complete new orthogonal dimension to the problem
I'm attempting to fix.

I'll reiterate the historical state of the problem because I think it's
important:

Pre-blockdev:
  - we internally assumed that if the image format of an backing image
was not present in the overlay it is 'raw'
  - this influenced security labelling but not actually how qemu viewed
or probed the file. If it was qcow2 probed as qcow2 qemu opened it
as qcow2 possibly even including the backing file if selinux or
other mechanism didn't prevent it.

post-blockdev:
  - the assumption of 'raw' would now be expressed into the qemu
configuration. This assumption turned into data corruption since we
no longer allowed qemu to probe the format and forced it as raw.
  - fix was to always require the format to be recorded in the overlay
  - this made users unhappy who neglected to record the format into the
overlay when creating it manually

Now since qemu didn't discourage the creation of overlays without format
there still are many users which will inevitably hit this problem when
used with libvirt.

My proposal tries to mitigate the regressions in behaviour in the valid
and secure use cases. (If the image whose format we detect doesn't have
a backing image)

This comes at a trade-off though. As Eric pointed out, if the format
probed by libvirt's internal code disagrees with qemu's format we are
getting into the image corruption region.

As a mitigation to the above I suggested using qemu-img to probe but
that's a complex change and as mentioned above not really welcome
upstream.

Now this adds an interresting dimension to this problem. If libvirt
forces the users to specify the image format, and the users don't know
it they will probe. So we are basically making this a problem of
somebody else. [2] As you can see in that patch, it uses 'qemu-img'
anyways and also additionally actually allows the chain to continue
deeper! [3]

A partial relief to the image detection problem is that qemu would
refuse to start if an non-qcow2 image is used in qcow2, thus we really
only run into problems if qcow2 is mis-detected as raw.

This boils down to whether we want to accept some possibility of image
corruption in trade for avoiding regression of behaviour in the secure
cases as well as management apps and users not having to re-invent when
probing an image is actually safe.

Finally I think we should either decide to fix it in this release, or
stick with the error message forever. Fixing it later will not make
much sense as many users already fixed their scripts and we'd just add
back the trade-off of possible image corruption.

Peter

[1] If e.g. the security subsystem of the host didn't forbid the use of
the backing file such a qcow2 qemu would happily open it.

[2] https://www.redhat.com/archives/libguestfs/2020-February/msg00013.html

[3] As implemented in [2] the backing image is not checked whether it
has a backing file or not but the format is probed, which way result
into accessing the backing chain of the probed image.

Prior to this detection, it would be prevented by sVirt or alternatively
also by libvit itself in -blockdev mode when this patch would be
accepted.

Re: [libvirt PATCH 0/2] travis: Use dedicated images for MinGW builds

[Andrea Bolognani]

On Mon, 2020-02-10 at 18:21 +0100, Andrea Bolognani wrote:
> Andrea Bolognani (2):
>   ci: Make container environment available to scripts
>   travis: Use dedicated images for MinGW builds
> 
>  .travis.yml | 10 --
>  ci/Makefile |  2 ++
>  2 files changed, 6 insertions(+), 6 deletions(-)

Pushed now as a CI fix, since the newly-build Fedora 30 image doesn't
contain MinGW packages.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] qemu: use correct backendType when checking memfd capability

[Daniel P . Berrangé]

On Mon, Feb 24, 2020 at 01:34:51PM +0100, Ján Tomko wrote:
> The backend name is memory-backend-memfd but we've been checking
> for memory-backend-memory.
> 
> Reported by GCC on rawhide:
> ../../../src/internal.h:75:22: error: 'strcmp' of a string of length 21 and
> an array of size 21 evaluates to nonzero [-Werror=string-compare]
> ../../../src/qemu/qemu_command.c:3525:20: note: in expansion of macro 'STREQ'
>  3525 | } else if (STREQ(backendType, "memory-backend-memory") &&
>   |^
> 
> Signed-off-by: Ján Tomko 
> Fixes: 24b74d187cab48a9dc9f409ea78900154c709579
> ---
>  src/qemu/qemu_command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[libvirt PATCH] qemu: use correct backendType when checking memfd capability

[Ján Tomko]

The backend name is memory-backend-memfd but we've been checking
for memory-backend-memory.

Reported by GCC on rawhide:
../../../src/internal.h:75:22: error: 'strcmp' of a string of length 21 and
an array of size 21 evaluates to nonzero [-Werror=string-compare]
../../../src/qemu/qemu_command.c:3525:20: note: in expansion of macro 'STREQ'
 3525 | } else if (STREQ(backendType, "memory-backend-memory") &&
  |^

Signed-off-by: Ján Tomko 
Fixes: 24b74d187cab48a9dc9f409ea78900154c709579
---
 src/qemu/qemu_command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f69a9e651c..6d5b53d30a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3522,7 +3522,7 @@ qemuBuildMemoryBackendProps(virJSONValuePtr *backendProps,
_("this qemu doesn't support the "
  "memory-backend-ram object"));
 return -1;
-} else if (STREQ(backendType, "memory-backend-memory") &&
+} else if (STREQ(backendType, "memory-backend-memfd") &&
!virQEMUCapsGet(priv->qemuCaps, 
QEMU_CAPS_OBJECT_MEMORY_MEMFD)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("this qemu doesn't support the "
-- 
2.24.1

Re: [PATCH 0/3] Tighten qemu-img rules on missing backing format

[Peter Krempa]

On Mon, Feb 24, 2020 at 12:01:45 +0100, Peter Krempa wrote:
> On Sat, Feb 22, 2020 at 05:23:38 -0600, Eric Blake wrote:

[...]

> > libvirt HAS to use blockdev-open on the backing chain and supply a
> > backing format there, and thus has to probe images.  If libvirt ever
> > probes differently than qemu, we are back to the potential
> > guest-visible data corruption or potential host CVEs.
> 
> As I've elaborated in [1] I disagree with the host CVE part. The

[1] https://www.redhat.com/archives/libvir-list/2020-February/msg00624.html

> insecure part is not probing the format itself, but probing format AND
> using the backing file of the image if we probed format.

Re: [libvirt RFC PATCH] util: vireventglibwatch: watch for G_IO_HUP and G_IO_ERR

[Daniel P . Berrangé]

On Wed, Feb 19, 2020 at 01:31:22AM +0100, Ján Tomko wrote:
> To more closely match the previous usage in virEventPollDispatchHandles,
> where called the handle callback for any revents returned by poll.
> 
> This should fix the virtlogd error on subsequent domain startup:
>   error: can't connect to virtlogd: Cannot open log file:
>   '/var/log/libvirt/qemu/f28live.log': Device or resource busy
> as well as virtlogd spinning caused by virLogHandlerDomainLogFileEvent
> never being called on hangup.
> 
> Signed-off-by: Ján Tomko 
> Fixes: f8ab47cb4491dd72d866c1a96a9d94b8c3341de9
> Fixes: 946a25274c4646323c62f567ae7e753aa921
> ---
>  src/util/vireventglibwatch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 

> diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> index 7694e74f23..178707f6b7 100644
> --- a/src/util/vireventglibwatch.c
> +++ b/src/util/vireventglibwatch.c
> @@ -89,11 +89,11 @@ GSource *virEventGLibCreateSocketWatch(int fd,
>sizeof(virEventGLibFDSource));
>  ssource = (virEventGLibFDSource *)source;
>  
> -ssource->condition = condition;
> +ssource->condition = condition | G_IO_HUP | G_IO_ERR;
>  ssource->fd = fd;
>  
>  ssource->pollfd.fd = fd;
> -ssource->pollfd.events = condition;
> +ssource->pollfd.events = condition | G_IO_HUP | G_IO_ERR;
>  
>  g_source_add_poll(source, &ssource->pollfd);

This is something I knew about but forgot to fix beforef pushing the
original patches. At the OS level poll()  will always return HUP/ERR
events and we relied on this historically.  GLib uses poll() and so
will also see HUP/ERR events but unless you requested G_IO_ERR/HUP,
the callback will never be invoked to dispatch the event. This is
what leads to the 100% CPU burn behaviour.

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

Re: [PATCH 3/3] qemu-img: Deprecate use of -b without -F

[Peter Krempa]

On Sat, Feb 22, 2020 at 05:23:41 -0600, Eric Blake wrote:
> Creating an image that requires format probing of the backing image is
> inherently unsafe (we've had several CVEs over the years based on
> probes leaking information to the guest on a subsequent boot).  If our
> probing algorithm ever changes, or if other tools like libvirt
> determine a different probe result than we do, then subsequent use of
> that backing file under a different format will present corrupted data
> to the guest.  Start a deprecation clock so that future qemu-img can
> refuse to create unsafe backing chains that would rely on probing.
> 
> However, there is one time where probing is safe: when we first create
> an image, no guest has yet used the new image, so as long as we record
> what we probed, all future uses of the image will see the same data -

I disagree. If you are creating an overlay on top of an existing image
it's not safe to probe the format any more generally. (obviously you'd
have to trust the image and express the trust somehow)

The image may have been used in a VM as raw and that means that the VM
might have recorded a valid qcow2 header into it. Creating the overlay
with probing would legitimize this.

Let's assume we have a malicious image written by the guest but we
simulate it by:

$ qemu-img  create -f qcow2 -F raw -b /etc/passwd /tmp/malicious
Formatting '/tmp/malicious', fmt=qcow2 size=2560 backing_file=/etc/passwd 
backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16


Now we want to create an overlay.

a) without this patchset:

$ qemu-img create -f qcow2 -b /tmp/malicious /tmp/pre-patch.qcow2
Formatting '/tmp/pre-patch.qcow2', fmt=qcow2 size=2560 
backing_file=/tmp/malicious cluster_size=65536 lazy_refcounts=off 
refcount_bits=16
$ qemu-img info /tmp/pre-patch.qcow2
image: /tmp/pre-patch.qcow2
file format: qcow2
virtual size: 2.5 KiB (2560 bytes)
disk size: 196 KiB
cluster_size: 65536
backing file: /tmp/malicious
Format specific information:
compat: 1.1
lazy refcounts: false
refcount bits: 16
corrupt: false

There's no 'backing file format'. When used by libvirt we'd not allow
the VM to touch the backing file of /tmp/malicious in pre-blockdev era
and in libvirt-6.0 we'd report an error right away.

b) Now with this patchset:

$ ./qemu-img create -f qcow2 -b /tmp/malicious /tmp/post-patch.qcow2
qemu-img: warning: Deprecated use of non-raw backing file without explicit 
backing format, using detected format of qcow2
Formatting '/tmp/post-patch.qcow2', fmt=qcow2 size=2560 
backing_file=/tmp/malicious backing_fmt=qcow2 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
$ qemu-img info /tmp/post-patch.qcow2
image: /tmp/post-patch.qcow2
file format: qcow2
virtual size: 2.5 KiB (2560 bytes)
disk size: 196 KiB
cluster_size: 65536
backing file: /tmp/malicious
backing file format: qcow2
Format specific information:
compat: 1.1
lazy refcounts: false
refcount bits: 16
corrupt: false

You now get a warning, but "backing file format" is now recorded in the
overlay. Now this is WAY worse than it was before. The overlay now
legitimizes the format recorded by the malicious guest which circumvents
libvirt's protections. The warning is very easy to miss, and if you run
it in scripts you might never get to see it. We can't allow that.


> so the code now records the probe results as if the user had passed
> -F.  When this happens, it is unconditionally safe to record a probe
> of 'raw', but any other probe is still worth warning the user in case

While it's safe I don't think it should be encouraged. IMO -F should be
made mandatory with -b.

> our probe differed from their expectations.  Similarly, if the backing
> file name uses the json: psuedo-protocol, the backing name includes
> the format.

Not necessarily. The backing store string can be e.g.:

$ ./qemu-img create -f qcow1 -b 
'json:{"driver":"file","filename":"/tmp/malicious"}' /tmp/json.qcow2
Formatting '/tmp/json.qcow1', fmt=qcow2 size=197120 
backing_file=json:{"driver":"file",,"filename":"/tmp/malicious"} 
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img info /tmp/json.qcow1
image: /tmp/json.qcow1
file format: qcow1
virtual size: 191 KiB (197120 bytes)
disk size: 195 KiB
cluster_size: 65535
backing file: json:{"driver":"file","filename":"/tmp/malicious"}
Format specific information:
compat: 0.1
lazy refcounts: false
refcount bits: 15
corrupt: false

Now this has the old semantics but we didn't even get the warning. But
at least the backing file format is not written into the overlay.


> iotest 114 specifically wants to create an unsafe image for later
> amendment rather than defaulting to our new default of recording a
> probed format, so it needs an update.
> 
> Signed-off-by: Eric Blake 
> ---
>  block.c| 17 -
>  qemu-deprecated.texi   | 12 
>  qemu-img.c |  8 +++-
>  tests/qemu-iotests/114 |

[libvirt-dockerfiles PATCH 1/2] Refresh after turning MinGW into a cross-building target

[Andrea Bolognani]

As a result of the change, the regular Fedora 30 container images
no longer include any MinGW-related package.

The corresponding libvirt-jenkins-ci commit is 0f6e1f237d65.

Signed-off-by: Andrea Bolognani 
---
 buildenv-libosinfo-fedora-30.zip | Bin 605 -> 544 bytes
 buildenv-libvirt-fedora-30.zip   | Bin 897 -> 776 bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

diff --git a/buildenv-libosinfo-fedora-30.zip b/buildenv-libosinfo-fedora-30.zip
index 96e195e..69ea788 100644
--- a/buildenv-libosinfo-fedora-30.zip
+++ b/buildenv-libosinfo-fedora-30.zip
@@ -33,18 +33,6 @@ RUN dnf update -y && \
 lsof \
 make \
 meson \
-mingw32-glib2 \
-mingw32-json-glib \
-mingw32-libarchive \
-mingw32-libsoup \
-mingw32-libxml2 \
-mingw32-libxslt \
-mingw64-glib2 \
-mingw64-json-glib \
-mingw64-libarchive \
-mingw64-libsoup \
-mingw64-libxml2 \
-mingw64-libxslt \
 net-tools \
 ninja-build \
 patch \
@@ -60,8 +48,7 @@ RUN dnf update -y && \
 strace \
 sudo \
 vala \
-vim \
-wget && \
+vim && \
 dnf autoremove -y && \
 dnf clean all -y
 
diff --git a/buildenv-libvirt-fedora-30.zip b/buildenv-libvirt-fedora-30.zip
index 800376c..ccc549e 100644
--- a/buildenv-libvirt-fedora-30.zip
+++ b/buildenv-libvirt-fedora-30.zip
@@ -58,32 +58,6 @@ RUN dnf update -y && \
 lvm2 \
 make \
 meson \
-mingw32-curl \
-mingw32-dbus \
-mingw32-dlfcn \
-mingw32-gcc \
-mingw32-gettext \
-mingw32-glib2 \
-mingw32-gnutls \
-mingw32-libssh2 \
-mingw32-libxml2 \
-mingw32-openssl \
-mingw32-pkg-config \
-mingw32-portablexdr \
-mingw32-readline \
-mingw64-curl \
-mingw64-dbus \
-mingw64-dlfcn \
-mingw64-gcc \
-mingw64-gettext \
-mingw64-glib2 \
-mingw64-gnutls \
-mingw64-libssh2 \
-mingw64-libxml2 \
-mingw64-openssl \
-mingw64-pkg-config \
-mingw64-portablexdr \
-mingw64-readline \
 ncurses-devel \
 net-tools \
 netcf-devel \

-- 
2.24.1

[libvirt-dockerfiles PATCH 0/2] Update for MinGW changes

[Andrea Bolognani]

Pushed under the Dockerfile refresh rule.

As usual, these patches cannot be applied to the git repository and
are posted to the list for humans' convenience only.

Andrea Bolognani (2):
  Refresh after turning MinGW into a cross-building target
  Add Dockerfiles for MinGW cross-compilation

 buildenv-libosinfo-fedora-30-cross-mingw32.zip | Bin 0 -> 687 bytes
 buildenv-libosinfo-fedora-30-cross-mingw64.zip | Bin 0 -> 689 bytes
 buildenv-libosinfo-fedora-30.zip   | Bin 605 -> 544 bytes
 buildenv-libvirt-fedora-30-cross-mingw32.zip   | Bin 0 -> 958 bytes
 buildenv-libvirt-fedora-30-cross-mingw64.zip   | Bin 0 -> 960 bytes
 buildenv-libvirt-fedora-30.zip | Bin 897 -> 776 bytes
 6 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 buildenv-libosinfo-fedora-30-cross-mingw32.zip
 create mode 100644 buildenv-libosinfo-fedora-30-cross-mingw64.zip
 create mode 100644 buildenv-libvirt-fedora-30-cross-mingw32.zip
 create mode 100644 buildenv-libvirt-fedora-30-cross-mingw64.zip

-- 
2.24.1

[libvirt-dockerfiles PATCH 2/2] Add Dockerfiles for MinGW cross-compilation

[Andrea Bolognani]

These build upon the Fedora 30 Dockerfiles and add the MinGW
packages on top, ensuring layers are shared.

The corresponding libvirt-jenkins-ci commit is 0f6e1f237d65.

Signed-off-by: Andrea Bolognani 
---
 buildenv-libosinfo-fedora-30-cross-mingw32.zip | Bin 0 -> 687 bytes
 buildenv-libosinfo-fedora-30-cross-mingw64.zip | Bin 0 -> 689 bytes
 buildenv-libvirt-fedora-30-cross-mingw32.zip   | Bin 0 -> 958 bytes
 buildenv-libvirt-fedora-30-cross-mingw64.zip   | Bin 0 -> 960 bytes
 4 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 buildenv-libosinfo-fedora-30-cross-mingw32.zip
 create mode 100644 buildenv-libosinfo-fedora-30-cross-mingw64.zip
 create mode 100644 buildenv-libvirt-fedora-30-cross-mingw32.zip
 create mode 100644 buildenv-libvirt-fedora-30-cross-mingw64.zip

diff --git a/buildenv-libosinfo-fedora-30-cross-mingw32.zip 
b/buildenv-libosinfo-fedora-30-cross-mingw32.zip
new file mode 100644
index 000..f70be8d
--- /dev/null
+++ b/buildenv-libosinfo-fedora-30-cross-mingw32.zip
@@ -0,0 +1,70 @@
+FROM fedora:30
+
+RUN dnf update -y && \
+dnf install -y \
+autoconf \
+automake \
+bash \
+bash-completion \
+ca-certificates \
+ccache \
+check-devel \
+chrony \
+cppi \
+gcc \
+gdb \
+gettext \
+gettext-devel \
+git \
+glib2-devel \
+glibc-devel \
+glibc-langpack-en \
+gobject-introspection-devel \
+gtk-doc \
+hwdata \
+intltool \
+json-glib-devel \
+libarchive-devel \
+libsoup-devel \
+libtool \
+libxml2 \
+libxml2-devel \
+libxslt-devel \
+lsof \
+make \
+meson \
+net-tools \
+ninja-build \
+patch \
+perl \
+pkgconfig \
+python3 \
+python3-lxml \
+python3-pytest \
+python3-requests \
+python3-setuptools \
+rpm-build \
+screen \
+strace \
+sudo \
+vala \
+vim && \
+dnf autoremove -y && \
+dnf clean all -y
+
+RUN dnf install -y \
+mingw32-glib2 \
+mingw32-json-glib \
+mingw32-libarchive \
+mingw32-libsoup \
+mingw32-libxml2 \
+mingw32-libxslt \
+wget && \
+dnf clean all -y
+
+ENV LANG "en_US.UTF-8"
+
+ENV ABI "i686-w64-mingw32"
+ENV CONFIGURE_OPTS "--host=i686-w64-mingw32 \
+--target=i686-w64-mingw32"
+ENV PKG_CONFIG_LIBDIR 
"/usr/i686-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/i686-w64-mingw32/sys-root/mingw/share/pkgconfig"
diff --git a/buildenv-libosinfo-fedora-30-cross-mingw64.zip 
b/buildenv-libosinfo-fedora-30-cross-mingw64.zip
new file mode 100644
index 000..7f75981
--- /dev/null
+++ b/buildenv-libosinfo-fedora-30-cross-mingw64.zip
@@ -0,0 +1,70 @@
+FROM fedora:30
+
+RUN dnf update -y && \
+dnf install -y \
+autoconf \
+automake \
+bash \
+bash-completion \
+ca-certificates \
+ccache \
+check-devel \
+chrony \
+cppi \
+gcc \
+gdb \
+gettext \
+gettext-devel \
+git \
+glib2-devel \
+glibc-devel \
+glibc-langpack-en \
+gobject-introspection-devel \
+gtk-doc \
+hwdata \
+intltool \
+json-glib-devel \
+libarchive-devel \
+libsoup-devel \
+libtool \
+libxml2 \
+libxml2-devel \
+libxslt-devel \
+lsof \
+make \
+meson \
+net-tools \
+ninja-build \
+patch \
+perl \
+pkgconfig \
+python3 \
+python3-lxml \
+python3-pytest \
+python3-requests \
+python3-setuptools \
+rpm-build \
+screen \
+strace \
+sudo \
+vala \
+vim && \
+dnf autoremove -y && \
+dnf clean all -y
+
+RUN dnf install -y \
+mingw64-glib2 \
+mingw64-json-glib \
+mingw64-libarchive \
+mingw64-libsoup \
+mingw64-libxml2 \
+mingw64-libxslt \
+wget && \
+dnf clean all -y
+
+ENV LANG "en_US.UTF-8"
+
+ENV ABI "x86_64-w64-mingw32"
+ENV CONFIGURE_OPTS "--host=x86_64-w64-mingw32 \
+--target=x86_64-w64-mingw32"
+ENV PKG_CONFIG_LIBDIR 
"/usr/x86_64-w64-mingw32/sys-root/mingw/lib/pkgconfig:/usr/x86_64-w64-mingw32/sys-root/mingw/share/pkgconfig"
diff --git a/buildenv-libvirt-fedora-30-cross-mingw32.zip 
b/buildenv-libvirt-fedora-30-cross-mingw32.zip
new file mode 100644
index 000..b933ad3
--- /dev/null
+++ b/buildenv-libvirt-fedora-30-cross-mingw32.zip
@@ -0,0 +1,120 @@
+FROM fedora:30
+
+RUN dnf update -y && \
+dnf install -y \
+audit-libs-devel \
+augeas \
+autoconf \
+automake \
+avahi-devel \
+bash \
+bash-completion \
+ca-certificates \
+ 

Re: [PATCH 0/3] Tighten qemu-img rules on missing backing format

[Peter Krempa]

On Sat, Feb 22, 2020 at 05:23:38 -0600, Eric Blake wrote:
> In the past, we have had CVEs caused by qemu probing one image type
> when an image started out as another but the guest was able to modify
> content.  The solution to those CVEs was to encode backing format
> information into qcow2, to ensure that once we make a decision, we
> don't have to probe any further.  However, we failed to enforce this
> at the time.  And now that libvirt is switching to -blockdev, it has
> come back to bite us: with -block, libvirt had no easy way (other than

s/-block/-drive/ ?

> json:{} pseudoprotocol) to force a backing file, but with -blockdev,

"json:{}" is basically -blockdev with extra steps. Old -drive usage
didn't have any way to do that apart from rewriting the image. Which is
basically the same since json:{} also needs to be recorded in the image
to take effect.

> libvirt HAS to use blockdev-open on the backing chain and supply a
> backing format there, and thus has to probe images.  If libvirt ever
> probes differently than qemu, we are back to the potential
> guest-visible data corruption or potential host CVEs.

As I've elaborated in [1] I disagree with the host CVE part. The
insecure part is not probing the format itself, but probing format AND
using the backing file of the image if we probed format.

I agree that mis-probing format leads to data corruption though.

> It's time to deprecate images without backing formats.  This patch
> series does two things: 1. record an implicit backing format where one
> is learned (although sadly, not all qemu-img commands are able to
> learn a format), 2. warn to the user any time a probe had ambiguous
> results or a backing format is omitted from an image.  All previous
> images without a backing format are still usable, but hopefully the
> warnings (along with libvirt's complaints about images without a
> backing format) help us pinpoint remaining applications that are

It is not a warning in libvirt though. We just refuse it now because we
don't do probing. Previously we allowed qemu to probe the format and the
only thing that prevented host CVEs was if the host used selinux or any
other security approach which would prevent opening the backing file.

> creating images on their own without recording a backing format.

Re: [dockerfiles PATCH] refresh: Drop MinGW hacks

[Andrea Bolognani]

On Mon, 2020-02-10 at 18:19 +0100, Andrea Bolognani wrote:
> Up until now we have had to hardcode some information in our
> refresh script, but with the recent improvements to lcitool that's
> no longer necessary.
> 
> Signed-off-by: Andrea Bolognani 
> ---
> This patch needs
> 
>   https://www.redhat.com/archives/libvir-list/2020-February/msg00409.html
> 
> to be merged into libvirt-jenkins-ci.
> 
>  refresh | 37 -
>  1 file changed, 12 insertions(+), 25 deletions(-)

Now that the libvirt-jenkins-ci changes have been merged, I'll push
this one under the build breaker rule because the refresh script no
longer works.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 00/11] qemu: introduce a per-VM event loop thread

[Daniel P . Berrangé]

On Sat, Feb 22, 2020 at 01:41:06PM +0100, Michal Prívozník wrote:
> On 2/14/20 1:51 PM, Daniel P. Berrangé wrote:
> > This series changes the way we manage the QEMU monitor and
> > QEMU agent, such that all I/O is processed by a dedicated
> > event loop thread.
> > 
> > Many times in the past years people are reported issues
> > where long running monitor event callbacks block the main
> > libvirtd event loop for an unacceptably long period of
> > time. In the best case, this delays other work being
> > completed, but in bad cases it leads to mgmt app failures
> > when keepalive times trigger a client disconnect.
> > 
> > With this series, when we spawn QEMU, we also spawn a
> > dedicated thread running a GMainLoop instance. Then QEMU
> > monitor and QEMU agent UNIX sockets are switched to use
> > GMainContext for events instead of the traditional libvirt
> > event loop APIs. We kill off the event thread when we see
> > EOF on the QEMU monitor during shutdown.
> > 
> > The cost of this approach is one extra thread per VM,
> > which incurs a new OS process and a new stack allocation.
> > 
> > The QEMU driver already delegates some QMP event handling
> > to a thread pool for certain types of event. This was a
> > previous hack to mitigate the impact on the main event
> > loop. It is likely that we can remove this thread pool
> > from the QEMU driver & rely on the per-VM event threads
> > to do all the work. This will, however, require careful
> > analysis of each handler we pushed into the thread pool
> > to make sure its work doesn't have a dependency on the
> > event loop running in parallel.
> > 
> > This should also eliminate the need to have the libvirt
> > event loop registered when using the embedded QEMU driver.
> > This has not yet been validated, however, so it is left
> > for a future patch to relax the constraint.
> > 
> > Daniel P. Berrangé (11):
> >   qemu: drop support for agent connections on PTYs
> >   qemu: drop ability to open monitor from FD
> >   src: set the OS level thread name
> >   src: improve thread naming with human targetted names
> >   src: introduce an abstraction for running event loops
> >   qemu: start/stop an event loop thread for domains
> >   qemu: start/stop an event thread for QMP probing
> >   tests: start/stop an event thread for QEMU monitor/agent tests
> >   qemu: convert monitor to use the per-VM event loop
> >   qemu: fix variable naming in agent code
> >   qemu: convert agent to use the per-VM event loop
> > 
> >  po/POTFILES.in  |   1 +
> >  src/libvirt_private.syms|   6 +
> >  src/libxl/libxl_domain.c|  10 +-
> >  src/libxl/libxl_migration.c |  23 +-
> >  src/lxc/lxc_fuse.c  |   4 +-
> >  src/node_device/node_device_udev.c  |   7 +-
> >  src/nwfilter/nwfilter_dhcpsnoop.c   |  11 +-
> >  src/nwfilter/nwfilter_learnipaddr.c |  10 +-
> >  src/qemu/qemu_agent.c   | 634 ++--
> >  src/qemu/qemu_agent.h   |   1 +
> >  src/qemu/qemu_domain.c  |  33 ++
> >  src/qemu/qemu_domain.h  |   6 +
> >  src/qemu/qemu_driver.c  |   3 +-
> >  src/qemu/qemu_migration.c   |   8 +-
> >  src/qemu/qemu_monitor.c | 155 +++
> >  src/qemu/qemu_monitor.h |   8 +-
> >  src/qemu/qemu_process.c |  61 ++-
> >  src/qemu/qemu_process.h |   2 +
> >  src/remote/remote_daemon.c  |   9 +-
> >  src/rpc/virnetserver.c  |   9 +-
> >  src/storage/storage_backend_scsi.c  |   4 +-
> >  src/storage/storage_driver.c|   4 +-
> >  src/util/Makefile.inc.am|   2 +
> >  src/util/vircommand.c   |   5 +-
> >  src/util/vireventthread.c   | 175 
> >  src/util/vireventthread.h   |  31 ++
> >  src/util/virfdstream.c  |  10 +-
> >  src/util/virnodesuspend.c   |   8 +-
> >  src/util/virthread.c|  44 +-
> >  src/util/virthread.h|   4 +-
> >  src/util/virthreadpool.c|  14 +-
> >  src/util/virthreadpool.h|   2 +-
> >  tests/qemumonitortestutils.c|  15 +
> >  33 files changed, 832 insertions(+), 487 deletions(-)
> >  create mode 100644 src/util/vireventthread.c
> >  create mode 100644 src/util/vireventthread.h
> > 
> 
> I've ACKed the first four patches which can be pushed independently. Do
> you want me to continue or will you resend v2 starting from patch 05/11?

I'll send a new series with the remaining patches.


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

Re: [libvirt PATCH 4/8] conf: rename virNetDevSupportBandwidth to virNetDevSupportsBandwidth

[Pavel Mores]

On Sat, Feb 22, 2020 at 05:31:55PM +0100, Ján Tomko wrote:
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/netdev_bandwidth_conf.c | 2 +-
>  src/conf/netdev_bandwidth_conf.h | 2 +-
>  src/lxc/lxc_driver.c | 4 ++--
>  src/lxc/lxc_process.c| 2 +-
>  src/qemu/qemu_command.c  | 2 +-
>  src/qemu/qemu_driver.c   | 4 ++--
>  src/qemu/qemu_hotplug.c  | 4 ++--
>  7 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/conf/netdev_bandwidth_conf.c 
> b/src/conf/netdev_bandwidth_conf.c
> index 5cbb9f46e4..349b451e41 100644
> --- a/src/conf/netdev_bandwidth_conf.c
> +++ b/src/conf/netdev_bandwidth_conf.c
> @@ -293,7 +293,7 @@ virDomainClearNetBandwidth(virDomainObjPtr vm)
>  for (i = 0; i < vm->def->nnets; i++) {
>  type = virDomainNetGetActualType(vm->def->nets[i]);
>  if (virDomainNetGetActualBandwidth(vm->def->nets[i]) &&
> -virNetDevSupportBandwidth(type))
> +virNetDevSupportsBandwidth(type))
>  virNetDevBandwidthClear(vm->def->nets[i]->ifname);
>  }
>  }
> diff --git a/src/conf/netdev_bandwidth_conf.h 
> b/src/conf/netdev_bandwidth_conf.h
> index b9e57168be..172f9a50f9 100644
> --- a/src/conf/netdev_bandwidth_conf.h
> +++ b/src/conf/netdev_bandwidth_conf.h
> @@ -37,7 +37,7 @@ int virNetDevBandwidthFormat(const virNetDevBandwidth *def,
>  void virDomainClearNetBandwidth(virDomainObjPtr vm)
>  ATTRIBUTE_NONNULL(1);
>  
> -static inline bool virNetDevSupportBandwidth(virDomainNetType type)
> +static inline bool virNetDevSupportsBandwidth(virDomainNetType type)
>  {
>  switch (type) {
>  case VIR_DOMAIN_NET_TYPE_BRIDGE:
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index f7376188f0..f01c71f9e2 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -3920,7 +3920,7 @@ lxcDomainAttachDeviceNetLive(virLXCDriverPtr driver,
>  /* Set bandwidth or warn if requested and not supported. */
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
>  if (actualBandwidth) {
> -if (virNetDevSupportBandwidth(actualType)) {
> +if (virNetDevSupportsBandwidth(actualType)) {
>  if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
>!virDomainNetTypeSharesHostView(net)) 
> < 0)
>  goto cleanup;
> @@ -4377,7 +4377,7 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
>  
>  /* clear network bandwidth */
>  if (virDomainNetGetActualBandwidth(detach) &&
> -virNetDevSupportBandwidth(actualType) &&
> +virNetDevSupportsBandwidth(actualType) &&
>  virNetDevBandwidthClear(detach->ifname))
>  goto cleanup;
>  
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 6851b3e3e2..d05304dd8f 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -621,7 +621,7 @@ virLXCProcessSetupInterfaces(virLXCDriverPtr driver,
>  /* Set bandwidth or warn if requested and not supported. */
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
>  if (actualBandwidth) {
> -if (virNetDevSupportBandwidth(type)) {
> +if (virNetDevSupportsBandwidth(type)) {
>  if (virNetDevBandwidthSet(net->ifname, actualBandwidth, 
> false,
>
> !virDomainNetTypeSharesHostView(net)) < 0)
>  goto cleanup;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f69a9e651c..c44f50b2a8 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8080,7 +8080,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver,
>  /* Set bandwidth or warn if requested and not supported. */
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
>  if (actualBandwidth) {
> -if (virNetDevSupportBandwidth(actualType)) {
> +if (virNetDevSupportsBandwidth(actualType)) {
>  if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
>!virDomainNetTypeSharesHostView(net)) 
> < 0)
>  goto cleanup;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 39e1f044e0..0faf79e0d1 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11594,12 +11594,12 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
>  
>  if (net) {
>  actualType = virDomainNetGetActualType(net);
> -qosSupported = virNetDevSupportBandwidth(actualType);
> +qosSupported = virNetDevSupportsBandwidth(actualType);
>  }
>  
>  if (qosSupported && persistentNet) {
>  actualType = virDomainNetGetActualType(persistentNet);
> -qosSupported = virNetDevSupportBandwidth(actualType);
> +qosSupported = virNetDevSupportsBandwidth(actualType);
>  }
>  
>  if (!qosSupported) {
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 9800491755..

Re: [libvirt PATCH] esx: Same order of arguments in definition and declaration

[Ján Tomko]

On Sun, Feb 23, 2020 at 12:22:25AM +0100, Rikard Falkeborn wrote:

The order of arguments were not the same in the definition and
declaration. All callers use the same order as the definition, so there
is no bug, but change the function declaration to match the
implementation to avoid confusion.



Did you find it visually, or using cppcheck? :)


Signed-off-by: Rikard Falkeborn 
---
src/esx/esx_vi.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)



Reviewed-by: Ján Tomko 

and pushed. Congrats on your first patch!

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] vz: Fix return value in error path

[Ján Tomko]

On Sun, Feb 23, 2020 at 12:22:47AM +0100, Rikard Falkeborn wrote:

If PrlVmDev_GetType(), PrlVmDev_GetIndex() or PrlVmCfg_GetBootDevCount()
fails, return false to indicate error. Returning -1 would be interpreted
as true when used in an if-statement.

Fixes: 8c9252aa6d95247537da0939b54fdd2f31695e32
Signed-off-by: Rikard Falkeborn 
---
src/vz/vz_sdk.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 
and pushed

Jano


signature.asc
Description: PGP signature

[libvirt PATCH] vz: Fix return value in error path

[Rikard Falkeborn]

If PrlVmDev_GetType(), PrlVmDev_GetIndex() or PrlVmCfg_GetBootDevCount()
fails, return false to indicate error. Returning -1 would be interpreted
as true when used in an if-statement.

Fixes: 8c9252aa6d95247537da0939b54fdd2f31695e32
Signed-off-by: Rikard Falkeborn 
---
 src/vz/vz_sdk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 877692aeba..2c68c7cb27 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1609,13 +1609,13 @@ prlsdkInBootList(PRL_HANDLE sdkdom,
 size_t i;
 
 pret = PrlVmDev_GetType(sdktargetdev, &targetType);
-prlsdkCheckRetExit(pret, -1);
+prlsdkCheckRetExit(pret, false);
 
 pret = PrlVmDev_GetIndex(sdktargetdev, &targetIndex);
-prlsdkCheckRetExit(pret, -1);
+prlsdkCheckRetExit(pret, false);
 
 pret = PrlVmCfg_GetBootDevCount(sdkdom, &bootNum);
-prlsdkCheckRetExit(pret, -1);
+prlsdkCheckRetExit(pret, false);
 
 for (i = 0; i < bootNum; ++i) {
 pret = PrlVmCfg_GetBootDev(sdkdom, i, &bootDev);
-- 
2.25.1

[libvirt PATCH] esx: Same order of arguments in definition and declaration

[Rikard Falkeborn]

The order of arguments were not the same in the definition and
declaration. All callers use the same order as the definition, so there
is no bug, but change the function declaration to match the
implementation to avoid confusion.

Signed-off-by: Rikard Falkeborn 
---
 src/esx/esx_vi.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/esx/esx_vi.h b/src/esx/esx_vi.h
index 5c60fd58f4..b960c0900a 100644
--- a/src/esx/esx_vi.h
+++ b/src/esx/esx_vi.h
@@ -204,8 +204,8 @@ struct _esxVI_Context {
 
 int esxVI_Context_Alloc(esxVI_Context **ctx);
 void esxVI_Context_Free(esxVI_Context **ctx);
-int esxVI_Context_Connect(esxVI_Context *ctx, const char *ipAddress,
-  const char *url, const char *username,
+int esxVI_Context_Connect(esxVI_Context *ctx, const char *url,
+  const char *ipAddress, const char *username,
   const char *password, esxUtil_ParsedUri *parsedUri);
 int esxVI_Context_LookupManagedObjects(esxVI_Context *ctx);
 int esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char 
*path);
-- 
2.25.1

Re: Requesting Guidance

[Michal Privoznik]

On 2/23/20 7:28 AM, Ritish kr singh wrote:

Hello, Sir



Hey!

Firstly, I suggest going through our GSoC FAQ [1]. Our student selection 
process is documented there.
Secondly, we should start simple, by fixing a small bug, or extending 
functionality a bit so that we get to know libvirt internals. There is a 
list of small tasks for newcomers linked from [1] but I'll link it here 
too [2].


In order to send a patch, you will need to read contributor guidelines 
[3] which describe patch writing step by step.


If you have any questions, I'm happy to help.

Michal


1: https://wiki.libvirt.org/page/Google_Summer_of_Code_FAQ
2: https://wiki.libvirt.org/page/BiteSizedTasks
3: https://libvirt.org/hacking.html