Re: [libvirt] [PATCH v6 18/23] conf: support recording ports against virNetworkObjPtr
On 5/23/19 11:32 AM, Daniel P. Berrangé wrote:
The virNetworkObjPtr state will need to maintain a record of all
virNetworkPortDefPtr objects associated with the network. Record these
in a hash and add APIs for manipulating them.
Signed-off-by: Daniel P. Berrangé
---
src/conf/virnetworkobj.c | 303 +++
src/conf/virnetworkobj.h | 34 +
src/libvirt_private.syms | 6 +
3 files changed, 343 insertions(+)
diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
index c9336e0472..47c142998e 100644
--- a/src/conf/virnetworkobj.c
+++ b/src/conf/virnetworkobj.c
@@ -58,6 +58,8 @@ struct _virNetworkObj {
/* Immutable pointer, self locking APIs */
virMacMapPtr macmap;
+
+virHashTablePtr ports; /* uuid -> virNetworkPortDefPtr */
};
struct _virNetworkObjList {
@@ -86,6 +88,17 @@ virNetworkObjOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virNetworkObj);
+static int
+virNetworkObjLoadAllPorts(virNetworkObjPtr net,
+ const char *stateDir);
+
+
+static void
+virNetworkObjPortFree(void *val, const void *key ATTRIBUTE_UNUSED)
+{
+virNetworkPortDefFree(val);
+}
+
virNetworkObjPtr
virNetworkObjNew(void)
{
@@ -106,6 +119,10 @@ virNetworkObjNew(void)
virBitmapSetBitExpand(obj->classIdMap, 2) < 0)
goto error;
+if (!(obj->ports = virHashCreate(10,
+ virNetworkObjPortFree)))
+goto error;
+
virObjectLock(obj);
return obj;
@@ -458,6 +475,7 @@ virNetworkObjDispose(void *opaque)
{
virNetworkObjPtr obj = opaque;
+virHashFree(obj->ports);
virNetworkDefFree(obj->def);
virNetworkDefFree(obj->newDef);
virBitmapFree(obj->classIdMap);
@@ -1072,9 +1090,16 @@ virNetworkObjLoadAllState(virNetworkObjListPtr nets,
continue;
obj = virNetworkLoadState(nets, stateDir, entry->d_name);
+
+if (obj &&
+virNetworkObjLoadAllPorts(obj, stateDir) < 0) {
+virNetworkObjEndAPI(&obj);
+goto cleanup;
+}
Why do you do this here instead of adding it to virNetworkLoadState()?
ACK as-is if there's a reason for it. Otherwise, ACK with that chunk
moved into virNetworkLoadState()
Reviewed-by: Laine Stump
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 17/23] virsh: add support for network port APIs
On 5/23/19 11:32 AM, Daniel P. Berrangé wrote: Signed-off-by: Daniel P. Berrangé These all worked when I tried them (including the uuid completer. Yay!) Reviewed-by: Laine Stump -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 03/23] network: make networkLogAllocation independent of domain conf
On 5/23/19 11:32 AM, Daniel P. Berrangé wrote:
Stop passing a virDomainNetDefPtr parameter to networkLogAllocation,
instead just pass in the MAC address. The actual device type is also not
required, since virNetworkForwardIfDefPtr has a type field that can be
used instead.
Reviewed-by: Laine Stump
Signed-off-by: Daniel P. Berrangé
---
src/network/bridge_driver.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c0c026e242..7374da0ac7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4326,32 +4326,29 @@ networkGetDHCPLeases(virNetworkPtr net,
static void
networkLogAllocation(virNetworkDefPtr netdef,
- virDomainNetType actualType,
virNetworkForwardIfDefPtr dev,
- virDomainNetDefPtr iface,
+ virMacAddrPtr mac,
bool inUse)
{
char macStr[VIR_MAC_STRING_BUFLEN];
const char *verb = inUse ? "using" : "releasing";
+virMacAddrFormat(mac, macStr);
if (!dev) {
VIR_INFO("MAC %s %s network %s (%d connections)",
- virMacAddrFormat(&iface->mac, macStr), verb,
- netdef->name, netdef->connections);
+ macStr, verb, netdef->name, netdef->connections);
} else {
-if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
+if (dev->type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI) {
VIR_INFO("MAC %s %s network %s (%d connections) "
"physical device %04x:%02x:%02x.%x (%d connections)",
- virMacAddrFormat(&iface->mac, macStr), verb,
- netdef->name, netdef->connections,
+ macStr, verb, netdef->name, netdef->connections,
dev->device.pci.domain, dev->device.pci.bus,
dev->device.pci.slot, dev->device.pci.function,
dev->connections);
} else {
VIR_INFO("MAC %s %s network %s (%d connections) "
"physical device %s (%d connections)",
- virMacAddrFormat(&iface->mac, macStr), verb,
- netdef->name, netdef->connections,
+ macStr, verb, netdef->name, netdef->connections,
dev->device.dev, dev->connections);
}
}
@@ -4764,7 +4761,7 @@ networkAllocateActualDevice(virNetworkPtr net,
dev->connections--;
goto error;
}
-networkLogAllocation(netdef, actualType, dev, iface, true);
+networkLogAllocation(netdef, dev, &iface->mac, true);
ret = 0;
@@ -4955,7 +4952,7 @@ networkNotifyActualDevice(virNetworkPtr net,
netdef->connections--;
goto error;
}
-networkLogAllocation(netdef, actualType, dev, iface, true);
+networkLogAllocation(netdef, dev, &iface->mac, true);
ret = 0;
cleanup:
@@ -5122,7 +5119,7 @@ networkReleaseActualDevice(virNetworkPtr net,
/* finally we can call the 'unplugged' hook script if any */
networkRunHook(obj, dom, iface, VIR_HOOK_NETWORK_OP_IFACE_UNPLUGGED,
VIR_HOOK_SUBOP_BEGIN);
-networkLogAllocation(netdef, actualType, dev, iface, false);
+networkLogAllocation(netdef, dev, &iface->mac, false);
}
ret = 0;
cleanup:
Reviewed-by: Laine Stump
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 04/23] conf: add APIs to convert virDomainNetDef to virNetworkPortDef
On 5/23/19 11:32 AM, Daniel P. Berrangé wrote: Helper APIs are needed to - Populate basic virNetworkPortDef from virDomainNetDef - Set a virDomainActualNetDef from virNetworkPortDef - Populate a full virNetworkPortDef from virDomainActualNetDef Signed-off-by: Daniel P. Berrangé Reviewed-by: Laine Stump (just so you don't have to go back and search through the v5 patches for the ACK I gave minutes before you sent v6 :-) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Add support for overriding max threads per process limit
Some VM configurations may result in a large number of threads created by
the associated qemu process which can exceed the system default limit. The
maximum number of threads allowed per process is controlled by the pids
cgroup controller and is set to 16k when creating VMs with systemd's
machined service. The maximum number of threads per process is recorded
in the pids.max file under the machine's pids controller cgroup hierarchy,
e.g.
$cgrp-mnt/pids/machine.slice/machine-qemu\\x2d1\\x2dtest.scope/pids.max
Maximum threads per process is controlled with the TasksMax property of
the systemd scope for the machine. This patch adds an option to qemu.conf
which can be used to override the maximum number of threads allowed per
qemu process. If the value of option is greater than zero, it will be set
in the TasksMax property of the machine's scope after creating the machine.
Signed-off-by: Jim Fehlig
---
src/lxc/lxc_cgroup.c | 1 +
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 10 ++
src/qemu/qemu_cgroup.c | 1 +
src/qemu/qemu_conf.c | 2 ++
src/qemu/qemu_conf.h | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
src/util/vircgroup.c | 6 +-
src/util/vircgroup.h | 1 +
src/util/virsystemd.c | 24 +++-
src/util/virsystemd.h | 3 ++-
tests/virsystemdtest.c | 12 ++--
12 files changed, 54 insertions(+), 9 deletions(-)
diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
index d93a19d684..76014f3bfd 100644
--- a/src/lxc/lxc_cgroup.c
+++ b/src/lxc/lxc_cgroup.c
@@ -455,6 +455,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def,
nnicindexes, nicindexes,
def->resource->partition,
-1,
+0,
&cgroup) < 0)
goto cleanup;
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index b311f02da6..c70b903fed 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -94,6 +94,7 @@ module Libvirtd_qemu =
| limits_entry "max_core"
| bool_entry "dump_guest_core"
| str_entry "stdio_handler"
+ | int_entry "max_threads_per_process"
let device_entry = bool_entry "mac_filter"
| bool_entry "relaxed_acs_check"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 5a85789d81..ab044c9cf3 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -608,6 +608,16 @@
#max_processes = 0
#max_files = 0
+# If max_threads_per_process is set to a positive integer, libvirt
+# will use it to set the maximum number of threads that can be
+# created by a qemu process. Some VM configurations can result in
+# qemu processes with tens of thousands of threads. systemd-based
+# systems typically limit the number of threads per process to
+# 16k. max_threads_per_process can be used to override default
+# limits in the host OS.
+#
+#max_threads_per_process = 0
+
# If max_core is set to a non-zero integer, then QEMU will be
# permitted to create core dumps when it crashes, provided its
# RAM size is smaller than the limit set.
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index ca76c4fdfa..9603f33e8a 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -930,6 +930,7 @@ qemuInitCgroup(virDomainObjPtr vm,
nnicindexes, nicindexes,
vm->def->resource->partition,
cfg->cgroupControllers,
+cfg->maxThreadsPerProc,
&priv->cgroup) < 0) {
if (virCgroupNewIgnoreError())
goto done;
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index daea11dacb..8ac2dc92b5 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -687,6 +687,8 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr
cfg,
return -1;
if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0)
return -1;
+if (virConfGetValueUInt(conf, "max_threads_per_process",
&cfg->maxThreadsPerProc) < 0)
+return -1;
if (virConfGetValueType(conf, "max_core") == VIR_CONF_STRING) {
if (virConfGetValueString(conf, "max_core", &corestr) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 983e74a3cf..48b8711cbd 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -171,6 +171,7 @@ struct _virQEMUDriverConfig {
unsigned int maxProcesses;
unsigned int maxFiles;
+unsigned int maxThreadsPerProc;
unsigned long long maxCore;
bool dumpGuestCore;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index fea1d308b7..ac7ad59ba8 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/
Re: [libvirt] [PATCH RFC 0/1] mdevctl: further config for vfio-ap
On 6/6/19 10:44 AM, Cornelia Huck wrote: > This patch adds a very rough implementation of additional config data > for mdev devices. The idea is to make it possible to specify some > type-specific key=value pairs in the config file for an mdev device. > If a device is started automatically, the device is stopped and restarted > after applying the config. > > The code has still some problems, like not doing a lot of error handling > and being ugly in general; but most importantly, I can't really test it, > as I don't have the needed hardware. Feedback welcome; would be good to > know if the direction is sensible in general. Hi Connie, This is very similar to what I was looking to do in zdev (config via key=value pairs), so I like your general approach. I pulled your code and took it for a spin on an LPAR with access to crypto cards: # mdevctl create-mdev `uuidgen` matrix vfio_ap-passthrough # mdevctl set-additional-config ap_adapters=0x4,0x5 # mdevctl set-additional-config ap_domains=0x36 # mdevctl set-additional-config ap_control_domains=0x37 Assuming all valid inputs, this successfully creates the appropriate mdev and what looks to be a valid mdevctl.d entry. A subsequent reboot successfully brings the same vfio_ap-passthrough device up again. Matt > > Also available at > > https://github.com/cohuck/mdevctl conf-data > > Cornelia Huck (1): > allow to specify additional config data > > mdevctl.libexec | 25 ++ > mdevctl.sbin| 56 - > 2 files changed, 80 insertions(+), 1 deletion(-) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/1] allow to specify additional config data
On Thu, 6 Jun 2019 09:32:24 -0600
Alex Williamson wrote:
> On Thu, 6 Jun 2019 16:44:17 +0200
> Cornelia Huck wrote:
>
> > Add a rough implementation for vfio-ap.
> >
> > Signed-off-by: Cornelia Huck
> > ---
> > mdevctl.libexec | 25 ++
> > mdevctl.sbin| 56 -
> > 2 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/mdevctl.libexec b/mdevctl.libexec
> > index 804166b5086d..cc0546142924 100755
> > --- a/mdevctl.libexec
> > +++ b/mdevctl.libexec
> > @@ -54,6 +54,19 @@ wait_for_supported_types () {
> > fi
> > }
> >
> > +# configure vfio-ap devices
> > +configure_ap_devices() {
> > +list="`echo "${config[$1]}" | sed 's/,/ /'`"
> > +[ -z "$list" ] && return
> > +for a in $list; do
> > +echo "$a" >
> > "$supported_types/${config[mdev_type]}/devices/$uuid/$2"
> > +if [ $? -ne 0 ]; then
> > +echo "Error writing '$a' to '$uuid/$2'" >&2
> > +exit 1
> > +fi
> > +done
> > +}
> > +
> > case ${1} in
> > start-mdev|stop-mdev)
> > if [ $# -ne 2 ]; then
> > @@ -148,6 +161,18 @@ case ${cmd} in
> > echo "Error creating mdev type ${config[mdev_type]} on
> > $parent" >&2
> > exit 1
> > fi
> > +
> > +# some types may specify additional config data
> > +case ${config[mdev_type]} in
> > +vfio_ap-passthrough)
>
> I think this could have some application beyond ap too, I know NVIDIA
> GRID vGPUs do have some controls under the vendor hierarchy of the
> device, ex. setting the frame rate limiter. The implementation here is
> a bit rigid, we know a specific protocol for a specific mdev type, but
> for supporting arbitrary vendor options we'd really just want to try to
> apply whatever options are provided. If we didn't care about ordering,
> we could just look for keys for every file in the device's immediate
> sysfs hierarchy and apply any value we find, independent of the
> mdev_type, ex. intel_vgpu/foo=bar Thanks,
For example:
for key in find -P $mdev_base/$uuid/ \( -path
"$mdev_base/$uuid/power/*" -o -path $mdev_base/$uuid/uevent -o -path
$mdev_base/$uuid/remove \) -prune -o -type f -print | sed -e
"s|$mdev_base/$uuid/||g"); do
[ -z ${config[$key]} ] && continue
... parse value(s) and iteratively apply to key
done
The find is a little ugly to exclude stuff, maybe we just let people do
screwy stuff like specify remove=1 in their config. Also need to think
about whether we're imposing a delimiter to apply multiple values to a
key that conflicts with the attribute usage. Thanks,
Alex
> > +configure_ap_devices ap_adapters assign_adapter
> > +configure_ap_devices ap_domains assign_domain
> > +configure_ap_devices ap_control_domains
> > assign_control_domain
> > +# TODO: is assigning idempotent? Should we unwind on error?
> > +;;
> > +*)
> > +;;
> > +esac
> > ;;
> >
> > add-mdev)
> > diff --git a/mdevctl.sbin b/mdevctl.sbin
> > index 276cf6ddc817..eb5ee0091879 100755
> > --- a/mdevctl.sbin
> > +++ b/mdevctl.sbin
> > @@ -33,6 +33,8 @@ usage() {
> > echo "set-start : change mdev start policy, if no option
> > specified," >&2
> > echo " system default policy is used" >&2
> > echo " options: [--auto] [--manual]" >&2
> > +echo "set-additional-config {fmt...}: supply additional
> > configuration" >&2
> > +echo "show-additional-config-format : prints the format
> > expected by the device" >&2
> > echo "list-all: list all possible mdev types supported in the system"
> > >&2
> > echo "list-available: list all mdev types currently available" >&2
> > echo "list-mdevs: list currently configured mdevs" >&2
> > @@ -48,7 +50,7 @@ while (($# > 0)); do
> > --manual)
> > config[start]=manual
> > ;;
> > -start-mdev|stop-mdev|remove-mdev|set-start)
> > +
> > start-mdev|stop-mdev|remove-mdev|set-start|show-additional-config-format)
> > [ $# -ne 2 ] && usage
> > cmd=$1
> > uuid=$2
> > @@ -67,6 +69,14 @@ while (($# > 0)); do
> > cmd=$1
> > break
> > ;;
> > +set-additional-config)
> > +[ $# -le 2 ] && usage
> > +cmd=$1
> > +uuid=$2
> > +shift 2
> > +addtl_config="$*"
> > +break
> > +;;
> > *)
> > usage
> > ;;
> > @@ -114,6 +124,50 @@ case ${cmd} in
> > fi
> > ;;
> >
> > +set-additional-config)
> > +file=$(find $persist_base -name $uuid -type f)
> > +if [ -w "$file" ]; then
> > +read_config "$file"
> > +if [ ${config[start]} == "auto" ]; then
> > +s
Re: [libvirt] [PATCH 2/4] test_driver: extract image loading code into a separate function
On Wed, May 29, 2019 at 02:22:57PM +0200, Ilias Stamatis wrote: > Extracting the code logic for opening and parsing a test image from > testDomainRestoreFlags into a separate function, allows us to reuse this > code in other functions such as testDomainSaveImageGetXMLDesc. > > Signed-off-by: Ilias Stamatis > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC 1/1] allow to specify additional config data
On Thu, 6 Jun 2019 16:44:17 +0200
Cornelia Huck wrote:
> Add a rough implementation for vfio-ap.
>
> Signed-off-by: Cornelia Huck
> ---
> mdevctl.libexec | 25 ++
> mdevctl.sbin| 56 -
> 2 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/mdevctl.libexec b/mdevctl.libexec
> index 804166b5086d..cc0546142924 100755
> --- a/mdevctl.libexec
> +++ b/mdevctl.libexec
> @@ -54,6 +54,19 @@ wait_for_supported_types () {
> fi
> }
>
> +# configure vfio-ap devices
> +configure_ap_devices() {
> +list="`echo "${config[$1]}" | sed 's/,/ /'`"
> +[ -z "$list" ] && return
> +for a in $list; do
> +echo "$a" > "$supported_types/${config[mdev_type]}/devices/$uuid/$2"
> +if [ $? -ne 0 ]; then
> +echo "Error writing '$a' to '$uuid/$2'" >&2
> +exit 1
> +fi
> +done
> +}
> +
> case ${1} in
> start-mdev|stop-mdev)
> if [ $# -ne 2 ]; then
> @@ -148,6 +161,18 @@ case ${cmd} in
> echo "Error creating mdev type ${config[mdev_type]} on $parent"
> >&2
> exit 1
> fi
> +
> +# some types may specify additional config data
> +case ${config[mdev_type]} in
> +vfio_ap-passthrough)
I think this could have some application beyond ap too, I know NVIDIA
GRID vGPUs do have some controls under the vendor hierarchy of the
device, ex. setting the frame rate limiter. The implementation here is
a bit rigid, we know a specific protocol for a specific mdev type, but
for supporting arbitrary vendor options we'd really just want to try to
apply whatever options are provided. If we didn't care about ordering,
we could just look for keys for every file in the device's immediate
sysfs hierarchy and apply any value we find, independent of the
mdev_type, ex. intel_vgpu/foo=bar Thanks,
Alex
> +configure_ap_devices ap_adapters assign_adapter
> +configure_ap_devices ap_domains assign_domain
> +configure_ap_devices ap_control_domains assign_control_domain
> +# TODO: is assigning idempotent? Should we unwind on error?
> +;;
> +*)
> +;;
> +esac
> ;;
>
> add-mdev)
> diff --git a/mdevctl.sbin b/mdevctl.sbin
> index 276cf6ddc817..eb5ee0091879 100755
> --- a/mdevctl.sbin
> +++ b/mdevctl.sbin
> @@ -33,6 +33,8 @@ usage() {
> echo "set-start : change mdev start policy, if no option
> specified," >&2
> echo " system default policy is used" >&2
> echo " options: [--auto] [--manual]" >&2
> +echo "set-additional-config {fmt...}: supply additional
> configuration" >&2
> +echo "show-additional-config-format : prints the format
> expected by the device" >&2
> echo "list-all: list all possible mdev types supported in the system" >&2
> echo "list-available: list all mdev types currently available" >&2
> echo "list-mdevs: list currently configured mdevs" >&2
> @@ -48,7 +50,7 @@ while (($# > 0)); do
> --manual)
> config[start]=manual
> ;;
> -start-mdev|stop-mdev|remove-mdev|set-start)
> +
> start-mdev|stop-mdev|remove-mdev|set-start|show-additional-config-format)
> [ $# -ne 2 ] && usage
> cmd=$1
> uuid=$2
> @@ -67,6 +69,14 @@ while (($# > 0)); do
> cmd=$1
> break
> ;;
> +set-additional-config)
> +[ $# -le 2 ] && usage
> +cmd=$1
> +uuid=$2
> +shift 2
> +addtl_config="$*"
> +break
> +;;
> *)
> usage
> ;;
> @@ -114,6 +124,50 @@ case ${cmd} in
> fi
> ;;
>
> +set-additional-config)
> +file=$(find $persist_base -name $uuid -type f)
> +if [ -w "$file" ]; then
> +read_config "$file"
> +if [ ${config[start]} == "auto" ]; then
> +systemctl stop mdev@$uuid.service
> +fi
> +# FIXME: validate input!
> +for i in $addtl_config; do
> +key="`echo "$i" | cut -d '=' -f 1`"
> +value="`echo "$i" | cut -d '=' -f 2-`"
> +if grep -q ^$key $file; then
> +if [ -z "$value" ]; then
> +sed -i "s/^$key=.*//g" $file
> +else
> +sed -i "s/^$key=.*/$key=$value/g" $file
> +fi
> +else
> +echo "$i" >> "$file"
> +fi
> +done
> +if [ ${config[start]} == "auto" ]; then
> +systemctl start mdev@$uuid.service
> +fi
> +else
> +exit 1
> +fi
> +;;
> +
> +show-additional-config-format)
> +file=$(find $per
[libvirt] [PATCH 2/2] gitdm: Add gitdm configuration
This configuration can be used by gitdm to generate reports about libvirt development. The goal I was working with was being able to generate a report for every single libvirt release and having zero "email address as company" entries; picking different commit ranges might result in some contributions not being accounted for. I had to make some judgement calls when the situation was not entirely clear-cut: when in doubt, and not finding any obvious signs of the opposite being true, I mostly ended up dumping people in the "unaffiliated contributions" bin. If I got it wrong, and companies want to get recognition for their sponsored contributions to libvirt, they can send patches. Signed-off-by: Andrea Bolognani --- docs/gitdm/aliases | 25 + docs/gitdm/companies/canonical | 11 docs/gitdm/companies/datto | 2 + docs/gitdm/companies/dreamhost | 4 ++ docs/gitdm/companies/nec | 2 + docs/gitdm/companies/others| 98 ++ docs/gitdm/companies/redhat| 6 +++ docs/gitdm/companies/suse | 7 +++ docs/gitdm/companies/virtuozzo | 2 + docs/gitdm/groups/education| 17 ++ docs/gitdm/groups/opensource | 12 + docs/gitdm/groups/unaffiliated | 83 gitdm.config | 37 + 13 files changed, 306 insertions(+) create mode 100644 docs/gitdm/aliases create mode 100644 docs/gitdm/companies/canonical create mode 100644 docs/gitdm/companies/datto create mode 100644 docs/gitdm/companies/dreamhost create mode 100644 docs/gitdm/companies/nec create mode 100644 docs/gitdm/companies/others create mode 100644 docs/gitdm/companies/redhat create mode 100644 docs/gitdm/companies/suse create mode 100644 docs/gitdm/companies/virtuozzo create mode 100644 docs/gitdm/groups/education create mode 100644 docs/gitdm/groups/opensource create mode 100644 docs/gitdm/groups/unaffiliated create mode 100644 gitdm.config diff --git a/docs/gitdm/aliases b/docs/gitdm/aliases new file mode 100644 index 00..c3c837f89f --- /dev/null +++ b/docs/gitdm/aliases @@ -0,0 +1,25 @@ +# Silly mistakes, mostly found in S-o-b or R-b tags. + +"jdenemar redhat com" jdene...@redhat.com +"pkrempa@redhat st.com" pkre...@redhat.com +jyang@redhat jy...@redhat.com +wangjie88.huawei.com wangji...@huawei.com + +# This is information that's already present in .mailmap, and having to +# duplicate it is annoying. Unfortunately gitdm doesn't parse .mailmap +# and the format is different, so we can't just point it to the file +# either. + +cedric.bosdon...@free.fr cbosdon...@suse.com +d...@berrange.com berra...@redhat.com +fabi...@fidencio.org fiden...@redhat.com +intrigeri+libv...@boum.org intrig...@boum.org +j...@meyering.net meyer...@redhat.com +la...@laine.org la...@redhat.com +red...@adrb.pl adrian.brzezin...@eo.pl +shilei.massclo...@gmx.com shi_...@massclouds.com + +# This deviates from what's found in .mailmap, but it makes more sense as +# far as gitdm is concerned since Jim was employed by Novell at the time. + +jfeh...@linux-ypgk.site jfeh...@novell.com diff --git a/docs/gitdm/companies/canonical b/docs/gitdm/companies/canonical new file mode 100644 index 00..3e7e59331f --- /dev/null +++ b/docs/gitdm/companies/canonical @@ -0,0 +1,11 @@ +canonical.com + +# Having an @ubuntu.com email address doesn't necessarily imply you're a +# Canonical employer; these people, however, seemed to be employed by +# Canonical at the time they contributed to libvirt. + +ja...@ubuntu.com +serge.hal...@ubuntu.com +smo...@ubuntu.com +so...@ubuntu.com +wgr...@ubuntu.com diff --git a/docs/gitdm/companies/datto b/docs/gitdm/companies/datto new file mode 100644 index 00..2c0ea6e286 --- /dev/null +++ b/docs/gitdm/companies/datto @@ -0,0 +1,2 @@ +datto.com +dattobackup.com diff --git a/docs/gitdm/companies/dreamhost b/docs/gitdm/companies/dreamhost new file mode 100644 index 00..fc97503f40 --- /dev/null +++ b/docs/gitdm/companies/dreamhost @@ -0,0 +1,4 @@ +dreamhost.com +dreamhost.net +newdream.com +newdream.net diff --git a/docs/gitdm/companies/nec b/docs/gitdm/companies/nec new file mode 100644 index 00..8af5e5cf9b --- /dev/null +++ b/docs/gitdm/companies/nec @@ -0,0 +1,2 @@ +nec.co.jp +nec.com diff --git a/docs/gitdm/companies/others b/docs/gitdm/companies/others new file mode 100644 index 00..516a273605 --- /dev/null +++ b/docs/gitdm/companies/others @@ -0,0 +1,98 @@ +6wind.com 6WIND +active.by ActiveCloud +aero.org Aerospace +akamai.com Akamai +amd.com AMD +anchor.net.au Anchor +aristanetworks.com Arista Networks +arpnetworks.com ARP Networks +av-test.de AV-TEST +b1-systems.de B1 Systems +brightbox.co.uk Brightbox +cisco.com Cisco +citrix.com Citrix +cloudwatt.com Cloudwatt +codethink.co.uk Codethink +cumulusnetworks.com Cumulus Networks +datagravity.com DataGravity +dell.com Dell +diateam.net DIATEAM +eldorado.org.br ELDORADO +endocode.com Endocode +eo.pl eo Networks +ericsson.com
Re: [libvirt] [PATCH] qemu: Fix NULL pointer access in qemuProcessInitCpuAffinity()
On 6/6/19 9:44 AM, Andrea Bolognani wrote: > Commit 2f2254c7f4e5 attempted to fix a memory leak by ensuring > cpumapToSet is always a freshly allocated bitmap, but regrettably > introduced a NULL pointer access while doing so, because it called > virBitmapCopy() without allocating the destination bitmap first. > > Solve the issue by using virBitmapNewCopy() instead. > > Reported-by: John Ferlan > Signed-off-by: Andrea Bolognani > --- > src/qemu/qemu_process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: John Ferlan Coverity is happy too ;-) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] gitdm: Add gitdm configuration
See the commit message for patch 2/2 for more information; patch 1/2 merely addresses some minor issues in .mailmap that I spotted while working on the gitdm configuration. Andrea Bolognani (2): mailmap: Remove some duplicates gitdm: Add gitdm configuration .mailmap | 4 ++ docs/gitdm/aliases | 25 + docs/gitdm/companies/canonical | 11 docs/gitdm/companies/datto | 2 + docs/gitdm/companies/dreamhost | 4 ++ docs/gitdm/companies/nec | 2 + docs/gitdm/companies/others| 98 ++ docs/gitdm/companies/redhat| 6 +++ docs/gitdm/companies/suse | 7 +++ docs/gitdm/companies/virtuozzo | 2 + docs/gitdm/groups/education| 17 ++ docs/gitdm/groups/opensource | 12 + docs/gitdm/groups/unaffiliated | 83 gitdm.config | 37 + 14 files changed, 310 insertions(+) create mode 100644 docs/gitdm/aliases create mode 100644 docs/gitdm/companies/canonical create mode 100644 docs/gitdm/companies/datto create mode 100644 docs/gitdm/companies/dreamhost create mode 100644 docs/gitdm/companies/nec create mode 100644 docs/gitdm/companies/others create mode 100644 docs/gitdm/companies/redhat create mode 100644 docs/gitdm/companies/suse create mode 100644 docs/gitdm/companies/virtuozzo create mode 100644 docs/gitdm/groups/education create mode 100644 docs/gitdm/groups/opensource create mode 100644 docs/gitdm/groups/unaffiliated create mode 100644 gitdm.config -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] mailmap: Remove some duplicates
Fabiano Fidencio was working for Red Hat when he contributed to libvirt, Shi Lei's non-company email address contains the company name so it's fair to assume contribution using it were made on company time, and Adrian Brzezinski's personal email was used for the S-o-b while the git authorship information clearly pointed at the company being involved. Signed-off-by: Andrea Bolognani --- .mailmap | 4 1 file changed, 4 insertions(+) diff --git a/.mailmap b/.mailmap index 311a10a8e9..2b080568bc 100644 --- a/.mailmap +++ b/.mailmap @@ -43,6 +43,10 @@ + + + + # Name consolidation: # Preferred author spelling -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC 1/1] allow to specify additional config data
Add a rough implementation for vfio-ap.
Signed-off-by: Cornelia Huck
---
mdevctl.libexec | 25 ++
mdevctl.sbin| 56 -
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/mdevctl.libexec b/mdevctl.libexec
index 804166b5086d..cc0546142924 100755
--- a/mdevctl.libexec
+++ b/mdevctl.libexec
@@ -54,6 +54,19 @@ wait_for_supported_types () {
fi
}
+# configure vfio-ap devices
+configure_ap_devices() {
+list="`echo "${config[$1]}" | sed 's/,/ /'`"
+[ -z "$list" ] && return
+for a in $list; do
+echo "$a" > "$supported_types/${config[mdev_type]}/devices/$uuid/$2"
+if [ $? -ne 0 ]; then
+echo "Error writing '$a' to '$uuid/$2'" >&2
+exit 1
+fi
+done
+}
+
case ${1} in
start-mdev|stop-mdev)
if [ $# -ne 2 ]; then
@@ -148,6 +161,18 @@ case ${cmd} in
echo "Error creating mdev type ${config[mdev_type]} on $parent" >&2
exit 1
fi
+
+# some types may specify additional config data
+case ${config[mdev_type]} in
+vfio_ap-passthrough)
+configure_ap_devices ap_adapters assign_adapter
+configure_ap_devices ap_domains assign_domain
+configure_ap_devices ap_control_domains assign_control_domain
+# TODO: is assigning idempotent? Should we unwind on error?
+;;
+*)
+;;
+esac
;;
add-mdev)
diff --git a/mdevctl.sbin b/mdevctl.sbin
index 276cf6ddc817..eb5ee0091879 100755
--- a/mdevctl.sbin
+++ b/mdevctl.sbin
@@ -33,6 +33,8 @@ usage() {
echo "set-start : change mdev start policy, if no option
specified," >&2
echo " system default policy is used" >&2
echo " options: [--auto] [--manual]" >&2
+echo "set-additional-config {fmt...}: supply additional
configuration" >&2
+echo "show-additional-config-format : prints the format
expected by the device" >&2
echo "list-all: list all possible mdev types supported in the system" >&2
echo "list-available: list all mdev types currently available" >&2
echo "list-mdevs: list currently configured mdevs" >&2
@@ -48,7 +50,7 @@ while (($# > 0)); do
--manual)
config[start]=manual
;;
-start-mdev|stop-mdev|remove-mdev|set-start)
+
start-mdev|stop-mdev|remove-mdev|set-start|show-additional-config-format)
[ $# -ne 2 ] && usage
cmd=$1
uuid=$2
@@ -67,6 +69,14 @@ while (($# > 0)); do
cmd=$1
break
;;
+set-additional-config)
+[ $# -le 2 ] && usage
+cmd=$1
+uuid=$2
+shift 2
+addtl_config="$*"
+break
+;;
*)
usage
;;
@@ -114,6 +124,50 @@ case ${cmd} in
fi
;;
+set-additional-config)
+file=$(find $persist_base -name $uuid -type f)
+if [ -w "$file" ]; then
+read_config "$file"
+if [ ${config[start]} == "auto" ]; then
+systemctl stop mdev@$uuid.service
+fi
+# FIXME: validate input!
+for i in $addtl_config; do
+key="`echo "$i" | cut -d '=' -f 1`"
+value="`echo "$i" | cut -d '=' -f 2-`"
+if grep -q ^$key $file; then
+if [ -z "$value" ]; then
+sed -i "s/^$key=.*//g" $file
+else
+sed -i "s/^$key=.*/$key=$value/g" $file
+fi
+else
+echo "$i" >> "$file"
+fi
+done
+if [ ${config[start]} == "auto" ]; then
+systemctl start mdev@$uuid.service
+fi
+else
+exit 1
+fi
+;;
+
+show-additional-config-format)
+file=$(find $persist_base -name $uuid -type f)
+read_config "$file"
+case ${config[mdev_type]} in
+vfio_ap-passthrough)
+echo "ap_adapters="
+echo "ap_domains="
+echo "ap_control_domains="
+;;
+*)
+echo "no additional configuration defined"
+;;
+esac
+;;
+
list-mdevs)
for mdev in $(find $mdev_base/ -maxdepth 1 -mindepth 1 -type l); do
uuid=$(basename $mdev)
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH RFC 0/1] mdevctl: further config for vfio-ap
This patch adds a very rough implementation of additional config data for mdev devices. The idea is to make it possible to specify some type-specific key=value pairs in the config file for an mdev device. If a device is started automatically, the device is stopped and restarted after applying the config. The code has still some problems, like not doing a lot of error handling and being ugly in general; but most importantly, I can't really test it, as I don't have the needed hardware. Feedback welcome; would be good to know if the direction is sensible in general. Also available at https://github.com/cohuck/mdevctl conf-data Cornelia Huck (1): allow to specify additional config data mdevctl.libexec | 25 ++ mdevctl.sbin| 56 - 2 files changed, 80 insertions(+), 1 deletion(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Fix NULL pointer access in qemuProcessInitCpuAffinity()
On Thu, Jun 06, 2019 at 03:44:33PM +0200, Andrea Bolognani wrote: > Commit 2f2254c7f4e5 attempted to fix a memory leak by ensuring > cpumapToSet is always a freshly allocated bitmap, but regrettably > introduced a NULL pointer access while doing so, because it called > virBitmapCopy() without allocating the destination bitmap first. > > Solve the issue by using virBitmapNewCopy() instead. > > Reported-by: John Ferlan > Signed-off-by: Andrea Bolognani > --- Reviewed-by: Erik Skultety -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML
On 05.06.2019 12:09, Michal Privoznik wrote:
> Changing domain definition must be guarded with acquiring modify
> job. The problem is if there is a thread executing say
> qemuDomainSetMemoryStatsPeriod() which is an API that acquires
> modify job and then possibly unlock the domain object and locks
> it again. However, becasue virDomainObjAssignDef() does not take
> a job (only object lock) it may have changed the domain
> definition while the other thread unlocked the domain object in
> order to talk on the monitor. For instance:
>
> Thread1:
> 1) lookup domain object and lock it
> 2) acquire job
> 3) get pointers to live and persistent defs
> 4) unlock the domain object
> 5) talk to qemu on the monitor
>
> Thread2 (Execute qemuDomainDefineXMLFlags):
> 1) lookup domain object and lock it
> 2) virDomainObjAssignDef() which overwrites persistent def and
>frees the old one
> 3) unlock domain object
>
> Thread1:
> 6) lock the domain object again
> 7) try to access the persistent def via pointer stored in 3)
>
> Now, this will crash because the pointer from step 3) points to a
> memory that was freed.
>
> However, if Thread2 waited and acquired modify job (just like
> Thread1) then these two would serialize and no harm would be
> done.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c| 55 +++
> src/qemu/qemu_domain.h| 6 +
> src/qemu/qemu_driver.c| 27 ++-
> src/qemu/qemu_migration.c | 5 +---
> 4 files changed, 70 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4d3a8868b2..f6b677c69e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7055,6 +7055,61 @@ virDomainDefParserConfig
> virQEMUDriverDomainDefParserConfig = {
> };
>
>
> +/**
> + * qemuDomainObjListAdd:
> + * @driver: qemu driver
> + * @def: domain definition
> + * @oldDef: previous domain definition
> + * @live: whether @def is live definition
> + * @flags: an bitwise-OR of virDomainObjListAdd flags
> + *
> + * Add a domain onto the list of domain object and sets its
> + * definition. If @oldDef is not NULL and there was pre-existing
> + * definition it's returned in @oldDef.
> + *
> + * In addition to that, if definition of an existing domain is
> + * changed a MODIFY job is acquired prior to that.
> + *
> + * Returns: domain object pointer on success,
> + * NULL otherwise.
> + */
> +virDomainObjPtr
> +qemuDomainObjListAdd(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + virDomainDefPtr *oldDef,
> + bool live,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +
> +if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt,
> flags)))
> +return NULL;
> +
> +/* At this point, @vm is locked. If it doesn't have any
> + * definition set, then the call above just added it and
> + * there can't be anybody else using the object. It's safe to
> + * just set the definition without acquiring job. */
> +if (!vm->def) {
> +virDomainObjAssignDef(vm, def, live, oldDef);
> +VIR_RETURN_PTR(vm);
> +}
> +
> +/* Bad luck. Domain was pre-existing and this call is trying
> + * to update its definition. Modify job MUST be acquired. */
> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
> +qemuDomainRemoveInactive(driver, vm);
Here we can remove transient domain.
Nikolay
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
On Thu, 2019-06-06 at 06:44 -0400, John Ferlan wrote:
> On 6/4/19 8:46 AM, Andrea Bolognani wrote:
> > } else if (vm->def->cputune.emulatorpin) {
> > -cpumapToSet = vm->def->cputune.emulatorpin;
> > +if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
>
> Now Coverity is unhappy for another reason. What happens to the NULL
> cpumapToSet when calling virBitmapCopy?
>
> Should have been virBitmapNewCopy
Geez, so embarrassing :/
I just posted a fix:
https://www.redhat.com/archives/libvir-list/2019-June/msg00171.html
> (sorry didn't get a chance to look at the patch when first posted)
No need to apologize, but if you really feel like you need to make
up for it reviewing the above will do ;)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Fix NULL pointer access in qemuProcessInitCpuAffinity()
Commit 2f2254c7f4e5 attempted to fix a memory leak by ensuring
cpumapToSet is always a freshly allocated bitmap, but regrettably
introduced a NULL pointer access while doing so, because it called
virBitmapCopy() without allocating the destination bitmap first.
Solve the issue by using virBitmapNewCopy() instead.
Reported-by: John Ferlan
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 42a6271411..50a76aa0ed 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2498,7 +2498,7 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
return -1;
} else if (vm->def->cputune.emulatorpin) {
-if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
+if (!(cpumapToSet = virBitmapNewCopy(vm->def->cputune.emulatorpin)))
return -1;
} else {
if (qemuProcessGetAllCpuAffinity(&cpumapToSet) < 0)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/11] qemu_domain: Allow qemuDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote:
> In some cases, caller of qemuDomainObjListAdd() tries to acquire
> MODIFY job after the call. Let's adjust qemuDomainObjListAdd() so
> that it will keep the job set upon return (if requested by
> caller).
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c| 17 ++---
> src/qemu/qemu_domain.h| 1 +
> src/qemu/qemu_driver.c| 24 +++-
> src/qemu/qemu_migration.c | 2 +-
> 4 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f6b677c69e..b0b3fa5fd8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7061,6 +7061,7 @@ virDomainDefParserConfig
> virQEMUDriverDomainDefParserConfig = {
> * @def: domain definition
> * @oldDef: previous domain definition
> * @live: whether @def is live definition
> + * @keepJob: whether to leave MODIFY job set on returned object
> * @flags: an bitwise-OR of virDomainObjListAdd flags
> *
> * Add a domain onto the list of domain object and sets its
> @@ -7070,6 +7071,10 @@ virDomainDefParserConfig
> virQEMUDriverDomainDefParserConfig = {
> * In addition to that, if definition of an existing domain is
> * changed a MODIFY job is acquired prior to that.
> *
> + * If @keepJob is true, then the MODIFY job is not ended upon
> + * successful return from this function. This might be handy if
> + * caller would try to acquire the job anyway.
> + *
> * Returns: domain object pointer on success,
> * NULL otherwise.
> */
> @@ -7078,9 +7083,11 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
> virDomainDefPtr def,
> virDomainDefPtr *oldDef,
> bool live,
> + bool keepJob,
> unsigned int flags)
> {
> virDomainObjPtr vm = NULL;
> +bool defSet = false;
>
> if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt,
> flags)))
> return NULL;
> @@ -7091,7 +7098,9 @@ qemuDomainObjListAdd(virQEMUDriverPtr driver,
> * just set the definition without acquiring job. */
> if (!vm->def) {
> virDomainObjAssignDef(vm, def, live, oldDef);
> -VIR_RETURN_PTR(vm);
> +defSet = true;
> +if (!keepJob)
> +VIR_RETURN_PTR(vm);
> }
Just realized. If we got in this branch and have @keepJob = true
and later fail to aqcuire job and remove vm entirely then we
free @def which is unexpected by callers.
Nikolay
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] backup: Add new qemu monitor bitmap
On 6/6/19 7:48 AM, Peter Krempa wrote:
> On Wed, Jun 05, 2019 at 22:01:09 -0500, Eric Blake wrote:
>> The upcoming virDomainBackup() API needs to take advantage of various
>> qcow2 bitmap manipulations as the basis to virDomainCheckpoints and
>> incremental backups. Add four functions to expose
>> block-dirty-bitmap-{add,enable,disable,merge} (this is the
>> recently-added QEMU_CAPS_BITMAP_MERGE capability).
>>
>> Signed-off-by: Eric Blake
>> ---
>> src/qemu/qemu_monitor.h | 19 ++
>> src/qemu/qemu_monitor_json.h | 17 +
>> src/qemu/qemu_monitor.c | 51 +++
>> src/qemu/qemu_monitor_json.c | 119 +++
>> 4 files changed, 206 insertions(+)
>
> I'd suggest you add GEN_TEST_FUNC/DO_TEST_GEN tests for those. It's
> simple to add and gives you checking of the arguments against the QAPI
> schema for free.
Will do. Do you need to see the amended patch with that added, or should
I go ahead and push once I have it working?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] backup: Prepare for Unix sockets in QMP nbd-server-start
On 6/6/19 7:53 AM, Peter Krempa wrote:
> On Wed, Jun 05, 2019 at 22:01:07 -0500, Eric Blake wrote:
>> Migration always uses a TCP socket for NBD servers, because we don't
>> support same-host migration. But upcoming pull-mode incremental backup
>> needs to also support a Unix socket, for retrieving the backup from
>> the same host. Support this by plumbing virStorageNetHostDef through
>> the monitor calls, since that is a nice reusable struct that can track
>> both TCP and Unix sockets.
>>
>> Update qemumonitorjsontest to not only test the response to the
>> command, but to actually verify that the command itself uses the two
>> correct QMP forms. I'm actually a bit surprised that we are not
>> utilizing qemuMonitorTestAddItemVerbatim more frequently.
>>
>> Signed-off-by: Eric Blake
>> ---
>> +static int
>> +testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
>> +{
>> +virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr) opaque;
>> +qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
>
> I'd suggest you use qemuMonitorTestNewSchema here so that we preserve
> the schema checks.
Oh, I _completely_ missed what TestNewSchema was providing.
>
>> +int ret = -1;
>> +virStorageNetHostDef server_tcp = {
>> +.name = (char *)"localhost",
>> +.port = 12345,
>> +.transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
>> +};
>> +virStorageNetHostDef server_unix = {
>> +.socket = (char *)"/tmp/sock",
>> +.transport = VIR_STORAGE_NET_HOST_TRANS_UNIX,
>> +};
>> +
>> +if (!test)
>> +return -1;
>> +
>> +if (qemuMonitorTestAddItemVerbatim(test,
>> + "{\"execute\":\"nbd-server-start\","
>> + " \"arguments\":{\"addr\":{"
>> + " \"type\":\"inet\",\"data\":{"
>> + " \"host\":\"localhost\","
>> + " \"port\":\"12345\"}},"
>> + " \"tls-creds\":\"test-alias\"},"
>> + " \"id\":\"libvirt-1\"}",
>> + NULL, "{\"return\":{}}") < 0)
Testing for a verbatim response proves that we generated at least one
form of acceptable JSON, but if TestNewSchema is able to validate that
the entire command complies with the introspected schema advertised by
qemu, then that also serves to validate things, and with a lot less
fragility. Yes, I'll fix that, now that I _finally_ understand what you
were asking for (you made a similar comment in your v8 review, and I
misunderstood it).
>> @@ -3036,6 +3089,7 @@ mymain(void)
>
> ACK with schema checks preserved.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/4] backup: Add two new qemu capabilities
On 6/6/19 7:51 AM, Peter Krempa wrote: > On Wed, Jun 05, 2019 at 22:01:08 -0500, Eric Blake wrote: >> Add two capabilities for testing features required for the upcoming >> virDomainBackupBegin: use block-dirty-bitmap-merge as the generic >> witness of bitmap support needed for checkpoints (since all of the >> bitmap management functionalities were finalized in the same qemu 4.0 >> release), and the bitmap parameter to nbd-server-add for pull-mode >> backup support. Even though both capabilities are likely to be >> present or absent together (that is, it is unlikely to encounter a >> qemu that backports only one of the two), it still makes sense to keep >> two capabilities as the two uses are orthogonal (full backups don't >> require checkpoints, push mode backups don't require NBD bitmap >> support, and checkpoints can be used for more than just incremental >> backups). >> >> Signed-off-by: Eric Blake >> --- >> src/qemu/qemu_capabilities.h | 4 >> src/qemu/qemu_capabilities.c | 6 ++ >> tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 ++ >> tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 ++ >> tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 ++ >> tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 ++ >> tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 ++ >> tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 ++ >> 8 files changed, 22 insertions(+) > > So the code looks good to me, but we need to clarify one thing before > ACK. > > Is there anything that would change libvirt's behaviour incompatibly if > these are specified? In some cases these also imply a libvirt behaviour > change (e.g. different command line) and thus not pushing those in the > same release as the implementation might cause problems. So far, the only things that depend on the capabilities are the new code additions for checkpoints and pull-mode backups; existing libvirt.git should have no change in behavior whether or not the capabilities are detected. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 11/11] libxl_domain: Allow libxlDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote: > In some cases, caller of libxlDomainObjListAdd() tries to acquire > MODIFY job after the call. Let's adjust libxlDomainObjListAdd() so > that it will keep the job set upon return (if requested by > caller). > > Signed-off-by: Michal Privoznik > --- > src/libxl/libxl_domain.c| 17 ++--- > src/libxl/libxl_domain.h| 1 + > src/libxl/libxl_driver.c| 24 +++- > src/libxl/libxl_migration.c | 18 ++ > 4 files changed, 24 insertions(+), 36 deletions(-) > Reviewed-by: Nikolay Shirokovskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/11] libxl: Grab modify job for changing domain XML
On 05.06.2019 12:09, Michal Privoznik wrote:
> The reasoning here is the same as in qemu driver fixed in
> previous commit. Long story short, changing an XML of a domain
> requires modify job to be acquired.
>
> Signed-off-by: Michal Privoznik
> ---
> src/libxl/libxl_domain.c| 57 +
> src/libxl/libxl_domain.h| 7 +
> src/libxl/libxl_driver.c| 24
> src/libxl/libxl_migration.c | 14 +++--
> 4 files changed, 73 insertions(+), 29 deletions(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 2d8569e592..f2e1af52e5 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -65,6 +65,63 @@ libxlDomainObjPrivateOnceInit(void)
>
> VIR_ONCE_GLOBAL_INIT(libxlDomainObjPrivate);
>
> +
> +/**
> + * libxlDomainObjListAdd:
> + * @driver: libxl driver
> + * @def: domain definition
> + * @oldDef: previous domain definition
> + * @live: whether @def is live definition
> + * @flags: an bitwise-OR of virDomainObjListAdd flags
> + *
> + * Add a domain onto the list of domain object and sets its
> + * definition. If @oldDef is not NULL and there was pre-existing
> + * definition it's returned in @oldDef.
> + *
> + * In addition to that, if definition of an existing domain is
> + * changed a MODIFY job is acquired prior to that.
> + *
> + * Returns: domain object pointer on success,
> + * NULL otherwise.
> + */
> +virDomainObjPtr
> +libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
> + virDomainDefPtr def,
> + virDomainDefPtr *oldDef,
> + bool live,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +
> +if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt,
> flags)))
> +return NULL;
> +
> +/* At this point, @vm is locked. If it doesn't have any
> + * definition set, then the call above just added it and
> + * there can't be anybody else using the object. It's safe to
> + * just set the definition without acquiring job. */
> +if (!vm->def) {
> +virDomainObjAssignDef(vm, def, live, oldDef);
> +VIR_RETURN_PTR(vm);
> +}
> +
> +/* Bad luck. Domain was pre-existing and this call is trying
> + * to update its definition. Modify job MUST be acquired. */
> +if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
> +if (!vm->persistent)
> +virDomainObjListRemove(driver->domains, vm);
> +virDomainObjEndAPI(&vm);
> +return NULL;
> +}
> +
> +virDomainObjAssignDef(vm, def, live, oldDef);
> +
> +libxlDomainObjEndJob(driver, vm);
> +
> +return vm;
> +}
> +
> +
> static int
> libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv)
> {
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index 769ee8ec4d..b5d5127e91 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -83,6 +83,13 @@ extern const struct libxl_event_hooks ev_hooks;
> int
> libxlDomainObjPrivateInitCtx(virDomainObjPtr vm);
>
> +virDomainObjPtr
> +libxlDomainObjListAdd(libxlDriverPrivatePtr driver,
> + virDomainDefPtr def,
> + virDomainDefPtr *oldDef,
> + bool live,
> + unsigned int flags);
> +
> int
> libxlDomainObjBeginJob(libxlDriverPrivatePtr driver,
> virDomainObjPtr obj,
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 809d298ac1..9c9a30bb90 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -601,12 +601,8 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
> if (virUUIDParse("----", def->uuid) < 0)
> goto cleanup;
>
> -if (!(vm = virDomainObjListAdd(driver->domains, def,
> - driver->xmlopt,
> - 0)))
> +if (!(vm = libxlDomainObjListAdd(driver, def, &oldDef, false, 0)))
> goto cleanup;
> -
> -virDomainObjAssignDef(vm, def, false, &oldDef);
> def = NULL;
>
> vm->persistent = 1;
> @@ -1030,12 +1026,9 @@ libxlDomainCreateXML(virConnectPtr conn, const char
> *xml,
> if (virDomainCreateXMLEnsureACL(conn, def) < 0)
> goto cleanup;
>
> -if (!(vm = virDomainObjListAdd(driver->domains, def,
> - driver->xmlopt,
> - VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
> +if (!(vm = libxlDomainObjListAdd(driver, def, NULL, true,
> + VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE)))
> goto cleanup;
> -
> -virDomainObjAssignDef(vm, def, true, NULL);
> def = NULL;
>
> if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
> @@ -1950,12 +1943,9 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char
> *from,
> if (virDomainRestoreFl
Re: [libvirt] [PATCH v2 09/11] lxc_domain: Allow lxcDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote: > In some cases, caller of lxcDomainObjListAdd() tries to acquire > MODIFY job after the call. Let's adjust lxcDomainObjListAdd() so > that it will keep the job set upon return (if requested by > caller). > > Signed-off-by: Michal Privoznik > --- > src/lxc/lxc_domain.c | 17 ++--- > src/lxc/lxc_domain.h | 1 + > src/lxc/lxc_driver.c | 19 --- > 3 files changed, 23 insertions(+), 14 deletions(-) > Reviewed-by: Nikolay Shirokovskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 08/11] lxc: Grab modify job for changing domain XML
On 05.06.2019 12:09, Michal Privoznik wrote: > The reasoning here is the same as in qemu driver fixed in > previous commit. Long story short, changing an XML of a domain > requires modify job to be acquired. > > Signed-off-by: Michal Privoznik > --- > src/lxc/lxc_domain.c | 57 > src/lxc/lxc_domain.h | 7 ++ > src/lxc/lxc_driver.c | 11 +++-- > 3 files changed, 67 insertions(+), 8 deletions(-) > Reviewed-by: Nikolay Shirokovskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] backup: Prepare for Unix sockets in QMP nbd-server-start
On Wed, Jun 05, 2019 at 22:01:07 -0500, Eric Blake wrote:
> Migration always uses a TCP socket for NBD servers, because we don't
> support same-host migration. But upcoming pull-mode incremental backup
> needs to also support a Unix socket, for retrieving the backup from
> the same host. Support this by plumbing virStorageNetHostDef through
> the monitor calls, since that is a nice reusable struct that can track
> both TCP and Unix sockets.
>
> Update qemumonitorjsontest to not only test the response to the
> command, but to actually verify that the command itself uses the two
> correct QMP forms. I'm actually a bit surprised that we are not
> utilizing qemuMonitorTestAddItemVerbatim more frequently.
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_monitor.h | 6 ++--
> src/qemu/qemu_monitor_json.h | 3 +-
> src/qemu/qemu_migration.c| 7 -
> src/qemu/qemu_monitor.c | 13 +---
> src/qemu/qemu_monitor_json.c | 23 ++
> tests/qemumonitorjsontest.c | 58 ++--
> 6 files changed, 92 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index dee594fa66..fa84ff821e 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1094,9 +1094,9 @@ int qemuMonitorGetObjectProps(qemuMonitorPtr mon,
> char *qemuMonitorGetTargetArch(qemuMonitorPtr mon);
>
> int qemuMonitorNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> - const char *tls_alias);
> + const virStorageNetHostDef *server,
> + const char *tls_alias)
> +ATTRIBUTE_NONNULL(2);
> int qemuMonitorNBDServerAdd(qemuMonitorPtr mon,
> const char *deviceID,
> bool writable);
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index acef1a0a79..e41bdc8c4f 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -459,8 +459,7 @@ int qemuMonitorJSONGetObjectProps(qemuMonitorPtr mon,
> char *qemuMonitorJSONGetTargetArch(qemuMonitorPtr mon);
>
> int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> + const virStorageNetHostDef *server,
>const char *tls_alias);
> int qemuMonitorJSONNBDServerAdd(qemuMonitorPtr mon,
> const char *deviceID,
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 32b3040473..267a729c6f 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -380,6 +380,10 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver,
> unsigned short port = 0;
> char *diskAlias = NULL;
> size_t i;
> +virStorageNetHostDef server = {
> +.name = (char *)listenAddr, /* cast away const */
> +.transport = VIR_STORAGE_NET_HOST_TRANS_TCP,
> +};
>
> if (nbdPort < 0 || nbdPort > USHRT_MAX) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -415,7 +419,8 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver,
> else if (virPortAllocatorAcquire(driver->migrationPorts, &port)
> < 0)
> goto exit_monitor;
>
> -if (qemuMonitorNBDServerStart(priv->mon, listenAddr, port,
> tls_alias) < 0)
> +server.port = port;
> +if (qemuMonitorNBDServerStart(priv->mon, &server, tls_alias) < 0)
> goto exit_monitor;
> }
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 6b731cd91a..187513a986 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3925,15 +3925,20 @@ qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon,
>
> int
> qemuMonitorNBDServerStart(qemuMonitorPtr mon,
> - const char *host,
> - unsigned int port,
> + const virStorageNetHostDef *server,
>const char *tls_alias)
> {
> -VIR_DEBUG("host=%s port=%u tls_alias=%s", host, port,
> NULLSTR(tls_alias));
> +/* Peek inside the struct for nicer logging */
> +if (server->transport == VIR_STORAGE_NET_HOST_TRANS_TCP)
> +VIR_DEBUG("server={tcp host=%s port=%u} tls_alias=%s",
> + NULLSTR(server->name), server->port, NULLSTR(tls_alias));
> +else
> +VIR_DEBUG("server={unix socket=%s} tls_alias=%s",
> + NULLSTR(server->socket), NULLSTR(tls_alias));
>
> QEMU_CHECK_MONITOR(mon);
>
> -return qemuMonitorJSONNBDServerStart(mon, host, port, tls_alias);
> +return qemuMonitorJSONNBDServerStart(mon, server, tls_alias);
> }
>
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor
Re: [libvirt] [PATCH 2/4] backup: Add two new qemu capabilities
On Wed, Jun 05, 2019 at 22:01:08 -0500, Eric Blake wrote: > Add two capabilities for testing features required for the upcoming > virDomainBackupBegin: use block-dirty-bitmap-merge as the generic > witness of bitmap support needed for checkpoints (since all of the > bitmap management functionalities were finalized in the same qemu 4.0 > release), and the bitmap parameter to nbd-server-add for pull-mode > backup support. Even though both capabilities are likely to be > present or absent together (that is, it is unlikely to encounter a > qemu that backports only one of the two), it still makes sense to keep > two capabilities as the two uses are orthogonal (full backups don't > require checkpoints, push mode backups don't require NBD bitmap > support, and checkpoints can be used for more than just incremental > backups). > > Signed-off-by: Eric Blake > --- > src/qemu/qemu_capabilities.h | 4 > src/qemu/qemu_capabilities.c | 6 ++ > tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 2 ++ > tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 2 ++ > tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 2 ++ > tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 2 ++ > tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 ++ > tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 ++ > 8 files changed, 22 insertions(+) So the code looks good to me, but we need to clarify one thing before ACK. Is there anything that would change libvirt's behaviour incompatibly if these are specified? In some cases these also imply a libvirt behaviour change (e.g. different command line) and thus not pushing those in the same release as the implementation might cause problems. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/4] backup: Add new parameters to qemu monitor nbd-server-add
On Wed, Jun 05, 2019 at 22:01:10 -0500, Eric Blake wrote: > The upcoming virDomainBackup() API needs to take advantage of the > ability to expose a bitmap as part of nbd-server-add for a pull-mode > backup (this is the recently-added QEMU_CAPS_NBD_BITMAP capability). > > Signed-off-by: Eric Blake > --- > src/qemu/qemu_monitor.h | 4 +++- > src/qemu/qemu_monitor_json.h | 4 +++- > src/qemu/qemu_migration.c| 2 +- > src/qemu/qemu_monitor.c | 10 +++--- > src/qemu/qemu_monitor_json.c | 7 ++- > tests/qemumonitorjsontest.c | 2 +- > 6 files changed, 21 insertions(+), 8 deletions(-) [...] > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c > index 9d707fcc7c..2a9e6cc75f 100644 > --- a/tests/qemumonitorjsontest.c > +++ b/tests/qemumonitorjsontest.c > @@ -1348,7 +1348,7 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", > "/foo/bar1", "/foo/bar2", "back > GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb") > GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, "/foo/bar") > GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) > -GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", true) > +GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", NULL, true, NULL) Please use non-NULL attributes here to ensure schema checking. > GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1") > GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayOpen, "foodev", true) > GEN_TEST_FUNC(qemuMonitorJSONBlockdevTrayClose, "foodev") ACK with the non-null test added and make check re-run. signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/4] backup: Add new qemu monitor bitmap
On Wed, Jun 05, 2019 at 22:01:09 -0500, Eric Blake wrote:
> The upcoming virDomainBackup() API needs to take advantage of various
> qcow2 bitmap manipulations as the basis to virDomainCheckpoints and
> incremental backups. Add four functions to expose
> block-dirty-bitmap-{add,enable,disable,merge} (this is the
> recently-added QEMU_CAPS_BITMAP_MERGE capability).
>
> Signed-off-by: Eric Blake
> ---
> src/qemu/qemu_monitor.h | 19 ++
> src/qemu/qemu_monitor_json.h | 17 +
> src/qemu/qemu_monitor.c | 51 +++
> src/qemu/qemu_monitor_json.c | 119 +++
> 4 files changed, 206 insertions(+)
I'd suggest you add GEN_TEST_FUNC/DO_TEST_GEN tests for those. It's
simple to add and gives you checking of the arguments against the QAPI
schema for free.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 07/11] qemu_domain: Allow qemuDomainObjListAdd to keep job upon return
On 05.06.2019 12:09, Michal Privoznik wrote: > In some cases, caller of qemuDomainObjListAdd() tries to acquire > MODIFY job after the call. Let's adjust qemuDomainObjListAdd() so > that it will keep the job set upon return (if requested by > caller). > > Signed-off-by: Michal Privoznik > --- > src/qemu/qemu_domain.c| 17 ++--- > src/qemu/qemu_domain.h| 1 + > src/qemu/qemu_driver.c| 24 +++- > src/qemu/qemu_migration.c | 2 +- > 4 files changed, 27 insertions(+), 17 deletions(-) > Reviewed-by: Nikolay Shirokovskiy -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 06/11] qemu: Grab modify job for changing domain XML
On 05.06.2019 12:09, Michal Privoznik wrote:
> Changing domain definition must be guarded with acquiring modify
> job. The problem is if there is a thread executing say
> qemuDomainSetMemoryStatsPeriod() which is an API that acquires
> modify job and then possibly unlock the domain object and locks
> it again. However, becasue virDomainObjAssignDef() does not take
> a job (only object lock) it may have changed the domain
> definition while the other thread unlocked the domain object in
> order to talk on the monitor. For instance:
>
> Thread1:
> 1) lookup domain object and lock it
> 2) acquire job
> 3) get pointers to live and persistent defs
> 4) unlock the domain object
> 5) talk to qemu on the monitor
>
> Thread2 (Execute qemuDomainDefineXMLFlags):
> 1) lookup domain object and lock it
> 2) virDomainObjAssignDef() which overwrites persistent def and
>frees the old one
> 3) unlock domain object
>
> Thread1:
> 6) lock the domain object again
> 7) try to access the persistent def via pointer stored in 3)
>
> Now, this will crash because the pointer from step 3) points to a
> memory that was freed.
>
> However, if Thread2 waited and acquired modify job (just like
> Thread1) then these two would serialize and no harm would be
> done.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_domain.c| 55 +++
> src/qemu/qemu_domain.h| 6 +
> src/qemu/qemu_driver.c| 27 ++-
> src/qemu/qemu_migration.c | 5 +---
> 4 files changed, 70 insertions(+), 23 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4d3a8868b2..f6b677c69e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7055,6 +7055,61 @@ virDomainDefParserConfig
> virQEMUDriverDomainDefParserConfig = {
> };
>
>
> +/**
> + * qemuDomainObjListAdd:
> + * @driver: qemu driver
> + * @def: domain definition
> + * @oldDef: previous domain definition
> + * @live: whether @def is live definition
> + * @flags: an bitwise-OR of virDomainObjListAdd flags
> + *
> + * Add a domain onto the list of domain object and sets its
> + * definition. If @oldDef is not NULL and there was pre-existing
> + * definition it's returned in @oldDef.
> + *
> + * In addition to that, if definition of an existing domain is
> + * changed a MODIFY job is acquired prior to that.
> + *
> + * Returns: domain object pointer on success,
> + * NULL otherwise.
> + */
> +virDomainObjPtr
> +qemuDomainObjListAdd(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + virDomainDefPtr *oldDef,
> + bool live,
> + unsigned int flags)
> +{
> +virDomainObjPtr vm = NULL;
> +
> +if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt,
> flags)))
> +return NULL;
> +
> +/* At this point, @vm is locked. If it doesn't have any
> + * definition set, then the call above just added it and
> + * there can't be anybody else using the object. It's safe to
> + * just set the definition without acquiring job. */
> +if (!vm->def) {
> +virDomainObjAssignDef(vm, def, live, oldDef);
> +VIR_RETURN_PTR(vm);
Not sure why use VIR_RETURN_PTR
> +}
> +
> +/* Bad luck. Domain was pre-existing and this call is trying
> + * to update its definition. Modify job MUST be acquired. */
> +if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
> +qemuDomainRemoveInactive(driver, vm);
> +virDomainObjEndAPI(&vm);
> +return NULL;
> +}
> +
> +virDomainObjAssignDef(vm, def, live, oldDef);
> +
> +qemuDomainObjEndJob(driver, vm);
> +
> +return vm;
> +}
> +
> +
> static void
> qemuDomainObjSaveJob(virQEMUDriverPtr driver, virDomainObjPtr obj)
> {
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index f92f0dbc27..f469f8eaca 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -543,6 +543,12 @@ void qemuDomainEventFlush(int timer, void *opaque);
> void qemuDomainEventEmitJobCompleted(virQEMUDriverPtr driver,
> virDomainObjPtr vm);
>
> +virDomainObjPtr qemuDomainObjListAdd(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + virDomainDefPtr *oldDef,
> + bool live,
> + unsigned int flags);
> +
> int qemuDomainObjBeginJob(virQEMUDriverPtr driver,
>virDomainObjPtr obj,
>qemuDomainJob job)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 8bbac339e0..fa93a686b7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1701,12 +1701,9 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr
> conn,
> if (virDomainCreateXMLEnsureACL(conn, def) < 0)
> goto cleanup;
>
Re: [libvirt] [PATCH v2 05/11] virDomainObjListAdd: Remove unused flag
On 05.06.2019 12:09, Michal Privoznik wrote:
> After the previous commit the VIR_DOMAIN_OBJ_LIST_ADD_LIVE flag
> is not used anymore. Let's remove it.
>
> Signed-off-by: Michal Privoznik
> ---
> src/conf/virdomainobjlist.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
> index 552a7cfaf2..54e7871c67 100644
> --- a/src/conf/virdomainobjlist.h
> +++ b/src/conf/virdomainobjlist.h
> @@ -38,8 +38,7 @@ virDomainObjPtr
> virDomainObjListFindByName(virDomainObjListPtr doms,
> const char *name);
>
> enum {
> -VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
> -VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
> +VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE = (1 << 0),
> };
> virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
> virDomainDefPtr def,
>
Reviewed-by: Nikolay Shirokovskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] qemu: Fix leak in qemuProcessInitCpuAffinity()
On 6/4/19 8:46 AM, Andrea Bolognani wrote:
> In two out of three scenarios we were cleaning up properly
> after ourselves, but in the remaining one we were leaking
> cpumapToSet.
>
> Refactor the logic so that cpumapToSet is always a freshly
> allocated bitmap that gets cleaned up automatically thanks
> to VIR_AUTOPTR(); this also allows us to remove hostcpumap.
>
> Reported-by: John Ferlan
> Signed-off-by: Andrea Bolognani
> ---
> src/qemu/qemu_process.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d44076288e..7d48c95973 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2464,8 +2464,7 @@ static int
> qemuProcessInitCpuAffinity(virDomainObjPtr vm)
> {
> int ret = -1;
> -virBitmapPtr cpumapToSet = NULL;
> -VIR_AUTOPTR(virBitmap) hostcpumap = NULL;
> +VIR_AUTOPTR(virBitmap) cpumapToSet = NULL;
> virDomainNumatuneMemMode mem_mode;
> qemuDomainObjPrivatePtr priv = vm->privateData;
>
> @@ -2500,11 +2499,11 @@ qemuProcessInitCpuAffinity(virDomainObjPtr vm)
> if (virNumaNodesetToCPUset(nodeset, &cpumapToSet) < 0)
> goto cleanup;
> } else if (vm->def->cputune.emulatorpin) {
> -cpumapToSet = vm->def->cputune.emulatorpin;
> +if (virBitmapCopy(cpumapToSet, vm->def->cputune.emulatorpin) < 0)
Now Coverity is unhappy for another reason. What happens to the NULL
cpumapToSet when calling virBitmapCopy?
Should have been virBitmapNewCopy
John
(sorry didn't get a chance to look at the patch when first posted)
> +goto cleanup;
> } else {
> -if (qemuProcessGetAllCpuAffinity(&hostcpumap) < 0)
> +if (qemuProcessGetAllCpuAffinity(&cpumapToSet) < 0)
> goto cleanup;
> -cpumapToSet = hostcpumap;
> }
>
> if (cpumapToSet &&
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH v4 0/3] numa: deprecate '-numa node, mem' and default memory distribution
On Wed, Jun 05, 2019 at 03:06:08PM -0300, Eduardo Habkost wrote: > On Wed, Jun 05, 2019 at 06:33:11PM +0100, Daniel P. Berrangé wrote: > [...] > > I wonder if there's a way to close the testing gap somehow ? Random idea > > would be a non-versioned "pc-no-deprecated" machine type, which blocks > > all use of deprecated features and does not promise any migration compat. > > Essentially it would exist just for testing purposem as a way todo > > functional tests for libvirt & mgmt apps to prove they don't use any > > deprecated features (any deprecated features, not merely this NUMA one). > > This isn't the first time I wish we had a machine type with > experimental features enabled. What about calling it "pc-next"? No objection from me. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu_conf: Fix XPath for parsing TSC frequency
On Thu, Jun 06, 2019 at 09:33:02AM +0200, Jiri Denemark wrote: Due to this bug the following command would fail on any host where TSC frequency can be probed: $ virsh capabilities | virsh cpu-baseline /dev/stdin error: unsupported configuration: Invalid TSC frequency https://bugzilla.redhat.com/show_bug.cgi?id=1641702 Signed-off-by: Jiri Denemark --- src/conf/cpu_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] cpu_conf: Fix XPath for parsing TSC frequency
Due to this bug the following command would fail on any host where TSC
frequency can be probed:
$ virsh capabilities | virsh cpu-baseline /dev/stdin
error: unsupported configuration: Invalid TSC frequency
https://bugzilla.redhat.com/show_bug.cgi?id=1641702
Signed-off-by: Jiri Denemark
---
src/conf/cpu_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index dc46e7f57a..825df88246 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -417,8 +417,8 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
if (VIR_ALLOC(tsc) < 0)
goto cleanup;
-if (virXPathULongLong("./counter[@name='tsc']/@frequency", ctxt,
- &tsc->frequency) < 0) {
+if (virXPathULongLong("string(./counter[@name='tsc']/@frequency)",
+ ctxt, &tsc->frequency) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Invalid TSC frequency"));
goto cleanup;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
