[PATCH v2 0/2] support tls certificates reload

[PATCH v2 0/2] support tls certificates reload

[Zihao Chang]

This patch set implements the reload interface for QCryptoTLSCredsX509
and add qmp to trigger the tls certificates reload.

Zihao Chang (2):
  crypto: add reload for QCryptoTLSCredsClass
  vnc: add qmp to support reload vnc tls certificates

 crypto/tlscredsx509.c     | 77 +++++++++++++++++++++++++++++++++++++++
 include/crypto/tlscreds.h |  8 ++--
 include/ui/console.h      |  1 +
 monitor/qmp-cmds.c        |  5 +++
 qapi/ui.json              | 18 +++++++++
 ui/vnc.c                  | 24 ++++++++++++
 6 files changed, 130 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH v2 1/2] crypto: add reload for QCryptoTLSCredsClass

[Zihao Chang]

This patch adds reload interface for QCryptoTLSCredsClass and implements
the interface for QCryptoTLSCredsX509.

Signed-off-by: Zihao Chang <changzihao1@huawei.com>
---
 crypto/tlscredsx509.c     | 77 +++++++++++++++++++++++++++++++++++++++
 include/crypto/tlscreds.h |  8 ++--
 2 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index dd7267ccdb..a94f862d1d 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -23,8 +23,10 @@
 #include "tlscredspriv.h"
 #include "crypto/secret.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qstring.h"
 #include "qemu/module.h"
 #include "qom/object_interfaces.h"
+#include "qom/qom-qobject.h"
 #include "trace.h"
 
 
@@ -771,6 +773,78 @@ qcrypto_tls_creds_x509_prop_get_sanity(Object *obj,
 }
 
 
+#ifdef CONFIG_GNUTLS
+
+
+static void
+qcrypto_tls_creds_x509_reload(QCryptoTLSCreds *creds, Error **errp)
+{
+    Object *old = OBJECT(creds);
+    QCryptoTLSCredsX509 *old_x509 = QCRYPTO_TLS_CREDS_X509(creds);
+    Object *new = object_new(TYPE_QCRYPTO_TLS_CREDS_X509);
+    QCryptoTLSCredsX509 *new_x509 = QCRYPTO_TLS_CREDS_X509(new);
+    ObjectProperty *prop;
+    ObjectPropertyIterator iter;
+    Error *local_err = NULL;
+
+    object_property_iter_init(&iter, old);
+    while ((prop = object_property_iter_next(&iter))) {
+        QObject *value;
+        /* loaded depends on other props, copy it finally */
+        if (g_strcmp0(prop->name, "loaded") == 0) {
+            continue;
+        }
+
+        value = object_property_get_qobject(old, prop->name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+
+        if ((!prop->set) ||
+            (qobject_type(value) == QTYPE_QSTRING &&
+             strlen(qstring_get_str(qobject_to(QString, value))) == 0)) {
+            continue;
+        }
+
+        object_property_set_qobject(new, prop->name, value, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+    }
+
+    qcrypto_tls_creds_x509_load(new_x509, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
+    /* load new cert successfully, free the old and update */
+    qcrypto_tls_creds_x509_unload(old_x509);
+    old_x509->data = new_x509->data;
+    old_x509->parent_obj.dh_params = new_x509->parent_obj.dh_params;
+    new_x509->data = NULL;
+    new_x509->parent_obj.dh_params = NULL;
+
+out:
+    object_unref(new);
+}
+
+
+#else /* ! CONFIG_GNUTLS */
+
+
+static void
+qcrypto_tls_creds_x509_reload(QCryptoTLSCreds *creds, Error **errp)
+{
+    /* nada */
+}
+
+
+#endif /* ! CONFIG_GNUTLS */
+
+
 static void
 qcrypto_tls_creds_x509_complete(UserCreatable *uc, Error **errp)
 {
@@ -801,6 +875,9 @@ static void
 qcrypto_tls_creds_x509_class_init(ObjectClass *oc, void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    QCryptoTLSCredsClass *ctcc = QCRYPTO_TLS_CREDS_CLASS(oc);
+
+    ctcc->reload = qcrypto_tls_creds_x509_reload;
 
     ucc->complete = qcrypto_tls_creds_x509_complete;
 
diff --git a/include/crypto/tlscreds.h b/include/crypto/tlscreds.h
index 079e376047..281c01a304 100644
--- a/include/crypto/tlscreds.h
+++ b/include/crypto/tlscreds.h
@@ -30,14 +30,15 @@
 
 #define TYPE_QCRYPTO_TLS_CREDS "tls-creds"
 typedef struct QCryptoTLSCreds QCryptoTLSCreds;
-DECLARE_INSTANCE_CHECKER(QCryptoTLSCreds, QCRYPTO_TLS_CREDS,
-                         TYPE_QCRYPTO_TLS_CREDS)
-
 typedef struct QCryptoTLSCredsClass QCryptoTLSCredsClass;
+DECLARE_OBJ_CHECKERS(QCryptoTLSCreds, QCryptoTLSCredsClass, QCRYPTO_TLS_CREDS,
+                     TYPE_QCRYPTO_TLS_CREDS)
+
 
 #define QCRYPTO_TLS_CREDS_DH_PARAMS "dh-params.pem"
 
 
+typedef void (*CryptoTLSCredsReload)(QCryptoTLSCreds *, Error **);
 /**
  * QCryptoTLSCreds:
  *
@@ -61,6 +62,7 @@ struct QCryptoTLSCreds {
 
 struct QCryptoTLSCredsClass {
     ObjectClass parent_class;
+    CryptoTLSCredsReload reload;
 };
 
 
-- 
2.23.0

[PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Zihao Chang]

QEMU loads vnc tls certificates only when vm is started. This patch
provides a new qmp to reload vnc tls certificates without restart
vnc-server/VM.
{"execute": "reload-vnc-cert"}

Signed-off-by: Zihao Chang <changzihao1@huawei.com>
---
 include/ui/console.h |  1 +
 monitor/qmp-cmds.c   |  5 +++++
 qapi/ui.json         | 18 ++++++++++++++++++
 ui/vnc.c             | 24 ++++++++++++++++++++++++
 4 files changed, 48 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 5dd21976a3..60a24a8bb5 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char *password);
 int vnc_display_pw_expire(const char *id, time_t expires);
 QemuOpts *vnc_parse(const char *str, Error **errp);
 int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
+void vnc_display_reload_cert(const char *id,  Error **errp);
 
 /* input.c */
 int index_from_key(const char *key, size_t key_length);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 34f7e75b7b..84f2b74ea8 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -287,6 +287,11 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
         qmp_change_vnc_listen(target, errp);
     }
 }
+
+void qmp_reload_vnc_cert(Error **errp)
+{
+    vnc_display_reload_cert(NULL, errp);
+}
 #endif /* !CONFIG_VNC */
 
 void qmp_change(const char *device, const char *target,
diff --git a/qapi/ui.json b/qapi/ui.json
index d08d72b439..855b1ac007 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1179,3 +1179,21 @@
 ##
 { 'command': 'query-display-options',
   'returns': 'DisplayOptions' }
+
+##
+# @reload-vnc-cert:
+#
+# Reload certificates for vnc.
+#
+# Returns: nothing
+#
+# Since: 5.2
+#
+# Example:
+#
+# -> { "execute": "reload-vnc-cert" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'reload-vnc-cert',
+  'if': 'defined(CONFIG_VNC)' }
diff --git a/ui/vnc.c b/ui/vnc.c
index 7452ac7df2..2cc88c2421 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -582,6 +582,30 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
     return prev;
 }
 
+void vnc_display_reload_cert(const char *id, Error **errp)
+{
+    VncDisplay *vd = vnc_display_find(id);
+    QCryptoTLSCredsClass *creds = NULL;
+
+    if (!vd) {
+        error_setg(errp, "Can not find Vnc Display");
+        return;
+    }
+
+    if (!vd->tlscreds) {
+        error_setg(errp, "Vnc tls is not enable");
+        return;
+    }
+
+    creds = QCRYPTO_TLS_CREDS_GET_CLASS(OBJECT(vd->tlscreds));
+    if (creds->reload == NULL) {
+        error_setg(errp, "%s doesn't support to reload TLS credential",
+                   object_get_typename(OBJECT(vd->tlscreds)));
+        return;
+    }
+    creds->reload(vd->tlscreds, errp);
+}
+
 /* TODO
    1) Get the queue working for IO.
    2) there is some weirdness when using the -S option (the screen is grey
-- 
2.23.0

Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Markus Armbruster]

Zihao Chang <changzihao1@huawei.com> writes:

> QEMU loads vnc tls certificates only when vm is started. This patch
> provides a new qmp to reload vnc tls certificates without restart
> vnc-server/VM.
> {"execute": "reload-vnc-cert"}
>
> Signed-off-by: Zihao Chang <changzihao1@huawei.com>
> ---
>  include/ui/console.h |  1 +
>  monitor/qmp-cmds.c   |  5 +++++
>  qapi/ui.json         | 18 ++++++++++++++++++
>  ui/vnc.c             | 24 ++++++++++++++++++++++++
>  4 files changed, 48 insertions(+)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 5dd21976a3..60a24a8bb5 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char *password);
>  int vnc_display_pw_expire(const char *id, time_t expires);
>  QemuOpts *vnc_parse(const char *str, Error **errp);
>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> +void vnc_display_reload_cert(const char *id,  Error **errp);

Make this return bool, please.

error.h's big comment:

 * = Rules =
 *
 * - Functions that use Error to report errors have an Error **errp
 *   parameter.  It should be the last parameter, except for functions
 *   taking variable arguments.
 *
 * - You may pass NULL to not receive the error, &error_abort to abort
 *   on error, &error_fatal to exit(1) on error, or a pointer to a
 *   variable containing NULL to receive the error.
 *
 * - Separation of concerns: the function is responsible for detecting
 *   errors and failing cleanly; handling the error is its caller's
 *   job.  Since the value of @errp is about handling the error, the
 *   function should not examine it.
 *
 * - The function may pass @errp to functions it calls to pass on
 *   their errors to its caller.  If it dereferences @errp to check
 *   for errors, it must use ERRP_GUARD().
 *
 * - On success, the function should not touch *errp.  On failure, it
 *   should set a new error, e.g. with error_setg(errp, ...), or
 *   propagate an existing one, e.g. with error_propagate(errp, ...).
 *
 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

>  
>  /* input.c */
>  int index_from_key(const char *key, size_t key_length);
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 34f7e75b7b..84f2b74ea8 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -287,6 +287,11 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
>          qmp_change_vnc_listen(target, errp);
>      }
>  }
> +
> +void qmp_reload_vnc_cert(Error **errp)
> +{
> +    vnc_display_reload_cert(NULL, errp);
> +}
>  #endif /* !CONFIG_VNC */
>  
>  void qmp_change(const char *device, const char *target,
> diff --git a/qapi/ui.json b/qapi/ui.json
> index d08d72b439..855b1ac007 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1179,3 +1179,21 @@
>  ##
>  { 'command': 'query-display-options',
>    'returns': 'DisplayOptions' }
> +
> +##
> +# @reload-vnc-cert:
> +#
> +# Reload certificates for vnc.
> +#
> +# Returns: nothing
> +#
> +# Since: 5.2

6.0 now.

> +#
> +# Example:
> +#
> +# -> { "execute": "reload-vnc-cert" }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'reload-vnc-cert',
> +  'if': 'defined(CONFIG_VNC)' }

