[Qemu-devel] [PATCH v0 0/4]: QMP related fixes

[Qemu-devel] [PATCH v0 0/4]: QMP related fixes

[Luiz Capitulino]

 Should be applied on top of feature negotiation series.

 Thanks.

[Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Luiz Capitulino]

Add an assert() to qobject_from_jsonf() to assure that the returned
QObject is not NULL. Currently this is duplicated in the callers.

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

diff --git a/qjson.c b/qjson.c
index 9ad8a91..0922c06 100644
--- a/qjson.c
+++ b/qjson.c
@@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
     obj = qobject_from_jsonv(string, &ap);
     va_end(ap);
 
+    assert(obj != NULL);
     return obj;
 }
 
-- 
1.6.6

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Anthony Liguori]

On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
> Add an assert() to qobject_from_jsonf() to assure that the returned
> QObject is not NULL. Currently this is duplicated in the callers.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   qjson.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/qjson.c b/qjson.c
> index 9ad8a91..0922c06 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>       obj = qobject_from_jsonv(string,&ap);
>       va_end(ap);
>
> +    assert(obj != NULL);
>    

This is wrong.  We may get JSON from an untrusted source.  Callers need 
to deal with failure appropriately.

It just so happens that we only parse JSON from an untrusted source via 
qobject_from_json(), but the trust relationship is not obvious given the 
two functions in their current form.

Regards,

Anthony Liguori

>       return obj;
>   }
>
>    

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
>> Add an assert() to qobject_from_jsonf() to assure that the returned
>> QObject is not NULL. Currently this is duplicated in the callers.
>>
>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>> ---
>>   qjson.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/qjson.c b/qjson.c
>> index 9ad8a91..0922c06 100644
>> --- a/qjson.c
>> +++ b/qjson.c
>> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>       obj = qobject_from_jsonv(string,&ap);
>>       va_end(ap);
>>
>> +    assert(obj != NULL);
>>    
>
> This is wrong.  We may get JSON from an untrusted source.  Callers
> need to deal with failure appropriately.
>
> It just so happens that we only parse JSON from an untrusted source
> via qobject_from_json(), but the trust relationship is not obvious
> given the two functions in their current form.

We have many uses of qobject_from_jsonf() with a literal argument, and
more to come.  Making them all deal with failure would be tedious and
clutter the code.  What about a wrapper function that cannot fail?

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Luiz Capitulino]

On Fri, 05 Feb 2010 10:13:33 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
> >> Add an assert() to qobject_from_jsonf() to assure that the returned
> >> QObject is not NULL. Currently this is duplicated in the callers.
> >>
> >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >> ---
> >>   qjson.c |    1 +
> >>   1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qjson.c b/qjson.c
> >> index 9ad8a91..0922c06 100644
> >> --- a/qjson.c
> >> +++ b/qjson.c
> >> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
> >>       obj = qobject_from_jsonv(string,&ap);
> >>       va_end(ap);
> >>
> >> +    assert(obj != NULL);
> >>    
> >
> > This is wrong.  We may get JSON from an untrusted source.  Callers
> > need to deal with failure appropriately.
> >
> > It just so happens that we only parse JSON from an untrusted source
> > via qobject_from_json(), but the trust relationship is not obvious
> > given the two functions in their current form.
> 
> We have many uses of qobject_from_jsonf() with a literal argument, and
> more to come.  Making them all deal with failure would be tedious and
> clutter the code.  What about a wrapper function that cannot fail?

 As far as I can understand, qobject_from_jsonf() is supposed to
be that wrapper already.

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Luiz Capitulino]

On Thu, 04 Feb 2010 16:31:46 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
> > Add an assert() to qobject_from_jsonf() to assure that the returned
> > QObject is not NULL. Currently this is duplicated in the callers.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> >   qjson.c |    1 +
> >   1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/qjson.c b/qjson.c
> > index 9ad8a91..0922c06 100644
> > --- a/qjson.c
> > +++ b/qjson.c
> > @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
> >       obj = qobject_from_jsonv(string,&ap);
> >       va_end(ap);
> >
> > +    assert(obj != NULL);
> >    
> 
> This is wrong.  We may get JSON from an untrusted source.  Callers need 
> to deal with failure appropriately.

 What kind of untrusted source? This function is only used by handlers
