[libvirt] [PATCH v3] qemu: Check for existence of provided *_tls_x509_cert_dir

[Ján Tomko]

On Fri, Jul 21, 2017 at 02:23:25PM -0400, John Ferlan wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1458630
>
>Introduce virQEMUDriverConfigTLSDirValidateResetDefault to validate
>that if any of the *_tls_x509_cert_dir values were set properly and
>reset the default value if the default_tls_x509_cert_dir changed.
>
>Update the qemu.conf description for default to describe the consequences
>if the default directory path does not exist.
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
>

[...]

>
> src/qemu/qemu.conf     |  8 +++++++
> src/qemu/qemu_conf.c   | 65 +++++++++++++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_conf.h   |  4 ++++
> src/qemu/qemu_driver.c |  3 +++
> 4 files changed, 79 insertions(+), 1 deletion(-)
>

[...]

>diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>index 6f44cbf..87d2c2d 100644
>--- a/src/qemu/qemu_conf.c
>+++ b/src/qemu/qemu_conf.c

[...]

>@@ -872,6 +873,68 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>     return ret;
> }
>
>+
>+/**
>+ * @cfg: Recently config values
>+ *
>+ * Validate the recently read *_tls_x509_cert_dir values and if necessary
>+ * update the default value to match the default_tls_x509_cert_dir
>+ *
>+ * Returns 0 on success, -1 on failure
>+ */
>+int
>+virQEMUDriverConfigTLSDirValidateResetDefault(virQEMUDriverConfigPtr cfg)

Or just virQEMUDriverConfigValidate, for future expansion.

>+{
>+    bool newDefault = false;
>+
>+    /* If the default entry was uncommented, then validate existence */
>+    if (cfg->checkdefaultTLSx509certdir) {
>+        if (!virFileExists(cfg->defaultTLSx509certdir)) {
>+            virReportError(VIR_ERR_CONF_SYNTAX,
>+                           _("default_tls_x509_cert_dir directory '%s' "
>+                             "does not exist"),
>+                           cfg->defaultTLSx509certdir);
>+            return -1;
>+        }
>+        if (STRNEQ(cfg->defaultTLSx509certdir, SYSCONFDIR "/pki/qemu"))
>+            newDefault = true;

This bool is only used to decide if we need to stdup the value to the
more specific directory variables. If the user took the time to put
the default value in the uncommented variable, it's only fair to have
them suffer the performance penalty of one extra strdup in exchange for
making this code clearer by dropping this bool.

cfg->checkdefaultTLSx509certdir can be used (or renamed to
'defaultTLSx509certdirProvided'.
Alternatively, the values can be left at NULL in virQEMUDriverConfigNew,
this function would only validate the user-provided values and the
defaults would be filled in by another function. (This has the benefit
of the TLSdir logic being in one place. On the other hand, virQEMUDriverConfigNew
would not give us a complete default config)

>+    }
>+
>+    /* We know virQEMUDriverConfigNew set the particular value to either
>+     * it's default or default_tls_x509_cert_dir's default. So, if not the
>+     * default default and the directory doesn't exist, then the entry was
>+     * set in the config file to something that doesn't exist, so error.
>+     *
>+     * Also, if the defaultTLSx509certdir value was changed from the default,
>+     * then we need to update the default for each setting as well to match
>+     * the default_tls_x509_cert_dir.
>+     */
>+#define VALIDATE_TLS_X509_CERT_DIR(val)                                   \
>+    do {                                                                  \
>+        if (STRNEQ(cfg->val ## TLSx509certdir, SYSCONFDIR "/pki/qemu") && \

Okay, here we assume that SYSCONFDIR "/pki/qemu" means the value was
untouched by the user for the simplicity of the code.

>+            !virFileExists(cfg->val ## TLSx509certdir)) {                 \
>+            virReportError(VIR_ERR_CONF_SYNTAX,                           \
>+                           _(#val"_tls_x509_cert_dir directory '%s' "     \
>+                             "does not exist"),                           \

This translatable string is untranslatable:
msgid "_tls_x509_cert_dir directory '%s' does not exist"

Either supply the whole variable name, or abolish macros and open-code
the error message like we do for {state,lib,cache}Dir in
qemuStateInitialize.

>+                           cfg->val ## TLSx509certdir);                   \
>+            return -1;                                                    \

>+        } else if (newDefault) {                                          \
>+            VIR_FREE(cfg->val ## TLSx509certdir);                         \
>+            if (VIR_STRDUP(cfg->val ## TLSx509certdir,                    \
>+                           cfg->defaultTLSx509certdir) < 0)               \
>+                return -1;                                                \
>+        }                                                                 \

This should not be in a macro called VALIDATE.
Maybe virQEMUDriverConfigTLSDirSetDefaults?

>+    } while (0)
>+
>+    VALIDATE_TLS_X509_CERT_DIR(vnc);
>+    VALIDATE_TLS_X509_CERT_DIR(spice);
>+    VALIDATE_TLS_X509_CERT_DIR(chardev);
>+    VALIDATE_TLS_X509_CERT_DIR(migrate);
>+
>+    return 0;
>+}
>+
>+
> virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver)
> {
>     virQEMUDriverConfigPtr conf;

[...]

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170726/92a52f28/attachment-0001.sig>