[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces |
Date: |
Wed, 7 Oct 2015 08:57:31 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 10/07/2015 06:00 AM, Markus Armbruster wrote:
>>> Looks like we're getting drawn into visitor contract territory again.
>>>
>> +++ b/hmp.c
>> @@ -1658,8 +1658,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
>>
>> object_add(type, id, pdict, opts_get_visitor(ov), &err);
>>
>> + visit_check_struct(opts_get_visitor(ov), &err_end);
>> out_end:
>> - visit_end_struct(opts_get_visitor(ov), &err_end);
>> + visit_end_struct(opts_get_visitor(ov));
>> if (!err && err_end) {
>> qmp_object_del(id, NULL);
>> }
>
> Preexisting: calling object_add() before visit_end_struct() is awkward.
> Can we simplify things now we have separate visit_check_struct() and
> visit_end_struct()? Call visit_check_struct() before object_add(),
> bypass object_add() on error, avoiding the need to undo it with
> qmp_object_del().
Okay, it sounds like I'm sitting on a pile of pre-patch cleanups, and
that I'm on the right track for having done the split.
>> +++ b/include/qapi/visitor.h
>> @@ -56,11 +56,19 @@ typedef struct GenericList
>> void visit_start_struct(Visitor *v, void **obj, const char *kind,
>> const char *name, size_t size, Error **errp);
>> /**
>> + * Check whether completing a struct is safe.
>
> "Safe"? We need to complete the struct visit with visit_end_struct()
> regardless of what this function returns...
>
>> + * Should be called prior to visit_end_struct() if all other intermediate
>> + * visit steps were successful, to allow the caller one last chance to
>> + * report errors such as remaining data that was not consumed by the visit.
>> + */
>> +void visit_check_struct(Visitor *v, Error **errp);
Maybe:
Declare the current struct complete, and check for unvisited contents.
>> static void
>> -opts_end_struct(Visitor *v, Error **errp)
>> +opts_check_struct(Visitor *v, Error **errp)
>> {
>> OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> GQueue *any;
>
> if (--ov->depth > 0) {
>
> Do we want to decrement ov->depth here? We'll decrement it again in
> opts_end_struct()...
Oh, good catch. This was an awkward split, and I got it off-by-one.
>> +++ b/qapi/qmp-input-visitor.c
>> @@ -88,14 +88,14 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject
>> *obj, Error **errp)
>> qiv->nb_stack++;
>> }
>>
>> -/** Only for qmp_input_pop. */
>> +/** Only for qmp_input_check. */
>
> Drop the comment instead?
>
> Aside: a loop would be more idiomatic C. Leave higher order functions
> to languages that are actually equipped for the job.
>
>> static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
>> {
>> *(const char **)user_pkey = (const char *)key;
>> return TRUE;
>> }
>>
>> -static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>> +static void qmp_input_check(QmpInputVisitor *qiv, Error **errp)
>> {
>> assert(qiv->nb_stack > 0);
>>
>> @@ -107,6 +107,17 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error
>> **errp)
>> g_hash_table_find(top_ht, always_true, &key);
always_true() exists for g_hash_table_find() - unless you know of some
other way to grab any arbitrary element of the hash table that doesn't
require a higher-order function.
>> +++ b/scripts/qapi-event.py
>> @@ -73,13 +73,14 @@ def gen_event_send(name, arg_type):
>> ret += gen_err_check()
>> ret += gen_visit_fields(arg_type.members, need_cast=True)
>> ret += mcgen('''
>> - visit_end_struct(v, &err);
>> + visit_check_struct(v, &err);
>> if (err) {
>> goto out;
>> }
>> + visit_end_struct(v);
>>
>> obj = qmp_output_get_qobject(qov);
>> - g_assert(obj != NULL);
>> + g_assert(obj);
>
> I prefer the more laconic form myself, but it's an unrelated change.
I can split that one-line change into a more appropriate patch.
>> out_obj:
>> error_propagate(errp, err);
>> err = NULL;
>> visit_end_union(v, !!(*obj)->data, &err);
>
> Should visit_end_union() be similarly split? Or should its Error **
> parameter be dropped? As far as I can tell, no visitor implements this
> method...
>
visit_end_union() gets completely dropped in a different patch. See v5
28/46.
>> +++ b/tests/test-qmp-output-visitor.c
>> @@ -194,10 +194,11 @@ static void visit_type_TestStruct(Visitor *v,
>> TestStruct **obj,
>> }
>> visit_type_str(v, &(*obj)->string, "string", &err);
>>
>> + if (!err) {
>> + visit_check_struct(v, &err);
>> + }
>
> ... but here, you guard it with an if. Either way works, but I'd like
> us to pick just one for the generators.
Sure.
>> - for (*head = i = visit_next_list(v, head, errp); i; i =
>> visit_next_list(v, &i, errp)) {
>> + for (*head = i = visit_next_list(v, head, &err);
>> + !err && i;
>> + i = visit_next_list(v, &i, &err)) {
>> TestStructList *native_i = (TestStructList *)i;
>> - visit_type_TestStruct(v, &native_i->value, NULL, errp);
>> + visit_type_TestStruct(v, &native_i->value, NULL, &err);
>> }
>
> Is this a silent bug fix? Before your patch, the loop doesn't break on
> error.
>
Yes, looks like it. And all the more reason for our test code to NOT
hand-write this, but to rely on the generator (so that we are testing a
single version of visit_* calls, rather than subtle differences in
generated vs. hand-rolled).
>> +++ b/vl.c
>> @@ -2796,23 +2796,25 @@ static int object_create(void *opaque, QemuOpts
>> *opts, Error **errp)
>> qdict_del(pdict, "qom-type");
>> visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
>> if (err) {
>> - goto out;
>> + goto obj_out;
>> +obj_out:
>> + visit_end_struct(opts_get_visitor(ov));
>> if (err) {
>> qmp_object_del(id, NULL);
>> }
>
> Silent bug fix: we now call visit_end_struct() even on error. Impact?
> Separate patch?
Yes, separate patch, and I'll evaluate impact there.
So sounds like I should proceed with this RFC (which means more respin
of my other patches, before I can post subset C - but that's okay, since
we aren't even through reviewing subset B, nor is subset A in a pull
request).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature