[PATCH] docs: point out that locals should be defined at the top of a block of code

[Laine Stump]

Although we have nothing in make syntax-check to enforce this, and
apparently there are places where it isn't the case (according to
Dan), we should discourage the practice of defining new variables in
the middle of a block of code.

https://www.redhat.com/archives/libvir-list/2020-July/msg00433.html
Signed-off-by: Laine Stump 
---
 docs/coding-style.rst | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/docs/coding-style.rst b/docs/coding-style.rst
index 03b89c86e5..b9b4a16987 100644
--- a/docs/coding-style.rst
+++ b/docs/coding-style.rst
@@ -541,6 +541,44 @@ diligent about this, when you see a non-const pointer, 
you're
 guaranteed that it is used to modify the storage it points to, or
 it is aliased to another pointer that is.
 
+Defining Local Variables
+
+
+Always define local variables at the top of the block in which they
+are used (before any pure code). Although modern C compilers allow
+defining a local variable in the middle of a block of code, this
+practice can lead to bugs, and must be avoided in all libvirt
+code. (As indicated in these examples, it is okay to initialize
+variables where they are defined, even if the initialization involves
+calling another function.)
+
+::
+
+  GOOD:
+int
+Bob(char *loblaw)
+{
+int x;
+int y = lawBlog(loblaw);
+char *z = NULL;
+
+x = y + 20;
+...
+}
+
+  BAD:
+int
+Bob(char *loblaw)
+{
+int x;
+int y = lawBlog(loblaw);
+
+x = y + 20;
+
+char *z = NULL; <===
+...
+}
+
 Attribute annotations
 -
 
-- 
2.25.4

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Jim Mattson]

On Thu, Jul 9, 2020 at 2:44 AM Gerd Hoffmann  wrote:

> (2) GUEST_MAXPHYADDR < HOST_MAXPHYADDR
>
> Mostly fine.  Some edge cases, like different page fault errors for
> addresses above GUEST_MAXPHYADDR and below HOST_MAXPHYADDR.  Which I
> think Mohammed fixed in the kernel recently.

Doesn't this require intercepting MOV-to-CR3 when the guest is in PAE
mode, so that the hypervisor can validate the high bits in the PDPTEs?

[PATCH v5 1/3] qemu: Move setting of TPM default to post parse function

[Stefan Berger]

From: Stefan Berger 

Move setting the TPM default version out of the validation function into
the post parse function.

Signed-off-by: Stefan Berger 
Reviewed-by: Peter Krempa 
Reviewed-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain.c   | 7 ---
 src/qemu/qemu_validate.c | 4 
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 42cc78ac1b..f916d840e2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4347,12 +4347,13 @@ qemuDomainDefTPMsPostParse(virDomainDefPtr def)
 virDomainTPMDefPtr regularTPM = NULL;
 size_t i;
 
-if (def->ntpms < 2)
-return 0;
-
 for (i = 0; i < def->ntpms; i++) {
 virDomainTPMDefPtr tpm = def->tpms[i];
 
+/* TPM 1.2 and 2 are not compatible, so we choose a specific version 
here */
+if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT)
+tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
+
 if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) {
 if (proxyTPM) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index bd7590a00a..d130b52bf2 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3644,10 +3644,6 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
 {
 virQEMUCapsFlags flag;
 
-/* TPM 1.2 and 2 are not compatible, so we choose a specific version here 
*/
-if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT)
-tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
-
 switch (tpm->version) {
 case VIR_DOMAIN_TPM_VERSION_1_2:
 /* TPM 1.2 + CRB do not work */
-- 
2.17.1

[PATCH v5 0/3] tpm: Fix default choices for CRB and SPAPR dev models

[Stefan Berger]

From: Stefan Berger 

This series of patches adds an additional check for the SPAPR device model
that prevents the choice of a TPM 1.2 backend and chooses a TPM 2 as default.
Also CRB now chooses a TPM 2 as default since TPM 1.2 wouldn't work with it,
either.

   Stefan

v4->v5:
  - Added R-b's

Stefan Berger (3):
  qemu: Move setting of TPM default to post parse function
  qemu: Set SPAPR TPM default to 2.0 and prevent 1.2 choice
  qemu: Choose TPM 2 for backend as default for CRB interface

 src/qemu/qemu_domain.c   | 12 +---
 src/qemu/qemu_validate.c | 10 ++
 2 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v5 3/3] qemu: Choose TPM 2 for backend as default for CRB interface

[Stefan Berger]

From: Stefan Berger 

Choose a TPM 2 device for the backend as default for the CRB interface
since TPM 1.2 would not work.

This patch addresses BZ 1781913: 
https://bugzilla.redhat.com/show_bug.cgi?id=1781913

Signed-off-by: Stefan Berger 
Reviewed-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b0f5e17613..161421b602 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4352,7 +4352,8 @@ qemuDomainDefTPMsPostParse(virDomainDefPtr def)
 
 /* TPM 1.2 and 2 are not compatible, so we choose a specific version 
here */
 if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) {
-if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR)
+if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR ||
+tpm->model == VIR_DOMAIN_TPM_MODEL_CRB)
 tpm->version = VIR_DOMAIN_TPM_VERSION_2_0;
 else
 tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
-- 
2.17.1

[PATCH v5 2/3] qemu: Set SPAPR TPM default to 2.0 and prevent 1.2 choice

[Stefan Berger]

From: Stefan Berger 

The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so
prevent the choice of TPM 1.2 when the SPAPR device model is chosen
and use a default of '2.0' (TPM 2) for the backend.

This patch addresses BZ 1781913: 
https://bugzilla.redhat.com/show_bug.cgi?id=1781913

Signed-off-by: Stefan Berger 
Reviewed-by: Peter Krempa 
Reviewed-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain.c   | 8 ++--
 src/qemu/qemu_validate.c | 6 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f916d840e2..b0f5e17613 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4351,8 +4351,12 @@ qemuDomainDefTPMsPostParse(virDomainDefPtr def)
 virDomainTPMDefPtr tpm = def->tpms[i];
 
 /* TPM 1.2 and 2 are not compatible, so we choose a specific version 
here */
-if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT)
-tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
+if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) {
+if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR)
+tpm->version = VIR_DOMAIN_TPM_VERSION_2_0;
+else
+tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
+}
 
 if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR_PROXY) {
 if (proxyTPM) {
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index d130b52bf2..488f258d00 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3654,6 +3654,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
virDomainTPMModelTypeToString(tpm->model));
 return -1;
 }
+/* TPM 1.2 + SPAPR do not work with any 'type' (backend) */
+if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("TPM 1.2 is not supported with the SPAPR device 
model"));
+return -1;
+}
 break;
 case VIR_DOMAIN_TPM_VERSION_2_0:
 case VIR_DOMAIN_TPM_VERSION_DEFAULT:
-- 
2.17.1

Re: Setting 'nodatacow' on VM image when on Btrfs

[Chris Murphy]

On Thu, Jul 9, 2020 at 12:27 PM Daniel P. Berrangé  wrote:
>
> On Thu, Jul 09, 2020 at 08:30:14AM -0600, Chris Murphy wrote:
> > It's generally recommended by upstream Btrfs development to set
> > 'nodatacow' using 'chattr +C' on the containing directory for VM
> > images. By setting it on the containing directory it means new VM
> > images inherit the attribute, including images copied to this
> > location.
> >
> > But 'nodatacow' also implies no compression and no data checksums.
> > Could there be use cases in which it's preferred to do compression and
> > checksumming on VM images? In that case the fragmentation problem can
> > be mitigated by periodic defragmentation.
>
> Setting nodatacow makes sense particularly when using qcow2, since
> qcow2 is already potentially performing copy-on-write.
>
> Skipping compression and data checksums is reasonable in many cases,
> as the OS inside the VM is often going to be able todo this itself,
> so we don't want double checksums or double compression as it just
> wastes performance.

Good point. I am open to suggestion/recommendation by Felipe Borges
about GNOME Boxes' ability to do installs by injecting kickstarts. It
might sound nutty but it's quite sane to consider the guest do
something like plain XFS (no LVM). But all of my VM's are as you
describe: guest is btrfs with the checksums and compression, on a raw
file with chattr +C set.

>
> > Is this something libvirt can and should do? Possibly by default with
> > a way for users to opt out?
>
> The default /var/lib/libvirt/images directory is created by RPM
> at install time. Not sure if there's a way to get RPM to set
> attributes on the dir at this time ?

For lives it's rsync today.  I'm not certain if rsync carries over
file attributes. tar does not. Also not sure if squashfs and
unsquashfs do either. So this might mean an anaconda post-install
script is a more reliable way to go, since Anaconda can support rsync
(Live) and rpm (netinstall,dvd) installs. And there is a proposal
dangling in the wind to use plain squashfs (no nested ext4 as today).

>
> Libvirt's storage pool APIs has support for setting "nodatacow"
> on a per-file basis when creating the images. This was added for
> btrfs benefit, but this is opt in and I'm not sure any mgmt tools
> actually use this right now. So in practice it probably dooesn't
> have any out of the box benefit.
>
> The storage pool APIs don't have any feature to set nodatacow
> for the directory as a whole, but probably we should add this.

systemd-journald checks for btrfs and sets +C on /var/log/journal if
it is. This can be inhibited by 'touch
/etc/tmpfiles.d/journal-nocow.conf'


>
> > Another option is to have the installer set 'chattr +C' in the short
> > term. But this doesn't help GNOME Boxes, since the user home isn't
> > created at installation time.
> >
> > Three advantages of libvirt awareness of Btrfs:
> >
> > (a) GNOME Boxes Cockpit, and other users of libvirt can then use this
> > same mechanism, and apply to their VM image locations.
> >
> > (b) Create the containing directory as a subvolume instead of a directory
> > (1) btrfs snapshots are not recursive, therefore making this a
> > subvolume would prevent it from being snapshot, and thus (temporarily)
> > resuming datacow.
> > (2) in heavy workloads there can be lock contention on file
> > btrees; a subvolume is a dedicated file btree; so this would reduce
> > tendency for lock contention in heavy workloads (this is not likely a
> > desktop/laptop use case)
>
> Being able to create subvolumes sounds like a reasonable idea. We already
> have a ZFS specific storage driver that can do the ZFS equivalent.
>
> Again though we'll also need mgmt tools modified to take advantage of
> this. Not sure how we would make this all work out of the box, with
> the way we let RPM pre-create /var/lib/libvirt/images, as we'd need
> different behaviour depending on what filesystem you install the RPM
> onto.

It seems reasonable to me that libvirtd can "own"
/var/lib/libvirt/images and make these decisions. i.e. if it's empty,
and if btrfs, then delete it and recreate as subvolume and also chattr
+C

There's also precedent by systemd to create its own nested subvolumes
for containers run by nspawn.


Thanks,


-- 
Chris Murphy

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Eduardo Habkost]

On Thu, Jul 09, 2020 at 10:00:59AM -0700, Jim Mattson wrote:
> On Thu, Jul 9, 2020 at 2:44 AM Gerd Hoffmann  wrote:
> 
> > (2) GUEST_MAXPHYADDR < HOST_MAXPHYADDR
> >
> > Mostly fine.  Some edge cases, like different page fault errors for
> > addresses above GUEST_MAXPHYADDR and below HOST_MAXPHYADDR.  Which I
> > think Mohammed fixed in the kernel recently.
> 
> Doesn't this require intercepting MOV-to-CR3 when the guest is in PAE
> mode, so that the hypervisor can validate the high bits in the PDPTEs?

If the fix has additional overhead, is the additional overhead
bad enough to warrant making it optional?  Most existing
GUEST_MAXPHYADDR < HOST_MAXPHYADDR guests already work today
without the fix.

-- 
Eduardo

[libvirt PATCH 7/9] remote: introduce virtd-nc helper binary

[Daniel P . Berrangé]

When accessing libvirtd over a SSH tunnel, the remote driver must spawn
the remote 'nc' process, pointing it to the libvirtd socket path. This
is problematic for a number of reasons:

 - The socket path varies according to the --prefix chosen at build
   time. The remote client is seeing the local prefix, but what we
   need is the remote prefix

 - The socket path varies according to remote env variables, such as
   the XDG_RUNTIME_DIR location. Again we see the local XDG_RUNTIME_DIR
   value, but what we need is the remote value (if any)

 - We can not able to autospawn the libvirtd daemon for session mode
   access

To address these problems this patch introduces the 'virtd-nc' helper
program which takes the URI for the remote driver as a CLI parameter.
It then figures out the socket path to connect to using the same
code as the remote driver does on the remote host.

Signed-off-by: Daniel P. Berrangé 
---
 build-aux/syntax-check.mk  |   2 +-
 po/POTFILES.in |   1 +
 src/remote/Makefile.inc.am |  30 +++
 src/remote/remote_nc.c | 424 +
 src/rpc/virnetsocket.h |   1 +
 5 files changed, 457 insertions(+), 1 deletion(-)
 create mode 100644 src/remote/remote_nc.c

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index d47a92b530..81b307ebe8 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -1967,7 +1967,7 @@ group-qemu-caps:
 # List all syntax-check exemptions:
 exclude_file_name_regexp--sc_avoid_strcase = ^tools/vsh\.h$$
 
-_src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon
+_src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon|remote/remote_nc
 
_test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock|commandhelper
 exclude_file_name_regexp--sc_avoid_write = \
   ^(src/($(_src1))|tools/virsh-console|tests/($(_test1)))\.c$$
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 8fd391a63a..8fa47ec276 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -180,6 +180,7 @@
 @SRCDIR@/src/remote/remote_daemon_dispatch.c
 @SRCDIR@/src/remote/remote_daemon_stream.c
 @SRCDIR@/src/remote/remote_driver.c
+@SRCDIR@/src/remote/remote_nc.c
 @SRCDIR@/src/remote/remote_sockets.c
 @SRCDIR@/src/rpc/virkeepalive.c
 @SRCDIR@/src/rpc/virnetclient.c
diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index 0ae97f4107..2527cc193f 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -221,6 +221,8 @@ if WITH_LIBVIRTD
 
 sbin_PROGRAMS += libvirtd virtproxyd
 
+libexec_PROGRAMS += virt-nc
+
 augeas_DATA += \
remote/libvirtd.aug \
remote/virtproxyd.aug \
@@ -286,6 +288,34 @@ remote/virtproxyd.conf: remote/libvirtd.conf.in
-e 's/[@]DAEMON_NAME[@]/virtproxyd/' \
$< > $@
 
+virt_nc_SOURCES = \
+   remote/remote_sockets.h \
+   remote/remote_sockets.c \
+   remote/remote_nc.c \
+   $(NULL)
+
+virt_nc_CFLAGS = \
+   $(LIBXML_CFLAGS) \
+   $(GLIB_CFLAGS) \
+   $(WARN_CFLAGS) \
+   $(PIE_CFLAGS) \
+   -I$(srcdir)/access \
+   -I$(srcdir)/rpc \
+   $(NULL)
+
+virt_nc_LDFLAGS = \
+   $(RELRO_LDFLAGS) \
+   $(PIE_LDFLAGS) \
+   $(NO_INDIRECT_LDFLAGS) \
+   $(NO_UNDEFINED_LDFLAGS) \
+   $(NULL)
+
+virt_nc_LDADD = \
+   libvirt.la \
+   $(LIBXML_LIBS) \
+   $(NULL)
+
+
 INSTALL_DATA_DIRS += remote
 
 install-data-remote:
diff --git a/src/remote/remote_nc.c b/src/remote/remote_nc.c
new file mode 100644
index 00..d304db1a04
--- /dev/null
+++ b/src/remote/remote_nc.c
@@ -0,0 +1,424 @@
+/*
+ * remote_nc.c: a netcat equivalent for remote driver tunnelling
+ *
+ * Copyright (C) 2020 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+
+#include "virnetsocket.h"
+#include "viralloc.h"
+#include "virlog.h"
+#include "virgettext.h"
+#include "virfile.h"
+
+#include "remote_sockets.h"
+
+#define VIR_FROM_THIS VIR_FROM_REMOTE
+
+VIR_LOG_INIT("remote.remote_nc");
+
+struct virRemoteProxyBuffer {
+size_t length;
+size_t offset;
+char *data;
+};
+
+typedef struct virRemoteProxy virRemoteProxy;
+typedef 

[libvirt PATCH 6/9] remote: extract logic for determining daemon to connect to

[Daniel P . Berrangé]

We'll shortly want to reuse code for determining whether to connect to
the system or session daemon from places outside the remote driver
client. Pulling it out into a self contained function facilitates reuse.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c  | 51 
 src/remote/remote_sockets.c | 59 +
 src/remote/remote_sockets.h |  6 
 3 files changed, 71 insertions(+), 45 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c7fd24625e..c2dcf20f91 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1198,7 +1198,8 @@ remoteConnectOpen(virConnectPtr conn,
 struct private_data *priv;
 int ret = VIR_DRV_OPEN_ERROR;
 int rflags = 0;
-const char *autostart = getenv("LIBVIRT_AUTOSTART");
+bool user;
+bool autostart;
 char *driver = NULL;
 remoteDriverTransport transport;
 
@@ -1233,51 +1234,11 @@ remoteConnectOpen(virConnectPtr conn,
 if (flags & VIR_CONNECT_RO)
 rflags |= VIR_DRV_OPEN_REMOTE_RO;
 
-/*
- * User session daemon is used for
- *
- *  - Any URI with /session suffix
- *  - Test driver, if a protocol is given
- *
- * provided we are running non-root
- */
-if (conn->uri &&
-conn->uri->path &&
-conn->uri->scheme &&
-(STREQ(conn->uri->path, "/session") ||
- STRPREFIX(conn->uri->scheme, "test+")) &&
-geteuid() > 0) {
-VIR_DEBUG("User session daemon required");
+remoteGetURIDaemonInfo(conn->uri, transport, , );
+if (user)
 rflags |= VIR_DRV_OPEN_REMOTE_USER;
-
-/*
- * Furthermore if no servername is given,
- * and the transport is unix,
- * and uid is unprivileged then auto-spawn a daemon.
- */
-if (!conn->uri->server &&
-(transport == REMOTE_DRIVER_TRANSPORT_UNIX) &&
-(!autostart ||
- STRNEQ(autostart, "0"))) {
-VIR_DEBUG("Try daemon autostart");
-rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART;
-}
-}
-
-/*
- * If URI is NULL, then do a UNIX connection possibly auto-spawning
- * unprivileged server and probe remote server for URI.
- */
-if (!conn->uri) {
-VIR_DEBUG("Auto-probe remote URI");
-if (geteuid() > 0) {
-VIR_DEBUG("Auto-spawn user daemon instance");
-rflags |= VIR_DRV_OPEN_REMOTE_USER;
-if (!autostart ||
-STRNEQ(autostart, "0"))
-rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART;
-}
-}
+if (autostart)
+rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART;
 
 ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags);
 if (ret != VIR_DRV_OPEN_SUCCESS) {
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 28e02e24d5..854775f401 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -224,3 +224,62 @@ remoteGetUNIXSocket(remoteDriverTransport transport,
   ro, session);
 return sock_name;
 }
