Hi On Wed, Jul 7, 2021 at 9:37 PM Steve Sistare 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 > --- > 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