and assuming that the only possible error here is bad syntax, not having
this check in the source will only duplicate it in the users.

> It just so happens that we only parse JSON from an untrusted source via 
> qobject_from_json(), but the trust relationship is not obvious given the 
> two functions in their current form.

 Not exactly, qobject_from_json() is not even being currently used.

 We parse JSON data from clients by using the low-level parser API,
that's by calling json_message_parser_feed() to read the input and
then calling json_parser_parse() when we have collected enough data.

 qobject_from_jsonf() is only used internally, by handlers.

 Both, qobject_from_jsonf() and qobject_from_json() are _wrappers_ to
qobject_from_jsonv(), which uses the low-level API directly.

 So, having the assert() in qobject_from_jsonf() should only
affect handlers, which seems fine to me.

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Anthony Liguori]

On 02/05/2010 06:12 AM, Luiz Capitulino wrote:
> On Thu, 04 Feb 2010 16:31:46 -0600
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
>>      
>>> Add an assert() to qobject_from_jsonf() to assure that the returned
>>> QObject is not NULL. Currently this is duplicated in the callers.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>>    qjson.c |    1 +
>>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qjson.c b/qjson.c
>>> index 9ad8a91..0922c06 100644
>>> --- a/qjson.c
>>> +++ b/qjson.c
>>> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>>        obj = qobject_from_jsonv(string,&ap);
>>>        va_end(ap);
>>>
>>> +    assert(obj != NULL);
>>>
>>>        
>> This is wrong.  We may get JSON from an untrusted source.  Callers need
>> to deal with failure appropriately.
>>      
>   What kind of untrusted source? This function is only used by handlers
> and assuming that the only possible error here is bad syntax, not having
> this check in the source will only duplicate it in the users.
>    

I don't know yet, but there's nothing about this function that indicates 
that it cannot handle malformed JSON.  I don't think it's a reasonable 
expectation either.

There are absolutely ways to mitigate this.  You can use GCC macros to 
enforce at compile time that the string argument is always a literal and 
never a user supplied string.

Run time asserts are a terrible way to deal with reasonably expected errors.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/05/2010 06:12 AM, Luiz Capitulino wrote:
>> On Thu, 04 Feb 2010 16:31:46 -0600
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>
>>    
>>> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
>>>      
>>>> Add an assert() to qobject_from_jsonf() to assure that the returned
>>>> QObject is not NULL. Currently this is duplicated in the callers.
>>>>
>>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>> ---
>>>>    qjson.c |    1 +
>>>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/qjson.c b/qjson.c
>>>> index 9ad8a91..0922c06 100644
>>>> --- a/qjson.c
>>>> +++ b/qjson.c
>>>> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>>>        obj = qobject_from_jsonv(string,&ap);
>>>>        va_end(ap);
>>>>
>>>> +    assert(obj != NULL);
>>>>
>>>>        
>>> This is wrong.  We may get JSON from an untrusted source.  Callers need
>>> to deal with failure appropriately.
>>>      
>>   What kind of untrusted source? This function is only used by handlers
>> and assuming that the only possible error here is bad syntax, not having
>> this check in the source will only duplicate it in the users.
>>    
>
> I don't know yet, but there's nothing about this function that
> indicates that it cannot handle malformed JSON.  I don't think it's a
> reasonable expectation either.
>
> There are absolutely ways to mitigate this.  You can use GCC macros to
> enforce at compile time that the string argument is always a literal
> and never a user supplied string.

A string literal always comes from the programmer, not the user, but the
converse is not true.  Therefore, I don't see why we should make the
function unusable with non-literal arguments.  But if you really want
-Wformat-nonliteral, you know where to find it :)

> Run time asserts are a terrible way to deal with reasonably expected errors.

Yes.  But what's reasonably expected entirely depends on the contract
between the function and its callers.

I think we need a function that cannot fail and shouldn't used with
untrusted arguments (for what it's worth, that's how we use
qobject_from_jsonf() now).  Having related functions with different
contracts is fine with me.

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Luiz Capitulino]

On Fri, 05 Feb 2010 18:14:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:

[...]

> Yes.  But what's reasonably expected entirely depends on the contract
> between the function and its callers.
> 
> I think we need a function that cannot fail and shouldn't used with
> untrusted arguments (for what it's worth, that's how we use
> qobject_from_jsonf() now).  Having related functions with different
> contracts is fine with me.

 I completely agree.

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Anthony Liguori]