+
+
+void
+remoteGetURIDaemonInfo(virURIPtr uri,
+   remoteDriverTransport transport,
+   bool *session,
+   bool *autostart)
+{
+const char *autostart_str = getenv("LIBVIRT_AUTOSTART");
+
+*session = false;
+*autostart = false;
+
+/*
+ * User session daemon is used for
+ *
+ *  - Any URI with /session suffix
+ *  - Test driver, if a protocol is given
+ *
+ * provided we are running non-root
+ */
+if (uri &&
+uri->path &&
+uri->scheme &&
+(STREQ(uri->path, "/session") ||
+ STRPREFIX(uri->scheme, "test+")) &&
+geteuid() > 0) {
+VIR_DEBUG("User session daemon required");
+*session = true;
+
+/*
+ * Furthermore if no servername is given,
+ * and the transport is unix,
+ * and uid is unprivileged then auto-spawn a daemon.
+ */
+if (!uri->server &&
+(transport == REMOTE_DRIVER_TRANSPORT_UNIX) &&
+(!autostart_str ||
+ STRNEQ(autostart_str, "0"))) {
+VIR_DEBUG("Try daemon autostart");
+*autostart = true;
+}
+}
+
+/*
+ * If URI is NULL, then do a UNIX connection possibly auto-spawning
+ * unprivileged server and probe remote server for URI.
+ */
+if (!uri) {
+VIR_DEBUG("Auto-probe remote URI");
+if (geteuid() > 0) {
+VIR_DEBUG("Auto-spawn user daemon instance");
+*session = true;
+if (!autostart_str ||
+STRNEQ(autostart_str, "0"))
+*autostart = true;
+}
+}
+}
diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h
index 64055f3d44..7526752835 100644
--- a/src/remote/remote_sockets.h
+++ b/src/remote/remote_sockets.h
@@ -62,3 

[libvirt PATCH 9/9] rpc: use new virt-nc binary for remote tunnelling

[Daniel P . Berrangé]

This wires up support for using the new virt-nc binary with the ssh,
libssh and libssh2 protocols.

The new binary will be used preferentially if it is available in $PATH,
otherwise we fall back to traditional netcat.

The "proxy" URI parameter can be used to force use of netcat e.g.

  qemu+ssh://host/system?proxy=netcat

or the disable fallback e.g.

  qemu+ssh://host/system?proxy=virt-nc

With use of virt-nc, we can now support remote session URIs

  qemu+ssh://host/session

and this will only use virt-nc, with no fallback. This also lets the
libvirtd process be auto-started.

Signed-off-by: Daniel P. Berrangé 
---
 docs/uri.html.in| 18 ++
 src/remote/remote_driver.c  | 30 +++-
 src/remote/remote_sockets.c |  8 -
 src/rpc/virnetclient.c  | 70 ++---
 src/rpc/virnetclient.h  | 30 +---
 tests/virnetsockettest.c|  7 ++--
 6 files changed, 136 insertions(+), 27 deletions(-)

diff --git a/docs/uri.html.in b/docs/uri.html.in
index 49f92773f8..5311579273 100644
--- a/docs/uri.html.in
+++ b/docs/uri.html.in
@@ -259,6 +259,24 @@ Note that parameter values must be
 
  Example: mode=direct 
   
+  
+
+  proxy
+
+auto, virt, generic 
+
+  
+autotry virt-nc, fallback to netcat
+netcatonly use netcat
+virt-nconly use virt-nc
+  
+  Can also be set in libvirt.conf as 
remote_proxy
+
+  
+  
+
+ Example: proxy=virt-nc 
+  
   
 
   command
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c1f7a45aab..83789a86a9 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -761,12 +761,14 @@ doRemoteOpen(virConnectPtr conn,
 g_autofree char *knownHosts = NULL;
 g_autofree char *mode_str = NULL;
 g_autofree char *daemon_name = NULL;
+g_autofree char *proxy_str = NULL;
 bool sanity = true;
 bool verify = true;
 #ifndef WIN32
 bool tty = true;
 #endif
 int mode;
+int proxy;
 
 if (inside_daemon && !conn->uri->server) {
 mode = REMOTE_DRIVER_MODE_DIRECT;
@@ -774,6 +776,14 @@ doRemoteOpen(virConnectPtr conn,
 mode = REMOTE_DRIVER_MODE_AUTO;
 }
 
+/* Historically we didn't allow ssh tunnel with session mode,
+ * since we can't construct the accurate path remotely,
+ * so we can default to modern virt-nc */
+if (flags & VIR_DRV_OPEN_REMOTE_USER)
+proxy = VIR_NET_CLIENT_PROXY_VIRT_NC;
+else
+proxy = VIR_NET_CLIENT_PROXY_NETCAT;
+
 /* We handle *ALL* URIs here. The caller has rejected any
  * URIs we don't care about */
 
@@ -813,6 +823,7 @@ doRemoteOpen(virConnectPtr conn,
 EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify);
 EXTRACT_URI_ARG_STR("tls_priority", tls_priority);
 EXTRACT_URI_ARG_STR("mode", mode_str);
+EXTRACT_URI_ARG_STR("proxy", proxy_str);
 EXTRACT_URI_ARG_BOOL("no_sanity", sanity);
 EXTRACT_URI_ARG_BOOL("no_verify", verify);
 #ifndef WIN32
@@ -865,6 +876,14 @@ doRemoteOpen(virConnectPtr conn,
 (mode = remoteDriverModeTypeFromString(mode_str)) < 0)
 goto failed;
 
+if (conf && !proxy_str &&
+virConfGetValueString(conf, "remote_proxy", _str) < 0)
+goto failed;
+
+if (proxy_str &&
+(proxy = virNetClientProxyTypeFromString(proxy_str)) < 0)
+goto failed;
+
 /* Sanity check that nothing requested !direct mode by mistake */
 if (inside_daemon && !conn->uri->server && mode != 
REMOTE_DRIVER_MODE_DIRECT) {
 virReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -949,8 +968,11 @@ doRemoteOpen(virConnectPtr conn,
   knownHosts,
   knownHostsVerify,
   sshauth,
+  proxy,
   netcat,
   sockname,
+  name,
+  flags & VIR_DRV_OPEN_REMOTE_RO,
   auth,
   conn->uri);
 if (!priv->client)
@@ -970,8 +992,11 @@ doRemoteOpen(virConnectPtr conn,
  knownHosts,
  knownHostsVerify,
  sshauth,
+ proxy,
  netcat,
  sockname,
+ name,
+ flags & VIR_DRV_OPEN_REMOTE_RO,
  

[libvirt PATCH 1/9] rpc: merge logic for generating remote SSH shell script

[Daniel P . Berrangé]

Three parts of the code all build up the same SSH shell script
snippet for remote tunneling the RPC protocol, but in slightly
different ways. Combine them all into one helper method in the
virNetClient code, since this logic doesn't really belong in
the virNetSocket code.

Note that the this change means the shell snippet is passed to
the SSH binary as a single arg, instead of three separate args,
but this is functionally identical, as the three separate args
were combined into one already when passed to the remote system.

Signed-off-by: Daniel P. Berrangé 
---
 src/libvirt_remote.syms  |   1 +
 src/rpc/virnetclient.c   | 105 +--
 src/rpc/virnetclient.h   |   3 ++
 src/rpc/virnetsocket.c   |  37 +-
 src/rpc/virnetsocket.h   |   3 +-
 tests/virnetsockettest.c |   9 +++-
 6 files changed, 70 insertions(+), 88 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0018a0c41d..0b00bce1fa 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -42,6 +42,7 @@ virNetClientSendStream;
 virNetClientSendWithReply;
 virNetClientSetCloseCallback;
 virNetClientSetTLSSession;
+virNetClientSSHHelperCommand;
 
 
 # rpc/virnetclientprogram.h
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 1c5bef86a1..aee2b52bf6 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -391,28 +391,75 @@ virNetClientPtr virNetClientNewTCP(const char *nodename,
 return virNetClientNew(sock, nodename);
 }
 
+
+/*
+ * The SSH Server uses shell to spawn the command we give
+ * it.  Our command then invokes shell again. Thus we need
+ * to apply two levels of escaping, so that commands with
+ * whitespace in their path get correctly interpreted.
+ */
+static char *
+virNetClientDoubleEscapeShell(const char *str)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+g_autofree char *tmp = NULL;
+
+virBufferEscapeShell(, str);
+
+tmp = virBufferContentAndReset();
+
+virBufferEscapeShell(, tmp);
+
+return virBufferContentAndReset();
+}
+
+char *
+virNetClientSSHHelperCommand(const char *netcatPath,
+ const char *socketPath)
+{
+g_autofree char *netcatPathSafe = 
virNetClientDoubleEscapeShell(netcatPath);
+
+return g_strdup_printf(
+"sh -c "
+"'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; 
then "
+  "ARG=-q0;"
+"else "
+  "ARG=;"
+"fi;"
+"'%s' $ARG -U %s'",
+netcatPathSafe, netcatPathSafe, socketPath);
+}
+
+
+#define DEFAULT_VALUE(VAR, VAL) \
+if (!VAR) \
+VAR = VAL;
+
 virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *service,
const char *binary,
const char *username,
bool noTTY,
bool noVerify,
-   const char *netcat,
+   const char *netcatPath,
const char *keyfile,
-   const char *path)
+   const char *socketPath)
 {
 virNetSocketPtr sock;
 
+g_autofree char *command = NULL;
+
+DEFAULT_VALUE(netcatPath, "nc");
+
+command = virNetClientSSHHelperCommand(netcatPath, socketPath);
+
 if (virNetSocketNewConnectSSH(nodename, service, binary, username, noTTY,
-  noVerify, netcat, keyfile, path, ) < 0)
+  noVerify, keyfile, command, ) < 0)
 return NULL;
 
 return virNetClientNew(sock, NULL);
 }
 
