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

2016-05-06 Thread Markus Armbruster 
Eric Blake  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 
>>  #endif
>> -#include 
>> +#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 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> +#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 
> whether or not you touch up the commit message to call out ids

Thanks!

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

2016-05-05 Thread Dr. David Alan Gilbert 
* Markus Armbruster (arm...@redhat.com) 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.
> Move it to migration/ for proper coverage by MAINTAINERS, and to deter
> accidental use outside migration.
> 
> Signed-off-by: Markus Armbruster 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  Makefile.objs   |  1 -
>  include/{ => migration}/qjson.h |  0
>  include/migration/vmstate.h |  2 +-
>  migration/Makefile.objs |  1 +
>  qjson.c => migration/qjson.c| 23 +--
>  migration/vmstate.c |  1 -
>  tests/Makefile  |  2 +-
>  7 files changed, 20 insertions(+), 10 deletions(-)
>  rename include/{ => migration}/qjson.h (100%)
>  rename qjson.c => migration/qjson.c (83%)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 8f705f6..da49b71 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -52,7 +52,6 @@ common-obj-$(CONFIG_LINUX) += fsdev/
>  common-obj-y += migration/
>  common-obj-y += qemu-char.o #aio.o
>  common-obj-y += page_cache.o
> -common-obj-y += qjson.o
>  
>  common-obj-$(CONFIG_SPICE) += spice-qemu-char.o
>  
> diff --git a/include/qjson.h b/include/migration/qjson.h
> similarity index 100%
> rename from include/qjson.h
> rename to include/migration/qjson.h
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 84ee355..30ecc44 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -29,7 +29,7 @@
>  #ifndef CONFIG_USER_ONLY
>  #include 
>  #endif
> -#include 
> +#include "migration/qjson.h"
>  
>  typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>  typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
> diff --git a/migration/Makefile.objs b/migration/Makefile.objs
> index 0cac6d7..d25ff48 100644
> --- a/migration/Makefile.objs
> +++ b/migration/Makefile.objs
> @@ -2,6 +2,7 @@ common-obj-y += migration.o tcp.o
>  common-obj-y += vmstate.o
>  common-obj-y += qemu-file.o qemu-file-buf.o qemu-file-unix.o 
> qemu-file-stdio.o
>  common-obj-y += xbzrle.o postcopy-ram.o
> +common-obj-y += qjson.o
>  
>  common-obj-$(CONFIG_RDMA) += rdma.o
>  common-obj-$(CONFIG_POSIX) += exec.o unix.o fd.o
> diff --git a/qjson.c b/migration/qjson.c
> similarity index 83%
> rename from qjson.c
> rename to migration/qjson.c
> index b65ca6e..cb479fe 100644
> --- a/qjson.c
> +++ b/migration/qjson.c
> @@ -1,5 +1,5 @@
>  /*
> - * QEMU JSON writer
> + * A simple JSON writer
>   *
>   * Copyright Alexander Graf
>   *
> @@ -11,12 +11,23 @@
>   *
>   */
>  
> +/*
> + * Type QJSON lets you build JSON text.  Its interface mirrors (a
> + * subset of) abstract JSON syntax.
> + *
> + * It does *not* detect incorrect use.  It happily produces invalid
> + * JSON then.  This is what migration wants.
> + *
> + * QAPI output visitors also produce JSON text.  However, they do
> + * assert their preconditions and invariants, and therefore abort on
> + * incorrect use.
> + */
> +
>  #include "qemu/osdep.h"
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> +#include "qapi/qmp/qstring.h"
> +#include "migration/qjson.h"
> +#include "qemu/module.h"
> +#include "qom/object.h"
>  
>  struct QJSON {
>  Object obj;
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index bf3d5db..46dc55e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -6,7 +6,6 @@
>  #include "qemu/bitops.h"
>  #include "qemu/error-report.h"
>  #include "trace.h"
> -#include "qjson.h"
>  
>  static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription 
> *vmsd,
>  void *opaque, QJSON *vmdesc);
> diff --git a/tests/Makefile b/tests/Makefile
> index 9194f18..4204d97 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -435,7 +435,7 @@ tests/test-qdev-global-props$(EXESUF): 
> tests/test-qdev-global-props.o \
>   $(test-qapi-obj-y)
>  tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
>   migration/vmstate.o migration/qemu-file.o migration/qemu-file-buf.o \
> -migration/qemu-file-unix.o qjson.o \
> +migration/qemu-file-unix.o migration/qjson.o \
>   $(test-qom-obj-y)
>  tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
>   $(test-util-obj-y)
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

