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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 05/02/2016 07:26 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Rather than using a QJSON object and converting the QString result
>>> to a char *, we can use the new JSON output visitor and get directly
>>> to a char *.
>>>
>>> The conversions are a bit tricky in place (in places, we have to
>>> copy an integer to an int64_t temporary to get the right pointer for
>>> visit_type_int(); and for several strings, we have to copy to a
>>> temporary variable so we can take an address (&char[] is not the
>>> same as &char*) and cast away const), but overall still fairly
>>> mechanical.
>>>
>>> Suggested-by: Paolo Bonzini <address@hidden>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>
>>> -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;
>>> +    const char *tmp;
>>>
>>>      old_offset = qemu_ftell_fast(f);
>>>      se->ops->save_state(f, se->opaque);
>>>      size = qemu_ftell_fast(f) - old_offset;
>>>
>>>      if (vmdesc) {
>> 
>> Conditionals could be avoided: use a null visitor.  Not sure it's worth
>> it, though.
>
> We could just teach qapi-visit-core.c to be a no-op for v==NULL (thus
> hiding the conditionals in the core code, but that then slows down the
> common case for more conditionals on every caller.  Maybe a null visitor
> is reasonable, after all?

I'd prefer a null visitor to messing up the core with conditionals.

Again: not sure avoiding the conditionals here is worth a null visitor.
If you want to find out, cook up a patch and we'll see.

>>> +        tmp = "data";
>>> +        visit_type_str(vmdesc, "name", (char **)&tmp, &error_abort);
>> 
>> The Visitor interface is the same for input and for output.  Convenient
>> when the code is direction-agnostic.  Inconvenient when it's output: you
>> have to pass the value by reference even though it's only read.  In
>> particular, literals need a temporary, and types have to be adjusted via
>> cast or temporary more frequently than for by-value.
>> 
>> If that bothers us, we can add by-value wrappers to the interface.
>> 
>> Are there other output-only visitor uses?
>
> qom-get is output-only, just as qom-set is input-only.  Maybe it's worth
> an experiment to see how difficult it would be.
>
>
>> Well, it doesn't exactly make this code prettier, but having a stupid
>> wrapper just to hide the ugliness isn't so hot, either.
>
> And now you see why I posted two alternatives, to see which way we want
> to go.  Having convenient wrappers for output-only visits may swing the
> vote.

Having read your first alternative, my immediate reaction was "why don't
you do X instead?", where X turned out to be your second alternative.  I
really don't like this feature-limited wrapper that is used in just one
place.  I also don't like its confusing git-log, courtesy of its
unwisely chosen filename.

But having read the second alternative, I understand why you're offering
the first one: both are ugly.

So which of the uglies do I prefer?  The experiment could indeed swing
my vote.