diff mbox

QJSON: Use OBJECT_CHECK

Message ID 1429975686-19494-1-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost April 25, 2015, 3:28 p.m. UTC
The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
There were even some functions using object_dynamic_cast() calls
followed by assert(), which is exactly what OBJECT_CHECK does (by
calling object_dynamic_cast_assert()).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qjson.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Andreas Färber April 25, 2015, 5:05 p.m. UTC | #1
Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> There were even some functions using object_dynamic_cast() calls
> followed by assert(), which is exactly what OBJECT_CHECK does (by
> calling object_dynamic_cast_assert()).

Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qjson.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Wasn't aware QJSON is using QOM - assuming this will go through some
QAPI/QMP tree.

Regards,
Andreas
Eduardo Habkost April 27, 2015, 5:23 p.m. UTC | #2
On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> > There were even some functions using object_dynamic_cast() calls
> > followed by assert(), which is exactly what OBJECT_CHECK does (by
> > calling object_dynamic_cast_assert()).
> 
> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.

I assume it can be fixed during commit by whoever is going to queue it.

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qjson.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> Wasn't aware QJSON is using QOM - assuming this will go through some
> QAPI/QMP tree.

The only user of qjson.c right now is migration code. Should it go through
the migration tree?

Also, why do we have two JSON writers in QEMU? And why do they have
exactly the same name?
Luiz Capitulino April 29, 2015, 12:38 p.m. UTC | #3
On Mon, 27 Apr 2015 14:23:20 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> > Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> > > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> > > There were even some functions using object_dynamic_cast() calls
> > > followed by assert(), which is exactly what OBJECT_CHECK does (by
> > > calling object_dynamic_cast_assert()).
> > 
> > Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.

Everywhere? You mean, in other places? In this case someone has to
post a different patch.

> I assume it can be fixed during commit by whoever is going to queue it.
> 
> > 
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  qjson.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > Reviewed-by: Andreas Färber <afaerber@suse.de>
> > 
> > Wasn't aware QJSON is using QOM - assuming this will go through some
> > QAPI/QMP tree.
> 
> The only user of qjson.c right now is migration code. Should it go through
> the migration tree?

It could be, but I can take it if nobody does.

> Also, why do we have two JSON writers in QEMU? And why do they have
> exactly the same name?

Not sure I got it, which writers?
Andreas Färber April 29, 2015, 12:46 p.m. UTC | #4
Am 29.04.2015 um 14:38 schrieb Luiz Capitulino:
> On Mon, 27 Apr 2015 14:23:20 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
>>> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
>>>> The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
>>>> There were even some functions using object_dynamic_cast() calls
>>>> followed by assert(), which is exactly what OBJECT_CHECK does (by
>>>> calling object_dynamic_cast_assert()).
>>>
>>> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
> 
> Everywhere? You mean, in other places?

No, I count 3x in commit message including subject.

Andreas

> In this case someone has to
> post a different patch.
> 
>> I assume it can be fixed during commit by whoever is going to queue it.
>>
>>>
>>>>
>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> ---
>>>>  qjson.c | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Wasn't aware QJSON is using QOM - assuming this will go through some
>>> QAPI/QMP tree.
>>
>> The only user of qjson.c right now is migration code. Should it go through
>> the migration tree?
> 
> It could be, but I can take it if nobody does.
> 
>> Also, why do we have two JSON writers in QEMU? And why do they have
>> exactly the same name?
> 
> Not sure I got it, which writers?
>
Luiz Capitulino April 29, 2015, 12:54 p.m. UTC | #5
On Wed, 29 Apr 2015 14:46:38 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 29.04.2015 um 14:38 schrieb Luiz Capitulino:
> > On Mon, 27 Apr 2015 14:23:20 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> >> On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> >>> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> >>>> The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> >>>> There were even some functions using object_dynamic_cast() calls
> >>>> followed by assert(), which is exactly what OBJECT_CHECK does (by
> >>>> calling object_dynamic_cast_assert()).
> >>>
> >>> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
> > 
> > Everywhere? You mean, in other places?
> 
> No, I count 3x in commit message including subject.

Ah, the problem is the *commit* message. Okay...

