Message ID | 566AAF3A.8090902@redhat.com |
---|---|
State | New |
Headers | show |
On 12/11/2015 04:10 AM, Paolo Bonzini wrote: > > > On 11/12/2015 00:53, Eric Blake wrote: >> Instead of rolling our own limited JSON outputter, we can just >> wrap the more full-featured JSON output Visitor. >> >> This slightly changes the output (different spacing), but the >> result is still equivalent JSON contents. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> qjson.c | 61 ++++++++++++++++++++++--------------------------------------- >> 1 file changed, 22 insertions(+), 39 deletions(-) > > I didn't find out which tree your patches apply to, but this: Mentioned in the cover letter: + v8 or later of my qapi subset E patches (at the time of this mail, only v7 has been posted): https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01181.html Also available as a tag at this location (with prerequisites applied): git fetch git://repo.or.cz/qemu/ericb.git qapi-jsonv1 (note that it is only a tag, not a branch, and you are right that it doesn't yet apply to any maintainer's tree). > @@ -1068,9 +1070,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only) > return; > } > > - vmdesc = qjson_new(); > - json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE); > - json_start_array(vmdesc, "devices"); > + vmdesc_jov = json_output_visitor_new(); > + vmdesc = json_output_get_visitor(vmdesc_jov); > + visit_start_struct(vmdesc, NULL, 0, NULL, &error_abort); > + visit_type_int(vmdesc, TARGET_PAGE_SIZE, "page_size", &error_abort); > + visit_start_list(vmdesc, NULL, 0, "devices", &error_abort); > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { Nice idea. > compiles and is pretty much a 1:1 translation from the qjson.c API to the > visitor API (using this patch as a guide). Feel free to include it and > remove qjson.c. Alternatively, you can leave out this patch and I'll test > and submit the transition. Should I squash the two together, or keep my current patch and drop qjson.c as a separate patch? Also, while reading this, I noticed that qjson.h has signatures that match JSON ('name' comes before 'value', when generating a "name":value pair), while visitor.h has signatures that are backwards ('obj' comes before 'name', even though the output will be "name":value-of-obj). I'm seriously debating about a tree-wide coccinelle patch to reverse the order or arguments of all the visit_type_* functions.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 11/12/2015 14:42, Eric Blake wrote: >>> compiles and is pretty much a 1:1 translation from the qjson.c >>> API to the visitor API (using this patch as a guide). Feel >>> free to include it and remove qjson.c. Alternatively, you can >>> leave out this patch and I'll test and submit the transition. > Should I squash the two together, or keep my current patch and > drop qjson.c as a separate patch? As you prefer. > Also, while reading this, I noticed that qjson.h has signatures > that match JSON ('name' comes before 'value', when generating a > "name":value pair), while visitor.h has signatures that are > backwards ('obj' comes before 'name', even though the output will > be "name":value-of-obj). I'm seriously debating about a tree-wide > coccinelle patch to reverse the order or arguments of all the > visit_type_* functions. That would be doable, there aren't many users outside generated code. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWatN5AAoJEL/70l94x66D8PQIAKEzIEUqjfhDMLhov4NwxGs3 LvyW+/w5AyDEZOJMayBuc3y7fSTbLp/eMQPsfBOIvBCMr/aEE4Tij0CzW3JE4FnH IFCW94F8Q7vq8jTYKFU6222Nk7vUGOLG9y3jQSnLX4RzpRizhWjk73DtkED7YVu3 5J0Ef3+ZEobCZA9zOwQT7+pqc9ah/M6tGsZodvML2FopCk/SsTBauPjoSeP0knHE 1MJsu/KVjn1508yn1SY9d0HJKQx2dM3nzQRtp0xbWx7x8o7u826axpVQieq7YBpb PaRW/vqiXvdnRkrCRHZAL6PJ4GLyquX42E7HjqCQjibA06fbtqmLoYQA2A/2Pyg= =EJIn -----END PGP SIGNATURE-----
On 12/11/2015 04:10 AM, Paolo Bonzini wrote: > > > On 11/12/2015 00:53, Eric Blake wrote: >> Instead of rolling our own limited JSON outputter, we can just >> wrap the more full-featured JSON output Visitor. >> >> This slightly changes the output (different spacing), but the >> result is still equivalent JSON contents. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> qjson.c | 61 ++++++++++++++++++++++--------------------------------------- >> 1 file changed, 22 insertions(+), 39 deletions(-) > > I didn't find out which tree your patches apply to, but this: > > if (vmdesc) { > - json_prop_int(vmdesc, "size", size); > - json_start_array(vmdesc, "fields"); > - json_start_object(vmdesc, NULL); > - json_prop_str(vmdesc, "name", "data"); > - json_prop_int(vmdesc, "size", size); > - json_prop_str(vmdesc, "type", "buffer"); > - json_end_object(vmdesc); > - json_end_array(vmdesc); > + visit_type_int(vmdesc, size, "size", &error_abort); visit_type_int() takes 'int64_t*', not 'int'. In places, that means I'd have to add a temporary variable to cover the fact that the input is not the right type for writing '&size' > @@ -1068,9 +1070,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only) > return; > } > > - vmdesc = qjson_new(); > - json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE); > - json_start_array(vmdesc, "devices"); > + vmdesc_jov = json_output_visitor_new(); > + vmdesc = json_output_get_visitor(vmdesc_jov); > + visit_start_struct(vmdesc, NULL, 0, NULL, &error_abort); > + visit_type_int(vmdesc, TARGET_PAGE_SIZE, "page_size", &error_abort); ...or even cover the case where it is a constant whose address does not even exist. > > compiles and is pretty much a 1:1 translation from the qjson.c API to the > visitor API (using this patch as a guide). Feel free to include it and > remove qjson.c. Alternatively, you can leave out this patch and I'll test > and submit the transition. So the patch to avoid qjson.c is not quite 1:1. Still, I'll go ahead and complete the patch for my v2, to see if you still like it.
diff --git a/migration/savevm.c b/migration/savevm.c index 0ad1b93..26986d8 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -646,7 +646,7 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) return vmstate_load_state(f, se->vmsd, se->opaque, version_id); } -static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc) +static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, Visitor *vmdesc) { int64_t old_offset, size; @@ -655,14 +655,14 @@ static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON *vmdes size = qemu_ftell_fast(f) - old_offset; if (vmdesc) { - json_prop_int(vmdesc, "size", size); - json_start_array(vmdesc, "fields"); - json_start_object(vmdesc, NULL); - json_prop_str(vmdesc, "name", "data"); - json_prop_int(vmdesc, "size", size); - json_prop_str(vmdesc, "type", "buffer"); - json_end_object(vmdesc); - json_end_array(vmdesc); + visit_type_int(vmdesc, size, "size", &error_abort); + visit_start_list(vmdesc, NULL, 0, "fields", &error_abort); + visit_start_struct(vmdesc, NULL, 0, NULL, &error_abort); + visit_type_str(vmdesc, (char **)&"data", "name", &error_abort); + visit_type_int(vmdesc, size, "size", &error_abort); + visit_type_str(vmdesc, (char **)&"buffer", "type", &error_abort); + visit_end_struct(vmdesc); + visit_end_list(vmdesc); } } @@ -1028,7 +1028,9 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f) void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only) { - QJSON *vmdesc; + JsonOutputVisitor *vmdesc_jov; + Visitor *vmdesc; + char *vmdesc_str; int vmdesc_len; SaveStateEntry *se; int ret; @@ -1068,9 +1070,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only) return; } - vmdesc = qjson_new(); - json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE); - json_start_array(vmdesc, "devices"); + vmdesc_jov = json_output_visitor_new(); + vmdesc = json_output_get_visitor(vmdesc_jov); + visit_start_struct(vmdesc, NULL, 0, NULL, &error_abort); + visit_type_int(vmdesc, TARGET_PAGE_SIZE, "page_size", &error_abort); + visit_start_list(vmdesc, NULL, 0, "devices", &error_abort); QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if ((!se->ops || !se->ops->save_state) && !se->vmsd) { @@ -1083,15 +1087,15 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only) trace_savevm_section_start(se->idstr, se->section_id); - json_start_object(vmdesc, NULL); - json_prop_str(vmdesc, "name", se->idstr); - json_prop_int(vmdesc, "instance_id", se->instance_id); + visit_start_struct(vmdesc, NULL, 0, NULL, &error_abort); + visit_type_str(vmdesc, (char **)&se->idstr, "name", &error_abort); + visit_type_int(vmdesc, se->instance_id, "instance_id", &error_abort); save_section_header(f, se, QEMU_VM_SECTION_FULL); vmstate_save(f, se, vmdesc); - json_end_object(vmdesc); + visit_end_struct(vmdesc); trace_savevm_section_end(se->idstr, se->section_id, 0); save_section_footer(f, se); } @@ -1101,16 +1105,18 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only) qemu_put_byte(f, QEMU_VM_EOF); } - json_end_array(vmdesc); - qjson_finish(vmdesc); - vmdesc_len = strlen(qjson_get_str(vmdesc)); + visit_end_list(vmdesc); + visit_end_struct(vmdesc); + vmdesc_str = json_output_get_string(vmdesc_jov); + vmdesc_len = strlen(vmdesc_str); if (should_send_vmdesc()) { qemu_put_byte(f, QEMU_VM_VMDESCRIPTION); qemu_put_be32(f, vmdesc_len); - qemu_put_buffer(f, (uint8_t *)qjson_get_str(vmdesc), vmdesc_len); + qemu_put_buffer(f, (uint8_t *)vmdesc_str, vmdesc_len); } - object_unref(OBJECT(vmdesc)); + g_free(vmdesc_str); + json_output_visitor_cleanup(vmdesc_jov); qemu_fflush(f); }