| Message ID | 1393237002-16980-3-git-send-email-ghammer@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 24/02/2014 11:16, Gal Hammer ha scritto: > Replacement of the default chardev handlers now requires a call to > release the current handlers. > > Signed-off-by: Gal Hammer <ghammer@redhat.com> > --- > monitor.c | 1 + > qtest.c | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/monitor.c b/monitor.c > index de90fba..db52e7f 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags) > if (monitor_ctrl_mode(mon)) { > mon->mc = g_malloc0(sizeof(MonitorControl)); > /* Control mode requires special handlers */ > + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); > qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read, > monitor_control_event, mon); > qemu_chr_fe_set_echo(chr, true); > diff --git a/qtest.c b/qtest.c > index ae941d6..a5682ee 100644 > --- a/qtest.c > +++ b/qtest.c > @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) > return; > } > > + /* Replace the default tcp's handlers with qtest's handlers. */ > + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); > qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr); > qemu_chr_fe_set_echo(chr, true); > > Why can't qemu_chr_add_handlers call remove_fd_in_watch when handlers are replaced, before setting the new ones? Paolo
On 24/02/2014 12:32, Paolo Bonzini wrote: > Il 24/02/2014 11:16, Gal Hammer ha scritto: >> Replacement of the default chardev handlers now requires a call to >> release the current handlers. >> >> Signed-off-by: Gal Hammer <ghammer@redhat.com> >> --- >> monitor.c | 1 + >> qtest.c | 2 ++ >> 2 files changed, 3 insertions(+) >> >> diff --git a/monitor.c b/monitor.c >> index de90fba..db52e7f 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags) >> if (monitor_ctrl_mode(mon)) { >> mon->mc = g_malloc0(sizeof(MonitorControl)); >> /* Control mode requires special handlers */ >> + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); >> qemu_chr_add_handlers(chr, monitor_can_read, >> monitor_control_read, >> monitor_control_event, mon); >> qemu_chr_fe_set_echo(chr, true); >> diff --git a/qtest.c b/qtest.c >> index ae941d6..a5682ee 100644 >> --- a/qtest.c >> +++ b/qtest.c >> @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const >> char *qtest_log, Error **errp) >> return; >> } >> >> + /* Replace the default tcp's handlers with qtest's handlers. */ >> + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); >> qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, >> qtest_event, chr); >> qemu_chr_fe_set_echo(chr, true); >> >> > > Why can't qemu_chr_add_handlers call remove_fd_in_watch when handlers > are replaced, before setting the new ones? > > Paolo It used to be like that (see patch 1/2 for the change). However, I removed it after I saw that the function remove_fd_in_watch() is always called when setting all the handlers to NULL (qemu-char.c +3872) so a second call in every chr_update_read_handler callback seems redundant. As far as I can tell only the qtest change the read handlers and not using the default ones. Gal.
Hi,
After reading the code again. It make sense to leave the call to remove_fd_in_watch() because I changed the code so the chr_update_read_handler function is called only when the front end is opened.
Submitting a new version of the patch.
Gal.
----- Original Message -----
From: "Gal Hammer" <ghammer@redhat.com>
To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
Cc: "amit shah" <amit.shah@redhat.com>, "peter maydell" <peter.maydell@linaro.org>, anthony@codemonkey.ws
Sent: Monday, February 24, 2014 1:44:31 PM
Subject: Re: [Qemu-devel] [PATCH 2/2] qtest: fix a "!chr->fd_in_tag" assertion error in qtest.
On 24/02/2014 12:32, Paolo Bonzini wrote:
> Il 24/02/2014 11:16, Gal Hammer ha scritto:
>> Replacement of the default chardev handlers now requires a call to
>> release the current handlers.
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>> monitor.c | 1 +
>> qtest.c | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index de90fba..db52e7f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags)
>> if (monitor_ctrl_mode(mon)) {
>> mon->mc = g_malloc0(sizeof(MonitorControl));
>> /* Control mode requires special handlers */
>> + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>> qemu_chr_add_handlers(chr, monitor_can_read,
>> monitor_control_read,
>> monitor_control_event, mon);
>> qemu_chr_fe_set_echo(chr, true);
>> diff --git a/qtest.c b/qtest.c
>> index ae941d6..a5682ee 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const
>> char *qtest_log, Error **errp)
>> return;
>> }
>>
>> + /* Replace the default tcp's handlers with qtest's handlers. */
>> + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>> qemu_chr_add_handlers(chr, qtest_can_read, qtest_read,
>> qtest_event, chr);
>> qemu_chr_fe_set_echo(chr, true);
>>
>>
>
> Why can't qemu_chr_add_handlers call remove_fd_in_watch when handlers
> are replaced, before setting the new ones?
>
> Paolo
It used to be like that (see patch 1/2 for the change). However, I
removed it after I saw that the function remove_fd_in_watch() is always
called when setting all the handlers to NULL (qemu-char.c +3872) so a
second call in every chr_update_read_handler callback seems redundant.
As far as I can tell only the qtest change the read handlers and not
using the default ones.
Gal.
diff --git a/monitor.c b/monitor.c index de90fba..db52e7f 100644 --- a/monitor.c +++ b/monitor.c @@ -5024,6 +5024,7 @@ void monitor_init(CharDriverState *chr, int flags) if (monitor_ctrl_mode(mon)) { mon->mc = g_malloc0(sizeof(MonitorControl)); /* Control mode requires special handlers */ + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read, monitor_control_event, mon); qemu_chr_fe_set_echo(chr, true); diff --git a/qtest.c b/qtest.c index ae941d6..a5682ee 100644 --- a/qtest.c +++ b/qtest.c @@ -519,6 +519,8 @@ void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error **errp) return; } + /* Replace the default tcp's handlers with qtest's handlers. */ + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr); qemu_chr_fe_set_echo(chr, true);
Replacement of the default chardev handlers now requires a call to release the current handlers. Signed-off-by: Gal Hammer <ghammer@redhat.com> --- monitor.c | 1 + qtest.c | 2 ++ 2 files changed, 3 insertions(+)