> 
> Andreas
> 
> > In this case someone has to
> > post a different patch.
> > 
> >> I assume it can be fixed during commit by whoever is going to queue it.
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>>> ---
> >>>>  qjson.c | 10 +++++-----
> >>>>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> Reviewed-by: Andreas Färber <afaerber@suse.de>
> >>>
> >>> Wasn't aware QJSON is using QOM - assuming this will go through some
> >>> QAPI/QMP tree.
> >>
> >> The only user of qjson.c right now is migration code. Should it go through
> >> the migration tree?
> > 
> > It could be, but I can take it if nobody does.
> > 
> >> Also, why do we have two JSON writers in QEMU? And why do they have
> >> exactly the same name?
> > 
> > Not sure I got it, which writers?
> > 
> 
>
Eduardo Habkost April 29, 2015, 12:55 p.m. UTC | #6
On Wed, Apr 29, 2015 at 08:38:02AM -0400, Luiz Capitulino wrote:
> On Mon, 27 Apr 2015 14:23:20 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> > > Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> > > > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> > > > There were even some functions using object_dynamic_cast() calls
> > > > followed by assert(), which is exactly what OBJECT_CHECK does (by
> > > > calling object_dynamic_cast_assert()).
> > > 
> > > Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
> 
> Everywhere? You mean, in other places? In this case someone has to
> post a different patch.

Just in the commit message.

> 
> > I assume it can be fixed during commit by whoever is going to queue it.
> > 
> > > 
> > > > 
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > >  qjson.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > Reviewed-by: Andreas Färber <afaerber@suse.de>
> > > 
> > > Wasn't aware QJSON is using QOM - assuming this will go through some
> > > QAPI/QMP tree.
> > 
> > The only user of qjson.c right now is migration code. Should it go through
> > the migration tree?
> 
> It could be, but I can take it if nobody does.

Thanks!

> 
> > Also, why do we have two JSON writers in QEMU? And why do they have
> > exactly the same name?
> 
> Not sure I got it, which writers?

qjson.c and qobject/qjson.c:to_json().
Luiz Capitulino April 29, 2015, 1 p.m. UTC | #7
On Wed, 29 Apr 2015 09:55:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> > > Also, why do we have two JSON writers in QEMU? And why do they have
> > > exactly the same name?
> > 
> > Not sure I got it, which writers?
> 
> qjson.c and qobject/qjson.c:to_json().

Hmm, yeah, you're right.
Paolo Bonzini April 29, 2015, 7:18 p.m. UTC | #8
On 27/04/2015 19:23, Eduardo Habkost wrote:
> Also, why do we have two JSON writers in QEMU? And why do they have
> exactly the same name?

qobject/qjson.c could use qjson.c... but then qjson.c probably should be
changed 1) to not use QOM 2) to use GString instead of QString, just
like most other places that use mutable QStrings.

Paolo
Eduardo Habkost May 1, 2015, 12:04 p.m. UTC | #9
On Wed, Apr 29, 2015 at 09:18:27PM +0200, Paolo Bonzini wrote:
> On 27/04/2015 19:23, Eduardo Habkost wrote:
> > Also, why do we have two JSON writers in QEMU? And why do they have
> > exactly the same name?
> 
> qobject/qjson.c could use qjson.c... but then qjson.c probably should be
> changed 1) to not use QOM 2) to use GString instead of QString, just
> like most other places that use mutable QStrings.

3) to support a larger subset of JSON. It doesn't even escape string
   contents.
Michael Roth May 1, 2015, 4:19 p.m. UTC | #10
Quoting Eduardo Habkost (2015-04-29 07:55:48)
> On Wed, Apr 29, 2015 at 08:38:02AM -0400, Luiz Capitulino wrote:
> > On Mon, 27 Apr 2015 14:23:20 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> > > > Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> > > > > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> > > > > There were even some functions using object_dynamic_cast() calls
> > > > > followed by assert(), which is exactly what OBJECT_CHECK does (by
> > > > > calling object_dynamic_cast_assert()).
> > > > 
> > > > Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
> > 
> > Everywhere? You mean, in other places? In this case someone has to
> > post a different patch.
> 
> Just in the commit message.
> 
> > 
> > > I assume it can be fixed during commit by whoever is going to queue it.
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > ---
> > > > >  qjson.c | 10 +++++-----
> > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > Reviewed-by: Andreas Färber <afaerber@suse.de>
> > > > 
> > > > Wasn't aware QJSON is using QOM - assuming this will go through some
> > > > QAPI/QMP tree.
> > > 
> > > The only user of qjson.c right now is migration code. Should it go through
> > > the migration tree?
> > 
> > It could be, but I can take it if nobody does.
> 
> Thanks!
> 
> > 
> > > Also, why do we have two JSON writers in QEMU? And why do they have
> > > exactly the same name?
> > 
> > Not sure I got it, which writers?
> 
> qjson.c and qobject/qjson.c:to_json().

