Re: [PATCH V5 20/25] chardev: cpr framework

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 14731 bytes --]

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
>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 19473 bytes --]