[PATCH 1/2] qemu: add append mode config for serial file

[Martin Kletzander]

On Tue, Nov 29, 2022 at 09:54:51PM +0600, Oleg Vasilev wrote:
>Serial log file contains lots of useful information for debugging
>configuration problems. It makes sense to preserve the log in between
>restarts, so that one can later figure out what was going on.
>

I don't understand why is the default not set in the mgmt app which
creates the XMLs since we allow that per domain, but I'll go with that.

>Signed-off-by: Oleg Vasilev <oleg.vasilev at virtuozzo.com>
>---
> src/qemu/libvirtd_qemu.aug         |  3 +++
> src/qemu/qemu.conf.in              | 10 ++++++++++
> src/qemu/qemu_conf.c               |  4 ++++
> src/qemu/qemu_conf.h               |  2 ++
> src/qemu/qemu_domain.c             | 13 +++++++++++++
> src/qemu/test_libvirtd_qemu.aug.in |  1 +
> 6 files changed, 33 insertions(+)
>
>diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
>index ed097ea3d9..7f3eec7cfd 100644
>--- a/src/qemu/libvirtd_qemu.aug
>+++ b/src/qemu/libvirtd_qemu.aug
>@@ -147,6 +147,8 @@ module Libvirtd_qemu =
>
>    let capability_filters_entry = str_array_entry "capability_filters"
>
>+   let serial_file_append_entry = str_entry "serial_file_append"
>+
>    (* Each entry in the config is one of the following ... *)
>    let entry = default_tls_entry
>              | vnc_entry
>@@ -171,6 +173,7 @@ module Libvirtd_qemu =
>              | swtpm_entry
>              | capability_filters_entry
>              | obsolete_entry
>+             | serial_file_append_entry
>
>    let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
>    let empty = [ label "#empty" . eol ]
>diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
>index 3895d42514..6b38bbeca0 100644
>--- a/src/qemu/qemu.conf.in
>+++ b/src/qemu/qemu.conf.in
>@@ -968,3 +968,13 @@
> # "full"    -  both QEMU and its helper processes are placed into separate
> #              scheduling group
> #sched_core = "none"
>+
>+# Default append mode for writing to serial file. QEMU will set the chosen
>+# value everytime it processes the config, unless some value is already there.
>+#
>+# Possible options are:
>+# "default" - leave as-is, omit "append=..." from conf, leave for QEMU to decide.

I'd remove the 'omit "append=..." from conf, ' part as it might be
confusing for some.

>+#             The default for QEMU is the same as with append="off".

You mention the default is to leave QEMU to decide, but also specify the
default behaviour of QEMU itself.  That way, if there is any change,
however unlikely, we might get some users complaining that the default
is specified to be "off" while it is not.  I'd just leave out this one line.

>+# "on"      - set append value to "on", meaning file won't be truncated on restart
>+# "off"     - set append value to "off", file will be cleared on restart
>+#serial_file_append = "default"
>diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>index ae5bbcd138..bfd93a168f 100644
>--- a/src/qemu/qemu_conf.c
>+++ b/src/qemu/qemu_conf.c
>@@ -288,6 +288,7 @@ virQEMUDriverConfig *virQEMUDriverConfigNew(bool privileged,
>         return NULL;
>
>     cfg->deprecationBehavior = g_strdup("none");
>+    cfg->serialFileAppend = g_strdup("default");
>
>     return g_steal_pointer(&cfg);
> }
>@@ -376,6 +377,7 @@ static void virQEMUDriverConfigDispose(void *obj)
>     g_strfreev(cfg->capabilityfilters);
>
>     g_free(cfg->deprecationBehavior);
>+    g_free(cfg->serialFileAppend);
> }
>
>
>@@ -903,6 +905,8 @@ virQEMUDriverConfigLoadDebugEntry(virQEMUDriverConfig *cfg,
>         return -1;
>     if (virConfGetValueString(conf, "deprecation_behavior", &cfg->deprecationBehavior) < 0)
>         return -1;
>+    if (virConfGetValueString(conf, "serial_file_append", &cfg->serialFileAppend) < 0)
>+        return -1;
>
>     return 0;
> }
>diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>index 8cf2dd2ec5..316200f38d 100644
>--- a/src/qemu/qemu_conf.h
>+++ b/src/qemu/qemu_conf.h
>@@ -229,6 +229,8 @@ struct _virQEMUDriverConfig {
>     char *deprecationBehavior;
>
>     virQEMUSchedCore schedCore;
>+
>+    char *serialFileAppend;
> };
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref);
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 8ae458ae45..0d1fe1ffcc 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -37,6 +37,7 @@
> #include "qemu_validate.h"
> #include "qemu_namespace.h"
> #include "viralloc.h"
>+#include "virenum.h"
> #include "virlog.h"
> #include "virerror.h"
> #include "viridentity.h"
>@@ -5357,6 +5358,8 @@ qemuDomainChrDefPostParse(virDomainChrDef *chr,
>                           virQEMUDriver *driver,
>                           unsigned int parseFlags)
> {
>+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>+
>     /* Historically, isa-serial and the default matched, so in order to
>      * maintain backwards compatibility we map them here. The actual default
>      * will be picked below based on the architecture and machine type. */
>@@ -5428,6 +5431,16 @@ qemuDomainChrDefPostParse(virDomainChrDef *chr,
>         }
>     }
>
>+
>+    if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
>+        chr->source &&
>+        chr->source->type == VIR_DOMAIN_CHR_TYPE_FILE &&
>+        chr->source->data.file.append == VIR_TRISTATE_SWITCH_ABSENT) {
>+
>+        chr->source->data.file.append =
>+            virTristateSwitchTypeFromString(cfg->serialFileAppend);

If we translate this during startup then we don't have to do that on
every XML parse, but I see some other things are used the same way.

Couple of places do a change to default if there is an _ABSENT option,
maybe it'd be nicer to have something like virTristateSwitchMerge()
that'd do the same, but that's just my rambling, an idea for future
someone, unrelated to your patch.

>+    }
>+
>     return 0;
> }
>
>diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
>index 1dbd692921..d1ca9c8d3d 100644
>--- a/src/qemu/test_libvirtd_qemu.aug.in
>+++ b/src/qemu/test_libvirtd_qemu.aug.in
>@@ -117,3 +117,4 @@ module Test_libvirtd_qemu =
> }
> { "deprecation_behavior" = "none" }
> { "sched_core" = "none" }
>+{ "serial_file_append" = "default" }
>-- 
>2.38.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230105/e76fa417/attachment.sig>