I'm guessing it's to avoid the need to build up a QObject throughout
the migration code, as opposed to just serializing metadata/vmstate
fields directly to string.

Does make me wonder though why we don't just use visit_type_{int,etc}()
interfaces to build up the QObject through a QMPOutputVisitor, then feed
the resulting QObject through the existing qobject/qjson.c code.

> 
> -- 
> Eduardo
>
Michael Roth May 1, 2015, 4:23 p.m. UTC | #11
Quoting Michael Roth (2015-05-01 11:19:05)
> Quoting Eduardo Habkost (2015-04-29 07:55:48)
> > On Wed, Apr 29, 2015 at 08:38:02AM -0400, Luiz Capitulino wrote:
> > > On Mon, 27 Apr 2015 14:23:20 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> > > > > Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> > > > > > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> > > > > > There were even some functions using object_dynamic_cast() calls
> > > > > > followed by assert(), which is exactly what OBJECT_CHECK does (by
> > > > > > calling object_dynamic_cast_assert()).
> > > > > 
> > > > > Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
> > > 
> > > Everywhere? You mean, in other places? In this case someone has to
> > > post a different patch.
> > 
> > Just in the commit message.
> > 
> > > 
> > > > I assume it can be fixed during commit by whoever is going to queue it.
> > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > ---
> > > > > >  qjson.c | 10 +++++-----
> > > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > 
> > > > > Reviewed-by: Andreas Färber <afaerber@suse.de>
> > > > > 
> > > > > Wasn't aware QJSON is using QOM - assuming this will go through some
> > > > > QAPI/QMP tree.
> > > > 
> > > > The only user of qjson.c right now is migration code. Should it go through
> > > > the migration tree?
> > > 
> > > It could be, but I can take it if nobody does.
> > 
> > Thanks!
> > 
> > > 
> > > > Also, why do we have two JSON writers in QEMU? And why do they have
> > > > exactly the same name?
> > > 
> > > Not sure I got it, which writers?
> > 
> > qjson.c and qobject/qjson.c:to_json().
> 
> I'm guessing it's to avoid the need to build up a QObject throughout
> the migration code, as opposed to just serializing metadata/vmstate
> fields directly to string.
> 
> Does make me wonder though why we don't just use visit_type_{int,etc}()
> interfaces to build up the QObject through a QMPOutputVisitor, then feed
> the resulting QObject through the existing qobject/qjson.c code.

I guess that would affect downtime. Don't imagine it would by much
though. A JSONOutputVisitor that simply wraps JSONMessageParser
and avoids the QObject intermediate might be another option.

> 
> > 
> > -- 
> > Eduardo
> >
Luiz Capitulino May 1, 2015, 5:16 p.m. UTC | #12
On Sat, 25 Apr 2015 12:28:06 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> There were even some functions using object_dynamic_cast() calls
> followed by assert(), which is exactly what OBJECT_CHECK does (by
> calling object_dynamic_cast_assert()).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Applied to the qmp branch, thanks.