-#define DEFAULT_VALUE(VAR, VAL) \
-if (!VAR) \
-VAR = VAL;
 virNetClientPtr virNetClientNewLibSSH2(const char *host,
const char *port,
int family,
@@ -428,8 +475,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 {
 virNetSocketPtr sock = NULL;
 
-virBuffer buf = VIR_BUFFER_INITIALIZER;
-g_autofree char *nc = NULL;
 g_autofree char *command = NULL;
 
 g_autofree char *homedir = NULL;
@@ -442,9 +487,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 knownhosts = g_strdup(knownHostsPath);
 } else {
 confdir = virGetUserConfigDirectory();
-virBufferAsprintf(, "%s/known_hosts", confdir);
-if (!(knownhosts = virBufferContentAndReset()))
-return NULL;
+knownhosts = g_strdup_printf("%s/known_hosts", confdir);
 }
 
 if (privkeyPath) {
@@ -468,26 +511,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 DEFAULT_VALUE(netcatPath, "nc");
 DEFAULT_VALUE(knownHostsVerify, "normal");
 
-virBufferEscapeShell(, netcatPath);
-if (!(nc = virBufferContentAndReset()))
-return NULL;
-virBufferEscapeShell(, nc);
-VIR_FREE(nc);
-  

[libvirt PATCH 4/9] remote: parse the remote transport string earlier

[Daniel P . Berrangé]

We delay converting the remote transport string to enum form until
fairly late. As a result we're doing string comparisons when we
could be just doing enum comparisons.

Signed-off-by: Daniel P. Berrangé 
---
 po/POTFILES.in  |  1 +
 src/remote/remote_driver.c  | 51 ++---
 src/remote/remote_sockets.c | 35 +
 src/remote/remote_sockets.h |  2 +-
 4 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index af52054aa4..8fd391a63a 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -180,6 +180,7 @@
 @SRCDIR@/src/remote/remote_daemon_dispatch.c
 @SRCDIR@/src/remote/remote_daemon_stream.c
 @SRCDIR@/src/remote/remote_driver.c
+@SRCDIR@/src/remote/remote_sockets.c
 @SRCDIR@/src/rpc/virkeepalive.c
 @SRCDIR@/src/rpc/virnetclient.c
 @SRCDIR@/src/rpc/virnetclientprogram.c
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b84b72522a..c39085951e 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -863,12 +863,11 @@ static int
 doRemoteOpen(virConnectPtr conn,
  struct private_data *priv,
  const char *driver_str,
- const char *transport_str,
+ remoteDriverTransport transport,
  virConnectAuthPtr auth G_GNUC_UNUSED,
  virConfPtr conf,
  unsigned int flags)
 {
-int transport;
 #ifndef WIN32
 g_autofree char *daemonPath = NULL;
 #endif
@@ -903,34 +902,6 @@ doRemoteOpen(virConnectPtr conn,
 /* We handle *ALL* URIs here. The caller has rejected any
  * URIs we don't care about */
 
-if (conn->uri) {
-if (!transport_str) {
-if (conn->uri->server)
-transport = REMOTE_DRIVER_TRANSPORT_TLS;
-else
-transport = REMOTE_DRIVER_TRANSPORT_UNIX;
-} else {
-if ((transport = 
remoteDriverTransportTypeFromString(transport_str)) < 0) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
-   _("remote_open: transport in URL not recognised 
"
- "(should be 
tls|unix|ssh|ext|tcp|libssh2|libssh)"));
-return VIR_DRV_OPEN_ERROR;
-}
-
-if (transport == REMOTE_DRIVER_TRANSPORT_UNIX &&
-conn->uri->server) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("using unix socket and remote "
- "server '%s' is not supported."),
-   conn->uri->server);
-return VIR_DRV_OPEN_ERROR;
-}
-}
-} else {
-/* No URI, then must be probing so use UNIX socket */
-transport = REMOTE_DRIVER_TRANSPORT_UNIX;
-}
-
 /* Remote server defaults to "localhost" if not specified. */
 if (conn->uri && conn->uri->port != 0) {
 port = g_strdup_printf("%d", conn->uri->port);
@@ -1352,11 +1323,16 @@ remoteConnectOpen(virConnectPtr conn,
 int rflags = 0;
 const char *autostart = getenv("LIBVIRT_AUTOSTART");
 char *driver = NULL;
-char *transport = NULL;
+remoteDriverTransport transport;
+
+if (conn->uri) {
+if (remoteSplitURIScheme(conn->uri, , ) < 0)
+goto cleanup;
+} else {
+/* No URI, then must be probing so use UNIX socket */
+transport = REMOTE_DRIVER_TRANSPORT_UNIX;
+}
 
-if (conn->uri &&
-remoteSplitURIScheme(conn->uri, , ) < 0)
-goto cleanup;
 
 if (inside_daemon) {
 if (!conn->uri) {
@@ -1398,12 +1374,12 @@ remoteConnectOpen(virConnectPtr conn,
 rflags |= VIR_DRV_OPEN_REMOTE_USER;
 
 /*
- * Furthermore if no servername is given, and no +XXX
- * transport is listed, or transport is unix,
+ * Furthermore if no servername is given,
+ * and the transport is unix,
  * and uid is unprivileged then auto-spawn a daemon.
  */
 if (!conn->uri->server &&
-(transport == NULL || STREQ(transport, "unix")) &&
+(transport == REMOTE_DRIVER_TRANSPORT_UNIX) &&
 (!autostart ||
  STRNEQ(autostart, "0"))) {
 VIR_DEBUG("Try daemon autostart");
@@ -1438,7 +1414,6 @@ remoteConnectOpen(virConnectPtr conn,
 
  cleanup:
 VIR_FREE(driver);
-VIR_FREE(transport);
 return ret;
 }
 
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 976124d0ed..cdc0a00293 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -21,6 +21,9 @@
 #include 
 
 #include "remote_sockets.h"
+#include "virerror.h"
+
+#define VIR_FROM_THIS VIR_FROM_REMOTE
 
 VIR_ENUM_IMPL(remoteDriverTransport,
   REMOTE_DRIVER_TRANSPORT_LAST,
@@ -42,25 +45,47 @@ VIR_ENUM_IMPL(remoteDriverMode,
 int
 remoteSplitURIScheme(virURIPtr uri,
  char **driver,
- char **transport)
+  

[libvirt PATCH 3/9] remote: split out function for parsing URI scheme

[Daniel P . Berrangé]

The remoteSplitURISCheme method will be needed by source files beyond
the remote driver client.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c  | 25 -
 src/remote/remote_sockets.c | 28 
 src/remote/remote_sockets.h |  6 ++
 3 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 880fce6e62..b84b72522a 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -165,31 +165,6 @@ static void 
make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho
 /*--*/
 
 /* Helper functions for remoteOpen. */
-static int remoteSplitURIScheme(virURIPtr uri,
-char **driver,
-char **transport)
-{
-char *p = strchr(uri->scheme, '+');
-
-*driver = *transport = NULL;
-
-if (p)
-*driver = g_strndup(uri->scheme, p - uri->scheme);
-else
-*driver = g_strdup(uri->scheme);
-
-if (p) {
-*transport = g_strdup(p + 1);
-
-p = *transport;
-while (*p) {
-*p = g_ascii_tolower(*p);
-p++;
-}
-}
-
-return 0;
-}
 
 
 static int
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 0662cbad14..976124d0ed 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -37,3 +37,31 @@ VIR_ENUM_IMPL(remoteDriverMode,
   "auto",
   "legacy",
   "direct");
+
+
+int
+remoteSplitURIScheme(virURIPtr uri,
+ char **driver,
+ char **transport)
+{
+char *p = strchr(uri->scheme, '+');
+
+*driver = *transport = NULL;
+
+if (p)
+*driver = g_strndup(uri->scheme, p - uri->scheme);
+else
+*driver = g_strdup(uri->scheme);
+
+if (p) {
+*transport = g_strdup(p + 1);
+
+p = *transport;
+while (*p) {
+*p = g_ascii_tolower(*p);
+p++;
+}
+}
+
+return 0;
+}
diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h
index 1d4ae3f9c1..bef3cdada9 100644
--- a/src/remote/remote_sockets.h
+++ b/src/remote/remote_sockets.h
@@ -21,6 +21,7 @@
 #pragma once
 
 #include "virenum.h"
+#include "viruri.h"
 
 typedef enum {
 REMOTE_DRIVER_TRANSPORT_TLS,
@@ -48,3 +49,8 @@ typedef enum {
 } remoteDriverMode;
 
 VIR_ENUM_DECL(remoteDriverMode);
+
+int
+remoteSplitURIScheme(virURIPtr uri,
+ char **driver,
+ char **transport);
-- 
2.26.2

[libvirt PATCH 0/9] remote: introduce a custom netcat impl for ssh tunnelling

[Daniel P . Berrangé]

We have long had a problem with use of netcat for ssh tunnelling because
there's no guarantee the UNIX socket path the client builds will match
the UNIX socket path the remote host uses. We don't even allow session
mode SSH tunnelling for this reason. We also can't easily auto-spawn
libvirtd in session mode.

With the introduction of modular daemons we also have potential for two
completely different UNIX socket paths even for system mode, and the
client can't know which to use.

The solution to all these problems is to introduce a custom netcat impl.
Instead passing the UNIX socket path, we pass the libvirt driver URI.
The custom netcat then decides which socket path to use based on the
remote build host environment.

We still have to support netcat for interoperability with legacy libvirt
versions, but we can default to the new virt-nc.

Daniel P. Berrangé (9):
  rpc: merge logic for generating remote SSH shell script
  remote: split off enums into separate source file
  remote: split out function for parsing URI scheme
  remote: parse the remote transport string earlier
  remote: split out function for constructing socket path
  remote: extract logic for determining daemon to connect to
  remote: introduce virtd-nc helper binary
  rpc: switch order of args in virNetClientNewSSH
  rpc: use new virt-nc binary for remote tunnelling

 build-aux/syntax-check.mk   |   2 +-
 docs/uri.html.in|  18 ++
 po/POTFILES.in  |   2 +
 src/libvirt_remote.syms |   1 +
 src/remote/Makefile.inc.am  |  32 +++
 src/remote/remote_driver.c  | 323 +--
 src/remote/remote_nc.c  | 424 
 src/remote/remote_sockets.c | 277 +++
 src/remote/remote_sockets.h |  70 ++
 src/rpc/virnetclient.c  | 151 -
 src/rpc/virnetclient.h  |  29 ++-
 src/rpc/virnetsocket.c  |  37 +---
 src/rpc/virnetsocket.h  |   4 +-
 tests/virnetsockettest.c|  12 +-
 14 files changed, 1018 insertions(+), 364 deletions(-)
 create mode 100644 src/remote/remote_nc.c
 create mode 100644 src/remote/remote_sockets.c
 create mode 100644 src/remote/remote_sockets.h

-- 
2.26.2

Re: libvirt opens kernel+initrd in read-write mode

[Olaf Hering]

Am Thu, 9 Jul 2020 19:00:18 +0200
schrieb Michal Privoznik :

> do you see an actual libvirt error? I think this may come from 
> secdrivers trying to remember the original owner of kernel/initrd files.

Jul 09 16:10:42 libvirtd[5741]: internal error: child reported (status=125): 
unable to open /path/to/initrd: Read-only file system
Jul 09 16:10:42 libvirtd[5741]: unable to open /path/to/initrd: Read-only file 
system
Jul 09 16:10:42 libvirtd[5741]: internal error: child reported (status=125): 
unable to open /path/to/initrd: Read-only file system
Jul 09 16:10:42 libvirtd[5741]: unable to open /path/to/initrd: Read-only file 
system

> If you set remember_owner=0 in /etc/libvirt/qemu.conf (and restart 
> libvirtd) then does it fix your problem?

Yes, this helps as a workaround.


Olaf


pgpCLjoBxfESK.pgp
Description: Digitale Signatur von OpenPGP

[libvirt PATCH 8/9] rpc: switch order of args in virNetClientNewSSH

[Daniel P . Berrangé]

Switch keyfile and netcat parameters, since the netcat path and
socket path are a logical pair that belong together. This patches
the other constructors.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c | 2 +-
 src/rpc/virnetclient.c | 2 +-
 src/rpc/virnetclient.h | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c2dcf20f91..c1f7a45aab 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1010,8 +1010,8 @@ doRemoteOpen(virConnectPtr conn,
 username,
 !tty,
 !verify,
-netcat ? netcat : "nc",
 keyfile,
+netcat ? netcat : "nc",
 sockname)))
 goto failed;
 
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index aee2b52bf6..cd1bcc3ab3 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -441,8 +441,8 @@ virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *username,
bool noTTY,
bool noVerify,
-   const char *netcatPath,
const char *keyfile,
+   const char *netcatPath,
const char *socketPath)
 {
 virNetSocketPtr sock;
diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
index 0005de46f3..6fdc370083 100644
--- a/src/rpc/virnetclient.h
+++ b/src/rpc/virnetclient.h
@@ -48,9 +48,9 @@ virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *username,
bool noTTY,
bool noVerify,
-   const char *netcat,
const char *keyfile,
-   const char *path);
+   const char *netcat,
+   const char *socketPath);
 
 virNetClientPtr virNetClientNewLibSSH2(const char *host,
const char *port,
-- 
2.26.2

[libvirt PATCH 5/9] remote: split out function for constructing socket path

[Daniel P . Berrangé]

The remoteGetUNIXSocketHelper method will be needed by source files
beyond the remote driver client.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/remote_driver.c  | 129 +-
 src/remote/remote_sockets.c | 134 
 src/remote/remote_sockets.h |   8 +++
 3 files changed, 145 insertions(+), 126 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index c39085951e..c7fd24625e 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -697,131 +697,6 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn,
 }
 
 
-static char *
-remoteGetUNIXSocketHelper(remoteDriverTransport transport,
-  const char *sock_prefix,
-  unsigned int flags)
-{
-char *sockname = NULL;
-g_autofree char *userdir = NULL;
-
-if (flags & VIR_DRV_OPEN_REMOTE_USER) {
-if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("Connecting to session instance without "
- "socket path is not supported by the %s "
- "transport"),
-   remoteDriverTransportTypeToString(transport));
-return NULL;
-}
-userdir = virGetUserRuntimeDirectory();
-
-sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix);
-} else {
-/* Intentionally do *NOT* use RUNSTATEDIR here. We might
- * be connecting to a remote machine, and cannot assume
- * the remote host has /run. The converse is ok though,
- * any machine with /run will have a /var/run symlink.
- * The portable option is to thus use $LOCALSTATEDIR/run
- */
-sockname = g_strdup_printf("%s/run/libvirt/%s-%s", LOCALSTATEDIR,
-   sock_prefix,
-   flags & VIR_DRV_OPEN_REMOTE_RO ? "sock-ro" 
: "sock");
-}
-
-VIR_DEBUG("Built UNIX sockname %s for transport %s prefix %s flags=0x%x",
-  sockname, remoteDriverTransportTypeToString(transport),
-  sock_prefix, flags);
-return sockname;
-}
-
-
-static char *
-remoteGetUNIXSocket(remoteDriverTransport transport,
-remoteDriverMode mode,
-const char *driver,
-char **daemon,
-unsigned int flags)
-{
-char *sock_name = NULL;
-g_autofree char *direct_daemon = NULL;
-g_autofree char *legacy_daemon = NULL;
-g_autofree char *direct_sock_name = NULL;
-g_autofree char *legacy_sock_name = NULL;
-
-if (driver)
-direct_daemon = g_strdup_printf("virt%sd", driver);
-
-legacy_daemon = g_strdup("libvirtd");
-
-if (driver &&
-!(direct_sock_name = remoteGetUNIXSocketHelper(transport, 
direct_daemon, flags)))
-return NULL;
-
-if (!(legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", 
flags)))
-return NULL;
-
-if (mode == REMOTE_DRIVER_MODE_AUTO) {
-if (transport == REMOTE_DRIVER_TRANSPORT_UNIX) {
-if (direct_sock_name && virFileExists(direct_sock_name)) {
-mode = REMOTE_DRIVER_MODE_DIRECT;
-} else if (virFileExists(legacy_sock_name)) {
-mode = REMOTE_DRIVER_MODE_LEGACY;
-} else if (driver) {
-/*
- * This constant comes from the configure script and
- * maps to either the direct or legacy mode constant
- */
-mode = REMOTE_DRIVER_MODE_DEFAULT;
-} else {
-mode = REMOTE_DRIVER_MODE_LEGACY;
-}
-} else {
-mode = REMOTE_DRIVER_MODE_LEGACY;
-}
-}
-
-switch ((remoteDriverMode)mode) {
-case REMOTE_DRIVER_MODE_LEGACY:
-sock_name = g_steal_pointer(_sock_name);
-*daemon = g_steal_pointer(_daemon);
-break;
-
-case REMOTE_DRIVER_MODE_DIRECT:
-if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-   _("Cannot use direct socket mode for %s transport"),
-   remoteDriverTransportTypeToString(transport));
-return NULL;
-}
-
-if (!direct_sock_name) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("Cannot use direct socket mode if no URI is 
set"));
-return NULL;
-}
-
-sock_name = g_steal_pointer(_sock_name);
-*daemon = g_steal_pointer(_daemon);
-break;
-
-case REMOTE_DRIVER_MODE_AUTO:
-case REMOTE_DRIVER_MODE_LAST:
-default:
-virReportEnumRangeError(remoteDriverMode, mode);
-return NULL;
-}
-
-VIR_DEBUG("Chosen UNIX sockname %s daemon %s "
-  "for mode %s transport %s 

[libvirt PATCH 2/9] remote: split off enums into separate source file

[Daniel P . Berrangé]

The remoteDriverTransport and remoteDriverMode enums are going to be
needed by source files beyond the remote driver client.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/Makefile.inc.am  |  2 ++
 src/remote/remote_driver.c  | 41 +-
 src/remote/remote_sockets.c | 39 +
 src/remote/remote_sockets.h | 50 +
 4 files changed, 92 insertions(+), 40 deletions(-)
 create mode 100644 src/remote/remote_sockets.c
 create mode 100644 src/remote/remote_sockets.h

diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index 1b1be8340d..0ae97f4107 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -13,6 +13,8 @@ REMOTE_DRIVER_GENERATED = \
$(NULL)
 
 REMOTE_DRIVER_SOURCES = \
+   remote/remote_sockets.c \
+   remote/remote_sockets.h \
remote/remote_driver.c \
remote/remote_driver.h \
$(NULL)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 653c68472a..880fce6e62 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -38,6 +38,7 @@
 #include "virbuffer.h"
 #include "remote_driver.h"
 #include "remote_protocol.h"
+#include "remote_sockets.h"
 #include "lxc_protocol.h"
 #include "qemu_protocol.h"
 #include "viralloc.h"
@@ -54,46 +55,6 @@
 
 VIR_LOG_INIT("remote.remote_driver");
 
-typedef enum {
-REMOTE_DRIVER_TRANSPORT_TLS,
-REMOTE_DRIVER_TRANSPORT_UNIX,
-REMOTE_DRIVER_TRANSPORT_SSH,
-REMOTE_DRIVER_TRANSPORT_LIBSSH2,
-REMOTE_DRIVER_TRANSPORT_EXT,
-REMOTE_DRIVER_TRANSPORT_TCP,
-REMOTE_DRIVER_TRANSPORT_LIBSSH,
-
-REMOTE_DRIVER_TRANSPORT_LAST,
-} remoteDriverTransport;
-
-VIR_ENUM_DECL(remoteDriverTransport);
-VIR_ENUM_IMPL(remoteDriverTransport,
-  REMOTE_DRIVER_TRANSPORT_LAST,
-  "tls",
-  "unix",
-  "ssh",
-  "libssh2",
-  "ext",
-  "tcp",
-  "libssh");
-
-typedef enum {
-/* Try to figure out the "best" choice magically */
-REMOTE_DRIVER_MODE_AUTO,
-/* Always use the legacy libvirtd */
-REMOTE_DRIVER_MODE_LEGACY,
-/* Always use the per-driver virt*d daemons */
-REMOTE_DRIVER_MODE_DIRECT,
-
-REMOTE_DRIVER_MODE_LAST
-} remoteDriverMode;
-
-VIR_ENUM_DECL(remoteDriverMode);
-VIR_ENUM_IMPL(remoteDriverMode,
-  REMOTE_DRIVER_MODE_LAST,
-  "auto",
-  "legacy",
-  "direct");
 
 #if SIZEOF_LONG < 8
 # define HYPER_TO_TYPE(_type, _to, _from) \
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
new file mode 100644
index 00..0662cbad14
--- /dev/null
+++ b/src/remote/remote_sockets.c
@@ -0,0 +1,39 @@
+/*
+ * remote_sockets.c: helpers for getting remote driver socket paths
+ *
+ * Copyright (C) 2007-2019 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "remote_sockets.h"
+
+VIR_ENUM_IMPL(remoteDriverTransport,
+  REMOTE_DRIVER_TRANSPORT_LAST,
+  "tls",
+  "unix",
+  "ssh",
+  "libssh2",
+  "ext",
+  "tcp",
+  "libssh");
+
+VIR_ENUM_IMPL(remoteDriverMode,
+  REMOTE_DRIVER_MODE_LAST,
+  "auto",
+  "legacy",
+  "direct");
diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h
new file mode 100644
index 00..1d4ae3f9c1
--- /dev/null
+++ b/src/remote/remote_sockets.h
@@ -0,0 +1,50 @@
+/*
+ * remote_sockets.h: helpers for getting remote driver socket paths
+ *
+ * Copyright (C) 2007-2020 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * 

Re: Setting 'nodatacow' on VM image when on Btrfs

[Daniel P . Berrangé]

On Thu, Jul 09, 2020 at 08:30:14AM -0600, Chris Murphy wrote:
> It's generally recommended by upstream Btrfs development to set
> 'nodatacow' using 'chattr +C' on the containing directory for VM
> images. By setting it on the containing directory it means new VM
> images inherit the attribute, including images copied to this
> location.
> 
> But 'nodatacow' also implies no compression and no data checksums.
> Could there be use cases in which it's preferred to do compression and
> checksumming on VM images? In that case the fragmentation problem can
> be mitigated by periodic defragmentation.

Setting nodatacow makes sense particularly when using qcow2, since
qcow2 is already potentially performing copy-on-write.

Skipping compression and data checksums is reasonable in many cases,
as the OS inside the VM is often going to be able todo this itself,
so we don't want double checksums or double compression as it just
wastes performance.

> Is this something libvirt can and should do? Possibly by default with
> a way for users to opt out?

The default /var/lib/libvirt/images directory is created by RPM
at install time. Not sure if there's a way to get RPM to set
attributes on the dir at this time ?

Libvirt's storage pool APIs has support for setting "nodatacow"
on a per-file basis when creating the images. This was added for
btrfs benefit, but this is opt in and I'm not sure any mgmt tools
actually use this right now. So in practice it probably dooesn't
have any out of the box benefit.

The storage pool APIs don't have any feature to set nodatacow
for the directory as a whole, but probably we should add this.

> Another option is to have the installer set 'chattr +C' in the short
> term. But this doesn't help GNOME Boxes, since the user home isn't
> created at installation time.
> 
> Three advantages of libvirt awareness of Btrfs:
> 
> (a) GNOME Boxes Cockpit, and other users of libvirt can then use this
> same mechanism, and apply to their VM image locations.
> 
> (b) Create the containing directory as a subvolume instead of a directory
> (1) btrfs snapshots are not recursive, therefore making this a
> subvolume would prevent it from being snapshot, and thus (temporarily)
> resuming datacow.
> (2) in heavy workloads there can be lock contention on file
> btrees; a subvolume is a dedicated file btree; so this would reduce
> tendency for lock contention in heavy workloads (this is not likely a
> desktop/laptop use case)

Being able to create subvolumes sounds like a reasonable idea. We already
have a ZFS specific storage driver that can do the ZFS equivalent.

Again though we'll also need mgmt tools modified to take advantage of
this. Not sure how we would make this all work out of the box, with
the way we let RPM pre-create /var/lib/libvirt/images, as we'd need
different behaviour depending on what filesystem you install the RPM
onto.

> (c) virtiofs might be able to take advantage of btrfs subvolumes.

Libvirt doesn't currently do anything much wrt virtiofs except
configure QEMU.  The creation of the directory containing the
share and populating its contents is left as an exercise for the
user/admin/mgmt tool.

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] All pointers to virXMLPropString() use g_autofree.

[Nicolás Brignone]

Ack, Thanks!

On Thu, Jul 9, 2020 at 2:48 PM Daniel P. Berrangé  wrote:
>
> On Thu, Jul 09, 2020 at 02:40:34PM -0300, Nicolás Brignone wrote:
> > On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé  
> > wrote:
> > >
> > > On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> > > > On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > > > >   All pointers to virXMLPropString() use g_autofree.
> > > >
> > > >
> > > > I changed the summary line like this, to be more precise:
> > > >
> > > >
> > > > conf: use g_autofree for all pointers to virXMLPropString() in 
> > > > device_conf.c
> > > >
> > > >
> > > > > All modified functions are similar, in all cases "cleanup" label is 
> > > > > removed,
> > > > > along with all the "goto" calls.
> > > >
> > > >
> > > > I've been advised in the recent past to put the g_autofree additions and
> > > > corresponding removals of free functions into one patch, then do the 
> > > > removal
> > > > of the extra labels (in favor of directly returning) in a separate 
> > > > patch to
> > > > make it easier to hand-verify / review. Here's a couple recent examples:
> > > >
> > > >
> > > > https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> > > >
> > > >
> > > > In your case the changes are few enough that I'm okay with it a single
> > > > patch, except...
> > > >
> > > >
> > > > >
> > > > > Signed-off-by: Nicolas Brignone 
> > > > > ---
> > > > >   src/conf/device_conf.c | 183 
> > > > > +
> > > > >   1 file changed, 56 insertions(+), 127 deletions(-)
> > > > >
> > > > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > > > > index 7d48a3f..9fa6141 100644
> > > > > --- a/src/conf/device_conf.c
> > > > > +++ b/src/conf/device_conf.c
> > > > > @@ -208,45 +208,43 @@ int
> > > > >   virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > > >   virPCIDeviceAddressPtr addr)
> > > > >   {
> > > > > -char *domain, *slot, *bus, *function, *multi;
> > > > >   xmlNodePtr cur;
> > > > >   xmlNodePtr zpci = NULL;
> > > > > -int ret = -1;
> > > > >   memset(addr, 0, sizeof(*addr));
> > > > > -domain   = virXMLPropString(node, "domain");
> > > > > -bus  = virXMLPropString(node, "bus");
> > > > > -slot = virXMLPropString(node, "slot");
> > > > > -function = virXMLPropString(node, "function");
> > > > > -multi= virXMLPropString(node, "multifunction");
> > > > > +g_autofree char *domain   = virXMLPropString(node, "domain");
> > > > > +g_autofree char *bus  = virXMLPropString(node, "bus");
> > > > > +g_autofree char *slot = virXMLPropString(node, "slot");
> > > > > +g_autofree char *function = virXMLPropString(node, "function");
> > > > > +g_autofree char *multi= virXMLPropString(node, 
> > > > > "multifunction");
> > > >
> > > >
> > > > ... you've modified it so that local variables are being declared 
> > > > *below* a
> > > > line of executable code rather than at the top of the block. Although I
> > > > don't see anything in the coding style document
> > > > (https://libvirt.org/coding-style.html) about it, and it may just be
> > > > leftover from a time when we supported a compiler that required all
> > > > declarations to be at top of scope, I think pretty much all of libvirts 
> > > > code
> > > > declares all local variables at the top of the scope.
> > >
> > > We should really document it, and even enforce it at compile time.
> >
> > I think you are asking for enabling:
> >
> > -Wdeclaration-after-statement (C and Objective-C only)
> >Warn when a declaration is found after a statement in a
> > block.  This construct, known from C++, was introduced
> >with ISO C99 and is by default allowed in GCC.  It is not
> > supported by ISO C90.
>
> Yes, but
>
> > Will take a look.
>
> ..don't spend your time on enabling -Wdeclaration-after-statement unless
> you are strongly motivated to do a lot of tedious work. I took at stab at
> it about 12 months ago, but didn't complete it as there are a surprising
> amount of subtle edge cases to deal with, so I never got as far as sending
> a patch proposal.
>
> FWIW, WIP is here
>
>https://gitlab.com/berrange/libvirt/-/tree/c99-vars
>
> but this is based against ancient version, not current
> git master, so I doubt it will rebase cleanly at all
>
> 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] All pointers to virXMLPropString() use g_autofree.

[Daniel P . Berrangé]

On Thu, Jul 09, 2020 at 02:40:34PM -0300, Nicolás Brignone wrote:
> On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé  wrote:
> >
> > On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> > > On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > > >   All pointers to virXMLPropString() use g_autofree.
> > >
> > >
> > > I changed the summary line like this, to be more precise:
> > >
> > >
> > > conf: use g_autofree for all pointers to virXMLPropString() in 
> > > device_conf.c
> > >
> > >
> > > > All modified functions are similar, in all cases "cleanup" label is 
> > > > removed,
> > > > along with all the "goto" calls.
> > >
> > >
> > > I've been advised in the recent past to put the g_autofree additions and
> > > corresponding removals of free functions into one patch, then do the 
> > > removal
> > > of the extra labels (in favor of directly returning) in a separate patch 
> > > to
> > > make it easier to hand-verify / review. Here's a couple recent examples:
> > >
> > >
> > > https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> > >
> > >
> > > In your case the changes are few enough that I'm okay with it a single
> > > patch, except...
> > >
> > >
> > > >
> > > > Signed-off-by: Nicolas Brignone 
> > > > ---
> > > >   src/conf/device_conf.c | 183 +
> > > >   1 file changed, 56 insertions(+), 127 deletions(-)
> > > >
> > > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > > > index 7d48a3f..9fa6141 100644
> > > > --- a/src/conf/device_conf.c
> > > > +++ b/src/conf/device_conf.c
> > > > @@ -208,45 +208,43 @@ int
> > > >   virPCIDeviceAddressParseXML(xmlNodePtr node,
> > > >   virPCIDeviceAddressPtr addr)
> > > >   {
> > > > -char *domain, *slot, *bus, *function, *multi;
> > > >   xmlNodePtr cur;
> > > >   xmlNodePtr zpci = NULL;
> > > > -int ret = -1;
> > > >   memset(addr, 0, sizeof(*addr));
> > > > -domain   = virXMLPropString(node, "domain");
> > > > -bus  = virXMLPropString(node, "bus");
> > > > -slot = virXMLPropString(node, "slot");
> > > > -function = virXMLPropString(node, "function");
> > > > -multi= virXMLPropString(node, "multifunction");
> > > > +g_autofree char *domain   = virXMLPropString(node, "domain");
> > > > +g_autofree char *bus  = virXMLPropString(node, "bus");
> > > > +g_autofree char *slot = virXMLPropString(node, "slot");
> > > > +g_autofree char *function = virXMLPropString(node, "function");
> > > > +g_autofree char *multi= virXMLPropString(node, 
> > > > "multifunction");
> > >
> > >
> > > ... you've modified it so that local variables are being declared *below* 
> > > a
> > > line of executable code rather than at the top of the block. Although I
> > > don't see anything in the coding style document
> > > (https://libvirt.org/coding-style.html) about it, and it may just be
> > > leftover from a time when we supported a compiler that required all
> > > declarations to be at top of scope, I think pretty much all of libvirts 
> > > code
> > > declares all local variables at the top of the scope.
> >
> > We should really document it, and even enforce it at compile time.
> 
> I think you are asking for enabling:
> 
> -Wdeclaration-after-statement (C and Objective-C only)
>Warn when a declaration is found after a statement in a
> block.  This construct, known from C++, was introduced
>with ISO C99 and is by default allowed in GCC.  It is not
> supported by ISO C90.

Yes, but

> Will take a look.

..don't spend your time on enabling -Wdeclaration-after-statement unless
you are strongly motivated to do a lot of tedious work. I took at stab at
it about 12 months ago, but didn't complete it as there are a surprising
amount of subtle edge cases to deal with, so I never got as far as sending
a patch proposal.

FWIW, WIP is here

   https://gitlab.com/berrange/libvirt/-/tree/c99-vars

but this is based against ancient version, not current
git master, so I doubt it will rebase cleanly at all

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] All pointers to virXMLPropString() use g_autofree.

[Nicolás Brignone]

On Thu, Jul 9, 2020 at 5:15 AM Daniel P. Berrangé  wrote:
>
> On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> > On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> > >   All pointers to virXMLPropString() use g_autofree.
> >
> >
> > I changed the summary line like this, to be more precise:
> >
> >
> > conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
> >
> >
> > > All modified functions are similar, in all cases "cleanup" label is 
> > > removed,
> > > along with all the "goto" calls.
> >
> >
> > I've been advised in the recent past to put the g_autofree additions and
> > corresponding removals of free functions into one patch, then do the removal
> > of the extra labels (in favor of directly returning) in a separate patch to
> > make it easier to hand-verify / review. Here's a couple recent examples:
> >
> >
> > https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> >
> >
> > In your case the changes are few enough that I'm okay with it a single
> > patch, except...
> >
> >
> > >
> > > Signed-off-by: Nicolas Brignone 
> > > ---
> > >   src/conf/device_conf.c | 183 +
> > >   1 file changed, 56 insertions(+), 127 deletions(-)
> > >
> > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > > index 7d48a3f..9fa6141 100644
> > > --- a/src/conf/device_conf.c
> > > +++ b/src/conf/device_conf.c
> > > @@ -208,45 +208,43 @@ int
> > >   virPCIDeviceAddressParseXML(xmlNodePtr node,
> > >   virPCIDeviceAddressPtr addr)
> > >   {
> > > -char *domain, *slot, *bus, *function, *multi;
> > >   xmlNodePtr cur;
> > >   xmlNodePtr zpci = NULL;
> > > -int ret = -1;
> > >   memset(addr, 0, sizeof(*addr));
> > > -domain   = virXMLPropString(node, "domain");
> > > -bus  = virXMLPropString(node, "bus");
> > > -slot = virXMLPropString(node, "slot");
> > > -function = virXMLPropString(node, "function");
> > > -multi= virXMLPropString(node, "multifunction");
> > > +g_autofree char *domain   = virXMLPropString(node, "domain");
> > > +g_autofree char *bus  = virXMLPropString(node, "bus");
> > > +g_autofree char *slot = virXMLPropString(node, "slot");
> > > +g_autofree char *function = virXMLPropString(node, "function");
> > > +g_autofree char *multi= virXMLPropString(node, "multifunction");
> >
> >
> > ... you've modified it so that local variables are being declared *below* a
> > line of executable code rather than at the top of the block. Although I
> > don't see anything in the coding style document
> > (https://libvirt.org/coding-style.html) about it, and it may just be
> > leftover from a time when we supported a compiler that required all
> > declarations to be at top of scope, I think pretty much all of libvirts code
> > declares all local variables at the top of the scope.
>
> We should really document it, and even enforce it at compile time.

I think you are asking for enabling:

-Wdeclaration-after-statement (C and Objective-C only)
   Warn when a declaration is found after a statement in a
block.  This construct, known from C++, was introduced
   with ISO C99 and is by default allowed in GCC.  It is not
supported by ISO C90.

Will take a look.

> When you have variables in the middle of a function, and do a goto
> jump over them, they are in scope at the goto label target, but
> you may have jumped over their initialization. This makes it hard
> to rationalize about correctness of the code and has led to bugs
> involving uninitialized data before.
>
> 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 opens kernel+initrd in read-write mode

[Michal Privoznik]

On 7/9/20 4:32 PM, Olaf Hering wrote:

Is there a reason why libvirtd v6.5.0 opens kernel+initrd in mode RW?
'virsh start vm' fails of both are on a read-only filesystem.
Not sure if this ever worked before.


   
 hvm
 /path/to/kernel
 /path/to/initrd
 net.ifnames=0 console=ttyS0,115200 linemode=1 panic=9 
start_shell
 
   



13111 stat("/path/to/initrd", {st_mode=S_IFREG|0755, st_size=119207064, ...}) = 0
13111 openat(AT_FDCWD, "/path/to/initrd", O_RDWR) = -1 EROFS (Read-only file 
system)




Hey,

do you see an actual libvirt error? I think this may come from 
secdrivers trying to remember the original owner of kernel/initrd files.


If you set remember_owner=0 in /etc/libvirt/qemu.conf (and restart 
libvirtd) then does it fix your problem?


Michal

Re: [PATCH] resctrl: Do not open directory for writing

[Andrea Bolognani]

On Thu, 2020-07-09 at 13:44 +0200, Martin Kletzander wrote:
> On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:
> > it seems to me that this change entirely flipped the semantics of
> > the lock.
> 
> Yep, you're right.  When you have a lock that has boolean as a parameter I 
> think
> the boolean should indicate whether the lock is writable, but maybe the proper
> and most readable solution would be virFileFlockShareable() and
> virFileFlockExclusive() to avoid any misconception in the future[2].  Given 
> the
> fact that there are no (and most probably won't be) any other users of 
> flock(2)
> and given the fact that resctrl is also pretty niche feature, I don't have any
> preference.  Also I feel like after some time I'm a little bit rusty with C 
> and
> the libvirt codebase (and most importantly the reasoning and decisions) has
> changed a bit, so I'll leave the decision on how to deal with that on someone
> else.  I'm happy to provide the patches when clear decision is made.

Either

  virFileFlockExclusive(fd)
  virFileFlockShared(fd)

or

  virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE)
  virFileFlock(fd, VIR_FILE_FLOCK_SHARED)

would work. I like the latter better because it's closer to the
original flock(), which it ultimately acts as a very thin wrapper
around of. I'm actually unclear why we'd have the last argument in
the first place: personally I'd just use

  virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK)

and keep things as unambiguous as they can be.

This is all bikeshedding, of course: what actually matters is making
that lock exclusive once again :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] docs: kbase: Fix the libvirt-host-validate typo

[Ján Tomko]

On a Wednesday in 2020, Erik Skultety wrote:

On Wed, Jul 08, 2020 at 01:28:49PM +0200, Ján Tomko wrote:

On a Wednesday in 2020, Erik Skultety wrote:
> I overlooked this typo during review of 2c3ffa37.

Please do not follow commit hashes directly by a period.
* rephrase the sentence
* just drop the period
* include a separator like <>
or put in into the Fixes: tag


I remember you used to oppose the idea of using the Fixes/Resolves tags
in commit messages and be quite adamant about it,


I objected to their usage with links to bugzilla, since the word seems
redundant in combination with a bugzilla link (especially since it is
only intended for human consumption). Or, if you say 'Resolves'
on a preparatory patch, you're actually lying in the current commit
message.

Plain https: links mess with git's interpret-trailers functionality,
but that can be worked around in your .gitconfig:
[trailer "https"]
key = https:
ifExists = doNothing

With commit message IDs, if you dropped a plain one without a context,
I would not know whether you find a bug in it, or just took it as an
inspiration.


so looks strange you would suggest it.


I've known you for a few years now, surely there would be other things
by now I've had a change of heart about. :)

Jano


Anyhow, noted, I forgot that the period breaks the double click
selection of the hash.

Erik


signature.asc
Description: PGP signature

Re: [PATCH] storage: fix vstorage backend build

[Andrea Bolognani]

On Tue, 2020-07-07 at 15:00 +0300, Nikolay Shirokovskiy wrote:
> Add headers with declarations of  geteuid/getegid
> and virGetUserName/virGetGroupName.

Can you share the exact error message you're hitting? Our CI seems to
be perfectly happy with the current status quo. Specifically, the
x86-centos-7 job, the only one which is supposed to be able to build
the OpenVZ driver, is green.

Looking at the raw log[1] for the latest instance of the job, you can
see that the vz driver is enabled (vz: yes), but the vstorage driver
is not (Virtuozzo storage: no). From the configure output, the reason
seems clear:

  checking for vstorage... no
  checking for vstorage-mount... no

The CentOS 7 container that we use for builds has libprlsdk-devel and
friends installed from the official repository[2]... Although, now
that I look at it there's apparently a newer release[3] out, so we
should probably switch to it.


Another thing: the spec file calls configure with hardcoded

  --without-vz
  --without-storage-vstorage

and there is no binary package definition for anything like

  libvirt-daemon-driver-vz
  libvirt-daemon-driver-storage-vstorage

which would probably be nice to have enabled at least on CentOS 7?
Funnily enough, we actually already have

  %if 0%{?rhel}
%define with_vz 0
  %endif

so clearly at some point there was the intention to conditionally
include or exclude these drivers from the build.


One thing at a time, though. First, how do we get the vstorage
commands included in the CentOS 7 container? What packages need to
be installed, and from what repository?


As an aside, I'm still very confused by the vz/openvz dichotomy.
AFAICT, the latter can be (and in fact is) built unconditionally,
but the former requires the "Parallels SDK" packages to be installed:
baffingly enough, said SDK is obtained from the repository mentioned
above, which just so happens to include the string "openvz" twice in
its URL...


[1] 
https://storage.googleapis.com/gitlab-gprd-artifacts/5d/af/5daf0e5fa057744f5bfc6ec3d7ebc435b6fdcd9d40afe5a23cc67756a826d8ee/2020_07_09/631037426/692468732/job.log?response-content-type=text%2Fplain%3B%20charset%3Dutf-8=inline=gitlab-object-storage-...@gitlab-production.iam.gserviceaccount.com=hiC9XbFsV78D1nFcOg09WWcUAR66M2nbIECVm%2BD0tLBWMviBriGhB7eFpNL3%0A7H1bZ%2Faw0M64HaCJ8PZxw4SO6KeRkd5RZ3HC%2Ff7TWngvtKGy9xp4L9QLNcf7%0AiJVYvFQqPcDurp61C%2F%2B%2B5WNMxJq1Vl1wT6Lk0XlJlnssV59FyV4UHV3UAye%2F%0AiHrjSLcbuwbNIAu19dTAJskQroJ8hfRgAcSWDdU2%2FXWsCb19oCzpLBvwtr2T%0AIOYgLDjVCzntxu1gngIB499EDl%2B38ASB27kEFRkEDp8cEqz0wGOUstN4dAWu%0A4hH3S2%2FuXWoVoJ6ibOU%2Bh8REB1luIzVDQelJfGPblA%3D%3D=1594308539
[2] https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os
[3] https://download.openvz.org/virtuozzo/releases/openvz-7.0.14-136/x86_64/os
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 31/31] tools: wireshark: fix compilation errors

[Pavel Hrdina]

On Thu, Jul 09, 2020 at 04:41:29PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 09, 2020 at 05:33:14PM +0200, Pavel Hrdina wrote:
> > On Thu, Jul 09, 2020 at 04:22:41PM +0100, Daniel P. Berrangé wrote:
> > > On Thu, Jul 02, 2020 at 02:25:29PM +0200, Pavel Hrdina wrote:
> > > > With meson introduction which is using the same CFLAGS for the whole
> > > > project some compilation errors were discovered.
> > > > 
> > > > Signed-off-by: Pavel Hrdina 
> > > > ---
> > > >  tools/wireshark/src/packet-libvirt.c | 19 +--
> > > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/tools/wireshark/src/packet-libvirt.c 
> > > > b/tools/wireshark/src/packet-libvirt.c
> > > > index 20b7a3ec812..db8efe45a39 100644
> > > > --- a/tools/wireshark/src/packet-libvirt.c
> > > > +++ b/tools/wireshark/src/packet-libvirt.c
> > > > @@ -77,15 +77,15 @@ static gint ett_libvirt_stream_hole = -1;
> > > >  
> > > >  XDR_PRIMITIVE_DISSECTOR(int, gint32,  int)
> > > >  XDR_PRIMITIVE_DISSECTOR(u_int,   guint32, uint)
> > > > -XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> > > > +//XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> > > >  XDR_PRIMITIVE_DISSECTOR(u_short, guint16, uint)
> > > >  XDR_PRIMITIVE_DISSECTOR(char,gchar,   int)
> > > >  XDR_PRIMITIVE_DISSECTOR(u_char,  guchar,  uint)
> > > >  XDR_PRIMITIVE_DISSECTOR(hyper,   gint64,  int64)
> > > >  XDR_PRIMITIVE_DISSECTOR(u_hyper, guint64, uint64)
> > > > -XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> > > > +//XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> > > >  XDR_PRIMITIVE_DISSECTOR(double,  gdouble, double)
> > > > -XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> > > > +//XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> > > 
> > > 
> > > This looks a bit iffy - what's the rationale for this ?
> > 
> > To keep it here if we ever need to use these types and to illustrate
> > that they are not used on purpose. But it can be completely removed
> > as well. I have no preference here.
> 
> Oh, so are you saying it was complaining about unused functions ?
> 
> We could just use a pragma to squelch the warning flag in this
> source file.

Correct, I'll include a patch with this change to trigger the errors
with autotools:

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 53df930e0ac..eb8f269b486 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -378,7 +378,7 @@ if WITH_WIRESHARK_DISSECTOR

 ws_plugin_LTLIBRARIES = wireshark/src/libvirt.la
 wireshark_src_libvirt_la_CFLAGS = \
-   -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) $(XDR_CFLAGS)
+   -I wireshark/src $(WIRESHARK_DISSECTOR_CFLAGS) $(XDR_CFLAGS) 
$(AM_CFLAGS)
 wireshark_src_libvirt_la_LDFLAGS = -avoid-version -module
 wireshark_src_libvirt_la_SOURCES = \
wireshark/src/packet-libvirt.h \


It will result in these errors:

../../tools/wireshark/src/packet-libvirt.c: In function 'dissect_libvirt_fds':
../../tools/wireshark/src/packet-libvirt.c:348:31: error: unused parameter 
'tvb' [-Werror=unused-parameter]
  348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
  | ~~^~~
../../tools/wireshark/src/packet-libvirt.c:348:41: error: unused parameter 
'start' [-Werror=unused-parameter]
  348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
  |~^
../../tools/wireshark/src/packet-libvirt.c:348:55: error: unused parameter 
'nfds' [-Werror=unused-parameter]
  348 | dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
  |~~~^~~~
At top level:
../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_bool' 
defined but not used [-Werror=unused-function]
   64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int 
hf) \
  | ^~~~
../../tools/wireshark/src/packet-libvirt.c:88:1: note: in expansion of macro 
'XDR_PRIMITIVE_DISSECTOR'
   88 | XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
  | ^~~
../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_float' 
defined but not used [-Werror=unused-function]
   64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int 
hf) \
  | ^~~~
../../tools/wireshark/src/packet-libvirt.c:86:1: note: in expansion of macro 
'XDR_PRIMITIVE_DISSECTOR'
   86 | XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
  | ^~~
../../tools/wireshark/src/packet-libvirt.c:64:5: error: 'dissect_xdr_short' 
defined but not used [-Werror=unused-function]
   64 | dissect_xdr_##xtype(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int 
hf) \
  | ^~~~
../../tools/wireshark/src/packet-libvirt.c:80:1: note: in expansion of macro 
'XDR_PRIMITIVE_DISSECTOR'
   80 | XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
  | ^~~
../../tools/wireshark/src/packet-libvirt.c: In function 

Re: [libvirt PATCH 00/31] fixes and cleanups for current build system

[Michal Privoznik]

On 7/2/20 2:24 PM, Pavel Hrdina wrote:

While working on rewrite to Meson I discovered some parts of our
current build system that could be improved to help with the
transition to Meson. It will make the review of the Meson patches
a bit easier.

Pavel Hrdina (31):
   build: use DLOPEN_LIBS directly
   configure: drop check for unsupported FreeBSD
   configure: introduce FLAT_NAMESPACE_FLAGS
   configure: remove usage of AC_HEADER_MAJOR
   Makefile: drop undefined LIB_CLOCK_GETTIME
   docs: remove incorrect generated files by apibuild.py
   docs: remove unused wrapstring.xsl file
   docs: drop %.png: %.fig rule
   m4: virt-sanlock: drop check for sanlock_inq_lockspace
   m4: virt-sanlock: use pkg-config to find libsanlock_client
   m4: virt-sanlock: drop check for SANLK_INQ_WAIT
   m4: virt-sanlock: drop check for sanlock_killpath()
   m4: virt-sanlock: drop check for sanlock_write_lockspace()
   m4: virt-secdriver-selinux: drop obsolete function checks
   m4: virt-selinux: drop check for selabel_open signature change
   m4: virt-xdr: rewrite XDR check
   po: change the format of POTFILES.in
   scripts: check-remote-protocol: remove unused OBJEXT argument
   src: Makefile: remove LOCK_CHECKING_CFLAGS leftover
   src: remove unnecessary -I$(srcdir)/secret include
   src: remote: Makefile: drop CFLAGS and LDFLAGS duplication
   src: logging: Makefile: drop undefined LOG_DRIVER
   src: util: rename some program macros
   src: util: Makefile: drop undefined LDEXP_LIBM
   src: util: Makefile: drop undefined OPENPTY_LIBS
   src: remote: Makefile: properly format sysconfdir in virtproxyd.conf
   src: unify virFileActivateDirOverride()
   tests: commandhelper: change how we detect if running as daemon
   tests: use WITH_NSS instead of NSS
   tools: virsh-secret: fix compilation error
   tools: wireshark: fix compilation errors

  build-aux/syntax-check.mk|   4 +-
  configure.ac |  26 +-
  docs/Makefile.am |  12 +-
  docs/wrapstring.xsl  |  56 ---
  m4/virt-driver-modules.m4|   3 -
  m4/virt-external-programs.m4 |   8 +-
  m4/virt-nss.m4   |   2 +-
  m4/virt-sanlock.m4   |  33 +-
  m4/virt-secdriver-selinux.m4 |  24 +-
  m4/virt-selinux.m4   |  17 -
  m4/virt-xdr.m4   |  39 +-
  po/Makefile.am   |   6 +-
  po/POTFILES.in   | 726 +--
  scripts/check-remote-protocol.py |   5 +-
  src/Makefile.am  |  25 +-
  src/admin/Makefile.inc.am|   1 +
  src/libvirt.c|   2 +-
  src/libvirt_private.syms |   3 +-
  src/libxl/Makefile.inc.am|   1 -
  src/locking/Makefile.inc.am  |   2 +
  src/locking/lock_daemon.c|   2 +-
  src/locking/lock_driver_sanlock.c|  38 --
  src/logging/Makefile.inc.am  |   2 +-
  src/logging/log_daemon.c |   2 +-
  src/qemu/Makefile.inc.am |   1 -
  src/qemu/qemu_shim.c |   2 +-
  src/remote/Makefile.inc.am   |  13 +-
  src/remote/remote_daemon.c   |   2 +-
  src/security/virt-aa-helper.c|   2 +-
  src/storage/Makefile.inc.am  |   4 -
  src/util/Makefile.inc.am |   3 -
  src/util/virfile.c   |  25 +-
  src/util/virfile.h   |   4 +-
  src/util/virnetdevmidonet.c  |   4 +-
  src/util/virnetdevopenvswitch.c  |  16 +-
  tests/Makefile.am|  10 +-
  tests/commandhelper.c|   2 +-
  tests/nssmock.c  |   2 +-
  tests/nsstest.c  |   2 +-
  tests/qemucapsprobe.c|   2 +-
  tests/securityselinuxhelper.c|   5 +-
  tests/testutils.c|   2 +-
  tools/virsh-secret.c |   2 +-
  tools/virsh.c|   2 +-
  tools/virt-admin.c   |   2 +-
  tools/wireshark/src/packet-libvirt.c |  19 +-
  46 files changed, 472 insertions(+), 693 deletions(-)
  delete mode 100644 docs/wrapstring.xsl



Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH 31/31] tools: wireshark: fix compilation errors

[Daniel P . Berrangé]

On Thu, Jul 09, 2020 at 05:33:14PM +0200, Pavel Hrdina wrote:
> On Thu, Jul 09, 2020 at 04:22:41PM +0100, Daniel P. Berrangé wrote:
> > On Thu, Jul 02, 2020 at 02:25:29PM +0200, Pavel Hrdina wrote:
> > > With meson introduction which is using the same CFLAGS for the whole
> > > project some compilation errors were discovered.
> > > 
> > > Signed-off-by: Pavel Hrdina 
> > > ---
> > >  tools/wireshark/src/packet-libvirt.c | 19 +--
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/wireshark/src/packet-libvirt.c 
> > > b/tools/wireshark/src/packet-libvirt.c
> > > index 20b7a3ec812..db8efe45a39 100644
> > > --- a/tools/wireshark/src/packet-libvirt.c
> > > +++ b/tools/wireshark/src/packet-libvirt.c
> > > @@ -77,15 +77,15 @@ static gint ett_libvirt_stream_hole = -1;
> > >  
> > >  XDR_PRIMITIVE_DISSECTOR(int, gint32,  int)
> > >  XDR_PRIMITIVE_DISSECTOR(u_int,   guint32, uint)
> > > -XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> > > +//XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> > >  XDR_PRIMITIVE_DISSECTOR(u_short, guint16, uint)
> > >  XDR_PRIMITIVE_DISSECTOR(char,gchar,   int)
> > >  XDR_PRIMITIVE_DISSECTOR(u_char,  guchar,  uint)
> > >  XDR_PRIMITIVE_DISSECTOR(hyper,   gint64,  int64)
> > >  XDR_PRIMITIVE_DISSECTOR(u_hyper, guint64, uint64)
> > > -XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> > > +//XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> > >  XDR_PRIMITIVE_DISSECTOR(double,  gdouble, double)
> > > -XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> > > +//XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> > 
> > 
> > This looks a bit iffy - what's the rationale for this ?
> 
> To keep it here if we ever need to use these types and to illustrate
> that they are not used on purpose. But it can be completely removed
> as well. I have no preference here.

Oh, so are you saying it was complaining about unused functions ?

We could just use a pragma to squelch the warning flag in this
source file.


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 31/31] tools: wireshark: fix compilation errors

[Pavel Hrdina]

On Thu, Jul 09, 2020 at 04:22:41PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 02, 2020 at 02:25:29PM +0200, Pavel Hrdina wrote:
> > With meson introduction which is using the same CFLAGS for the whole
> > project some compilation errors were discovered.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  tools/wireshark/src/packet-libvirt.c | 19 +--
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/wireshark/src/packet-libvirt.c 
> > b/tools/wireshark/src/packet-libvirt.c
> > index 20b7a3ec812..db8efe45a39 100644
> > --- a/tools/wireshark/src/packet-libvirt.c
> > +++ b/tools/wireshark/src/packet-libvirt.c
> > @@ -77,15 +77,15 @@ static gint ett_libvirt_stream_hole = -1;
> >  
> >  XDR_PRIMITIVE_DISSECTOR(int, gint32,  int)
> >  XDR_PRIMITIVE_DISSECTOR(u_int,   guint32, uint)
> > -XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> > +//XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> >  XDR_PRIMITIVE_DISSECTOR(u_short, guint16, uint)
> >  XDR_PRIMITIVE_DISSECTOR(char,gchar,   int)
> >  XDR_PRIMITIVE_DISSECTOR(u_char,  guchar,  uint)
> >  XDR_PRIMITIVE_DISSECTOR(hyper,   gint64,  int64)
> >  XDR_PRIMITIVE_DISSECTOR(u_hyper, guint64, uint64)
> > -XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> > +//XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> >  XDR_PRIMITIVE_DISSECTOR(double,  gdouble, double)
> > -XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> > +//XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> 
> 
> This looks a bit iffy - what's the rationale for this ?

To keep it here if we ever need to use these types and to illustrate
that they are not used on purpose. But it can be completely removed
as well. I have no preference here.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 31/31] tools: wireshark: fix compilation errors

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

With meson introduction which is using the same CFLAGS for the whole
project some compilation errors were discovered.


What were the errors?



Signed-off-by: Pavel Hrdina 
---
tools/wireshark/src/packet-libvirt.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index 20b7a3ec812..db8efe45a39 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -77,15 +77,15 @@ static gint ett_libvirt_stream_hole = -1;

XDR_PRIMITIVE_DISSECTOR(int, gint32,  int)
XDR_PRIMITIVE_DISSECTOR(u_int,   guint32, uint)
-XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
+//XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
XDR_PRIMITIVE_DISSECTOR(u_short, guint16, uint)
XDR_PRIMITIVE_DISSECTOR(char,gchar,   int)
XDR_PRIMITIVE_DISSECTOR(u_char,  guchar,  uint)
XDR_PRIMITIVE_DISSECTOR(hyper,   gint64,  int64)
XDR_PRIMITIVE_DISSECTOR(u_hyper, guint64, uint64)
-XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
+//XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
XDR_PRIMITIVE_DISSECTOR(double,  gdouble, double)
-XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
+//XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)



If we don't need these, we can just delete them instead of commenting
them out.


typedef gboolean (*vir_xdr_dissector_t)(tvbuff_t *tvb, proto_tree *tree, XDR 
*xdrs, int hf);



The rest of the changes look reasonable.

Jano


@@ -345,7 +345,9 @@ dissect_libvirt_num_of_fds(tvbuff_t *tvb, proto_tree *tree)
}

static void
-dissect_libvirt_fds(tvbuff_t *tvb, gint start, gint32 nfds)
+dissect_libvirt_fds(tvbuff_t *tvb G_GNUC_UNUSED,
+gint start G_GNUC_UNUSED,
+gint32 nfds G_GNUC_UNUSED)
{
/* TODO: NOP for now */
}
@@ -420,8 +422,13 @@ dissect_libvirt_payload(tvbuff_t *tvb, proto_tree *tree,
return; /* No payload */

if (status == VIR_NET_OK) {
-vir_xdr_dissector_t xd = find_payload_dissector(proc, type, 
get_program_data(prog, VIR_PROGRAM_DISSECTORS),
-*(gsize 
*)get_program_data(prog, VIR_PROGRAM_DISSECTORS_LEN));
+const vir_dissector_index_t *pds = get_program_data(prog, 
VIR_PROGRAM_DISSECTORS);
+const gsize *len = get_program_data(prog, VIR_PROGRAM_DISSECTORS_LEN);
+
+if (!len)
+goto unknown;
+
+vir_xdr_dissector_t xd = find_payload_dissector(proc, type, pds, *len);
if (xd == NULL)
goto unknown;
dissect_libvirt_payload_xdr_data(tvb, tree, payload_length, status, xd);
--
2.26.2



signature.asc
Description: PGP signature

Re: [libvirt PATCH 30/31] tools: virsh-secret: fix compilation error

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

../tools/virsh-secret.c: In function ‘cmdSecretSetValue’:
../tools/virsh-secret.c:262:15: error: pointer targets in assignment from ‘char 
*’ to ‘unsigned char *’ differ in signedness [-Werror=pointer-sign]
 262 | value = g_steal_pointer(_buf);
 |   ^
cc1: all warnings being treated as errors



Where do you get this error?


Signed-off-by: Pavel Hrdina 
---
tools/virsh-secret.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 16/31] m4: virt-xdr: rewrite XDR check

[Pavel Hrdina]

On Thu, Jul 09, 2020 at 04:12:29PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 02, 2020 at 02:25:14PM +0200, Pavel Hrdina wrote:
> > The current code to check XDR support was obsolete and way to
> > complicated.
> > 
> > On linux we can use pkg-config to check for libtirpc and have
> > the CFLAGS and LIBS configured by it as well.
> 
> Historically we've only used libtirpc for RHEL 8, not RHEL 7,
> though I see it did indeed exist in RHEL-7. So this would mean
> the libvirt.spec.in condition needs updating.

Good point, I'll fix it in v2, thanks.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 27/31] src: unify virFileActivateDirOverride()

[Daniel P . Berrangé]

On Thu, Jul 02, 2020 at 02:25:25PM +0200, Pavel Hrdina wrote:
> We have two functions to set that we want to override directory where to
> look for libvirt files, one checks the binary name and the other one
> checks env variable.
> 
> This patch removes the binary name check to simplify code by using only
> the env variable.
> 
> This change will be required for Meson introduction where the binary
> name doesn't have any prefix by default.

This means that we won't be able to run  "./src/libvirtd" directly from
the build dir - we'll *always* have to use  "./run ./src/libvirtd" to
get the env var set IIUC.

Instead of checking the binary name, can we check for existance of a
known valid build file in the directory we're running the binary from.
eg look for "Makefile" or "ninja.build" or something like that.

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 31/31] tools: wireshark: fix compilation errors

[Daniel P . Berrangé]

On Thu, Jul 02, 2020 at 02:25:29PM +0200, Pavel Hrdina wrote:
> With meson introduction which is using the same CFLAGS for the whole
> project some compilation errors were discovered.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  tools/wireshark/src/packet-libvirt.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/wireshark/src/packet-libvirt.c 
> b/tools/wireshark/src/packet-libvirt.c
> index 20b7a3ec812..db8efe45a39 100644
> --- a/tools/wireshark/src/packet-libvirt.c
> +++ b/tools/wireshark/src/packet-libvirt.c
> @@ -77,15 +77,15 @@ static gint ett_libvirt_stream_hole = -1;
>  
>  XDR_PRIMITIVE_DISSECTOR(int, gint32,  int)
>  XDR_PRIMITIVE_DISSECTOR(u_int,   guint32, uint)
> -XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
> +//XDR_PRIMITIVE_DISSECTOR(short,   gint16,  int)
>  XDR_PRIMITIVE_DISSECTOR(u_short, guint16, uint)
>  XDR_PRIMITIVE_DISSECTOR(char,gchar,   int)
>  XDR_PRIMITIVE_DISSECTOR(u_char,  guchar,  uint)
>  XDR_PRIMITIVE_DISSECTOR(hyper,   gint64,  int64)
>  XDR_PRIMITIVE_DISSECTOR(u_hyper, guint64, uint64)
> -XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
> +//XDR_PRIMITIVE_DISSECTOR(float,   gfloat,  float)
>  XDR_PRIMITIVE_DISSECTOR(double,  gdouble, double)
> -XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)
> +//XDR_PRIMITIVE_DISSECTOR(bool,bool_t,  boolean)


