Re: [PATCH 09/10] spice: Put spice functions in a separate load module

[Christophe de Dinechin]

On 2020-06-30 at 01:00 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>> > If so the more normal approach would be to have a struct defining
>> > a set of callbacks, that can be registered. Or if there's a natural
>> > fit with QOM, then a QOM interface that can then have a QOM object
>> > impl registered as a singleton.
>>
>> That was my second attempt (after the weak symbols). I cleaned it up a bit
>> and put it here: https://github.com/c3d/qemu/commits/spice-vtable.
>
> I think this is the direction we should take.
>
>> What made me switch to the approach in this series is the following
>> considerations:
>>
>> - A vtable is useful if there can be multiple values for a method, e.g. to
>>   implement inheritance, or if you have multiple instances. This is not the
>>   case here.
>
> Well, we'll have two.  The normal functions.  And the stubs.
>
> The stubs are inline functions right now, in include/ui/qemu-spice.h, in
> the !CONFIG_SPICE section.  We can turn them into normal functions, move
> them to some C file.  Let QemuSpiceOpts function pointers point to the
> stubs initially.  When spice initializes (no matter whenever modular or
> not) it'll set QemuSpiceOpts to the normal implementation.

Good idea. I'll do that in the next round.

>
> That way we'll also remove some spice #ifdefs as part of the spice
> modularization effort.
>
> Things like the "using_spice" variable which don't depend on the spice
> shared libraries can also be moved to the new C file with the spice
> stubs.

OK.

>
> I don't think we need to hide QemuSpiceOpts with inline functions like
> qemu_spice_migrate_info().  I would simply use ...
>
>       struct QemuSpiceOps {
>               [ ... ]
>               int (*migrate_info)(...);
>               [ ... ]
>       } qemu_spice;
>
> ... then change the ...
>
>       qemu_spice_migrate_info(...)
>
> .. callsites into ...
>
>       qemu_spice.migrate_info(...)
>

OK.

>> - Overloading QOM for that purpose looked more confusing than anything else.
>>   It looked like I was mixing unrelated concepts. Maybe that's just me.
>
> Hmm?  Not sure what you mean.  There is no need for QOM here (and I
> can't see anything like that in your spice-vtable branch either).

I was responding to Daniels's earlier comment:

> Or if there's a natural fit with QOM, then a QOM interface that can then
> have a QOM object impl registered as a singleton.

>
>> - The required change with a vtable ends up being more extensive. Instead of
>>   changing a single line to put an entry point in a DSO, you need to create
>>   the vtable, add functions to it, add a register function, etc. I was
>>   looking for an easier and more scalable way.
>
> IMHO it isn't too much overhead, and I find the code is more readable
> that way.

There is certainly very little overhead in that case, since none of the
functions is performance critical.

>
>> - In particular, with a vtable, you cannot take advantage of the syntactic
>>   trick I used here, which is that foo(x) is a shortcut for (*foo)(x).
>>   So for a vtable, you need to manually write wrappers.
>
> See above, I don't think we need wrappers.

Well, so far that's two for two for the vtable approach. So unless someone
else agrees with my arguments for pointer patching, that will be my next
iteration of that series :-)

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)