Re: [PATCH] Fix linkage to libutil and libkvm on FreeBSD 11

[Roman Bogorodskiy]

  Daniel P. Berrangé wrote:

> We are currently adding -lutil and -lkvm to the linker using the
> add_project_link_arguments method. On FreeBSD 11.4, this results in
> build errors because the args appear too early in the command line.
> 
> We need to pass the libraries as dependencies so that they get placed
> at the same point in the linker args as other dependencies.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  meson.build   | 16 +++-
>  src/bhyve/meson.build | 15 +++
>  src/util/meson.build  | 26 +-
>  3 files changed, 35 insertions(+), 22 deletions(-)
> 
> Using the CI patch I posted earlier to add FreeBSD 11:
> 
>   https://gitlab.com/berrange/libvirt/-/pipelines/185834799
> 
> 
> diff --git a/meson.build b/meson.build
> index 1eadea33bf..c30ff187aa 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1086,7 +1086,8 @@ endif
>  # Check for BSD kvm (kernel memory interface)
>  if host_machine.system() == 'freebsd'
>kvm_dep = cc.find_library('kvm')
> -  add_project_link_arguments('-lkvm', language: 'c')
> +else
> +  kvm_dep = disabler()
>  endif
>  
>  libiscsi_version = '1.18.0'
> @@ -1203,11 +1204,9 @@ have_gnu_gettext_tools = false
>  if not get_option('nls').disabled()
>have_gettext = cc.has_function('gettext')
>if not have_gettext
> -intl_lib = cc.find_library('intl', required: false)
> -have_gettext = intl_lib.found()
> -if have_gettext
> -  add_project_link_arguments('-lintl', language: 'c')
> -endif
> +intl_dep = cc.find_library('intl', required: false)
> +  else
> +intl_dep = disabler()
>endif
>if not have_gettext and get_option('nls').enabled()
>  error('gettext() is required to build libvirt')

Looks like this error is issued even if we found intl_dep.
Should this check be updated to:

 if not (have_gettext or intl_dep.found()) and get_option('nls').enabled()
   error(...)

?

> @@ -1235,6 +1234,8 @@ if not get_option('nls').disabled()
>have_gnu_gettext_tools = true
>  endif
>endif
> +else
> +  intl_dep = disabler()
>  endif
>  
>  numactl_dep = cc.find_library('numa', required: get_option('numactl'))
> @@ -1402,9 +1403,6 @@ if udev_dep.found()
>  endif
>  
>  util_dep = cc.find_library('util', required: false)
> -if util_dep.found()
> -  add_project_link_arguments('-lutil', language: 'c')
> -endif
>  
>  if not get_option('virtualport').disabled()
>if cc.has_header_symbol('linux/if_link.h', 'IFLA_PORT_MAX')

Roman Bogorodskiy


signature.asc
Description: PGP signature

[GSoC] Final Project report

[Prathamesh Chavan]

Since GSoC has come to an end, I would like to share with the
libvirt-community the work which I was able to do during this time[1].

I would like to thanks the libvirt-community and my mentors for giving
me this opportunity, and helping me through this amazing experience.

Thanks,
Prathamesh Chavan

[1]: 
https://drive.google.com/drive/u/0/folders/19KMJhUUp1WdXGl7slk9hLqXSwK8aG4di

[libvirt PATCH 0/2] Fix IPv6 network startup after removal of "dummy" tap device

[Laine Stump]

After the patch that removed the dummy tap device was added (commit
ee6c936fbb), IPv6 networks would no longer start due to a "failure to
complete DAD". Since this wait was only necessary because of the
existence of the dummy tap device (see detailed git log spelunking in
Patch 1 comment), we can just remove the wait.

Laine Stump (2):
  network: don't wait for IPv6 DAD completion when starting a network
  util: remove unused virNetDevIPWaitDadFinish()

 src/libvirt_private.syms|   1 -
 src/network/bridge_driver.c |  32 --
 src/util/virnetdevip.c  | 119 
 src/util/virnetdevip.h  |   2 -
 4 files changed, 154 deletions(-)

-- 
2.26.2

[libvirt PATCH 2/2] util: remove unused virNetDevIPWaitDadFinish()

[Laine Stump]

Since we no longer need to wait for IPv6 DAD to complete, we never
call this function.

Signed-off-by: Laine Stump 
---
 src/libvirt_private.syms |   1 -
 src/util/virnetdevip.c   | 119 ---
 src/util/virnetdevip.h   |   2 -
 3 files changed, 122 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1272ac6506..f8cdd01797 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2633,7 +2633,6 @@ virNetDevIPRouteGetAddress;
 virNetDevIPRouteGetGateway;
 virNetDevIPRouteGetMetric;
 virNetDevIPRouteGetPrefix;
-virNetDevIPWaitDadFinish;
 
 
 # util/virnetdevmacvlan.h
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 1e7e64f8f3..7bd5a75f85 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -45,8 +45,6 @@
 # include 
 #endif
 
-#define VIR_DAD_WAIT_TIMEOUT 20 /* seconds */
-
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 VIR_LOG_INIT("util.netdevip");
@@ -372,113 +370,6 @@ virNetDevIPRouteAdd(const char *ifname,
 }
 
 