> ---
>  qjson.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/qjson.c b/qjson.c
> index 0cda269..e478802 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -24,6 +24,8 @@ struct QJSON {
>      bool omit_comma;
>  };
>  
> +#define QJSON(obj) OBJECT_CHECK(QJSON, (obj), TYPE_QJSON)
> +
>  static void json_emit_element(QJSON *json, const char *name)
>  {
>      /* Check whether we need to print a , before an element */
> @@ -87,7 +89,7 @@ const char *qjson_get_str(QJSON *json)
>  
>  QJSON *qjson_new(void)
>  {
> -    QJSON *json = (QJSON *)object_new(TYPE_QJSON);
> +    QJSON *json = QJSON(object_new(TYPE_QJSON));
>      return json;
>  }
>  
> @@ -98,8 +100,7 @@ void qjson_finish(QJSON *json)
>  
>  static void qjson_initfn(Object *obj)
>  {
> -    QJSON *json = (QJSON *)object_dynamic_cast(obj, TYPE_QJSON);
> -    assert(json);
> +    QJSON *json = QJSON(obj);
>  
>      json->str = qstring_from_str("{ ");
>      json->omit_comma = true;
> @@ -107,9 +108,8 @@ static void qjson_initfn(Object *obj)
>  
>  static void qjson_finalizefn(Object *obj)
>  {
> -    QJSON *json = (QJSON *)object_dynamic_cast(obj, TYPE_QJSON);
> +    QJSON *json = QJSON(obj);
>  
> -    assert(json);
>      qobject_decref(QOBJECT(json->str));
>  }
>
Alexander Graf May 2, 2015, 2:44 p.m. UTC | #13
On 01.05.15 18:23, Michael Roth wrote:
> Quoting Michael Roth (2015-05-01 11:19:05)
>> Quoting Eduardo Habkost (2015-04-29 07:55:48)
>>> On Wed, Apr 29, 2015 at 08:38:02AM -0400, Luiz Capitulino wrote:
>>>> On Mon, 27 Apr 2015 14:23:20 -0300
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>
>>>>> On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
>>>>>> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
>>>>>>> The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
>>>>>>> There were even some functions using object_dynamic_cast() calls
>>>>>>> followed by assert(), which is exactly what OBJECT_CHECK does (by
>>>>>>> calling object_dynamic_cast_assert()).
>>>>>>
>>>>>> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
>>>>
>>>> Everywhere? You mean, in other places? In this case someone has to
>>>> post a different patch.
>>>
>>> Just in the commit message.
>>>
>>>>
>>>>> I assume it can be fixed during commit by whoever is going to queue it.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>>>> ---
>>>>>>>  qjson.c | 10 +++++-----
>>>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>>>>
>>>>>> Wasn't aware QJSON is using QOM - assuming this will go through some
>>>>>> QAPI/QMP tree.
>>>>>
>>>>> The only user of qjson.c right now is migration code. Should it go through
>>>>> the migration tree?
>>>>
>>>> It could be, but I can take it if nobody does.
>>>
>>> Thanks!
>>>
>>>>
>>>>> Also, why do we have two JSON writers in QEMU? And why do they have
>>>>> exactly the same name?
>>>>
>>>> Not sure I got it, which writers?
>>>
>>> qjson.c and qobject/qjson.c:to_json().
>>
>> I'm guessing it's to avoid the need to build up a QObject throughout
>> the migration code, as opposed to just serializing metadata/vmstate
>> fields directly to string.
>>
>> Does make me wonder though why we don't just use visit_type_{int,etc}()
>> interfaces to build up the QObject through a QMPOutputVisitor, then feed
>> the resulting QObject through the existing qobject/qjson.c code.
> 
> I guess that would affect downtime. Don't imagine it would by much
> though. A JSONOutputVisitor that simply wraps JSONMessageParser
> and avoids the QObject intermediate might be another option.

Well, there were a number of reasons why I didn't want to use the
QObject json writer. The biggest one is complexity. We're really trying
to do something incredibly trivial, namely writing json string data
continuously. Allocating all that memory (potentially running oom)
didn't seem incredibly appealing to me.


Alex
Juan Quintela May 5, 2015, 12:43 p.m. UTC | #14
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
>> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
>> > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
>> > There were even some functions using object_dynamic_cast() calls
>> > followed by assert(), which is exactly what OBJECT_CHECK does (by
>> > calling object_dynamic_cast_assert()).
>> 
>> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
>
> I assume it can be fixed during commit by whoever is going to queue it.
>
>> 
>> > 
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > ---
>> >  qjson.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> 
>> Wasn't aware QJSON is using QOM - assuming this will go through some
>> QAPI/QMP tree.
>
> The only user of qjson.c right now is migration code. Should it go through
> the migration tree?


I will take it, but I trust your reviews-by O:-)

>
> Also, why do we have two JSON writers in QEMU? And why do they have
> exactly the same name?

Alex?  I guess alex have this implementation when he did the code long
ago?

Later, Juan.
Luiz Capitulino May 5, 2015, 12:53 p.m. UTC | #15
On Tue, 05 May 2015 14:43:19 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
> >> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
> >> > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
> >> > There were even some functions using object_dynamic_cast() calls
> >> > followed by assert(), which is exactly what OBJECT_CHECK does (by
> >> > calling object_dynamic_cast_assert()).
> >> 
> >> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
> >
> > I assume it can be fixed during commit by whoever is going to queue it.
> >
> >> 
> >> > 
> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > ---
> >> >  qjson.c | 10 +++++-----
> >> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >> 
> >> Reviewed-by: Andreas Färber <afaerber@suse.de>
> >> 
> >> Wasn't aware QJSON is using QOM - assuming this will go through some
> >> QAPI/QMP tree.
> >
> > The only user of qjson.c right now is migration code. Should it go through
> > the migration tree?
> 
> 
> I will take it, but I trust your reviews-by O:-)

