qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an impli


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 2/9] chardev: mark the calls that allow an implicit mux monitor
Date: Thu, 30 Aug 2018 16:55:49 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)

Marc-André Lureau <address@hidden> writes:

> This is mostly for readability of the code. Let's make it clear which
> callers can create an implicit monitor when the chardev is muxed.
>
> This will also enforce a safer behaviour, as we don't really support
> creating monitor anywhere/anytime at the moment. Add an assert() to
> make sure the programmer explicitely wanted that behaviour.
>
> There are documented cases, such as: -serial/-parallel/-virtioconsole
> and to less extent -debugcon.
>
> Less obvious and questionable ones are -gdb, SLIRP -guestfwd and Xen
> console. Add a FIXME note for those, but keep the support for now.
>
> Other qemu_chr_new() callers either have a fixed parameter/filename
> string or do not need it, such as -qtest:
>
> * qtest.c: qtest_init()
>   Afaik, only used by tests/libqtest.c, without mux. I don't think we
>   support it outside of qemu testing: drop support for implicit mux
>   monitor (qemu_chr_new() call: no implicit mux now).
>
> * hw/
>   All with literal @filename argument that doesn't enable mux monitor.
>
> * tests/
>   All with @filename argument that doesn't enable mux monitor.
>
> On a related note, the list of monitor creation places:
>
> - the chardev creators listed above: all from command line (except
>   perhaps Xen console?)
>
> - -gdb & hmp gdbserver will create a "GDB monitor command" chardev
>   that is wired to an HMP monitor.
>
> - -mon command line option
>
> From this short study, I would like to think that a monitor may only
> be created in the main thread today, though I remain skeptical :)
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/chardev/char.h | 24 ++++++++++++++++++++----
>  chardev/char.c         | 32 +++++++++++++++++++++++++-------
>  gdbstub.c              |  6 +++++-
>  hw/char/xen_console.c  |  5 ++++-
>  net/slirp.c            |  5 ++++-
>  vl.c                   | 10 +++++-----
>  6 files changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 3e4fe6dad0..268daaa90b 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -105,14 +105,27 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>   * @filename: the URI
>   *
>   * Create a new character backend from a URI.
> + * Do not implicitly initialize a monitor if the chardev is muxed.
>   *
>   * Returns: a new character backend
>   */
>  Chardev *qemu_chr_new(const char *label, const char *filename);
>  
>  /**
> - * qemu_chr_change:
> - * @opts: the new backend options
> + * qemu_chr_new_mux_mon:
> + * @label: the name of the backend
> + * @filename: the URI
> + *
> + * Create a new character backend from a URI.
> + * Implicitly initialize a monitor if the chardev is muxed.
> + *
> + * Returns: a new character backend
> + */
> +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> +
> +/**
> +* qemu_chr_change:
> +* @opts: the new backend options
>   *
>   * Change an existing character backend
>   */
> @@ -129,6 +142,7 @@ void qemu_chr_cleanup(void);
>   * qemu_chr_new_noreplay:
>   * @label: the name of the backend
>   * @filename: the URI
> + * @with_mux_mon: if chardev is muxed, initialize a monitor
>   *
>   * Create a new character backend from a URI.
>   * Character device communications are not written
> @@ -136,7 +150,8 @@ void qemu_chr_cleanup(void);
>   *
>   * Returns: a new character backend
>   */
> -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename);
> +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> +                               bool with_mux_mon);
>  
>  /**
>   * qemu_chr_be_can_write:
> @@ -194,7 +209,8 @@ bool qemu_chr_has_feature(Chardev *chr,
>                            ChardevFeature feature);
>  void qemu_chr_set_feature(Chardev *chr,
>                            ChardevFeature feature);
> -QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
> +QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
> +                                bool with_mux_mon);
>  int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>  #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
>  int qemu_chr_wait_connected(Chardev *chr, Error **errp);
> diff --git a/chardev/char.c b/chardev/char.c
> index 76d866e6fe..c1b89b6638 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -329,7 +329,8 @@ int qemu_chr_wait_connected(Chardev *chr, Error **errp)
>      return 0;
>  }
>  
> -QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename)
> +QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
> +                                bool with_mux_mon)
>  {
>      char host[65], port[33], width[8], height[8];
>      int pos;
> @@ -344,6 +345,10 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const 
> char *filename)
>      }
>  
>      if (strstart(filename, "mon:", &p)) {
> +        if (!with_mux_mon) {
> +            error_report("mon: isn't supported in this context");
> +            return NULL;
> +        }
>          filename = p;
>          qemu_opt_set(opts, "mux", "on", &error_abort);
>          if (strcmp(filename, "stdio") == 0) {

Perhaps @permit_mux_mon would be a better name.  Your choice.

> @@ -683,7 +688,8 @@ out:
>      return chr;
>  }
>  
> -Chardev *qemu_chr_new_noreplay(const char *label, const char *filename)
> +Chardev *qemu_chr_new_noreplay(const char *label, const char *filename,
> +                               bool with_mux_mon)
>  {
>      const char *p;
>      Chardev *chr;
> @@ -694,25 +700,27 @@ Chardev *qemu_chr_new_noreplay(const char *label, const 
> char *filename)
>          return qemu_chr_find(p);
>      }
>  
> -    opts = qemu_chr_parse_compat(label, filename);
> +    opts = qemu_chr_parse_compat(label, filename, with_mux_mon);
>      if (!opts)
>          return NULL;
>  
>      chr = qemu_chr_new_from_opts(opts, &err);
>      if (err) {
>          error_report_err(err);
> -    }
> -    if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> +    } else if (qemu_opt_get_bool(opts, "mux", 0)) {
> +        assert(with_mux_mon);
>          monitor_init(chr, MONITOR_USE_READLINE);
>      }
>      qemu_opts_del(opts);
>      return chr;
>  }

Took me a second look to understand.  I'd prefer

       chr = qemu_chr_new_from_opts(opts, &err);
       if (!chr) {
           error_report_err(err);
           goto out;
       }
       if (qemu_opt_get_bool(opts, "mux", 0)) {
           assert(with_mux_mon);
           monitor_init(chr, MONITOR_USE_READLINE);
       }

   out:
       qemu_opts_del(opts);
       return chr;

or

       mux = qemu_opt_get_bool(opts, "mux", 0);
       chr = qemu_chr_new_from_opts(opts, &err);
       qemu_opts_del(opts);
       if (!chr) {
           error_report_err(err);
           return NULL;
       }
       if (mux) {
           monitor_init(chr, MONITOR_USE_READLINE);
       }
       return chr;

Your choice, of course.

>  
> -Chardev *qemu_chr_new(const char *label, const char *filename)
> +static Chardev *qemu_chr_new_with_mux_mon(const char *label,
> +                                          const char *filename,
> +                                          bool with_mux_mon)
>  {
>      Chardev *chr;
> -    chr = qemu_chr_new_noreplay(label, filename);
> +    chr = qemu_chr_new_noreplay(label, filename, with_mux_mon);
>      if (chr) {
>          if (replay_mode != REPLAY_MODE_NONE) {
>              qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_REPLAY);
> @@ -726,6 +734,16 @@ Chardev *qemu_chr_new(const char *label, const char 
> *filename)
>      return chr;
>  }
>  
> +Chardev *qemu_chr_new(const char *label, const char *filename)
> +{
> +    return qemu_chr_new_with_mux_mon(label, filename, false);
> +}
> +
> +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename)
> +{
> +    return qemu_chr_new_with_mux_mon(label, filename, true);
> +}
> +
>  static int qmp_query_chardev_foreach(Object *obj, void *data)
>  {
>      Chardev *chr = CHARDEV(obj);
> diff --git a/gdbstub.c b/gdbstub.c
> index d6ab95006c..c8478de8f5 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2038,7 +2038,11 @@ int gdbserver_start(const char *device)
>              sigaction(SIGINT, &act, NULL);
>          }
>  #endif
> -        chr = qemu_chr_new_noreplay("gdb", device);
> +        /*
> +         * FIXME: it's a bit weird to allow using a mux chardev here
> +         * and implicitly setup a monitor. We may want to break this.
> +         */
> +        chr = qemu_chr_new_noreplay("gdb", device, true);
>          if (!chr)
>              return -1;
>      }
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index 8b4b4bf523..6a231d487b 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -207,7 +207,10 @@ static int con_init(struct XenDevice *xendev)
>      } else {
>          snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
>          qemu_chr_fe_init(&con->chr,
> -                         qemu_chr_new(label, output), &error_abort);
> +                         /*
> +                          * FIXME: should it support implicit muxed monitors?

This sounds like it didn't, but perhaps it should.  Actually, it does,
but perhaps it shouldn't.  Suggest something like "FIXME sure we want to
support implicit muxed monitors here".

> +                          */
> +                         qemu_chr_new_mux_mon(label, output), &error_abort);
>      }
>  
>      xenstore_store_pv_console_info(con->xendev.dev,
> diff --git a/net/slirp.c b/net/slirp.c
> index 1e14318b4d..677dc36fe4 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -836,7 +836,10 @@ static int slirp_guestfwd(SlirpState *s, const char 
> *config_str,
>          }
>      } else {
>          Error *err = NULL;
> -        Chardev *chr = qemu_chr_new(buf, p);
> +        /*
> +         * FIXME: should it support implicit muxed monitors?
> +         */

Likewise.

> +        Chardev *chr = qemu_chr_new_mux_mon(buf, p);
>  
>          if (!chr) {
>              error_setg(errp, "Could not open guest forwarding device '%s'",
> diff --git a/vl.c b/vl.c
> index 5ba06adf78..b38e49ca43 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2353,7 +2353,7 @@ static void monitor_parse(const char *optarg, const 
> char *mode, bool pretty)
>      } else {
>          snprintf(label, sizeof(label), "compat_monitor%d",
>                   monitor_device_index);
> -        opts = qemu_chr_parse_compat(label, optarg);
> +        opts = qemu_chr_parse_compat(label, optarg, true);
>          if (!opts) {
>              error_report("parse error: %s", optarg);
>              exit(1);
> @@ -2425,7 +2425,7 @@ static int serial_parse(const char *devname)
>      snprintf(label, sizeof(label), "serial%d", index);
>      serial_hds = g_renew(Chardev *, serial_hds, index + 1);
>  
> -    serial_hds[index] = qemu_chr_new(label, devname);
> +    serial_hds[index] = qemu_chr_new_mux_mon(label, devname);
>      if (!serial_hds[index]) {
>          error_report("could not connect serial device"
>                       " to character backend '%s'", devname);
> @@ -2461,7 +2461,7 @@ static int parallel_parse(const char *devname)
>          exit(1);
>      }
>      snprintf(label, sizeof(label), "parallel%d", index);
> -    parallel_hds[index] = qemu_chr_new(label, devname);
> +    parallel_hds[index] = qemu_chr_new_mux_mon(label, devname);
>      if (!parallel_hds[index]) {
>          error_report("could not connect parallel device"
>                       " to character backend '%s'", devname);
> @@ -2492,7 +2492,7 @@ static int virtcon_parse(const char *devname)
>      qemu_opt_set(dev_opts, "driver", "virtconsole", &error_abort);
>  
>      snprintf(label, sizeof(label), "virtcon%d", index);
> -    virtcon_hds[index] = qemu_chr_new(label, devname);
> +    virtcon_hds[index] = qemu_chr_new_mux_mon(label, devname);
>      if (!virtcon_hds[index]) {
>          error_report("could not connect virtio console"
>                       " to character backend '%s'", devname);
> @@ -2508,7 +2508,7 @@ static int debugcon_parse(const char *devname)
>  {
>      QemuOpts *opts;
>  
> -    if (!qemu_chr_new("debugcon", devname)) {
> +    if (!qemu_chr_new_mux_mon("debugcon", devname)) {
>          exit(1);
>      }
>      opts = qemu_opts_create(qemu_find_opts("device"), "debugcon", 1, NULL);

I have a couple of suggestions, but nothing to withhold my
Reviewed-by: Markus Armbruster <address@hidden>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]