This looks a bit iffy - what's the rationale for this ?


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 14/31] m4: virt-secdriver-selinux: drop obsolete function checks

[Pavel Hrdina]

On Thu, Jul 09, 2020 at 05:00:39PM +0200, Ján Tomko wrote:
> On a Thursday in 2020, Pavel Hrdina wrote:
> > All of the listed functions are available in libselinux version 2.2.
> > Our supported OSes start with version 2.5 so there is no need to check
> > it.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> > m4/virt-secdriver-selinux.m4 | 24 ++--
> > 1 file changed, 2 insertions(+), 22 deletions(-)
> 
> This doesn't look right with no changes to the SELinux driver.
> 
> > 
> > diff --git a/m4/virt-secdriver-selinux.m4 b/m4/virt-secdriver-selinux.m4
> > index a48569fc33a..4174249a510 100644
> > --- a/m4/virt-secdriver-selinux.m4
> > +++ b/m4/virt-secdriver-selinux.m4
> > @@ -32,28 +32,8 @@ AC_DEFUN([LIBVIRT_SECDRIVER_CHECK_SELINUX], [
> >   AC_MSG_ERROR([You must install the libselinux development package and 
> > enable SELinux with the --with-selinux=yes in order to compile libvirt 
> > --with-secdriver-selinux=yes])
> > fi
> >   elif test "$with_secdriver_selinux" != "no"; then
> > -old_CFLAGS="$CFLAGS"
> > -old_LIBS="$LIBS"
> > -CFLAGS="$CFLAGS $SELINUX_CFLAGS"
> > -LIBS="$CFLAGS $SELINUX_LIBS"
> > -
> > -fail=0
> > -AC_CHECK_FUNC([selinux_virtual_domain_context_path], [], [fail=1])
> > -AC_CHECK_FUNC([selinux_virtual_image_context_path], [], [fail=1])
> > -AC_CHECK_FUNCS([selinux_lxc_contexts_path])
> 
> This means the code relying on HAVE_SELINUX_LXC_CONTEXTS_PATH will no
> longer be compiled.
> 
> Jano

Nice catch, thanks, I'll fix that in v2.

Pavel


signature.asc
Description: PGP signature

Re: [libvirt PATCH 29/31] tests: use WITH_NSS instead of NSS

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
m4/virt-nss.m4  | 2 +-
tests/nssmock.c | 2 +-
tests/nsstest.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 27/31] src: unify virFileActivateDirOverride()

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

We have two functions to set that we want to override directory where to
look for libvirt files, one checks the binary name and the other one
checks env variable.

This patch removes the binary name check to simplify code by using only
the env variable.

This change will be required for Meson introduction where the binary
name doesn't have any prefix by default.

Signed-off-by: Pavel Hrdina 
---
src/libvirt.c |  2 +-
src/libvirt_private.syms  |  3 +--
src/locking/lock_daemon.c |  2 +-
src/logging/log_daemon.c  |  2 +-
src/qemu/qemu_shim.c  |  2 +-
src/remote/remote_daemon.c|  2 +-
src/security/virt-aa-helper.c |  2 +-
src/util/virfile.c| 25 +
src/util/virfile.h|  4 +---
tests/Makefile.am |  1 +
tests/qemucapsprobe.c |  2 +-
tests/testutils.c |  2 +-
tools/virsh.c |  2 +-
tools/virt-admin.c|  2 +-
14 files changed, 14 insertions(+), 39 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 23/31] src: util: rename some program macros

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Fixes inconsistency with macro names for external programs.