-/* return true if there is a known address with 'tentative' flag set */
-static bool
-virNetDevIPParseDadStatus(struct nlmsghdr *nlh, int len,
-  virSocketAddrPtr *addrs, size_t count)
-{
-struct ifaddrmsg *ifaddrmsg_ptr;
-unsigned int ifaddrmsg_len;
-struct rtattr *rtattr_ptr;
-size_t i;
-struct in6_addr *addr;
-
-VIR_WARNINGS_NO_CAST_ALIGN
-for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) {
-VIR_WARNINGS_RESET
-if (NLMSG_PAYLOAD(nlh, 0) < sizeof(struct ifaddrmsg)) {
-/* Message without payload is the last one. */
-break;
-}
-
-ifaddrmsg_ptr = (struct ifaddrmsg *)NLMSG_DATA(nlh);
-if (!(ifaddrmsg_ptr->ifa_flags & IFA_F_TENTATIVE)) {
-/* Not tentative: we are not interested in this entry. */
-continue;
-}
-
-ifaddrmsg_len = IFA_PAYLOAD(nlh);
-VIR_WARNINGS_NO_CAST_ALIGN
-rtattr_ptr = (struct rtattr *) IFA_RTA(ifaddrmsg_ptr);
-for (; RTA_OK(rtattr_ptr, ifaddrmsg_len);
- rtattr_ptr = RTA_NEXT(rtattr_ptr, ifaddrmsg_len)) {
-VIR_WARNINGS_RESET
-if (RTA_PAYLOAD(rtattr_ptr) != sizeof(struct in6_addr)) {
-/* No address: ignore. */
-continue;
-}
-
-/* We check only known addresses. */
-for (i = 0; i < count; i++) {
-addr = [i]->data.inet6.sin6_addr;
-if (!memcmp(addr, RTA_DATA(rtattr_ptr),
-sizeof(struct in6_addr))) {
-/* We found matching tentative address. */
-return true;
-}
-}
-}
-}
-return false;
-}
-
-
-/* return after DAD finishes for all known IPv6 addresses or an error */
-int
-virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
-{
-struct ifaddrmsg ifa;
-unsigned int recvbuflen;
-bool dad = true;
-time_t max_time = time(NULL) + VIR_DAD_WAIT_TIMEOUT;
-g_autoptr(virNetlinkMsg) nlmsg = NULL;
-
-if (!(nlmsg = nlmsg_alloc_simple(RTM_GETADDR,
- NLM_F_REQUEST | NLM_F_DUMP))) {
-virReportOOMError();
-return -1;
-}
-
-memset(, 0, sizeof(ifa));
-/* DAD is for IPv6 addresses only. */
-ifa.ifa_family = AF_INET6;
-if (nlmsg_append(nlmsg, , sizeof(ifa), NLMSG_ALIGNTO) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("allocated netlink buffer is too small"));
-return -1;
-}
-
-/* Periodically query netlink until DAD finishes on all known addresses. */
-while (dad && time(NULL) < max_time) {
-g_autofree struct nlmsghdr *resp = NULL;
-
-if (virNetlinkCommand(nlmsg, , , 0, 0,
-  NETLINK_ROUTE, 0) < 0)
-return -1;
-
-if (virNetlinkGetErrorCode(resp, recvbuflen) < 0) {
-virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
-   _("error reading DAD state information"));
-return -1;
-}
-
-/* Parse response. */
-dad = virNetDevIPParseDadStatus(resp, recvbuflen, addrs, count);
-if (dad)
-g_usleep(1000 * 10);
-}
-/* Check timeout. */
-if (dad) {
-virReportError(VIR_ERR_SYSTEM_ERROR,
-   _("Duplicate Address Detection "
- "not finished in %d seconds"), VIR_DAD_WAIT_TIMEOUT);
-} else {
-return 0;
-}
-
-return -1;
-}
-
 static int
 virNetDevIPGetAcceptRA(const char *ifname)
 {
@@ -798,16 +689,6 @@ virNetDevIPRouteAdd(const char *ifname,
 }
 
 
-/* return after DAD finishes for all known IPv6 addresses or an error */
-int
-virNetDevIPWaitDadFinish(virSocketAddrPtr *addrs G_GNUC_UNUSED,
- size_t count G_GNUC_UNUSED)
-{
-virReportSystemError(ENOSYS, "%s",
-  

[libvirt PATCH 1/2] network: don't wait for IPv6 DAD completion when starting a network

[Laine Stump]

0f7436ca54 added code during virtual network startup to wait for DAD
(Duplicate Address Detection) to complete if there were any IPv6
addresses on the network. This wait was needed because (according to
the commit log) "created problems when [the "dummy" tap device] is set
to IFF_DOWN prior to DAD completing".

That commit in turn referenced commit db488c7917, which had added the
code to set the dummy tap device IFF_DOWN, commenting "DAD has
happened (dnsmasq waits for it)", and in its commit message pointed
out that if we just got rid of the dummy tap device this wouldn't be
needed.

Now that the dummy tap device has indeed been removed (commit
ee6c936fbb), there is no longer any need to set it IFF_DOWN, and thus
nothing requiring us to wait for DAD to complete. At any rate, with
the dummy tap device removed, leaving nothing else on the bridge when
it is first started, DAD never completes, leading to failure to start
any IPv6 network.

So, yes, this patch removes the wait for DAD completion, and IPv6
networks can once again start, and their associated dnsmasq process
starts successfully (this is the problem that the DAD wait was
originally intended to fix)

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c | 32 
 1 file changed, 32 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 5c00befc16..87d7acab06 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2286,32 +2286,6 @@ networkAddRouteToBridge(virNetworkObjPtr obj,
 return 0;
 }
 
-static int
-networkWaitDadFinish(virNetworkObjPtr obj)
-{
-virNetworkDefPtr def = virNetworkObjGetDef(obj);
-virNetworkIPDefPtr ipdef;
-g_autofree virSocketAddrPtr *addrs = NULL;
-virSocketAddrPtr addr = NULL;
-size_t naddrs = 0;
-int ret = -1;
-
-VIR_DEBUG("Begin waiting for IPv6 DAD on network %s", def->name);
-
-while ((ipdef = virNetworkDefGetIPByIndex(def, AF_INET6, naddrs))) {
-addr = >address;
-if (VIR_APPEND_ELEMENT_COPY(addrs, naddrs, addr) < 0)
-goto cleanup;
-}
-
-ret = (naddrs == 0) ? 0 : virNetDevIPWaitDadFinish(addrs, naddrs);
-
- cleanup:
-VIR_DEBUG("Finished waiting for IPv6 DAD on network %s with status %d",
-  def->name, ret);
-return ret;
-}
-
 
 static int
 networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
@@ -2444,12 +2418,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr 
driver,
 if (v6present && networkStartRadvd(driver, obj) < 0)
 goto error;
 
-/* dnsmasq does not wait for DAD to complete before daemonizing,
- * so we need to wait for it ourselves.
- */
-if (v6present && networkWaitDadFinish(obj) < 0)
-goto error;
-
 if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
 goto error;
 
-- 
2.26.2

Re: [libvirt PATCH] remove HAL node device driver

[Daniel Henrique Barboza]

This makes meson complains about POTFILES.in:


po_check
--- /home/danielhb/kvm-project/libvirt/po/POTFILES.in
+++ /home/danielhb/kvm-project/libvirt/po/POTFILES.in
@@ -130,7 +130,6 @@
 @SRCDIR@src/network/bridge_driver_linux.c
 @SRCDIR@src/network/leaseshelper.c
 @SRCDIR@src/node_device/node_device_driver.c
-@SRCDIR@src/node_device/node_device_hal.c
 @SRCDIR@src/node_device/node_device_udev.c
 @SRCDIR@src/nwfilter/nwfilter_dhcpsnoop.c
 @SRCDIR@src/nwfilter/nwfilter_driver.c
make: Leaving directory '/home/danielhb/kvm-project/libvirt/build/build-aux'
--- stderr ---
build-aux/syntax-check.mk: you have changed the set of files with translatable 
diagnostics;
 apply the above patch
make: *** [/home/danielhb/kvm-project/libvirt/build-aux/syntax-check.mk:1798: 
sc_po_check] Error 1
---

Full log written to 
/home/danielhb/kvm-project/libvirt/build/meson-logs/testlog.txt
FAILED: meson-test



Thanks,


DHB

On 9/3/20 9:46 AM, Pavel Hrdina wrote:

There was one attempt a year ago done by me to drop HAL [1] but it was
never resolved. There was another time when Dan suggested to drop HAL
driver [2] but it was decided to keep it around in case device
assignment will be implemented for FreeBSD and the fact that
virt-manager uses node device driver [3].

I checked git history and code and it doesn't look like bhyve supports
device assignment so from that POV it should not block removing HAL.

The argument about virt-manager is not strong as well because libvirt
installed from FreeBSD packages doesn't have HAL support so it will not
affect these users as well [4].

The only users affected by this change would be the ones compiling
libvirt from GIT on FreeBSD.

I looked into alternatives and there is libudev-devd package on FreeBSD
but unfortunately it doesn't work as it doesn't list any devices when
used with libvirt. It provides libudev APIs using devd.

I also looked into devd directly and it provides some APIs but there are
no APIs for device monitoring and events so that would have to be
somehow done by libvirt.

Main motivation for dropping HAL support is to replace libdbus with GLib
dbus implementation and it cannot be done with HAL driver present in
libvirt because HAL APIs heavily depends on symbols provided by libdbus.

[1] 
[2] 
[3] 
[4] 

Signed-off-by: Pavel Hrdina 
---
  meson.build  |   9 +-
  meson_options.txt|   1 -
  src/node_device/meson.build  |   5 -
  src/node_device/node_device_driver.c |  10 +-
  src/node_device/node_device_driver.h |   5 -
  src/node_device/node_device_hal.c| 843 ---
  src/node_device/node_device_hal.h|  22 -
  7 files changed, 3 insertions(+), 892 deletions(-)
  delete mode 100644 src/node_device/node_device_hal.c
  delete mode 100644 src/node_device/node_device_hal.h

diff --git a/meson.build b/meson.build
index 1aad385ad1..d9c91d88dd 100644
--- a/meson.build
+++ b/meson.build
@@ -1079,12 +1079,6 @@ glusterfs_dep = dependency('glusterfs-api', version: 
'>=' + glusterfs_version, r
  gnutls_version = '3.2.0'
  gnutls_dep = dependency('gnutls', version: '>=' + gnutls_version)
  
-hal_version = '0.5.0'

-hal_dep = dependency('hal', version: '>=' + hal_version, required: 
get_option('hal'))
-if hal_dep.found()
-  conf.set('WITH_HAL', 1)
-endif
-
  # Check for BSD kvm (kernel memory interface)
  if host_machine.system() == 'freebsd'
kvm_dep = cc.find_library('kvm')
@@ -1728,7 +1722,7 @@ if not get_option('driver_network').disabled() and 
conf.has('WITH_LIBVIRTD') and
conf.set('WITH_NETWORK', 1)
  endif
  
-if hal_dep.found() or udev_dep.found()

+if udev_dep.found()
conf.set('WITH_NODE_DEVICES', 1)
  endif
  
@@ -2433,7 +2427,6 @@ libs_summary = {

'glib_dep': glib_dep.found(),
'glusterfs': glusterfs_dep.found(),
'gnutls': gnutls_dep.found(),
-  'hal': hal_dep.found(),
'libiscsi': libiscsi_dep.found(),
'libnl': libnl_dep.found(),
'libpcap': libpcap_dep.found(),
diff --git a/meson_options.txt b/meson_options.txt
index 7838630c1e..c8886e1430 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -23,7 +23,6 @@ option('firewalld', type: 'feature', value: 'auto', 
description: 'firewalld supp
  option('firewalld_zone', type: 'feature', value: 'auto', description: 
'whether to install firewalld libvirt zone')
  option('fuse', type: 'feature', value: 'auto', description: 'fuse support')
  option('glusterfs', type: 'feature', value: 'auto', description: 'glusterfs 
support')
-option('hal', type: 'feature', value: 'auto', description: 'hal support')
  option('libiscsi', type: 'feature', value: 'auto', description: 'libiscsi 
support')
  option('libpcap', type: 'feature', value: 'auto', 

Re: [RFC PATCH] docs/system/deprecated: mark ppc64abi32-linux-user for deprecation

[Peter Maydell]

On Fri, 4 Sep 2020 at 17:52, Alex Bennée  wrote:
>
> It's buggy and we are not sure anyone uses it.

> +``ppc64abi32`` CPUs (since 5.2.0)
> +'
> +
> +The ``ppc64abi32`` architecture has a number of issues which regularly
> +trip up our CI testing and is suspected to be quite broken.
> +Furthermore the maintainers are unsure what the correct behaviour
> +should be and strongly suspect no one actually uses it.

IRC discussion suggests we do know what the correct behaviour
is -- it should be "what the compat32 interface of a 64-bit
PPC kernel gives you", it's just that the code doesn't do that
(and never has?). It's like the mipsn32, mipsn32el, sparc32plus
ABIs which we also implement (hopefully correctly...)

But "this has always been broken and nobody complained" is
a good reason to deprecate anyway.

thanks
-- PMM

Re: [PATCH] Fix linkage to libutil and libkvm on FreeBSD 11

[Ján Tomko]

On a Friday in 2020, Daniel P. Berrangé wrote:

We are currently adding -lutil and -lkvm to the linker using the
add_project_link_arguments method. On FreeBSD 11.4, this results in
build errors because the args appear too early in the command line.

We need to pass the libraries as dependencies so that they get placed
at the same point in the linker args as other dependencies.

Signed-off-by: Daniel P. Berrangé 
---
meson.build   | 16 +++-
src/bhyve/meson.build | 15 +++
src/util/meson.build  | 26 +-
3 files changed, 35 insertions(+), 22 deletions(-)

Using the CI patch I posted earlier to add FreeBSD 11:

 https://gitlab.com/berrange/libvirt/-/pipelines/185834799




Reviewed-by: Ján Tomko 

And it looks like a fun and safe change to push on a Friday afternoon.

Jano


signature.asc
Description: PGP signature

Re: [RFC PATCH] docs/system/deprecated: mark ppc64abi32-linux-user for deprecation

[Alex Bennée]

Alex Bennée  writes:

> It's buggy and we are not sure anyone uses it.
>
> Cc: David Gibson 
> Cc: Richard Henderson 
> Signed-off-by: Alex Bennée 

A more aggressive follow-up patch which would also solve the CI failures
across the board:

--8<---cut here---start->8---
configure: don't enable ppc64abi32-linux-user by default

The user can still enable this explicitly but they will get a warning
at the end of configure for their troubles. This also drops any builds
of ppc64abi32 from our CI tests.

Signed-off-by: Alex Bennée 

1 file changed, 27 insertions(+), 19 deletions(-)
configure | 46 +++---

modified   configure
@@ -574,6 +574,8 @@ gettext=""
 bogus_os="no"
 malloc_trim=""
 
+deprecated_features=""
+
 # parse CC options first
 for opt do
   optarg=$(expr "x$opt" : 'x[^=]*=\(.*\)')
@@ -1769,26 +1771,25 @@ if [ "$bsd_user" = "yes" ]; then
 mak_wilds="${mak_wilds} $source_path/default-configs/*-bsd-user.mak"
 fi
 
-if test -z "$target_list_exclude"; then
-for config in $mak_wilds; do
-default_target_list="${default_target_list} $(basename "$config" .mak)"
-done
-else
-exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
-for config in $mak_wilds; do
-target="$(basename "$config" .mak)"
-exclude="no"
-for excl in $exclude_list; do
-if test "$excl" = "$target"; then
-exclude="yes"
-break;
-fi
-done
-if test "$exclude" = "no"; then
-default_target_list="${default_target_list} $target"
+if test -z "$target_list_exclude" -a -z "$target_list"; then
+# if the user doesn't specify anything lets skip deprecating stuff
+target_list_exclude=ppc64abi32-linux-user
+fi
+
+exclude_list=$(echo "$target_list_exclude" | sed -e 's/,/ /g')
+for config in $mak_wilds; do
+target="$(basename "$config" .mak)"
+exclude="no"
+for excl in $exclude_list; do
+if test "$excl" = "$target"; then
+exclude="yes"
+break;
 fi
 done
-fi
+if test "$exclude" = "no"; then
+default_target_list="${default_target_list} $target"
+fi
+done
 
 # Enumerate public trace backends for --help output
 trace_backend_list=$(echo $(grep -le '^PUBLIC = True$' 
"$source_path"/scripts/tracetool/backend/*.py | sed -e 
's/^.*\/\(.*\)\.py$/\1/'))
@@ -7691,7 +7692,7 @@ TARGET_SYSTBL=""
 case "$target_name" in
   i386)
 mttcg="yes"
-   gdb_xml_files="i386-32bit.xml"
+gdb_xml_files="i386-32bit.xml"
 TARGET_SYSTBL_ABI=i386
 TARGET_SYSTBL=syscall_32.tbl
   ;;
@@ -7802,6 +7803,7 @@ case "$target_name" in
 TARGET_SYSTBL_ABI=common,nospu,32
 echo "TARGET_ABI32=y" >> $config_target_mak
 gdb_xml_files="power64-core.xml power-fpu.xml power-altivec.xml 
power-spe.xml power-vsx.xml"
+deprecated_features="ppc64abi32 ${deprecated_features}"
   ;;
   riscv32)
 TARGET_BASE_ARCH=riscv
@@ -8232,6 +8234,12 @@ fi
 touch ninjatool.stamp
 fi
 
+if test -n "${deprecated_features}"; then
+echo "Warning, deprecated features enabled."
+echo "Please see docs/system/deprecated.rst"
+echo "  features: ${deprecated_features}"
+fi
+
 # Save the configure command line for later reuse.
 cat 8---

-- 
Alex Bennée

Re: [PATCH] tools: avoid unused parameter warning when readline is disabled

[Daniel P . Berrangé]

On Fri, Sep 04, 2020 at 06:22:58PM +0200, Ján Tomko wrote:
> On a Friday in 2020, Daniel P. Berrangé wrote:
> > The vshReadlineHistoryAdd stub method does not use its parameter.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> > 
> > Pushed to fix minimal build used by layered repo CI
> > 
> 
> Which repo?

Essentially all of the libvirt repos :)



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

Re: [PATCH-for-4.2] hw/mips: Deprecate the r4k machine

[Philippe Mathieu-Daudé]

Hi,

On 12/17/19 7:43 PM, Aleksandar Markovic wrote:
> From: Thomas Huth 
> Sent: Tuesday, December 17, 2019 7:10 PM
> To: Philippe Mathieu-Daudé; qemu-de...@nongnu.org
> Cc: libvir-list@redhat.com; Hervé Poussineau; Aleksandar Markovic; Aleksandar 
> Rikalo; Aurelien Jarno
> Subject: [EXTERNAL]Re: [PATCH-for-4.2] hw/mips: Deprecate the r4k machine
> 
>  Hi,
> 
> On 25/11/2019 11.41, Philippe Mathieu-Daudé wrote:
>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>> index 4b4b7425ac..05265b43c8 100644
>>> --- a/qemu-deprecated.texi
>>> +++ b/qemu-deprecated.texi
>>> @@ -266,6 +266,11 @@ The 'scsi-disk' device is deprecated. Users should use 
>>> 'scsi-hd' or
>>>
>>>  @section System emulator machines
>>>
>>> +@subsection mips r4k platform (since 4.2)
>>
>> Since the patch has now been merged after the release of 4.2, the mips
>> 4k platform will be deprecated in 5.0 instead. Could you send a patch to
>> fix it up?
> 
> OK, I'll send a patch that'll certainly be applied to the next MIPS queue.
> 
> Thanks for spotting this, Thomas.
> 
> Aleksandar

Any update on this?

Thanks,

Phil.

[RFC PATCH] docs/system/deprecated: mark ppc64abi32-linux-user for deprecation

[Alex Bennée]

It's buggy and we are not sure anyone uses it.

Cc: David Gibson 
Cc: Richard Henderson 
Signed-off-by: Alex Bennée 
---
 docs/system/deprecated.rst | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 851dbdeb8ab..11c763383d9 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -424,6 +424,15 @@ linux-user mode) is deprecated and will be removed in a 
future version
 of QEMU. Support for this CPU was removed from the upstream Linux
 kernel in 2018, and has also been dropped from glibc.
 
+``ppc64abi32`` CPUs (since 5.2.0)
+'
+
+The ``ppc64abi32`` architecture has a number of issues which regularly
+trip up our CI testing and is suspected to be quite broken.
+Furthermore the maintainers are unsure what the correct behaviour
+should be and strongly suspect no one actually uses it.
+
+
 Related binaries
 
 
-- 
2.20.1

Re: [PATCH v2 10/13] qemu: avoid deadlock in qemuDomainObjStopWorker

[Daniel P . Berrangé]

On Fri, Sep 04, 2020 at 05:46:18PM +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 23, 2020 at 01:14:10PM +0300, Nikolay Shirokovskiy wrote:
> > We are dropping the only reference here so that the event loop thread
> > is going to be exited synchronously. In order to avoid deadlocks we
> > need to unlock the VM so that any handler being called can finish
> > execution and thus even loop thread be finished too.
> > 
> > Signed-off-by: Nikolay Shirokovskiy 
> > ---
> >  src/qemu/qemu_domain.c | 18 ++
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 5b22eb2..82b3d11 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -1637,11 +1637,21 @@ void
> >  qemuDomainObjStopWorker(virDomainObjPtr dom)
> >  {
> >  qemuDomainObjPrivatePtr priv = dom->privateData;
> > +virEventThread *eventThread;
> >  
> > -if (priv->eventThread) {
> > -g_object_unref(priv->eventThread);
> > -priv->eventThread = NULL;
> > -}
> > +if (!priv->eventThread)
> > +return;
> > +
> > +/*
> > + * We are dropping the only reference here so that the event loop 
> > thread
> > + * is going to be exited synchronously. In order to avoid deadlocks we
> > + * need to unlock the VM so that any handler being called can finish
> > + * execution and thus even loop thread be finished too.
> > + */
> > +eventThread = g_steal_pointer(>eventThread);
> > +virObjectUnlock(dom);
> > +g_object_unref(eventThread);
> > +virObjectLock(dom);
> 
> Hmm, I'm really not at all comfortable with this. I don't have confidence
> that qemuProcessStop() safe if we drpo & reacquire the lock half way
> through its cleanup process.  The 'virDomainObj' is in a semi-clean
> state, 1/2 running 1/2 shutoff.
> 
> This is the key reason I think I didn't do the synchronous thread join
> in the event loop.

Hmm, I see your justification in the other reply now.

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v2 10/13] qemu: avoid deadlock in qemuDomainObjStopWorker

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 01:14:10PM +0300, Nikolay Shirokovskiy wrote:
> We are dropping the only reference here so that the event loop thread
> is going to be exited synchronously. In order to avoid deadlocks we
> need to unlock the VM so that any handler being called can finish
> execution and thus even loop thread be finished too.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_domain.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5b22eb2..82b3d11 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1637,11 +1637,21 @@ void
>  qemuDomainObjStopWorker(virDomainObjPtr dom)
>  {
>  qemuDomainObjPrivatePtr priv = dom->privateData;
> +virEventThread *eventThread;
>  
> -if (priv->eventThread) {
> -g_object_unref(priv->eventThread);
> -priv->eventThread = NULL;
> -}
> +if (!priv->eventThread)
> +return;
> +
> +/*
> + * We are dropping the only reference here so that the event loop thread
> + * is going to be exited synchronously. In order to avoid deadlocks we
> + * need to unlock the VM so that any handler being called can finish
> + * execution and thus even loop thread be finished too.
> + */
> +eventThread = g_steal_pointer(>eventThread);
> +virObjectUnlock(dom);
> +g_object_unref(eventThread);
> +virObjectLock(dom);

Hmm, I'm really not at all comfortable with this. I don't have confidence
that qemuProcessStop() safe if we drpo & reacquire the lock half way
through its cleanup process.  The 'virDomainObj' is in a semi-clean
state, 1/2 running 1/2 shutoff.

This is the key reason I think I didn't do the synchronous thread join
in the event loop.

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 v2 09/13] vireventthread: exit thread synchronously on finalize

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 01:14:09PM +0300, Nikolay Shirokovskiy wrote:
> It useful to be sure no thread is running after we drop all references to
> virEventThread. Otherwise in order to avoid crashes we need to synchronize 
> some
> other way or we make extra references in event handler callbacks to all the
> object in use. And some of them are not prepared to be refcounted.
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/util/vireventthread.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v2 08/13] qemu: don't shutdown event thread in monitor EOF callback

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 01:14:08PM +0300, Nikolay Shirokovskiy wrote:
> This hunk was introduced in [1] in order to avoid loosing
> events from monitor on stopping qemu process. But as explained
> in [2] on destroy we won't get neither EOF nor any other
> events as monitor is just closed. In case of crash/shutdown
> we won't get any more events as well and qemuDomainObjStopWorker
> will be called by qemuProcessStop eventually. Thus let's
> remove qemuDomainObjStopWorker from qemuProcessHandleMonitorEOF
> as it is not useful anymore.
> 
> [1] e6afacb0f: qemu: start/stop an event loop thread for domains
> [2] d2954c072: qemu: ensure domain event thread is always stopped
> 
> Signed-off-by: Nikolay Shirokovskiy 
> ---
>  src/qemu/qemu_process.c | 3 ---
>  1 file changed, 3 deletions(-)


Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH v2 07/13] rpc: finish all threads before exiting main loop

[Daniel P . Berrangé]

On Thu, Jul 23, 2020 at 01:14:07PM +0300, Nikolay Shirokovskiy wrote:
> Currently we have issues like [1] on libvirtd shutdown as we cleanup while RPC
> and other threads are still running. Let's finish all threads other then main
> before cleanup.
> 
> The approach to finish threads is suggested in [2]. In order to finish RPC
> threads serving API calls we let the event loop run but stop accepting new API
> calls and block processing any pending API calls. We also inform all drivers 
> of
> shutdown so they can prepare for shutdown too. Then we wait for all RPC 
> threads
> and driver's background thread to finish. If finishing takes more then 15s we
> just exit as we can't safely cleanup in time.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1828207
> [2] https://www.redhat.com/archives/libvir-list/2020-April/msg01328.html
> 
> Signed-off-by: Nikolay Shirokovskiy 
> Reviewed-by: Daniel Henrique Barboza 
> ---
>  src/remote/remote_daemon.c |  6 ++--
>  src/rpc/virnetdaemon.c | 81 
> +-
>  2 files changed, 83 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH] tools: avoid unused parameter warning when readline is disabled

[Ján Tomko]

On a Friday in 2020, Daniel P. Berrangé wrote:

The vshReadlineHistoryAdd stub method does not use its parameter.

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

Pushed to fix minimal build used by layered repo CI



Which repo?

Jano


tools/vsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index ef2a3f62d7..0e8edcd78c 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2971,7 +2971,7 @@ vshReadline(vshControl *ctl G_GNUC_UNUSED,
}

void
-vshReadlineHistoryAdd(const char *cmd)
+vshReadlineHistoryAdd(const char *cmd G_GNUC_UNUSED)
{
/* empty */
}
--
2.26.2



signature.asc
Description: PGP signature

Re: [PATCH 3/3] qemu_namespace: Don't leak mknod items that are being skipped over

[Ján Tomko]

On a Friday in 2020, Michal Privoznik wrote:

When building and populating domain NS a couple of functions is
called that append paths to a string list. This string list is
then inspected, one item at the time by
qemuNamespacePrepareOneItem() which gathers all the info for
given path (stat buffer, possible link target, ACLs, SELinux
label) using qemuNamespaceMknodItemInit(). If the path needs to
be created in the domain's private /dev then it's added onto this
qemuNamespaceMknodData list which is freed later in the process.
But, if the path does not need to be created in the domain's
private /dev, then the memory allocated by
qemuNamespaceMknodItemInit() is not freed anywhere leading to a
leak.

Signed-off-by: Michal Privoznik 
---

Honestly, I think this patch looks ugly. Ideas are welcome.

src/qemu/qemu_namespace.c | 42 +--
1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 87f4fd8d58..917e140f6a 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1166,22 +1166,29 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr 
data,
size_t ndevMountsPath)
{
long ttl = sysconf(_SC_SYMLOOP_MAX);
-const char *next = file;
+g_autofree char *next = g_strdup(file);
size_t i;

while (1) {
qemuNamespaceMknodItem item = { 0 };
+bool added = false;
+bool isLink;
int rc;

rc = qemuNamespaceMknodItemInit(, cfg, vm, next);


@next gets consumed here. That seems to be the cause of all grief.


-if (rc == -2) {
-/* @file doesn't exist. We can break here. */
-break;
-} else if (rc < 0) {
+if (rc < 0)  {
+qemuNamespaceMknodItemClear();
+
+if (rc == -2) {
+/* @file doesn't exist. We can break here. */
+break;


Strictly speaking, there is nothing to free yet because -2 is returned
pretty early. And ItemInit could be changed to clean up after itself
if it returns -1.


+}
/* Some other (critical) error. */
return -1;
}

+isLink = S_ISLNK(item.sb.st_mode);
+


If you move the next assignment here, nothing below the APPEND_ELEMENT_COPY
needs to access item anymore, so you can switch it to the non-copy
version and clear it unconditionally and use g_auto to clear it.


if (STRPREFIX(next, QEMU_DEVPREFIX)) {
for (i = 0; i < ndevMountsPath; i++) {
if (STREQ(devMountsPath[i], "/dev"))
@@ -1190,22 +1197,35 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr 
data,
break;
}

-if (i == ndevMountsPath &&
-VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0)
-return -1;
+if (i == ndevMountsPath) {
+if (VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 
0) {
+qemuNamespaceMknodItemClear();
+return -1;
+}
+added = true;
+}
}

-if (!S_ISLNK(item.sb.st_mode))
+if (!isLink) {
+if (!added)
+qemuNamespaceMknodItemClear();
break;
+}

if (ttl-- == 0) {
+if (!added)
+qemuNamespaceMknodItemClear();



virReportSystemError(ELOOP,
 _("Too many levels of symbolic links: %s"),
- next);
+ file);
return -1;


This change makes sense on its own and can be separated.


}

-next = item.target;
+g_free(next);


Apart from the g_free/g_autofree mixup, this frees previous iteration's
item->file, possibly after it has been added to data->items. So we do need
to g_strdup it in ItemInit.


+next = g_strdup(item.target);
+


If you add a second array to hold the thrown away items during this
function, you could leave the iterator const char*. Not sure whether
that is nicer or not.

Jano


+if (!added)
+qemuNamespaceMknodItemClear();
}

return 0;
--
2.26.2



signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] qemu: Allow migration over UNIX socket

[Jiri Denemark]

On Fri, Sep 04, 2020 at 10:58:45 +0200, Martin Kletzander wrote:
> This allows:
> 
>  a) migration without access to network
> 
>  b) complete control of the migration stream
> 
>  c) easy migration between containerised libvirt daemons on the same host
> 
> Resolves: https://bugzilla.redhat.com/1638889
> 
> Signed-off-by: Martin Kletzander 
> ---
>  docs/manpages/virsh.rst  |  13 ++-
>  docs/migration.html.in   |  33 
>  src/qemu/qemu_driver.c   |  22 -
>  src/qemu/qemu_migration.c| 138 +++
>  src/qemu/qemu_migration_params.c |   9 ++
>  src/qemu/qemu_migration_params.h |   3 +
>  src/qemu/qemu_monitor.c  |  15 
>  src/qemu/qemu_monitor.h  |   4 +
>  8 files changed, 198 insertions(+), 39 deletions(-)
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index f3870b3c0b9d..f862942f87f9 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
...
> @@ -3684,6 +3715,14 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
>  if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
>   QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
>  if (mig->nbd) {
> +const char *host = "";
> +
> +if (spec->destType == MIGRATION_DEST_HOST ||
> +spec->destType == MIGRATION_DEST_CONNECT_HOST) {
> +host = spec->dest.host.name;
> +}
> +
> +

One empty line would be enough :-)

>  /* Currently libvirt does not support setting up of the NBD
>   * non-shared storage migration with TLS. As we need to honour 
> the
>   * VIR_MIGRATE_TLS flag, we need to reject such migration until
...

Reviewed-by: Jiri Denemark 

Re: [libvirt PATCH] wireshark: Don't include config.h

[Michal Prívozník]

On 9/4/20 5:12 PM, Andrea Bolognani wrote:

On Fri, 2020-09-04 at 15:30 +0200, Michal Prívozník wrote:

On 9/4/20 2:21 PM, Andrea Bolognani wrote:

On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:

The patch looks good. ws_version.h was introduced with 2.9.0 release
which is 1.5 years old. Given that the dissector is aimed mostly on us,
developers to help us debug RPC issues, I think we can safely bump the
minimal wireshark version required (currently 2.4.0 which is 3 years old).


That sounds reasonable in theory, but if you look at

https://gitlab.com/abologna/libvirt/-/pipelines/185421025

you'll see that even platforms that ship pretty recent Wireshark[1]
don't include ws_version.h among the headers.

Not building the dissector on those non-obsolete platforms seems
excessively harsh, so I think an approach similar to the one I
described above is still necessary. And at that point, you might as
well not bump the minimum required version and keep building the
dissector on the current list of platforms...


Any idea why they are not installing the file? Because while current
solution is hacky, intentionally removing a header file that a package
wants installed is way worse.


I don't think it's done on purpose: it's probably just a bug in the
Debian packaging that got propagated to Ubuntu.

Even assuming that's the case, it will take some time for it to be
addressed in sid, and the first Ubuntu LTS that will carry the
resulting fix is almost two years out... So I think we have to just
support both ws_version.h and config.h for a while.



Ah, so on one hand we have progressive distro that doesn't install 
internal header files, on the other we have LTS distros where changing 
something may take years to take effect. In that case our only option is 
to implement both ways. Or motivate LTS distros to implement the change 
sooner ;-)


Since you wrote the original patch, do you want to write this one too? 
Or do you want me to do it?


Michal

Re: [PATCH 3/3] qemu_namespace: Don't leak mknod items that are being skipped over

[Laine Stump]

On 9/4/20 4:24 AM, Michal Privoznik wrote:

When building and populating domain NS a couple of functions is
called



s/is/are/



that append paths to a string list. This string list is
then inspected, one item at the time by
qemuNamespacePrepareOneItem() which gathers all the info for
given path (stat buffer, possible link target, ACLs, SELinux
label) using qemuNamespaceMknodItemInit(). If the path needs to
be created in the domain's private /dev then it's added onto this
qemuNamespaceMknodData list which is freed later in the process.
But, if the path does not need to be created in the domain's
private /dev, then the memory allocated by
qemuNamespaceMknodItemInit() is not freed anywhere leading to a
leak.

Signed-off-by: Michal Privoznik 
---

Honestly, I think this patch looks ugly. Ideas are welcome.

  src/qemu/qemu_namespace.c | 42 +--
  1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 87f4fd8d58..917e140f6a 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1166,22 +1166,29 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr 
data,
  size_t ndevMountsPath)
  {
  long ttl = sysconf(_SC_SYMLOOP_MAX);
-const char *next = file;
+g_autofree char *next = g_strdup(file);
  size_t i;
  
  while (1) {

  qemuNamespaceMknodItem item = { 0 };
+bool added = false;
+bool isLink;
  int rc;
  
  rc = qemuNamespaceMknodItemInit(, cfg, vm, next);

-if (rc == -2) {
-/* @file doesn't exist. We can break here. */
-break;
-} else if (rc < 0) {
+if (rc < 0)  {
+qemuNamespaceMknodItemClear();
+
+if (rc == -2) {
+/* @file doesn't exist. We can break here. */
+break;
+}
  /* Some other (critical) error. */
  return -1;
  }
  
+isLink = S_ISLNK(item.sb.st_mode);

+
  if (STRPREFIX(next, QEMU_DEVPREFIX)) {
  for (i = 0; i < ndevMountsPath; i++) {
  if (STREQ(devMountsPath[i], "/dev"))
@@ -1190,22 +1197,35 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr 
data,
  break;
  }
  
-if (i == ndevMountsPath &&

-VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0)
-return -1;
+if (i == ndevMountsPath) {
+if (VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 
0) {
+qemuNamespaceMknodItemClear();
+return -1;
+}
+added = true;
+}
  }
  
-if (!S_ISLNK(item.sb.st_mode))

+if (!isLink) {
+if (!added)
+qemuNamespaceMknodItemClear();
  break;
+}
  
  if (ttl-- == 0) {

+if (!added)
+qemuNamespaceMknodItemClear();
  virReportSystemError(ELOOP,
   _("Too many levels of symbolic links: %s"),
- next);
+ file);
  return -1;
  }
  
-next = item.target;

+g_free(next);



I recall once being reprimanded for calling g_free on a g_autofree() 
pointer (by someone who said that *they* had been reprimanded for same). 
I will say



Reviewed-by: Laine Stump 


but don't want to later be fingered as "the person" who allowed this 
into the code, so I guess maybe look for a way around that (or a signoff 
from whoever it was that made the initial "it's bad to manually free a 
g_autofree pointer" statement). (I don't think it's particularly ugly 
BTW. Sure, it could probably be done in a cleaner manner somehow, but 
sometimes things are just messy; you can move the mess, but you can't 
eliminate it)




+next = g_strdup(item.target);
+
+if (!added)
+qemuNamespaceMknodItemClear();
  }
  
  return 0;

Re: [libvirt PATCH] wireshark: Don't include config.h

[Andrea Bolognani]

On Fri, 2020-09-04 at 15:30 +0200, Michal Prívozník wrote:
> On 9/4/20 2:21 PM, Andrea Bolognani wrote:
> > On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:
> > > The patch looks good. ws_version.h was introduced with 2.9.0 release
> > > which is 1.5 years old. Given that the dissector is aimed mostly on us,
> > > developers to help us debug RPC issues, I think we can safely bump the
> > > minimal wireshark version required (currently 2.4.0 which is 3 years old).
> > 
> > That sounds reasonable in theory, but if you look at
> > 
> >https://gitlab.com/abologna/libvirt/-/pipelines/185421025
> > 
> > you'll see that even platforms that ship pretty recent Wireshark[1]
> > don't include ws_version.h among the headers.
> > 
> > Not building the dissector on those non-obsolete platforms seems
> > excessively harsh, so I think an approach similar to the one I
> > described above is still necessary. And at that point, you might as
> > well not bump the minimum required version and keep building the
> > dissector on the current list of platforms...
> 
> Any idea why they are not installing the file? Because while current 
> solution is hacky, intentionally removing a header file that a package 
> wants installed is way worse.

I don't think it's done on purpose: it's probably just a bug in the
Debian packaging that got propagated to Ubuntu.

Even assuming that's the case, it will take some time for it to be
addressed in sid, and the first Ubuntu LTS that will carry the
resulting fix is almost two years out... So I think we have to just
support both ws_version.h and config.h for a while.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 2/3] lib: Prefer g_autoptr() declaration of virQEMUDriverConfigPtr