Daniel's objection to another patch that adds a rather specialized QMP
command may apply: "I'm not a fan of adding adhoc new commands for
specific properties."

    From: Daniel P. Berrangé <berrange@redhat.com>
    Subject: Re: [PATCH] vnc: add qmp to support change authz
    Message-ID: <20210107161830.GE1029501@redhat.com>

[...]

Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Daniel P. Berrangé]

On Fri, Jan 15, 2021 at 02:37:33PM +0100, Markus Armbruster wrote:
> Zihao Chang <changzihao1@huawei.com> writes:
> 
> > QEMU loads vnc tls certificates only when vm is started. This patch
> > provides a new qmp to reload vnc tls certificates without restart
> > vnc-server/VM.
> > {"execute": "reload-vnc-cert"}
> >
> > Signed-off-by: Zihao Chang <changzihao1@huawei.com>
> > ---
> >  include/ui/console.h |  1 +
> >  monitor/qmp-cmds.c   |  5 +++++
> >  qapi/ui.json         | 18 ++++++++++++++++++
> >  ui/vnc.c             | 24 ++++++++++++++++++++++++
> >  4 files changed, 48 insertions(+)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 5dd21976a3..60a24a8bb5 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char *password);
> >  int vnc_display_pw_expire(const char *id, time_t expires);
> >  QemuOpts *vnc_parse(const char *str, Error **errp);
> >  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> > +void vnc_display_reload_cert(const char *id,  Error **errp);
> 
> Make this return bool, please.
> 
> error.h's big comment:
> 
>  * = Rules =
>  *
>  * - Functions that use Error to report errors have an Error **errp
>  *   parameter.  It should be the last parameter, except for functions
>  *   taking variable arguments.
>  *
>  * - You may pass NULL to not receive the error, &error_abort to abort
>  *   on error, &error_fatal to exit(1) on error, or a pointer to a
>  *   variable containing NULL to receive the error.
>  *
>  * - Separation of concerns: the function is responsible for detecting
>  *   errors and failing cleanly; handling the error is its caller's
>  *   job.  Since the value of @errp is about handling the error, the
>  *   function should not examine it.
>  *
>  * - The function may pass @errp to functions it calls to pass on
>  *   their errors to its caller.  If it dereferences @errp to check
>  *   for errors, it must use ERRP_GUARD().
>  *
>  * - On success, the function should not touch *errp.  On failure, it
>  *   should set a new error, e.g. with error_setg(errp, ...), or
>  *   propagate an existing one, e.g. with error_propagate(errp, ...).
>  *
>  * - Whenever practical, also return a value that indicates success /
>  *   failure.  This can make the error checking more concise, and can
>  *   avoid useless error object creation and destruction.  Note that
>  *   we still have many functions returning void.  We recommend
>  *   • bool-valued functions return true on success / false on failure,
>  *   • pointer-valued functions return non-null / null pointer, and
>  *   • integer-valued functions return non-negative / negative.
> 
> >  
> >  /* input.c */
> >  int index_from_key(const char *key, size_t key_length);
> > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> > index 34f7e75b7b..84f2b74ea8 100644
> > --- a/monitor/qmp-cmds.c
> > +++ b/monitor/qmp-cmds.c
> > @@ -287,6 +287,11 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> >          qmp_change_vnc_listen(target, errp);
> >      }
> >  }
> > +
> > +void qmp_reload_vnc_cert(Error **errp)
> > +{
> > +    vnc_display_reload_cert(NULL, errp);
> > +}
> >  #endif /* !CONFIG_VNC */
> >  
> >  void qmp_change(const char *device, const char *target,
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index d08d72b439..855b1ac007 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1179,3 +1179,21 @@
> >  ##
> >  { 'command': 'query-display-options',
> >    'returns': 'DisplayOptions' }
> > +
> > +##
> > +# @reload-vnc-cert:
> > +#
> > +# Reload certificates for vnc.
> > +#
> > +# Returns: nothing
> > +#
> > +# Since: 5.2
> 
> 6.0 now.
> 
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "reload-vnc-cert" }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'reload-vnc-cert',
> > +  'if': 'defined(CONFIG_VNC)' }
> 
> Daniel's objection to another patch that adds a rather specialized QMP
> command may apply: "I'm not a fan of adding adhoc new commands for
> specific properties."
> 
>     From: Daniel P. Berrangé <berrange@redhat.com>
>     Subject: Re: [PATCH] vnc: add qmp to support change authz
>     Message-ID: <20210107161830.GE1029501@redhat.com>