Signed-off-by: Pavel Hrdina 
---
m4/virt-external-programs.m4|  8 
src/util/virnetdevmidonet.c |  4 ++--
src/util/virnetdevopenvswitch.c | 16 
3 files changed, 14 insertions(+), 14 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 25/31] src: util: Makefile: drop undefined OPENPTY_LIBS

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Commit  added check for
openpty function from util library using AC_CHECK_LIB(). However, that
macro doesn't define OPENPTY_LIBS, it only defines WITH_LIBUTIL and
prepends -lutil into LIBS for the whole project.

Signed-off-by: Pavel Hrdina 
---
src/util/Makefile.inc.am | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 26/31] src: remote: Makefile: properly format sysconfdir in virtproxyd.conf

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Commit <5b816e16968ba02def56f067774ecd9a8c8d44d7> removed hard-coded
sysconfdir path from *.conf files but missed virtproxyd.

Signed-off-by: Pavel Hrdina 
---
src/remote/Makefile.inc.am | 3 +++
1 file changed, 3 insertions(+)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 21/31] src: remote: Makefile: drop CFLAGS and LDFLAGS duplication

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Signed-off-by: Pavel Hrdina 
---
src/remote/Makefile.inc.am | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index 8a40c96563c..893d6894e2f 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -35,16 +35,13 @@ REMOTE_DAEMON_SOURCES = \
$(NULL)

REMOTE_DAEMON_CFLAGS = \
-   $(LIBXML_CFLAGS) \
-   $(GLIB_CFLAGS) \
+   $(AM_CFLAGS) \
$(GNUTLS_CFLAGS) \
$(SASL_CFLAGS) \
$(XDR_CFLAGS) \
$(DBUS_CFLAGS) \
$(LIBNL_CFLAGS) \
-   $(WARN_CFLAGS) \
$(PIE_CFLAGS) \
-   $(COVERAGE_CFLAGS) \
-I$(srcdir)/access \
-I$(builddir)/access \
-I$(srcdir)/conf \


No practical change in this hunk.


@@ -54,11 +51,9 @@ REMOTE_DAEMON_CFLAGS = \
$(NULL)

REMOTE_DAEMON_LD_FLAGS = \
-   $(RELRO_LDFLAGS) \
+   $(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
-   $(NO_INDIRECT_LDFLAGS) \
$(NO_UNDEFINED_LDFLAGS) \
-   $(COVERAGE_LDFLAGS) \
$(NULL)



AM_LDFLAGS also contains $(DRIVER_MODULES_LDFLAGS)
i.e. -export-dynamic

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 22/31] src: logging: Makefile: drop undefined LOG_DRIVER

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Commit <0985a9597bb0348d46c0d18dc548a676bf0ad8e2> added unused variable
so remove it.

Signed-off-by: Pavel Hrdina 
---
src/logging/Makefile.inc.am | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] cpu_arm: fix build on non-Linux

[Roman Bogorodskiy]

  Michal Privoznik wrote:

> On 7/8/20 4:28 PM, Roman Bogorodskiy wrote:
> >   - Add a check for asm/hwcap.h header presence,
> >   - Add a check for getauxval() function that is used
> > on Linux, and for elf_aux_info() which is a FreeBSD
> > equivalent.
> > 
> > This is based on a patch submitted by Mikael Urankar in
> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=247722.
> > 
> > Signed-off-by: Roman Bogorodskiy 
> > ---
> >   configure.ac  |  3 +++
> >   src/cpu/cpu_arm.c | 12 +++-
> >   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Michal Privoznik 
> 
> Michal
> 

