Eric Blake <ebl...@redhat.com> writes:
> On 05/02/2016 07:26 AM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> 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 <pbonz...@redhat.com>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>>
>
>>> -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.