2016-05-04 Thread Eric Blake 
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?

> 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 
>  #endif
> -#include 
> +#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.

> +
>  #include "qemu/osdep.h"
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> +#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 :)

> +++ 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.

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

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2016-05-04 Thread Markus Armbruster 
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.
Move it to migration/ for proper coverage by MAINTAINERS, and to deter
accidental use outside migration.

Signed-off-by: Markus Armbruster 
---
 Makefile.objs   |  1 -
 include/{ => migration}/qjson.h |  0
 include/migration/vmstate.h |  2 +-
 migration/Makefile.objs |  1 +
 qjson.c => migration/qjson.c| 23 +--
 migration/vmstate.c |  1 -
 tests/Makefile  |  2 +-
 7 files changed, 20 insertions(+), 10 deletions(-)
 rename include/{ => migration}/qjson.h (100%)
 rename qjson.c => migration/qjson.c (83%)

diff --git a/Makefile.objs b/Makefile.objs
index 8f705f6..da49b71 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -52,7 +52,6 @@ common-obj-$(CONFIG_LINUX) += fsdev/
 common-obj-y += migration/
 common-obj-y += qemu-char.o #aio.o
 common-obj-y += page_cache.o
-common-obj-y += qjson.o
 
 common-obj-$(CONFIG_SPICE) += spice-qemu-char.o
 
diff --git a/include/qjson.h b/include/migration/qjson.h
similarity index 100%
rename from include/qjson.h
rename to include/migration/qjson.h
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 84ee355..30ecc44 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -29,7 +29,7 @@
 #ifndef CONFIG_USER_ONLY
 #include 
 #endif
-#include 
+#include "migration/qjson.h"
 
 typedef void SaveStateHandler(QEMUFile *f, void *opaque);
 typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 0cac6d7..d25ff48 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -2,6 +2,7 @@ common-obj-y += migration.o tcp.o
 common-obj-y += vmstate.o
 common-obj-y += qemu-file.o qemu-file-buf.o qemu-file-unix.o qemu-file-stdio.o
 common-obj-y += xbzrle.o postcopy-ram.o
+common-obj-y += qjson.o
 
 common-obj-$(CONFIG_RDMA) += rdma.o
 common-obj-$(CONFIG_POSIX) += exec.o unix.o fd.o
diff --git a/qjson.c b/migration/qjson.c
similarity index 83%
rename from qjson.c
rename to migration/qjson.c
index b65ca6e..cb479fe 100644
--- a/qjson.c
+++ b/migration/qjson.c
@@ -1,5 +1,5 @@
 /*
- * QEMU JSON writer
+ * A simple JSON writer
  *
  * Copyright Alexander Graf
  *
@@ -11,12 +11,23 @@
  *
  */
 
+/*
+ * Type QJSON lets you build JSON text.  Its interface mirrors (a
+ * subset of) abstract JSON syntax.
+ *
+ * It does *not* detect incorrect use.  It happily produces invalid
+ * JSON then.  This is what migration wants.
+ *
+ * QAPI output visitors also produce JSON text.  However, they do
+ * assert their preconditions and invariants, and therefore abort on
+ * incorrect use.
+ */
+
 #include "qemu/osdep.h"
-#include 
-#include 
-#include 
-#include 
-#include 
+#include "qapi/qmp/qstring.h"
+#include "migration/qjson.h"
+#include "qemu/module.h"
+#include "qom/object.h"
 
 struct QJSON {
 Object obj;
diff --git a/migration/vmstate.c b/migration/vmstate.c
index bf3d5db..46dc55e 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -6,7 +6,6 @@
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
 #include "trace.h"
-#include "qjson.h"
 
 static void vmstate_subsection_save(QEMUFile *f, const VMStateDescription 
*vmsd,
 void *opaque, QJSON *vmdesc);
diff --git a/tests/Makefile b/tests/Makefile
index 9194f18..4204d97 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -435,7 +435,7 @@ tests/test-qdev-global-props$(EXESUF): 
tests/test-qdev-global-props.o \
$(test-qapi-obj-y)
 tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
migration/vmstate.o migration/qemu-file.o migration/qemu-file-buf.o \
-migration/qemu-file-unix.o qjson.o \
+migration/qemu-file-unix.o migration/qjson.o \
$(test-qom-obj-y)
 tests/test-timed-average$(EXESUF): tests/test-timed-average.o qemu-timer.o \
$(test-util-obj-y)
-- 
2.5.5