Yeah, though this scenario is a ittle more complicated. Tihs patch is
not actually changing any object properties in the VNC server config.
It is simply telling the VNC server to reload the existing object it
has configured.

My proposed  "display-update" command would not directly map to what
this "reload-vnc-cert" command does, unless we declared the certs are
always reloaded after every display-update command.

Or we could have a more generic  "display-reload" command, which has
fields indicating what aspect to reload. eg a 'tls-certs: bool' field
to indicate reload of TLS certs in the display. This could be useful
if we wanted the ability to reload authz access control lists, though
we did actually wire them up to auto-reload using inotify.


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 v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Zihao Chang]

On 2021/1/15 21:47, Daniel P. Berrangé wrote:
> On Fri, Jan 15, 2021 at 02:37:33PM +0100, Markus Armbruster wrote:
>> Zihao Chang <changzihao1@huawei.com> writes:
>>
>>> QEMU loads vnc tls certificates only when vm is started. This patch
>>> provides a new qmp to reload vnc tls certificates without restart
>>> vnc-server/VM.
>>> {"execute": "reload-vnc-cert"}
>>>
>>> Signed-off-by: Zihao Chang <changzihao1@huawei.com>
>>> ---
>>>  include/ui/console.h |  1 +
>>>  monitor/qmp-cmds.c   |  5 +++++
>>>  qapi/ui.json         | 18 ++++++++++++++++++
>>>  ui/vnc.c             | 24 ++++++++++++++++++++++++
>>>  4 files changed, 48 insertions(+)
>>>
>>> diff --git a/include/ui/console.h b/include/ui/console.h
>>> index 5dd21976a3..60a24a8bb5 100644
>>> --- a/include/ui/console.h
>>> +++ b/include/ui/console.h
>>> @@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char *password);
>>>  int vnc_display_pw_expire(const char *id, time_t expires);
>>>  QemuOpts *vnc_parse(const char *str, Error **errp);
>>>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
>>> +void vnc_display_reload_cert(const char *id,  Error **errp);
>>
>> Make this return bool, please.
>>
I will fix this in the next version.
Thank your for your reviews.