[Laine Stump]

On 9/4/20 4:24 AM, Michal Privoznik wrote:

In the past we had to declare @cfg and then explicitly unref it.
But now, with glib we can use g_autoptr() which will do the unref
automatically and thus is more bulletproof.

Signed-off-by: Michal Privoznik 



Reviewed-by: Laine Stump 

Re: [PATCH] qemu: Do not error out when setting affinity failed

[Ján Tomko]

On a Friday in 2020, Martin Kletzander wrote:

On Fri, Sep 04, 2020 at 01:32:24PM +0100, Daniel P. Berrangé wrote:

On Fri, Sep 04, 2020 at 02:22:14PM +0200, Martin Kletzander wrote:

At least in a particular scenario described in the code.  Basically when
libvirtd is running without CAP_SYS_NICE (e.g. in a container) and it is trying
to set QEMU affinity to all CPUs (because there is no setting requested in the
XML) it fails.  But if we ignore the failure in this particular case than you
can limit the CPUs used by controlling the affinity for libvirtd itself.

In any other case (anything requested in the XML, pinning a live domain, etc.)
the call is still considered fatal and the action errors out.

Resolves: https://bugzilla.redhat.com/1819801


I'd prefer if this commit message outlined the reason why this change is
ok, instead of just pointing to the BZ

