[Qemu-devel] [FOR 0.12 0/5]: Misc QMP related fixes

[Qemu-devel] [FOR 0.12 0/5]: Misc QMP related fixes

[Luiz Capitulino]

 This small series contains some QMP related fixes.

 Thanks.

[Qemu-devel] [PATCH 1/5] QDict: Fix size update

[Luiz Capitulino]

Key replacement should not update the dictionary's size.

This commit also adds a test for the bug.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 check-qdict.c |    2 ++
 qdict.c       |    3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/check-qdict.c b/check-qdict.c
index c37d448..f2b4826 100644
--- a/check-qdict.c
+++ b/check-qdict.c
@@ -205,6 +205,8 @@ START_TEST(qdict_put_exists_test)
 
     value = qdict_get_int(tests_dict, key);
     fail_unless(value == 2);
+
+    fail_unless(qdict_size(tests_dict) == 1);
 }
 END_TEST
 
diff --git a/qdict.c b/qdict.c
index ef73265..ba8eef0 100644
--- a/qdict.c
+++ b/qdict.c
@@ -122,9 +122,8 @@ void qdict_put_obj(QDict *qdict, const char *key, QObject *value)
         /* allocate a new entry */
         entry = alloc_entry(key, value);
         QLIST_INSERT_HEAD(&qdict->table[hash], entry, next);
+        qdict->size++;
     }
-
-    qdict->size++;
 }
 
 /**
-- 
1.6.6.rc2.5.g49666

[Qemu-devel] [PATCH 2/5] monitor: Use 'device' in eject

[Luiz Capitulino]

Monitor's eject command uses 'filename' for the device name
argument, but 'device' is a better name.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    2 +-
 qemu-monitor.hx |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index d97d529..d5041dc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -864,7 +864,7 @@ static void do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     BlockDriverState *bs;
     int force = qdict_get_int(qdict, "force");
-    const char *filename = qdict_get_str(qdict, "filename");
+    const char *filename = qdict_get_str(qdict, "device");
 
     bs = bdrv_find(filename);
     if (!bs) {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index c788c73..338e30e 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -130,7 +130,7 @@ ETEXI
 
     {
         .name       = "eject",
-        .args_type  = "force:-f,filename:B",
+        .args_type  = "force:-f,device:B",
         .params     = "[-f] device",
         .help       = "eject a removable medium (use -f to force it)",
         .user_print = monitor_user_noop,
-- 
1.6.6.rc2.5.g49666

[Qemu-devel] [PATCH 3/5] monitor: do_balloon(): Check for errors

[Luiz Capitulino]

do_balloon() should check for ballooning availability as
do_info_balloon() does.

Noted by Daniel P. Berrange <berrange@redhat.com>.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index d5041dc..920ccff 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2053,14 +2053,34 @@ static void do_info_status(Monitor *mon, QObject **ret_data)
                                     vm_running, singlestep);
 }
 
+static ram_addr_t balloon_get_value(void)
+{
+    ram_addr_t actual;
+
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
+        qemu_error_new(QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
+        return 0;
+    }
+
+    actual = qemu_balloon_status();
+    if (actual == 0) {
+        qemu_error_new(QERR_DEVICE_NOT_ACTIVE, "balloon");
+        return 0;
+    }
+
+    return actual;
+}
+
 /**
  * do_balloon(): Request VM to change its memory allocation
  */
 static void do_balloon(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    int value = qdict_get_int(qdict, "value");
-    ram_addr_t target = value;
-    qemu_balloon(target << 20);
+    if (balloon_get_value()) {
+        /* ballooning is active */
+        ram_addr_t value = qdict_get_int(qdict, "value");
+        qemu_balloon(value << 20);
+    }
 }
 
 static void monitor_print_balloon(Monitor *mon, const QObject *data)