On 02/05/2010 11:14 AM, Markus Armbruster wrote:
>> Run time asserts are a terrible way to deal with reasonably expected errors.
>>      
> Yes.  But what's reasonably expected entirely depends on the contract
> between the function and its callers.
>
> I think we need a function that cannot fail and shouldn't used with
> untrusted arguments (for what it's worth, that's how we use
> qobject_from_jsonf() now).  Having related functions with different
> contracts is fine with me.
>    

I think the key point is that if we're going to establish these 
contracts, it must be obvious.

A reasonable programmer is going to assume that if a function can return 
a NULL, it can possibly return an error.  If you want to deviate from 
those semantics, you either have to name the function appropriately or 
put a big comment above the declaration explaining the semantics.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Luiz Capitulino]

On Mon, 08 Feb 2010 08:53:26 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/05/2010 11:14 AM, Markus Armbruster wrote:
> >> Run time asserts are a terrible way to deal with reasonably expected errors.
> >>      
> > Yes.  But what's reasonably expected entirely depends on the contract
> > between the function and its callers.
> >
> > I think we need a function that cannot fail and shouldn't used with
> > untrusted arguments (for what it's worth, that's how we use
> > qobject_from_jsonf() now).  Having related functions with different
> > contracts is fine with me.
> >    
> 
> I think the key point is that if we're going to establish these 
> contracts, it must be obvious.
> 
> A reasonable programmer is going to assume that if a function can return 
> a NULL, it can possibly return an error.  If you want to deviate from 
> those semantics, you either have to name the function appropriately or 
> put a big comment above the declaration explaining the semantics.

 Given that qobject_from_jsonf() is already a good and long name, I
prefer to add the comment.

 I will do that and re-submit.

[Qemu-devel] [PATCH 2/4] Monitor: remove unneeded checks

[Luiz Capitulino]

It's not needed to check the return of qobject_from_jsonf()
anymore, as an assert() has been added there.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c          |    3 ---
 hw/pci-hotplug.c |    1 -
 migration.c      |    3 ---
 monitor.c        |    5 -----
 4 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..89d697e 100644
--- a/block.c
+++ b/block.c
@@ -1259,7 +1259,6 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
                                     "'removable': %i, 'locked': %i }",
                                     bs->device_name, type, bs->removable,
                                     bs->locked);
-        assert(bs_obj != NULL);
 
         if (bs->drv) {
             QObject *obj;
@@ -1270,7 +1269,6 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
                                      bs->filename, bs->read_only,
                                      bs->drv->format_name,
                                      bdrv_is_encrypted(bs));
-            assert(obj != NULL);
             if (bs->backing_file[0] != '\0') {
                 QDict *qdict = qobject_to_qdict(obj);
                 qdict_put(qdict, "backing_file",
@@ -1356,7 +1354,6 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data)
                                  bs->device_name,
                                  bs->rd_bytes, bs->wr_bytes,
                                  bs->rd_ops, bs->wr_ops);
-        assert(obj != NULL);
         qlist_append_obj(devices, obj);
     }
 
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index ba13d2b..0fb96f0 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -285,7 +285,6 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
         qobject_from_jsonf("{ 'domain': 0, 'bus': %d, 'slot': %d, "
                            "'function': %d }", pci_bus_num(dev->bus),
                            PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
-        assert(*ret_data != NULL);
     } else
         monitor_printf(mon, "failed to add %s\n", opts);
 }
diff --git a/migration.c b/migration.c
index 5b28e42..6f26028 100644
--- a/migration.c
+++ b/migration.c
@@ -183,8 +183,6 @@ static void migrate_put_status(QDict *qdict, const char *name,
     obj = qobject_from_jsonf("{ 'transferred': %" PRId64 ", "
                                "'remaining': %" PRId64 ", "
                                "'total': %" PRId64 " }", trans, rem, total);
-    assert(obj != NULL);
-
     qdict_put_obj(qdict, name, obj);
 }
 
@@ -258,7 +256,6 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
             *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }");
             break;
         }
-        assert(*ret_data != NULL);
     }
 }
 
