Hi On Mon, Jul 12, 2021 at 11:20 PM Steven Sistare <steven.sist...@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.sist...@oracle.com > <mailto: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? > > 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.sist...@oracle.com <mailto: > 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() > > 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 > -- Marc-André Lureau