eg add the following text:


Consider a host with 8 CPUs. There are the following possible scenarios

1. Bare metal; libvirtd has affinity of 8 CPUs; QEMU should get 8 CPUs

2. Bare metal; libvirtd has affinity of 2 CPUs; QEMU should get 8 CPUs

3. Container has affinity of 8 CPUs; libvirtd has affinity of 8 CPus;
 QEMU should get 8 CPUs

4. Container has affinity of 8 CPUs; libvirtd has affinity of 2 CPus;
 QEMU should get 8 CPUs

5. Container has affinity of 4 CPUs; libvirtd has affinity of 4 CPus;
 QEMU should get 4 CPUs

6. Container has affinity of 4 CPUs; libvirtd has affinity of 2 CPus;
 QEMU should get 4 CPUs

Scenarios 1 & 2 always work unless systemd restricted libvirtd privs.

Scenario 3 works because libvirt checks current affinity first and
skips the sched_setaffinity call, avoiding the SYS_NICE issue

Scenario 4 works only if CAP_SYS_NICE is availalbe

Scenarios 5 & 6 works only if CAP_SYS_NICE is present *AND* the cgroups
cpuset is not set on the container.

If libvirt blindly ignores the sched_setaffinity failure, then scenarios
4, 5 and 6 should all work, but with caveat in case 4 and 6, that
QEMU will only get 2 CPUs instead of the possible 8 and 4 respectively.
This is still better than failing.

Therefore libvirt can blindly ignore the setaffinity failure, but *ONLY*
ignore it when there was no affinity specified in the XML config.
If user specified affinity explicitly, libvirt must report an error if
it can't be honoured.



I replaced my commit message with yours.



Suggested-by: Daniel P. Berrangé 
Signed-off-by: Martin Kletzander 
---
src/qemu/qemu_process.c | 41 ++---
1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index cfe09d632633..270bb37d3682 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2528,6 +2528,7 @@ qemuProcessGetAllCpuAffinity(virBitmapPtr *cpumapRet)
static int
qemuProcessInitCpuAffinity(virDomainObjPtr vm)
{
+bool settingAll = false;
g_autoptr(virBitmap) cpumapToSet = NULL;
virDomainNumatuneMemMode mem_mode;
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2566,13 +2567,30 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
if (!(cpumapToSet = virBitmapNewCopy(vm->def->cputune.emulatorpin)))
return -1;
} else {
+settingAll = true;
if (qemuProcessGetAllCpuAffinity() < 0)
return -1;
}