>> error.h's big comment:
>>
>>  * = Rules =
>>  *
>>  * - Functions that use Error to report errors have an Error **errp
>>  *   parameter.  It should be the last parameter, except for functions
>>  *   taking variable arguments.
>>  *
>>  * - You may pass NULL to not receive the error, &error_abort to abort
>>  *   on error, &error_fatal to exit(1) on error, or a pointer to a
>>  *   variable containing NULL to receive the error.
>>  *
>>  * - Separation of concerns: the function is responsible for detecting
>>  *   errors and failing cleanly; handling the error is its caller's
>>  *   job.  Since the value of @errp is about handling the error, the
>>  *   function should not examine it.
>>  *
>>  * - The function may pass @errp to functions it calls to pass on
>>  *   their errors to its caller.  If it dereferences @errp to check
>>  *   for errors, it must use ERRP_GUARD().
>>  *
>>  * - On success, the function should not touch *errp.  On failure, it
>>  *   should set a new error, e.g. with error_setg(errp, ...), or
>>  *   propagate an existing one, e.g. with error_propagate(errp, ...).
>>  *
>>  * - Whenever practical, also return a value that indicates success /
>>  *   failure.  This can make the error checking more concise, and can
>>  *   avoid useless error object creation and destruction.  Note that
>>  *   we still have many functions returning void.  We recommend
>>  *   • bool-valued functions return true on success / false on failure,
>>  *   • pointer-valued functions return non-null / null pointer, and
>>  *   • integer-valued functions return non-negative / negative.
>>
>>>  
>>>  /* input.c */
>>>  int index_from_key(const char *key, size_t key_length);
>>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>>> index 34f7e75b7b..84f2b74ea8 100644
>>> --- a/monitor/qmp-cmds.c
>>> +++ b/monitor/qmp-cmds.c
>>> @@ -287,6 +287,11 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
>>>          qmp_change_vnc_listen(target, errp);
>>>      }
>>>  }
>>> +
>>> +void qmp_reload_vnc_cert(Error **errp)
>>> +{
>>> +    vnc_display_reload_cert(NULL, errp);
>>> +}
>>>  #endif /* !CONFIG_VNC */
>>>  
>>>  void qmp_change(const char *device, const char *target,
>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>> index d08d72b439..855b1ac007 100644
>>> --- a/qapi/ui.json
>>> +++ b/qapi/ui.json
>>> @@ -1179,3 +1179,21 @@
>>>  ##
>>>  { 'command': 'query-display-options',
>>>    'returns': 'DisplayOptions' }
>>> +
>>> +##
>>> +# @reload-vnc-cert:
>>> +#
>>> +# Reload certificates for vnc.
>>> +#
>>> +# Returns: nothing
>>> +#
>>> +# Since: 5.2
>>
>> 6.0 now.
>>
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "reload-vnc-cert" }
>>> +# <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'reload-vnc-cert',
>>> +  'if': 'defined(CONFIG_VNC)' }
>>
>> Daniel's objection to another patch that adds a rather specialized QMP
>> command may apply: "I'm not a fan of adding adhoc new commands for
>> specific properties."
>>
>>     From: Daniel P. Berrangé <berrange@redhat.com>
>>     Subject: Re: [PATCH] vnc: add qmp to support change authz
>>     Message-ID: <20210107161830.GE1029501@redhat.com>
> 
> Yeah, though this scenario is a ittle more complicated. Tihs patch is
> not actually changing any object properties in the VNC server config.
> It is simply telling the VNC server to reload the existing object it
> has configured.
> 
> My proposed  "display-update" command would not directly map to what
> this "reload-vnc-cert" command does, unless we declared the certs are
> always reloaded after every display-update command.
> 
> Or we could have a more generic  "display-reload" command, which has
> fields indicating what aspect to reload. eg a 'tls-certs: bool' field
> to indicate reload of TLS certs in the display. This could be useful
> if we wanted the ability to reload authz access control lists, though
> we did actually wire them up to auto-reload using inotify.
> 
> 
> Regards,
> Daniel
> 

