Reworking our support for platform specific functions

[Daniel van Vugt]

Liking the idea of a generalised approach... Although on first glance 
this does not sound like a real function name:
    mir_connection_platform_operation()
I know we want to be generic, but that sounds somehow overly generic 
with "platform operation". Maybe we need a better function name there. 
Not "ioctl" :)

Also, to generalise you would probably not use a parameter named 
"DataAndFds" as "Fds" is too specific. The structure would need to 
contain a size and pointer (void*) field, although they may as well be 
parameters instead of a struct. And Protobuf could use opaque type "bytes".

Finally instead of MesaPlatformOperationType, you could just make "type" 
a UUID string. That has a few advantages:
   1. Adding new (private-ish) operations does not require public header 
changes.
   2. An operation can be made portable and understood by multiple 
platforms if need be.
   3. Operation types can be removed and retired without lingering or 
leaving a hole in any enum definition.

Then again, enum/ints are beautifully simple.


On 13/10/14 19: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?
>
> Thanks,
> Alexandros
>