| Message ID | 1257258596-4556-3-git-send-email-amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
Amit Shah wrote: > The initial_reset sent to chardevs doesn't do much other than setting > a bool to true. Char devices are interested in the open event and > that gets sent whenever the device is opened. Have you checked with the monitor in all use cases (dedicated & muxed console, stdio & SDL console, etc.)? It was introduced once to fix a corner case, I think it's even documented... Jan > > Moreover, the reset logic breaks as and when qemu's bh scheduling > changes. > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > --- > qemu-char.c | 9 --------- > qemu-char.h | 1 - > vl.c | 1 - > 3 files changed, 0 insertions(+), 11 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 1f63019..d78bae3 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -128,15 +128,6 @@ void qemu_chr_reset(CharDriverState *s) > } > } > > -void qemu_chr_initial_reset(void) > -{ > - CharDriverState *chr; > - > - QTAILQ_FOREACH(chr, &chardevs, next) { > - qemu_chr_reset(chr); > - } > -} > - > int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len) > { > return s->chr_write(s, buf, len); > diff --git a/qemu-char.h b/qemu-char.h > index 05fe15d..b420111 100644 > --- a/qemu-char.h > +++ b/qemu-char.h > @@ -83,7 +83,6 @@ void qemu_chr_add_handlers(CharDriverState *s, > void *opaque); > int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg); > void qemu_chr_reset(CharDriverState *s); > -void qemu_chr_initial_reset(void); > int qemu_chr_can_read(CharDriverState *s); > void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len); > int qemu_chr_get_msgfd(CharDriverState *s); > diff --git a/vl.c b/vl.c > index e57f58f..5ad4e42 100644 > --- a/vl.c > +++ b/vl.c > @@ -5750,7 +5750,6 @@ int main(int argc, char **argv, char **envp) > } > > text_consoles_set_display(display_state); > - qemu_chr_initial_reset(); > > for (i = 0; i < MAX_MONITOR_DEVICES; i++) { > if (monitor_devices[i] && monitor_hds[i]) {
On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: > Amit Shah wrote: > > The initial_reset sent to chardevs doesn't do much other than setting > > a bool to true. Char devices are interested in the open event and > > that gets sent whenever the device is opened. > > Have you checked with the monitor in all use cases (dedicated & muxed > console, stdio & SDL console, etc.)? It was introduced once to fix a I've checked with -monitor stdio, monitor in SDL and also chardevs using unix sockets. I've not tried mux yet; I'll try that and report back. BTW if it ends up not working with this patch, it'd be broken in the current master as well. > corner case, I think it's even documented... Hm; I'll have to look really deep; haven't found any such thing yet. Amit
On (Tue) Nov 03 2009 [23:25:52], Amit Shah wrote: > On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: > > Amit Shah wrote: > > > The initial_reset sent to chardevs doesn't do much other than setting > > > a bool to true. Char devices are interested in the open event and > > > that gets sent whenever the device is opened. > > > > Have you checked with the monitor in all use cases (dedicated & muxed > > console, stdio & SDL console, etc.)? It was introduced once to fix a > > I've checked with -monitor stdio, monitor in SDL and also chardevs using > unix sockets. > > I've not tried mux yet; I'll try that and report back. BTW if it ends up > not working with this patch, it'd be broken in the current master as > well. I tried with: -chardev stdio,mux=on,id=mux -monitor chardev:mux -serial chardev:mux The monitor prompt shows up as does the serial output. (btw I've also tried closing and opening char devs and that works fine too) Amit
Amit Shah wrote: > On (Tue) Nov 03 2009 [23:25:52], Amit Shah wrote: >> On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: >>> Amit Shah wrote: >>>> The initial_reset sent to chardevs doesn't do much other than setting >>>> a bool to true. Char devices are interested in the open event and >>>> that gets sent whenever the device is opened. >>> Have you checked with the monitor in all use cases (dedicated & muxed >>> console, stdio & SDL console, etc.)? It was introduced once to fix a >> I've checked with -monitor stdio, monitor in SDL and also chardevs using >> unix sockets. >> >> I've not tried mux yet; I'll try that and report back. BTW if it ends up >> not working with this patch, it'd be broken in the current master as >> well. > > I tried with: > > -chardev stdio,mux=on,id=mux -monitor chardev:mux -serial chardev:mux > > The monitor prompt shows up as does the serial output. > > (btw I've also tried closing and opening char devs and that works fine > too) > That sounds good. Then something must have changed since 2970a6c943, do you see what? Jan
On (Tue) Nov 03 2009 [19:53:43], Jan Kiszka wrote: > Amit Shah wrote: > > On (Tue) Nov 03 2009 [23:25:52], Amit Shah wrote: > >> On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: > >>> Amit Shah wrote: > >>>> The initial_reset sent to chardevs doesn't do much other than setting > >>>> a bool to true. Char devices are interested in the open event and > >>>> that gets sent whenever the device is opened. > >>> Have you checked with the monitor in all use cases (dedicated & muxed > >>> console, stdio & SDL console, etc.)? It was introduced once to fix a > >> I've checked with -monitor stdio, monitor in SDL and also chardevs using > >> unix sockets. > >> > >> I've not tried mux yet; I'll try that and report back. BTW if it ends up > >> not working with this patch, it'd be broken in the current master as > >> well. > > > > I tried with: > > > > -chardev stdio,mux=on,id=mux -monitor chardev:mux -serial chardev:mux > > > > The monitor prompt shows up as does the serial output. > > > > (btw I've also tried closing and opening char devs and that works fine > > too) > > That sounds good. Then something must have changed since 2970a6c943, do > you see what? I think that depended on the resets being sent. I've now removed the need for resets altogether. Amit
Amit Shah wrote: > On (Tue) Nov 03 2009 [19:53:43], Jan Kiszka wrote: >> Amit Shah wrote: >>> On (Tue) Nov 03 2009 [23:25:52], Amit Shah wrote: >>>> On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: >>>>> Amit Shah wrote: >>>>>> The initial_reset sent to chardevs doesn't do much other than setting >>>>>> a bool to true. Char devices are interested in the open event and >>>>>> that gets sent whenever the device is opened. >>>>> Have you checked with the monitor in all use cases (dedicated & muxed >>>>> console, stdio & SDL console, etc.)? It was introduced once to fix a >>>> I've checked with -monitor stdio, monitor in SDL and also chardevs using >>>> unix sockets. >>>> >>>> I've not tried mux yet; I'll try that and report back. BTW if it ends up >>>> not working with this patch, it'd be broken in the current master as >>>> well. >>> I tried with: >>> >>> -chardev stdio,mux=on,id=mux -monitor chardev:mux -serial chardev:mux >>> >>> The monitor prompt shows up as does the serial output. >>> >>> (btw I've also tried closing and opening char devs and that works fine >>> too) >> That sounds good. Then something must have changed since 2970a6c943, do >> you see what? > > I think that depended on the resets being sent. I've now removed the > need for resets altogether. No, this is in fact the reason why we no longer need it: 9a1e948129 (Introduce contexts for asynchronous callbacks) As the initial reset of the char device that is marked pending on open is now no longer consumed by the IDE initialization, we can actually drop the later regeneration via qemu_chr_initial_reset. I just hope this stays like it is... Jan
On (Wed) Nov 04 2009 [10:39:39], Jan Kiszka wrote: > Amit Shah wrote: > > On (Tue) Nov 03 2009 [19:53:43], Jan Kiszka wrote: > >> Amit Shah wrote: > >>> On (Tue) Nov 03 2009 [23:25:52], Amit Shah wrote: > >>>> On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: > >>>>> Amit Shah wrote: > >>>>>> The initial_reset sent to chardevs doesn't do much other than setting > >>>>>> a bool to true. Char devices are interested in the open event and > >>>>>> that gets sent whenever the device is opened. > >>>>> Have you checked with the monitor in all use cases (dedicated & muxed > >>>>> console, stdio & SDL console, etc.)? It was introduced once to fix a > >>>> I've checked with -monitor stdio, monitor in SDL and also chardevs using > >>>> unix sockets. > >>>> > >>>> I've not tried mux yet; I'll try that and report back. BTW if it ends up > >>>> not working with this patch, it'd be broken in the current master as > >>>> well. > >>> I tried with: > >>> > >>> -chardev stdio,mux=on,id=mux -monitor chardev:mux -serial chardev:mux > >>> > >>> The monitor prompt shows up as does the serial output. > >>> > >>> (btw I've also tried closing and opening char devs and that works fine > >>> too) > >> That sounds good. Then something must have changed since 2970a6c943, do > >> you see what? > > > > I think that depended on the resets being sent. I've now removed the > > need for resets altogether. > > No, this is in fact the reason why we no longer need it: > > 9a1e948129 (Introduce contexts for asynchronous callbacks) > > As the initial reset of the char device that is marked pending on open > is now no longer consumed by the IDE initialization, we can actually > drop the later regeneration via qemu_chr_initial_reset. I just hope this > stays like it is... I tested this even on a tree that doesn't have this patch. I haven't really delved deep to see why this was added earlier -- the commit log only says very little. Plus my testing with the current tree works fine so I'm happy to mention these things in the commit log. Amit
Amit Shah wrote: > On (Wed) Nov 04 2009 [10:39:39], Jan Kiszka wrote: >> Amit Shah wrote: >>> On (Tue) Nov 03 2009 [19:53:43], Jan Kiszka wrote: >>>> Amit Shah wrote: >>>>> On (Tue) Nov 03 2009 [23:25:52], Amit Shah wrote: >>>>>> On (Tue) Nov 03 2009 [18:08:57], Jan Kiszka wrote: >>>>>>> Amit Shah wrote: >>>>>>>> The initial_reset sent to chardevs doesn't do much other than setting >>>>>>>> a bool to true. Char devices are interested in the open event and >>>>>>>> that gets sent whenever the device is opened. >>>>>>> Have you checked with the monitor in all use cases (dedicated & muxed >>>>>>> console, stdio & SDL console, etc.)? It was introduced once to fix a >>>>>> I've checked with -monitor stdio, monitor in SDL and also chardevs using >>>>>> unix sockets. >>>>>> >>>>>> I've not tried mux yet; I'll try that and report back. BTW if it ends up >>>>>> not working with this patch, it'd be broken in the current master as >>>>>> well. >>>>> I tried with: >>>>> >>>>> -chardev stdio,mux=on,id=mux -monitor chardev:mux -serial chardev:mux >>>>> >>>>> The monitor prompt shows up as does the serial output. >>>>> >>>>> (btw I've also tried closing and opening char devs and that works fine >>>>> too) >>>> That sounds good. Then something must have changed since 2970a6c943, do >>>> you see what? >>> I think that depended on the resets being sent. I've now removed the >>> need for resets altogether. >> No, this is in fact the reason why we no longer need it: >> >> 9a1e948129 (Introduce contexts for asynchronous callbacks) >> >> As the initial reset of the char device that is marked pending on open >> is now no longer consumed by the IDE initialization, we can actually >> drop the later regeneration via qemu_chr_initial_reset. I just hope this >> stays like it is... > > I tested this even on a tree that doesn't have this patch. The whole things was (and maybe still is) fragile. So you have to take care what further patches with side effects you include. If you revert 9a1e948129 and remove qemu_chr_initial_reset, you get the old bug back again, ie. no prompt. > > I haven't really delved deep to see why this was added earlier -- the > commit log only says very little. Plus my testing with the current tree > works fine so I'm happy to mention these things in the commit log. I've nothing against your patch, specifically as it removes an obviously no longer needed workaround, not a feature. I just want to make sure that the current behavior is not only there by chance. Jan
On (Wed) Nov 04 2009 [15:30:21], Jan Kiszka wrote: > > I've nothing against your patch, specifically as it removes an obviously > no longer needed workaround, not a feature. I just want to make sure > that the current behavior is not only there by chance. Sure; I understand that. I'll be looking more at this code to free it from any surprises. Amit
diff --git a/qemu-char.c b/qemu-char.c index 1f63019..d78bae3 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -128,15 +128,6 @@ void qemu_chr_reset(CharDriverState *s) } } -void qemu_chr_initial_reset(void) -{ - CharDriverState *chr; - - QTAILQ_FOREACH(chr, &chardevs, next) { - qemu_chr_reset(chr); - } -} - int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len) { return s->chr_write(s, buf, len); diff --git a/qemu-char.h b/qemu-char.h index 05fe15d..b420111 100644 --- a/qemu-char.h +++ b/qemu-char.h @@ -83,7 +83,6 @@ void qemu_chr_add_handlers(CharDriverState *s, void *opaque); int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg); void qemu_chr_reset(CharDriverState *s); -void qemu_chr_initial_reset(void); int qemu_chr_can_read(CharDriverState *s); void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len); int qemu_chr_get_msgfd(CharDriverState *s); diff --git a/vl.c b/vl.c index e57f58f..5ad4e42 100644 --- a/vl.c +++ b/vl.c @@ -5750,7 +5750,6 @@ int main(int argc, char **argv, char **envp) } text_consoles_set_display(display_state); - qemu_chr_initial_reset(); for (i = 0; i < MAX_MONITOR_DEVICES; i++) { if (monitor_devices[i] && monitor_hds[i]) {
The initial_reset sent to chardevs doesn't do much other than setting a bool to true. Char devices are interested in the open event and that gets sent whenever the device is opened. Moreover, the reset logic breaks as and when qemu's bh scheduling changes. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- qemu-char.c | 9 --------- qemu-char.h | 1 - vl.c | 1 - 3 files changed, 0 insertions(+), 11 deletions(-)