if (cpumapToSet &&
virProcessSetAffinity(vm->pid, cpumapToSet) < 0) {
-return -1;
+/*
+ * We only want to error out if we failed to set the affinity to
+ * user-requested mapping.  If we are just trying to reset the affinity
+ * to all CPUs and this fails it can only be an issue if:
+ *  1) libvirtd does not have CAP_SYS_NICE
+ *  2) libvirtd does not run on all CPUs
+ *
+ * However since this scenario is very improbable, we rather skip
+ * reporting the error because it helps running libvirtd in a a 
scenario
+ * where pinning is handled by someone else.


The patch as-is reports the error in all cases, it is merely ignored in
some cases.

virResetLastError clears the thread-local error structure, but it has no
power to remove the error from the log. It mostly should not be used
outside of public APIs where it makes sure we don't leave an error
from a previous API invocation there.

Reporting harmless errors confuses users and we have made changes in the
past specifically to suppress harmless errors when libvirt is run
in a container.



I wouldn't call this scenario "improbably" - it is entirely expected
by some of our users. Replace these three lines with

"This scenario can easily occurr when libvirtd is run


*occur


 inside a container with restrictive permissions and CPU
 pinning"



Yeah, I meant "improbable on bare metal".  I replaced them with your 
explanation.


Note that in the version pushed to master, you only replaced the text in
one of the two comments.

Jano



With the text changes

Reviewed-by: Daniel P. Berrangé 

Re: [PATCH 1/3] qemu_interface: Fix @cfg refcounting in qemuInterfacePrepareSlirp()

[Laine Stump]

On 9/4/20 4:24 AM, Michal Privoznik wrote:

In the qemuInterfacePrepareSlirp() function, the qemu driver
config is obtained (via virQEMUDriverGetConfig()), but it is
never unrefed leading to mangled refcounter.

Fixes: 9145b3f1cc334e946b3f9ea45d6c24c868301e6f
Signed-off-by: Michal Privoznik 



Reviewed-by: Laine Stump 

Re: [libvirt PATCH v4 00/11] remote: introduce a custom netcat impl for ssh tunnelling

[Michal Prívozník]

On 8/7/20 7:40 PM, Daniel P. Berrangé wrote:

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.

v4: Now with many fixed bugs to make it actually work
v3: Now with more meson and less autotools !

Daniel P. Berrangé (11):
   rpc: merge logic for generating remote SSH shell script
   remote: push logic for default netcat binary into common helper
   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 virt-ssh-helper binary
   rpc: switch order of args in virNetClientNewSSH
   rpc: use new virt-ssh-helper binary for remote tunnelling
   remote: fix error reporting for invalid daemon mode

  build-aux/syntax-check.mk  |   2 +-
  docs/uri.html.in   |  24 +-
  libvirt.spec.in|   2 +
  po/POTFILES.in |   2 +
  src/libvirt_remote.syms|   1 +
  src/remote/meson.build |  18 ++
  src/remote/remote_driver.c | 331 +
  src/remote/remote_sockets.c| 277 +
  src/remote/remote_sockets.h|  70 ++
  src/remote/remote_ssh_helper.c | 425 +
  src/rpc/virnetclient.c | 167 +
  src/rpc/virnetclient.h |  29 ++-
  src/rpc/virnetsocket.c |  37 +--
  src/rpc/virnetsocket.h |   4 +-
  tests/virnetsockettest.c   |  12 +-
  15 files changed, 1030 insertions(+), 371 deletions(-)
  create mode 100644 src/remote/remote_sockets.c
  create mode 100644 src/remote/remote_sockets.h
  create mode 100644 src/remote/remote_ssh_helper.c



Reviewed-by: Michal Privoznik 

Michal

Re: [libvirt PATCH] cpu_map: Use g_auto* in cpuMapLoad

[Ján Tomko]

On a Friday in 2020, Tim Wiederhake wrote:

Signed-off-by: Tim Wiederhake 
---
src/cpu/cpu_map.c | 33 -
1 file changed, 12 insertions(+), 21 deletions(-)



Reviewed-by: Ján Tomko 
and pushed.

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH] Add FreeBSD 11.4 CI job on Cirrus

[Ján Tomko]

On a Friday in 2020, Daniel P. Berrangé wrote:

The previous 11.3 image provided by Cirrus did not boot, but they have
now provided a working 11.4 image

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

NB, as expected this CI job fails due to the bug that Roman
identified here:

 https://www.redhat.com/archives/libvir-list/2020-September/msg00151.html

.gitlab-ci.yml| 9 +
ci/cirrus/libvirt-freebsd-11.vars | 9 +
2 files changed, 18 insertions(+)
create mode 100644 ci/cirrus/libvirt-freebsd-11.vars



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH] util: re-add conditional for ifi_iqdrops field for macOS

[Ján Tomko]

On a Friday in 2020, Daniel P. Berrangé wrote:

The conditional was removed in

 commit ebbf8ebe4fa6f9d43b40673f0f2dad6bf50e2085
 Author: Ján Tomko 
 Date:   Tue Sep 1 22:56:37 2020 +0200

   util: virnetdevtap: stats: fix txdrop on FreeBSD

That commit was correct about this no longer being required for FreeBSD,
but missed that the code is also built on macOS.

Rather than testing for this field in meson though, we can simply use
a platform conditional test in the code.

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

Pushed to fix macOS CI build jobs



Thank you.

Jano


src/util/virnetdevtap.c | 8 
1 file changed, 8 insertions(+)



signature.asc
Description: PGP signature

Re: [libvirt PATCH] wireshark: Don't include config.h

[Michal Prívozník]

On 9/4/20 2:21 PM, Andrea Bolognani wrote:

On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:

On 9/4/20 10:33 AM, Andrea Bolognani wrote:

I should have pushed the branch to GitLab first:

https://gitlab.com/abologna/libvirt/-/pipelines/185421025

We might be able to adopt a more nuanced approach, where we use
ws_version.h where available and fall back to config.h where it's the
only option, but I'm not quite sure how to implement that in Meson
and can't spare the time to learn right now.

Are either you or Michal interested in giving it a try?


I had this patched marked for review but did not get to it yesterday, sorry.


No need to apologise :)


I understand your reasoning and agree that the current code looks bad.
But this is just the way it used to be when you wanted to built plugins
outside of wireshark. I had a long discussion with wireshark devels when
our plugin was being written. Part of them did not want other projects
to build plugins from outside at all (which didn't really work for us
because our RPC was changing a lot back then and we would have to wait
full release cycle of wireshark to dissect new procedures), and part was
at least willing to merge my patches that made our code less horrible.

And truth to be told, I did not really watch the discussion on wireshark
end since and with your patch it looks they moved towards making it
easier for other to build plugins out of wireshark's source.

End of history class.

The patch looks good. ws_version.h was introduced with 2.9.0 release
which is 1.5 years old. Given that the dissector is aimed mostly on us,
developers to help us debug RPC issues, I think we can safely bump the
minimal wireshark version required (currently 2.4.0 which is 3 years old).


That sounds reasonable in theory, but if you look at

   https://gitlab.com/abologna/libvirt/-/pipelines/185421025

you'll see that even platforms that ship pretty recent Wireshark[1]
don't include ws_version.h among the headers.

Not building the dissector on those non-obsolete platforms seems
excessively harsh, so I think an approach similar to the one I
described above is still necessary. And at that point, you might as
well not bump the minimum required version and keep building the
dissector on the current list of platforms...


[1] Debian sid has 3.2.6 and Ubuntu 20.04 has 3.2.3


Any idea why they are not installing the file? Because while current 
solution is hacky, intentionally removing a header file that a package 
wants installed is way worse.


Michal

Re: [PATCH] tests: add FreeBSD dependencies

[Daniel P . Berrangé]

On Thu, Sep 03, 2020 at 12:59:43PM +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 03, 2020 at 01:49:11PM +0200, Pavel Hrdina wrote:
> > On Thu, Sep 03, 2020 at 03:21:51PM +0400, Roman Bogorodskiy wrote:
> > >   Daniel P. Berrangé wrote:
> > > 
> > > > On Thu, Sep 03, 2020 at 02:52:41PM +0400, Roman Bogorodskiy wrote:
> > > > >   Daniel P. Berrangé wrote:
> > > > > 
> > > > > > On Thu, Sep 03, 2020 at 02:21:37PM +0400, Roman Bogorodskiy wrote:
> > > > > > > Add some FreeBSD-specific libraries (-lutil, -lkvm) to tests 
> > > > > > > dependencies.
> > > > > > > 
> > > > > > > Without that, FreeBSD 11.x, which uses the GNU ld, fails to link 
> > > > > > > tests.
> > > > > > > Interestingly, newer FreeBSD versions that use LLVM ld tolerate 
> > > > > > > this
> > > > > > > behaviour and builds successfully as is.
> > > > > > 
> > > > > > Hmm, we need a CI job for FreeBSD 11 added
> > > > > > 
> > > > > > Cirrus supports FreeBSD 11.4 so ought to be possible to add it to 
> > > > > > our
> > > > > > matrix.
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Roman Bogorodskiy 
> > > > > > > ---
> > > > > > >  tests/meson.build | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/tests/meson.build b/tests/meson.build
> > > > > > > index ad13e2de60..ea2458efbc 100644
> > > > > > > --- a/tests/meson.build
> > > > > > > +++ b/tests/meson.build
> > > > > > > @@ -10,11 +10,13 @@ tests_dep = declare_dependency(
> > > > > > >  dlopen_dep,
> > > > > > >  glib_dep,
> > > > > > >  gnutls_dep,
> > > > > > > +kvm_dep,
> > > > > > 
> > > > > > Makes sense, as we don't reference kvm_dep anywhere.
> > > > > > 
> > > > > > >  libnl_dep,
> > > > > > >  libxml_dep,
> > > > > > >  rpc_dep,
> > > > > > >  sasl_dep,
> > > > > > >  selinux_dep,
> > > > > > > +util_dep,
> > > > > > 
> > > > > > In the top level meson.build, we appear to add -lutil as a linker
> > > > > > arg to the entire project, so i'm surprised this was needed.
> > > > > 
> > > > > -lutil was actually the first issue spotted, and when fixed, -lkvm
> > > > > showed up.
> > > > > 
> > > > > Here's the original report I got:
> > > > > 
> > > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=249056
> > > > > 
> > > > > It contains some initial thoughts on this issue; there I also assumed
> > > > > that add_global_link_arguments() could fix the issue, but was 
> > > > > satisfied
> > > > > with the current solution.
> > > > 
> > > > Hmm, this is all very strange if I'm reading the the
> > > > add_project_link_arguments doc correctly it should apply to everything
> > > > we have. The difference between add_project and add_global  is just
> > > > about meson subprojects, but the tests are in our main project.
> > > > 
> > > > If add_project_link_arguments isn't working as we expect, we might be
> > > > better removing its use entirely in favour of explicitly adding the
> > > > _deps objects in the particular places they are needed.
> > > 
> > > I've just tried this change:
> > > 
> > > --- meson.build.orig2020-09-03 11:14:20.233605000 +
> > > +++ meson.build 2020-09-03 11:14:43.661313000 +
> > > @@ -1405,7 +1405,7 @@
> > > 
> > >  util_dep = cc.find_library('util', required: false)
> > >  if util_dep.found()
> > > -  add_project_link_arguments('-lutil', language: 'c')
> > > +  add_global_link_arguments('-lutil', language: 'c')
> > >  endif
> > > 
> > >  if not get_option('virtualport').disabled()
> > > 
> > > It doesn't work, the build is still failing with it.
> > 
> > This is strange, after checking the freebsd bug there is -lutil and it
> > still fails which I don't understand:
> > 
> > [846/936] cc  -o tests/domaincapstest 
> > 'tests/59830eb@@domaincapstest@exe/domaincapstest.c.o'
> > -I/usr/local/include -L/usr/local/lib -Wl,--as-needed -Wl,--no-undefined 
> > -Wl,-O1 -Wl,-export-dynamic
> > -pie -Wl,--whole-archive -Wl,--start-group tests/libtest_utils.a 
> > tests/libtest_file_wrapper.a
> > -Wl,--no-whole-archive -lkvm -lintl -lutil -O2 -pipe 
> > -fstack-protector-strong -fno-strict-aliasing
> >^
> >here you can see there is -lutil
> > 
> >  ^ and here we have -lkvm as well
> > 
> > -fstack-protector -fstack-protector-strong src/libvirt.so.0.6007.0 
> > src/bhyve/libvirt_driver_bhyve_impl.a
> > -Wl,-export-dynamic -ldl /usr/local/lib/libglib-2.0.so 
> > /usr/local/lib/libintl.so /usr/local/lib/libgobject-2.0.so
> > /usr/local/lib/libgio-2.0.so /usr/local/lib/libgnutls.so 
> > /usr/local/lib/libxml2.so -Wl,--end-group -Wl,-z,relro
> > -Wl,-z,now -Wl,-export-dynamic -Wl,-export-dynamic -Wl,-export-dynamic 
> > '-Wl,-rpath,$ORIGIN/../src:$ORIGIN/../src/bhyve'
> > -Wl,-rpath-link,/wrkdirs/usr/ports/devel/libvirt/work/libvirt-6.7.0/_build/src
> >  
> > -Wl,-rpath-link,/wrkdirs/usr/ports/devel/libvirt/work/libvirt-6.7.0/_build/src/bhyve
> > FAILED: 

[PATCH] Fix linkage to libutil and libkvm on FreeBSD 11

[Daniel P . Berrangé]

We are currently adding -lutil and -lkvm to the linker using the
add_project_link_arguments method. On FreeBSD 11.4, this results in
build errors because the args appear too early in the command line.

We need to pass the libraries as dependencies so that they get placed
at the same point in the linker args as other dependencies.

Signed-off-by: Daniel P. Berrangé 
---
 meson.build   | 16 +++-
 src/bhyve/meson.build | 15 +++
 src/util/meson.build  | 26 +-
 3 files changed, 35 insertions(+), 22 deletions(-)

Using the CI patch I posted earlier to add FreeBSD 11:

  https://gitlab.com/berrange/libvirt/-/pipelines/185834799


diff --git a/meson.build b/meson.build
index 1eadea33bf..c30ff187aa 100644
--- a/meson.build
+++ b/meson.build
@@ -1086,7 +1086,8 @@ endif
 # Check for BSD kvm (kernel memory interface)
 if host_machine.system() == 'freebsd'
   kvm_dep = cc.find_library('kvm')
-  add_project_link_arguments('-lkvm', language: 'c')
+else
+  kvm_dep = disabler()
 endif
 
 libiscsi_version = '1.18.0'
@@ -1203,11 +1204,9 @@ have_gnu_gettext_tools = false
 if not get_option('nls').disabled()
   have_gettext = cc.has_function('gettext')
   if not have_gettext
-intl_lib = cc.find_library('intl', required: false)
-have_gettext = intl_lib.found()
-if have_gettext
-  add_project_link_arguments('-lintl', language: 'c')
-endif
+intl_dep = cc.find_library('intl', required: false)
+  else
+intl_dep = disabler()
   endif
   if not have_gettext and get_option('nls').enabled()
 error('gettext() is required to build libvirt')
@@ -1235,6 +1234,8 @@ if not get_option('nls').disabled()
   have_gnu_gettext_tools = true
 endif
   endif
+else
+  intl_dep = disabler()
 endif
 
 numactl_dep = cc.find_library('numa', required: get_option('numactl'))
@@ -1402,9 +1403,6 @@ if udev_dep.found()
 endif
 
 util_dep = cc.find_library('util', required: false)
-if util_dep.found()
-  add_project_link_arguments('-lutil', language: 'c')
-endif
 
 if not get_option('virtualport').disabled()
   if cc.has_header_symbol('linux/if_link.h', 'IFLA_PORT_MAX')
diff --git a/src/bhyve/meson.build b/src/bhyve/meson.build
index 7d54718820..c382f64aee 100644
--- a/src/bhyve/meson.build
+++ b/src/bhyve/meson.build
@@ -14,15 +14,22 @@ driver_source_files += bhyve_sources
 stateful_driver_source_files += bhyve_sources
 
 if conf.has('WITH_BHYVE')
+  bhyve_driver_deps = [
+access_dep,
+src_dep,
+  ]
+  if kvm_dep.found()
+bhyve_driver_deps += kvm_dep
+  endif
+  if util_dep.found()
+bhyve_driver_deps += util_dep
+  endif
   bhyve_driver_impl = static_library(
 'virt_driver_bhyve_impl',
 [
   bhyve_sources,
 ],
-dependencies: [
-  access_dep,
-  src_dep,
-],
+dependencies: bhyve_driver_deps,
 include_directories: [
   conf_inc_dir,
   hypervisor_inc_dir,
diff --git a/src/util/meson.build b/src/util/meson.build
index f7092cc3f1..c899f232e6 100644
--- a/src/util/meson.build
+++ b/src/util/meson.build
@@ -172,15 +172,7 @@ io_helper_sources = [
   'iohelper.c',
 ]
 
-virt_util_lib = static_library(
-  'virt_util',
-  [
-util_sources,
-util_public_sources,
-keycode_gen_sources,
-dtrace_gen_headers,
-  ],
-  dependencies: [
+virt_util_deps = [
 acl_dep,
 audit_dep,
 capng_dep,
@@ -195,7 +187,23 @@ virt_util_lib = static_library(
 thread_dep,
 win32_dep,
 yajl_dep,
+  ]
+if util_dep.found()
+  virt_util_deps += util_dep
+endif
+if intl_dep.found()
+  virt_util_deps += intl_dep
+endif
+
+virt_util_lib = static_library(
+  'virt_util',
+  [
+util_sources,
+util_public_sources,
+keycode_gen_sources,
+dtrace_gen_headers,
   ],
+  dependencies: virt_util_deps,
 )
 
 libvirt_libs += virt_util_lib
-- 
2.26.2

Re: [PATCH] qemu: Do not error out when setting affinity failed

[Martin Kletzander]

On Fri, Sep 04, 2020 at 01:32:24PM +0100, Daniel P. Berrangé wrote:

On Fri, Sep 04, 2020 at 02:22:14PM +0200, Martin Kletzander wrote:

At least in a particular scenario described in the code.  Basically when
libvirtd is running without CAP_SYS_NICE (e.g. in a container) and it is trying
to set QEMU affinity to all CPUs (because there is no setting requested in the
XML) it fails.  But if we ignore the failure in this particular case than you
can limit the CPUs used by controlling the affinity for libvirtd itself.

In any other case (anything requested in the XML, pinning a live domain, etc.)
the call is still considered fatal and the action errors out.

Resolves: https://bugzilla.redhat.com/1819801


I'd prefer if this commit message outlined the reason why this change is
ok, instead of just pointing to the BZ

eg add the following text:


Consider a host with 8 CPUs. There are the following possible scenarios

1. Bare metal; libvirtd has affinity of 8 CPUs; QEMU should get 8 CPUs

2. Bare metal; libvirtd has affinity of 2 CPUs; QEMU should get 8 CPUs

3. Container has affinity of 8 CPUs; libvirtd has affinity of 8 CPus;
  QEMU should get 8 CPUs

4. Container has affinity of 8 CPUs; libvirtd has affinity of 2 CPus;
  QEMU should get 8 CPUs

5. Container has affinity of 4 CPUs; libvirtd has affinity of 4 CPus;
  QEMU should get 4 CPUs

6. Container has affinity of 4 CPUs; libvirtd has affinity of 2 CPus;
  QEMU should get 4 CPUs

Scenarios 1 & 2 always work unless systemd restricted libvirtd privs.

Scenario 3 works because libvirt checks current affinity first and
skips the sched_setaffinity call, avoiding the SYS_NICE issue

Scenario 4 works only if CAP_SYS_NICE is availalbe

Scenarios 5 & 6 works only if CAP_SYS_NICE is present *AND* the cgroups
cpuset is not set on the container.

If libvirt blindly ignores the sched_setaffinity failure, then scenarios
4, 5 and 6 should all work, but with caveat in case 4 and 6, that
QEMU will only get 2 CPUs instead of the possible 8 and 4 respectively.
This is still better than failing.

Therefore libvirt can blindly ignore the setaffinity failure, but *ONLY*
ignore it when there was no affinity specified in the XML config.
If user specified affinity explicitly, libvirt must report an error if
it can't be honoured.



I replaced my commit message with yours.



Suggested-by: Daniel P. Berrangé 
Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_process.c | 41 ++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index cfe09d632633..270bb37d3682 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2528,6 +2528,7 @@ qemuProcessGetAllCpuAffinity(virBitmapPtr *cpumapRet)
 static int
 qemuProcessInitCpuAffinity(virDomainObjPtr vm)
 {
+bool settingAll = false;
 g_autoptr(virBitmap) cpumapToSet = NULL;
 virDomainNumatuneMemMode mem_mode;
 qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2566,13 +2567,30 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
 if (!(cpumapToSet = virBitmapNewCopy(vm->def->cputune.emulatorpin)))
 return -1;
 } else {
+settingAll = true;
 if (qemuProcessGetAllCpuAffinity() < 0)
 return -1;
 }

 if (cpumapToSet &&
 virProcessSetAffinity(vm->pid, cpumapToSet) < 0) {
-return -1;
+/*
+ * We only want to error out if we failed to set the affinity to
+ * user-requested mapping.  If we are just trying to reset the affinity
+ * to all CPUs and this fails it can only be an issue if:
+ *  1) libvirtd does not have CAP_SYS_NICE
+ *  2) libvirtd does not run on all CPUs
+ *
+ * However since this scenario is very improbable, we rather skip
+ * reporting the error because it helps running libvirtd in a a 
scenario
+ * where pinning is handled by someone else.


I wouldn't call this scenario "improbably" - it is entirely expected
by some of our users. Replace these three lines with

 "This scenario can easily occurr when libvirtd is run
  inside a container with restrictive permissions and CPU
  pinning"



Yeah, I meant "improbable on bare metal".  I replaced them with your 
explanation.



With the text changes

Reviewed-by: Daniel P. Berrangé 



Thanks, pushed.



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


signature.asc
Description: PGP signature

Re: [PATCH] qemu: Do not error out when setting affinity failed

[Daniel P . Berrangé]

On Fri, Sep 04, 2020 at 02:22:14PM +0200, Martin Kletzander wrote:
> At least in a particular scenario described in the code.  Basically when
> libvirtd is running without CAP_SYS_NICE (e.g. in a container) and it is 
> trying
> to set QEMU affinity to all CPUs (because there is no setting requested in the
> XML) it fails.  But if we ignore the failure in this particular case than you
> can limit the CPUs used by controlling the affinity for libvirtd itself.
> 
> In any other case (anything requested in the XML, pinning a live domain, etc.)
> the call is still considered fatal and the action errors out.
> 
> Resolves: https://bugzilla.redhat.com/1819801

I'd prefer if this commit message outlined the reason why this change is
ok, instead of just pointing to the BZ

eg add the following text:


Consider a host with 8 CPUs. There are the following possible scenarios

1. Bare metal; libvirtd has affinity of 8 CPUs; QEMU should get 8 CPUs

2. Bare metal; libvirtd has affinity of 2 CPUs; QEMU should get 8 CPUs

3. Container has affinity of 8 CPUs; libvirtd has affinity of 8 CPus;
   QEMU should get 8 CPUs

4. Container has affinity of 8 CPUs; libvirtd has affinity of 2 CPus;
   QEMU should get 8 CPUs

5. Container has affinity of 4 CPUs; libvirtd has affinity of 4 CPus;
   QEMU should get 4 CPUs

6. Container has affinity of 4 CPUs; libvirtd has affinity of 2 CPus;
   QEMU should get 4 CPUs

Scenarios 1 & 2 always work unless systemd restricted libvirtd privs.

Scenario 3 works because libvirt checks current affinity first and
skips the sched_setaffinity call, avoiding the SYS_NICE issue

Scenario 4 works only if CAP_SYS_NICE is availalbe

Scenarios 5 & 6 works only if CAP_SYS_NICE is present *AND* the cgroups
cpuset is not set on the container.

If libvirt blindly ignores the sched_setaffinity failure, then scenarios
4, 5 and 6 should all work, but with caveat in case 4 and 6, that
QEMU will only get 2 CPUs instead of the possible 8 and 4 respectively.
This is still better than failing.

Therefore libvirt can blindly ignore the setaffinity failure, but *ONLY*
ignore it when there was no affinity specified in the XML config.
If user specified affinity explicitly, libvirt must report an error if
it can't be honoured.

> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Martin Kletzander 
> ---
>  src/qemu/qemu_process.c | 41 ++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index cfe09d632633..270bb37d3682 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2528,6 +2528,7 @@ qemuProcessGetAllCpuAffinity(virBitmapPtr *cpumapRet)
>  static int
>  qemuProcessInitCpuAffinity(virDomainObjPtr vm)
>  {
> +bool settingAll = false;
>  g_autoptr(virBitmap) cpumapToSet = NULL;
>  virDomainNumatuneMemMode mem_mode;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
> @@ -2566,13 +2567,30 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
>  if (!(cpumapToSet = virBitmapNewCopy(vm->def->cputune.emulatorpin)))
>  return -1;
>  } else {
> +settingAll = true;
>  if (qemuProcessGetAllCpuAffinity() < 0)
>  return -1;
>  }
>  
>  if (cpumapToSet &&
>  virProcessSetAffinity(vm->pid, cpumapToSet) < 0) {
> -return -1;
> +/*
> + * We only want to error out if we failed to set the affinity to
> + * user-requested mapping.  If we are just trying to reset the 
> affinity
> + * to all CPUs and this fails it can only be an issue if:
> + *  1) libvirtd does not have CAP_SYS_NICE
> + *  2) libvirtd does not run on all CPUs
> + *
> + * However since this scenario is very improbable, we rather skip
> + * reporting the error because it helps running libvirtd in a a 
> scenario
> + * where pinning is handled by someone else.

I wouldn't call this scenario "improbably" - it is entirely expected
by some of our users. Replace these three lines with

  "This scenario can easily occurr when libvirtd is run
   inside a container with restrictive permissions and CPU
   pinning"


With the text changes

Reviewed-by: Daniel P. Berrangé 


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

[libvirt PATCH] cpu_map: Use g_auto* in cpuMapLoad

[Tim Wiederhake]

Signed-off-by: Tim Wiederhake 
---
 src/cpu/cpu_map.c | 33 -
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/src/cpu/cpu_map.c b/src/cpu/cpu_map.c
index 913e34a067..cbf90d1395 100644
--- a/src/cpu/cpu_map.c
+++ b/src/cpu/cpu_map.c
@@ -169,12 +169,11 @@ int cpuMapLoad(const char *arch,
cpuMapLoadCallback modelCB,
void *data)
 {
-xmlDocPtr xml = NULL;
-xmlXPathContextPtr ctxt = NULL;
+g_autoptr(xmlDoc) xml = NULL;
+g_autoptr(xmlXPathContext) ctxt = NULL;
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-char *xpath = NULL;
-int ret = -1;
-char *mapfile;
+g_autofree char *xpath = NULL;
+g_autofree char *mapfile = NULL;
 
 if (!(mapfile = virFileFindResource("index.xml",
 abs_top_srcdir "/src/cpu_map",
@@ -186,11 +185,11 @@ int cpuMapLoad(const char *arch,
 if (arch == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("undefined hardware architecture"));
-goto cleanup;
+return -1;
 }
 
 if (!(xml = virXMLParseFileCtxt(mapfile, )))
-goto cleanup;
+return -1;
 
 virBufferAsprintf(, "./arch[@name='%s']", arch);
 
@@ -201,28 +200,20 @@ int cpuMapLoad(const char *arch,
 if ((ctxt->node = virXPathNode(xpath, ctxt)) == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot find CPU map for %s architecture"), arch);
-goto cleanup;
+return -1;
 }
 
 if (loadData(mapfile, ctxt, "vendor", vendorCB, data) < 0)
-goto cleanup;
+return -1;
 
 if (loadData(mapfile, ctxt, "feature", featureCB, data) < 0)
-goto cleanup;
+return -1;
 
 if (loadData(mapfile, ctxt, "model", modelCB, data) < 0)
-goto cleanup;
+return -1;
 
 if (loadIncludes(ctxt, vendorCB, featureCB, modelCB, data) < 0)
-goto cleanup;
-
-ret = 0;
-
- cleanup:
-xmlXPathFreeContext(ctxt);
-xmlFreeDoc(xml);
-VIR_FREE(xpath);
-VIR_FREE(mapfile);
+return -1;
 
-return ret;
+return 0;
 }
-- 
2.26.2

[PATCH] qemu: Do not error out when setting affinity failed

[Martin Kletzander]

At least in a particular scenario described in the code.  Basically when
libvirtd is running without CAP_SYS_NICE (e.g. in a container) and it is trying
to set QEMU affinity to all CPUs (because there is no setting requested in the
XML) it fails.  But if we ignore the failure in this particular case than you
can limit the CPUs used by controlling the affinity for libvirtd itself.

In any other case (anything requested in the XML, pinning a live domain, etc.)
the call is still considered fatal and the action errors out.

Resolves: https://bugzilla.redhat.com/1819801

Suggested-by: Daniel P. Berrangé 
Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_process.c | 41 ++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index cfe09d632633..270bb37d3682 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2528,6 +2528,7 @@ qemuProcessGetAllCpuAffinity(virBitmapPtr *cpumapRet)
 static int
 qemuProcessInitCpuAffinity(virDomainObjPtr vm)
 {
+bool settingAll = false;
 g_autoptr(virBitmap) cpumapToSet = NULL;
 virDomainNumatuneMemMode mem_mode;
 qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2566,13 +2567,30 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
 if (!(cpumapToSet = virBitmapNewCopy(vm->def->cputune.emulatorpin)))
 return -1;
 } else {
+settingAll = true;
 if (qemuProcessGetAllCpuAffinity() < 0)
 return -1;
 }
 
 if (cpumapToSet &&
 virProcessSetAffinity(vm->pid, cpumapToSet) < 0) {
-return -1;
+/*
+ * We only want to error out if we failed to set the affinity to
+ * user-requested mapping.  If we are just trying to reset the affinity
+ * to all CPUs and this fails it can only be an issue if:
+ *  1) libvirtd does not have CAP_SYS_NICE
+ *  2) libvirtd does not run on all CPUs
+ *
+ * However since this scenario is very improbable, we rather skip
+ * reporting the error because it helps running libvirtd in a a 
scenario
+ * where pinning is handled by someone else.
+ *
+ * See also: https://bugzilla.redhat.com/1819801#c2
+ */
+if (settingAll)
+virResetLastError();
+else
+return -1;
 }
 
 return 0;
