[Spice-devel] [PATCH 0/4] Add context to SpiceCoreInterface

[Frediano Ziglio]

> 
> On 12/21/2015 12:33 PM, Frediano Ziglio wrote:
> > This patchset want to solve the problem of not having a context in
> > SpiceCoreInterface.  SpiceCoreInterface defines a set of callbacks to
> > handle events in spice-server.  These callbacks allow to handle timers,
> > watch for file descriptors and send channel events.  All these callbacks
> > does not accept a context (usually in C passed as a void* parameter) so
> > they is hard for them to differentiate the interface specified.
> > Unfortunately this structure is used even internally from different
> > contextes for instance every RedWorker thread have a different context. To
> > solve this issue some workarounds are used. Currently for timers a variable
> > depending on the current thread is used while for watches the opaque
> > parameter to pass to the event callback is used as it currently points just
> > to RedChannelClient structure.  This however impose some implicit
> > maintanance problem in the future. What happen for instance if for some
> > reason a timer is registered during worker initialization, run in another
> > thread? What if we decide to register a file descriptor callback for
> > something not a RedChannelClient?  Could be that the program will run
> > without any issue till some bytes changes and weird thing could happen.
> >
> > The implementation of this solution is done implementing an internal "core"
> > interface that have context specification and use it to differentiate the
> > context instead of relying to some other, hard to maintain, detail. Then an
> > adapter structure (name inpired to the adapter pattern) will provide the
> > internal core interface using the external, public, definition (in the
> > future this technique can be used to extend the external interface without
> > breaking the ABI).
> >
> > First patches are intended to be just mechanical changes to make easier to
> > understand the idea. The idea if this solution is accepted is to squash
> > them all in a single patch. For this reason patches comments are quite
> > small and will be replaced by top of this one. Note that not all callbacks
> > from the public SpiceCoreInterface have a context (a "const
> > SpiceCoreInterfaceInternal *" argument) but this can be easily extended.
> > Last patch is the main reason for these patches.
> 
> 
> Hi Frediano,
> 
> This patchset does associate between the internal core and the worker
> (only for display channels).
> However context was already being passed to core function via the
> opaque parameter. The internal context does not show up when the
> callback is called (as that would change the API), only "opaque"
> is available.
> 
> Thanks,
>      Uri.
> 

The association between internal core and worker is done by worker on
the internal core built by worker and is used by cursor and display channels.
In

  SpiceTimer *(*timer_add)(SpiceTimerFunc func, void *opaque);

(or similar callbacks) opaque is the context for func but I want the context
for timer_add callback which is not available.
I used in my patches SpiceCoreInterfaceInternal* for the context.
This change the internal API but not the public one so it's not an issue.

I don't understand where does your misinterpretation came. Are you
suggesting a different comment or a different implementation?
Perhaps are you suggesting to use another void* pointer for function context
instead of SpiceCoreInterfaceInternal*? I can do it, I don't know how big
would be the change.

Frediano