If we add field for each reloadable attribute(tls-certs, authz,...),
the number of parameters for qmp_display_reload() may increase sharply
(bool has_tls_certs, bool tls_certs, ... twice the number of attributes).

How about using a list[] to determine which attributes need to be
reloaded["tls_certs", "authz*"...], and define an enum to ensure the
validity of list elements.


Regards,
Zihao

Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Markus Armbruster]

Zihao Chang <changzihao1@huawei.com> writes:

> On 2021/1/15 21:47, Daniel P. Berrangé wrote:
>> On Fri, Jan 15, 2021 at 02:37:33PM +0100, Markus Armbruster wrote:
>>> Zihao Chang <changzihao1@huawei.com> writes:
[...]
>>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>>> index d08d72b439..855b1ac007 100644
>>>> --- a/qapi/ui.json
>>>> +++ b/qapi/ui.json
>>>> @@ -1179,3 +1179,21 @@
>>>>  ##
>>>>  { 'command': 'query-display-options',
>>>>    'returns': 'DisplayOptions' }
>>>> +
>>>> +##
>>>> +# @reload-vnc-cert:
>>>> +#
>>>> +# Reload certificates for vnc.
>>>> +#
>>>> +# Returns: nothing
>>>> +#
>>>> +# Since: 5.2
>>>
>>> 6.0 now.
>>>
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "reload-vnc-cert" }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'reload-vnc-cert',
>>>> +  'if': 'defined(CONFIG_VNC)' }
>>>
>>> Daniel's objection to another patch that adds a rather specialized QMP
>>> command may apply: "I'm not a fan of adding adhoc new commands for
>>> specific properties."
>>>
>>>     From: Daniel P. Berrangé <berrange@redhat.com>
>>>     Subject: Re: [PATCH] vnc: add qmp to support change authz
>>>     Message-ID: <20210107161830.GE1029501@redhat.com>
>> 
>> Yeah, though this scenario is a ittle more complicated. Tihs patch is
>> not actually changing any object properties in the VNC server config.
>> It is simply telling the VNC server to reload the existing object it
>> has configured.
>> 
>> My proposed  "display-update" command would not directly map to what
>> this "reload-vnc-cert" command does, unless we declared the certs are
>> always reloaded after every display-update command.
>> 
>> Or we could have a more generic  "display-reload" command, which has
>> fields indicating what aspect to reload. eg a 'tls-certs: bool' field
>> to indicate reload of TLS certs in the display. This could be useful
>> if we wanted the ability to reload authz access control lists, though
>> we did actually wire them up to auto-reload using inotify.
>> 
>> 
>> Regards,
>> Daniel
>> 
>
> If we add field for each reloadable attribute(tls-certs, authz,...),
> the number of parameters for qmp_display_reload() may increase sharply
> (bool has_tls_certs, bool tls_certs, ... twice the number of attributes).

'boxed': true can give you a reasonable C function even then.
docs/devel/qapi-code-gen.txt:

    When member 'boxed' is absent, [the function generated for the
    command] takes the command arguments as arguments one by one, in
    QAPI schema order.  Else it takes them wrapped in the C struct
    generated for the complex argument type.  It takes an additional
    Error ** argument in either case.

> How about using a list[] to determine which attributes need to be
> reloaded["tls_certs", "authz*"...], and define an enum to ensure the
> validity of list elements.