@@ -2726,8 +2744,25 @@ qemuProcessSetupPid(virDomainObjPtr vm,
 affinity_cpumask = use_cpumask;
 
 /* Setup legacy affinity. */
-if (affinity_cpumask && virProcessSetAffinity(pid, affinity_cpumask) < 0)
-goto cleanup;
+if (affinity_cpumask && virProcessSetAffinity(pid, affinity_cpumask) < 0) {
+/*
+ * We only want to error out if we failed to set the affinity to
+ * user-requested mapping.  If we are just trying to reset the affinity
+ * to all CPUs and this fails it can only be an issue if:
+ *  1) libvirtd does not have CAP_SYS_NICE
+ *  2) libvirtd does not run on all CPUs
+ *
+ * However since this scenario is very improbable, we rather skip
+ * reporting the error because it helps running libvirtd in a a 
scenario
+ * where pinning is handled by someone else.
+ *
+ * See also: https://bugzilla.redhat.com/1819801#c2
+ */
+if (affinity_cpumask == hostcpumap)
+virResetLastError();
+else
+goto cleanup;
+}
 
 /* Set scheduler type and priority, but not for the main thread. */
 if (sched &&
-- 
2.28.0

Re: [libvirt PATCH] wireshark: Don't include config.h

[Andrea Bolognani]

On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote:
> On 9/4/20 10:33 AM, Andrea Bolognani wrote:
> > I should have pushed the branch to GitLab first:
> > 
> >https://gitlab.com/abologna/libvirt/-/pipelines/185421025
> > 
> > We might be able to adopt a more nuanced approach, where we use
> > ws_version.h where available and fall back to config.h where it's the
> > only option, but I'm not quite sure how to implement that in Meson
> > and can't spare the time to learn right now.
> > 
> > Are either you or Michal interested in giving it a try?
> 
> I had this patched marked for review but did not get to it yesterday, sorry.

No need to apologise :)

> I understand your reasoning and agree that the current code looks bad. 
> But this is just the way it used to be when you wanted to built plugins 
> outside of wireshark. I had a long discussion with wireshark devels when 
> our plugin was being written. Part of them did not want other projects 
> to build plugins from outside at all (which didn't really work for us 
> because our RPC was changing a lot back then and we would have to wait 
> full release cycle of wireshark to dissect new procedures), and part was 
> at least willing to merge my patches that made our code less horrible.
> 
> And truth to be told, I did not really watch the discussion on wireshark 
> end since and with your patch it looks they moved towards making it 
> easier for other to build plugins out of wireshark's source.
> 
> End of history class.
> 
> The patch looks good. ws_version.h was introduced with 2.9.0 release 
> which is 1.5 years old. Given that the dissector is aimed mostly on us, 
> developers to help us debug RPC issues, I think we can safely bump the 
> minimal wireshark version required (currently 2.4.0 which is 3 years old).

That sounds reasonable in theory, but if you look at

  https://gitlab.com/abologna/libvirt/-/pipelines/185421025

you'll see that even platforms that ship pretty recent Wireshark[1]
don't include ws_version.h among the headers.

Not building the dissector on those non-obsolete platforms seems
excessively harsh, so I think an approach similar to the one I
described above is still necessary. And at that point, you might as
well not bump the minimum required version and keep building the
dissector on the current list of platforms...


[1] Debian sid has 3.2.6 and Ubuntu 20.04 has 3.2.3
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] Modify virCPUarmCompare to perform compare actions

[Jiri Denemark]

On Thu, Sep 03, 2020 at 19:50:13 +0800, Zhenyu Zheng wrote:
> Hi,
> 
> Thanks alot for the review and feedback. As for host-passthrough cases, I
> have some other understandings,
> if I understand correctly, what you mean is that if a vm uses
> host-passthrough, it can migrate to any other
> host, since it asks for host-passthrough. In my point of view, I think in
> real cases, there are few different kinds
> of ARM datacenter CPUs on the market, and there CPU capabilities are
> different, so one might create a vm on
> hostA with feature1 and feature2 because it uses host-passthrough and hostA
> has these features. Now in your
> definition(if I understand correctly) of host-passthrough and the current
> code(returns identical directly), this vm
> can be migrated to hostB with only feature1, since there are no
> limitations. If one has some important application
> that depends on feature2, the app will break as feature2 is not available
> on hostB.

And that's why migrating a domain with host-passthrough CPU is generally
not supported. It should work if you have two identical hosts in terms
of CPU, microcode (if applicable to ARM), system settings (not sure
about ARM, but x86 let's you hide some CPU features), kernel and its
command line arguments, kvm modules and their options, and QEMU. If any
of this is different the guest CPU may change in an unexpected way
during migration and there's no way libvirt could know about it or
prevent it in general.

Even if we made the change and compared CPU features, the CPUs may have
other features which libvirt does not know about or the CPUs could even
differ in other aspects which libvirt does not cover in the CPU modeling
code.

But thinking about this and your patch, I guess I know what you are
trying to do. You're not trying to compare the CPU definition from a
domain XML (which was the primary use case for CPU comparison APIs), but
you want to compare the host CPU definition from one host to the host
CPU on the other host to see whether the two hosts might be compatible.

This sounds like a valid use case (not sure it is that useful), but we
need to be careful. But we should make sure implementing this does not
break anything. This means, we need to do different things depending on
the type of the CPU definition we are asked to compare. Guest CPU
definitions should keep the old behaviour (return IDENTICAL) and host
CPU definitions can be compared to the host CPU. But when doing so,
don't get too influenced by x86 code, which I believe is way too
complicated for ARM. Specifically you don't need to create armCompute
and mess with guestData and other stuff there as all you want to do is
compare the two CPU definitions. In x86 the same function is used for
several things, but that's not the case for ARM.

> Considering this, I proposed to add basic checks to compare CPU to
> limit the migration to only the same CPU models. And once the
> capability of ARM driver is enhanced in QEMU or other related
> projects, we can make the compare API better.

Sure, but that would be comparing guest CPU definition with the CPU we
get from QEMU rather than the one we detect ourselves.

Jirka

Re: Various issues when using multiple graphic outputs

[Gerd Hoffmann]

  Hi,

> Well my initial target before I realized that it seemed to affect all
> combinations in different ways was to get a mediated device to work well.
> So If the answer is "just use only one" then I need to find a way to convince
> libvirt to -not- re-add a graphics device when the mdev of the vgpu is
> present.

model='none'.

HTH,
  Gerd

[libvirt PATCH] Add FreeBSD 11.4 CI job on Cirrus

[Daniel P . Berrangé]

The previous 11.3 image provided by Cirrus did not boot, but they have
now provided a working 11.4 image

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

NB, as expected this CI job fails due to the bug that Roman
identified here:

  https://www.redhat.com/archives/libvir-list/2020-September/msg00151.html

 .gitlab-ci.yml| 9 +
 ci/cirrus/libvirt-freebsd-11.vars | 9 +
 2 files changed, 18 insertions(+)
 create mode 100644 ci/cirrus/libvirt-freebsd-11.vars

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4a42eb479f..ee404bf50a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -383,6 +383,15 @@ x64-ubuntu-2004:
   variables:
 NAME: ubuntu-2004
 
+x64-freebsd-11-build:
+  <<: *cirrus_build_job_definition
+  variables:
+NAME: freebsd-11
+CIRRUS_VM_INSTANCE_TYPE: freebsd_instance
+CIRRUS_VM_IMAGE_SELECTOR: image_family
+CIRRUS_VM_IMAGE_NAME: freebsd-11-4
+INSTALL_COMMAND: pkg install -y
+
 x64-freebsd-12-build:
   <<: *cirrus_build_job_definition
   variables:
diff --git a/ci/cirrus/libvirt-freebsd-11.vars 
b/ci/cirrus/libvirt-freebsd-11.vars
new file mode 100644
index 00..7c29c6f899
--- /dev/null
+++ b/ci/cirrus/libvirt-freebsd-11.vars
@@ -0,0 +1,9 @@
+PACKAGING_COMMAND='pkg'
+CC='/usr/bin/clang'
+CCACHE='/usr/local/bin/ccache'
+MAKE='/usr/local/bin/gmake'
+NINJA='/usr/local/bin/ninja'
+PYTHON='/usr/local/bin/python3'
+PIP='/usr/local/bin/pip-3.7'
+PKGS='augeas autoconf automake avahi bash bash-completion ca_root_nss ccache 
chrony cppi curl cyrus-sasl dbus diskscrub dnsmasq fusefs-libs gdb gettext 
gettext-tools git glib gmake gnutls hal libpcap libpciaccess libssh libssh2 
libtool libxml2 libxslt lsof ncurses ninja p5-App-cpanminus patch perl5 pkgconf 
polkit py37-docutils py37-flake8 py37-pip py37-setuptools py37-wheel python3 
qemu-utils radvd readline screen sudo vim yajl'
+PYPI_PKGS='meson==0.54.0'
-- 
2.26.2

Re: Various issues when using multiple graphic outputs

[Daniel P . Berrangé]

On Fri, Sep 04, 2020 at 12:37:06PM +0200, Gerd Hoffmann wrote:
> On Fri, Sep 04, 2020 at 12:05:08PM +0200, Christian Ehrhardt wrote:
> > Hi,
> > I've had continuous issues with this and wanted to reach out
> > if that is a common issue everyone has or just me lacking a little
> > detail on my setup.
> 
> > - QXL with multiple heads
> >   - works fine (so multi display in general seems fine for the
> > involved components)
> 
> Yep.
> 
> > - VGA + QXL, QXL + Virtio, Virtio + QXL:
> >   - Guest only detects primary display
> >   - Can't convince X in the guest to use the second device as well
> >   - lspci lists both devices just fine
> >   - dmesg shows the kernel is initializing them, e.g. drm for virtio
> 
> As far I know xorg can't really deal with multiple display devices.
> So typically you have a single device with multiple connectors, so
> you can hook up multiple monitors.

IIRC, it Xorg can support multiple devices, but it is not zero-conf,
it needs a custom config file to be written, and has some functional
limitations compared to a multi-head layout. It has been at least 10+
years since I did something like this though :-)

> In the virtual world qxl and virtio support multiple heads, so pick
> one of these two and enable as many heads as you need.  Also make sure
> spice-agent is active in the guest, otherwise operating the mouse is a
> PITA.

