[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces |
Date: |
Wed, 07 Oct 2015 17:23:20 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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.
I think so, too.
>>> +++ 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.
That's one purpose of implementing the method wrapped by this function.
Perhaps simply: Prepare completing a struct visit.
>>> 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.
g_hash_table_iter_init() and g_hash_table_iter_next().
>>> +++ 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).
Yes, please.
Subset A is ready, and will be in my next QAPI pull request.