Open a firewall hole when a TCP server or UDP socket is bound.

chromeos/network/firewall_hole.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_NETWORK_FIREWALL_HOLE_H_
+#define CHROMEOS_NETWORK_FIREWALL_HOLE_H_
+
+#include <string>
+
+#include "base/basictypes.h"

[pneubeck (no reviews), 2015/03/03 22:24:51]

what for? please see the comment in that header regarding uint16* .

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

Done.

+#include "base/callback.h"

[pneubeck (no reviews), 2015/03/03 22:24:51]

is callback_forward.h sufficient?

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

Done.

+#include "base/threading/thread_checker.h"
+#include "chromeos/chromeos_export.h"
+
+namespace dbus {
+class FileDescriptor;
+}
+
+namespace chromeos {
+
+// This class works with the Chrome OS permission broker to open a port in the
+// system firewall. It is closed on destruction.
+class CHROMEOS_EXPORT FirewallHole {
+ public:
+  enum PortType {

[pneubeck (no reviews), 2015/03/03 22:24:52]

I think 'enum class' is now the preferred kind as it provides correct
scoping/name qualification.

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

Done.

+    UDP,
+    TCP,
+  };
+
+  typedef base::Callback<void(scoped_ptr<FirewallHole>)> OpenCallback;
+
+  static void Open(PortType type,

[pneubeck (no reviews), 2015/03/03 22:24:51]

This interface implies that the newly created object is self-owned (or say
unonwed) until it is passed to |callback|, which forces the caller to use a
WeakPtr.

Can you instead synchronously pass ownership and instead hide a weakptr
internally?

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

I don't understand your objection. There are no weak pointers necessary in this
implementation.

[pneubeck (no reviews), 2015/03/08 03:35:01]

Don't get me wrong. I just want to make sure we're on the same line
understanding what the interface of this class is and that we agree that it is
what is reasonable.
With the current Open(...,callback) function, |callback| is called at anytime in
the future and the caller can't prevent it.
So the caller has to guarantee that |callback| is valid until it's called, but
doesn't know when. The only way to do that is using a WeakPtr or binding
ownership to |callback|, e.g. with a ref counted ptr.

In your case it happens that ExtensionFunction is already refcounted.

An alternative interface that doesn't force the caller to an indefinite lifetime
and instead uses explicit ownership could have been:

  // |callback| is not called after the returned object is destructed.
  static scoped_ptr<FirewallHole> Open(..., callback);
  bool IsOpen();

Please think about it. I know that you changed your code already because of
another comment. I should have stated my point about the WeakPtr a bit clearer
to prevent this.

+                   uint16 port,
+                   const std::string& interface,

[pneubeck (no reviews), 2015/03/03 22:24:51]

needs a comment what |interface| is / what format it expects / what values are
valid.

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

Done.

+                   const OpenCallback& callback);
+
+  ~FirewallHole();
+
+ private:
+  FirewallHole(PortType type,
+               uint16 port,
+               const std::string& interface,
+               dbus::FileDescriptor* lifeline_fd);

[pneubeck (no reviews), 2015/03/03 22:24:51]

this takes ownership of lifeline_fd, which should be documented. ideally using
scoped_ptr and not a comment.

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

I believe this is addressed in the latest patchset.

+
+  void OnLifelineCreated(dbus::FileDescriptor* lifeline_remote,

[pneubeck (no reviews), 2015/03/03 22:24:51]

what's lifeline? that's unclear by looking only at this declaration.

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

I believe this is addressed in the latest patchset.

+                         const OpenCallback& callback);
+  void OnPortAccessGranted(const FirewallHole::OpenCallback& callback,
+                           bool success);
+
+  const PortType type_;
+  const uint16 port_;
+  const std::string interface_;
+
+  // Has the permission broker granted this firewall exception?
+  bool opened_;
+
+  // An file descriptor used by firewalld to track the lifetime of this process.
+  dbus::FileDescriptor* lifeline_fd_;

[pneubeck (no reviews), 2015/03/03 22:24:51]

should document that this is owning the filedescriptor.

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

I believe this is addressed in the latest patchset.

+
+  base::ThreadChecker thread_checker_;
+};
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_FIREWALL_HOLE_H_