This is definitely the better approach to get zero-conf though.

> Dunno how things are with wayland.
> 
> > - 2*QXL
> >   - X has issues to initialize on user login
> >   - no error in the log, just hung
> 
> The windows guest driver doesn't support qxl devices with multiple
> outputs and expects one qxl device per head instead.  So use this
> one for windows guests.
> 
> HTH,
>   Gerd
> 

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: Various issues when using multiple graphic outputs

[Christian Ehrhardt]

On Fri, Sep 4, 2020 at 12:37 PM Gerd Hoffmann  wrote:
>
> On Fri, Sep 04, 2020 at 12:05:08PM +0200, Christian Ehrhardt wrote:
> > Hi,
> > I've had continuous issues with this and wanted to reach out
> > if that is a common issue everyone has or just me lacking a little
> > detail on my setup.
>
> > - QXL with multiple heads
> >   - works fine (so multi display in general seems fine for the
> > involved components)
>
> Yep.
>
> > - VGA + QXL, QXL + Virtio, Virtio + QXL:
> >   - Guest only detects primary display
> >   - Can't convince X in the guest to use the second device as well
> >   - lspci lists both devices just fine
> >   - dmesg shows the kernel is initializing them, e.g. drm for virtio
>
> As far I know xorg can't really deal with multiple display devices.
> So typically you have a single device with multiple connectors, so
> you can hook up multiple monitors.
>
> In the virtual world qxl and virtio support multiple heads, so pick
> one of these two and enable as many heads as you need.

Well my initial target before I realized that it seemed to affect all
combinations in different ways was to get a mediated device to work well.
So If the answer is "just use only one" then I need to find a way to convince
libvirt to -not- re-add a graphics device when the mdev of the vgpu is
present.

> Also make sure
> spice-agent is active in the guest, otherwise operating the mouse is a
> PITA.

I think it was active but surely will give it a second look to be sure
- thank you!

> Dunno how things are with wayland.
>
> > - 2*QXL
> >   - X has issues to initialize on user login
> >   - no error in the log, just hung
>
> The windows guest driver doesn't support qxl devices with multiple
> outputs and expects one qxl device per head instead.  So use this
> one for windows guests.
>
> HTH,
>   Gerd
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: Various issues when using multiple graphic outputs

[Gerd Hoffmann]

On Fri, Sep 04, 2020 at 12:05:08PM +0200, Christian Ehrhardt wrote:
> Hi,
> I've had continuous issues with this and wanted to reach out
> if that is a common issue everyone has or just me lacking a little
> detail on my setup.

> - QXL with multiple heads
>   - works fine (so multi display in general seems fine for the
> involved components)

Yep.

> - VGA + QXL, QXL + Virtio, Virtio + QXL:
>   - Guest only detects primary display
>   - Can't convince X in the guest to use the second device as well
>   - lspci lists both devices just fine
>   - dmesg shows the kernel is initializing them, e.g. drm for virtio

As far I know xorg can't really deal with multiple display devices.
So typically you have a single device with multiple connectors, so
you can hook up multiple monitors.

In the virtual world qxl and virtio support multiple heads, so pick
one of these two and enable as many heads as you need.  Also make sure
spice-agent is active in the guest, otherwise operating the mouse is a
PITA.

Dunno how things are with wayland.

> - 2*QXL
>   - X has issues to initialize on user login
>   - no error in the log, just hung

The windows guest driver doesn't support qxl devices with multiple
outputs and expects one qxl device per head instead.  So use this
one for windows guests.

HTH,
  Gerd

[PATCH] util: re-add conditional for ifi_iqdrops field for macOS

[Daniel P . Berrangé]

The conditional was removed in

  commit ebbf8ebe4fa6f9d43b40673f0f2dad6bf50e2085
  Author: Ján Tomko 
  Date:   Tue Sep 1 22:56:37 2020 +0200

util: virnetdevtap: stats: fix txdrop on FreeBSD

That commit was correct about this no longer being required for FreeBSD,
but missed that the code is also built on macOS.

Rather than testing for this field in meson though, we can simply use
a platform conditional test in the code.

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

Pushed to fix macOS CI build jobs

 src/util/virnetdevtap.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index cc6f254aa1..ab5959c646 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -974,12 +974,20 @@ virNetDevTapInterfaceStats(const char *ifname,
 stats->rx_bytes = ifd->ifi_obytes;
 stats->rx_packets = ifd->ifi_opackets;
 stats->rx_errs = ifd->ifi_oerrors;
+# ifndef __APPLE__
 stats->rx_drop = ifd->ifi_oqdrops;
+# else
+stats->rx_drop = 0;
+# endif
 } else {
 stats->tx_bytes = ifd->ifi_obytes;
 stats->tx_packets = ifd->ifi_opackets;
 stats->tx_errs = ifd->ifi_oerrors;
+# ifndef __APPLE__
 stats->tx_drop = ifd->ifi_oqdrops;
+# else
+stats->tx_drop = 0;
+# endif
 stats->rx_bytes = ifd->ifi_ibytes;
 stats->rx_packets = ifd->ifi_ipackets;
 stats->rx_errs = ifd->ifi_ierrors;
-- 
2.26.2

Various issues when using multiple graphic outputs

[Christian Ehrhardt]

Hi,
I've had continuous issues with this and wanted to reach out
if that is a common issue everyone has or just me lacking a little
detail on my setup.

Setup:
- tried qemu up to 4.2
- tried libvirt up to 6.0
- virt-viewer up to 7.0-2build1
- virt-manager up to 2.2.1
- I plan to retry with qemu 5.0, libvirt 6.6 and virt-viewer 9.0 but I
  don't have all pieces ready yet.
- get a Desktop Guest (I've seen it with Ubuntu and Windows, therefore
  I'm rather sure it will affect all desktops)
- configure your guest-xml to use multiple graphic adapters and start it
- connect with virt-viewer to your guest which will open both screens

The exact issue depends on the kind of graphic device I configure.
The following list is ordered in terms of increasing confusion :-):

- QXL with multiple heads
  - works fine (so multi display in general seems fine for the
involved components)
- VGA + QXL, QXL + Virtio, Virtio + QXL:
  - Guest only detects primary display
  - Can't convince X in the guest to use the second device as well
  - lspci lists both devices just fine
  - dmesg shows the kernel is initializing them, e.g. drm for virtio
- 2*QXL
  - X has issues to initialize on user login
  - no error in the log, just hung
- QXL + Mdev
  - mouse handling seems broken
- in gnome I see no mouse pointer at all
- in windows it is just strange
  - has offsets relative to mouse pointer entering
  - and can't move in one random direction (e.g. not up)
- keyboard works just fine
  - I have played with adding/removing different input devices without
much success.
  - I disabled the qxl display via xorg conf without the case getting any better

The last case is what I initially tried and totally confused me at first.

And to be clear, each of those graphics adapter types works fine if being
attached "alone" on the guest (Except mdev alone, as I'm unable to convince
libvirt to only attach the mdev without adding back a qxl graphics adapter).

The Guest XML is the default that virt-manager gives me plus adding the second
graphics adapter (example 2*QXL: https://paste.ubuntu.com/p/cVw8GVZ9dD/).

If this is known in some way or even "yeah try version XY of component Foo" I'm
happy about any hint/pointer you can give me. But if there is a chance that I'm
the only one seeing it I'd like to understand why.

Thanks in advance for thinking into this with me,
Christian

P.S. to avoid cross-posting I'll start with libvirt as it is in the
middle of all involved components.

Re: [libvirt PATCH] wireshark: Don't include config.h

[Michal Prívozník]

On 9/4/20 10:33 AM, Andrea Bolognani wrote:

On Thu, 2020-09-03 at 12:16 -0400, Laine Stump wrote:

On 9/3/20 6:43 AM, Andrea Bolognani wrote:

While both Debian and Fedora include the header in their development
packages for Wireshark, that's not something that the upstream
developers intended and arguably quite wrong, as config.h is obviously
intended to only be used to drive the compilation of Wireshark itself.
The Arch Linux package behaves like the upstream Wireshark package, and
thus libvirt fails to build there.

It seems that there are multiple bugs to be addressed:

* libvirt shouldn't include config.h;

* Debian and Fedora shouldn't be shipping config.h in their Wireshark
  packages;

* Wireshark should not use config.h defines such as HAVE_PLUGINS in
  its public headers, and define a public variant of them instead.

This patch takes care of the first one.

https://gitlab.com/libvirt/libvirt/-/issues/74
Signed-off-by: Andrea Bolognani 


Funny, just this morning I was grepping for " HAVE_" and " WITH_" in
/usr/include, saw a couple of "config.h"s shoot past, and thought "Hmm.
That doesn't seem right! Surely those packages didn't *really* intend to
ship their config.h with their -dev package".


Reviewed-by: Laine Stump 


I should have pushed the branch to GitLab first:

   https://gitlab.com/abologna/libvirt/-/pipelines/185421025

We might be able to adopt a more nuanced approach, where we use
ws_version.h where available and fall back to config.h where it's the
only option, but I'm not quite sure how to implement that in Meson
and can't spare the time to learn right now.

Are either you or Michal interested in giving it a try?



I had this patched marked for review but did not get to it yesterday, sorry.

I understand your reasoning and agree that the current code looks bad. 
But this is just the way it used to be when you wanted to built plugins 
outside of wireshark. I had a long discussion with wireshark devels when 
our plugin was being written. Part of them did not want other projects 
to build plugins from outside at all (which didn't really work for us 
because our RPC was changing a lot back then and we would have to wait 
full release cycle of wireshark to dissect new procedures), and part was 
at least willing to merge my patches that made our code less horrible.


And truth to be told, I did not really watch the discussion on wireshark 
end since and with your patch it looks they moved towards making it 
easier for other to build plugins out of wireshark's source.


End of history class.

The patch looks good. ws_version.h was introduced with 2.9.0 release 
which is 1.5 years old. Given that the dissector is aimed mostly on us, 
developers to help us debug RPC issues, I think we can safely bump the 
minimal wireshark version required (currently 2.4.0 which is 3 years old).


Michal

[PATCH] tools: avoid unused parameter warning when readline is disabled

[Daniel P . Berrangé]

The vshReadlineHistoryAdd stub method does not use its parameter.

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

Pushed to fix minimal build used by layered repo CI

 tools/vsh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index ef2a3f62d7..0e8edcd78c 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2971,7 +2971,7 @@ vshReadline(vshControl *ctl G_GNUC_UNUSED,
 }
 
 void
-vshReadlineHistoryAdd(const char *cmd)
+vshReadlineHistoryAdd(const char *cmd G_GNUC_UNUSED)
 {
 /* empty */
 }
-- 
2.26.2

[PATCH v3 1/2] qemu: Allow migration over UNIX socket

[Martin Kletzander]

This allows:

 a) migration without access to network

 b) complete control of the migration stream

 c) easy migration between containerised libvirt daemons on the same host

Resolves: https://bugzilla.redhat.com/1638889

Signed-off-by: Martin Kletzander 
---
 docs/manpages/virsh.rst  |  13 ++-
 docs/migration.html.in   |  33 
 src/qemu/qemu_driver.c   |  22 -
 src/qemu/qemu_migration.c| 138 +++
 src/qemu/qemu_migration_params.c |   9 ++
 src/qemu/qemu_migration_params.h |   3 +
 src/qemu/qemu_monitor.c  |  15 
 src/qemu/qemu_monitor.h  |   4 +
 8 files changed, 198 insertions(+), 39 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index a0d6c3fadda6..ca5acf84cad2 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -3270,6 +3270,14 @@ There are a few scenarios where specifying *migrateuri* 
may help:
   might be specified to choose a specific port number outside the default 
range in
   order to comply with local firewall policies.
 
+* The *desturi* uses UNIX transport method.  In this advanced case libvirt
+  should not guess a *migrateuri* and it should be specified using
+  UNIX socket path URI:
+
+.. code-block::
+
+  unix:///path/to/socket
+
 See `https://libvirt.org/migration.html#uris 
`_ for more details on
 migration URIs.
 
@@ -3296,8 +3304,9 @@ specific parameters separated by '&'. Currently 
recognized parameters are
 Optional *listen-address* sets the listen address that hypervisor on the
 destination side should bind to for incoming migration. Both IPv4 and IPv6
 addresses are accepted as well as hostnames (the resolving is done on
-destination). Some hypervisors do not support this feature and will return an
-error if this parameter is used.
+destination).  Some hypervisors do not support specifying the listen address 
and
+will return an error if this parameter is used. This parameter cannot be used 
if
+*desturi* uses UNIX transport method.
 
 Optional *disks-port* sets the port that hypervisor on destination side should
 bind to for incoming disks traffic. Currently it is supported only by QEMU.
diff --git a/docs/migration.html.in b/docs/migration.html.in
index e95ee9de6f1b..162c202227b9 100644
--- a/docs/migration.html.in
+++ b/docs/migration.html.in
@@ -201,6 +201,9 @@
 numbers. In the latter case the management application may wish
 to choose a specific port number outside the default range in order
 to comply with local firewall policies.
+  The second URI uses UNIX transport method.  In this advanced case
+libvirt should not guess a *migrateuri* and it should be specified 
using
+UNIX socket path URI: unix:///path/to/socket.
 
 
 Configuration file handling
@@ -628,5 +631,35 @@ virsh migrate --p2p --tunnelled web1 
qemu+ssh://desthost/system qemu+ssh://10.0.
   Supported by QEMU driver
 
 
+
+Migration using only UNIX sockets
+
+
+  In niche scenarios where libvirt daemon does not have access to the
+  network (e.g. running in a restricted container on a host that has
+  accessible network), when a management application wants to have complete
+  control over the transfer or when migrating between two containers on the
+  same host all the communication can be done using UNIX sockets.  This
+  includes connecting to non-standard socket path for the destination
+  daemon, using UNIX sockets for hypervisor's communication or for the NBD
+  data transfer.  All of that can be used with both peer2peer and direct
+  migration options.
+
+
+
+  Example using /tmp/migdir as a directory representing the
+  same path visible from both libvirt daemons.  That can be achieved by
+  bind-mounting the same directory to different containers running separate
+  daemons or forwarding connections to these sockets manually
+  (using socat, netcat or a custom piece of
+  software):
+
+virsh migrate web1 [--p2p] --copy-storage-all 
'qemu+unix:///system?socket=/tmp/migdir/test-sock-driver' 
'unix:///tmp/migdir/test-sock-qemu' --disks-uri unix:///tmp/migdir/test-sock-nbd
+
+
+
+  Supported by QEMU driver
+
+
   
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 80c56fec603e..ce72e1021d16 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11480,7 +11480,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn,
 const char *dom_xml = NULL;
 const char *dname = NULL;
 const char *uri_in = NULL;
-const char *listenAddress = cfg->migrationAddress;
+const char *listenAddress = NULL;
 int nbdPort = 0;
 int nmigrate_disks;
 g_autofree const char **migrate_disks = NULL;
