[PATCH v1] Introduce virDomainReloadTLSCertificates API
Michal Prívozník
mprivozn at redhat.com
Thu May 6 12:17:05 UTC 2021
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/9cc07651655ee86eca41059f5ead8c4e5607c734
> ---
> 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
More information about the libvir-list
mailing list