Re: [Xen-devel] [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor
Hi
On Mon, Aug 27, 2018 at 10:10 AM Markus Armbruster wrote:
>
> Marc-André Lureau writes:
>
> > Hi
> > On Fri, Aug 24, 2018 at 9:37 AM Markus Armbruster wrote:
> >>
> >> Marc-André Lureau 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.
> >> >
> >> > There are documented cases, such as: -serial/-parallel/-virtioconsole
> >> > and to less extent -debugcon.
> >> >
> >> > Less obvious and questionnable ones are -gdb 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 preliminary checks on the string (such as slirp), or do
> >> > not need it, such as -qtest.
> >> >
> >> > 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
> >>
> >> Aside: the question "what does it mean to connect a monitor to a chardev
> >> that has an implicit monitor" crosses my mind. Next thought is "the
> >> day's too short to go there".
> >>
> >> > 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 :)
> >>
> >> Hah!
> >>
> >> > Signed-off-by: Marc-André Lureau
> >> > ---
> >> > include/chardev/char.h | 18 +-
> >> > chardev/char.c | 21 +
> >> > gdbstub.c | 6 +-
> >> > hw/char/xen_console.c | 5 -
> >> > vl.c | 8
> >> > 5 files changed, 47 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> >> > index 6f0576e214..be2b9b817e 100644
> >> > --- a/
> >> > +++ b/include/chardev/char.h
> >> > @@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> >> > * @qemu_chr_new:
> >> > *
> >> > * Create a new character backend from a URI.
> >> > + * Do not implicitely initialize a monitor if the chardev is muxed.
> >> > *
> >> > * @label the name of the backend
> >> > * @filename the URI
> >> > @@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> >> > */
> >> > Chardev *qemu_chr_new(const char *label, const char *filename);
> >> >
> >> > +/**
> >> > + * @qemu_chr_new_mux_mon:
> >> > + *
> >> > + * Create a new character backend from a URI.
> >> > + * Implicitely initialize a monitor if the chardev is muxed.
> >> > + *
> >> > + * @label the name of the backend
> >> > + * @filename the URI
> >>
> >> I'm no friend of GTK-Doc style, but where we use it, we should use it
> >> correctly: put a colon after @param.
> >
> > I copied existing comment... Should I fixed all others in the headers?
>
> Do we expect to actually *use* doc comments for anything? We haven't in
> a decade or so, but if we still expect to all the same, then not fixing
> them up now merely delays the inevitable.
>
> Do we think adding the colons makes the comments easier to read? For
> what it's worth, I do.
>
> Decision's up to you.
Let's improve it.
>
> >> > + *
> >> > + * Returns: a new character backend
> >> > + */
> >> > +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> >> > +
> >> > /**
> >> > * @qemu_chr_change:
> >> > *
> >> > @@ -138,10 +152,12 @@ void qemu_chr_cleanup(void);
> >> > *
> >> > * @label the name of the backend
> >> > * @filename the URI
> >> > + * @with_mux_mon if chardev is muxed, initialize a monitor
> >> > *
> >> > * 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:
> >> > diff --git a/chardev/char.c b/chardev/char.c
> >> > index 76d866e6fe..2c295ad676 100644
> >> > --- a/chardev/char.c
> >> > +++ b/chardev/char.c
> >> > @@ -683,7 +683,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;
> >> > @@ -702,17 +703,19 @@ Chardev *qemu_chr_new_noreplay(const char *label,
> >> > const char *filename)
> >> > if (err) {
> >> > error_report_err(err);
> >> > }
> >> > -if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
>
Re: [Xen-devel] [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor
Marc-André Lureau writes:
> Hi
> On Fri, Aug 24, 2018 at 9:37 AM Markus Armbruster wrote:
>>
>> Marc-André Lureau 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.
>> >
>> > There are documented cases, such as: -serial/-parallel/-virtioconsole
>> > and to less extent -debugcon.
>> >
>> > Less obvious and questionnable ones are -gdb 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 preliminary checks on the string (such as slirp), or do
>> > not need it, such as -qtest.
>> >
>> > 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
>>
>> Aside: the question "what does it mean to connect a monitor to a chardev
>> that has an implicit monitor" crosses my mind. Next thought is "the
>> day's too short to go there".
>>
>> > 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 :)
>>
>> Hah!
>>
>> > Signed-off-by: Marc-André Lureau
>> > ---
>> > include/chardev/char.h | 18 +-
>> > chardev/char.c | 21 +
>> > gdbstub.c | 6 +-
>> > hw/char/xen_console.c | 5 -
>> > vl.c | 8
>> > 5 files changed, 47 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/include/chardev/char.h b/include/chardev/char.h
>> > index 6f0576e214..be2b9b817e 100644
>> > --- a/
>> > +++ b/include/chardev/char.h
>> > @@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>> > * @qemu_chr_new:
>> > *
>> > * Create a new character backend from a URI.
>> > + * Do not implicitely initialize a monitor if the chardev is muxed.
>> > *
>> > * @label the name of the backend
>> > * @filename the URI
>> > @@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>> > */
>> > Chardev *qemu_chr_new(const char *label, const char *filename);
>> >
>> > +/**
>> > + * @qemu_chr_new_mux_mon:
>> > + *
>> > + * Create a new character backend from a URI.
>> > + * Implicitely initialize a monitor if the chardev is muxed.
>> > + *
>> > + * @label the name of the backend
>> > + * @filename the URI
>>
>> I'm no friend of GTK-Doc style, but where we use it, we should use it
>> correctly: put a colon after @param.
>
> I copied existing comment... Should I fixed all others in the headers?
Do we expect to actually *use* doc comments for anything? We haven't in
a decade or so, but if we still expect to all the same, then not fixing
them up now merely delays the inevitable.
Do we think adding the colons makes the comments easier to read? For
what it's worth, I do.
Decision's up to you.
>> > + *
>> > + * Returns: a new character backend
>> > + */
>> > +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
>> > +
>> > /**
>> > * @qemu_chr_change:
>> > *
>> > @@ -138,10 +152,12 @@ void qemu_chr_cleanup(void);
>> > *
>> > * @label the name of the backend
>> > * @filename the URI
>> > + * @with_mux_mon if chardev is muxed, initialize a monitor
>> > *
>> > * 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:
>> > diff --git a/chardev/char.c b/chardev/char.c
>> > index 76d866e6fe..2c295ad676 100644
>> > --- a/chardev/char.c
>> > +++ b/chardev/char.c
>> > @@ -683,7 +683,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;
>> > @@ -702,17 +703,19 @@ Chardev *qemu_chr_new_noreplay(const char *label,
>> > const char *filename)
>> > if (err) {
>> > error_report_err(err);
>> > }
>> > -if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
>> > +if (with_mux_mon && chr && qemu_opt_get_bool(opts, "mux", 0)) {
>> > monitor_init(chr, MONITOR_USE_READLINE);
>> > }
>>
>> When !chr, the function already failed, so that part of the condition is
>> uninteresting here[*].
>
> yeah, hopefully err is always set if the return value is NULL.
>
>> That leaves
Re: [Xen-devel] [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor
Hi
On Fri, Aug 24, 2018 at 9:37 AM Markus Armbruster wrote:
>
> Marc-André Lureau 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.
> >
> > There are documented cases, such as: -serial/-parallel/-virtioconsole
> > and to less extent -debugcon.
> >
> > Less obvious and questionnable ones are -gdb 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 preliminary checks on the string (such as slirp), or do
> > not need it, such as -qtest.
> >
> > 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
>
> Aside: the question "what does it mean to connect a monitor to a chardev
> that has an implicit monitor" crosses my mind. Next thought is "the
> day's too short to go there".
>
> > 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 :)
>
> Hah!
>
> > Signed-off-by: Marc-André Lureau
> > ---
> > include/chardev/char.h | 18 +-
> > chardev/char.c | 21 +
> > gdbstub.c | 6 +-
> > hw/char/xen_console.c | 5 -
> > vl.c | 8
> > 5 files changed, 47 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> > index 6f0576e214..be2b9b817e 100644
> > --- a/
> > +++ b/include/chardev/char.h
> > @@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> > * @qemu_chr_new:
> > *
> > * Create a new character backend from a URI.
> > + * Do not implicitely initialize a monitor if the chardev is muxed.
> > *
> > * @label the name of the backend
> > * @filename the URI
> > @@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> > */
> > Chardev *qemu_chr_new(const char *label, const char *filename);
> >
> > +/**
> > + * @qemu_chr_new_mux_mon:
> > + *
> > + * Create a new character backend from a URI.
> > + * Implicitely initialize a monitor if the chardev is muxed.
> > + *
> > + * @label the name of the backend
> > + * @filename the URI
>
> I'm no friend of GTK-Doc style, but where we use it, we should use it
> correctly: put a colon after @param.
I copied existing comment... Should I fixed all others in the headers?
>
> > + *
> > + * Returns: a new character backend
> > + */
> > +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> > +
> > /**
> > * @qemu_chr_change:
> > *
> > @@ -138,10 +152,12 @@ void qemu_chr_cleanup(void);
> > *
> > * @label the name of the backend
> > * @filename the URI
> > + * @with_mux_mon if chardev is muxed, initialize a monitor
> > *
> > * 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:
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 76d866e6fe..2c295ad676 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -683,7 +683,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;
> > @@ -702,17 +703,19 @@ Chardev *qemu_chr_new_noreplay(const char *label,
> > const char *filename)
> > if (err) {
> > error_report_err(err);
> > }
> > -if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> > +if (with_mux_mon && chr && qemu_opt_get_bool(opts, "mux", 0)) {
> > monitor_init(chr, MONITOR_USE_READLINE);
> > }
>
> When !chr, the function already failed, so that part of the condition is
> uninteresting here[*].
yeah, hopefully err is always set if the return value is NULL.
> That leaves qemu_opt_get_bool(opts, "mux", 0). Behavior changes when
> it's true and with_mux_mon is false: we no longer create a monitor.
>
> Can this happen?
>
> If it can, when exactly, and how does it affect externally visible
> behavior?
we could add an assert() for with_mux_mon || !opt("mux").
>
> I guess we'll see below.
>
> > qemu_opts_del(opts);
> > return chr;
> > }
> >
> > -Chardev *qemu_chr_new(const char *label, const char *filename)
> > +static Chardev
Re: [Xen-devel] [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor
Marc-André Lureau 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.
>
> There are documented cases, such as: -serial/-parallel/-virtioconsole
> and to less extent -debugcon.
>
> Less obvious and questionnable ones are -gdb 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 preliminary checks on the string (such as slirp), or do
> not need it, such as -qtest.
>
> 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
Aside: the question "what does it mean to connect a monitor to a chardev
that has an implicit monitor" crosses my mind. Next thought is "the
day's too short to go there".
> 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 :)
Hah!
> Signed-off-by: Marc-André Lureau
> ---
> include/chardev/char.h | 18 +-
> chardev/char.c | 21 +
> gdbstub.c | 6 +-
> hw/char/xen_console.c | 5 -
> vl.c | 8
> 5 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 6f0576e214..be2b9b817e 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> * @qemu_chr_new:
> *
> * Create a new character backend from a URI.
> + * Do not implicitely initialize a monitor if the chardev is muxed.
> *
> * @label the name of the backend
> * @filename the URI
> @@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> */
> Chardev *qemu_chr_new(const char *label, const char *filename);
>
> +/**
> + * @qemu_chr_new_mux_mon:
> + *
> + * Create a new character backend from a URI.
> + * Implicitely initialize a monitor if the chardev is muxed.
> + *
> + * @label the name of the backend
> + * @filename the URI
I'm no friend of GTK-Doc style, but where we use it, we should use it
correctly: put a colon after @param.
> + *
> + * Returns: a new character backend
> + */
> +Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename);
> +
> /**
> * @qemu_chr_change:
> *
> @@ -138,10 +152,12 @@ void qemu_chr_cleanup(void);
> *
> * @label the name of the backend
> * @filename the URI
> + * @with_mux_mon if chardev is muxed, initialize a monitor
> *
> * 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:
> diff --git a/chardev/char.c b/chardev/char.c
> index 76d866e6fe..2c295ad676 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -683,7 +683,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;
> @@ -702,17 +703,19 @@ Chardev *qemu_chr_new_noreplay(const char *label, const
> char *filename)
> if (err) {
> error_report_err(err);
> }
> -if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> +if (with_mux_mon && chr && qemu_opt_get_bool(opts, "mux", 0)) {
> monitor_init(chr, MONITOR_USE_READLINE);
> }
When !chr, the function already failed, so that part of the condition is
uninteresting here[*].
That leaves qemu_opt_get_bool(opts, "mux", 0). Behavior changes when
it's true and with_mux_mon is false: we no longer create a monitor.
Can this happen?
If it can, when exactly, and how does it affect externally visible
behavior?
I guess we'll see below.
> qemu_opts_del(opts);
> return chr;
> }
>
> -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 +729,16 @@ Chardev
Re: [Xen-devel] [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor
On 08/22/2018 01:02 PM, Marc-André Lureau wrote:
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.
There are documented cases, such as: -serial/-parallel/-virtioconsole
and to less extent -debugcon.
Less obvious and questionnable ones are -gdb and Xen console. Add a
s/questionnable/questionable/
FIXME note for those, but keep the support for now.
Other qemu_chr_new() callers either have a fixed parameter/filename
string, or do preliminary checks on the string (such as slirp), or do
not need it, such as -qtest.
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
---
include/chardev/char.h | 18 +-
chardev/char.c | 21 +
gdbstub.c | 6 +-
hw/char/xen_console.c | 5 -
vl.c | 8
5 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 6f0576e214..be2b9b817e 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
* @qemu_chr_new:
*
* Create a new character backend from a URI.
+ * Do not implicitely initialize a monitor if the chardev is muxed.
s/implicitely/implicitly/
*
* @label the name of the backend
* @filename the URI
@@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
*/
Chardev *qemu_chr_new(const char *label, const char *filename);
+/**
+ * @qemu_chr_new_mux_mon:
+ *
+ * Create a new character backend from a URI.
+ * Implicitely initialize a monitor if the chardev is muxed.
and again
+++ b/gdbstub.c
@@ -2038,7 +2038,11 @@ int gdbserver_start(const char *device)
sigaction(SIGINT, , NULL);
}
#endif
-chr = qemu_chr_new_noreplay("gdb", device);
+/*
+ * FIXME: it's a bit weird to allow using a mux chardev here
+ * and setup implicitely a monitor. We may want to break this.
s/setup implicitely/implicitly set up/
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
