Merge lp:~afrantzis/mir/client-api-platform-operation-spike into lp:mir

[Alexandros Frantzis]

client: First cut at API for platform operations

client: First cut at API for platform operations

This mostly an RFC spike, to get some feedback from the team. It's missing support for passing fds.

We use an opaque MirPlatformMessage type to pass and retrieve platform operation requests/replies. This type use a create/ref/unref scheme instead of the usual create/release (i.e. it's reference-counted) to maintain both memory safety in the callback and ease of use.

If the team doesn't like the reference counting scheme we could also use a create/copy/release scheme (possibly with copy-on-write semantics for efficiency).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2111
http://jenkins.qa.ubuntu.com/job/mir-ci/2283/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/379
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/379
    ABORTED: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/353/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/274
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/274/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/353
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/353/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2283/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> ABORTED: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/353/console

As it says, the mako build was aborted, but others passed.. Waiting for CI to get back into shape before resubmitting, but feel free to review in the meantime.

[Kevin DuBois (kdub)]

(needs fixing)
We should clarify the FD lifetimes in the C API via comments.

(my 2 cents)
MirPlatformPackage is somewhat clunky in that it is 66 ints, and the fds/ints are limited to 32. It also has a fd type that's just a plain int, making fd lifetimes less obvious. I'd rather keep that type just in the C api and have a richer/more c++ friendly type internally.

I'm in favor of improving MirBufferPackage to be something like:
struct MirBufferPackage
{
    int* data;
    int num_data;
    int* fd;
    int num_fd;
};
or it being opaque with the functions named in the description

[Alexandros Frantzis (afrantzis)]

> (needs fixing)
> We should clarify the FD lifetimes in the C API via comments.

I left out FD handling (and tests) on purpose for this MP, so we could focus on the overall form of the API instead. We will have a chance to discuss the FD lifetimes in an upcoming MP that will add support for exchanging FDs.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2111
http://jenkins.qa.ubuntu.com/job/mir-ci/2309/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/408
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/408
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/382/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/300
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/300/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/382
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/382/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3621/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16387

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2309/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> socket.gaierror: [Errno -2] Name or service not known

Sigh...

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2111
http://jenkins.qa.ubuntu.com/job/mir-ci/2311/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/411
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/411
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/385/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/302
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/302/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/385
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/385/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3623/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16394

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2311/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> MirPlatformMessage* mir_create_platform_message(unsigned int opcode);
> void mir_platform_message_set_data(MirPlatformMessage* message, int const* data, size_t ndata);
> void mir_platform_message_set_fds(MirPlatformMessage* message, int const* data, size_t nfds);

Couldn't the create function take the data and fds? Vis:

MirPlatformMessage* mir_create_platform_message(unsigned int opcode, int const* data, size_t ndata, int const* fds, size_t nfds);

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> Couldn't the create function take the data and fds? Vis:

It could. However, neither data nor fds are required for a platform message, so I proposed this approach following the "constructor takes required arguments" rationale we used for the MirSurfaceSpec. The trade off is more functions vs having larger constructor calls with possibly null arguments and the potential to create immutable objects (if we choose not to have any other setters).

I don't have any strong preference for one approach over the other.

[Alan Griffiths (alan-griffiths)]

> or it being opaque with the functions named in the description

I'd favor opaque - it makes it far easier to manage changes and (when we get to it) Fd ownership.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2111
http://jenkins.qa.ubuntu.com/job/mir-ci/2312/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/412
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/412
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/386
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/303
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/303/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/386
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/386/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3624
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16417

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2312/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

1. Since we are no longer bound to MirPlatformPackage, we could change our data array to hold bytes instead of ints. I am in favor of the byte array, since it makes it easier to pass arbitrary data. Any objections?

2. If the team doesn't like the reference counting scheme we could also use a create/copy/release scheme (possibly with copy-on-write semantics for efficiency, but copying is probably not a problem for the current use cases).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2113
http://jenkins.qa.ubuntu.com/job/mir-ci/2328/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/430
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/430
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/404
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/319
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/319/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/404
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/404/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3639
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16454

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2328/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> 2. If the team doesn't like the reference counting scheme we could also use a create/copy/release scheme (possibly with copy-on-write semantics for efficiency, but copying is probably not a problem for the current use cases).

The goal of this scheme (and the ref/unref scheme) is to make MirPlatformMessage an object that is useful to the client outside the strict context of the platform operation call and callback. If we don't need/want this then we could just make it a simple container that the user must extract/acquire data from as discussed in the call.

[Alan Griffiths (alan-griffiths)]

Without seeing plausible code that uses this I'm not convinced that the ref counting scheme is useful.

I see several "simpler" options:

1. never pass "ownership" to the client and require them to save anything they want. That could simply be:
    void* mir_platform_message_take_data(message, &size) and
    int* mir_platform_message_take_fds(message, &nfds)

...

    free(data);

    for (int* fd = fds; fd != fds+nfds; ++fd) close(*fd);
    free(fds);

2. always pass "ownership" and require mir_platform_message_release(message) to be called

3. Have the callback return a bool indicating whether ownership has been taken. (Which, at least, gives a compile error if no decision is taken.)

But I'm ready to be told that all the above are simplistic and ref counting is a requirement.

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> 1. Since we are no longer bound to MirPlatformPackage, we could change our data array to hold bytes
> instead of ints. I am in favor of the byte array, since it makes it easier to pass arbitrary data.

Done.

> 2. If the team doesn't like the reference counting scheme we could also use a create/copy/release scheme

I decided to abandon these schemes completely and I also stopped trying to make the message object smarter/safer than it can reasonably be in C. It is meant to be just an opaque struct of arrays. The new state of things is:

1. MirPlatformMessage is owned by the callee in the context of the callback. The callee is responsible for forwarding it, releasing it etc.

2. MirPlatformMessage is just a container of information. It owns the memory needed to hold the information but not the information itself. That means that when in upcoming MPs it will contain fds, the fds will not be closed when the message is released, much like an array of int fds doesn't release them. The user will need to get and manipulate/close the fds manually.

[Kevin DuBois (kdub)]

Looks good to me. I like this approach better than the other more-involved approaches in the C api.

review: Approve

[Alan Griffiths (alan-griffiths)]

src/server/frontend/session_mediator.cpp:40:53: fatal error: mir/graphics/platform_operation_message.h: No such file or directory
 #include "mir/graphics/platform_operation_message.h"

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> src/server/frontend/session_mediator.cpp:40:53: fatal error: mir/graphics /platform_operation_message.h: No such file or directory

Fixed.

[Alberto Aguirre (albaguirre)]

Why do we need this?

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> Why do we need this?

See https://lists.ubuntu.com/archives/mir-devel/2014-October/000923.html . In short (quoting from the email):

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).

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