Pushed, thanks!

Roman Bogorodskiy


signature.asc
Description: PGP signature

Re: [libvirt PATCH 24/31] src: util: Makefile: drop undefined LDEXP_LIBM

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

It was introduced by commit 
as a gnulib ldexp module and later removed by commit
<09fe607b4de8eb883c966e90aaf5563299a22738>.

Signed-off-by: Pavel Hrdina 
---
src/util/Makefile.inc.am | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 08/31] docs: drop %.png: %.fig rule

[Daniel P . Berrangé]

On Thu, Jul 09, 2020 at 05:13:47PM +0200, Michal Privoznik wrote:
> On 7/9/20 4:59 PM, Daniel P. Berrangé wrote:
> > On Thu, Jul 02, 2020 at 02:25:06PM +0200, Pavel Hrdina wrote:
> > > convert bin is part of ImageMagick package and uses uniconvertor to
> > > create png file from fig files.
> > > 
> > > Unfortunately uniconvertor is python2 only and not available in most
> > > recent distributions which makes the convert command fail with:
> > > 
> > > sh: uniconvertor: command not found
> > > /usr/bin/mv: cannot stat '/tmp/magick-1397138DRT8Pzx4Qmoc.svg': No such 
> > > file or directory
> > > convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv 
> > > '%o.svg' '%o'' @ error/delegate.c/InvokeDelegate/1958.
> > > convert: unable to open file `/tmp/magick-1397138S8ARueJXLXkc': No such 
> > > file or directory @ error/constitute.c/ReadImage/605.
> > > convert: no images defined `docs/migration-managed-direct.png' @ 
> > > error/convert.c/ConvertImageCommand/3226.
> > > 
> > > It looks like that there are plans to somehow port uniconvertor into
> > > python3 but as part of different project color-picker but the job is
> > > far from complete.
> > 
> > FYI i would support replacing the .fig files in git with .svgs
> > which will avoid the extra conversion step. The .fig stuff is just
> > too anachronistic to be worth worrying about.
> 
> Actually, I think that is the right way to go instead of just removing this
> rule. Because if we ever change a *.fig file we have to remember to
> regenerate the corresponding .png file. And then we can drop .png files from
> our repos.

In practice that turned out to not be a good idea. When I did the new
logos a few years back I tried having the pngs auto-generated at build
time. The Inkscape SVG -> bitmap rendering was not very good quality
wrt its anti-aliasing.  Rendering them in GIMP gave far better results.

I still think we should convert the fig to svg though, despite not using
them in the build procss.

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 08/31] docs: drop %.png: %.fig rule

[Michal Privoznik]

On 7/9/20 4:59 PM, Daniel P. Berrangé wrote:

On Thu, Jul 02, 2020 at 02:25:06PM +0200, Pavel Hrdina wrote:

convert bin is part of ImageMagick package and uses uniconvertor to
create png file from fig files.

Unfortunately uniconvertor is python2 only and not available in most
recent distributions which makes the convert command fail with:

sh: uniconvertor: command not found
/usr/bin/mv: cannot stat '/tmp/magick-1397138DRT8Pzx4Qmoc.svg': No such file or 
directory
convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' 
'%o'' @ error/delegate.c/InvokeDelegate/1958.
convert: unable to open file `/tmp/magick-1397138S8ARueJXLXkc': No such file or 
directory @ error/constitute.c/ReadImage/605.
convert: no images defined `docs/migration-managed-direct.png' @ 
error/convert.c/ConvertImageCommand/3226.

It looks like that there are plans to somehow port uniconvertor into
python3 but as part of different project color-picker but the job is
far from complete.


FYI i would support replacing the .fig files in git with .svgs
which will avoid the extra conversion step. The .fig stuff is just
too anachronistic to be worth worrying about.


Actually, I think that is the right way to go instead of just removing 
this rule. Because if we ever change a *.fig file we have to remember to 
regenerate the corresponding .png file. And then we can drop .png files 
from our repos.


Michal

Re: [libvirt PATCH 16/31] m4: virt-xdr: rewrite XDR check

[Daniel P . Berrangé]

On Thu, Jul 02, 2020 at 02:25:14PM +0200, Pavel Hrdina wrote:
> The current code to check XDR support was obsolete and way to
> complicated.
> 
> On linux we can use pkg-config to check for libtirpc and have
> the CFLAGS and LIBS configured by it as well.

Historically we've only used libtirpc for RHEL 8, not RHEL 7,
though I see it did indeed exist in RHEL-7. So this would mean
the libvirt.spec.in condition needs updating.

> 
> On MinGW there is portablexdr library which installs header files
> directly into system include directory.
> 
> On FreeBSD and macOS XDR functions are part of libc so there is
> no library needed, we just need to call AM_CONDITIONAL to silence
> configure which otherwise complains about missing WITH_XDR.

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 20/31] src: remove unnecessary -I$(srcdir)/secret include

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Commit <894556ca813ad3c4ebb01083b7971d73b4f53c8b> moved function
virSecretGetSecretString out of secret directory but forgot to update
CFLAGS in places where the include is no longer needed.

Signed-off-by: Pavel Hrdina 
---
src/libxl/Makefile.inc.am   | 1 -
src/qemu/Makefile.inc.am| 1 -
src/storage/Makefile.inc.am | 4 
3 files changed, 6 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 19/31] src: Makefile: remove LOCK_CHECKING_CFLAGS leftover

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Commit  removed objectlocking
test but forgot to remove all of the usages of LOCK_CHECKING_CFLAGS.

Signed-off-by: Pavel Hrdina 
---
src/Makefile.am | 1 -
1 file changed, 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 17/31] po: change the format of POTFILES.in

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

There is no need to provide relative paths to the current directory if
we provide search paths using --directory option for xgentext.


xgettext



In addition it will make libvirt.pot file look cleaner as it will not
contain relative paths to current directory. It improves the situation
for developers which are using different build path as that would
change the relative path in libvirt.pot as well. After this patch
it will not happen anymore.

Signed-off-by: Pavel Hrdina 
---
build-aux/syntax-check.mk |   4 +-
po/Makefile.am|   6 +-
po/POTFILES.in| 726 +++---
3 files changed, 369 insertions(+), 367 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 08/31] docs: drop %.png: %.fig rule

[Daniel P . Berrangé]

On Thu, Jul 02, 2020 at 02:25:06PM +0200, Pavel Hrdina wrote:
> convert bin is part of ImageMagick package and uses uniconvertor to
> create png file from fig files.
> 
> Unfortunately uniconvertor is python2 only and not available in most
> recent distributions which makes the convert command fail with:
> 
> sh: uniconvertor: command not found
> /usr/bin/mv: cannot stat '/tmp/magick-1397138DRT8Pzx4Qmoc.svg': No such file 
> or directory
> convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' 
> '%o'' @ error/delegate.c/InvokeDelegate/1958.
> convert: unable to open file `/tmp/magick-1397138S8ARueJXLXkc': No such file 
> or directory @ error/constitute.c/ReadImage/605.
> convert: no images defined `docs/migration-managed-direct.png' @ 
> error/convert.c/ConvertImageCommand/3226.
> 
> It looks like that there are plans to somehow port uniconvertor into
> python3 but as part of different project color-picker but the job is
> far from complete.

FYI i would support replacing the .fig files in git with .svgs
which will avoid the extra conversion step. The .fig stuff is just
too anachronistic to be worth worrying about. 

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 16/31] m4: virt-xdr: rewrite XDR check

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

The current code to check XDR support was obsolete and way to
complicated.

On linux we can use pkg-config to check for libtirpc and have
the CFLAGS and LIBS configured by it as well.

On MinGW there is portablexdr library which installs header files
directly into system include directory.

On FreeBSD and macOS XDR functions are part of libc so there is
no library needed, we just need to call AM_CONDITIONAL to silence
configure which otherwise complains about missing WITH_XDR.

Signed-off-by: Pavel Hrdina 
---
m4/virt-xdr.m4  | 39 +++--
src/Makefile.am |  4 +++-
src/admin/Makefile.inc.am   |  1 +
src/locking/Makefile.inc.am |  2 ++
src/logging/Makefile.inc.am |  1 +
src/remote/Makefile.inc.am  |  1 +
6 files changed, 19 insertions(+), 29 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 14/31] m4: virt-secdriver-selinux: drop obsolete function checks

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

All of the listed functions are available in libselinux version 2.2.
Our supported OSes start with version 2.5 so there is no need to check
it.

Signed-off-by: Pavel Hrdina 
---
m4/virt-secdriver-selinux.m4 | 24 ++--
1 file changed, 2 insertions(+), 22 deletions(-)


This doesn't look right with no changes to the SELinux driver.



diff --git a/m4/virt-secdriver-selinux.m4 b/m4/virt-secdriver-selinux.m4
index a48569fc33a..4174249a510 100644
--- a/m4/virt-secdriver-selinux.m4
+++ b/m4/virt-secdriver-selinux.m4
@@ -32,28 +32,8 @@ AC_DEFUN([LIBVIRT_SECDRIVER_CHECK_SELINUX], [
  AC_MSG_ERROR([You must install the libselinux development package and 
enable SELinux with the --with-selinux=yes in order to compile libvirt 
--with-secdriver-selinux=yes])
fi
  elif test "$with_secdriver_selinux" != "no"; then
-old_CFLAGS="$CFLAGS"
-old_LIBS="$LIBS"
-CFLAGS="$CFLAGS $SELINUX_CFLAGS"
-LIBS="$CFLAGS $SELINUX_LIBS"
-
-fail=0
-AC_CHECK_FUNC([selinux_virtual_domain_context_path], [], [fail=1])
-AC_CHECK_FUNC([selinux_virtual_image_context_path], [], [fail=1])
-AC_CHECK_FUNCS([selinux_lxc_contexts_path])


This means the code relying on HAVE_SELINUX_LXC_CONTEXTS_PATH will no
longer be compiled.

Jano


-CFLAGS="$old_CFLAGS"
-LIBS="$old_LIBS"
-
-if test "$fail" = "1" ; then
-  if test "$with_secdriver_selinux" = "check" ; then
-with_secdriver_selinux=no
-  else
-AC_MSG_ERROR([You must install libselinux development package >= 
2.0.82 in order to compile libvirt --with-secdriver-selinux=yes])
-  fi
-else
-  with_secdriver_selinux=yes
-  AC_DEFINE_UNQUOTED([WITH_SECDRIVER_SELINUX], 1, [whether SELinux 
security driver is available])
-fi
+with_secdriver_selinux=yes
+AC_DEFINE_UNQUOTED([WITH_SECDRIVER_SELINUX], 1, [whether SELinux security 
driver is available])
  fi
  AM_CONDITIONAL([WITH_SECDRIVER_SELINUX], [test "$with_secdriver_selinux" != 
"no"])
])
--
2.26.2



signature.asc
Description: PGP signature

Re: [libvirt PATCH 15/31] m4: virt-selinux: drop check for selabel_open signature change

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

All supported OSes have at least libselinux version 2.5 so it's safe
to drop this check.

Signed-off-by: Pavel Hrdina 
---
m4/virt-selinux.m4| 17 -
tests/securityselinuxhelper.c |  5 ++---
2 files changed, 2 insertions(+), 20 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 13/31] m4: virt-sanlock: drop check for sanlock_write_lockspace()

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Function sanlock_write_lockspace() was introduced in 2.7 version which
is available in all supported OSes.

Signed-off-by: Pavel Hrdina 
---
m4/virt-sanlock.m4| 11 ---
src/locking/lock_driver_sanlock.c | 19 ---
2 files changed, 30 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 12/31] m4: virt-sanlock: drop check for sanlock_killpath()

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

Function sanlock_killpath() was introduced in 2.4 version and had
modified one of the arguments from `char *` into `const char *` in
version 2.7. All of this is available in all supported OSes.

Signed-off-by: Pavel Hrdina 
---
m4/virt-sanlock.m4|  7 ---
src/locking/lock_driver_sanlock.c | 13 -
2 files changed, 20 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 10/31] m4: virt-sanlock: use pkg-config to find libsanlock_client

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

The last distribution supported by libvirt and lacking pkg-config file
for libsanlock_client was Ubuntu 16.04. It is no longer supported so
switch to pkg-config.

Signed-off-by: Pavel Hrdina 
---
m4/virt-sanlock.m4 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 08/31] docs: drop %.png: %.fig rule

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

convert bin is part of ImageMagick package and uses uniconvertor to
create png file from fig files.

Unfortunately uniconvertor is python2 only and not available in most
recent distributions which makes the convert command fail with:

sh: uniconvertor: command not found
/usr/bin/mv: cannot stat '/tmp/magick-1397138DRT8Pzx4Qmoc.svg': No such file or 
directory
convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' 
'%o'' @ error/delegate.c/InvokeDelegate/1958.
convert: unable to open file `/tmp/magick-1397138S8ARueJXLXkc': No such file or 
directory @ error/constitute.c/ReadImage/605.
convert: no images defined `docs/migration-managed-direct.png' @ 
error/convert.c/ConvertImageCommand/3226.

It looks like that there are plans to somehow port uniconvertor into
python3 but as part of different project color-picker but the job is
far from complete.

Signed-off-by: Pavel Hrdina 
---
docs/Makefile.am | 3 ---
1 file changed, 3 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 09/31] m4: virt-sanlock: drop check for sanlock_inq_lockspace

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

This check was introduced by commit
<96a02703daad4dc6663165adbc0feade9900cebd> to guard calling
sanlock_inq_lockspace() function but it used SANLK_INQ_WAIT as a
parameter which was introduced later.  This was eventually fixed by
commit <238dba0f9c925359cb3b8beddd8c8ae739cb4e06>.

We can safely replace check for sanlock_inq_lockspace as that function
was introduced in sanlock-1.9.  The oldest used version, sanlock-2.2,
is by Ubuntu 16.04.

Signed-off-by: Pavel Hrdina 
---
m4/virt-sanlock.m4| 12 
src/locking/lock_driver_sanlock.c |  2 +-
2 files changed, 5 insertions(+), 9 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 11/31] m4: virt-sanlock: drop check for SANLK_INQ_WAIT

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

SANLK_INQ_WAIT was introduced in sanlock 2.4 which is available in all
supported OSes.

Signed-off-by: Pavel Hrdina 
---
m4/virt-sanlock.m4| 9 -
src/locking/lock_driver_sanlock.c | 6 --
2 files changed, 15 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.

[Nicolás Brignone]

On Thu, Jul 9, 2020 at 12:15 AM Laine Stump  wrote:
>
> On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> >   All pointers to virXMLPropString() use g_autofree.
>
>
> I changed the summary line like this, to be more precise:
>
>
> conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
>
>
> > All modified functions are similar, in all cases "cleanup" label is removed,
> > along with all the "goto" calls.
>
>
> I've been advised in the recent past to put the g_autofree additions and
> corresponding removals of free functions into one patch, then do the
> removal of the extra labels (in favor of directly returning) in a
> separate patch to make it easier to hand-verify / review. Here's a

Makes sense, absolutely!

> couple recent examples:
>
>
> https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
>
>
> In your case the changes are few enough that I'm okay with it a single
> patch, except...
>
>
> >
> > Signed-off-by: Nicolas Brignone 
> > ---
> >   src/conf/device_conf.c | 183 +
> >   1 file changed, 56 insertions(+), 127 deletions(-)
> >
> > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > index 7d48a3f..9fa6141 100644
> > --- a/src/conf/device_conf.c
> > +++ b/src/conf/device_conf.c
> > @@ -208,45 +208,43 @@ int
> >   virPCIDeviceAddressParseXML(xmlNodePtr node,
> >   virPCIDeviceAddressPtr addr)
> >   {
> > -char *domain, *slot, *bus, *function, *multi;
> >   xmlNodePtr cur;
> >   xmlNodePtr zpci = NULL;
> > -int ret = -1;
> >
> >   memset(addr, 0, sizeof(*addr));
> >
> > -domain   = virXMLPropString(node, "domain");
> > -bus  = virXMLPropString(node, "bus");
> > -slot = virXMLPropString(node, "slot");
> > -function = virXMLPropString(node, "function");
> > -multi= virXMLPropString(node, "multifunction");
> > +g_autofree char *domain   = virXMLPropString(node, "domain");
> > +g_autofree char *bus  = virXMLPropString(node, "bus");
> > +g_autofree char *slot = virXMLPropString(node, "slot");
> > +g_autofree char *function = virXMLPropString(node, "function");
> > +g_autofree char *multi= virXMLPropString(node, "multifunction");
>
>
> ... you've modified it so that local variables are being declared
> *below* a line of executable code rather than at the top of the block.
> Although I don't see anything in the coding style document
> (https://libvirt.org/coding-style.html) about it, and it may just be
> leftover from a time when we supported a compiler that required all
> declarations to be at top of scope, I think pretty much all of libvirts
> code declares all local variables at the top of the scope.
>
> That's simple enough for me to just fixup before pushing, so

Thanks for that!

>
>
> Reviewed-by: Laine Stump 
>
>
> Congratulations on your first libvirt patch! :-)

Thanks to you for reviewing so quickly and for the precise feedback. Regards!

>
> >
> >   if (domain &&
> >   virStrToLong_uip(domain, NULL, 0, >domain) < 0) {
> >   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >  _("Cannot parse  'domain' attribute"));
> > -goto cleanup;
> > +return -1;
> >   }
> >
> >   if (bus &&
> >   virStrToLong_uip(bus, NULL, 0, >bus) < 0) {
> >   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >  _("Cannot parse  'bus' attribute"));
> > -goto cleanup;
> > +return -1;
> >   }
> >
> >   if (slot &&
> >   virStrToLong_uip(slot, NULL, 0, >slot) < 0) {
> >   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >  _("Cannot parse  'slot' attribute"));
> > -goto cleanup;
> > +return -1;
> >   }
> >
> >   if (function &&
> >   virStrToLong_uip(function, NULL, 0, >function) < 0) {
> >   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >  _("Cannot parse  'function' attribute"));
> > -goto cleanup;
> > +return -1;
> >   }
> >
> >   if (multi &&
> > @@ -254,11 +252,11 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> >   virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >  _("Unknown value '%s' for  
> > 'multifunction' attribute"),
> >  multi);
> > -goto cleanup;
> > +return -1;
> >
> >   }
> >   if (!virPCIDeviceAddressIsEmpty(addr) && 
> > !virPCIDeviceAddressIsValid(addr, true))
> > -goto cleanup;
> > +return -1;
> >
> >   cur = node->children;
> >   while (cur) {
> > @@ -270,17 +268,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
> >   }
> >
> >   if (zpci && virZPCIDeviceAddressParseXML(zpci, addr) < 0)
> > -goto cleanup;
> > +return -1;
> >
> > -ret = 0;
> > -
> > - cleanup:
> > -VIR_FREE(domain);
> > -VIR_FREE(bus);
> > -

Re: [libvirt PATCH 06/31] docs: remove incorrect generated files by apibuild.py

[Ján Tomko]

On a Thursday in 2020, Pavel Hrdina wrote:

These files are generated by xsltproc as part of html/index.html and
html/index-%.html rules.

Signed-off-by: Pavel Hrdina 
---
docs/Makefile.am | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH 00/31] fixes and cleanups for current build system

[Pavel Hrdina]