diff --git a/monitor.c b/monitor.c
index a0ec7fc..cb7eb65 100644
--- a/monitor.c
+++ b/monitor.c
@@ -351,8 +351,6 @@ static void timestamp_put(QDict *qdict)
     obj = qobject_from_jsonf("{ 'seconds': %" PRId64 ", "
                                 "'microseconds': %" PRId64 " }",
                                 (int64_t) tv.tv_sec, (int64_t) tv.tv_usec);
-    assert(obj != NULL);
-
     qdict_put_obj(qdict, "timestamp", obj);
 }
 
@@ -897,7 +895,6 @@ static void do_info_cpus(Monitor *mon, QObject **ret_data)
         obj = qobject_from_jsonf("{ 'CPU': %d, 'current': %i, 'halted': %i }",
                                  env->cpu_index, env == mon->mon_cpu,
                                  env->halted);
-        assert(obj != NULL);
 
         cpu = qobject_to_qdict(obj);
 
@@ -4412,8 +4409,6 @@ static void monitor_control_event(void *opaque, int event)
         json_message_parser_init(&mon->mc->parser, handle_qmp_command);
 
         data = get_qmp_greeting();
-        assert(data != NULL);
-
         monitor_json_emitter(mon, data);
         qobject_decref(data);
     }
-- 
1.6.6

[Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults

[Luiz Capitulino]

Ideally, Monitor code should report an error only once and
return the error information up the call chain.

To assure that this happens as expected and that no error is
lost, we have an assert() in qemu_error_internal().

However, we still have not fully converted handlers using
monitor_printf() to report errors. As there can be multiple
monitor_printf() calls on an error, the assertion is easily
triggered when debugging is enabled; and we will get a memory
leak if it's not.

The solution to this problem is to allow multiple faults by only
reporting the first one, and to release the additional error objects.

A better mechanism to report multiple errors to programmers is
underway.

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

diff --git a/monitor.c b/monitor.c
index cb7eb65..c8b63aa 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4625,8 +4625,13 @@ void qemu_error_internal(const char *file, int linenr, const char *func,
         QDECREF(qerror);
         break;
     case ERR_SINK_MONITOR:
-        assert(qemu_error_sink->mon->error == NULL);
-        qemu_error_sink->mon->error = qerror;
+        /* report only the first error */
+        if (!qemu_error_sink->mon->error) {
+            qemu_error_sink->mon->error = qerror;
+        } else {
+            /* XXX: warn the programmer */
+            QDECREF(qerror);
+        }
         break;
     }
 }
-- 
1.6.6

