[devcoordinator] Encapsulate Device::children
This no longer exposes the underlying list outside of the child class
Change-Id: Ib70437cc84a9649f43fa0abc3c93caff73aab08c
diff --git a/zircon/system/core/devmgr/devcoordinator/coordinator.cpp b/zircon/system/core/devmgr/devcoordinator/coordinator.cpp
index 5c79032..9072e29 100644
--- a/zircon/system/core/devmgr/devcoordinator/coordinator.cpp
+++ b/zircon/system/core/devmgr/devcoordinator/coordinator.cpp
@@ -294,7 +294,7 @@
indent++;
DumpDevice(vmo, dev->proxy.get(), indent);
}
- for (const auto& child : dev->children) {
+ for (const auto& child : dev->children()) {
DumpDevice(vmo, &child, indent + 1);
}
}
@@ -345,7 +345,7 @@
if (dev->proxy) {
DumpDeviceProps(vmo, dev->proxy.get());
}
- for (const auto& child : dev->children) {
+ for (const auto& child : dev->children()) {
DumpDeviceProps(vmo, &child);
}
}
@@ -765,12 +765,9 @@
// if we have a parent, disconnect and downref it
fbl::RefPtr<Device> parent = dev->parent();
if (parent != nullptr) {
- dev->set_parent(nullptr);
- if (dev->flags & DEV_CTX_PROXY) {
- parent->proxy = nullptr;
- } else {
- parent->children.erase(*dev);
- if (parent->children.is_empty()) {
+ dev->DetachFromParent();
+ if (!(dev->flags & DEV_CTX_PROXY)) {
+ if (parent->children().is_empty()) {
parent->flags &= (~DEV_CTX_BOUND);
// TODO: This code is to cause the bind process to
diff --git a/zircon/system/core/devmgr/devcoordinator/device.cpp b/zircon/system/core/devmgr/devcoordinator/device.cpp
index 028b743..845094a 100644
--- a/zircon/system/core/devmgr/devcoordinator/device.cpp
+++ b/zircon/system/core/devmgr/devcoordinator/device.cpp
@@ -104,7 +104,7 @@
// TODO host == nullptr should be impossible
dev->host_->devices().push_back(dev.get());
}
- real_parent->children.push_back(dev.get());
+ real_parent->children_.push_back(dev.get());
log(DEVLC, "devcoord: dev %p name='%s' (child)\n", real_parent.get(), real_parent->name.data());
*device = std::move(dev);
@@ -186,6 +186,15 @@
return ZX_OK;
}
+void Device::DetachFromParent() {
+ if (this->flags & DEV_CTX_PROXY) {
+ parent_->proxy = nullptr;
+ } else {
+ parent_->children_.erase(*this);
+ }
+ parent_ = nullptr;
+}
+
zx_status_t Device::SignalReadyForBind(zx::duration delay) {
return publish_task_.PostDelayed(this->coordinator->dispatcher(), delay);
}
diff --git a/zircon/system/core/devmgr/devcoordinator/device.h b/zircon/system/core/devmgr/devcoordinator/device.h
index 6c6007d..800eab25 100644
--- a/zircon/system/core/devmgr/devcoordinator/device.h
+++ b/zircon/system/core/devmgr/devcoordinator/device.h
@@ -77,6 +77,71 @@
async::WaitBase* wait, zx_status_t status,
const zx_packet_signal_t* signal);
+ // Node for entry in device child list
+ struct Node {
+ static fbl::DoublyLinkedListNodeState<Device*>& node_state(Device& obj) {
+ return obj.node_;
+ }
+ };
+
+ // This iterator provides access to a list of devices that does not provide
+ // mechanisms for mutating that list. With this, a user can get mutable
+ // access to a device in the list. This is achieved by making the linked
+ // list iterator opaque. It is not safe to modify the underlying list while
+ // this iterator is in use.
+ template <typename IterType, typename ValueType>
+ class ChildListIterator {
+ public:
+ explicit ChildListIterator(IterType itr) : itr_(std::move(itr)) {}
+
+ ChildListIterator& operator++() { ++itr_; return *this; }
+ ChildListIterator operator++(int) {
+ auto other = *this;
+ ++*this;
+ return other;
+ }
+ ValueType& operator*() const { return *itr_; }
+ bool operator==(const ChildListIterator& other) const { return itr_ == other.itr_; }
+ bool operator!=(const ChildListIterator& other) const { return itr_ != other.itr_; }
+ private:
+ IterType itr_;
+ };
+
+ // This class exists to allow consumers of the Device class to write
+ // for (auto& child : dev->children())
+ // and get mutable access to the children without getting mutable access to
+ // the list.
+ template <typename ListType, typename IterType>
+ class ChildListIteratorFactory {
+ public:
+ explicit ChildListIteratorFactory(ListType& list) : list_(list) {}
+
+ IterType begin() const { return IterType(list_.begin()); }
+ IterType end() const { return IterType(list_.end()); }
+
+ bool is_empty() const { return begin() == end(); }
+ private:
+ ListType& list_;
+ };
+
+ using NonConstChildListIterator =
+ ChildListIterator<fbl::DoublyLinkedList<Device*, Node>::iterator, Device>;
+ using ConstChildListIterator =
+ ChildListIterator<fbl::DoublyLinkedList<Device*, Node>::const_iterator, const Device>;
+ using NonConstChildListIteratorFactory = ChildListIteratorFactory<
+ fbl::DoublyLinkedList<Device*, Node>, NonConstChildListIterator>;
+ using ConstChildListIteratorFactory = ChildListIteratorFactory<
+ const fbl::DoublyLinkedList<Device*, Node>, ConstChildListIterator>;
+ // We do not want to expose the list itself for mutation, even if the
+ // children are allowed to be mutated. We manage this by making the
+ // iterator opaque.
+ NonConstChildListIteratorFactory children() {
+ return NonConstChildListIteratorFactory(children_);
+ }
+ ConstChildListIteratorFactory children() const {
+ return ConstChildListIteratorFactory(children_);
+ }
+
// Signal that this device is ready for bind to happen. This should happen
// either immediately after the device is created, if it's created visible,
// or after it becomes visible.
@@ -110,13 +175,6 @@
// or once the device becomes visible.
zx::channel client_remote;
- // listnode for this device in its parent's
- // list-of-children
- fbl::DoublyLinkedListNodeState<Device*> node;
- struct Node {
- static fbl::DoublyLinkedListNodeState<Device*>& node_state(Device& obj) { return obj.node; }
- };
-
// listnode for this device in its devhost's
// list-of-devices
fbl::DoublyLinkedListNodeState<Device*> dhnode;
@@ -126,9 +184,6 @@
}
};
- // list of all child devices of this device
- fbl::DoublyLinkedList<Device*, Node> children;
-
// listnode for this device in the all devices list
fbl::DoublyLinkedListNodeState<Device*> anode;
struct AllDevicesNode {
@@ -137,6 +192,9 @@
}
};
+ // Break the relationship between this device object and its parent
+ void DetachFromParent();
+
// Metadata entries associated to this device.
fbl::DoublyLinkedList<fbl::unique_ptr<Metadata>, Metadata::Node> metadata;
@@ -198,7 +256,7 @@
kSuspended,
};
- State state() { return state_; }
+ State state() const { return state_; }
private:
zx_status_t HandleRead();
@@ -212,6 +270,12 @@
async::TaskClosure publish_task_;
+ // listnode for this device in its parent's list-of-children
+ fbl::DoublyLinkedListNodeState<Device*> node_;
+
+ // list of all child devices of this device
+ fbl::DoublyLinkedList<Device*, Node> children_;
+
// - If this device is part of a composite device, this is inhabited by
// CompositeDeviceComponent* and it points to the component that matched it.
// Note that this is only set on the device that matched the component, not
diff --git a/zircon/system/core/devmgr/devcoordinator/suspend-task.cpp b/zircon/system/core/devmgr/devcoordinator/suspend-task.cpp
index 2e91131..6035380 100644
--- a/zircon/system/core/devmgr/devcoordinator/suspend-task.cpp
+++ b/zircon/system/core/devmgr/devcoordinator/suspend-task.cpp
@@ -21,7 +21,7 @@
void SuspendTask::Run() {
bool found_more_dependencies = false;
- for (auto& child : device_->children) {
+ for (auto& child : device_->children()) {
// Use a switch statement here so that this gets reconsidered if we add
// more states.
switch (child.state()) {
