| Message ID | 1625678434-240960-21-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live Update | expand |
Hi On Wed, Jul 7, 2021 at 9:37 PM Steve Sistare <steven.sistare@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.sistare@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 > > >
On 7/8/2021 12:03 PM, Marc-André Lureau wrote: > Hi > > On Wed, Jul 7, 2021 at 9:37 PM Steve Sistare <steven.sistare@oracle.com <mailto:steven.sistare@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? Char devices with buffering in the qemu process do not support cpr, as there is no general mechanism for saving and restoring the buffer and synchronizing that with device operation. In theory vmstate could provide that mechanism, but sync'ing the device with vmstate operations would be non-trivial, as every device handles it differently, and I did not tackle it. However, some very useful devices do not buffer, and do support cpr, so I introduce QEMU_CHAR_FEATURE_CPR to identify them. CPR support can be incrementally added to more devices in the future via this mechanism. > Could use some tests in tests/unit/test-char.c OK, I'll check it out. I have deferred adding unit tests until I get more buy in on the patch series. > Signed-off-by: Steve Sistare <steven.sistare@oracle.com <mailto:steven.sistare@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() Will do. (the glibc functions are new to me, and my fingers do not automatically type them). > + > + 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. Setting to true is the workaround for missing CPR support, so that cpr may still be performed. The device will be reopened post exec. That is not as nice as transparently preserving the device, but is nicer than disallowing cpr because some device(s) of many do not support it. > } > > 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) Given my explanation at top, does this make sense now? - Steve > + 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
Hi On Mon, Jul 12, 2021 at 11:20 PM Steven Sistare <steven.sistare@oracle.com> wrote: > On 7/8/2021 12:03 PM, Marc-André Lureau wrote: > > Hi > > > > On Wed, Jul 7, 2021 at 9:37 PM Steve Sistare <steven.sistare@oracle.com > <mailto:steven.sistare@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? > > Char devices with buffering in the qemu process do not support cpr, as > there is no general mechanism > for saving and restoring the buffer and synchronizing that with device > operation. In theory vmstate > could provide that mechanism, but sync'ing the device with vmstate > operations would be non-trivial, > as every device handles it differently, and I did not tackle it. However, > some very useful devices > do not buffer, and do support cpr, so I introduce QEMU_CHAR_FEATURE_CPR to > identify them. CPR support > can be incrementally added to more devices in the future via this > mechanism. > > > Could use some tests in tests/unit/test-char.c > > OK, I'll check it out. I have deferred adding unit tests until I get more > buy in on the patch series. > I understand :) Tbh, I have no clue if you are close to acceptance. (too late for 6.1 anyway, you can already update the docs) > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com <mailto: > steven.sistare@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() > > Will do. > (the glibc functions are new to me, and my fingers do not automatically > type them). > > > + > > + 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. > > Setting to true is the workaround for missing CPR support, so that cpr may > still be performed. > The device will be reopened post exec. That is not as nice as > transparently preserving the device, > but is nicer than disallowing cpr because some device(s) of many do not > support it. > ok, "reopen-on-cpr" would be more descriptive then. > > } > > > > 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) > > Given my explanation at top, does this make sense now? > I think I understand the purpose, but it feels quite adventurous to rely on this behaviour by default, even if the feature flag is set. Could it require both FEATURE_CPR && reopen-on-cpr? > - Steve > > > > + 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 >
On 7/12/2021 3:49 PM, Marc-André Lureau wrote: > Hi > > On Mon, Jul 12, 2021 at 11:20 PM Steven Sistare <steven.sistare@oracle.com <mailto:steven.sistare@oracle.com>> wrote: > > On 7/8/2021 12:03 PM, Marc-André Lureau wrote: > > Hi > > > > On Wed, Jul 7, 2021 at 9:37 PM Steve Sistare <steven.sistare@oracle.com <mailto:steven.sistare@oracle.com> <mailto:steven.sistare@oracle.com <mailto:steven.sistare@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? > > Char devices with buffering in the qemu process do not support cpr, as there is no general mechanism > for saving and restoring the buffer and synchronizing that with device operation. In theory vmstate > could provide that mechanism, but sync'ing the device with vmstate operations would be non-trivial, > as every device handles it differently, and I did not tackle it. However, some very useful devices > do not buffer, and do support cpr, so I introduce QEMU_CHAR_FEATURE_CPR to identify them. CPR support > can be incrementally added to more devices in the future via this mechanism. > > > Could use some tests in tests/unit/test-char.c > > OK, I'll check it out. I have deferred adding unit tests until I get more buy in on the patch series. > > > I understand :) Tbh, I have no clue if you are close to acceptance. (too late for 6.1 anyway, you can already update the docs) > > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com <mailto:steven.sistare@oracle.com> <mailto:steven.sistare@oracle.com <mailto:steven.sistare@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() > > Will do. > (the glibc functions are new to me, and my fingers do not automatically type them). > > > + > > + 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. > > Setting to true is the workaround for missing CPR support, so that cpr may still be performed. > The device will be reopened post exec. That is not as nice as transparently preserving the device, > but is nicer than disallowing cpr because some device(s) of many do not support it. > > > ok, "reopen-on-cpr" would be more descriptive then. > > > > } > > > > 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) > > Given my explanation at top, does this make sense now? > > I think I understand the purpose, but it feels quite adventurous to rely on this behaviour by default, even if the feature flag is set. Could it require both FEATURE_CPR && reopen-on-cpr? I'm not following your point. Let me elaborate in case my intent is not clear. I use your suggested name reopen-on-cpr, but the functionality is otherwise the same: reopen-on-cpr is false by default for all devices, and QEMU_CHAR_FEATURE_CPR is true only for devices that operate properly when their descriptor is preserved across exec. cpr is blocked if any device exists that does not support the feature, and is not marked as reopen-on-cpr. The user must explicitly add close-on-cpr=on to the device parameters to allow cpr to proceed for such a device. For devices that do support QEMU_CHAR_FEATURE_CPR, the user may still set reopen-on-cpr=true to ignore the feature and recreate the device in its initial state after exec. Are you suggesting that the user should opt-in to using QEMU_CHAR_FEATURE_CPR? Such as via a more expressive flag? -chardev ...[,cpr=on|off|reopen] on: allow cpr iff the device has QEMU_CHAR_FEATURE_CPR off: do not allow cpr reopen: allow cpr. reopen the device after exec. chr_cpr_capable(Object *obj, void *opaque) if ((qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_CPR) && chr->cpr == ON) || chr->cpr == RESTART) return 0; /* cpr is allowed */ - Steve > > + 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 > > > > -- > Marc-André Lureau
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]; + + 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); } 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) { + 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``
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. Signed-off-by: Steve Sistare <steven.sistare@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(-)