Patchwork fix monitor

login
register
mail settings
Submitter Gerd Hoffmann
Date March 19, 2013, 2:04 p.m.
Message ID <1363701863-13004-1-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/229078/
State New
Headers show

Comments

Gerd Hoffmann - March 19, 2013, 2:04 p.m.
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(-)
Laszlo Ersek - March 19, 2013, 2:45 p.m.
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
Gerd Hoffmann - March 19, 2013, 3:11 p.m.
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
Markus Armbruster - March 19, 2013, 5:40 p.m.
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.
Anthony Liguori - March 19, 2013, 6:58 p.m.
Applied.  Thanks.

Regards,

Anthony Liguori
Anthony Liguori - March 19, 2013, 7 p.m.
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
Peter Lieven - April 3, 2013, 12:17 p.m.
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
>
>
>
Gerd Hoffmann - April 3, 2013, 12:35 p.m.
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
Luiz Capitulino - April 3, 2013, 1:36 p.m.
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
Peter Lieven - April 4, 2013, 5:42 a.m.
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

Patch

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);
     }
 }