Re: [PATCH V2] qemu: pre-create the dbus directory in qemuStateInitialize
On Wednesday, 22 July 2020 04:09:24 CEST Bihong Yu wrote:
> +if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
> + VIR_DIR_CREATE_ALLOW_EXIST) < 0) {
> +virReportSystemError(errno, _("Failed to create dbus state dir %s"),
> + cfg->dbusStateDir);
Minor notes on the message:
- spell "D-Bus" correctly
- no need to abbreviate "directory"
- quote the path placeholder
so I suggest something like:
"Failed to create the D-Bus state directory '%s'"
(Can't comment on the rest of the changes, sorry.)
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] news: Remove one of last two instances of -drive if=none usage
On Wed, Jul 22, 2020 at 10:13:16 +0800, Jianan Gao wrote: > There is a series of patched for describing remove one of last two instances > of -drive if=none usage to help QEMU in deprecation of -drive if=none without > the need to refactor all old boards. > > Signed-off-by: Jianan Gao > --- > NEWS.rst | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index ff977968c7..b763e45e11 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -147,6 +147,11 @@ v6.4.0 (2020-06-02) > already does in these cases. Users are encouraged to provide complete > NUMA > topologies to avoid unexpected changes in the domain XML. > > + * qemu: remove one of last two instances of -drive if=none usage > + > + Â Remove one of last two instances of -drive if=none usage to help QEMU > in > +deprecation of -drive if=none without the need to refactor all old > boards. I don't think it's worth documenting this change. It's meant to be totally transparent for users.
[PATCH] news: Remove one of last two instances of -drive if=none usage
There is a series of patched for describing remove one of last two instances of -drive if=none usage to help QEMU in deprecation of -drive if=none without the need to refactor all old boards. Signed-off-by: Jianan Gao --- NEWS.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index ff977968c7..b763e45e11 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -147,6 +147,11 @@ v6.4.0 (2020-06-02) already does in these cases. Users are encouraged to provide complete NUMA topologies to avoid unexpected changes in the domain XML. + * qemu: remove one of last two instances of -drive if=none usage + + Remove one of last two instances of -drive if=none usage to help QEMU in +deprecation of -drive if=none without the need to refactor all old boards. + * **Bug fixes** * qemu: fixed regression in network device hotplug with new qemu versions -- 2.21.3
[PATCH V2] qemu: pre-create the dbus directory in qemuStateInitialize
>From 165abdd77c7db83ebf98232b80d6b988471185c0 Mon Sep 17 00:00:00 2001
From: Bihong Yu
Date: Tue, 14 Jul 2020 15:44:05 +0800
Subject: [PATCH] qemu: pre-create the dbus directory in qemuStateInitialize
There are races condiction to make '/run/libvirt/qemu/dbus' directory in
virDirCreateNoFork() while concurrent start VMs, and get "failed to create
directory '/run/libvirt/qemu/dbus': File exists" error message. pre-create the
dbus directory in qemuStateInitialize.
Signed-off-by:Bihong Yu
---
src/qemu/qemu_dbus.c| 10 --
src/qemu/qemu_dbus.h| 2 --
src/qemu/qemu_driver.c | 7 +++
src/qemu/qemu_process.c | 3 ---
4 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
index 51f6c94..8104287 100644
--- a/src/qemu/qemu_dbus.c
+++ b/src/qemu/qemu_dbus.c
@@ -33,16 +33,6 @@
VIR_LOG_INIT("qemu.dbus");
-int
-qemuDBusPrepareHost(virQEMUDriverPtr driver)
-{
-g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-
-return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
-VIR_DIR_CREATE_ALLOW_EXIST);
-}
-
-
static char *
qemuDBusCreatePidFilename(virQEMUDriverConfigPtr cfg,
const char *shortName)
diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h
index 474eb10..3c2145a 100644
--- a/src/qemu/qemu_dbus.h
+++ b/src/qemu/qemu_dbus.h
@@ -21,8 +21,6 @@
#include "qemu_conf.h"
#include "qemu_domain.h"
-int qemuDBusPrepareHost(virQEMUDriverPtr driver);
-
char *qemuDBusGetAddress(virQEMUDriverPtr driver,
virDomainObjPtr vm);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8e81c30..53980d4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -743,6 +743,13 @@ qemuStateInitialize(bool privileged,
goto error;
}
+if (virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
+ VIR_DIR_CREATE_ALLOW_EXIST) < 0) {
+virReportSystemError(errno, _("Failed to create dbus state dir %s"),
+ cfg->dbusStateDir);
+goto error;
+}
+
if ((qemu_driver->lockFD =
virPidFileAcquire(cfg->stateDir, "driver", false, getpid())) < 0)
goto error;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e8b15ee..1006f41 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6466,9 +6466,6 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-if (qemuDBusPrepareHost(driver) < 0)
-return -1;
-
if (qemuPrepareNVRAM(cfg, vm) < 0)
return -1;
--
1.8.3.1
Re: [libvirt PATCH] docs: virConnectGetCapabilities do not provide pool types
On 7/21/20 10:14 AM, Daniel P. Berrangé wrote: > On Tue, Jul 21, 2020 at 08:46:55AM -0400, John Ferlan wrote: >> >> Upon further reflection and some memory jiggling... >> >> virsh -c storage:///system capabilities >> >> >> >> >> dir >> fs >> netfs >> logical >> iscsi >> iscsi-direct >> scsi >> mpath >> disk >> rbd >> sheepdog >> gluster >> zfs >> >> >> >> >> >> But yeah, without the -c storage:///system one won't see the results >> because 'npools == 0' when virCapabilitiesFormatStoragePoolXML is called >> from virCapabilitiesFormatXML unless it's the storage driver URI. > > Err, don't you mean "virsh pool-capabilities". > pool-capabilities takes a different path ending in virStoragePoolCapsFormat as opposed to capabilities which ends up in virCapabilitiesFormatXML > The "capabilities" command is exclusively for the virt driver usage, > not storage. # virsh version Compiled against library: libvirt 6.6.0 Using library: libvirt 6.6.0 Using API: QEMU 6.6.0 Running hypervisor: QEMU 5.0.50 # virsh -c storage:///system capabilities [as above] This work is/was done Jan/Feb 2019 - Perhaps I'm only getting asked (again) about it now as a result of some downstream documenting :-/. Far too long ago for me to recall why both options were done. But separable enough in commit 05fe03505a to be undone. John > > In the new modular daemon work, "capabilities" API will never be > sent to the storage daemon. > > Regards, > Daniel >
Re: [PATCH 2/4] conf: allow to map sound device to host device
On Sat, Jul 18, 2020 at 04:31:16PM +0400, Roman Bogorodskiy wrote:
> Extend device element to accept "soundDevice"
> sub-element which allows to map guest sound device to host
> sound device.
>
> Example
>
>
>
>
IIUC, FreeBSD's audio subsystem is the classic OSS API ?
>
> The "playback" attribute points to the playback device,
> and "recording" attribute points to the recording device.
I'm thinking about how we'll have to deal with QEMU's sound backend
options, and alignment with BHyve / FreeBSD.
In QEMU there are multiple backends, OSS, ALSA, PulseAudio, SPICE,
VNC, and many more. The backends have many properties, and many
of the properties can be set separately for input and output.
IIUC, QEMU can expose multiple sound devices to the guest too.
I think this means that we can have a M:N relationship between
a sound device, and an audio backend, not just 1:1.
Assuming I'm right about the M:N relationship, I assume that
of multiple cards all do playback concurrently, something
will have todo mixing of the streams ? How will that work
with audio capture, is only one card allowed to capture at
any time ?
I'm copying Gerd to confirm the above...
Anyway, if we have M:N relation, then we'll need separate
configuration elements.
So I think we'd need to allow something like this:
Here we have one sound device connected to OSS, and two sound
devices connected to SPICE.
>
> Signed-off-by: Roman Bogorodskiy
> ---
> docs/schemas/domaincommon.rng | 15 +++
> src/conf/domain_conf.c| 24
> src/conf/domain_conf.h| 3 +++
> 3 files changed, 42 insertions(+)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index a810f569c6..b11b3f2af6 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4346,6 +4346,18 @@
>
>
>
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
>
>
>
> @@ -4366,6 +4378,9 @@
>
>
>
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7ecd2818b9..b678a2319d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2850,6 +2850,9 @@ void virDomainSoundDefFree(virDomainSoundDefPtr def)
> virDomainSoundCodecDefFree(def->codecs[i]);
> VIR_FREE(def->codecs);
>
> +VIR_FREE(def->playback);
> +VIR_FREE(def->recording);
> +
> VIR_FREE(def);
> }
>
> @@ -14984,6 +14987,8 @@ virDomainSoundDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> virDomainSoundDefPtr def;
> VIR_XPATH_NODE_AUTORESTORE(ctxt);
> g_autofree char *model = NULL;
> +char *recording = NULL;
> +xmlNodePtr soundDeviceNode;
>
> if (VIR_ALLOC(def) < 0)
> return NULL;
> @@ -15024,6 +15029,14 @@ virDomainSoundDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> }
> }
>
> +soundDeviceNode = virXPathNode("./soundDevice", ctxt);
> +if (soundDeviceNode) {
> +def->playback = virXMLPropString(soundDeviceNode, "playback");
> +recording = virXMLPropString(soundDeviceNode, "recording");
> +if (recording)
> +def->recording = recording;
> +}
> +
> if (virDomainDeviceInfoParseXML(xmlopt, node, >info, flags) < 0)
> goto error;
>
> @@ -15056,6 +15069,9 @@ virDomainSoundDefEquals(const virDomainSoundDef *a,
> !virDomainDeviceInfoAddressIsEqual(>info, >info))
> return false;
>
> +if ((a->playback != b->playback) || (a->recording != b->recording))
> +return false;
> +
> return true;
> }
>
> @@ -27284,6 +27300,14 @@ virDomainSoundDefFormat(virBufferPtr buf,
> for (i = 0; i < def->ncodecs; i++)
> virDomainSoundCodecDefFormat(, def->codecs[i]);
>
> +if (def->playback) {
> +virBufferAsprintf(, " def->playback);
> +if (def->recording)
> +virBufferAsprintf(, " recording='%s'/>\n",
> def->recording);
> +else
> +virBufferAddLit(, "/>\n");
> +}
> +
> if (virDomainDeviceInfoFormat(, >info, flags) < 0)
> return -1;
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 241149af24..b501f48442 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1415,6 +1415,9 @@ struct _virDomainSoundDef {
>
> size_t ncodecs;
> virDomainSoundCodecDefPtr *codecs;
> +
> +char *playback;
> +char *recording;
> };
>
> typedef enum {
> --
> 2.27.0
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 08/10] qemu: implement driver's shutdown/shutdown wait methods
On Tue, Jul 14, 2020 at 12:32:59PM +0300, Nikolay Shirokovskiy wrote:
> On shutdown we just stop accepting new jobs for worker thread so that on
> shutdown wait we can exit worker thread faster. Yes we basically stop
> processing of events for VMs but we are going to do so anyway in case of
> daemon
> shutdown.
>
> At the same time synchronous event processing that some API calls may require
> are still possible as per VM event loop is still running and we don't need
> worker thread for synchronous event processing.
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_driver.c | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d185666..f7ff0fb 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1098,6 +1098,36 @@ qemuStateStop(void)
> return ret;
> }
>
> +
> +static int
> +qemuStateShutdown(void)
> +{
> +virThreadPoolStop(qemu_driver->workerPool);
> +return 0;
> +}
> +
> +
> +static int
> +qemuDomainObjStopWorkerIter(virDomainObjPtr vm,
> +void *opaque G_GNUC_UNUSED)
> +{
> +virObjectLock(vm);
> +qemuDomainObjStopWorker(vm);
My comment on the previous patch makes me slightly concerned about
this too but
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 10/10] util: remove unused virThreadPoolNew macro
On Tue, Jul 14, 2020 at 12:33:01PM +0300, Nikolay Shirokovskiy wrote: > Signed-off-by: Nikolay Shirokovskiy > --- > src/util/virthreadpool.h | 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 09/10] rpc: cleanup virNetDaemonClose method
On Tue, Jul 14, 2020 at 12:33:00PM +0300, Nikolay Shirokovskiy wrote: > Signed-off-by: Nikolay Shirokovskiy > --- > src/libvirt_remote.syms | 1 - > src/rpc/virnetdaemon.c | 13 - > src/rpc/virnetdaemon.h | 2 -- > 3 files changed, 16 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 07/10] qemu: exit thread synchronously in qemuDomainObjStopWorker
On Tue, Jul 14, 2020 at 12:32:58PM +0300, Nikolay Shirokovskiy wrote:
> The change won't hurt much current callers performance I guess and now we can
> use the function when we need to be sure of synchronous thread exit as well.
I can't remember the exact scenario, but I'm reasonably sure i tried
this approach when adding the event thread support and hit scenario
where this would deadlock.
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/qemu/qemu_domain.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2d9d822..18651d0 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1571,6 +1571,7 @@ qemuDomainObjStopWorker(virDomainObjPtr dom)
> qemuDomainObjPrivatePtr priv = dom->privateData;
>
> if (priv->eventThread) {
> +virEventThreadClose(priv->eventThread);
> g_object_unref(priv->eventThread);
IIRC, it was something like this unref does not always release the
last reference.
> priv->eventThread = NULL;
IOW, despite setting evnetThread to NULL, the thread may linger for
a short while longer in the background until any operations have
completed.
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 06/10] vireventthread: add virEventThreadClose
On Tue, Jul 14, 2020 at 12:32:57PM +0300, Nikolay Shirokovskiy wrote:
> virEventThreadClose is used when event loop thread should
> be exited synchronously (which is not the case when event loop
> is just unreferenced).
>
> Signed-off-by: Nikolay Shirokovskiy
> ---
> src/libvirt_private.syms | 1 +
> src/util/vireventthread.c | 9 +
> src/util/vireventthread.h | 1 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index f64b1de..c85ec43 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2023,6 +2023,7 @@ virEventGLibRunOnce;
>
>
> # util/vireventthread.h
> +virEventThreadClose;
> virEventThreadGetContext;
> virEventThreadNew;
>
> diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
> index cf86592..0a7c436 100644
> --- a/src/util/vireventthread.c
> +++ b/src/util/vireventthread.c
> @@ -183,6 +183,15 @@ virEventThreadNew(const char *name)
> }
>
>
> +void
> +virEventThreadClose(virEventThread *evt)
> +{
> +g_main_loop_quit(evt->loop);
> +g_thread_join(evt->thread);
> +evt->thread = NULL;
> +}
Lets invoke this from vir_event_thread_finalize too.
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 05/10] rpc: finish all threads before exiting main loop
On Tue, Jul 14, 2020 at 12:32:56PM +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
> ---
> src/remote/remote_daemon.c | 3 --
> src/rpc/virnetdaemon.c | 82
> +-
> src/rpc/virnetserver.c | 8 +
> src/rpc/virnetserver.h | 1 +
> 4 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 1aa9bfc..222bb5f 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -1201,9 +1201,6 @@ int main(int argc, char **argv) {
> 0, "shutdown", NULL, NULL);
>
> cleanup:
> -/* Keep cleanup order in inverse order of startup */
> -virNetDaemonClose(dmn);
> -
> virNetlinkEventServiceStopAll();
>
> if (driversInitialized) {
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index bb81a43..c4b31c6 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -23,6 +23,7 @@
> #include
> #include
>
> +#include "libvirt_internal.h"
> #include "virnetdaemon.h"
> #include "virlog.h"
> #include "viralloc.h"
> @@ -69,7 +70,10 @@ struct _virNetDaemon {
> virHashTablePtr servers;
> virJSONValuePtr srvObject;
>
> +int finishTimer;
> bool quit;
> +bool finished;
> +bool graceful;
>
> unsigned int autoShutdownTimeout;
> size_t autoShutdownInhibitions;
> @@ -80,6 +84,11 @@ struct _virNetDaemon {
>
> static virClassPtr virNetDaemonClass;
>
> +static int
> +daemonServerClose(void *payload,
> + const void *key G_GNUC_UNUSED,
> + void *opaque G_GNUC_UNUSED);
> +
> static void
> virNetDaemonDispose(void *obj)
> {
> @@ -796,11 +805,53 @@ daemonServerProcessClients(void *payload,
> return 0;
> }
>
> +static int
> +daemonServerShutdownWait(void *payload,
> + const void *key G_GNUC_UNUSED,
> + void *opaque G_GNUC_UNUSED)
> +{
> +virNetServerPtr srv = payload;
> +
> +virNetServerShutdownWait(srv);
> +return 0;
> +}
> +
> +static void
> +daemonShutdownWait(void *opaque)
> +{
> +virNetDaemonPtr dmn = opaque;
> +bool graceful = false;
> +
> +virHashForEach(dmn->servers, daemonServerShutdownWait, NULL);
> +if (virStateShutdownWait() < 0)
This code can't have any dependancy on virStateShutdownWait because
it is used in virtlockd and virtlogd, neither of which use the
virState infrastructure.
> +goto finish;
> +
> +graceful = true;
> +
> + finish:
> +virObjectLock(dmn);
> +dmn->graceful = graceful;
> +virEventUpdateTimeout(dmn->finishTimer, 0);
> +virObjectUnlock(dmn);
> +}
> +
> +static void
> +virNetDaemonFinishTimer(int timerid G_GNUC_UNUSED,
> +void *opaque)
> +{
> +virNetDaemonPtr dmn = opaque;
> +
> +virObjectLock(dmn);
> +dmn->finished = true;
> +virObjectUnlock(dmn);
> +}
> +
> void
> virNetDaemonRun(virNetDaemonPtr dmn)
> {
> int timerid = -1;
> bool timerActive = false;
> +virThread shutdownThread;
>
> virObjectLock(dmn);
>
> @@ -811,6 +862,9 @@ virNetDaemonRun(virNetDaemonPtr dmn)
> }
>
> dmn->quit = false;
> +dmn->finishTimer = -1;
> +dmn->finished = false;
> +dmn->graceful = false;
>
> if (dmn->autoShutdownTimeout &&
> (timerid = virEventAddTimeout(-1,
> @@ -826,7 +880,7 @@ virNetDaemonRun(virNetDaemonPtr dmn)
> virSystemdNotifyStartup();
>
> VIR_DEBUG("dmn=%p quit=%d", dmn, dmn->quit);
> -while (!dmn->quit) {
> +while (!dmn->finished) {
> /* A shutdown timeout is specified, so check
> * if any drivers have active state, if not
> * shutdown after timeout seconds
> @@ -857,6 +911,32 @@ virNetDaemonRun(virNetDaemonPtr dmn)
> virObjectLock(dmn);
>
> virHashForEach(dmn->servers, daemonServerProcessClients, NULL);
> +
> +if (dmn->quit && dmn->finishTimer == -1) {
> +virHashForEach(dmn->servers, daemonServerClose, NULL);
> +if (virStateShutdown() < 0)
> +break;
Again, we cna't have this direct call here.
Regards,
Re: [PATCH 04/10] rpc: don't unref service ref on socket behalf twice
On Tue, Jul 14, 2020 at 12:32:55PM +0300, Nikolay Shirokovskiy wrote:
> Second unref was added in [1]. We don't need it actually as
> we pass free callback to virNetSocketAddIOCallback thus
> when we call virNetSocketRemoveIOCallback the extra ref for
> callback will be dropped without extra efforts.
Oh, so this is actually fixing unref of free'd memory. Surprised we
don't crash in this already.
>
> [1] 355d8f470f9: virNetServerServiceClose: Don't leak sockets
> ---
> src/rpc/virnetserverservice.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Daniel P. Berrangé
> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
> index 9d5df45..e4165ea 100644
> --- a/src/rpc/virnetserverservice.c
> +++ b/src/rpc/virnetserverservice.c
> @@ -449,6 +449,5 @@ void virNetServerServiceClose(virNetServerServicePtr svc)
> for (i = 0; i < svc->nsocks; i++) {
> virNetSocketRemoveIOCallback(svc->socks[i]);
> virNetSocketClose(svc->socks[i]);
> -virObjectUnref(svc);
> }
> }
> --
> 1.8.3.1
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 03/10] util: add stop/drain functions to thread pool
On Tue, Jul 14, 2020 at 12:32:54PM +0300, Nikolay Shirokovskiy wrote: > Stop just send signal for threads to exit when they finish with > current task. Drain waits when all threads will finish. > > Signed-off-by: Nikolay Shirokovskiy > --- > src/libvirt_private.syms | 2 ++ > src/util/virthreadpool.c | 43 ++- > src/util/virthreadpool.h | 3 +++ > 3 files changed, 43 insertions(+), 5 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 01/10] libvirt: add stateShutdown/stateShutdownWait to drivers
On Tue, Jul 14, 2020 at 12:32:52PM +0300, Nikolay Shirokovskiy wrote: > stateShutdown is supposed to inform driver that it will be closed soon so that > the driver can prepare and finish all background threads quickly on > stateShutdownWait call. > > Signed-off-by: Nikolay Shirokovskiy > --- > scripts/check-drivername.py | 2 ++ > src/driver-state.h | 8 > src/libvirt.c | 42 ++ > src/libvirt_internal.h | 2 ++ > 4 files changed, 54 insertions(+) > > diff --git a/scripts/check-drivername.py b/scripts/check-drivername.py > index 39eff83..19d1cd1 100644 > --- a/scripts/check-drivername.py > +++ b/scripts/check-drivername.py > @@ -50,6 +50,8 @@ for drvfile in drvfiles: > "virDrvStateCleanup", > "virDrvStateReload", > "virDrvStateStop", > +"virDrvStateShutdown", > +"virDrvStateShutdownWait", > "virDrvConnectSupportsFeature", > "virDrvConnectURIProbe", > "virDrvDomainMigratePrepare", > diff --git a/src/driver-state.h b/src/driver-state.h > index 6b3f501..1f664f3 100644 > --- a/src/driver-state.h > +++ b/src/driver-state.h > @@ -45,6 +45,12 @@ typedef int > typedef int > (*virDrvStateStop)(void); > > +typedef int > +(*virDrvStateShutdown)(void); > + > +typedef int > +(*virDrvStateShutdownWait)(void); This is a bit of a bikeshedding comment, but I would have called them "StateShutdownPrepare" and "StateShutdownComplete" (or Wait) just to make it slightly clearer that the first method is just intended to kick off a shutdown asynchronously. The design overall looks fine though, so regardless of the name 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 02/10] util: always initialize priority condition
On Tue, Jul 14, 2020 at 12:32:53PM +0300, Nikolay Shirokovskiy wrote: > Even if we have no priority threads on pool creation we can add them thru > virThreadPoolSetParameters later. > > Signed-off-by: Nikolay Shirokovskiy > --- > src/util/virthreadpool.c | 22 +++--- > 1 file changed, 7 insertions(+), 15 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: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier
On Tue, Jul 21, 2020 at 02:20:01PM +0800, Jason Wang wrote: > > On 2020/7/20 下午9:03, Peter Xu wrote: > > On Mon, Jul 20, 2020 at 12:02:06PM +0800, Jason Wang wrote: > > > Right, so there's no need to deal with unmap in vtd's replay > > > implementation > > > (as what generic one did). > > We don't even for now; see vtd_page_walk_info.notify_unmap. Thanks, > > > Right, but I meant the vtd_address_space_unmap() in vtd_iomm_replay(). It > looks to me it will trigger UNMAP notifiers. Should be pretty safe for now, but I agree it seems optional (as we've discussed about this previously). Thanks, -- Peter Xu
Re: [libvirt PATCH] docs: virConnectGetCapabilities do not provide pool types
On Tue, Jul 21, 2020 at 08:46:55AM -0400, John Ferlan wrote: > > Upon further reflection and some memory jiggling... > > virsh -c storage:///system capabilities > > > > > dir > fs > netfs > logical > iscsi > iscsi-direct > scsi > mpath > disk > rbd > sheepdog > gluster > zfs > > > > > > But yeah, without the -c storage:///system one won't see the results > because 'npools == 0' when virCapabilitiesFormatStoragePoolXML is called > from virCapabilitiesFormatXML unless it's the storage driver URI. Err, don't you mean "virsh pool-capabilities". The "capabilities" command is exclusively for the virt driver usage, not storage. In the new modular daemon work, "capabilities" API will never be sent to the storage daemon. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On 7/21/20 8:04 AM, John Ferlan wrote:
On 7/7/20 5:08 PM, Laine Stump wrote:
All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 264 --
src/network/bridge_driver_linux.c | 15 +-
2 files changed, 113 insertions(+), 166 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
@@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
g_autofree char *field = NULL;
-int ret = -1;
bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
/* set disable_ipv6 if there are no ipv6 addresses defined for the
@@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (!enableIPv6)
VIR_DEBUG("ipv6 appears to already be disabled on %s",
def->bridge);
-ret = 0;
-goto cleanup;
+return 0;
}
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
Sigh.
I *thought* I had removed all the changes to this function when I
rebased the series the last time (since Jan already had a better patch
for it), but I guess I didn't look carefully enough at the diffs before
I pushed :-(
Fortunately, Jan has pushed his patch, which completely replaces the
function.
if (virFileWriteStr(field, enableIPv6 ? "0" : "1", 0) < 0) {
virReportSystemError(errno,
_("cannot write to %s to enable/disable IPv6 "
"on bridge %s"), field, def->bridge);
-goto cleanup;
+return -1;
}
/* The rest of the ipv6 sysctl tunables should always be set the
@@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (virFileWriteStr(field, "0", 0) < 0) {
virReportSystemError(errno,
_("cannot disable %s"), field);
-goto cleanup;
+return -1;
}
/* All interfaces used as a gateway (which is what this is, by
@@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (virFileWriteStr(field, "0", 0) < 0) {
virReportSystemError(errno,
_("cannot disable %s"), field);
-goto cleanup;
+return -1;
}
-ret = 0;
- cleanup:
-return ret;
+return 0;
}
[...]
Re: [PATCH] NEWS: mention readonly attribute is not yet supported by virtiofsd
On a Tuesday in 2020, Jianan Gao wrote: There was a clear statement on not supported by virtiofsd with readonly attribute. Signed-off-by: Jianan Gao --- NEWS.rst | 4 1 file changed, 4 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH] security: Remove the superfluous break
On a Friday in 2020, Yi Wang wrote: From: Liao Pingfang Remove the superfuous break, as there is a 'return' before it. Signed-off-by: Liao Pingfang Signed-off-by: Yi Wang --- src/security/security_apparmor.c | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Ján Tomko and pushed Jano signature.asc Description: PGP signature
Re: [libvirt PATCH] qemu: Drop ret variable from qemuConnectCPUModelComparison
On a Tuesday in 2020, Jiri Denemark wrote: Signed-off-by: Jiri Denemark --- src/qemu/qemu_driver.c | 17 + 1 file changed, 9 insertions(+), 8 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH] network: refactor networkSetIPv6Sysctls() for proper g_autofree usage
On a Friday in 2020, Laine Stump wrote: This function used the same char* three times for different purposes, freeing it after each use. Since we don't want to ever manually free an autofree'd pointer, modify it to use three separate char*, and make them all g_autofree. Signed-off-by: Laine Stump --- This was suggested by Jan in https://www.redhat.com/archives/libvir-list/2020-July/msg00805.html Oops, I should have been more verboser. Just a note for the mailing list that this was resolved by my: commit 5c50d1dda5664d480e6370111c0218947706bd31 network: split out networkSetIPv6Sysctl which removed the repetitive use by moving it to a separate function. Jano pushing this patch along with the patch 5 referenced there will permit pushing patch 06/15 of that series unmodified. src/network/bridge_driver.c | 32 1 file changed, 16 insertions(+), 16 deletions(-) signature.asc Description: PGP signature
Re: [PATCH] qemu: Remove superfluous breaks
On a Friday in 2020, Yi Wang wrote: From: Liao Pingfang Remove superfluous breaks, as there is a "return" before them. Signed-off-by: Liao Pingfang Signed-off-by: Yi Wang --- src/qemu/qemu_firmware.c | 1 - src/qemu/qemu_hostdev.c | 1 - 2 files changed, 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[libvirt PATCH] qemu: Drop ret variable from qemuConnectCPUModelComparison
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_driver.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b8ba2e3fb9..8e81c30a93 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13150,7 +13150,6 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
{
g_autoptr(qemuProcessQMP) proc = NULL;
g_autofree char *result = NULL;
-int ret = VIR_CPU_COMPARE_ERROR;
if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps),
libDir, runUid, runGid, false)))
@@ -13163,15 +13162,17 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
return VIR_CPU_COMPARE_ERROR;
if (STREQ(result, "identical"))
-ret = VIR_CPU_COMPARE_IDENTICAL;
-else if (STREQ(result, "superset"))
-ret = VIR_CPU_COMPARE_SUPERSET;
-else if (failIncompatible)
+return VIR_CPU_COMPARE_IDENTICAL;
+
+if (STREQ(result, "superset"))
+return VIR_CPU_COMPARE_SUPERSET;
+
+if (failIncompatible) {
virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL);
-else
-ret = VIR_CPU_COMPARE_INCOMPATIBLE;
+return VIR_CPU_COMPARE_ERROR;
+}
-return ret;
+return VIR_CPU_COMPARE_INCOMPATIBLE;
}
--
2.27.0
RE: [PATCH v2 2/2] qemu: allow configuring mem-path for ivshmem-plain device
> On Fri, Jul 17, 2020 at 09:04:51PM +0800, Wang Xin wrote: > >The shared memory path is generated by shmem name as default, > >however, we may need to change it to avoid filename conflict > >when VM migrate to other host. > > > > At which point there is no need for the name at all. I agree that having the > 'name' was an unfortunate decision, but adding more attributes does not seem > like a proper fix. If it needs to be changed then we should allow changing > the > name in the process. You can also unplug the old name and plug in the new > one, > just like you'd have to do with role='peer'. Did I miss any other reason for > this? e.g. Ivshmem config: 4 qemu args: -object memory-backend-file,id=shmmem-shmem0,mem-path=/dev/shm/abcdefg,size=4194304,share=yes -device ivshmem-plain,id=shmem0,memdev=shmmem-shmem0,bus=pci.0,addr=0x3 The shmem 'name' usually mean the ivshmem device name/id, I think we shouldn't change it in migration. However, here we use the auto generated 'alias name' as device name/id instead. Add a new optional attribute for easier understanding what we changed, no other reason. Update the shmem 'name' will also work, I can support it in V3.
Re: [PATCH] conf: Remove superfluous breaks
On a Friday in 2020, Yi Wang wrote: From: Liao Pingfang Remove superfluous breaks, as there is a "return" before them. Signed-off-by: Liao Pingfang Signed-off-by: Yi Wang --- src/conf/domain_conf.c | 6 -- src/conf/network_conf.c| 12 src/conf/nwfilter_params.c | 4 3 files changed, 22 deletions(-) Reviewed-by: Ján Tomko and pushed Jano signature.asc Description: PGP signature
Re: [PATCH] qemu: pre-create the dbus directory in qemuStateInitialize
On a Monday in 2020, Bihong Yu wrote:
From 187323ce736dcd9b1a177d552630b0c6859a4798 Mon Sep 17 00:00:00 2001
From: Bihong Yu
Date: Tue, 14 Jul 2020 15:44:05 +0800
Subject: [PATCH] qemu: pre-create the dbus directory in qemuStateInitialize
There are races condiction to make '/run/libvirt/qemu/dbus' directory in
virDirCreateNoFork() while concurrent start VMs, and get "failed to create
directory '/run/libvirt/qemu/dbus': File exists" error message. pre-create the
dbus directory in qemuStateInitialize.
Signed-off-by:Bihong Yu
---
src/qemu/qemu_dbus.c| 4 +---
src/qemu/qemu_dbus.h| 2 +-
src/qemu/qemu_driver.c | 4
src/qemu/qemu_process.c | 3 ---
4 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
index 51f6c94..0e0306a 100644
--- a/src/qemu/qemu_dbus.c
+++ b/src/qemu/qemu_dbus.c
@@ -34,10 +34,8 @@ VIR_LOG_INIT("qemu.dbus");
int
-qemuDBusPrepareHost(virQEMUDriverPtr driver)
+qemuDBusPreparePath(virQEMUDriverConfigPtr cfg)
Instead of renaming this function, we can just remove it completely
{
-g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-
return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
VIR_DIR_CREATE_ALLOW_EXIST);
This virDirCreate call would then fit nicely after
virFileMakePath(cfg->slirpStateDir),
which is where all the other directories are created.
}
Jano
signature.asc
Description: PGP signature
[PATCH] NEWS: mention readonly attribute is not yet supported by virtiofsd
There was a clear statement on not supported by virtiofsd with readonly attribute. Signed-off-by: Jianan Gao --- NEWS.rst | 4 1 file changed, 4 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 2c6c628c61..ff977968c7 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -170,6 +170,10 @@ v6.4.0 (2020-06-02) firewalld resets all iptables rules and chains on restart, and this includes deleting those created by libvirt. + * qemu: reject readonly attribute for virtiofs + +virtiofs does not yet support read-only shares. + v6.3.0 (2020-05-05) === -- 2.21.3
Re: [PATCH 4/5] qemu_driver.c: modernize qemuConnectCPUModelComparison()
On Fri, Jul 17, 2020 at 18:15:55 -0300, Daniel Henrique Barboza wrote: > Use g_auto* on pointers to avoid using the 'cleanup' label. > > In theory the 'ret' variable can also be discarded if the flow > of the logic is reworked. Perhaps another time. Doing so would actually be pretty easy, mostly ret = ...; would need to be replaced with direct return. I'll send a trivial patch doing this. Jirka
Re: [PATCH 0/5] trivial qemuProcessQMPPtr related cleanups
On Fri, Jul 17, 2020 at 18:15:51 -0300, Daniel Henrique Barboza wrote: > Hi, > > These are some simple cleanups related to the qemuProcessQMPPtr > pointer I ended up making while trying to understand how and when > qemuProcessQMPFree() is called. > > Daniel Henrique Barboza (5): > qemu_process.h: register AUTOPTR_CLEANUP_FUNC for qemuProcessQMPPtr > qemu_process.c: modernize qemuProcessQMPNew() > qemu_driver.c: modernize qemuConnectCPUModelBaseline() > qemu_driver.c: modernize qemuConnectCPUModelComparison() > qemu_capabilities.c: use g_autoptr() in virQEMUCapsInitQMPSingle() > > src/qemu/qemu_capabilities.c | 3 +-- > src/qemu/qemu_driver.c | 41 ++-- > src/qemu/qemu_process.c | 13 > src/qemu/qemu_process.h | 1 + > 4 files changed, 22 insertions(+), 36 deletions(-) Reviewed-by: Jiri Denemark And pushed.
Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On a Tuesday in 2020, John Ferlan wrote:
On 7/7/20 5:08 PM, Laine Stump wrote:
All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.
Signed-off-by: Laine Stump
---
src/network/bridge_driver.c | 264 --
src/network/bridge_driver_linux.c | 15 +-
2 files changed, 113 insertions(+), 166 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
@@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
{
virNetworkDefPtr def = virNetworkObjGetDef(obj);
g_autofree char *field = NULL;
-int ret = -1;
bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
/* set disable_ipv6 if there are no ipv6 addresses defined for the
@@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
if (!enableIPv6)
VIR_DEBUG("ipv6 appears to already be disabled on %s",
def->bridge);
-ret = 0;
-goto cleanup;
+return 0;
}
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
Should be fixed in git master by:
commit 5c50d1dda5664d480e6370111c0218947706bd31
Author: Ján Tomko
CommitDate: 2020-07-21 14:55:00 +0200
network: split out networkSetIPv6Sysctl
https://gitlab.com/libvirt/libvirt/-/commit/5c50d1dda5664d480e6370111c0218947706bd31
Jano
signature.asc
Description: PGP signature
Re: [libvirt PATCH] docs: virConnectGetCapabilities do not provide pool types
Upon further reflection and some memory jiggling... virsh -c storage:///system capabilities dir fs netfs logical iscsi iscsi-direct scsi mpath disk rbd sheepdog gluster zfs But yeah, without the -c storage:///system one won't see the results because 'npools == 0' when virCapabilitiesFormatStoragePoolXML is called from virCapabilitiesFormatXML unless it's the storage driver URI. John On 7/20/20 1:31 PM, John Ferlan wrote: > > > On 7/20/20 3:50 AM, Pino Toscano wrote: >> Remove the paragraph in the storage pool page that mentions >> virConnectGetCapabilities, as virConnectGetCapabilities does not return >> any information about pools. >> >> Signed-off-by: Pino Toscano >> --- >> docs/formatstoragecaps.html.in | 6 -- >> 1 file changed, 6 deletions(-) >> > > I was asked by Pino to review to be "ok" with removal, so yeah: > > Reviewed-by: John Ferlan > > John > > If anyone cares to dredge up history back in Jan/Feb 2019 I believe it > was a true statement in at least a patches I had locally, but reviews, > conversations, and other adjustments to the code changed that. Been too > long to recall all those details. > >> diff --git a/docs/formatstoragecaps.html.in b/docs/formatstoragecaps.html.in >> index ee3888f44d..d8a1cacd96 100644 >> --- a/docs/formatstoragecaps.html.in >> +++ b/docs/formatstoragecaps.html.in >> @@ -13,12 +13,6 @@ >> supported, and if relevant the source format types, the required >> source elements, and the target volume format types. >> >> -The Storage Pool Capabilities XML provides more information than the >> - >> -virConnectGetCapabilities >> - >> -which only provides an enumerated list of supported pool types. >> - >> Element and attribute overview >> >> A query interface was added to the virConnect API's to retrieve the >> >
Plans for the next release
Hi all, We are getting close to the next release of libvirt. Because I am on vacation next week, I think we will have a little bit longer freeze period (although mainly extended by a weekend). So to aim for the release at the beginning of August I suggest entering the freeze just before the upcoming weekend starts, i.e., on Friday July 24 (in late evening) and I think we can skip RC2 this time and just push the release on August 1 or 2. Normal freeze period would start on Monday with RC2 on Thursday and a final release on August 1, which is pretty similar in terms of frozen work days. I hope this works for everyone. Jirka
Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels
On 7/7/20 5:08 PM, Laine Stump wrote:
> All these cleanup/error labels were reduced to having just "return
> ret" by a previous patch, so get rid of them and return directly.
>
> Signed-off-by: Laine Stump
> ---
> src/network/bridge_driver.c | 264 --
> src/network/bridge_driver_linux.c | 15 +-
> 2 files changed, 113 insertions(+), 166 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 31bd0dd92c..79b2ca3330 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
[...]
Coverity noted there's a leak with this part of the change for @field...
>
> @@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> {
> virNetworkDefPtr def = virNetworkObjGetDef(obj);
> g_autofree char *field = NULL;
> -int ret = -1;
> bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>
> /* set disable_ipv6 if there are no ipv6 addresses defined for the
> @@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (!enableIPv6)
> VIR_DEBUG("ipv6 appears to already be disabled on %s",
>def->bridge);
> -ret = 0;
> -goto cleanup;
> +return 0;
> }
Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.
>
> if (virFileWriteStr(field, enableIPv6 ? "0" : "1", 0) < 0) {
> virReportSystemError(errno,
> _("cannot write to %s to enable/disable IPv6 "
> "on bridge %s"), field, def->bridge);
> -goto cleanup;
> +return -1;
> }
>
> /* The rest of the ipv6 sysctl tunables should always be set the
> @@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (virFileWriteStr(field, "0", 0) < 0) {
> virReportSystemError(errno,
> _("cannot disable %s"), field);
> -goto cleanup;
> +return -1;
> }
>
> /* All interfaces used as a gateway (which is what this is, by
> @@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
> if (virFileWriteStr(field, "0", 0) < 0) {
> virReportSystemError(errno,
> _("cannot disable %s"), field);
> -goto cleanup;
> +return -1;
> }
>
> -ret = 0;
> - cleanup:
> -return ret;
> +return 0;
> }
[...]
RE: [PATCH v2 1/2] qemu: add support for shmem-{plain, doorbell} role
> On Fri, Jul 17, 2020 at 09:04:50PM +0800, Wang Xin wrote:
> >Role(master or peer) controls how the domain behaves on migration.
> >For more details about migration with ivshmem, see
> >https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=HEAD
> >
> >It's a optional attribute in libvirt, and qemu will choose default
> >role for ivshmem device if the user is not specified.
> >
> >With device property 'role', the value can be 'master' or 'peer'.
> > - 'master' (means 'master=on' in qemu), the guest will copy
> > the shared memory on migration to the destination host.
> > - 'peer' (means 'master=off' in qemu), the migration is disabled.
> >
> >Signed-off-by: Martin Kletzander
>
>
> I do not remember signing off this patch. Is this a re-spin of some old
> series?
It's about 4 years ago,
https://www.redhat.com/archives/libvir-list/2016-September/msg00536.html
And as you mentioned in msg(see link), you may had a plan to do it, but I
didn't find the patch.
https://www.redhat.com/archives/libvir-list/2016-August/msg00591.html
So, I do some tests and re-send your first patch.
>
> I see with this v2 you fixed the `make check`. We tend to use the cover
> letters
> for that, it's nicely recognisable what are the changes there. However make
> syntax-check still fails after this patch, even though that's just a minor
> whitespace change that can be done automatically. Not only make syntax-check
> outputs the diff for you (which you can apply), but it also suggests the
> script
> we have there that does all of that for you.
Sorry for missing the check, I will fix it in V3, and list the changes in cover
letter.
>
> >Signed-off-by: Yang Hang
> >Signed-off-by: Wang Xin
> >
> >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >index f5ee97de81..d4d90ad44d 100644
> >--- a/docs/formatdomain.html.in
> >+++ b/docs/formatdomain.html.in
> >@@ -9141,7 +9141,7 @@ qemu-kvm -net nic,model=? /dev/null
> >
> > ...
> > devices
> >- shmem name='my_shmem0'
> >+ shmem name='my_shmem0' role='peer'
> > model type='ivshmem-plain'/
> > size unit='M'4/size
> > /shmem
> >@@ -9159,9 +9159,15 @@ qemu-kvm -net nic,model=? /dev/null
> > shmem
> >
> > The shmem element has one mandatory attribute,
> >- name to identify the shared memory. This attribute cannot
> >- be directory specific to . or .. as well as
> >- it cannot involve path separator /.
> >+ name to identify the shared memory. Optional attribute
> >+ role can be set to values "master" or "peer". The former
> >+ will mean that upon migration, the data in the shared memory is
> >migrated
> >+ with the domain. There should be only one "master" per shared memory
> >object.
> >+ Migration with "peer" role is disabled. If migration of such domain
> >is required,
> >+ the shmem device needs to be unplugged before migration and plugged
> >in at the
> >+ destination upon successful migration. This attribute cannot be
> >directory
> >+ specific to . or .. as well as it cannot
> >involve path
> >+ separator /.
>
> You wrote your text in the middle of the explanation of another attribute.
> Now
> it looks like the optional attribute "role" can be set to values "master" or
> "peer", cannot be a directory specific to "." or ".." and it cannot use a path
> separator "/". It's a bit confusing, even though it is still true =)
>
> You should also describe what leaving out that attribute does. We usually
> leave
> it to the hypervisor to decide although this might be enough of a difference
> that we might fill in the default ourselves. But I'll leave the decision up
> to
> you.
Good suggestion, accept.
>
> >
> > model
> >
> >diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> >index a810f569c6..09d4ad3e96 100644
> >--- a/docs/schemas/domaincommon.rng
> >+++ b/docs/schemas/domaincommon.rng
> >@@ -4417,6 +4417,14 @@
> > [^/]*
> >
> >
> >+
> >+
> >+
> >+master
> >+peer
> >+
> >+
> >+
> >
> >
> >
> >diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >index 7ecd2818b9..6e67c7ebe0 100644
> >--- a/src/conf/domain_conf.c
> >+++ b/src/conf/domain_conf.c
> >@@ -1325,6 +1325,13 @@ VIR_ENUM_IMPL(virDomainShmemModel,
> > "ivshmem-doorbell",
> > );
> >
> >+VIR_ENUM_IMPL(virDomainShmemRole,
> >+ VIR_DOMAIN_SHMEM_ROLE_LAST,
> >+ "default",
> >+ "master",
> >+ "peer",
> >+);
> >+
> > VIR_ENUM_IMPL(virDomainLaunchSecurity,
> > VIR_DOMAIN_LAUNCH_SECURITY_LAST,
> > "",
> >@@ -15357,6 +15364,19 @@ virDomainShmemDefParseXML(virDomainXMLOptionPtr
> >xmlopt,
> > goto cleanup;
> > }
> >
> >+if (def->model != VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) {
> >+tmp = virXMLPropString(node,
[PATCH 2/2] tests: commandtest: Make 'test4' checking daemonization more reliable
The 'commandhelper' checks effectively whether the parent process is
still around to report whether it was daemonized or not.
This creates a unlikely race condition in cases when we do actually
daemonize the process as the intermediate process used for the
daemonization might not have terminated yet which would report wrong
result leading to test failure.
For now there's just 'test4' which actually daemonizes the process.
Add an argument '--check-daemonize' which asks for retires of the
daemonization check in cases where we expect that the commandhelper is
going to be daemonized and use it in 'test4' to make the test more
reliable.
I've observed the test failure sporradically when my box is more loaded
e.g. while building two trees at once.
Signed-off-by: Peter Krempa
---
tests/commanddata/test4.log | 1 +
tests/commandhelper.c | 17 -
tests/commandtest.c | 3 ++-
3 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log
index 24a37a7e96..4fd40cc474 100644
--- a/tests/commanddata/test4.log
+++ b/tests/commanddata/test4.log
@@ -1,3 +1,4 @@
+ARG:--check-daemonize
ENV:DISPLAY=:0.0
ENV:HOME=/home/test
ENV:HOSTNAME=test
diff --git a/tests/commandhelper.c b/tests/commandhelper.c
index 4266e11902..b366483771 100644
--- a/tests/commandhelper.c
+++ b/tests/commandhelper.c
@@ -72,6 +72,8 @@ int main(int argc, char **argv) {
char *buffers[3] = {NULL, NULL, NULL};
size_t buflen[3] = {0, 0, 0};
char c;
+bool daemonize_check = false;
+size_t daemonize_retries = 3;
if (!log)
return ret;
@@ -83,6 +85,8 @@ int main(int argc, char **argv) {
sscanf(argv[i], "%u%c", [numreadfds++], ) != 1) {
printf("Could not parse fd %s\n", argv[i]);
goto cleanup;
+} else if (STREQ(argv[i], "--check-daemonize")) {
+daemonize_check = true;
}
}
@@ -126,7 +130,18 @@ int main(int argc, char **argv) {
fprintf(log, "FD:%zu\n", i);
}
-fprintf(log, "DAEMON:%s\n", getpgrp() != getppid() ? "yes" : "no");
+while (true) {
+bool daemonized = getpgrp() != getppid();
+
+if (daemonize_check && !daemonized && daemonize_retries-- > 0) {
+usleep(100*1000);
+continue;
+}
+
+fprintf(log, "DAEMON:%s\n", daemonized ? "yes" : "no");
+break;
+}
+
if (!(cwd = getcwd(NULL, 0)))
goto cleanup;
if (strlen(cwd) > strlen(".../commanddata") &&
diff --git a/tests/commandtest.c b/tests/commandtest.c
index f0e60ee5fe..9bef825239 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -255,7 +255,8 @@ static int test3(const void *unused G_GNUC_UNUSED)
*/
static int test4(const void *unused G_GNUC_UNUSED)
{
-virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
+virCommandPtr cmd = virCommandNewArgList(abs_builddir "/commandhelper",
+ "--check-daemonize", NULL);
char *pidfile = virPidFileBuildPath(abs_builddir, "commandhelper");
pid_t pid;
int ret = -1;
--
2.26.2
[PATCH 1/2] vircommand: REPRODUCER: Add artificial timeout before exitting daemonization helper
Let the intermediate process linger for a moment to reproduce the
sporradic failure in 'commandtest'.
---
src/util/vircommand.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 5ce69ef241..f6da8b5cf1 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -783,6 +783,7 @@ virExec(virCommandPtr cmd)
_("Unable to wait for child process"));
_exit(EXIT_FAILURE);
}
+g_usleep(100*1000);
_exit(EXIT_SUCCESS);
}
}
--
2.26.2
[PATCH 0/2] commandtest: Make 'test4' more reliable
See 2/2 for the explanation. Peter Krempa (2): vircommand: REPRODUCER: Add artificial timeout before exitting daemonization helper tests: commandtest: Make 'test4' checking daemonization more reliable src/util/vircommand.c | 1 + tests/commanddata/test4.log | 1 + tests/commandhelper.c | 17 - tests/commandtest.c | 3 ++- 4 files changed, 20 insertions(+), 2 deletions(-) -- 2.26.2
Re: [PATCH v2 2/2] qemu: allow configuring mem-path for ivshmem-plain device
On Fri, Jul 17, 2020 at 09:04:51PM +0800, Wang Xin wrote: The shared memory path is generated by shmem name as default, however, we may need to change it to avoid filename conflict when VM migrate to other host. At which point there is no need for the name at all. I agree that having the 'name' was an unfortunate decision, but adding more attributes does not seem like a proper fix. If it needs to be changed then we should allow changing the name in the process. You can also unplug the old name and plug in the new one, just like you'd have to do with role='peer'. Did I miss any other reason for this? signature.asc Description: PGP signature
Re: [PATCH v2 1/2] qemu: add support for shmem-{plain, doorbell} role
On Fri, Jul 17, 2020 at 09:04:50PM +0800, Wang Xin wrote:
Role(master or peer) controls how the domain behaves on migration.
For more details about migration with ivshmem, see
https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=HEAD
It's a optional attribute in libvirt, and qemu will choose default
role for ivshmem device if the user is not specified.
With device property 'role', the value can be 'master' or 'peer'.
- 'master' (means 'master=on' in qemu), the guest will copy
the shared memory on migration to the destination host.
- 'peer' (means 'master=off' in qemu), the migration is disabled.
Signed-off-by: Martin Kletzander
I do not remember signing off this patch. Is this a re-spin of some old series?
I see with this v2 you fixed the `make check`. We tend to use the cover letters
for that, it's nicely recognisable what are the changes there. However make
syntax-check still fails after this patch, even though that's just a minor
whitespace change that can be done automatically. Not only make syntax-check
outputs the diff for you (which you can apply), but it also suggests the script
we have there that does all of that for you.
Signed-off-by: Yang Hang
Signed-off-by: Wang Xin
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f5ee97de81..d4d90ad44d 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -9141,7 +9141,7 @@ qemu-kvm -net nic,model=? /dev/null
...
devices
- shmem name='my_shmem0'
+ shmem name='my_shmem0' role='peer'
model type='ivshmem-plain'/
size unit='M'4/size
/shmem
@@ -9159,9 +9159,15 @@ qemu-kvm -net nic,model=? /dev/null
shmem
The shmem element has one mandatory attribute,
- name to identify the shared memory. This attribute cannot
- be directory specific to . or .. as well as
- it cannot involve path separator /.
+ name to identify the shared memory. Optional attribute
+ role can be set to values "master" or "peer". The former
+ will mean that upon migration, the data in the shared memory is migrated
+ with the domain. There should be only one "master" per shared memory
object.
+ Migration with "peer" role is disabled. If migration of such domain is
required,
+ the shmem device needs to be unplugged before migration and plugged in
at the
+ destination upon successful migration. This attribute cannot be directory
+ specific to . or .. as well as it cannot
involve path
+ separator /.
You wrote your text in the middle of the explanation of another attribute. Now
it looks like the optional attribute "role" can be set to values "master" or
"peer", cannot be a directory specific to "." or ".." and it cannot use a path
separator "/". It's a bit confusing, even though it is still true =)
You should also describe what leaving out that attribute does. We usually leave
it to the hypervisor to decide although this might be enough of a difference
that we might fill in the default ourselves. But I'll leave the decision up to
you.
model
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index a810f569c6..09d4ad3e96 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4417,6 +4417,14 @@
[^/]*
+
+
+
+master
+peer
+
+
+
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7ecd2818b9..6e67c7ebe0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1325,6 +1325,13 @@ VIR_ENUM_IMPL(virDomainShmemModel,
"ivshmem-doorbell",
);
+VIR_ENUM_IMPL(virDomainShmemRole,
+ VIR_DOMAIN_SHMEM_ROLE_LAST,
+ "default",
+ "master",
+ "peer",
+);
+
VIR_ENUM_IMPL(virDomainLaunchSecurity,
VIR_DOMAIN_LAUNCH_SECURITY_LAST,
"",
@@ -15357,6 +15364,19 @@ virDomainShmemDefParseXML(virDomainXMLOptionPtr xmlopt,
goto cleanup;
}
+if (def->model != VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) {
+tmp = virXMLPropString(node, "role");
+if (tmp) {
+if ((def->role = virDomainShmemRoleTypeFromString(tmp)) <= 0) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("Unknown shmem role type '%s'"), tmp);
+goto cleanup;
+}
+
+VIR_FREE(tmp);
+}
+}
+
if (virParseScaledValue("./size[1]", NULL, ctxt,
>size, 1, ULLONG_MAX, false) < 0)
goto cleanup;
@@ -18579,6 +18599,9 @@ virDomainShmemDefEquals(virDomainShmemDefPtr src,
if (src->model != dst->model)
return false;
+if (src->role != dst->role)
+return false;
+
if (src->server.enabled != dst->server.enabled)
return false;
@@ -23758,6 +23781,15 @@
virDomainShmemDefCheckABIStability(virDomainShmemDefPtr src,
[PATCH] mdev: fix daemon crash on reattach mdevs
Causing a crash when virMediatedDeviceListFindIndex because of some pointers in mgr->activeMediatedHostdevs become dangling pointers if goto cleanup label in virMediatedDeviceListMarkDevices. Reproduction scenario: 1. start vm1 with mdev1 2. start vm2 with mdev2, mdev1 (the order cannot be changed) Backtrace: #0 0xb8c36250 in strcmp #1 0xb9b80754 in virMediatedDeviceListFindIndex #2 0xb9b80870 in virMediatedDeviceListFind #3 0xb9c9e168 in virHostdevReAttachMediatedDevices #4 0x9949f724 in qemuHostdevReAttachMediatedDevices #5 0x9949f7f8 in qemuHostdevReAttachDomainDevices #6 0x994bcd70 in qemuProcessStop #7 0x994bf4e0 in qemuProcessStart . Signed-off-by: Binfeng Wu --- src/util/virmdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index b8023dd991..26cb8300ff 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -439,7 +439,7 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, if (virMediatedDeviceIsUsed(mdev, dst) || virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) -goto cleanup; +goto rollback; /* Copy mdev references to the driver list: * - caller is responsible for NOT freeing devices in @src on success -- 2.26.2.windows.1
Re: [libvirt PATCH 9/9] rpc: use new virt-nc binary for remote tunnelling
On Mon, 2020-07-20 at 19:36 +0100, Daniel P. Berrangé wrote: > On Mon, Jul 20, 2020 at 08:20:12PM +0200, Andrea Bolognani wrote: > > We could special-case binaries called 'virt-nc' and use our internal > > syntax for those. Having two separate URI parameters to control the > > same knob sounds like trouble, especially since you can mix and > > match: if you try to connect to > > > > qemu+ssh://host/system?proxy=virt-nc=my-cool-nc > > > > for example, what happens? As far as I can tell virt-nc will be used, > > but it's certainly not as obvious as it would be if everything was > > controlled by a single URI parameter. > > No, I really don't want to do magic based on the name of the binary. > That is a recipe for long term pain. It never turns out well when we > try to overload two distinct concepts onto a single tunable. > > That URL you illustrate should be reported as an error since it > is using mutually exclusive args. Adding validation sounds good. We should also cross-reference the two parameters in the documentation, to make sure users changing one of them is aware of the other existing as well. > > Actually, and I might be very well missing something because I looked > > at the code rather quickly, from what I can tell the default value > > for proxy will cause libvirt to always prefer virt-nc when available, > > which means that the URI > > > > qemu+ssh://host/system?netcat=my-cool-nc > > > > will suddenly stop using my-cool-nc and start using virt-nc after > > libvirt has been upgraded - a breaking change. > > It will only stop using my-cool-nc if you have upgraded the remote > host to have virt-nc installed, and your local host also has the > libvirt supporting virt-nc. I'd consider that desirable, as netcat > is redundant once both sides are upgraded. If the user is explicitly asking for a specific netcat binary to be used, then we need to comply with that request, even if we think that virt-nc would be better. Doing otherwise has the potential to break the user's setup. Basically, when the netcat parameter is specified we should behave as if proxy=netcat had been specified as well. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH 3/5] docs: add missing balloon stats docs in domstats
On 21.07.2020 11:07, Nikolay Shirokovskiy wrote: > Signed-off-by: Nikolay Shirokovskiy > --- > docs/manpages/virsh.rst | 4 > 1 file changed, 4 insertions(+) > > diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst > index 6a1ae40..d9caef1 100644 > --- a/docs/manpages/virsh.rst > +++ b/docs/manpages/virsh.rst > @@ -2265,6 +2265,10 @@ When selecting the *--state* group the following > fields are returned: >(in seconds) > * ``balloon.disk_caches`` - the amount of memory that can be reclaimed >without additional I/O, typically disk (in KiB) > +* ``hugetlb_pgalloc`` - the number of successful huge page allocations > + from inside the domain via virtio balloon > +* ``hugetlb_pgfail`` - the number of failed huge page allocations > + from inside the domain via virtio balloon In case someone will decide to push it - I missed balloon prefix here. Nikolay > > > *--vcpu* returns: >
[PATCH 5/5] src: add missing balloon stats docs
Signed-off-by: Nikolay Shirokovskiy --- src/libvirt-domain.c | 28 1 file changed, 28 insertions(+) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index fe4dab7..36fa8a2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11455,6 +11455,34 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * as unsigned long long. * "balloon.maximum" - the maximum memory in kiB allowed * as unsigned long long. + * "balloon.swap_in" - the amount of data read from swap space (in KiB) + * as unsigned long long + * "balloon.swap_out" - the amount of memory written out to swap space + * (in KiB) as unsigned long long + * "balloon.major_fault" - the number of page faults when disk IO was + * required as unsigned long long + * "balloon.minor_fault" - the number of other page faults + * as unsigned long long + * "balloon.unused" - the amount of memory left unused by the system + *(in KiB) as unsigned long long + * "balloon.available" - the amount of usable memory as seen by the domain + * (in KiB) as unsigned long long + * "balloon.rss" - Resident Set Size of running domain's process + * (in KiB) as unsigned long long + * "balloon.usable" - the amount of memory which can be reclaimed by balloon + *without causing host swapping (in KiB) + *as unsigned long long + * "balloon.last-update" - timestamp of the last update of statistics + * (in seconds) as unsigned long long + * "balloon.disk_caches" - the amount of memory that can be reclaimed + * without additional I/O, typically disk (in KiB) + * as unsigned long long + * "hugetlb_pgalloc" - the number of successful huge page allocations + * from inside the domain via virtio balloon + * as unsigned long long + * "hugetlb_pgfail" - the number of failed huge page allocations + *from inside the domain via virtio balloon + *as unsigned long long * * VIR_DOMAIN_STATS_VCPU: * Return virtual CPU statistics. -- 1.8.3.1
[PATCH 3/5] docs: add missing balloon stats docs in domstats
Signed-off-by: Nikolay Shirokovskiy --- docs/manpages/virsh.rst | 4 1 file changed, 4 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 6a1ae40..d9caef1 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2265,6 +2265,10 @@ When selecting the *--state* group the following fields are returned: (in seconds) * ``balloon.disk_caches`` - the amount of memory that can be reclaimed without additional I/O, typically disk (in KiB) +* ``hugetlb_pgalloc`` - the number of successful huge page allocations + from inside the domain via virtio balloon +* ``hugetlb_pgfail`` - the number of failed huge page allocations + from inside the domain via virtio balloon *--vcpu* returns: -- 1.8.3.1
[PATCH 1/5] docs: fix typo in virsh.rst for balloon.major_fault
Signed-off-by: Nikolay Shirokovskiy --- docs/manpages/virsh.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 1a2cf09..6a1ae40 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2250,7 +2250,7 @@ When selecting the *--state* group the following fields are returned: * ``balloon.swap_in`` - the amount of data read from swap space (in KiB) * ``balloon.swap_out`` - the amount of memory written out to swap space (in KiB) -* ``balloon.major_fault`` - the number of page faults then disk IO +* ``balloon.major_fault`` - the number of page faults when disk IO was required * ``balloon.minor_fault`` - the number of other page faults * ``balloon.unused`` - the amount of memory left unused by the -- 1.8.3.1
[PATCH 4/5] docs: add missing iothread stats docs in domstats
Signed-off-by: Nikolay Shirokovskiy --- docs/manpages/virsh.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index d9caef1..ed7cefc 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2374,6 +2374,11 @@ too high will consume too much CPU time per IOThread failing to allow other threads running on the CPU to get time. The polling interval is not available for statistical purposes. +* ``iothread.count`` - maximum number of IOThreads in the subsequent list + as unsigned int. Each IOThread in the list will + will use it's iothread_id value as the . There + may be fewer entries than the iothread.count + value if the polling values are not supported. * ``iothread..poll-max-ns`` - maximum polling time in nanoseconds used by the IOThread. A value of 0 (zero) indicates polling is disabled. * ``iothread..poll-grow`` - polling time grow value. A value of 0 (zero) -- 1.8.3.1
[PATCH 2/5] include: clarify docs for hugetlb in virDomainMemoryStatTags
The term number is used for other stats and even for hugetlb
stats in virsh man page. The term number is also more clear.
Signed-off-by: Nikolay Shirokovskiy
---
include/libvirt/libvirt-domain.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index f129e6a..8b9d9c1 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -637,13 +637,13 @@ typedef enum {
VIR_DOMAIN_MEMORY_STAT_DISK_CACHES = 10,
/*
- * The amount of successful huge page allocations from inside the domain
via
+ * The number of successful huge page allocations from inside the domain
via
* virtio balloon.
*/
VIR_DOMAIN_MEMORY_STAT_HUGETLB_PGALLOC= 11,
/*
- * The amount of failed huge page allocations from inside the domain via
+ * The number of failed huge page allocations from inside the domain via
* virtio balloon.
*/
VIR_DOMAIN_MEMORY_STAT_HUGETLB_PGFAIL= 12,
--
1.8.3.1
[PATCH 0/5] docs: misc docs enhancements for statistic API
Nikolay Shirokovskiy (5): docs: fix typo in virsh.rst for balloon.major_fault include: clarify docs for hugetlb in virDomainMemoryStatTags docs: add missing balloon stats docs in domstats docs: add missing iothread stats docs in domstats src: add missing balloon stats docs docs/manpages/virsh.rst | 11 ++- include/libvirt/libvirt-domain.h | 4 ++-- src/libvirt-domain.c | 28 3 files changed, 40 insertions(+), 3 deletions(-) -- 1.8.3.1
Re: [PATCH 09/10] qemuDomainGetStorageSourceByDevstr: Look also in 'mirror' chain
On Mon, Jul 20, 2020 at 16:07:25 -0500, Eric Blake wrote: > On 7/15/20 8:10 AM, Peter Krempa wrote: > > A disk can have a mirror, look also in it's backing chain. > > its > > (it's is only valid when you can replace with 'it is') > > > > > Signed-off-by: Peter Krempa > > --- > > src/qemu/qemu_domain.c | 13 + > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > Reviewed-by: Eric Blake > > Hmm, when doing a pull-mode backup, do we ever want a write-threshold on the > temporary image? That's a good question. I didn't yet have a request for it. > Or is this only for actual block-copy mirroring, and not > backups? This one is just for block copy for now. The slight problem is that the backup scratch is not associated with the disk in the VM xml, but technically nothing prevents us from adding the event for that as well. The question is only how to do it API-wise.
Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier
On 2020/7/20 下午9:03, Peter Xu wrote: On Mon, Jul 20, 2020 at 12:02:06PM +0800, Jason Wang wrote: Right, so there's no need to deal with unmap in vtd's replay implementation (as what generic one did). We don't even for now; see vtd_page_walk_info.notify_unmap. Thanks, Right, but I meant the vtd_address_space_unmap() in vtd_iomm_replay(). It looks to me it will trigger UNMAP notifiers. Thanks