On Thu, Jul 02, 2020 at 02:24:58PM +0200, Pavel Hrdina wrote:
> While working on rewrite to Meson I discovered some parts of our
> current build system that could be improved to help with the
> transition to Meson. It will make the review of the Meson patches
> a bit easier.
> 
> Pavel Hrdina (31):
>   build: use DLOPEN_LIBS directly
>   configure: drop check for unsupported FreeBSD
>   configure: introduce FLAT_NAMESPACE_FLAGS
>   configure: remove usage of AC_HEADER_MAJOR
>   Makefile: drop undefined LIB_CLOCK_GETTIME
>   docs: remove incorrect generated files by apibuild.py
>   docs: remove unused wrapstring.xsl file
>   docs: drop %.png: %.fig rule
>   m4: virt-sanlock: drop check for sanlock_inq_lockspace
>   m4: virt-sanlock: use pkg-config to find libsanlock_client
>   m4: virt-sanlock: drop check for SANLK_INQ_WAIT
>   m4: virt-sanlock: drop check for sanlock_killpath()
>   m4: virt-sanlock: drop check for sanlock_write_lockspace()
>   m4: virt-secdriver-selinux: drop obsolete function checks
>   m4: virt-selinux: drop check for selabel_open signature change
>   m4: virt-xdr: rewrite XDR check
>   po: change the format of POTFILES.in
>   scripts: check-remote-protocol: remove unused OBJEXT argument
>   src: Makefile: remove LOCK_CHECKING_CFLAGS leftover
>   src: remove unnecessary -I$(srcdir)/secret include
>   src: remote: Makefile: drop CFLAGS and LDFLAGS duplication
>   src: logging: Makefile: drop undefined LOG_DRIVER
>   src: util: rename some program macros
>   src: util: Makefile: drop undefined LDEXP_LIBM
>   src: util: Makefile: drop undefined OPENPTY_LIBS
>   src: remote: Makefile: properly format sysconfdir in virtproxyd.conf
>   src: unify virFileActivateDirOverride()
>   tests: commandhelper: change how we detect if running as daemon
>   tests: use WITH_NSS instead of NSS
>   tools: virsh-secret: fix compilation error
>   tools: wireshark: fix compilation errors

Ping?

I know that this is not that popular part of libvirt code-base to work
on but it would be nice to have it pushed quickly so I can send a
followup series that switches from autoconf to Meson and it depends on
these cleanup patches.

Pavel


signature.asc
Description: PGP signature

libvirt opens kernel+initrd in read-write mode

[Olaf Hering]

Is there a reason why libvirtd v6.5.0 opens kernel+initrd in mode RW?
'virsh start vm' fails of both are on a read-only filesystem.
Not sure if this ever worked before.


  
hvm
/path/to/kernel
/path/to/initrd
net.ifnames=0 console=ttyS0,115200 linemode=1 panic=9 
start_shell

  



13111 stat("/path/to/initrd", {st_mode=S_IFREG|0755, st_size=119207064, ...}) = 0
13111 openat(AT_FDCWD, "/path/to/initrd", O_RDWR) = -1 EROFS (Read-only file 
system)


Olaf


pgpPolfl7yCbl.pgp
Description: Digitale Signatur von OpenPGP

Setting 'nodatacow' on VM image when on Btrfs

[Chris Murphy]

It's generally recommended by upstream Btrfs development to set
'nodatacow' using 'chattr +C' on the containing directory for VM
images. By setting it on the containing directory it means new VM
images inherit the attribute, including images copied to this
location.

But 'nodatacow' also implies no compression and no data checksums.
Could there be use cases in which it's preferred to do compression and
checksumming on VM images? In that case the fragmentation problem can
be mitigated by periodic defragmentation.

Is this something libvirt can and should do? Possibly by default with
a way for users to opt out?

Another option is to have the installer set 'chattr +C' in the short
term. But this doesn't help GNOME Boxes, since the user home isn't
created at installation time.

Three advantages of libvirt awareness of Btrfs:

(a) GNOME Boxes Cockpit, and other users of libvirt can then use this
same mechanism, and apply to their VM image locations.

(b) Create the containing directory as a subvolume instead of a directory
(1) btrfs snapshots are not recursive, therefore making this a
subvolume would prevent it from being snapshot, and thus (temporarily)
resuming datacow.
(2) in heavy workloads there can be lock contention on file
btrees; a subvolume is a dedicated file btree; so this would reduce
tendency for lock contention in heavy workloads (this is not likely a
desktop/laptop use case)

(c) virtiofs might be able to take advantage of btrfs subvolumes.


(This is a partial rewrite of
https://bugzilla.redhat.com/show_bug.cgi?id=1855000)

Thanks,

-- 
Chris Murphy

Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Peter Xu]

On Thu, Jul 09, 2020 at 01:58:33PM +0800, Jason Wang wrote:
> > > - If we care the performance, it's better to implement the MAP event for
> > > vhost, otherwise it could be a lot of IOTLB miss
> > I feel like these are two things.
> > 
> > So far what we are talking about is whether vt-d should have knowledge about
> > what kind of events one iommu notifier is interested in.  I still think we
> > should keep this as answered in question 1.
> > 
> > The other question is whether we want to switch vhost from UNMAP to 
> > MAP/UNMAP
> > events even without vDMA, so that vhost can establish the mapping even 
> > before
> > IO starts.  IMHO it's doable, but only if the guest runs DPDK workloads.  
> > When
> > the guest is using dynamic iommu page mappings, I feel like that can be even
> > slower, because then the worst case is for each IO we'll need to vmexit 
> > twice:
> > 
> >- The first vmexit caused by an invalidation to MAP the page tables, so 
> > vhost
> >  will setup the page table before IO starts
> > 
> >- IO/DMA triggers and completes
> > 
> >- The second vmexit caused by another invalidation to UNMAP the page 
> > tables
> > 
> > So it seems to be worse than when vhost only uses UNMAP like right now.  At
> > least we only have one vmexit (when UNMAP).  We'll have a vhost translate()
> > request from kernel to userspace, but IMHO that's cheaper than the vmexit.
> 
> 
> Right but then I would still prefer to have another notifier.
> 
> Since vtd_page_walk has nothing to do with device IOTLB. IOMMU have a
> dedicated command for flushing device IOTLB. But the check for
> vtd_as_has_map_notifier is used to skip the device which can do demand
> paging via ATS or device specific way. If we have two different notifiers,
> vhost will be on the device iotlb notifier so we don't need it at all?

But we can still have iommu notifier that only registers to UNMAP even after we
introduce dev-iotlb notifier?  We don't want to do page walk for them as well.
TCG should be the only one so far, but I don't know.. maybe there can still be
new ones?

Thanks,

-- 
Peter Xu

Re: [PATCH] resctrl: Do not open directory for writing

[Martin Kletzander]

On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:

On Thu, 2020-07-09 at 11:40 +0200, Martin Kletzander wrote:

 static int
 virResctrlLockWrite(void)
 {
-int fd = open(SYSFS_RESCTRL_PATH, O_RDWR | O_CLOEXEC);
+int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);


I got curious, so I did some digging.

The commit message for 18dca21a32e9 says

 The O_DIRECTORY flag causes open() to return an error if the
 filename is a directory. There's no obvious reason why resctrl
 needs to use this [...]



I think that was a typo, yes.


but open(2) claims

 O_DIRECTORY
   If pathname is not a directory, cause the open to fail.

instead, so using O_DIRECTORY in the first place doesn't seem at all
unreasonable: the resctrl mount path *is* going to be a directory. I
guess we don't need to be too paranoid about it, though, so I don't
think removing the flag is a big issue.



The main reason for removing it was that it is not available on other platforms,
I think it is linux-only and gnulib was handling other platforms for us.  We
don't really care if it is a directory or not.  The very next read somewhere in
that path will fail if it is not a directory.  We only need to make sure we use
flock(2) as that is the lock used for mutual exclusion on resctrl.  Yes, it is
Linux-only, just as O_DIRECTORY, and same as resctrl itself.  At least to my
knowledge.  We could also wrap it in an #ifdef clause, but there is no need for
it, really.  virFileFlock() will currently fail with ENOSYS for non-Linux (or
non-__linux__, to be overly specific).


Now, when it comes to opening R/O or R/W, I agree that using the
latter it was not the right choice and it altered the behavior, but
I assume the decision was influenced by the name of the function,
virResctrlLockWrite().



That could've been, although it refers to a write (exclusive) lock.


I looked back through the file's history to see whether that name was
justified by virResctrlLockRead() existing at some point, but that
doesn't seem to be the case.


So, funny[0] story, there was.  It was just never committed because i could not
use it in the end.  The only usage would be for reporting some information in
capabilities for example.  Since flock(2) does not have lock promotion (and it's
always an issue anyway) reading information has to use a write lock if we are
going to write something based on the values we read, otherwise the data we
would base the settings on could change in the meantime.

It's not a proof, but you can see that by looking up virResctrlLockInternal()
which used to be the function deduplicating same code between read and write
locking.


I guess the name was inspired by virRWLockWrite()? But since there's no "read"
equivalent, it seems that the simpler virResctrlLock() would have worked just
as fine. But I digress.



It should be just Lock, no Write, it's confusing, I agree.


The more interesting thing I noticed is that in 657ddeff2313, the
locking call was changed from

 flock(fd, LOCK_EX)

to

 virFileFlock(fd, true, true)



Yes, that was part of a clean-up that did not get in as a whole, IIRC.  Further
information available only on in-person requests and enough alcohol nearby[1].


and given the documentation for virFileFlock()

 /**
  * virFileFlock:
  * @fd: file descriptor to call flock on
  * @lock: true for lock, false for unlock
  * @shared: true if shared, false for exclusive, ignored if
  *  `@lock == false`
  *
  * This is just a simple wrapper around flock(2) that errors out
  * on unsupported platforms.
  *
  * The lock will be released when @fd is closed or this function
  * is called with `@lock == false`.
  *
  * Returns 0 on success, -1 otherwise (with errno set)
  */
 int virFileFlock(int fd, bool lock, bool shared)

it seems to me that this change entirely flipped the semantics of
the lock.



Yep, you're right.  When you have a lock that has boolean as a parameter I think
the boolean should indicate whether the lock is writable, but maybe the proper
and most readable solution would be virFileFlockShareable() and
virFileFlockExclusive() to avoid any misconception in the future[2].  Given the
fact that there are no (and most probably won't be) any other users of flock(2)
and given the fact that resctrl is also pretty niche feature, I don't have any
preference.  Also I feel like after some time I'm a little bit rusty with C and
the libvirt codebase (and most importantly the reasoning and decisions) has
changed a bit, so I'll leave the decision on how to deal with that on someone
else.  I'm happy to provide the patches when clear decision is made.


tl;dr

 Reviewed-by: Andrea Bolognani 



Thanks, I'm pushing this one (let's see if I screw it up or not) and will
prepare for the next ones.


for this patch, but we probably need to change the second argument
of the virFileFlock() call and maybe consider renaming the function.



[0] not really funny at all
[1] drink responsibly or not at all
[2] well, apart 

Re: [PATCHv2] Add a changelog entry for the esx NIC limit changes

[Andrea Bolognani]

On Thu, 2020-07-09 at 12:30 +0200, Bastien Orivel wrote:
> This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8
> 
> Signed-off-by: Bastien Orivel 
> ---
>  NEWS.rst | 5 +
>  1 file changed, 5 insertions(+)

Looks good now!

  Reviewed-by: Andrea Bolognani 

and pushed. Thanks :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] resctrl: Do not open directory for writing

[Andrea Bolognani]

On Thu, 2020-07-09 at 11:40 +0200, Martin Kletzander wrote:
>  static int
>  virResctrlLockWrite(void)
>  {
> -int fd = open(SYSFS_RESCTRL_PATH, O_RDWR | O_CLOEXEC);
> +int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);

I got curious, so I did some digging.

The commit message for 18dca21a32e9 says

  The O_DIRECTORY flag causes open() to return an error if the
  filename is a directory. There's no obvious reason why resctrl
  needs to use this [...]

but open(2) claims

  O_DIRECTORY
If pathname is not a directory, cause the open to fail.

instead, so using O_DIRECTORY in the first place doesn't seem at all
unreasonable: the resctrl mount path *is* going to be a directory. I
guess we don't need to be too paranoid about it, though, so I don't
think removing the flag is a big issue.

Now, when it comes to opening R/O or R/W, I agree that using the
latter it was not the right choice and it altered the behavior, but
I assume the decision was influenced by the name of the function,
virResctrlLockWrite().

I looked back through the file's history to see whether that name was
justified by virResctrlLockRead() existing at some point, but that
doesn't seem to be the case. I guess the name was inspired by
virRWLockWrite()? But since there's no "read" equivalent, it seems
that the simpler virResctrlLock() would have worked just as fine. But
I digress.

The more interesting thing I noticed is that in 657ddeff2313, the
locking call was changed from

  flock(fd, LOCK_EX)

to

  virFileFlock(fd, true, true)

and given the documentation for virFileFlock()

  /**
   * virFileFlock:
   * @fd: file descriptor to call flock on
   * @lock: true for lock, false for unlock
   * @shared: true if shared, false for exclusive, ignored if
   *  `@lock == false`
   *
   * This is just a simple wrapper around flock(2) that errors out
   * on unsupported platforms.
   *
   * The lock will be released when @fd is closed or this function
   * is called with `@lock == false`.
   *
   * Returns 0 on success, -1 otherwise (with errno set)
   */
  int virFileFlock(int fd, bool lock, bool shared)

it seems to me that this change entirely flipped the semantics of
the lock.

tl;dr

  Reviewed-by: Andrea Bolognani 

for this patch, but we probably need to change the second argument
of the virFileFlock() call and maybe consider renaming the function.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] Add a changelog entry for the esx NIC limit changes

[Peter Krempa]

On Thu, Jul 09, 2020 at 12:18:16 +0200, Bastien Orivel wrote:
> 
> 
> On 09/07/2020 12:10, Andrea Bolognani wrote:
> > On Thu, 2020-07-09 at 11:25 +0200, Bastien Orivel wrote:
> >> This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8
> > You're missing the Signed-off-by tag...
> Sorry, not used to using that git feature.
> >
> >>  * **Improvements**
> >>  
> >> +   * esx: Change the NIC limit for recent virtualHW versions
> >> +
> >> +   Specifying a virtualHW version greater or equal to 7 (ESXi 4.0) will 
> >> allow
> >> +   you to use up to 10 NICs instead of 4 as it was previously.
> > ... and indentation is off (see existing entries for comparison).
> >
> > I can fix both before pushing if you're okay with that, or you could
> > repost, whatever is better for you.
> >
> If you can fix it for me it'll avoid me battling git send-email for a
> while to find out how to send updated patches.

Oops, hit send too soon.

You must do the certification of the DCO explicitly, e.g. by replying
with your signoff here at least. Otherwise we can't add the tag on your
behalf to the commit.

[PATCHv2] Add a changelog entry for the esx NIC limit changes

[Bastien Orivel]

This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8

Signed-off-by: Bastien Orivel 
---
 NEWS.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index cc35cb26b2..1928220854 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -20,6 +20,11 @@ v6.6.0 (unreleased)
 
 * **Improvements**
 
+  * esx: Change the NIC limit for recent virtualHW versions
+
+Specifying a virtualHW version greater or equal to 7 (ESXi 4.0) will allow
+you to use up to 10 NICs instead of 4 as it was previously.
+
 * **Bug fixes**
 
 
-- 
2.20.1

Re: [PATCH] Add a changelog entry for the esx NIC limit changes

[Peter Krempa]

On Thu, Jul 09, 2020 at 12:18:16 +0200, Bastien Orivel wrote:
> 
> 
> On 09/07/2020 12:10, Andrea Bolognani wrote:
> > On Thu, 2020-07-09 at 11:25 +0200, Bastien Orivel wrote:
> >> This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8
> > You're missing the Signed-off-by tag...
> Sorry, not used to using that git feature.

Note that the presence of the 'Signed-off' tag is not just a git feature
but a certification that you are submitting the commit in accordance
with the developer certificate of origin:

https://developercertificate.org/

Re: [PATCH] Add a changelog entry for the esx NIC limit changes

[Bastien Orivel]

On 09/07/2020 12:10, Andrea Bolognani wrote:
> On Thu, 2020-07-09 at 11:25 +0200, Bastien Orivel wrote:
>> This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8
> You're missing the Signed-off-by tag...
Sorry, not used to using that git feature.
>
>>  * **Improvements**
>>  
>> +   * esx: Change the NIC limit for recent virtualHW versions
>> +
>> +   Specifying a virtualHW version greater or equal to 7 (ESXi 4.0) will 
>> allow
>> +   you to use up to 10 NICs instead of 4 as it was previously.
> ... and indentation is off (see existing entries for comparison).
>
> I can fix both before pushing if you're okay with that, or you could
> repost, whatever is better for you.
>
If you can fix it for me it'll avoid me battling git send-email for a
while to find out how to send updated patches.

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Paolo Bonzini]

On 09/07/20 11:55, Mohammed Gamal wrote:
>> Ideally we would simply outlaw (3), but it's hard for backward
>> compatibility reasons.  Second best solution is a flag somewhere
>> (msr, cpuid, ...) telling the guest firmware "you can use
>> GUEST_MAXPHYADDR, we guarantee it is <= HOST_MAXPHYADDR".
> Problem is GUEST_MAXPHYADDR > HOST_MAXPHYADDR is actually a supported
> configuration on some setups. Namely when memory encryption is enabled
> on AMD CPUs[1].
> 

It's not that bad since there's two MAXPHYADDRs, the one in CPUID and
the one computed internally by the kernel.  GUEST_MAXPHYADDR greater
than the host CPUID maxphyaddr is never supported.

Paolo

Re: [PATCH] Add a changelog entry for the esx NIC limit changes

[Andrea Bolognani]

On Thu, 2020-07-09 at 11:25 +0200, Bastien Orivel wrote:
> This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8

You're missing the Signed-off-by tag...

>  * **Improvements**
>  
> +   * esx: Change the NIC limit for recent virtualHW versions
> +
> +   Specifying a virtualHW version greater or equal to 7 (ESXi 4.0) will allow
> +   you to use up to 10 NICs instead of 4 as it was previously.

... and indentation is off (see existing entries for comparison).

I can fix both before pushing if you're okay with that, or you could
repost, whatever is better for you.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Mohammed Gamal]

On Thu, 2020-07-09 at 11:44 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > (CCing libvir-list, and people who were included in the OVMF
> > > thread[1])
> > > 
> > > [1] 
> > > https://lore.kernel.org/qemu-devel/99779e9c-f05f-501b-b4be-ff719f140...@canonical.com/
> > > Also, it's important that we work with libvirt and management
> > > software to ensure they have appropriate APIs to choose what to
> > > do when a cluster has hosts with different MAXPHYADDR.
> > 
> > There's been so many complex discussions that it is hard to have
> > any
> > understanding of what we should be doing going forward. There's
> > enough
> > problems wrt phys bits, that I think we would benefit from a doc
> > that
> > outlines the big picture expectation for how to handle this in the
> > virt stack.
> 
> Well, the fundamental issue is not that hard actually.  We have three
> cases:
> 
> (1) GUEST_MAXPHYADDR == HOST_MAXPHYADDR
> 
> Everything is fine ;)
> 
> (2) GUEST_MAXPHYADDR < HOST_MAXPHYADDR
> 
> Mostly fine.  Some edge cases, like different page fault errors
> for
> addresses above GUEST_MAXPHYADDR and below
> HOST_MAXPHYADDR.  Which I
> think Mohammed fixed in the kernel recently.
> 
> (3) GUEST_MAXPHYADDR > HOST_MAXPHYADDR
> 
> Broken.  If the guest uses addresses above HOST_MAXPHYADDR
> everything
> goes south.
> 
> The (2) case isn't much of a problem.  We only need to figure
> whenever
> we want qemu allow this unconditionally (current state) or only in
> case
> the kernel fixes are present (state with this patch applied if I read
> it
> correctly).
> 
> The (3) case is the reason why guest firmware never ever uses
> GUEST_MAXPHYADDR and goes with very conservative heuristics instead,
> which in turn leads to the consequences discussed at length in the
> OVMF thread linked above.
> 
> Ideally we would simply outlaw (3), but it's hard for backward
> compatibility reasons.  Second best solution is a flag somewhere
> (msr, cpuid, ...) telling the guest firmware "you can use
> GUEST_MAXPHYADDR, we guarantee it is <= HOST_MAXPHYADDR".

