6:23 a.m.
Thanks for Michal's nice review opinions.
I followed these proposals rewrite and retest the patch again, and will send the v2 patch
through git send-email later.
int
virDomainReloadTLSCertificates(virDomainPtr domain,
unsigned int type,
virTypedParameterPtr params,
int nparams,
unsigned int flags);
Then after that provide a matching libvirt-python API for virDomainReloadTLSCertificates:
def reloadTLSCertificates(self, type, params, flags=0)
-----Original Message-----
From: Michal Prívozník [mailto:mprivozn@redhat.com]
Sent: Thursday, May 6, 2021 8:17 PM
To: Yanzheng (A) <yanzheng759(a)huawei.com>; libvir-list(a)redhat.com
Cc: Wangxin (Alexander) <wangxinxin.wang(a)huawei.com>; changzihao
<changzihao1(a)huawei.com>; hexiaoyu (A) <hexiaoyu3(a)huawei.com>; Zhangbo (Oscar)
<oscar.zhangbo(a)huawei.com>
Subject: Re: [PATCH v1] Introduce virDomainReloadTLSCertificates API
On 5/6/21 5:31 AM, Yanzheng (A) wrote:
Introduce a new virDomainReloadTLSCertificates API for notify domain
reload its certificates without restart, and avoid service interruption.
Take reload QEMU VNC TLS certificates as an example, we can call:
virDomainReloadTLSCertificates(dom,
VIR_DOMAIN_TLS_CERT_GRAPHICS_VNC)
Then the specified QMP message would be send to QEMU:
{"execute": "display-reload",
"arguments":{"type": "vnc", "tls-certs":
true}}
Refers:
https://gitlab.com/qemu-project/qemu/-/commit/9cc07651655ee86eca41059f
5ead8c4e5607c734
---
include/libvirt/libvirt-domain.h | 17 ++++++++++++++++
src/driver-hypervisor.h | 5 +++++
src/libvirt-domain.c | 33 ++++++++++++++++++++++++++++++++
src/qemu/qemu_driver.c | 11 +++++++++++
src/qemu/qemu_hotplug.c | 21 ++++++++++++++++++++
src/qemu/qemu_hotplug.h | 4 ++++
src/qemu/qemu_monitor.c | 10 ++++++++++
src/qemu/qemu_monitor.h | 3 +++
src/qemu/qemu_monitor_json.c | 22 +++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 3 +++
10 files changed, 129 insertions(+)
I can't apply this patch.
Applying: Introduce virDomainReloadTLSCertificates API
error: corrupt patch at line 38
Patch failed at 0001 Introduce virDomainReloadTLSCertificates API
Can you please use git send-email? It is known to send patches properly.
diff --git a/include/libvirt/libvirt-domain.h
b/include/libvirt/libvirt-domain.h
index e99bfb7654..aeb33d69d9 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -5152,4 +5152,21 @@ int virDomainStartDirtyRateCalc(virDomainPtr domain,
int seconds,
unsigned int flags);
+/**
+ * virDomainTLSCertificaType:
+ *
+ * the used scene of TLS certificates for doamin.
+ */
+typedef enum {
+ VIR_DOMAIN_TLS_CERT_GRAPHICS_VNC = 0,
+ VIR_DOMAIN_TLS_CERT_GRAPHICS_SPICE = 1,
+
+ VIR_DOMAIN_TLS_CERT_LAST
+} virDomainTLSCertificaType;
+
+int
+virDomainReloadTLSCertificates(virDomainPtr domain,
+ unsigned int type);
Every new API must have 'flags' argument, even though it might be unused (the API
implementation should then check that the flags value is zero).
It has bitten us too many times so that we turned it into a rule.
And thinking future proof - I wonder if this should be more generic API.
For instance:
virDomainDisplayReload(virDomainPtr domain,
unsigned int type,
virTypedParameterPtr params,
int nparams,
unsigned int flags)
And for TLS certs we would invent new typed param. I think this would be more extensible -
if the qemu "display-reload" command ever gains new arguments.
+
+
#endif /* LIBVIRT_DOMAIN_H */
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index
d642af8a37..8de2bc4137 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1410,6 +1410,10 @@ typedef int
int seconds,
unsigned int flags);
+typedef int
+(*virDrvDomainReloadTLSCertificates)(virDomainPtr domain,
+ unsigned int type);
+
typedef struct _virHypervisorDriver virHypervisorDriver;
/**
@@ -1676,4 +1680,5 @@ struct _virHypervisorDriver {
virDrvDomainAuthorizedSSHKeysSet domainAuthorizedSSHKeysSet;
virDrvDomainGetMessages domainGetMessages;
virDrvDomainStartDirtyRateCalc domainStartDirtyRateCalc;
+ virDrvDomainReloadTLSCertificates domainiReloadTLSCertificates;
};
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index
42c75f6cc5..fb9e5ec2d1 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -13218,3 +13218,36 @@ virDomainStartDirtyRateCalc(virDomainPtr domain,
virDispatchError(conn);
return -1;
}
+
+/**
+ * virDomainReloadTLSCertificates:
+ * @domain: a domain object.
+ * @type: a value of virDomainTLSCertificaType
+ *
+ * Notify domain reload its certificates with specified 'type'.
+ *
+ * Returns 0 in case of success, -1 otherwise .
+ */
+int
+virDomainReloadTLSCertificates(virDomainPtr domain,
+ unsigned int type) {
+ virConnectPtr conn;
+ VIR_DOMAIN_DEBUG(domain, "certificate type=%d", type);
Can you please use empty lines to split the body into chunks? Look around the file - you
will find a lot of examples.
+ virResetLastError();
+ virCheckDomainReturn(domain, -1);
+ conn = domain->conn;
+ if (type >= VIR_DOMAIN_TLS_CERT_LAST)
+ goto error;
Alright, fair enough (although I'm not a big fan of checking this in public API
implementation). But, if you want this to be here an error must be reported, otherwise
this API will return -1 with no error set.
virReportInvalidArg() sounds like a good candidate to report an error.
+ if (conn->driver->domainiReloadTLSCertificates) {
+ int ret;
+ ret = conn->driver->domainiReloadTLSCertificates(domain, type);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+ virReportUnsupportedError();
+ error:
+ virDispatchError(domain->conn);
+ return -1;
+}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index
c90d52edc0..61cd8cfa24 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20449,6 +20449,16 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
return ret;
}
+static int
+qemuDomainReloadTLSCertificates(virDomainPtr domain,
+ unsigned int type) {
+ virQEMUDriver *driver = domain->conn->privateData;
+ virDomainObj *vm = qemuDomainObjFromDomain(domain);
+ if (!driver || !vm)
+ return -1;
I'd expect a QEMU_JOB_MODIFY job to be acquired here, because ...
+ return qemuDomainReloadTLSCerts(driver, vm, type);
.. this ^^ enters monitor. And thus has potential to change the state of domain (state in
broader sense, not just what virDomainGetState() would report).
+}
static virHypervisorDriver qemuHypervisorDriver = {
.name = QEMU_DRIVER_NAME,
@@ -20693,6 +20703,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */
.domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */
.domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /*
7.2.0 */
+ .domainiReloadTLSCertificates = qemuDomainReloadTLSCertificates,
+ /* 7.2.0 */
It's too late for 7.2.0. We are working on 7.4.0 now.
};
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index
444d89d64a..013d8728a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -6704,3 +6704,24 @@ qemuDomainSetVcpuInternal(virQEMUDriver *driver,
virBitmapFree(livevcpus);
return ret;
}
+
+int qemuDomainReloadTLSCerts(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ int type) {
+ int ret = -1;
+ qemuDomainObjPrivate *priv = vm->privateData;
+ /* for now, only VNC is supported */
+ if (type != VIR_DOMAIN_TLS_CERT_GRAPHICS_VNC)
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("invalid certificate type=%d, only support VNC"),
+ type);
+ return ret;
+ }
+ if (qemuDomainObjEnterMonitorAsync(driver, vm,
+QEMU_ASYNC_JOB_NONE) < 0)
Is there a reason why you want to enter monitor asynchronously? I would expect this to be
plain qemuDomainObjEnterMonitor().
+ return ret;
+ ret = qemuMonitorReloadTLSCerts(priv->mon, type);
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ ret = -1;
+ return ret;
+}
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index
df8f76f8d6..44afe23f0a 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -160,3 +160,7 @@ int qemuHotplugAttachDBusVMState(virQEMUDriver
*driver, int qemuHotplugRemoveDBusVMState(virQEMUDriver *driver,
virDomainObj *vm,
qemuDomainAsyncJob asyncJob);
+
+int qemuDomainReloadTLSCerts(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ int type);
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index
3a7f231ce0..952ef87a6b 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4746,3 +4746,13 @@ qemuMonitorQueryDirtyRate(qemuMonitor *mon,
return qemuMonitorJSONQueryDirtyRate(mon, info); }
+
+int
+qemuMonitorReloadTLSCerts(qemuMonitorPtr mon, int type) {
+ const char *protocol = qemuMonitorTypeToProtocol(type);
+ if (!protocol)
+ return -1;
+ VIR_DEBUG("protocol=%s", protocol);
Missing QEMU_CHECK_MONITOR(mon); call.
+ return qemuMonitorJSONReloadTLSCerts(mon, protocol); }
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index
6a25def78b..a5b702b023 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1496,3 +1496,6 @@ struct _qemuMonitorDirtyRateInfo { int
qemuMonitorQueryDirtyRate(qemuMonitor *mon,
qemuMonitorDirtyRateInfo *info);
+
+int qemuMonitorReloadTLSCerts(qemuMonitorPtr mon,
+ int type);
diff --git a/src/qemu/qemu_monitor_json.c
b/src/qemu/qemu_monitor_json.c index 46aa3330a8..d2b06c4703 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9446,3 +9446,25 @@ qemuMonitorJSONQueryDirtyRate(qemuMonitor *mon,
return qemuMonitorJSONExtractDirtyRateInfo(data, info); }
+
+int qemuMonitorJSONReloadTLSCerts(qemuMonitorPtr mon,
+ const char *protocol) {
+ int ret = -1;
+ virJSONValuePtr reply = NULL;
+ virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("display-reload",
+ "s:type", protocol,
+ "b:tls-certs", 1,
+ NULL);
+ if (!cmd)
+ return -1;
+ if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+ goto cleanup;
+ if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+ goto cleanup;
+ ret = 0;
+ cleanup:
+ virJSONValueFree(cmd);
+ virJSONValueFree(reply);
+ return ret;
+}
diff --git a/src/qemu/qemu_monitor_json.h
b/src/qemu/qemu_monitor_json.h index 01a3ba25f1..d9ad77e873 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -706,3 +706,6 @@ qemuMonitorJSONStartDirtyRateCalc(qemuMonitor
*mon, int qemuMonitorJSONQueryDirtyRate(qemuMonitor *mon,
qemuMonitorDirtyRateInfo *info);
+
+int qemuMonitorJSONReloadTLSCerts(qemuMonitorPtr mon,
+ const char *protocol);
--
2.25.1
Michal