Merge lp:~kdub/mir/fd-refcount into lp:mir

[Kevin DuBois]

make the mir::Fd type CopyConstructible (and also CopyAssignable)

make the mir::Fd type CopyConstructible (and also CopyAssignable)

the Fd type that landed a bit earlier this week was MoveConstructible/MoveAssignable, but there's no reason it can't be CopyConstructible/Assignable. This could be done via dup(), but that entails an extra hop to the kernel, so I implemented refcounting for the raw fd instead of using dup.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1799
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2238/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1091
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1097
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1081
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/760
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/760/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/761
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/761/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/16
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/16/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2201
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10439

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

26 + struct CountedFd
27 + {
28 + CountedFd(int fd);
29 + int const raw_fd;
30 + std::atomic<unsigned int> refcount;
31 + };
32 + CountedFd *fd;

Use a std::shared_ptr<int> (with a custom deleter)?

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

@"This could be done via dup()"

Can it? doesn't that, for example, lose FD_CLOEXEC.

[Alan Griffiths (alan-griffiths)]

> 26 + struct CountedFd
> 27 + {
> 28 + CountedFd(int fd);
> 29 + int const raw_fd;
> 30 + std::atomic<unsigned int> refcount;
> 31 + };
> 32 + CountedFd *fd;
>
> Use a std::shared_ptr<int> (with a custom deleter)?

Yes. That's simpler.

review: Needs Fixing

[Kevin DuBois (kdub)]

But isn't it more fun to read new hand-crafted, artisan code instead of mass produced code? (kidding)
Updated to use shared_ptr<>, it is a bit cleaner.

[Alan Griffiths (alan-griffiths)]

17 + Fd(Fd const&);
...
51 +mir::Fd::Fd(Fd const& other) :
52 + fd{other.fd}
53 {
54 }

Can be even simpler:

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1801
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2254/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1124
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1109
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/776
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/776/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/777
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/777/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/44
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/44/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2232
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10554

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

> @"This could be done via dup()"
>
> Can it? doesn't that, for example, lose FD_CLOEXEC.

Not if you use dup2().

That said, std::shared_ptr does the job admirably.

Not blocking, but I *think* you could use std::shared_ptr<int const> now, throw when constructing from an invalid fd, and drop the conditional in the shared_ptr's deleter?

While we're at it, is there any reason not to make operator int() throw when called on an invalid fd?

[Alexandros Frantzis (afrantzis)]

OK.

review: Approve

[Kevin DuBois (kdub)]

@Chris, I'm shying away from throwing on invalid fds because some code has the concept of an invalid fd, and it sometimes needs to access the invalid fd without throwing. (thinking mostly of some of the hwc classes)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1802
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2262/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1129
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1120
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/784
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/784/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/785
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/785/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/55
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/55/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2248
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10639

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

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

Looking through MPs to see how other peopel are dealing with this sha1sums file...shouldn't include/shared/mir/fd.h be in server sha1sums?

[Chris Halse Rogers (raof)]

> @Chris, I'm shying away from throwing on invalid fds because some code has the
> concept of an invalid fd, and it sometimes needs to access the invalid fd
> without throwing. (thinking mostly of some of the hwc classes)