@@ -2088,14 +2108,11 @@ static void do_info_balloon(Monitor *mon, QObject **ret_data)
 {
     ram_addr_t actual;
 
-    actual = qemu_balloon_status();
-    if (kvm_enabled() && !kvm_has_sync_mmu())
-        qemu_error_new(QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon");
-    else if (actual == 0)
-        qemu_error_new(QERR_DEVICE_NOT_ACTIVE, "balloon");
-    else
+    actual = balloon_get_value();
+    if (actual != 0) {
         *ret_data = qobject_from_jsonf("{ 'balloon': %" PRId64 "}",
                                        (int64_t) actual);
+    }
 }
 
 static qemu_acl *find_acl(Monitor *mon, const char *name)
-- 
1.6.6.rc2.5.g49666

[Qemu-devel] [PATCH 4/5] monitor: Avoid readline functions in QMP

[Luiz Capitulino]

The monitor_read_command() function is readline specific
and should only be used when readline is available.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index 920ccff..b518cc4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -140,6 +140,9 @@ static inline int monitor_ctrl_mode(const Monitor *mon)
 
 static void monitor_read_command(Monitor *mon, int show_prompt)
 {
+    if (!mon->rs)
+        return;
+
     readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
     if (show_prompt)
         readline_show_prompt(mon->rs);
-- 
1.6.6.rc2.5.g49666

Re: [Qemu-devel] [PATCH 4/5] monitor: Avoid readline functions in QMP

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> The monitor_read_command() function is readline specific
> and should only be used when readline is available.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

I figure this bug breaks password entry (VNC and block) on non-readline
monitors.  Suspect commit cde76ee1.

Re: [Qemu-devel] [PATCH 4/5] monitor: Avoid readline functions in QMP

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> The monitor_read_command() function is readline specific
>> and should only be used when readline is available.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>
> I figure this bug breaks password entry (VNC and block) on non-readline
> monitors.  Suspect commit cde76ee1.

PS: Subject is wrong.  gdbserver_start() creates a monitor with
MONITOR_USE_READLINE off, so it's not just QMP.

Re: [Qemu-devel] [PATCH 4/5] monitor: Avoid readline functions in QMP

[Luiz Capitulino]

On Tue, 15 Dec 2009 10:54:23 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >> The monitor_read_command() function is readline specific
> >> and should only be used when readline is available.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >
> > I figure this bug breaks password entry (VNC and block) on non-readline
> > monitors.  Suspect commit cde76ee1.

 I can only assume that it didn't matter until now.

> PS: Subject is wrong.  gdbserver_start() creates a monitor with
> MONITOR_USE_READLINE off, so it's not just QMP.

 Ditto.

Re: [Qemu-devel] [PATCH 4/5] monitor: Avoid readline functions in QMP

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 15 Dec 2009 10:54:23 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >
>> >> The monitor_read_command() function is readline specific
>> >> and should only be used when readline is available.
>> >>
>> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >
>> > I figure this bug breaks password entry (VNC and block) on non-readline
>> > monitors.  Suspect commit cde76ee1.
>
>  I can only assume that it didn't matter until now.
>
>> PS: Subject is wrong.  gdbserver_start() creates a monitor with
>> MONITOR_USE_READLINE off, so it's not just QMP.
>
>  Ditto.

Subject is wrong, because the commit avoids readline not only in QMP,
but whenever the monitor is configured not to use readline.  Yes, I'm
nitpicking :)

[Qemu-devel] Re: [PATCH 4/5] monitor: Avoid readline functions in QMP

[Jan Kiszka]

Luiz Capitulino wrote:
> On Tue, 15 Dec 2009 10:54:23 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>>
>>>> The monitor_read_command() function is readline specific
>>>> and should only be used when readline is available.
>>>>
>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> I figure this bug breaks password entry (VNC and block) on non-readline
>>> monitors.  Suspect commit cde76ee1.
> 
>  I can only assume that it didn't matter until now.
> 
>> PS: Subject is wrong.  gdbserver_start() creates a monitor with
>> MONITOR_USE_READLINE off, so it's not just QMP.
> 
>  Ditto.
> 

Right. monitor_read_password was never supposed to be used on monitors
that have not readline state and is reported as error by
monitor_read_password. So far the only monitor that was lacking such
state was the gdbserver, now we also have QMP. The latter handles this
separately, the former should still fail loudly when such commands are
issued.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 5/5] monitor: Catch printing to non-existent monitor

[Luiz Capitulino]

The monitor_vprintf() function now touches the 'mon' pointer
before calling monitor_puts(), this causes block migration
to segfault as its functions call monitor_printf() with a
NULL 'mon'.

To fix the problem this commit moves the 'mon' NULL check
from monitor_puts() to monitor_vprintf().

This can potentially hide bugs, but for some reason this has
been the behavior for a long time.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index b518cc4..ebd0282 100644
--- a/monitor.c
+++ b/monitor.c
@@ -177,9 +177,6 @@ static void monitor_puts(Monitor *mon, const char *str)
 {
     char c;
 
-    if (!mon)
-        return;
-
     for(;;) {
         c = *str++;
         if (c == '\0')
@@ -195,6 +192,9 @@ static void monitor_puts(Monitor *mon, const char *str)
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
+    if (!mon)
+        return;
+
     if (mon->mc && !mon->mc->print_enabled) {
         qemu_error_new(QERR_UNDEFINED_ERROR);
     } else {
-- 
1.6.6.rc2.5.g49666

Re: [Qemu-devel] [PATCH 5/5] monitor: Catch printing to non-existent monitor

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> The monitor_vprintf() function now touches the 'mon' pointer
> before calling monitor_puts(), this causes block migration
> to segfault as its functions call monitor_printf() with a
> NULL 'mon'.

I figure this worked fine until commit 4a29a85d made monitor_vprintf()
dereference mon.

> To fix the problem this commit moves the 'mon' NULL check
> from monitor_puts() to monitor_vprintf().
>
> This can potentially hide bugs, but for some reason this has
> been the behavior for a long time.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index b518cc4..ebd0282 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -177,9 +177,6 @@ static void monitor_puts(Monitor *mon, const char *str)
>  {
>      char c;
>  
> -    if (!mon)
> -        return;
> -
>      for(;;) {
>          c = *str++;
>          if (c == '\0')
> @@ -195,6 +192,9 @@ static void monitor_puts(Monitor *mon, const char *str)
>  
>  void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  {
> +    if (!mon)
> +        return;
> +
>      if (mon->mc && !mon->mc->print_enabled) {
>          qemu_error_new(QERR_UNDEFINED_ERROR);
>      } else {

There are no other callers of monitor_puts(), so removing the check
there is okay.

Before the code motion, we throw QERR_UNDEFINED_ERROR on
monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
why that's okay?

Re: [Qemu-devel] [PATCH 5/5] monitor: Catch printing to non-existent monitor

[Luiz Capitulino]

On Tue, 15 Dec 2009 10:42:46 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Before the code motion, we throw QERR_UNDEFINED_ERROR on
> monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
> why that's okay?

 We never did that. A call like that will just segfault QEMU
w/o this fix.

Re: [Qemu-devel] [PATCH 5/5] monitor: Catch printing to non-existent monitor

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 15 Dec 2009 10:42:46 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Before the code motion, we throw QERR_UNDEFINED_ERROR on
>> monitor_vprintf(NULL, ...).  Afterwards, we don't.  Could you explain
>> why that's okay?
>
>  We never did that. A call like that will just segfault QEMU
> w/o this fix.

You're right.  Brain fart on my part.