Problem is GUEST_MAXPHYADDR > HOST_MAXPHYADDR is actually a supported
configuration on some setups. Namely when memory encryption is enabled
on AMD CPUs[1].

> 
> > As mentioned in the thread quoted above, using host_phys_bits is a
> > obvious thing to do when the user requested "-cpu host".
> > 
> > The harder issue is how to handle other CPU models. I had suggested
> > we should try associating a phys bits value with them, which would
> > probably involve creating Client/Server variants for all our CPU
> > models which don't currently have it. I still think that's worth
> > exploring as a strategy and with versioned CPU models we should
> > be ok wrt back compatibility with that approach.
> 
> Yep, better defaults for GUEST_MAXPHYADDR would be good too, but that
> is a separate (although related) discussion.
> 
> take care,
>   Gerd
> 
[1] - https://lkml.org/lkml/2020/6/19/2371

Re: [PATCH] resctrl: Do not open directory for writing

[Ján Tomko]

On a Thursday in 2020, Martin Kletzander wrote:

When preparing for the removal of GNULIB commit 18dca21a32e9 removed the
unneeded O_DIRECTORY, but unfortunately started opening the directory for
writing which fails every time for a directory.  There is also no need for that
as flock() works on O_RDONLY file descriptor as well, even for LOCK_EX.

https://bugzilla.redhat.com/1852741

Signed-off-by: Martin Kletzander 
---
src/util/virresctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH 2/2] x86/cpu: Handle GUEST_MAXPHYADDR < HOST_MAXPHYADDR for hosts that don't support it

[Gerd Hoffmann]

  Hi,

> > (CCing libvir-list, and people who were included in the OVMF
> > thread[1])
> > 
> > [1] 
> > https://lore.kernel.org/qemu-devel/99779e9c-f05f-501b-b4be-ff719f140...@canonical.com/

> > Also, it's important that we work with libvirt and management
> > software to ensure they have appropriate APIs to choose what to
> > do when a cluster has hosts with different MAXPHYADDR.
> 
> There's been so many complex discussions that it is hard to have any
> understanding of what we should be doing going forward. There's enough
> problems wrt phys bits, that I think we would benefit from a doc that
> outlines the big picture expectation for how to handle this in the
> virt stack.

Well, the fundamental issue is not that hard actually.  We have three
cases:

(1) GUEST_MAXPHYADDR == HOST_MAXPHYADDR

Everything is fine ;)

(2) GUEST_MAXPHYADDR < HOST_MAXPHYADDR

Mostly fine.  Some edge cases, like different page fault errors for
addresses above GUEST_MAXPHYADDR and below HOST_MAXPHYADDR.  Which I
think Mohammed fixed in the kernel recently.

(3) GUEST_MAXPHYADDR > HOST_MAXPHYADDR

Broken.  If the guest uses addresses above HOST_MAXPHYADDR everything
goes south.

The (2) case isn't much of a problem.  We only need to figure whenever
we want qemu allow this unconditionally (current state) or only in case
the kernel fixes are present (state with this patch applied if I read it
correctly).

The (3) case is the reason why guest firmware never ever uses
GUEST_MAXPHYADDR and goes with very conservative heuristics instead,
which in turn leads to the consequences discussed at length in the
OVMF thread linked above.

Ideally we would simply outlaw (3), but it's hard for backward
compatibility reasons.  Second best solution is a flag somewhere
(msr, cpuid, ...) telling the guest firmware "you can use
GUEST_MAXPHYADDR, we guarantee it is <= HOST_MAXPHYADDR".

> As mentioned in the thread quoted above, using host_phys_bits is a
> obvious thing to do when the user requested "-cpu host".
> 
> The harder issue is how to handle other CPU models. I had suggested
> we should try associating a phys bits value with them, which would
> probably involve creating Client/Server variants for all our CPU
> models which don't currently have it. I still think that's worth
> exploring as a strategy and with versioned CPU models we should
> be ok wrt back compatibility with that approach.

Yep, better defaults for GUEST_MAXPHYADDR would be good too, but that
is a separate (although related) discussion.

take care,
  Gerd

[PATCH] resctrl: Do not open directory for writing

[Martin Kletzander]

When preparing for the removal of GNULIB commit 18dca21a32e9 removed the
unneeded O_DIRECTORY, but unfortunately started opening the directory for
writing which fails every time for a directory.  There is also no need for that
as flock() works on O_RDONLY file descriptor as well, even for LOCK_EX.

https://bugzilla.redhat.com/1852741

Signed-off-by: Martin Kletzander 
---
 src/util/virresctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 33044da3a1ef..f6da13645ae3 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -456,7 +456,7 @@ VIR_ONCE_GLOBAL_INIT(virResctrl);
 static int
 virResctrlLockWrite(void)
 {
-int fd = open(SYSFS_RESCTRL_PATH, O_RDWR | O_CLOEXEC);
+int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
 
 if (fd < 0) {
 virReportSystemError(errno, "%s", _("Cannot open resctrl"));
-- 
2.27.0

[PATCH] Add a changelog entry for the esx NIC limit changes

[Bastien Orivel]

This was forgotten in 4bd375b6ce3a4c134bab19cd7c9a7a83609547c8
---
 NEWS.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index cc35cb26b2..ce5d227068 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -20,6 +20,11 @@ v6.6.0 (unreleased)
 
 * **Improvements**
 
+   * esx: Change the NIC limit for recent virtualHW versions
+
+   Specifying a virtualHW version greater or equal to 7 (ESXi 4.0) will allow
+   you to use up to 10 NICs instead of 4 as it was previously.
+
 * **Bug fixes**
 
 
-- 
2.20.1

Re: [GSoC][PATCH v2] qemu_domainjob: removal of its dependency on other qemu-files

[Michal Privoznik]

On 7/8/20 8:33 PM, Prathamesh Chavan wrote:

It was seen that `qemu_domain.h` file depended upon
`qemu_migration_params.h` and `qmeu_monitor.h` as they
were required by some qemu_domainjob stuctures.
This dependency was removed by the introduction of
a `void *privateData` pointer. This privateData pointer
was handled using a structure of callback functions.

Additionally, the patch also moves funcitons
`qemuDomainObjPrivateXMLFormatJob` and
`qemuDomainObjPrivateXMLParseJob` from `qemu_domain`
and handles them using the callback structure of
domain jobs.

Signed-off-by: Prathamesh Chavan 
---
Previous version of this patch can be found here[1].

This patch adds a funciton to the domainJobInfo callback
structure to specifically to copy the jobInfo privateData
structure.

Also, it was noticed that qemuDomainNamespace was reciding
in `qmeu_domainjob.c`, and should rather be in its original file
`qmeu_domain.c`. hence was moved.

  src/qemu/qemu_backup.c   |  13 +-
  src/qemu/qemu_domain.c   | 251 +---
  src/qemu/qemu_domainjob.c| 386 ---
  src/qemu/qemu_domainjob.h|  69 --
  src/qemu/qemu_driver.c   |  21 +-
  src/qemu/qemu_migration.c|  45 ++--
  src/qemu/qemu_migration_cookie.c |   7 +-
  src/qemu/qemu_migration_params.c |   9 +-
  src/qemu/qemu_migration_params.h |  28 +++
  src/qemu/qemu_process.c  |  24 +-
  10 files changed, 515 insertions(+), 338 deletions(-)


This is very large change. Can it be split into smaller patches so that 
it is easier to review? There are several things happening at the same 
time hence I think it should be possible.


Michal

Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.

[Daniel P . Berrangé]

On Wed, Jul 08, 2020 at 11:15:27PM -0400, Laine Stump wrote:
> On 7/8/20 4:19 PM, Nicolas Brignone wrote:
> >   All pointers to virXMLPropString() use g_autofree.
> 
> 
> I changed the summary line like this, to be more precise:
> 
> 
> conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c
> 
> 
> > All modified functions are similar, in all cases "cleanup" label is removed,
> > along with all the "goto" calls.
> 
> 
> I've been advised in the recent past to put the g_autofree additions and
> corresponding removals of free functions into one patch, then do the removal
> of the extra labels (in favor of directly returning) in a separate patch to
> make it easier to hand-verify / review. Here's a couple recent examples:
> 
> 
> https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html
> 
> 
> In your case the changes are few enough that I'm okay with it a single
> patch, except...
> 
> 
> > 
> > Signed-off-by: Nicolas Brignone 
> > ---
> >   src/conf/device_conf.c | 183 +
> >   1 file changed, 56 insertions(+), 127 deletions(-)
> > 
> > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> > index 7d48a3f..9fa6141 100644
> > --- a/src/conf/device_conf.c
> > +++ b/src/conf/device_conf.c
> > @@ -208,45 +208,43 @@ int
> >   virPCIDeviceAddressParseXML(xmlNodePtr node,
> >   virPCIDeviceAddressPtr addr)
> >   {
> > -char *domain, *slot, *bus, *function, *multi;
> >   xmlNodePtr cur;
> >   xmlNodePtr zpci = NULL;
> > -int ret = -1;
> >   memset(addr, 0, sizeof(*addr));
> > -domain   = virXMLPropString(node, "domain");
> > -bus  = virXMLPropString(node, "bus");
> > -slot = virXMLPropString(node, "slot");
> > -function = virXMLPropString(node, "function");
> > -multi= virXMLPropString(node, "multifunction");
> > +g_autofree char *domain   = virXMLPropString(node, "domain");
> > +g_autofree char *bus  = virXMLPropString(node, "bus");
> > +g_autofree char *slot = virXMLPropString(node, "slot");
> > +g_autofree char *function = virXMLPropString(node, "function");
> > +g_autofree char *multi= virXMLPropString(node, "multifunction");
> 
> 
> ... you've modified it so that local variables are being declared *below* a
> line of executable code rather than at the top of the block. Although I
> don't see anything in the coding style document
> (https://libvirt.org/coding-style.html) about it, and it may just be
> leftover from a time when we supported a compiler that required all
> declarations to be at top of scope, I think pretty much all of libvirts code
> declares all local variables at the top of the scope.

We should really document it, and even enforce it at compile time.
When you have variables in the middle of a function, and do a goto
jump over them, they are in scope at the goto label target, but
you may have jumped over their initialization. This makes it hard
to rationalize about correctness of the code and has led to bugs
involving uninitialized data before.

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] src: fix word spell typos

[Ján Tomko]

On a Thursday in 2020, Fangge Jin wrote:

Signed-off-by: Fangge Jin 
---
src/bhyve/libvirtd_bhyve.aug   | 2 +-
src/cpu/cpu_arm.c  | 4 ++--
src/esx/esx_vi.c   | 2 +-
src/interface/interface_backend_udev.c | 4 ++--
src/internal.h | 2 +-
src/libvirt-domain.c   | 2 +-
src/libvirt-nodedev.c  | 2 +-
src/libxl/libxl_conf.c | 2 +-
src/libxl/libxl_driver.c   | 2 +-
src/libxl/libxl_logger.c   | 2 +-
src/locking/libvirt_lockd.aug  | 2 +-
src/locking/libvirt_sanlock.aug| 2 +-
src/locking/virtlockd.aug  | 2 +-
src/logging/virtlogd.aug   | 2 +-
src/lxc/libvirtd_lxc.aug   | 2 +-
src/lxc/lxc_native.c   | 2 +-
src/lxc/lxc_process.c  | 2 +-
src/node_device/node_device_udev.c | 2 +-
src/qemu/qemu_agent.c  | 2 +-
src/qemu/qemu_blockjob.c   | 2 +-
src/qemu/qemu_capabilities.c   | 4 ++--
src/qemu/qemu_command.c| 4 ++--
src/qemu/qemu_conf.c   | 2 +-
src/qemu/qemu_domain.c | 8 
src/qemu/qemu_domain_address.c | 2 +-
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_hotplug.c| 2 +-
src/qemu/qemu_migration.c  | 2 +-
src/qemu/qemu_monitor.c| 2 +-
src/qemu/qemu_qapi.c   | 4 ++--
src/qemu/qemu_security.c   | 2 +-
src/remote/libvirtd.aug.in | 2 +-
src/remote/remote_daemon_dispatch.c| 2 +-
src/rpc/virnetlibsshsession.c  | 4 ++--
src/rpc/virnetserverprogram.c  | 2 +-
src/rpc/virnetsshsession.c | 4 ++--
src/storage/storage_util.c | 4 ++--
src/util/virbuffer.h   | 2 +-
src/util/vircommand.c  | 2 +-
src/util/virdnsmasq.c  | 2 +-
src/util/virhostcpu.c  | 2 +-
src/util/virnetdev.c   | 2 +-
src/util/virnetdevopenvswitch.c| 4 ++--
src/util/virnetdevtap.c| 2 +-
src/util/virqemu.c | 2 +-
src/util/virresctrl.c  | 2 +-
src/util/virsysinfo.c  | 2 +-
src/util/virsystemd.c  | 2 +-
src/util/virtpm.c  | 2 +-
src/vbox/vbox_common.h | 2 +-
src/vbox/vbox_network.c| 4 ++--
src/vbox/vbox_snapshot_conf.c  | 4 ++--
src/vbox/vbox_tmpl.c   | 2 +-
src/vmx/vmx.c  | 2 +-
src/vz/vz_driver.c | 2 +-
src/vz/vz_sdk.c| 4 ++--
56 files changed, 72 insertions(+), 72 deletions(-)

diff --git a/src/bhyve/libvirtd_bhyve.aug b/src/bhyve/libvirtd_bhyve.aug
index 66079376c4..b6bee261a6 100644
--- a/src/bhyve/libvirtd_bhyve.aug
+++ b/src/bhyve/libvirtd_bhyve.aug
@@ -24,7 +24,7 @@ module Libvirtd_bhyve =

   let log_entry = str_entry "firmware_dir"

-   (* Each enty in the config is one of the following three ... *)
+   (* Each entry in the config is one of the following three ... *)


I can't believe we copied this typo so many times.


   let entry = log_entry
   let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . 
del /\n/ "\n" ]
   let empty = [ label "#empty" . eol ]
diff --git a/src/libxl/libxl_logger.c b/src/libxl/libxl_logger.c
index 379d7a8d00..58b6e7bfa1 100644
--- a/src/libxl/libxl_logger.c
+++ b/src/libxl/libxl_logger.c
@@ -119,7 +119,7 @@ libvirt_progress(xentoollog_logger *logger_in G_GNUC_UNUSED,
 unsigned long done G_GNUC_UNUSED,
 unsigned long total G_GNUC_UNUSED)
{
-/* This function purposedly does nothing: it's no logging info */
+/* This function purposely does nothing: it's no logging info */


purposedly is a valid word, I'm dropping this hunk before pushing.


}

static void
diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c
index b4dea15452..752c3fc915 100644
--- a/src/rpc/virnetsshsession.c
+++ b/src/rpc/virnetsshsession.c
@@ -681,7 +681,7 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess,
VIR_DEBUG("sess=%p", sess);

if (priv->password) {
-/* tunelled password authentication */
+/* tunnelled password authentication */


I would not really call this wrong - it's just US vs UK spelling.
But since our API flag uses TUNNELLED, I'll leave these in.


if ((rc = libssh2_userauth_password(sess->session,
priv->username,
priv->password)) == 0) {


Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] Replace "OS-X" with "macOS" in index.html.in

[Ján Tomko]

On a Thursday in 2020, Ján Tomko wrote:

From: Ryan Schmidt 

Apple changed the operating system's name from "OS X" to "macOS" a few years 
ago.

Signed-off-by: Ryan Schmidt 
Reviewed-by: Ján Tomko 
Signed-off-by: Ján Tomko 
---
docs/index.html.in | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)



Forgot to say that this trivial patch was sent via a merge request:
https://gitlab.com/libvirt/libvirt/-/merge_requests/12/

Jano


diff --git a/docs/index.html.in b/docs/index.html.in
index 586defff54..20184a7441 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -27,7 +27,7 @@
  LXC,
  BHyve and
  more
-targets Linux, FreeBSD, Windows and 
OS-X
+targets Linux, FreeBSD, Windows and 
macOS
is used by many applications
  
  Recent / forthcoming release changes
--
2.25.4



signature.asc
Description: PGP signature

[libvirt PATCH] Replace "OS-X" with "macOS" in index.html.in

[Ján Tomko]

From: Ryan Schmidt 

Apple changed the operating system's name from "OS X" to "macOS" a few years 
ago.

Signed-off-by: Ryan Schmidt 
Reviewed-by: Ján Tomko 
Signed-off-by: Ján Tomko 
---
 docs/index.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/index.html.in b/docs/index.html.in
index 586defff54..20184a7441 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -27,7 +27,7 @@
   LXC,
   BHyve and
   more
-targets Linux, FreeBSD, Windows and 
OS-X
+targets Linux, FreeBSD, Windows and 
macOS
 is used by many applications
   
   Recent / forthcoming release changes
-- 
2.25.4

Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Jason Wang]

On 2020/7/8 下午10:16, Peter Xu wrote:

On Wed, Jul 08, 2020 at 01:42:30PM +0800, Jason Wang wrote:

So it should be functional equivalent to vtd_as_has_notifier().

For example: in vtd_iommu_replay() we'll skip the replay if vhost has
registered the iommu notifier because vtd_as_has_map_notifier() will return
false.


Two questions:

- Do we care the performance here? If not, vhost may just ignore the MAP
event?

I think we care, because vtd_page_walk() can be expensive.



Ok.





- If we care the performance, it's better to implement the MAP event for
vhost, otherwise it could be a lot of IOTLB miss

I feel like these are two things.

So far what we are talking about is whether vt-d should have knowledge about
what kind of events one iommu notifier is interested in.  I still think we
should keep this as answered in question 1.

The other question is whether we want to switch vhost from UNMAP to MAP/UNMAP
events even without vDMA, so that vhost can establish the mapping even before
IO starts.  IMHO it's doable, but only if the guest runs DPDK workloads.  When
the guest is using dynamic iommu page mappings, I feel like that can be even
slower, because then the worst case is for each IO we'll need to vmexit twice:

   - The first vmexit caused by an invalidation to MAP the page tables, so vhost
 will setup the page table before IO starts

   - IO/DMA triggers and completes

   - The second vmexit caused by another invalidation to UNMAP the page tables

So it seems to be worse than when vhost only uses UNMAP like right now.  At
least we only have one vmexit (when UNMAP).  We'll have a vhost translate()
request from kernel to userspace, but IMHO that's cheaper than the vmexit.



Right but then I would still prefer to have another notifier.

Since vtd_page_walk has nothing to do with device IOTLB. IOMMU have a 
dedicated command for flushing device IOTLB. But the check for 
vtd_as_has_map_notifier is used to skip the device which can do demand 
paging via ATS or device specific way. If we have two different 
notifiers, vhost will be on the device iotlb notifier so we don't need 
it at all?


Thanks