Urgh, ok :(

review: Abstain

[Cemil Azizoglu (cemil-azizoglu)]

> Looking through MPs to see how other peopel are dealing with this sha1sums
> file...shouldn't include/shared/mir/fd.h be in server sha1sums?

[Cemil Azizoglu (cemil-azizoglu)]

> > Looking through MPs to see how other peopel are dealing with this sha1sums
> > file...shouldn't include/shared/mir/fd.h be in server sha1sums?

+1

(Misclicked earlier)

[Alan Griffiths (alan-griffiths)]

> Looking through MPs to see how other peopel are dealing with this sha1sums
> file...shouldn't include/shared/mir/fd.h be in server sha1sums?

It should once it is pushed to distro. (ABI versioning doesn't apply until then.)

review: Approve

=== modified file 'include/shared/mir/fd.h'
--- include/shared/mir/fd.h	2014-07-14 14:52:08 +0000
+++ include/shared/mir/fd.h	2014-07-29 16:48:39 +0000
@@ -18,6 +18,8 @@
 #ifndef MIR_FD_H_
 #define MIR_FD_H_
 
+#include <memory>
+
 namespace mir
 {
 class Fd
@@ -29,14 +31,14 @@
     static int const invalid{-1};
     Fd(); //Initializes fd to the mir::Fd::invalid;
     Fd(Fd&&);
+    Fd(Fd const&) = default;
     Fd& operator=(Fd);
-    ~Fd() noexcept;
 
     //bit of a convenient kludge. take care not to close or otherwise destroy the FD.
     operator int() const;
 
 private:
-    int fd;
+    std::shared_ptr<int> fd;
 };
 } // namespace mir
 

=== modified file 'src/shared/fd/fd.cpp'
--- src/shared/fd/fd.cpp	2014-07-14 14:52:08 +0000
+++ src/shared/fd/fd.cpp	2014-07-29 16:48:39 +0000
@@ -24,20 +24,20 @@
 {
 }
 
-mir::Fd::Fd(int other_fd) :
-    fd{other_fd}
+mir::Fd::Fd(int raw_fd) :
+    fd{new int{raw_fd},
+        [](int* fd)
+        {
+            if (!fd) return;
+            if (*fd > mir::Fd::invalid) ::close(*fd);
+            delete fd;
+        }} 
 {
 }
 
 mir::Fd::Fd(Fd&& other) :
-    fd{other.fd}
-{
-    other.fd = invalid;
-}
-
-mir::Fd::~Fd() noexcept
-{
-    if (fd > invalid) ::close(fd);
+    fd{std::move(other.fd)}
+{
 }
 
 mir::Fd& mir::Fd::operator=(Fd other)
@@ -48,5 +48,6 @@
 
 mir::Fd::operator int() const
 {
-    return fd;
+    if (fd) return *fd;
+    return invalid;
 }

=== modified file 'tests/unit-tests/CMakeLists.txt'
--- tests/unit-tests/CMakeLists.txt	2014-07-28 16:26:39 +0000
+++ tests/unit-tests/CMakeLists.txt	2014-07-29 16:48:39 +0000
@@ -20,6 +20,7 @@
   test_thread_name.cpp
   test_default_emergency_cleanup.cpp
   test_basic_observers.cpp
+  test_fd.cpp
 )
 
 add_subdirectory(options/)

=== added file 'tests/unit-tests/test_fd.cpp'
--- tests/unit-tests/test_fd.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/test_fd.cpp	2014-07-29 16:48:39 +0000
@@ -0,0 +1,81 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#include "mir/fd.h"
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdio.h>
+
+namespace
+{
+
+bool fd_is_open(int fd)
+{
+    return fcntl(fd, F_GETFD) != -1;
+}
+
+struct Fd : public testing::Test
+{
+    Fd() :
+        raw_fd(::open("/dev/null", 0))
+    {
+    }
+
+    ~Fd()
+    {
+        if (fd_is_open(raw_fd))
+            ::close(raw_fd);
+    }
+
+    int const raw_fd;
+};
+}
+
+TEST_F(Fd, closes_when_refcount_is_zero)
+{
+    EXPECT_TRUE(fd_is_open(raw_fd));
+    mir::Fd fd2(-1);
+    {
+        mir::Fd fd(raw_fd);
+        {
+            mir::Fd fd1(fd);
+            fd2 = fd1;
+        }
+    }
+
+    EXPECT_TRUE(fd_is_open(raw_fd));
+    fd2 = mir::Fd(-1);
+    EXPECT_FALSE(fd_is_open(raw_fd));
+}
+
+TEST_F(Fd, moves_around)
+{
+    EXPECT_TRUE(fd_is_open(raw_fd));
+    mir::Fd fd0(-1);
+    fd0 = mir::Fd(raw_fd);
+    mir::Fd fd1(std::move(fd0));
+    mir::Fd fd2(fd1);
+
+    EXPECT_TRUE(fd_is_open(raw_fd));
+    fd1 = mir::Fd(-1);
+    EXPECT_TRUE(fd_is_open(raw_fd));
+    fd2 = mir::Fd(-1);
+    EXPECT_FALSE(fd_is_open(raw_fd));
+}