Eric Blake <ebl...@redhat.com> writes:
> qobject_from_jsonv() was unusual in that it took a va_list*, instead
> of the more typical va_list; this was so that callers could pass
> NULL to avoid % interpolation. While this works under the hood, it
> is awkward for callers, so move the magic into qjson.c rather than
> in the public interface, and finally improve the documentation of
> qobject_from_jsonf().
>
> test-qobject-input-visitor.c's visitor_input_test_init_internal()
> was the only caller passing NULL, fix it to instead use a QObject
> created by the various callers, who now use the appropriate form
> of qobject_from_json*() according to whether % interpolation is
> desired.
>
> Once that is done, all remaining callers to qobject_from_jsonv() are
> passing &error_abort; drop this parameter to match the counterpart
> qobject_from_jsonf() which assert()s success instead. Besides, all
> current callers that need interpolation live in the testsuite, where
> enforcing well-defined input by asserts can help catch typos, and
> where we should not be operating on dynamic untrusted arbitrary
> input in the first place.
>
> Asserting success has another nice benefit: if we pass more than one
> %p, but could return failure, we would have to worry about whether
> all arguments in the va_list had consistent refcount treatment (it
> should be an all-or-none decision on whether each QObject in the
> va_list had its reference count altered - but whichever way we
> prefer, it's a lot of overhead to ensure we do it for everything
> in the va_list even if we failed halfway through). But now that we
> promise success, we can now easily promise that all %p arguments will
> now be cleaned up when freeing the returned object.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> include/qapi/qmp/qjson.h | 2 +-
> tests/libqtest.c | 10 ++------
> qobject/qjson.c | 49
> +++++++++++++++++++++++++++++++++++---
> tests/test-qobject-input-visitor.c | 18 ++++++++------
> 4 files changed, 60 insertions(+), 19 deletions(-)
>
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index 6e84082d5f..9aacb1ccf6 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -19,7 +19,7 @@
>
> QObject *qobject_from_json(const char *string, Error **errp);
> QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
> -QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> +QObject *qobject_from_jsonv(const char *string, va_list ap)
> GCC_FMT_ATTR(1, 0);
>
> QString *qobject_to_json(const QObject *obj);
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 99a07c246f..cde737ec5a 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -448,7 +448,6 @@ QDict *qtest_qmp_receive(QTestState *s)
> */
> void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
> {
> - va_list ap_copy;
> QObject *qobj;
> int log = getenv("QTEST_LOG") != NULL;
> QString *qstr;
> @@ -463,13 +462,8 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
> }
> assert(*fmt);
>
> - /* Going through qobject ensures we escape strings properly.
> - * This seemingly unnecessary copy is required in case va_list
> - * is an array type.
> - */
> - va_copy(ap_copy, ap);
> - qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
> - va_end(ap_copy);
> + /* Going through qobject ensures we escape strings properly. */
> + qobj = qobject_from_jsonv(fmt, ap);
> qstr = qobject_to_json(qobj);
>
> /*
Wait! Oh, the va_copy() moves iinto qobject_from_jsonv(). Okay, I
guess.
> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 2e0930884e..210c290aa9 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -35,7 +35,8 @@ static void parse_json(JSONMessageParser *parser, GQueue
> *tokens)
> s->result = json_parser_parse_err(tokens, s->ap, &s->err);
> }
>
> -QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> +static QObject *qobject_from_json_internal(const char *string, va_list *ap,
> + Error **errp)
> {
> JSONParsingState state = {};
>
> @@ -50,12 +51,31 @@ QObject *qobject_from_jsonv(const char *string, va_list
> *ap, Error **errp)
> return state.result;
> }
>
> +/*
> + * Parses JSON input without interpolation.
Imperative mood, please. Same elsewhere.
Suggest "without interpolation of % sequences".
> + *
> + * Returns a QObject matching the input on success, or NULL with
> + * an error set if the input is not valid JSON.
> + */
> QObject *qobject_from_json(const char *string, Error **errp)
> {
> - return qobject_from_jsonv(string, NULL, errp);
> + return qobject_from_json_internal(string, NULL, errp);
> }
>
> /*
> + * Parses JSON input with interpolation of % sequences.
> + *
> + * The set of sequences recognized is compatible with gcc's -Wformat
> + * warnings, although not all printf sequences are valid. All use of
> + * % must occur outside JSON strings.
> + *
> + * %i - treat corresponding integer value as JSON bool
> + * %[l[l]]d, %PRId64 - treat sized integer value as signed JSON number
> + * %[l[l]]u, %PRIu64 - treat sized integer value as unsigned JSON number
> + * %f - treat double as JSON number (undefined for inf, NaN)
> + * %s - convert char * into JSON string (adds escapes, outer quotes)
> + * %p - embed QObject, transferring the reference to the returned object
> + *
> * IMPORTANT: This function aborts on error, thus it must not
> * be used with untrusted arguments.
> */
Use the opportunity to stop shouting IMPORTANT?
> @@ -65,13 +85,36 @@ QObject *qobject_from_jsonf(const char *string, ...)
> va_list ap;
>
> va_start(ap, string);
> - obj = qobject_from_jsonv(string, &ap, &error_abort);
> + obj = qobject_from_json_internal(string, &ap, &error_abort);
> va_end(ap);
>
> assert(obj != NULL);
> return obj;
> }
>
> +/*
> + * va_list form of qobject_from_jsonf().
> + *
> + * IMPORTANT: This function aborts on error, thus it must not
> + * be used with untrusted arguments.
> + */
> +QObject *qobject_from_jsonv(const char *string, va_list ap)
> +{
> + QObject *obj;
> + va_list ap_copy;
> +
> + /*
> + * This seemingly unnecessary copy is required in case va_list
> + * is an array type.
> + */
--verbose?
> + va_copy(ap_copy, ap);
> + obj = qobject_from_json_internal(string, &ap_copy, &error_abort);
> + va_end(ap_copy);
> +
> + assert(obj != NULL);
> + return obj;
> +}
> +
> typedef struct ToJsonIterState
> {
> int indent;
> diff --git a/tests/test-qobject-input-visitor.c
> b/tests/test-qobject-input-visitor.c
> index bcf02617dc..a9addd9f98 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -45,13 +45,11 @@ static void visitor_input_teardown(TestInputVisitorData
> *data,
> function so that the JSON string used by the tests are kept in the test
> functions (and not in main()). */
> static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
> - bool keyval,
> - const char *json_string,
> - va_list *ap)
> + bool keyval, QObject *obj)
> {
> visitor_input_teardown(data, NULL);
>
> - data->obj = qobject_from_jsonv(json_string, ap, &error_abort);
> + data->obj = obj;
> g_assert(data->obj);
>
> if (keyval) {
> @@ -69,10 +67,12 @@ Visitor
> *visitor_input_test_init_full(TestInputVisitorData *data,
> const char *json_string, ...)
> {
> Visitor *v;
> + QObject *obj;
> va_list ap;
>
> va_start(ap, json_string);
> - v = visitor_input_test_init_internal(data, keyval, json_string, &ap);
> + obj = qobject_from_jsonv(json_string, ap);
> + v = visitor_input_test_init_internal(data, keyval, obj);
> va_end(ap);
> return v;
> }
> @@ -82,10 +82,12 @@ Visitor *visitor_input_test_init(TestInputVisitorData
> *data,
> const char *json_string, ...)
> {
> Visitor *v;
> + QObject *obj;
> va_list ap;
>
> va_start(ap, json_string);
> - v = visitor_input_test_init_internal(data, false, json_string, &ap);
> + obj = qobject_from_jsonv(json_string, ap);
> + v = visitor_input_test_init_internal(data, false, obj);
> va_end(ap);
> return v;
> }
> @@ -100,7 +102,9 @@ Visitor *visitor_input_test_init(TestInputVisitorData
> *data,
> static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
> const char *json_string)
> {
> - return visitor_input_test_init_internal(data, false, json_string, NULL);
> + QObject *obj = qobject_from_json(json_string, &error_abort);
> +
> + return visitor_input_test_init_internal(data, false, obj);
> }
>
> static void test_visitor_in_int(TestInputVisitorData *data,
