| Message ID | 1363701863-13004-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/19/13 15:04, Gerd Hoffmann wrote: > chardev flow control broke monitor, fix it by adding watch support. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > v2: fix tyops Well played, Sir :) > --- > monitor.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 112e920..74807f9 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -261,11 +261,30 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, > } > } > > +static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > + void *opaque) > +{ > + monitor_flush(opaque); > + return FALSE; > +} > + > void monitor_flush(Monitor *mon) > { > + int rc; > + > if (mon && mon->outbuf_index != 0 && !mon->mux_out) { > - qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); > - mon->outbuf_index = 0; > + rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); > + if (rc == mon->outbuf_index) { > + /* all flushed */ > + mon->outbuf_index = 0; > + return; > + } > + if (rc > 0) { > + /* partial write */ > + memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc); > + mon->outbuf_index -= rc; > + } > + qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon); > } > } > > Looks good to me. Should we maybe forego (re)installing the (new) watch when rc<0? (Especially because on a persistent error the fd might immediately report writability.) Laszlo
On 03/19/13 15:45, Laszlo Ersek wrote: > On 03/19/13 15:04, Gerd Hoffmann wrote: >> chardev flow control broke monitor, fix it by adding watch support. >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> v2: fix tyops > > Well played, Sir :) > >> --- >> monitor.c | 23 +++++++++++++++++++++-- >> 1 file changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/monitor.c b/monitor.c >> index 112e920..74807f9 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -261,11 +261,30 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, >> } >> } >> >> +static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, >> + void *opaque) >> +{ >> + monitor_flush(opaque); >> + return FALSE; >> +} >> + >> void monitor_flush(Monitor *mon) >> { >> + int rc; >> + >> if (mon && mon->outbuf_index != 0 && !mon->mux_out) { >> - qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); >> - mon->outbuf_index = 0; >> + rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); >> + if (rc == mon->outbuf_index) { >> + /* all flushed */ >> + mon->outbuf_index = 0; >> + return; >> + } >> + if (rc > 0) { >> + /* partial write */ >> + memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc); >> + mon->outbuf_index -= rc; >> + } >> + qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon); >> } >> } >> >> > > Looks good to me. > > Should we maybe forego (re)installing the (new) watch when rc<0? > (Especially because on a persistent error the fd might immediately > report writability.) Yes, it is probably wise to check for errno == EAGAIN (assuming qemu_chr_fe_write makes sure errno it set to something sensible on error). cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > chardev flow control broke monitor, fix it by adding watch support. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > v2: fix tyops > --- Subject lacks v2. Anthony, holler if you want a respin to unconfuse your tools.
Applied. Thanks. Regards, Anthony Liguori
Markus Armbruster <armbru@redhat.com> writes: > Gerd Hoffmann <kraxel@redhat.com> writes: > >> chardev flow control broke monitor, fix it by adding watch support. >> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> v2: fix tyops >> --- > > Subject lacks v2. Anthony, holler if you want a respin to unconfuse > your tools. I already processed this patch--specifically this copy of it. Newest mail always wins when there's a conflict but please don't rely on that :-) Regards, Anthony Liguori
Hi Gerd, I today saw this assert when live migrating a VM. Is this related? The below patch was already applied. qemu-1.4.5: /usr/src/qemu-1.4.5/monitor.c:297: monitor_puts: Assertion `mon->outbuf_index < sizeof(mon->outbuf) - 1' failed. Peter Gerd Hoffmann wrote: > chardev flow control broke monitor, fix it by adding watch support. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > v2: fix tyops > --- > monitor.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 112e920..74807f9 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -261,11 +261,30 @@ int monitor_read_password(Monitor *mon, ReadLineFunc > *readline_func, > } > } > > +static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, > + void *opaque) > +{ > + monitor_flush(opaque); > + return FALSE; > +} > + > void monitor_flush(Monitor *mon) > { > + int rc; > + > if (mon && mon->outbuf_index != 0 && !mon->mux_out) { > - qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); > - mon->outbuf_index = 0; > + rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); > + if (rc == mon->outbuf_index) { > + /* all flushed */ > + mon->outbuf_index = 0; > + return; > + } > + if (rc > 0) { > + /* partial write */ > + memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - > rc); > + mon->outbuf_index -= rc; > + } > + qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, > mon); > } > } > > -- > 1.7.9.7 > > >
On 04/03/13 14:17, Peter Lieven wrote: > Hi Gerd, > > I today saw this assert when live migrating a VM. Is this related? The > below patch was already applied. > > qemu-1.4.5: /usr/src/qemu-1.4.5/monitor.c:297: monitor_puts: Assertion > `mon->outbuf_index < sizeof(mon->outbuf) - 1' failed. Probably, patches (from luiz) are on the list to make the monitor buffer size dynamic, they should fix it. cheers, Gerd
On Wed, 03 Apr 2013 14:35:46 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > On 04/03/13 14:17, Peter Lieven wrote: > > Hi Gerd, > > > > I today saw this assert when live migrating a VM. Is this related? The > > below patch was already applied. > > > > qemu-1.4.5: /usr/src/qemu-1.4.5/monitor.c:297: monitor_puts: Assertion > > `mon->outbuf_index < sizeof(mon->outbuf) - 1' failed. > > Probably, patches (from luiz) are on the list to make the monitor buffer > size dynamic, they should fix it. Yes. Peter, can you please try my series and report if it really fixes the assertion for you? http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg00369.html
On 03.04.2013 15:36, Luiz Capitulino wrote: > On Wed, 03 Apr 2013 14:35:46 +0200 > Gerd Hoffmann <kraxel@redhat.com> wrote: > >> On 04/03/13 14:17, Peter Lieven wrote: >>> Hi Gerd, >>> >>> I today saw this assert when live migrating a VM. Is this related? The >>> below patch was already applied. >>> >>> qemu-1.4.5: /usr/src/qemu-1.4.5/monitor.c:297: monitor_puts: Assertion >>> `mon->outbuf_index < sizeof(mon->outbuf) - 1' failed. >> Probably, patches (from luiz) are on the list to make the monitor buffer >> size dynamic, they should fix it. > Yes. > > Peter, can you please try my series and report if it really fixes the > assertion for you? > > http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg00369.html > I will do, but the assertion was not as easy triggerable for me as typing '?'. We run some tests today with the patches applied and I will let you know if there where any further assertions. Peter
diff --git a/monitor.c b/monitor.c index 112e920..74807f9 100644 --- a/monitor.c +++ b/monitor.c @@ -261,11 +261,30 @@ int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, } } +static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond, + void *opaque) +{ + monitor_flush(opaque); + return FALSE; +} + void monitor_flush(Monitor *mon) { + int rc; + if (mon && mon->outbuf_index != 0 && !mon->mux_out) { - qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); - mon->outbuf_index = 0; + rc = qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); + if (rc == mon->outbuf_index) { + /* all flushed */ + mon->outbuf_index = 0; + return; + } + if (rc > 0) { + /* partial write */ + memmove(mon->outbuf, mon->outbuf + rc, mon->outbuf_index - rc); + mon->outbuf_index -= rc; + } + qemu_chr_fe_add_watch(mon->chr, G_IO_OUT, monitor_unblocked, mon); } }
chardev flow control broke monitor, fix it by adding watch support. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- v2: fix tyops --- monitor.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)