[PATCH v1] Introduce virDomainReloadTLSCertificates API

Yanzheng (A) yanzheng759 at huawei.com
Fri May 7 11:23:13 UTC 2021


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 at redhat.com] 
Sent: Thursday, May 6, 2021 8:17 PM
To: Yanzheng (A) <yanzheng759 at huawei.com>; libvir-list at redhat.com
Cc: Wangxin (Alexander) <wangxinxin.wang at huawei.com>; changzihao <changzihao1 at huawei.com>; hexiaoyu (A) <hexiaoyu3 at huawei.com>; Zhangbo (Oscar) <oscar.zhangbo at 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





More information about the libvir-list mailing list