Hi
On Thu, Aug 30, 2018 at 4:58 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
> > ---
> > 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.
ok
>
> > @@ -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.
I prefer the first version, ok
>
> >
> > -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".
>
ok
> > + */
> > + 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 <arm...@redhat.com>
>
thanks
--
Marc-André Lureau
