Re: [Qemu-devel] [PATCH v3 10/18] vmstate: Use new JSON output visitor

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> "Dr. David Alan Gilbert" <address@hidden> writes:
>> 
>> > * Markus Armbruster (address@hidden) wrote:
>> >> "Dr. David Alan Gilbert" <address@hidden> writes:
>> >
>> >> "git-grep assert migration" suggests you do kill the source on certain
>> >> programming errors.
>> >
>> > I'm just trying hard to reduce them; I know I'm not there, but I'd rather
>> > we didn't have any - especially on the source side.
>> >
>> >> I reiterate my point that fancy, untestable error recovery is unlikely
>> >> to actually recover.  "Fancy" can work, "untestable" might work (but
>> >> color me skeptic), but once you got both, you're a dead man walking.
>> >
>> > Then we should make the error recovery paths easy; at the moment visitor
>> > error paths are just too painful.
>> 
>> I've never seen error handling in C that wasn't painful and still
>> correct.  Surprise me!
>
> The thing that makes it hard for the visitor code is the need to check
> it after every call and the check is complicated.

Having to check every call is certainly painful, but there's no general
and safe way around it.  Accumulating errors that need to be checked
only at the end of a job can be less painful, but then the job's code
needs to be very carefully written to be safe even in presence of
errors.  Most code isn't, and some code can't.

The check for failure is simple, but annoyingly verbose when the
function's return value is useless:

    Error *err = NULL;
    foo(..., &err);
    if (err) {
        ...
    }

I'm playing with a update to conventions and usage to permit

    if (!foo(..., &err)) {
        ...
    }

Just as simple, but more readable.

[...]
>> I figure we're unlikely to reach consensus on this, so I'd like to
>> propose we agree to disagree, and do the following:
>> 
>> * We shelve the de-duplication of JSON formatting (this patch)
>>   indefinitely.
>> 
>> * We move qjson.c to migration/, next to its only user, and add a
>>   comment explaining why it migration doesn't want to use general
>>   infrastructure here (JSON output visitor), but needs its own thing.
>>   This gets the file covered in MAINTAINERS, and will help prevent it
>>   growing additional users.
>> 
>> Deal?
>
> No, sorry; the JSON use in the migration is just a debug thing;
> we don't want to maintain a separate JSON instance for it.

Well, you already do, except in name.  Who else do you think is
maintaining qjson.[ch], created by migration people, for migration's
use?  Certainly not me.

If you can't use the general JSON output code I maintain because of
special requirements, you get to continue maintaining your own.  All 109
SLOC of it.  All I'm asking is to make it official, and to deter
accidental use of migration's JSON writer instead of the general one.