@@ -11530,6 +11530,17 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn,
 return -1;
 }
 
+if (listenAddress) {
+if 

[PATCH v3 2/2] news: qemu: Allow migration over UNIX sockets

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
Reviewed-by: Jiri Denemark 
---
 NEWS.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 05d04b29e1c8..008a6358c648 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -22,6 +22,12 @@ v6.8.0 (unreleased)
 
 * **Improvements**
 
+  * qemu: Allow migration over UNIX sockets
+
+QEMU migration can now be performed completely over UNIX sockets. This is
+useful for containerised scenarios and can be used in both peer2peer and
+direct migrations.
+
 * **Bug fixes**
 
 
-- 
2.28.0

[PATCH v3 0/2] qemu: Allow migration over UNIX sockets

[Martin Kletzander]

v3:
 - Forbid listen-address with unix: desturi

v2:
 - Allow TLS and parallel migration as well
 - Use simpler unix socket URIs

Resolves: https://bugzilla.redhat.com/1638889

Martin Kletzander (2):
  qemu: Allow migration over UNIX socket
  news: qemu: Allow migration over UNIX sockets

 NEWS.rst |   6 ++
 docs/manpages/virsh.rst  |  13 ++-
 docs/migration.html.in   |  33 
 src/qemu/qemu_driver.c   |  22 -
 src/qemu/qemu_migration.c| 138 +++
 src/qemu/qemu_migration_params.c |   9 ++
 src/qemu/qemu_migration_params.h |   3 +
 src/qemu/qemu_monitor.c  |  15 
 src/qemu/qemu_monitor.h  |   4 +
 9 files changed, 204 insertions(+), 39 deletions(-)

-- 
2.28.0

Re: [libvirt PATCH] wireshark: Don't include config.h

[Andrea Bolognani]

On Thu, 2020-09-03 at 12:16 -0400, Laine Stump wrote:
> On 9/3/20 6:43 AM, Andrea Bolognani wrote:
> > While both Debian and Fedora include the header in their development
> > packages for Wireshark, that's not something that the upstream
> > developers intended and arguably quite wrong, as config.h is obviously
> > intended to only be used to drive the compilation of Wireshark itself.
> > The Arch Linux package behaves like the upstream Wireshark package, and
> > thus libvirt fails to build there.
> > 
> > It seems that there are multiple bugs to be addressed:
> > 
> >* libvirt shouldn't include config.h;
> > 
> >* Debian and Fedora shouldn't be shipping config.h in their Wireshark
> >  packages;
> > 
> >* Wireshark should not use config.h defines such as HAVE_PLUGINS in
> >  its public headers, and define a public variant of them instead.
> > 
> > This patch takes care of the first one.
> > 
> > https://gitlab.com/libvirt/libvirt/-/issues/74
> > Signed-off-by: Andrea Bolognani 
> 
> Funny, just this morning I was grepping for " HAVE_" and " WITH_" in 
> /usr/include, saw a couple of "config.h"s shoot past, and thought "Hmm. 
> That doesn't seem right! Surely those packages didn't *really* intend to 
> ship their config.h with their -dev package".
> 
> 
> Reviewed-by: Laine Stump 

I should have pushed the branch to GitLab first:

  https://gitlab.com/abologna/libvirt/-/pipelines/185421025

We might be able to adopt a more nuanced approach, where we use
ws_version.h where available and fall back to config.h where it's the
only option, but I'm not quite sure how to implement that in Meson
and can't spare the time to learn right now.

Are either you or Michal interested in giving it a try?

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH 0/3] Some random fixes

[Michal Privoznik]

I've tried to run libvirt under valgrind and found two issues. Here are
the fixes.

Michal Prívozník (3):
  qemu_interface: Fix @cfg refcounting in qemuInterfacePrepareSlirp()
  lib: Prefer g_autoptr() declaration of virQEMUDriverConfigPtr
  qemu_namespace: Don't leak mknod items that are being skipped over

 src/qemu/qemu_conf.h |  6 +
 src/qemu/qemu_hostdev.c  |  4 +--
 src/qemu/qemu_interface.c| 11 +++--
 src/qemu/qemu_migration.c|  8 ++
 src/qemu/qemu_migration_cookie.c |  4 +--
 src/qemu/qemu_namespace.c| 42 +++-
 tests/domaincapstest.c   |  4 +--
 tests/qemuxml2xmltest.c  |  2 +-
 8 files changed, 42 insertions(+), 39 deletions(-)

-- 
2.26.2

[PATCH 3/3] qemu_namespace: Don't leak mknod items that are being skipped over

[Michal Privoznik]

When building and populating domain NS a couple of functions is
called that append paths to a string list. This string list is
then inspected, one item at the time by
qemuNamespacePrepareOneItem() which gathers all the info for
given path (stat buffer, possible link target, ACLs, SELinux
label) using qemuNamespaceMknodItemInit(). If the path needs to
be created in the domain's private /dev then it's added onto this
qemuNamespaceMknodData list which is freed later in the process.
But, if the path does not need to be created in the domain's
private /dev, then the memory allocated by
qemuNamespaceMknodItemInit() is not freed anywhere leading to a
leak.

Signed-off-by: Michal Privoznik 
---

Honestly, I think this patch looks ugly. Ideas are welcome.

 src/qemu/qemu_namespace.c | 42 +--
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c
index 87f4fd8d58..917e140f6a 100644
--- a/src/qemu/qemu_namespace.c
+++ b/src/qemu/qemu_namespace.c
@@ -1166,22 +1166,29 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr 
data,
 size_t ndevMountsPath)
 {
 long ttl = sysconf(_SC_SYMLOOP_MAX);
-const char *next = file;
+g_autofree char *next = g_strdup(file);
 size_t i;
 
 while (1) {
 qemuNamespaceMknodItem item = { 0 };
+bool added = false;
+bool isLink;
 int rc;
 
 rc = qemuNamespaceMknodItemInit(, cfg, vm, next);
-if (rc == -2) {
-/* @file doesn't exist. We can break here. */
-break;
-} else if (rc < 0) {
+if (rc < 0)  {
+qemuNamespaceMknodItemClear();
+
+if (rc == -2) {
+/* @file doesn't exist. We can break here. */
+break;
+}
 /* Some other (critical) error. */
 return -1;
 }
 
+isLink = S_ISLNK(item.sb.st_mode);
+
 if (STRPREFIX(next, QEMU_DEVPREFIX)) {
 for (i = 0; i < ndevMountsPath; i++) {
 if (STREQ(devMountsPath[i], "/dev"))
@@ -1190,22 +1197,35 @@ qemuNamespacePrepareOneItem(qemuNamespaceMknodDataPtr 
data,
 break;
 }
 
-if (i == ndevMountsPath &&
-VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 0)
-return -1;
+if (i == ndevMountsPath) {
+if (VIR_APPEND_ELEMENT_COPY(data->items, data->nitems, item) < 
0) {
+qemuNamespaceMknodItemClear();
+return -1;
+}
+added = true;
+}
 }
 
-if (!S_ISLNK(item.sb.st_mode))
+if (!isLink) {
+if (!added)
+qemuNamespaceMknodItemClear();
 break;
+}
 
 if (ttl-- == 0) {
+if (!added)
+qemuNamespaceMknodItemClear();
 virReportSystemError(ELOOP,
  _("Too many levels of symbolic links: %s"),
- next);
+ file);
 return -1;
 }
 
-next = item.target;
+g_free(next);
+next = g_strdup(item.target);
+
+if (!added)
+qemuNamespaceMknodItemClear();
 }
 
 return 0;
-- 
2.26.2

[PATCH 2/3] lib: Prefer g_autoptr() declaration of virQEMUDriverConfigPtr

[Michal Privoznik]

In the past we had to declare @cfg and then explicitly unref it.
But now, with glib we can use g_autoptr() which will do the unref
automatically and thus is more bulletproof.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_conf.h | 6 +-
 src/qemu/qemu_hostdev.c  | 4 +---
 src/qemu/qemu_interface.c| 9 +++--
 src/qemu/qemu_migration.c| 8 ++--
 src/qemu/qemu_migration_cookie.c | 4 +---
 tests/domaincapstest.c   | 4 +---
 tests/qemuxml2xmltest.c  | 2 +-
 7 files changed, 10 insertions(+), 27 deletions(-)

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 687829123c..1d7afa8738 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -64,13 +64,9 @@ typedef virQEMUDriverConfig *virQEMUDriverConfigPtr;
  * being released while they use it.
  *
  * eg
- *  qemuDriverLock(driver);
- *  virQEMUDriverConfigPtr cfg = virObjectRef(driver->config);
- *  qemuDriverUnlock(driver);
+ *  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
  *
  *  ...do stuff with 'cfg'..
- *
- *  virObjectUnref(cfg);
  */
 struct _virQEMUDriverConfig {
 virObject parent;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index d39f9d7584..721fe5da82 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -405,14 +405,12 @@ qemuHostdevReAttachPCIDevices(virQEMUDriverPtr driver,
   virDomainHostdevDefPtr *hostdevs,
   int nhostdevs)
 {
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 const char *oldStateDir = cfg->stateDir;
 virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
 
 virHostdevReAttachPCIDevices(hostdev_mgr, QEMU_DRIVER_NAME, name,
  hostdevs, nhostdevs, oldStateDir);
-
-virObjectUnref(cfg);
 }
 
 void
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index ddd4ee2731..cbf3d99981 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -261,7 +261,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
 {
 int ret = -1;
 char *res_ifname = NULL;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
 
 if (qemuInterfaceIsVnetCompatModel(net))
@@ -290,7 +290,6 @@ qemuInterfaceDirectConnect(virDomainDefPtr def,
 while (tapfdSize--)
 VIR_FORCE_CLOSE(tapfd[tapfdSize]);
 }
-virObjectUnref(cfg);
 return ret;
 }
 
@@ -413,7 +412,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
 int ret = -1;
 unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
 bool template_ifname = false;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 const char *tunpath = "/dev/net/tun";
 const char *auditdev = tunpath;
 
@@ -514,7 +513,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
 if (template_ifname)
 VIR_FREE(net->ifname);
 }
-virObjectUnref(cfg);
 
 return ret;
 }
@@ -542,7 +540,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
 int ret = -1;
 unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
 bool template_ifname = false;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 const char *tunpath = "/dev/net/tun";
 
 if (net->backend.tap) {
@@ -633,7 +631,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
 if (template_ifname)
 VIR_FREE(net->ifname);
 }
-virObjectUnref(cfg);
 
 return ret;
 }
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 142faa2cf9..1e80a22db6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3101,11 +3101,9 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver,
 int cancelled)
 {
 qemuMigrationJobPhase phase;
-virQEMUDriverConfigPtr cfg = NULL;
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 int ret = -1;
 
-cfg = virQEMUDriverGetConfig(driver);
-
 if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_OUT))
 goto cleanup;
 
@@ -3133,7 +3131,6 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver,
 
  cleanup:
 virDomainObjEndAPI();
-virObjectUnref(cfg);
 return ret;
 }
 
@@ -3928,7 +3925,7 @@ qemuMigrationSrcPerformTunnel(virQEMUDriverPtr driver,
 {
 int ret = -1;
 qemuMigrationSpec spec;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 int fds[2] = { -1, -1 };
 
 VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, 

[PATCH 1/3] qemu_interface: Fix @cfg refcounting in qemuInterfacePrepareSlirp()

[Michal Privoznik]

In the qemuInterfacePrepareSlirp() function, the qemu driver
config is obtained (via virQEMUDriverGetConfig()), but it is
never unrefed leading to mangled refcounter.

Fixes: 9145b3f1cc334e946b3f9ea45d6c24c868301e6f
Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 10a87a2528..ddd4ee2731 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -643,7 +643,7 @@ qemuSlirpPtr
 qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
   virDomainNetDefPtr net)
 {
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 g_autoptr(qemuSlirp) slirp = NULL;
 size_t i;
 
-- 
2.26.2

Re: [libvirt PATCH] ci: Disable verbose mode for cirrus-run

[Andrea Bolognani]

On Thu, 2020-09-03 at 18:16 +0100, Daniel P. Berrangé wrote:
> On Thu, Sep 03, 2020 at 07:10:58PM +0200, Andrea Bolognani wrote:
> > -- cirrus-run -v --show-build-log always ci/cirrus/$NAME.yml
> > +- cirrus-run --show-build-log always ci/cirrus/$NAME.yml
> 
> I think it might be premature - last week we hit a new problem with
> Cirrus CI we'd not seeen before and debug logs were useful
> 
> https://github.com/sio/cirrus-run/issues/7

Alright, let's wait a bit longer then :)

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH] conf: Add support for GlusterFS volumes as disk

[Vincent Vanlaer]

While libvirt has support for GlusterFS pools and volumes, they cannot
be added to a domain. This commit adds the necessary translation between
the pool definition and the disk definition, allowing volumes in a domain
to be backed by a gluster pool.

Signed-off-by: Vincent Vanlaer 
---
 src/conf/domain_conf.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 72ac4f4191..c872d02200 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -32620,6 +32620,32 @@ virDomainDiskTranslateISCSIDirect(virStorageSourcePtr 
src,
 }
 
 
+static int
+virDomainDiskTranslateGluster(virStorageSourcePtr src,
+  virStoragePoolDefPtr pooldef)
+{
+size_t i;
+
+src->srcpool->actualtype = VIR_STORAGE_TYPE_NETWORK;
+src->protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER;
+
+src->nhosts = pooldef->source.nhost;
+src->hosts = g_new0(virStorageNetHostDef, src->nhosts);
+
+for (i = 0; i < src->nhosts; i++) {
+src->hosts[i].socket = NULL;
+src->hosts[i].transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+src->hosts[i].name = g_strdup(pooldef->source.hosts[i].name);
+src->hosts[i].port = pooldef->source.hosts[i].port;
+}
+
+src->volume = g_strdup(pooldef->source.name);
+src->path = g_strdup_printf("%s/%s", pooldef->source.dir, 
src->srcpool->volume);
+
+return 0;
+}
+
+
 static int
 virDomainStorageSourceTranslateSourcePool(virStorageSourcePtr src,
   virConnectPtr conn)
@@ -32736,10 +32762,14 @@ 
virDomainStorageSourceTranslateSourcePool(virStorageSourcePtr src,
}
break;
 
+case VIR_STORAGE_POOL_GLUSTER:
+   if (virDomainDiskTranslateGluster(src, pooldef) < 0)
+   return -1;
+   break;
+
 case VIR_STORAGE_POOL_MPATH:
 case VIR_STORAGE_POOL_RBD:
 case VIR_STORAGE_POOL_SHEEPDOG:
-case VIR_STORAGE_POOL_GLUSTER:
 case VIR_STORAGE_POOL_LAST:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("using '%s' pools for backing 'volume' disks "
-- 
2.28.0