Re: [Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Ideally, Monitor code should report an error only once and
> return the error information up the call chain.
>
> To assure that this happens as expected and that no error is
> lost, we have an assert() in qemu_error_internal().
>
> However, we still have not fully converted handlers using
> monitor_printf() to report errors. As there can be multiple
> monitor_printf() calls on an error, the assertion is easily
> triggered when debugging is enabled; and we will get a memory
> leak if it's not.
>
> The solution to this problem is to allow multiple faults by only
> reporting the first one, and to release the additional error objects.

I want this badly.

[...]

Re: [Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> Ideally, Monitor code should report an error only once and
>> return the error information up the call chain.
>>
>> To assure that this happens as expected and that no error is
>> lost, we have an assert() in qemu_error_internal().
>>
>> However, we still have not fully converted handlers using
>> monitor_printf() to report errors. As there can be multiple
>> monitor_printf() calls on an error, the assertion is easily
>> triggered when debugging is enabled; and we will get a memory
>> leak if it's not.
>>
>> The solution to this problem is to allow multiple faults by only
>> reporting the first one, and to release the additional error objects.
>
> I want this badly.
>
> [...]

Let me elaborate a bit.  While this patch is a much wanted improvement,
what I *really* want is something else.

Right now, we have 41 uses of qemu_error_new().  We still have >300 uses
of monitor_printf(), many of them errors.  Plus some 100 uses of
qemu_error(), which boils down to monitor_printf() when running within a
monitor.  Not to mention >1000 uses of stderr.

To convert a monitor handler to QError, we have to make it report
exactly one error on every unsuccessful path, with qemu_error_new().
That's not too hard.  Then we have to ensure it does not call
monitor_printf() directly (not hard either) or indirectly (ouch).  I say
"ouch", because those prints can hide behind long call chains, in code
shared with other users.  Cleaning up all those stray prints will take
time.

Without this patch, a stray print is fatal, unless it happens to be the
only one *and* there is no real error.

With this patch, we survive, but the UndefinedError triggered by the
stray print displaces any later real error.

What I really want is that stray prints do not mess with my real errors.

Re: [Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults

[Luiz Capitulino]

On Fri, 05 Feb 2010 15:21:13 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >
> >> Ideally, Monitor code should report an error only once and
> >> return the error information up the call chain.
> >>
> >> To assure that this happens as expected and that no error is
> >> lost, we have an assert() in qemu_error_internal().
> >>
> >> However, we still have not fully converted handlers using
> >> monitor_printf() to report errors. As there can be multiple
> >> monitor_printf() calls on an error, the assertion is easily
> >> triggered when debugging is enabled; and we will get a memory
> >> leak if it's not.
> >>
> >> The solution to this problem is to allow multiple faults by only
> >> reporting the first one, and to release the additional error objects.
> >
> > I want this badly.
> >
> > [...]
> 
> Let me elaborate a bit.  While this patch is a much wanted improvement,
> what I *really* want is something else.
> 
> Right now, we have 41 uses of qemu_error_new().  We still have >300 uses
> of monitor_printf(), many of them errors.  Plus some 100 uses of
> qemu_error(), which boils down to monitor_printf() when running within a
> monitor.  Not to mention >1000 uses of stderr.
> 
> To convert a monitor handler to QError, we have to make it report
> exactly one error on every unsuccessful path, with qemu_error_new().
> That's not too hard.  Then we have to ensure it does not call
> monitor_printf() directly (not hard either) or indirectly (ouch).  I say
> "ouch", because those prints can hide behind long call chains, in code
> shared with other users.  Cleaning up all those stray prints will take
> time.

 As we have talked, this situation will be improved by making cmd_new
return an error code, right?

 I've started working on it already, patches will be sent soon.

> Without this patch, a stray print is fatal, unless it happens to be the
> only one *and* there is no real error.
> 
> With this patch, we survive, but the UndefinedError triggered by the
> stray print displaces any later real error.
> 
> What I really want is that stray prints do not mess with my real errors.

 There are two issues here:

1. In command handlers stray prints _usually_ report errors. If we go
with shallow conversion, I believe that clients should be informed
(in the form of an undefined error) that monitor_printf() has been
called

2. We have agreed that multiple faults are not allowed and that
reporting only the first one is fine. In shallow conversion (or
even in buggy conversions) we can get multiple faults and we have
to handle it

 So, the situation will be improved by my next series as we can
use the return code to 'audit' qemu_error_new() usage (which
includes monitor_printf() calls).

Re: [Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 05 Feb 2010 15:21:13 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >
>> >> Ideally, Monitor code should report an error only once and
>> >> return the error information up the call chain.
>> >>
>> >> To assure that this happens as expected and that no error is
>> >> lost, we have an assert() in qemu_error_internal().
>> >>
>> >> However, we still have not fully converted handlers using
>> >> monitor_printf() to report errors. As there can be multiple
>> >> monitor_printf() calls on an error, the assertion is easily
>> >> triggered when debugging is enabled; and we will get a memory
>> >> leak if it's not.
>> >>
>> >> The solution to this problem is to allow multiple faults by only
>> >> reporting the first one, and to release the additional error objects.
>> >
>> > I want this badly.
>> >
>> > [...]
>> 
>> Let me elaborate a bit.  While this patch is a much wanted improvement,
>> what I *really* want is something else.
>> 
>> Right now, we have 41 uses of qemu_error_new().  We still have >300 uses
>> of monitor_printf(), many of them errors.  Plus some 100 uses of
>> qemu_error(), which boils down to monitor_printf() when running within a
>> monitor.  Not to mention >1000 uses of stderr.
>> 
>> To convert a monitor handler to QError, we have to make it report
>> exactly one error on every unsuccessful path, with qemu_error_new().
>> That's not too hard.  Then we have to ensure it does not call
>> monitor_printf() directly (not hard either) or indirectly (ouch).  I say
>> "ouch", because those prints can hide behind long call chains, in code
>> shared with other users.  Cleaning up all those stray prints will take
>> time.
>
>  As we have talked, this situation will be improved by making cmd_new
> return an error code, right?

Yes.

>  I've started working on it already, patches will be sent soon.

Excellent.

>> Without this patch, a stray print is fatal, unless it happens to be the
>> only one *and* there is no real error.
>> 
>> With this patch, we survive, but the UndefinedError triggered by the
>> stray print displaces any later real error.
>> 
>> What I really want is that stray prints do not mess with my real errors.
>
>  There are two issues here:
>
> 1. In command handlers stray prints _usually_ report errors. If we go
> with shallow conversion, I believe that clients should be informed
> (in the form of an undefined error) that monitor_printf() has been
> called

It's not so easy.

A command should report an error if and only if it really failed.
Reporting an error even though the command succeeded is just as bad as
not reporting an error when it failed.

Barring bugs, a handler *knows* whether it got the job done or not.  It
can and should communicate that knowledge up by returning status.  I
understand one of your next patch series will do that.

If a handler returns failure, and we haven't reported an error, we must
report UndefinedError whether we had stray prints or not.

If a handler returns success, we should *not* report UndefinedError just
because it had stray prints.  A stray print does not necessarily imply
command failure, and hence a stray print should not make an otherwise
successful command fail.

Adding a suitable mechanism to alert developers to stray prints is fine.
But abusing error replies for that is not.

> 2. We have agreed that multiple faults are not allowed and that
> reporting only the first one is fine. In shallow conversion (or
> even in buggy conversions) we can get multiple faults and we have
> to handle it

Reporting only the first one sucks when it's an UndefinedError triggered
by a stray print, while one of the other ones is the real error.

>  So, the situation will be improved by my next series as we can
> use the return code to 'audit' qemu_error_new() usage (which
> includes monitor_printf() calls).

Yes.

Re: [Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults

[Luiz Capitulino]

On Fri, 05 Feb 2010 16:15:56 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 05 Feb 2010 15:21:13 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> 
> >> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >
> >> >> Ideally, Monitor code should report an error only once and
> >> >> return the error information up the call chain.
> >> >>
> >> >> To assure that this happens as expected and that no error is
> >> >> lost, we have an assert() in qemu_error_internal().
> >> >>
> >> >> However, we still have not fully converted handlers using
> >> >> monitor_printf() to report errors. As there can be multiple
> >> >> monitor_printf() calls on an error, the assertion is easily
> >> >> triggered when debugging is enabled; and we will get a memory
> >> >> leak if it's not.
> >> >>
> >> >> The solution to this problem is to allow multiple faults by only
> >> >> reporting the first one, and to release the additional error objects.
> >> >
> >> > I want this badly.
> >> >
> >> > [...]
> >> 
> >> Let me elaborate a bit.  While this patch is a much wanted improvement,
> >> what I *really* want is something else.
> >> 
> >> Right now, we have 41 uses of qemu_error_new().  We still have >300 uses
> >> of monitor_printf(), many of them errors.  Plus some 100 uses of
> >> qemu_error(), which boils down to monitor_printf() when running within a
> >> monitor.  Not to mention >1000 uses of stderr.
> >> 
> >> To convert a monitor handler to QError, we have to make it report
> >> exactly one error on every unsuccessful path, with qemu_error_new().
> >> That's not too hard.  Then we have to ensure it does not call
> >> monitor_printf() directly (not hard either) or indirectly (ouch).  I say
> >> "ouch", because those prints can hide behind long call chains, in code
> >> shared with other users.  Cleaning up all those stray prints will take
> >> time.
> >
> >  As we have talked, this situation will be improved by making cmd_new
> > return an error code, right?
> 
> Yes.
> 
> >  I've started working on it already, patches will be sent soon.
> 
> Excellent.
> 
> >> Without this patch, a stray print is fatal, unless it happens to be the
> >> only one *and* there is no real error.
> >> 
> >> With this patch, we survive, but the UndefinedError triggered by the
> >> stray print displaces any later real error.
> >> 
> >> What I really want is that stray prints do not mess with my real errors.
> >
> >  There are two issues here:
> >
> > 1. In command handlers stray prints _usually_ report errors. If we go
> > with shallow conversion, I believe that clients should be informed
> > (in the form of an undefined error) that monitor_printf() has been
> > called
> 
> It's not so easy.
> 
> A command should report an error if and only if it really failed.
> Reporting an error even though the command succeeded is just as bad as
> not reporting an error when it failed.
> 
> Barring bugs, a handler *knows* whether it got the job done or not.  It
> can and should communicate that knowledge up by returning status.  I
> understand one of your next patch series will do that.

 Yes.

> If a handler returns failure, and we haven't reported an error, we must
> report UndefinedError whether we had stray prints or not.

 Agreed and the stray prints have to reported to the developer.

> If a handler returns success, we should *not* report UndefinedError just
> because it had stray prints.  A stray print does not necessarily imply
> command failure, and hence a stray print should not make an otherwise
> successful command fail.

 Right, but a stray print which is not reporting an error and which is
not just OK, is returning data.

 We have to detect this, because this has to be done by using the
QObject API and shallow conversion can't miss that.

 I'm not saying we should use UndefinedError, I'm saying we have to
have a mechanism to detect this reliably. Unfortunately this can
only be detected at run time.

[Qemu-devel] [PATCH 4/4] QMP: Don't leak on connection close

[Luiz Capitulino]

QMP's chardev event callback doesn't call
json_message_parser_destroy() on CHR_EVENT_CLOSED. As the call
to json_message_parser_init() on CHR_EVENT_OPENED allocates memory,
we'are leaking on close.

Fix that by just calling json_message_parser_destroy() on
CHR_EVENT_CLOSED.

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

diff --git a/monitor.c b/monitor.c
index c8b63aa..aacc0af 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4401,16 +4401,20 @@ static QObject *get_qmp_greeting(void)
  */
 static void monitor_control_event(void *opaque, int event)
 {
-    if (event == CHR_EVENT_OPENED) {
-        QObject *data;
-        Monitor *mon = opaque;
+    QObject *data;
+    Monitor *mon = opaque;
 
+    switch (event) {
+    case CHR_EVENT_OPENED:
         mon->mc->command_mode = 0;
         json_message_parser_init(&mon->mc->parser, handle_qmp_command);
-
         data = get_qmp_greeting();
         monitor_json_emitter(mon, data);
         qobject_decref(data);
+        break;
+    case CHR_EVENT_CLOSED:
+        json_message_parser_destroy(&mon->mc->parser);
+        break;
     }
 }
 
-- 
1.6.6

[Qemu-devel] [PATCH v1 0/4]: QMP related fixes

[Luiz Capitulino]

 Should be applied on top of feature negotiation series.

changelog:
----------

v0 -> v1:

- Document qobject_from_jsonf() new semantics

[Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Luiz Capitulino]

Add an assert() to qobject_from_jsonf() to assure that the returned
QObject is not NULL. Currently this is duplicated in the callers.

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

diff --git a/qjson.c b/qjson.c
index 9ad8a91..483c667 100644
--- a/qjson.c
+++ b/qjson.c
@@ -53,6 +53,10 @@ QObject *qobject_from_json(const char *string)
     return qobject_from_jsonv(string, NULL);
 }
 
+/*
+ * IMPORTANT: This function aborts on error, thus it must not
+ * be used with untrusted arguments.
+ */
 QObject *qobject_from_jsonf(const char *string, ...)
 {
     QObject *obj;
@@ -62,6 +66,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
     obj = qobject_from_jsonv(string, &ap);
     va_end(ap);
 
+    assert(obj != NULL);
     return obj;
 }
 
-- 
1.6.6

Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Anthony Liguori]

On 02/08/2010 01:01 PM, Luiz Capitulino wrote:
> Add an assert() to qobject_from_jsonf() to assure that the returned
> QObject is not NULL. Currently this is duplicated in the callers.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Applied all.  Thanks.

Regards,

Anthony Liguori

> ---
>   qjson.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/qjson.c b/qjson.c
> index 9ad8a91..483c667 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -53,6 +53,10 @@ QObject *qobject_from_json(const char *string)
>       return qobject_from_jsonv(string, NULL);
>   }
>
> +/*
> + * IMPORTANT: This function aborts on error, thus it must not
> + * be used with untrusted arguments.
> + */
>   QObject *qobject_from_jsonf(const char *string, ...)
>   {
>       QObject *obj;
> @@ -62,6 +66,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>       obj = qobject_from_jsonv(string,&ap);
>       va_end(ap);
>
> +    assert(obj != NULL);
>       return obj;
>   }
>
>