Representing a set of named things as a "list of enum of thing names" is
workable.

It's a fairly rigid design, though.  When you start with "struct of bool
members", you can add members of other types, and the whole still looks
regular.  You can even evolve an existing bool into an alternate of bool
and something else.

What kind of evolution do you want to prepare for?  Two foolish answers
to avoid: "any and all, regardless of cost" (you should not brush off
cost like that, ever), and "none, because I can predict the future
confidently" (no, you can't).

Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Daniel P. Berrangé]

On Mon, Jan 18, 2021 at 03:27:33PM +0800, Zihao Chang wrote:
> 
> 
> On 2021/1/15 21:47, Daniel P. Berrangé wrote:
> > On Fri, Jan 15, 2021 at 02:37:33PM +0100, Markus Armbruster wrote:
> >> Zihao Chang <changzihao1@huawei.com> writes:
> >>
> >>> QEMU loads vnc tls certificates only when vm is started. This patch
> >>> provides a new qmp to reload vnc tls certificates without restart
> >>> vnc-server/VM.
> >>> {"execute": "reload-vnc-cert"}
> >>>
> >>> Signed-off-by: Zihao Chang <changzihao1@huawei.com>
> >>> ---
> >>>  include/ui/console.h |  1 +
> >>>  monitor/qmp-cmds.c   |  5 +++++
> >>>  qapi/ui.json         | 18 ++++++++++++++++++
> >>>  ui/vnc.c             | 24 ++++++++++++++++++++++++
> >>>  4 files changed, 48 insertions(+)
> >>>
> >>> diff --git a/include/ui/console.h b/include/ui/console.h
> >>> index 5dd21976a3..60a24a8bb5 100644
> >>> --- a/include/ui/console.h
> >>> +++ b/include/ui/console.h
> >>> @@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char *password);
> >>>  int vnc_display_pw_expire(const char *id, time_t expires);
> >>>  QemuOpts *vnc_parse(const char *str, Error **errp);
> >>>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> >>> +void vnc_display_reload_cert(const char *id,  Error **errp);
> >>
> >> Make this return bool, please.
> >>
> I will fix this in the next version.
> Thank your for your reviews.
> 
> >> error.h's big comment:
> >>
> >>  * = Rules =
> >>  *
> >>  * - Functions that use Error to report errors have an Error **errp
> >>  *   parameter.  It should be the last parameter, except for functions
> >>  *   taking variable arguments.
> >>  *
> >>  * - You may pass NULL to not receive the error, &error_abort to abort
> >>  *   on error, &error_fatal to exit(1) on error, or a pointer to a
> >>  *   variable containing NULL to receive the error.
> >>  *
> >>  * - Separation of concerns: the function is responsible for detecting
> >>  *   errors and failing cleanly; handling the error is its caller's
> >>  *   job.  Since the value of @errp is about handling the error, the
> >>  *   function should not examine it.
> >>  *
> >>  * - The function may pass @errp to functions it calls to pass on
> >>  *   their errors to its caller.  If it dereferences @errp to check
> >>  *   for errors, it must use ERRP_GUARD().
> >>  *
> >>  * - On success, the function should not touch *errp.  On failure, it
> >>  *   should set a new error, e.g. with error_setg(errp, ...), or
> >>  *   propagate an existing one, e.g. with error_propagate(errp, ...).
> >>  *
> >>  * - Whenever practical, also return a value that indicates success /
> >>  *   failure.  This can make the error checking more concise, and can
> >>  *   avoid useless error object creation and destruction.  Note that
> >>  *   we still have many functions returning void.  We recommend
> >>  *   • bool-valued functions return true on success / false on failure,
> >>  *   • pointer-valued functions return non-null / null pointer, and
> >>  *   • integer-valued functions return non-negative / negative.
> >>
> >>>  
> >>>  /* input.c */
> >>>  int index_from_key(const char *key, size_t key_length);
> >>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> >>> index 34f7e75b7b..84f2b74ea8 100644
> >>> --- a/monitor/qmp-cmds.c
> >>> +++ b/monitor/qmp-cmds.c
> >>> @@ -287,6 +287,11 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> >>>          qmp_change_vnc_listen(target, errp);
> >>>      }
> >>>  }
> >>> +
> >>> +void qmp_reload_vnc_cert(Error **errp)
> >>> +{
> >>> +    vnc_display_reload_cert(NULL, errp);
> >>> +}
> >>>  #endif /* !CONFIG_VNC */
> >>>  
> >>>  void qmp_change(const char *device, const char *target,
> >>> diff --git a/qapi/ui.json b/qapi/ui.json
> >>> index d08d72b439..855b1ac007 100644
> >>> --- a/qapi/ui.json
> >>> +++ b/qapi/ui.json
> >>> @@ -1179,3 +1179,21 @@
> >>>  ##
> >>>  { 'command': 'query-display-options',
> >>>    'returns': 'DisplayOptions' }
> >>> +
> >>> +##
> >>> +# @reload-vnc-cert:
> >>> +#
> >>> +# Reload certificates for vnc.
> >>> +#
> >>> +# Returns: nothing
> >>> +#
> >>> +# Since: 5.2
> >>
> >> 6.0 now.
> >>
> >>> +#
> >>> +# Example:
> >>> +#
> >>> +# -> { "execute": "reload-vnc-cert" }
> >>> +# <- { "return": {} }
> >>> +#
> >>> +##
> >>> +{ 'command': 'reload-vnc-cert',
> >>> +  'if': 'defined(CONFIG_VNC)' }
> >>
> >> Daniel's objection to another patch that adds a rather specialized QMP
> >> command may apply: "I'm not a fan of adding adhoc new commands for
> >> specific properties."
> >>
> >>     From: Daniel P. Berrangé <berrange@redhat.com>
> >>     Subject: Re: [PATCH] vnc: add qmp to support change authz
> >>     Message-ID: <20210107161830.GE1029501@redhat.com>
> > 
> > Yeah, though this scenario is a ittle more complicated. Tihs patch is
> > not actually changing any object properties in the VNC server config.
> > It is simply telling the VNC server to reload the existing object it
> > has configured.
> > 
> > My proposed  "display-update" command would not directly map to what
> > this "reload-vnc-cert" command does, unless we declared the certs are
> > always reloaded after every display-update command.
> > 
> > Or we could have a more generic  "display-reload" command, which has
> > fields indicating what aspect to reload. eg a 'tls-certs: bool' field
> > to indicate reload of TLS certs in the display. This could be useful
> > if we wanted the ability to reload authz access control lists, though
> > we did actually wire them up to auto-reload using inotify.
> > 
> > 
> > Regards,
> > Daniel
> > 
> 
> If we add field for each reloadable attribute(tls-certs, authz,...),
> the number of parameters for qmp_display_reload() may increase sharply
> (bool has_tls_certs, bool tls_certs, ... twice the number of attributes).

There's a fairly limited conceptual set of VNC features which are going
to require a reload operation, so I don't think it'll grow too unreasonably
large.

> How about using a list[] to determine which attributes need to be
> reloaded["tls_certs", "authz*"...], and define an enum to ensure the
> validity of list elements.

The enum is simple, but if we require data to be provided fr the
reload operation, instead of a simple boolean, then it gets a bit
more limiting.


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 v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Gerd Hoffmann]

  Hi,

> Or we could have a more generic  "display-reload" command, which has
> fields indicating what aspect to reload. eg a 'tls-certs: bool' field
> to indicate reload of TLS certs in the display. This could be useful
> if we wanted the ability to reload authz access control lists, though
> we did actually wire them up to auto-reload using inotify.

Maybe we should just use inotify-based reload for the certs too?

take care,
  Gerd

Re: [PATCH v2 2/2] vnc: add qmp to support reload vnc tls certificates

[Daniel P. Berrangé]

On Mon, Jan 18, 2021 at 05:13:16PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > Or we could have a more generic  "display-reload" command, which has
> > fields indicating what aspect to reload. eg a 'tls-certs: bool' field
> > to indicate reload of TLS certs in the display. This could be useful
> > if we wanted the ability to reload authz access control lists, though
> > we did actually wire them up to auto-reload using inotify.
> 
> Maybe we should just use inotify-based reload for the certs too?

The authz access control is easy because it is just one file.

When updating the certs though, we have 1-4 files that need loading, and
they can only be reloaded once all of them are updated on disk. This gives
a synchronization challenge for use of inotify, as when we see 1 updated,
we don't know if we need to wait for the others to be updated or not.


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 :|