Merge lp:~robertcarr/mir/1233944-addendum into lp:mir

[Robert Carr]

Fix for LP#1233944

Fixes the Mir-side of bug https://bugs.launchpad.net/mir/+bug/1233944

Event files are first created with root:root permissions and only later
udev rules are applied to it, changing its permissions to root:android-input
and therefore making it readable by unity8-mir

in short: Retry opening a file when its permissions change as it might be
readable now.

Reproposing from my addendum branch as we want to land this quickly and dandrader is EOD. Thanks dandrader :D

==
Fixes bug 1233944.

But further work might be required (likely outside mir) due to the following error message when android-input loads the autopilot-finger device file:

"""
[InputReader] Touch device 'autopilot-finger' did not report support for X or Y axis! The device will be inoperable.
"""

[Robert Carr (robertcarr)]

+1

review: Approve

[Robert Carr (robertcarr)]

I'm worried this may lead to opening devices multiple times. Digging deeper.

review: Needs Information

[Robert Carr (robertcarr)]

I think this can lead to open devices multiple times (ending with multiple instances in the mDevices member). I think the behavior is a little unclear here...so I've proposed a small fix:

https://code.launchpad.net/~robertcarr/mir/1233944-addendum/+merge/188896

review: Needs Fixing

[Robert Carr (robertcarr)]

review: Abstain

[Robert Ancell (robert-ancell)]

Not tested locally but the code makes sense to me as a workaround.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1100
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/78/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2210
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2095
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/77/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/78/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

code looks okay to me too, but i'm unfamiliar with this part of the code.

review: Abstain

[kevin gunn (kgunn72)]

this was ok and unity8 ran fine according to racarr

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

I'm too late to the party but still:

hasDeviceByPathLocked() pretty much duplicates code (i.e. code bloat) as you could perfectly use the existing getDeviceByPathLocked(), like this:

s/hasDeviceByPathLocked(path)/getDeviceByPathLocked(path) != nullptr/

or even simply:

s/hasDeviceByPathLocked(path)/getDeviceByPathLocked(path)/

Secondly, we are checking for it twice. There's already a check for a pre-existing device in on diff line 31. Not checking for it in the "if(event->mask & IN_CREATE) {...}" case was already the case and it's not a problem as files do not get created twice (you gotta destroy it before recreating it).

In short: duplicated code + extra, superfluous, runtime check.

review: Disapprove

[Daniel d'Andrada (dandrader)]

> In short: duplicated code + extra, superfluous, runtime check.

This is not the end of the world, naturally. :)
But really think this addendum is redundant.

=== modified file '3rd_party/android-input/android/frameworks/base/services/input/EventHub.cpp'
--- 3rd_party/android-input/android/frameworks/base/services/input/EventHub.cpp	2013-05-28 17:52:18 +0000
+++ 3rd_party/android-input/android/frameworks/base/services/input/EventHub.cpp	2013-10-02 19:00:39 +0000
@@ -223,7 +223,7 @@
     LOG_ALWAYS_FATAL_IF(mEpollFd < 0, "Could not create epoll instance.  errno=%d", errno);
 
     mINotifyFd = inotify_init();
-    int result = inotify_add_watch(mINotifyFd, DEVICE_PATH, IN_DELETE | IN_CREATE);
+    int result = inotify_add_watch(mINotifyFd, DEVICE_PATH, IN_DELETE | IN_CREATE | IN_ATTRIB);
     LOG_ALWAYS_FATAL_IF(result < 0, "Could not register INotify for %s.  errno=%d",
             DEVICE_PATH, errno);
 
@@ -955,6 +955,11 @@
     char buffer[80];
 
     ALOGV("Opening device: %s", devicePath);
+    if (hasDeviceByPathLocked(String8(devicePath)))
+    {
+        ALOGV("Not opening device (%s), as it is already opened", devicePath);
+        return -1;
+    }
 
     int fd = open(devicePath, O_RDWR | O_CLOEXEC);
     if(fd < 0) {
@@ -1416,6 +1421,14 @@
             strcpy(filename, event->name);
             if(event->mask & IN_CREATE) {
                 openDeviceLocked(devname);
+            } else if (event->mask & IN_ATTRIB) {
+                Device* device = getDeviceByPathLocked(devname);
+                if (!device) {
+                    ALOGI("Retry opening device file %s", devname);
+                    // file permissions might have changed, making the device readable now
+                    // let's retry opening it
+                    openDeviceLocked(devname);
+                }
             } else {
                 ALOGI("Removing device '%s' due to inotify event\n", devname);
                 closeDeviceByPathLocked(devname);
@@ -1523,4 +1536,13 @@
     }
 }
 
+bool EventHub::hasDeviceByPathLocked(String8 const& path)
+{
+    for (size_t i = 0; i < mDevices.size(); i++) {
+        auto const& device = mDevices.valueAt(i);
+        if (device->path == path) return true;
+    }
+    return false;
+}
+
 }; // namespace android

=== modified file '3rd_party/android-input/android/frameworks/base/services/input/EventHub.h'
--- 3rd_party/android-input/android/frameworks/base/services/input/EventHub.h	2013-05-28 17:47:03 +0000
+++ 3rd_party/android-input/android/frameworks/base/services/input/EventHub.h	2013-10-02 19:00:39 +0000
@@ -395,6 +395,7 @@
 
     int32_t mNextDeviceId;
 
+    bool hasDeviceByPathLocked(String8 const& path);
     KeyedVector<int32_t, Device*> mDevices;
 
     Device *mOpeningDevices;