Re: [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 05/04/2016 10:49 AM, Markus Armbruster wrote:
>> Type QJSON lets you build JSON text.  Its interface mirrors (a subset
>> of) abstract JSON syntax.
>> 
>> QAPI output visitors also produce JSON text.  They assert their
>> preconditions and invariants, and therefore abort on incorrect use.
>> 
>> Contrastingly, QJSON does *not* detect incorrect use.  It happily
>> produces invalid JSON then.  This is what migration wants.
>> 
>> QJSON was designed for migration, and migration is its only user.
>
> Worth calling out commits 0457d07..b174257 here?

Can't hurt, will do if I need to respin.

>> Move it to migration/ for proper coverage by MAINTAINERS, and to deter
>> accidental use outside migration.
>> 
>
>> +++ b/include/migration/vmstate.h
>> @@ -29,7 +29,7 @@
>>  #ifndef CONFIG_USER_ONLY
>>  #include <migration/qemu-file.h>
>>  #endif
>> -#include <qjson.h>
>> +#include "migration/qjson.h"
>
> I thought you weren't a fan of including .h from .h, where it was
> avoidable.  But I guess you aren't adding any new .h, so much as
> converting an existing use.

Correct.

>> +
>>  #include "qemu/osdep.h"
>> -#include <qapi/qmp/qstring.h>
>> -#include <glib.h>
>> -#include <qjson.h>
>> -#include <qemu/module.h>
>> -#include <qom/object.h>
>> +#include "qapi/qmp/qstring.h"
>> +#include "migration/qjson.h"
>> +#include "qemu/module.h"
>> +#include "qom/object.h"
>
> Thanks for fixing the mis-use of <> while at it :)

I intend to do that globally, in my header cleanup project.

>> +++ b/migration/vmstate.c
>> @@ -6,7 +6,6 @@
>>  #include "qemu/bitops.h"
>>  #include "qemu/error-report.h"
>>  #include "trace.h"
>> -#include "qjson.h"
>
> This is because you are relying on the .h doing it for you.

Yes.

> As mentioned on the cover letter,
> Reviewed-by: Eric Blake <address@hidden>
> whether or not you touch up the commit message to call out ids

Thanks!