qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up
@ 2016-05-04 16:49 Markus Armbruster
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/ Markus Armbruster
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Markus Armbruster @ 2016-05-04 16:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: dgilbert, eblake, quintela, amit.shah

qjson.[ch] is a JSON writer used by migration.  Eric proposed to
replace it by common code in his "Add qapi-to-JSON and clone visitors"
series.  David's review led to the conclusion that migration would
prefer to keep its own JSON writer, to better serve its requirements.
PATCH 1 move it to its proper place, and explains why it exists in a
bit more detail.  PATCH 2 simplifies it a bit.

Markus Armbruster (2):
  migration: Move qjson.[ch] to migration/
  migration/qjson: Drop gratuitous use of QOM

 Makefile.objs                   |  1 -
 include/{ => migration}/qjson.h |  2 +-
 include/migration/vmstate.h     |  2 +-
 migration/Makefile.objs         |  1 +
 qjson.c => migration/qjson.c    | 58 +++++++++++++++--------------------------
 migration/savevm.c              |  2 +-
 migration/vmstate.c             |  1 -
 tests/Makefile                  |  2 +-
 8 files changed, 26 insertions(+), 43 deletions(-)
 rename include/{ => migration}/qjson.h (95%)
 rename qjson.c => migration/qjson.c (70%)

