Hi
On Wed, Jul 7, 2021 at 9:37 PM Steve Sistare <steven.sist...@oracle.com>
wrote:
> Add QEMU_CHAR_FEATURE_CPR for devices that support cpr.
> Add the chardev close_on_cpr option for devices that can be closed on cpr
> and reopened after exec.
> cpr is allowed only if either QEMU_CHAR_FEATURE_CPR or close_on_cpr is set
> for all chardevs in the configuration.
>
Why not do the right thing by default?
Could use some tests in tests/unit/test-char.c
> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
> ---
> chardev/char.c | 41 ++++++++++++++++++++++++++++++++++++++---
> include/chardev/char.h | 5 +++++
> migration/cpr.c | 3 +++
> qapi/char.json | 5 ++++-
> qemu-options.hx | 26 ++++++++++++++++++++++----
> 5 files changed, 72 insertions(+), 8 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index d959eec..f10fb94 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -36,6 +36,7 @@
> #include "qemu/help_option.h"
> #include "qemu/module.h"
> #include "qemu/option.h"
> +#include "qemu/env.h"
> #include "qemu/id.h"
> #include "qemu/coroutine.h"
> #include "qemu/yank.h"
> @@ -239,6 +240,9 @@ static void qemu_char_open(Chardev *chr,
> ChardevBackend *backend,
> ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> /* Any ChardevCommon member would work */
> ChardevCommon *common = backend ? backend->u.null.data : NULL;
> + char fdname[40];
>
Please use g_autoptr char *fdname = NULL; & g_strdup_printf()
+
> + chr->close_on_cpr = (common && common->close_on_cpr);
>
> if (common && common->has_logfile) {
> int flags = O_WRONLY | O_CREAT;
> @@ -248,7 +252,14 @@ static void qemu_char_open(Chardev *chr,
> ChardevBackend *backend,
> } else {
> flags |= O_TRUNC;
> }
> - chr->logfd = qemu_open_old(common->logfile, flags, 0666);
> + snprintf(fdname, sizeof(fdname), "%s_log", chr->label);
> + chr->logfd = getenv_fd(fdname);
> + if (chr->logfd < 0) {
> + chr->logfd = qemu_open_old(common->logfile, flags, 0666);
> + if (!chr->close_on_cpr) {
> + setenv_fd(fdname, chr->logfd);
> + }
> + }
> if (chr->logfd < 0) {
> error_setg_errno(errp, errno,
> "Unable to open logfile %s",
> @@ -300,11 +311,12 @@ static void char_finalize(Object *obj)
> if (chr->be) {
> chr->be->chr = NULL;
> }
> - g_free(chr->filename);
> - g_free(chr->label);
> if (chr->logfd != -1) {
> close(chr->logfd);
> + unsetenv_fdv("%s_log", chr->label);
> }
> + g_free(chr->filename);
> + g_free(chr->label);
> qemu_mutex_destroy(&chr->chr_write_lock);
> }
>
> @@ -504,6 +516,8 @@ void qemu_chr_parse_common(QemuOpts *opts,
> ChardevCommon *backend)
>
> backend->has_logappend = true;
> backend->logappend = qemu_opt_get_bool(opts, "logappend", false);
> +
> + backend->close_on_cpr = qemu_opt_get_bool(opts, "close-on-cpr",
> false);
>
If set to true and the backend doesn't implement the CPR feature, it should
raise an error.
}
>
> static const ChardevClass *char_get_class(const char *driver, Error
> **errp)
> @@ -945,6 +959,9 @@ QemuOptsList qemu_chardev_opts = {
> },{
> .name = "abstract",
> .type = QEMU_OPT_BOOL,
> + },{
> + .name = "close-on-cpr",
> + .type = QEMU_OPT_BOOL,
> #endif
> },
> { /* end of list */ }
> @@ -1212,6 +1229,24 @@ GSource *qemu_chr_timeout_add_ms(Chardev *chr,
> guint ms,
> return source;
> }
>
> +static int chr_cpr_capable(Object *obj, void *opaque)
> +{
> + Chardev *chr = (Chardev *)obj;
> + Error **errp = opaque;
> +
> + if (qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_CPR) ||
> chr->close_on_cpr) {
>
That'd be easy to misuse. Chardev should always explicitly support CPR
feature (even if close_on_cpr is set)
> + return 0;
> + }
> + error_setg(errp, "error: chardev %s -> %s is not capable of cpr",
> + chr->label, chr->filename);
> + return 1;
> +}
> +
> +bool qemu_chr_cpr_capable(Error **errp)
> +{
> + return !object_child_foreach(get_chardevs_root(), chr_cpr_capable,
> errp);
> +}
> +
> void qemu_chr_cleanup(void)
> {
> object_unparent(get_chardevs_root());
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 7c0444f..e488ad1 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -50,6 +50,8 @@ typedef enum {
> /* Whether the gcontext can be changed after calling
> * qemu_chr_be_update_read_handlers() */
> QEMU_CHAR_FEATURE_GCONTEXT,
> + /* Whether the device supports cpr */
> + QEMU_CHAR_FEATURE_CPR,
>
> QEMU_CHAR_FEATURE_LAST,
> } ChardevFeature;
> @@ -67,6 +69,7 @@ struct Chardev {
> int be_open;
> /* used to coordinate the chardev-change special-case: */
> bool handover_yank_instance;
> + bool close_on_cpr;
> GSource *gsource;
> GMainContext *gcontext;
> DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
> @@ -291,4 +294,6 @@ void resume_mux_open(void);
> /* console.c */
> void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error
> **errp);
>
> +bool qemu_chr_cpr_capable(Error **errp);
> +
> #endif
> diff --git a/migration/cpr.c b/migration/cpr.c
> index 6333988..feff97f 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -138,6 +138,9 @@ void cprexec(strList *args, Error **errp)
> error_setg(errp, "cprexec requires cprsave with restart mode");
> return;
> }
> + if (!qemu_chr_cpr_capable(errp)) {
> + return;
> + }
> if (vfio_cprsave(errp)) {
> return;
> }
> diff --git a/qapi/char.json b/qapi/char.json
> index adf2685..5efaf59 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -204,12 +204,15 @@
> # @logfile: The name of a logfile to save output
> # @logappend: true to append instead of truncate
> # (default to false to truncate)
> +# @close-on-cpr: if true, close device's fd on cprsave. defaults to false.
> +# since 6.1.
> #
> # Since: 2.6
> ##
> { 'struct': 'ChardevCommon',
> 'data': { '*logfile': 'str',
> - '*logappend': 'bool' } }
> + '*logappend': 'bool',
> + '*close-on-cpr': 'bool' } }
>
> ##
> # @ChardevFile:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index fa53734..d5ff45f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3134,43 +3134,57 @@ DEFHEADING(Character device options:)
>
> DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
> "-chardev help\n"
> - "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + "-chardev
> null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off][,close-on-cpr=on|off]\n"
> "-chardev
> socket,id=id[,host=host],port=port[,to=to][,ipv4=on|off][,ipv6=on|off][,nodelay=on|off][,reconnect=seconds]\n"
> "
>
> [,server=on|off][,wait=on|off][,telnet=on|off][,websocket=on|off][,reconnect=seconds][,mux=on|off]\n"
> - "
> [,logfile=PATH][,logappend=on|off][,tls-creds=ID][,tls-authz=ID] (tcp)\n"
> + "
>
> [,logfile=PATH][,logappend=on|off][,tls-creds=ID][,tls-authz=ID][,close-on-cpr=on|off]
> (tcp)\n"
> "-chardev
> socket,id=id,path=path[,server=on|off][,wait=on|off][,telnet=on|off][,websocket=on|off][,reconnect=seconds]\n"
> - "
>
> [,mux=on|off][,logfile=PATH][,logappend=on|off][,abstract=on|off][,tight=on|off]
> (unix)\n"
> + "
>
> [,mux=on|off][,logfile=PATH][,logappend=on|off][,abstract=on|off][,tight=on|off][,close-on-cpr=on|off]
> (unix)\n"
> "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n"
> "
> [,localport=localport][,ipv4=on|off][,ipv6=on|off][,mux=on|off]\n"
> - " [,logfile=PATH][,logappend=on|off]\n"
> + " [,logfile=PATH][,logappend=on|off][,close-on-cpr=on|off]\n"
> "-chardev
> msmouse,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> vc,id=id[[,width=width][,height=height]][[,cols=cols][,rows=rows]]\n"
> " [,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> ringbuf,id=id[,size=size][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> file,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> pipe,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> #ifdef _WIN32
> "-chardev
> console,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> "-chardev
> serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> #else
> "-chardev pty,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> stdio,id=id[,mux=on|off][,signal=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> #endif
> #ifdef CONFIG_BRLAPI
> "-chardev
> braille,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> #endif
> #if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
> || defined(__NetBSD__) || defined(__OpenBSD__) ||
> defined(__DragonFly__)
> "-chardev
> serial,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> tty,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> #endif
> #if defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__)
> "-chardev
> parallel,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> parport,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> #endif
> #if defined(CONFIG_SPICE)
> "-chardev
> spicevmc,id=id,name=name[,debug=debug][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> "-chardev
> spiceport,id=id,name=name[,debug=debug][,logfile=PATH][,logappend=on|off]\n"
> + " [,close-on-cpr=on|off]\n"
> #endif
> , QEMU_ARCH_ALL
> )
> @@ -3245,6 +3259,10 @@ The general form of a character device option is:
> ``logappend`` option controls whether the log file will be truncated
> or appended to when opened.
>
> + Every backend supports the ``close-on-cpr`` option. If on, the
> + devices's descriptor is closed during cprsave, and reopened after
> exec.
> + This is useful for devices that do not support cpr.
> +
> The available backends are:
>
> ``-chardev null,id=id``
> --
> 1.8.3.1
>
>
>
--
Marc-André Lureau