So you'll be proposing drmauth, etc to use this plumbing in a subsequent MP, right?

  163 + * a call to mir_platform_message_unref() to avoid memory leaks.

Needs update.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> So you'll be proposing drmauth, etc to use this plumbing in a subsequent MP,
> right?

Yes.

>
> 163 + * a call to mir_platform_message_unref() to avoid memory leaks.
>
> Needs update.

Fixed.

[Cemil Azizoglu (cemil-azizoglu)]

LGTM.

review: Approve

[Daniel van Vugt (vanvugt)]

Landed and then reverted. This branch is making cross-compile fail to build.

review: Needs Fixing

[lp:~afrantzis/mir/client-api-platform-operation-spike]

2123. By Alexandros Frantzis on 2014-12-10

Sync with lp:mir

2124. By Alexandros Frantzis on 2014-12-10

Fix android tests build

[Alexandros Frantzis (afrantzis)]

A merge of this branch in trunk followed by a revert has confused bzr. Rejecting this branch and resubmitted this code as part of:

https://code.launchpad.net/~afrantzis/mir/client-api-platform-operation/+merge/244272

Unmerged revisions

2124. By Alexandros Frantzis on 2014-12-10

Fix android tests build

2123. By Alexandros Frantzis on 2014-12-10

Sync with lp:mir

=== modified file 'tests/unit-tests/graphics/android/test_ipc_operations.cpp'
--- tests/unit-tests/graphics/android/test_ipc_operations.cpp	2014-11-13 16:41:26 +0000
+++ tests/unit-tests/graphics/android/test_ipc_operations.cpp	2014-12-10 09:33:25 +0000
@@ -17,7 +17,7 @@
  */
 
 #include "src/platform/graphics/android/ipc_operations.h"
-#include "mir/graphics/platform_ipc_package.h"
+#include "mir/graphics/platform_operation_message.h"
 #include <gtest/gtest.h>
 
 namespace mg = mir::graphics;
@@ -32,19 +32,19 @@
 TEST_F(IpcOperations, test_ipc_data_packed_correctly_for_full_ipc)
 {
     //android has no valid operations platform specific operations yet, expect throw
-    mg::PlatformIPCPackage package{
-        std::vector<int32_t>{2,4,8,16,32},
+    mg::PlatformOperationMessage message{
+        std::vector<uint8_t>{2,4,8,16,32},
         std::vector<int32_t>{fileno(tmpfile()), fileno(tmpfile())}
     };
 
     EXPECT_THROW({
-        ipc_operations.platform_operation(0u, package);
+        ipc_operations.platform_operation(0u, message);
     }, std::invalid_argument);
 
     EXPECT_THROW({
-        ipc_operations.platform_operation(1u, package);
+        ipc_operations.platform_operation(1u, message);
     }, std::invalid_argument);
 
-    for (auto fd : package.ipc_fds)
+    for (auto fd : message.fds)
         close(fd); 
 }