-- 
2.5.5

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/
  2016-05-04 16:49 [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Markus Armbruster
@ 2016-05-04 16:49 ` Markus Armbruster
  2016-05-04 17:11   ` Eric Blake
  2016-05-05 13:52   ` Dr. David Alan Gilbert
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 2/2] migration/qjson: Drop gratuitous use of QOM Markus Armbruster
  2016-05-04 17:08 ` [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Eric Blake
  2 siblings, 2 replies; 9+ messages in thread
From: Markus Armbruster @ 2016-05-04 16:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: dgilbert, eblake, quintela, amit.shah

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 <armbru@redhat.com>
---
 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 <migration/qemu-file.h>
 #endif
-#include <qjson.h>
+#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 <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"
 
 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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/2] migration/qjson: Drop gratuitous use of QOM
  2016-05-04 16:49 [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Markus Armbruster
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/ Markus Armbruster
@ 2016-05-04 16:49 ` Markus Armbruster
  2016-05-04 17:08 ` [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Eric Blake
  2 siblings, 0 replies; 9+ messages in thread
From: Markus Armbruster @ 2016-05-04 16:49 UTC (permalink / raw)
  To: qemu-devel; +Cc: dgilbert, eblake, quintela, amit.shah

All the use of QOM buys us here is the ability to destroy the thing
with object_unref(OBJECT(vmdesc)).  Not worth the notational overhead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/migration/qjson.h |  2 +-
 migration/qjson.c         | 39 ++++++---------------------------------
 migration/savevm.c        |  2 +-
 3 files changed, 8 insertions(+), 35 deletions(-)

diff --git a/include/migration/qjson.h b/include/migration/qjson.h
index 7c54fdf..2978b5f 100644
--- a/include/migration/qjson.h
+++ b/include/migration/qjson.h
@@ -13,10 +13,10 @@
 #ifndef QEMU_QJSON_H
 #define QEMU_QJSON_H
 
-#define TYPE_QJSON "QJSON"
 typedef struct QJSON QJSON;
 
 QJSON *qjson_new(void);
+void qjson_destroy(QJSON *json);
 void json_prop_str(QJSON *json, const char *name, const char *str);
 void json_prop_int(QJSON *json, const char *name, int64_t val);
 void json_end_array(QJSON *json);
diff --git a/migration/qjson.c b/migration/qjson.c
index cb479fe..5cae55a 100644
--- a/migration/qjson.c
+++ b/migration/qjson.c
@@ -26,17 +26,12 @@
 #include "qemu/osdep.h"
 #include "qapi/qmp/qstring.h"
 #include "migration/qjson.h"
-#include "qemu/module.h"
-#include "qom/object.h"
 
 struct QJSON {
-    Object obj;
     QString *str;
     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 */
@@ -100,7 +95,10 @@ const char *qjson_get_str(QJSON *json)
 
 QJSON *qjson_new(void)
 {
-    QJSON *json = QJSON(object_new(TYPE_QJSON));
+    QJSON *json = g_new0(QJSON, 1);
+
+    json->str = qstring_from_str("{ ");
+    json->omit_comma = true;
     return json;
 }
 
@@ -109,32 +107,7 @@ void qjson_finish(QJSON *json)
     json_end_object(json);
 }
 
-static void qjson_initfn(Object *obj)
+void qjson_destroy(QJSON *json)
 {
-    QJSON *json = QJSON(obj);
-
-    json->str = qstring_from_str("{ ");
-    json->omit_comma = true;
+    g_free(json);
 }
-
-static void qjson_finalizefn(Object *obj)
-{
-    QJSON *json = QJSON(obj);
-
-    qobject_decref(QOBJECT(json->str));
-}
-
-static const TypeInfo qjson_type_info = {
-    .name = TYPE_QJSON,
-    .parent = TYPE_OBJECT,
-    .instance_size = sizeof(QJSON),
-    .instance_init = qjson_initfn,
-    .instance_finalize = qjson_finalizefn,
-};
-
-static void qjson_register_types(void)
-{
-    type_register_static(&qjson_type_info);
-}
-
-type_init(qjson_register_types)
diff --git a/migration/savevm.c b/migration/savevm.c
index 16ba443..c5937c8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1114,7 +1114,7 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
         qemu_put_be32(f, vmdesc_len);
         qemu_put_buffer(f, (uint8_t *)qjson_get_str(vmdesc), vmdesc_len);
     }
-    object_unref(OBJECT(vmdesc));
+    qjson_destroy(vmdesc);
 
     qemu_fflush(f);
 }
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up
  2016-05-04 16:49 [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Markus Armbruster
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/ Markus Armbruster
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 2/2] migration/qjson: Drop gratuitous use of QOM Markus Armbruster
@ 2016-05-04 17:08 ` Eric Blake
  2016-05-06 13:11   ` Markus Armbruster
  2 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2016-05-04 17:08 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: dgilbert, quintela, amit.shah

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

On 05/04/2016 10:49 AM, Markus Armbruster wrote:
> qjson.[ch] is a JSON writer used by migration.  Eric proposed to
> replace it by common code in his "Add qapi-to-JSON and clone visitors"
> series.  David's review led to the conclusion that migration would
> prefer to keep its own JSON writer, to better serve its requirements.
> PATCH 1 move it to its proper place, and explains why it exists in a
> bit more detail.  PATCH 2 simplifies it a bit.

Whose tree would this go in through?

At any rate, series:
Reviewed-by: Eric Blake <eblake@redhat.com>
but see nit on 1/2

> 
> Markus Armbruster (2):
>   migration: Move qjson.[ch] to migration/
>   migration/qjson: Drop gratuitous use of QOM
> 
>  Makefile.objs                   |  1 -
>  include/{ => migration}/qjson.h |  2 +-
>  include/migration/vmstate.h     |  2 +-
>  migration/Makefile.objs         |  1 +
>  qjson.c => migration/qjson.c    | 58 +++++++++++++++--------------------------
>  migration/savevm.c              |  2 +-
>  migration/vmstate.c             |  1 -
>  tests/Makefile                  |  2 +-
>  8 files changed, 26 insertions(+), 43 deletions(-)
>  rename include/{ => migration}/qjson.h (95%)
>  rename qjson.c => migration/qjson.c (70%)
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/ Markus Armbruster
@ 2016-05-04 17:11   ` Eric Blake
  2016-05-06 13:07     ` Markus Armbruster
  2016-05-05 13:52   ` Dr. David Alan Gilbert
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Blake @ 2016-05-04 17:11 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: dgilbert, quintela, amit.shah

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

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

> +
>  #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 :)

> +++ 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 <eblake@redhat.com>
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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/
  2016-05-04 16:49 ` [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/ Markus Armbruster
  2016-05-04 17:11   ` Eric Blake
@ 2016-05-05 13:52   ` Dr. David Alan Gilbert
  1 sibling, 0 replies; 9+ messages in thread
From: Dr. David Alan Gilbert @ 2016-05-05 13:52 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, eblake, quintela, amit.shah

* Markus Armbruster (armbru@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 <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  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 <migration/qemu-file.h>
>  #endif
> -#include <qjson.h>
> +#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 <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"
>  
>  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 / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/
  2016-05-04 17:11   ` Eric Blake
@ 2016-05-06 13:07     ` Markus Armbruster
  0 siblings, 0 replies; 9+ messages in thread
From: Markus Armbruster @ 2016-05-06 13:07 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, amit.shah, dgilbert, quintela

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!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up
  2016-05-04 17:08 ` [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Eric Blake
@ 2016-05-06 13:11   ` Markus Armbruster
  2016-05-23  8:36     ` Amit Shah
  0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2016-05-06 13:11 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, amit.shah, dgilbert, quintela

Eric Blake <eblake@redhat.com> writes:

> On 05/04/2016 10:49 AM, Markus Armbruster wrote:
>> qjson.[ch] is a JSON writer used by migration.  Eric proposed to
>> replace it by common code in his "Add qapi-to-JSON and clone visitors"
>> series.  David's review led to the conclusion that migration would
>> prefer to keep its own JSON writer, to better serve its requirements.
>> PATCH 1 move it to its proper place, and explains why it exists in a
>> bit more detail.  PATCH 2 simplifies it a bit.
>
> Whose tree would this go in through?

Migration is the natural choice.  Would that inconvenience you?

> At any rate, series:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> but see nit on 1/2

Perhaps the maintainer can add the commit hashes on merge.

Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up
  2016-05-06 13:11   ` Markus Armbruster
@ 2016-05-23  8:36     ` Amit Shah
  0 siblings, 0 replies; 9+ messages in thread
From: Amit Shah @ 2016-05-23  8:36 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Eric Blake, qemu-devel, dgilbert, quintela

On (Fri) 06 May 2016 [15:11:04], Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 05/04/2016 10:49 AM, Markus Armbruster wrote:
> >> qjson.[ch] is a JSON writer used by migration.  Eric proposed to
> >> replace it by common code in his "Add qapi-to-JSON and clone visitors"
> >> series.  David's review led to the conclusion that migration would
> >> prefer to keep its own JSON writer, to better serve its requirements.
> >> PATCH 1 move it to its proper place, and explains why it exists in a
> >> bit more detail.  PATCH 2 simplifies it a bit.
> >
> > Whose tree would this go in through?
> 
> Migration is the natural choice.  Would that inconvenience you?
> 
> > At any rate, series:
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > but see nit on 1/2
> 
> Perhaps the maintainer can add the commit hashes on merge.

I'm doing that.

Thanks,

		Amit

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-05-23  8:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-04 16:49 [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Markus Armbruster
2016-05-04 16:49 ` [Qemu-devel] [PATCH 1/2] migration: Move qjson.[ch] to migration/ Markus Armbruster
2016-05-04 17:11   ` Eric Blake
2016-05-06 13:07     ` Markus Armbruster
2016-05-05 13:52   ` Dr. David Alan Gilbert
2016-05-04 16:49 ` [Qemu-devel] [PATCH 2/2] migration/qjson: Drop gratuitous use of QOM Markus Armbruster
2016-05-04 17:08 ` [Qemu-devel] [PATCH 0/2] migration: Move qjson.[ch] to migration/, clean up Eric Blake
2016-05-06 13:11   ` Markus Armbruster
2016-05-23  8:36     ` Amit Shah

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).