Reworking our support for platform specific functions

[Alan Griffiths]

On 13/10/14 12:37, Alexandros Frantzis wrote:
> Hi all,
>
> the need has arisen to add another platform-specific function, in order
> to resolve bug LP: #1379266. This addition triggered some concerns about
> the way we implement such functions.
>
> The problem
> -----------
>
> In our current implementation, adding a platform-specific function
> causes platform-specific details to leak into many layers from client to
> server. For example, for the existing drm_auth_magic() call we have:
>
> Client API: mir_connection_drm_auth_magic(int magic, ...)
> Client    : MirConnection::drm_auth_magic(...)
> Protobuf  : rpc drm_auth_magic(DRMMagic) returns (DRMAuthMagicStatus)
> Server    : SessionMediator::drm_auth_magic(), mg::DRMAuthenticator
>
> This approach has a few drawbacks:
>
> 1. Platform-specific details leak in code that should be platform
>    agnostic.
> 2. It can't support platform-specific functions for 3rd party platforms.
> 3. It's burdensome to add a new platform-specific functions (I guess
>    this is a problem for our API functions in general).
>
> Proposed solution
> -----------------
>
> To solve the issues mentioned above we need to provide a way to make an
> opaque (as far as the rest of Mir is concerned) request to the platform:
>
> Client API: mir_connection_platform_operation(type, DataAndFds request, ...);
> Client    : MirConnection::platform_operation(type, ...)
> Protobuf  : rpc platform_operation(type, DataAndFds) returns (DataAndFds)
> Server    : SessionMediator::platform_operation()
> Platform  : DataAndFds PlatformIpcOperations::platform_operation(type, DataAndFds)
>
> Each platform should expose a header with its operation types and
> expected parameters types. For mesa it could be something like:
>
> enum MesaPlatformOperationType { drm_auth_magic, drm_get_auth_fd };
> struct DRMAuthMagicRequest { int magic; }
> struct DRMAuthMagicReply { int status; }
> ...
>
> And a request would be:
>
> DRMAuthMagicRequest request = { magic };
>
> void platform_operation_callback(
>     MirConnection* connection, DataAndFds const* reply, void* context)
> {
>     struct DRMAuthMagicReply const* magic_reply = reply->data;
>     ...
> }
>
> mir_connection_platform_operation(
>     drm_auth_magic, request, platform_operation_callback);
>
> With this approach:
>
> 1. No platform-specific details leak into unrelated layers. Only the
>    platform knows about these details and exposes them to other interested
>    parties.
>
> 2+3. It's trivial for 3rd party platforms to expose platform-specific operations
>
> This proposal involves changes and ABI bumps to a few layers, so I would
> appreciate some early feedback and discussion.
>
> Thoughts?

I like the general approach, but have concerns about a bit of the detail.

1. Is there any point to having separating "type" from "data" and "fds"?
Or should the various data structures combine all three.
2. In the client API would be good to use an opaque "PlatformMessage"
type, rather than a DataAndFds struct.

E.g.

   DRMAuthMagicReply const* magic_reply = (DRMAuthMagicReply const*)mir_platform_get_data(reply);

3. With a C API we have to be explicit about ownership. E.g. does the client have to release 
magic_reply or does it become invalid after the call?
4. Do we need to address byte encoding, sizes and alignment of platform data?