I've already applied this one to the QMP tree.

> 
> >
> > Also, why do we have two JSON writers in QEMU? And why do they have
> > exactly the same name?
> 
> Alex?  I guess alex have this implementation when he did the code long
> ago?
> 
> Later, Juan.
>
Juan Quintela May 5, 2015, 1:32 p.m. UTC | #16
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Tue, 05 May 2015 14:43:19 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Sat, Apr 25, 2015 at 07:05:55PM +0200, Andreas Färber wrote:
>> >> Am 25.04.2015 um 17:28 schrieb Eduardo Habkost:
>> >> > The QJSON code used casts to (QJSON*) directly, instead of OBJECT_CHECK.
>> >> > There were even some functions using object_dynamic_cast() calls
>> >> > followed by assert(), which is exactly what OBJECT_CHECK does (by
>> >> > calling object_dynamic_cast_assert()).
>> >> 
>> >> Suggest s/OBJECT_CHECK/OBJECT_CHECK()/g everywhere for clarity.
>> >
>> > I assume it can be fixed during commit by whoever is going to queue it.
>> >
>> >> 
>> >> > 
>> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> >> > ---
>> >> >  qjson.c | 10 +++++-----
>> >> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> >> 
>> >> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> >> 
>> >> Wasn't aware QJSON is using QOM - assuming this will go through some
>> >> QAPI/QMP tree.
>> >
>> > The only user of qjson.c right now is migration code. Should it go through
>> > the migration tree?
>> 
>> 
>> I will take it, but I trust your reviews-by O:-)
>
> I've already applied this one to the QMP tree.

I saw it later on the thread, I was about to say that all for you O:-)


>
>> 
>> >
>> > Also, why do we have two JSON writers in QEMU? And why do they have
>> > exactly the same name?
>> 
>> Alex?  I guess alex have this implementation when he did the code long
>> ago?
>> 
>> Later, Juan.
>>
diff mbox

Patch

diff --git a/qjson.c b/qjson.c
index 0cda269..e478802 100644
--- a/qjson.c
+++ b/qjson.c
@@ -24,6 +24,8 @@  struct QJSON {
     bool omit_comma;
 };
 
+#define QJSON(obj) OBJECT_CHECK(QJSON, (obj), TYPE_QJSON)
+
 static void json_emit_element(QJSON *json, const char *name)
 {
     /* Check whether we need to print a , before an element */
@@ -87,7 +89,7 @@  const char *qjson_get_str(QJSON *json)
 
 QJSON *qjson_new(void)
 {
-    QJSON *json = (QJSON *)object_new(TYPE_QJSON);
+    QJSON *json = QJSON(object_new(TYPE_QJSON));
     return json;
 }
 
@@ -98,8 +100,7 @@  void qjson_finish(QJSON *json)
 
 static void qjson_initfn(Object *obj)
 {
-    QJSON *json = (QJSON *)object_dynamic_cast(obj, TYPE_QJSON);
-    assert(json);
+    QJSON *json = QJSON(obj);
 
     json->str = qstring_from_str("{ ");
     json->omit_comma = true;
@@ -107,9 +108,8 @@  static void qjson_initfn(Object *obj)
 
 static void qjson_finalizefn(Object *obj)
 {
-    QJSON *json = (QJSON *)object_dynamic_cast(obj, TYPE_QJSON);
+    QJSON *json = QJSON(obj);
 
-    assert(json);
     qobject_decref(QOBJECT(json->str));
 }