Re: [libvirt] [PATCH 35/41] remote: change hand written methods to not directly access connection
On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
[...]
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -4210,14 +4128,13 @@
> remoteDispatchConnectDomainEventRegister(virNetServerPtr server
> ATTRIBUTE_UNUSED
> daemonClientEventCallbackPtr ref;
> struct daemonClientPrivate *priv =
> virNetServerClientGetPrivateData(client);
> -
> -if (!priv->conn) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
> open"));
> -goto cleanup;
> -}
> +virConnectPtr conn = remoteGetHypervisorConn(client);
>
> virMutexLock(&priv->lock);
>
> +if (!conn)
> +goto cleanup;
> +
Shouldn't this be *before* the virMutexLock() call? As far as I can
tell, that would match the existing behavior...
The same is true for
remoteDispatchConnectDomainEventDeregister()
remoteDispatchConnectDomainEventRegisterAny()
remoteDispatchConnectDomainEventDeregisterAny()
remoteDispatchConnectDomainEventCallbackRegisterAny()
remoteDispatchConnectDomainEventCallbackDeregisterAny()
remoteDispatchConnectNetworkEventRegisterAny()
remoteDispatchConnectNetworkEventDeregisterAny()
remoteDispatchConnectStoragePoolEventRegisterAny()
remoteDispatchConnectStoragePoolEventDeregisterAny()
remoteDispatchConnectNodeDeviceEventRegisterAny()
remoteDispatchConnectNodeDeviceEventDeregisterAny()
remoteDispatchConnectSecretEventRegisterAny()
remoteDispatchConnectSecretEventDeregisterAny()
qemuDispatchConnectDomainMonitorEventRegister()
qemuDispatchConnectDomainMonitorEventDeregister()
With either all of them updated, if my understanding of the situation
as described above is correct, or left as is otherwise,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 34/41] remote: change generated methods to not directly access connection
On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -2013,6 +2134,7 @@ remoteDispatchConnectClose(virNetServerPtr server
> ATTRIBUTE_UNUSED,
> }
>
>
> +
> static int
> remoteDispatchDomainGetSchedulerType(virNetServerPtr server ATTRIBUTE_UNUSED,
> virNetServerClientPtr client,
Unrelated whitespace change.
[...]
> +++ b/src/rpc/gendispatch.pl
> @@ -581,11 +599,11 @@ elsif ($mode eq "server") {
> "virObjectUnref(snapshot);\n" .
> "virObjectUnref(dom);");
> } elsif ($args_member =~
> m/^(?:(?:admin|remote)_string|remote_uuid) (\S+)<\S+>;/) {
> -push(@args_list, $conn) if !@args_list;
> +push(@args_list, "$conn_var") if !@args_list;
I don't speak Perl, so asking mostly out of curiosity: why is the
argument quoted now? Especially since...
> @@ -1095,7 +1105,7 @@ elsif ($mode eq "server") {
> } elsif (!$multi_ret) {
> my $proc_name = $call->{ProcName};
>
> -push(@args_list, $conn) if !@args_list;
> +push(@args_list, $conn_var) if !@args_list;
... this code looks like it's performing the same operation, yet the
argument is not quoted here, which leads me to believe one of the two
is not correct.
Anyway, from a high-level perspective the changes in the script seem
reasonable enough and so do the changes they produce in the generated
files, so
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 33/41] remote: get rid of bogus ATTRIBUTE_UNUSED annotation client param
On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote: > The client parameter is always used to get access to the private data > struct. > > Signed-off-by: Daniel P. Berrangé > --- > src/remote/remote_daemon_dispatch.c | 98 ++--- > 1 file changed, 49 insertions(+), 49 deletions(-) Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 32/41] admin: add ability to connect to the per-driver daemon sockets
On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
[...]
> +++ b/src/libvirt-admin.c
> @@ -101,6 +97,7 @@ virAdmInitialize(void)
> return 0;
> }
>
> +
> static char *
> getSocketPath(virURIPtr uri)
> {
Unrelated whitespace change.
[...]
> @@ -127,27 +124,28 @@ getSocketPath(virURIPtr uri)
> if (STREQ_NULLABLE(uri->path, "/system")) {
> -if (virAsprintf(&sock_path, LOCALSTATEDIR "/run/libvirt/%s",
> -sockbase) < 0)
> +if (virAsprintf(&sock_path, LOCALSTATEDIR
> "/run/libvirt/%s-admin-sock",
> +legacy ? "libvirt" : uri->scheme) < 0)
Since you're touching this anyway, you might as well turn it into
virAsprintf(&sock_path, "%s/run/libvirt/%s-admin-sock",
LOCALSTATEDIR, legacy ? "libvirt" : uri->scheme)
With these nits addressed,
Reviewed-by: Andrea Bolognani
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/41] secret: introduce virtsecretd daemon
On Sun, 2019-07-28 at 17:22 +0200, Andrea Bolognani wrote: > Anyway, with the caveat that the comments made for previous patches > are addressed here as well if they apply, > > Reviewed-by: Andrea Bolognani Patches 21-31 are basically the same as this one, so they also get a R-b with the same caveats. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 20/41] secret: introduce virtsecretd daemon
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote: [...] > +++ b/src/secret/virtsecretd.service.in [...] > +[Install] > +WantedBy=multi-user.target > +Also=virtsecretd.socket > +Also=virtsecretd-ro.socket > +Also=virtsecretd-admin.socket Interestingly, we *do* include the admin socket here, and for all other newly-introduced daemons too it would seem! Anyway, with the caveat that the comments made for previous patches are addressed here as well if they apply, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> - We can make virtproxyd and the virtXXXd per-driver daemons all
>have "Conflicts: libvirtd.service" in their systemd unit files.
>This will guarantee that libvirtd is never started at the same
>time, as this would result in two daemons running the same driver.
>Fortunately drivers use locking to protect themselves, but it is
>better to avoid starting a daemon we know will conflict.
I feel like this will need to be tested extensively to make sure
we're always doing the right thing, including on non-systemd hosts.
[...]
> After some time we can deprecate use of libvirtd and after some more
> time delete it entirely, leaving us in a pretty world filled with
> prancing unicorns.
Awww!
> The main downside with introducing a new daemon, and with the
> per-driver daemons in general, is figuring out the correct upgrade
> path.
>
> The conservative option is to leave libvirtd running if it was
> an existing installation. Only use the new daemons & virtproxyd
> on completely new installs.
>
> The aggressive option is to disable libvirtd if already running
> and activate all the new daemons.
I vote for the conservative option :)
As an aside, the above basically a master class in how to write a
good commit message. Well done!
[...]
> +++ b/src/remote/Makefile.inc.am
[...]
> +VIRTD_UNIT_VARS = \
> + $(COMMON_UNIT_VARS) \
> + -e 's|[@]deps[@]|Conflicts=$(LIBVIRTD_SOCKET_UNIT_FILES)|g' \
> + $(NULL)
Considering that we only use LIBVIRTD_SOCKET_UNIT_FILES here, I'd
move its definition to this general area.
[...]
> +++ b/src/remote/remote_daemon.c
> @@ -303,11 +303,19 @@ static int daemonErrorLogFilter(virErrorPtr err, int
> priority)
>
> static int daemonInitialize(void)
> {
> -#ifdef MODULE_NAME
> +#ifndef LIBVIRTD
> +# ifdef MODULE_NAME
> +/* This a dedicated per-driver daemon build */
> if (virDriverLoadModule(MODULE_NAME, MODULE_NAME "Register", true) < 0)
> return -1;
> +# else
> +/* This is virtproxyd which merely proxies to the per-driver
> + * daemons for back compat, and also allows IP connectivity.
> + */
> +# endif
> #else
> -/*
> +/* This is the legacy monolithic libvirtd built with all drivers
> + *
This is exactly the kind of comment I suggested you add in patch 9,
so I guess just move the first and third one to that patch.
[...]
> @@ -893,9 +901,9 @@ daemonUsage(const char *argv0, bool privileged)
> { "-h | --help", N_("Display program help") },
> { "-v | --verbose", N_("Verbose messages") },
> { "-d | --daemon", N_("Run as a daemon & write PID file") },
> -#ifdef ENABLE_IP
> +#if defined(ENABLE_IP) && defined (LIBVIRTD)
Extra whitespace in "defined (LIBVIRTD)".
[...]
> +++ b/src/remote/virtproxyd.service.in
> @@ -0,0 +1,24 @@
> +[Unit]
> +Description=Virtualization daemon
> +Conflicts=libvirtd.service
> +Requires=virtproxyd.socket
> +Requires=virtproxyd-ro.socket
> +Requires=virtproxyd-admin.socket
> +After=network.target
> +After=dbus.service
> +After=apparmor.service
> +After=local-fs.target
> +After=remote-fs.target
> +Documentation=man:libvirtd(8)
> +Documentation=https://libvirt.org
There are a few non-obvious changes between libvirtd.service.in and
this file:
-Requires=virtlogd.socket
-Requires=virtlockd.socket
-Wants=systemd-machined.service
-Before=libvirt-guests.service
-After=iscsid.service
-After=systemd-logind.service
-After=systemd-machined.service
I can see why we'd move the relationships with iscsid and virtlockd
to virtstoraged, except looking ahead to patch 23 I see you haven't
actually done that; either way, I'm not so convinced about the
remaining changes. Care to explain the rationale behind them?
> +[Service]
> +Type=notify
> +ExecStart=@sbindir@/virtproxyd --timeout 120
> +ExecReload=/bin/kill -HUP $MAINPID
> +Restart=on-failure
More changes in this section:
-EnvironmentFile=-@sysconfdir@/sysconfig/libvirtd
-KillMode=process
-LimitNOFILE=8192
-TasksMax=32768
EnvironmentFile is clearly no longer needed, while both LimitNOFILE
and TasksMax probably belong to the hypervisor-specific daemons, but
I'm unclear on why KillMode was changed.
> +[Install]
> +WantedBy=multi-user.target
> +Also=virtproxyd.socket
> +Also=virtproxyd-ro.socket
Kind of a side note since it's pre-existing, but don't we want to
list virtproxyd-admin.socket here too?
Overall, the changes look good.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] conf: Remove unnecessary declaration in virDomainCheckpointDefParse
The @creation variable wasn't used - caused a Travis build failure. Signed-off-by: John Ferlan --- Pushing under the trivial and build break rules. src/conf/checkpoint_conf.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 30c6d2e717..5f4c275dd8 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -133,7 +133,6 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, int n; char *tmp; VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; -VIR_AUTOFREE(char *)creation = NULL; VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL; if (!(def = virDomainCheckpointDefNew())) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 1/2] tests: virsh-optparse: remove no longer valid cpu-stats test cases
These test cases are no longer valid since this series provides an implementation of the virDomainGetCPUStats API for the test driver. Signed-off-by: Ilias Stamatis --- tests/virsh-optparse | 20 1 file changed, 20 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 090d6c205c..d9c8f3c731 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -179,26 +179,6 @@ virsh -q -c $test_url cpu-stats test --start -1 >out 2>err && fail=1 test -s out && fail=1 compare exp-err err || fail=1 -# Zero. The test driver doesn't support the operation so the command -# fails, but the value has been parsed correctly -cat <<\EOF > exp-err || framework_failure -error: Failed to retrieve CPU statistics for domain 'test' -error: this function is not supported by the connection driver: virDomainGetCPUStats -EOF -virsh -q -c $test_url cpu-stats test --start 0 >out 2>err && fail=1 -test -s out && fail=1 -compare exp-err err || fail=1 - -# Valid numeric value. The test driver doesn't support the operation -# so the command fails, but the value has been parsed correctly -cat <<\EOF > exp-err || framework_failure -error: Failed to retrieve CPU statistics for domain 'test' -error: this function is not supported by the connection driver: virDomainGetCPUStats -EOF -virsh -q -c $test_url cpu-stats test --start 42 >out 2>err && fail=1 -test -s out && fail=1 -compare exp-err err || fail=1 - ### Test a scaled numeric option # Non-numeric value -- 2.22.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/2] test_driver: implement virDomainGetCPUStats
Signed-off-by: Ilias Stamatis
---
src/test/test_driver.c | 132 +
1 file changed, 132 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ab0f8b06d6..56f08fc3d2 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3629,6 +3629,137 @@ static int testDomainSetMetadata(virDomainPtr dom,
return ret;
}
+#define TEST_TOTAL_CPUTIME 48772617035
+
+static int
+testDomainGetDomainTotalCpuStats(virTypedParameterPtr params,
+int nparams)
+{
+if (nparams == 0) /* return supported number of params */
+return 3;
+
+if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
+VIR_TYPED_PARAM_ULLONG, TEST_TOTAL_CPUTIME) <
0)
+return -1;
+
+if (nparams > 1 &&
+virTypedParameterAssign(¶ms[1],
+VIR_DOMAIN_CPU_STATS_USERTIME,
+VIR_TYPED_PARAM_ULLONG, 554000) < 0)
+return -1;
+
+if (nparams > 2 &&
+virTypedParameterAssign(¶ms[2],
+VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
+VIR_TYPED_PARAM_ULLONG, 646000) < 0)
+return -1;
+
+if (nparams > 3)
+nparams = 3;
+
+return nparams;
+}
+
+
+static int
+testDomainGetPercpuStats(virTypedParameterPtr params,
+ unsigned int nparams,
+ int start_cpu,
+ unsigned int ncpus,
+ int total_cpus)
+{
+size_t i;
+int need_cpus;
+int param_idx;
+int ret = -1;
+
+/* return the number of supported params */
+if (nparams == 0 && ncpus != 0)
+return 2;
+
+/* return total number of cpus */
+if (ncpus == 0) {
+ret = total_cpus;
+goto cleanup;
+}
+
+if (start_cpu >= total_cpus) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("start_cpu %d larger than maximum of %d"),
+ start_cpu, total_cpus - 1);
+goto cleanup;
+}
+
+/* return percpu cputime in index 0 */
+param_idx = 0;
+
+/* number of cpus to compute */
+need_cpus = MIN(total_cpus, start_cpu + ncpus);
+
+for (i = 0; i < need_cpus; i++) {
+if (i < start_cpu)
+continue;
+int idx = (i - start_cpu) * nparams + param_idx;
+if (virTypedParameterAssign(¶ms[idx],
+VIR_DOMAIN_CPU_STATS_CPUTIME,
+VIR_TYPED_PARAM_ULLONG,
+(TEST_TOTAL_CPUTIME / total_cpus) + i) < 0)
+goto cleanup;
+}
+
+/* return percpu vcputime in index 1 */
+param_idx = 1;
+
+if (param_idx < nparams) {
+for (i = start_cpu; i < need_cpus; i++) {
+int idx = (i - start_cpu) * nparams + param_idx;
+if (virTypedParameterAssign(¶ms[idx],
+VIR_DOMAIN_CPU_STATS_VCPUTIME,
+VIR_TYPED_PARAM_ULLONG,
+(TEST_TOTAL_CPUTIME / total_cpus) -
1234567890 + i) < 0)
+goto cleanup;
+}
+param_idx++;
+}
+
+ret = param_idx;
+ cleanup:
+return ret;
+}
+
+
+static int
+testDomainGetCPUStats(virDomainPtr dom,
+ virTypedParameterPtr params,
+ unsigned int nparams,
+ int start_cpu,
+ unsigned int ncpus,
+ unsigned int flags)
+{
+virDomainObjPtr vm = NULL;
+testDriverPtr privconn = dom->conn->privateData;
+int ret = -1;
+
+virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+if (!(vm = testDomObjFromDomain(dom)))
+return -1;
+
+if (virDomainObjCheckActive(vm) < 0)
+goto cleanup;
+
+if (start_cpu == -1)
+ret = testDomainGetDomainTotalCpuStats(params, nparams);
+else
+ret = testDomainGetPercpuStats(params, nparams, start_cpu, ncpus,
+ privconn->nodeInfo.cores);
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+return ret;
+}
+
+
static int
testDomainSendProcessSignal(virDomainPtr dom,
long long pid_value,
@@ -8552,6 +8683,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainSendKey = testDomainSendKey, /* 5.5.0 */
.domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */
.domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */
+.domainGetCPUStats = testDomainGetCPUStats, /* 5.6.0 */
.domainSendProcessSignal = testDomainSendProcessSignal, /* 5.5.0 */
.connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */
.domainManagedSave = testDomainManagedSave, /* 1.1.4 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/2] test_driver: implement virDomainGetCPUStats
Changes since v1: * Removed failing test cases * Total CPUTIME now is the sum of the individual CPUs Ilias Stamatis (2): tests: virsh-optparse: remove no longer valid cpu-stats test cases test_driver: implement virDomainGetCPUStats src/test/test_driver.c | 132 + tests/virsh-optparse | 20 --- 2 files changed, 132 insertions(+), 20 deletions(-) -- 2.22.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
