All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] fix monitor
@ 2013-03-19 14:04 Gerd Hoffmann
  2013-03-19 14:45 ` Laszlo Ersek
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2013-03-19 14:04 UTC (permalink / raw)
  To: qemu-devel
  Cc: amit.shah, Markus Armbruster, Gerd Hoffmann, anthony, Luiz Capitulino

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19 14:04 [Qemu-devel] [PATCH] fix monitor Gerd Hoffmann
@ 2013-03-19 14:45 ` Laszlo Ersek
  2013-03-19 15:11   ` Gerd Hoffmann
  2013-03-19 17:40 ` Markus Armbruster
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2013-03-19 14:45 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: amit.shah, Luiz Capitulino, qemu-devel, anthony, Markus Armbruster

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19 14:45 ` Laszlo Ersek
@ 2013-03-19 15:11   ` Gerd Hoffmann
  0 siblings, 0 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2013-03-19 15:11 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: amit.shah, Luiz Capitulino, qemu-devel, anthony, Markus Armbruster

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19 14:04 [Qemu-devel] [PATCH] fix monitor Gerd Hoffmann
  2013-03-19 14:45 ` Laszlo Ersek
@ 2013-03-19 17:40 ` Markus Armbruster
  2013-03-19 19:00   ` Anthony Liguori
  2013-03-19 18:58 ` Anthony Liguori
  2013-04-03 12:17 ` Peter Lieven
  3 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2013-03-19 17:40 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: amit.shah, qemu-devel, anthony, Luiz Capitulino

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19 14:04 [Qemu-devel] [PATCH] fix monitor Gerd Hoffmann
  2013-03-19 14:45 ` Laszlo Ersek
  2013-03-19 17:40 ` Markus Armbruster
@ 2013-03-19 18:58 ` Anthony Liguori
  2013-04-03 12:17 ` Peter Lieven
  3 siblings, 0 replies; 13+ messages in thread
From: Anthony Liguori @ 2013-03-19 18:58 UTC (permalink / raw)
  To: Gerd Hoffmann, qemu-devel
  Cc: amit.shah, Luiz Capitulino, Markus Armbruster, anthony

Applied.  Thanks.

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19 17:40 ` Markus Armbruster
@ 2013-03-19 19:00   ` Anthony Liguori
  0 siblings, 0 replies; 13+ messages in thread
From: Anthony Liguori @ 2013-03-19 19:00 UTC (permalink / raw)
  To: Markus Armbruster, Gerd Hoffmann; +Cc: amit.shah, qemu-devel, Luiz Capitulino

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19 14:04 [Qemu-devel] [PATCH] fix monitor Gerd Hoffmann
                   ` (2 preceding siblings ...)
  2013-03-19 18:58 ` Anthony Liguori
@ 2013-04-03 12:17 ` Peter Lieven
  2013-04-03 12:35   ` Gerd Hoffmann
  3 siblings, 1 reply; 13+ messages in thread
From: Peter Lieven @ 2013-04-03 12:17 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Markus Armbruster, qemu-devel, Luiz Capitulino, anthony, amit.shah

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
>
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-04-03 12:17 ` Peter Lieven
@ 2013-04-03 12:35   ` Gerd Hoffmann
  2013-04-03 13:36     ` Luiz Capitulino
  0 siblings, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2013-04-03 12:35 UTC (permalink / raw)
  To: Peter Lieven
  Cc: amit.shah, Luiz Capitulino, qemu-devel, anthony, Markus Armbruster

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-04-03 12:35   ` Gerd Hoffmann
@ 2013-04-03 13:36     ` Luiz Capitulino
  2013-04-04  5:42       ` Peter Lieven
  0 siblings, 1 reply; 13+ messages in thread
From: Luiz Capitulino @ 2013-04-03 13:36 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: amit.shah, Peter Lieven, qemu-devel, anthony, Markus Armbruster

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-04-03 13:36     ` Luiz Capitulino
@ 2013-04-04  5:42       ` Peter Lieven
  0 siblings, 0 replies; 13+ messages in thread
From: Peter Lieven @ 2013-04-04  5:42 UTC (permalink / raw)
  To: qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19  9:57 Gerd Hoffmann
  2013-03-19 13:58 ` Hans de Goede
@ 2013-03-19 18:58 ` Anthony Liguori
  1 sibling, 0 replies; 13+ messages in thread
From: Anthony Liguori @ 2013-03-19 18:58 UTC (permalink / raw)
  To: Gerd Hoffmann, qemu-devel
  Cc: amit.shah, Luiz Capitulino, Markus Armbruster, anthony

Applied.  Thanks.

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH] fix monitor
  2013-03-19  9:57 Gerd Hoffmann
@ 2013-03-19 13:58 ` Hans de Goede
  2013-03-19 18:58 ` Anthony Liguori
  1 sibling, 0 replies; 13+ messages in thread
From: Hans de Goede @ 2013-03-19 13:58 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: amit.shah, Luiz Capitulino, qemu-devel, anthony, Markus Armbruster

Hi,

Looks good, one minor nitpick.

On 03/19/2013 10:57 AM, Gerd Hoffmann wrote:
> chardev flow control broke monitor, fix it by adding watch support.
> ---
>   monitor.c |   23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 112e920..680d344 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) {
> +            /* partinal write */

Typo in the comment.

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

Regards,

Hans

^ permalink raw reply	[flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH] fix monitor
@ 2013-03-19  9:57 Gerd Hoffmann
  2013-03-19 13:58 ` Hans de Goede
  2013-03-19 18:58 ` Anthony Liguori
  0 siblings, 2 replies; 13+ messages in thread
From: Gerd Hoffmann @ 2013-03-19  9:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: amit.shah, Markus Armbruster, Gerd Hoffmann, anthony, Luiz Capitulino

chardev flow control broke monitor, fix it by adding watch support.
---
 monitor.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 112e920..680d344 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) {
+            /* partinal 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-04-04  5:42 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-19 14:04 [Qemu-devel] [PATCH] fix monitor Gerd Hoffmann
2013-03-19 14:45 ` Laszlo Ersek
2013-03-19 15:11   ` Gerd Hoffmann
2013-03-19 17:40 ` Markus Armbruster
2013-03-19 19:00   ` Anthony Liguori
2013-03-19 18:58 ` Anthony Liguori
2013-04-03 12:17 ` Peter Lieven
2013-04-03 12:35   ` Gerd Hoffmann
2013-04-03 13:36     ` Luiz Capitulino
2013-04-04  5:42       ` Peter Lieven
  -- strict thread matches above, loose matches on Subject: below --
2013-03-19  9:57 Gerd Hoffmann
2013-03-19 13:58 ` Hans de Goede
2013-03-19 18:58 ` Anthony Liguori

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.