From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51356)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1ayfUN-0005cO-VB
	for qemu-devel@nongnu.org; Fri, 06 May 2016 09:08:22 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1ayfUC-0006SY-6g
	for qemu-devel@nongnu.org; Fri, 06 May 2016 09:08:10 -0400
Received: from mx1.redhat.com ([209.132.183.28]:58055)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1ayfUC-0006PE-25
	for qemu-devel@nongnu.org; Fri, 06 May 2016 09:08:04 -0400
Received: from int-mx10.intmail.prod.int.phx2.redhat.com
	(int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id DFF4E5F2C
	for <qemu-devel@nongnu.org>; Fri,  6 May 2016 13:07:52 +0000 (UTC)
From: Markus Armbruster <armbru@redhat.com>
References: <1462380558-2030-1-git-send-email-armbru@redhat.com>
	<1462380558-2030-2-git-send-email-armbru@redhat.com>
	<572A2D41.1050805@redhat.com>
Date: Fri, 06 May 2016 15:07:50 +0200
In-Reply-To: <572A2D41.1050805@redhat.com> (Eric Blake's message of "Wed, 4
	May 2016 11:11:29 -0600")
Message-ID: <87r3dfz655.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to
 migration/
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, amit.shah@redhat.com, dgilbert@redhat.com, quintela@redhat.com

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
> whether or not you touch up the commit message to call out ids

Thanks!