cryptohome: Clean up CryptohomeEventSource
While staring at this code anyways, replace some of the ancient code
with more modern C++ idioms. No changes in functionality intended.
BUG=None
TEST=Builds and passes tests.
Change-Id: Ia40858e458cee6c9aa1722e187a4519f3334bdc4
Reviewed-on: https://chromium-review.googlesource.com/1350979
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
diff --git a/cryptohome/cryptohome_event_source.cc b/cryptohome/cryptohome_event_source.cc
index 0e26f0b..14b6c4d 100644
--- a/cryptohome/cryptohome_event_source.cc
+++ b/cryptohome/cryptohome_event_source.cc
@@ -4,45 +4,36 @@
#include "cryptohome/cryptohome_event_source.h"
-#include <base/logging.h>
#include <poll.h>
#include <unistd.h>
+#include <memory>
+#include <utility>
+
+#include <base/logging.h>
+
namespace cryptohome {
GSourceFuncs CryptohomeEventSource::source_functions_ = {
CryptohomeEventSource::Prepare,
CryptohomeEventSource::Check,
CryptohomeEventSource::Dispatch,
- NULL
+ nullptr
};
-CryptohomeEventSource::CryptohomeEventSource()
- : sink_(NULL),
- source_(NULL),
- events_(),
- events_lock_(),
- poll_fd_() {
+CryptohomeEventSource::CryptohomeEventSource() {
pipe_fds_[0] = -1;
pipe_fds_[1] = -1;
}
CryptohomeEventSource::~CryptohomeEventSource() {
- if (source_) {
- g_source_destroy(source_);
- g_source_unref(source_);
- }
Clear();
}
void CryptohomeEventSource::Reset(CryptohomeEventSourceSink* sink,
GMainContext* main_context) {
sink_ = sink;
- if (source_) {
- g_source_destroy(source_);
- g_source_unref(source_);
- source_ = NULL;
- }
+ source_.reset();
for (int i = 0; i < 2; i++) {
if (pipe_fds_[i] != -1) {
@@ -54,17 +45,17 @@
Clear();
if (!pipe(pipe_fds_)) {
- poll_fd_.fd = pipe_fds_[0];
- poll_fd_.events = G_IO_IN | G_IO_HUP | G_IO_ERR;
- poll_fd_.revents = 0;
- source_ = static_cast<Source*>(g_source_new(&source_functions_,
- sizeof(Source)));
+ source_.reset(
+ static_cast<Source*>(g_source_new(&source_functions_, sizeof(Source))));
source_->event_source = this;
- g_source_add_poll(source_, &poll_fd_);
+ source_->poll_fd.fd = pipe_fds_[0];
+ source_->poll_fd.events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+ source_->poll_fd.revents = 0;
+ g_source_add_poll(source_.get(), &source_->poll_fd);
if (main_context) {
- g_source_attach(source_, main_context);
+ g_source_attach(source_.get(), main_context);
}
- g_source_set_can_recurse(source_, true);
+ g_source_set_can_recurse(source_.get(), true);
} else {
LOG(ERROR) << "Couldn't set up pipe for notifications.";
}
@@ -97,46 +88,39 @@
more = false;
}
} while (more);
- // Now handle pending events
- more = false;
- do {
- events_lock_.Acquire();
- if (!events_.empty()) {
- CryptohomeEventBase* event = events_[0];
- events_.erase(events_.begin());
- more = !events_.empty();
- events_lock_.Release();
- if (sink_) {
- sink_->NotifyEvent(event);
- }
- delete event;
- } else {
- more = false;
- events_lock_.Release();
+
+ // Now handle pending events.
+ std::vector<std::unique_ptr<CryptohomeEventBase>> events;
+ {
+ base::AutoLock lock(events_lock_);
+ events_.swap(events);
+ }
+
+ for (auto& event : events) {
+ if (sink_) {
+ sink_->NotifyEvent(event.get());
}
- } while (more);
+ }
}
-void CryptohomeEventSource::AddEvent(CryptohomeEventBase* event) {
- events_lock_.Acquire();
- events_.push_back(event);
+void CryptohomeEventSource::AddEvent(
+ std::unique_ptr<CryptohomeEventBase> event) {
+ base::AutoLock lock(events_lock_);
+ events_.push_back(std::move(event));
if (write(pipe_fds_[1], "G", 1) != 1) {
LOG(INFO) << "Couldn't notify of pending events through the message pipe."
<< " Events will be cleared on next call to Prepare().";
}
- events_lock_.Release();
}
void CryptohomeEventSource::Clear() {
- std::vector<CryptohomeEventBase*> events;
- events_lock_.Acquire();
- events_.swap(events);
- events_lock_.Release();
- for (std::vector<CryptohomeEventBase*>::const_iterator itr = events.begin();
- itr != events.end();
- ++itr) {
- delete (*itr);
- }
+ base::AutoLock lock(events_lock_);
+ events_.clear();
+}
+
+void CryptohomeEventSource::SourceDeleter::operator()(Source* source) {
+ g_source_destroy(source);
+ g_source_unref(source);
}
gboolean CryptohomeEventSource::Prepare(GSource* source, gint* timeout_ms) {
diff --git a/cryptohome/cryptohome_event_source.h b/cryptohome/cryptohome_event_source.h
index 33a8e9b..6101df4 100644
--- a/cryptohome/cryptohome_event_source.h
+++ b/cryptohome/cryptohome_event_source.h
@@ -5,7 +5,7 @@
// CryptohomeEventSource - Implements a GSource for the glib main event loop.
// This class is used to marshal asynchronous mount results from the worker
// thread (see service.h/.cc) over to the main event loop. That way, all of the
-// dbus messages are received and sent from the one thread, ensuring that
+// D-Bus messages are received and sent from the one thread, ensuring that
// signals returned by asynchronous commands are serialized with the original
// call.
//
@@ -25,6 +25,7 @@
#include <base/synchronization/lock.h>
#include <dbus/dbus-glib.h>
#include <glib-object.h>
+#include <memory>
#include <vector>
namespace cryptohome {
@@ -51,11 +52,10 @@
void HandleDispatch();
// Adds an event to the queue for processing.
- // This method DOES take ownership of the |event| pointer.
//
// Parameters
// task_result - The event to add
- void AddEvent(CryptohomeEventBase* event);
+ void AddEvent(std::unique_ptr<CryptohomeEventBase> event);
// Clears all pending events from the queue
void Clear();
@@ -66,6 +66,11 @@
// getting the instance of this CryptohomeEventSource.
struct Source : public GSource {
CryptohomeEventSource* event_source;
+ GPollFD poll_fd;
+ };
+
+ struct SourceDeleter {
+ void operator()(Source* source);
};
// Called by glib (see GSourceFuncs in the glib documentation)
@@ -80,13 +85,13 @@
gpointer unused_data);
// The event sink that handles event notifications
- CryptohomeEventSourceSink* sink_;
+ CryptohomeEventSourceSink* sink_ = nullptr;
- // The dbus GSource that we provide
- Source* source_;
+ // The D-Bus GSource that we provide
+ std::unique_ptr<Source, SourceDeleter> source_;
// Pending events vector
- std::vector<CryptohomeEventBase*> events_;
+ std::vector<std::unique_ptr<CryptohomeEventBase>> events_;
// Used to provide thread-safe access to events_
base::Lock events_lock_;
@@ -96,22 +101,21 @@
// The pipe used for our GPollFD
int pipe_fds_[2];
- GPollFD poll_fd_;
DISALLOW_COPY_AND_ASSIGN(CryptohomeEventSource);
};
class CryptohomeEventBase {
public:
- CryptohomeEventBase() { }
- virtual ~CryptohomeEventBase() { }
+ CryptohomeEventBase() = default;
+ virtual ~CryptohomeEventBase() = default;
virtual const char* GetEventName() const = 0;
};
class CryptohomeEventSourceSink {
public:
- virtual ~CryptohomeEventSourceSink() { }
+ virtual ~CryptohomeEventSourceSink() = default;
virtual void NotifyEvent(CryptohomeEventBase* event) = 0;
};
diff --git a/cryptohome/cryptohome_event_source_unittest.cc b/cryptohome/cryptohome_event_source_unittest.cc
index deaa018..2b777cd 100644
--- a/cryptohome/cryptohome_event_source_unittest.cc
+++ b/cryptohome/cryptohome_event_source_unittest.cc
@@ -6,17 +6,20 @@
#include "cryptohome/cryptohome_event_source.h"
-#include "cryptohome/vault_keyset.h"
+#include <memory>
+#include <utility>
#include <base/logging.h>
#include <gtest/gtest.h>
+#include "cryptohome/vault_keyset.h"
+
namespace cryptohome {
class CryptohomeEventSourceTest : public ::testing::Test {
public:
- CryptohomeEventSourceTest() { }
- virtual ~CryptohomeEventSourceTest() { }
+ CryptohomeEventSourceTest() = default;
+ virtual ~CryptohomeEventSourceTest() = default;
private:
DISALLOW_COPY_AND_ASSIGN(CryptohomeEventSourceTest);
@@ -29,29 +32,19 @@
class MyEvent : public CryptohomeEventBase {
public:
- MyEvent()
- : destructor_watcher_(NULL),
- id_(-1) { }
-
- virtual ~MyEvent() {
+ MyEvent(EventDestructorWatcher* watcher, int id)
+ : destructor_watcher_(watcher), id_(id) {}
+ ~MyEvent() override {
if (destructor_watcher_) {
destructor_watcher_->NotifyDestroy(this);
}
}
- void set_destructor_watcher(EventDestructorWatcher* watcher) {
- destructor_watcher_ = watcher;
- }
-
- void set_id(int id) {
- id_ = id;
- }
-
int get_id() {
return id_;
}
- virtual const char* GetEventName() const {
+ const char* GetEventName() const override {
return "MyEvent";
}
@@ -63,16 +56,14 @@
class EventSink : public CryptohomeEventSourceSink,
public EventDestructorWatcher {
public:
- EventSink()
- : completed_events_() { }
+ EventSink() = default;
+ ~EventSink() override = default;
- virtual ~EventSink() { }
-
- virtual void NotifyEvent(CryptohomeEventBase* event) {
+ void NotifyEvent(CryptohomeEventBase* event) override {
completed_events_.push_back(static_cast<MyEvent*>(event)->get_id());
}
- virtual void NotifyDestroy(CryptohomeEventBase* event) {
+ void NotifyDestroy(CryptohomeEventBase* event) override {
destroyed_events_.push_back(static_cast<MyEvent*>(event)->get_id());
}
@@ -87,10 +78,7 @@
static const int kEventCount = 4096;
for (int i = 0; i < kEventCount; i++) {
- MyEvent* event = new MyEvent();
- event->set_id(i);
- event->set_destructor_watcher(&event_sink);
- event_source.AddEvent(event);
+ event_source.AddEvent(std::make_unique<MyEvent>(&event_sink, i));
}
EXPECT_TRUE(event_source.EventsPending());
@@ -110,10 +98,7 @@
event_source.Reset(&event_sink, NULL);
for (int i = 0; i < kEventCount; i++) {
- MyEvent* event = new MyEvent();
- event->set_id(i);
- event->set_destructor_watcher(&event_sink);
- event_source.AddEvent(event);
+ event_source.AddEvent(std::make_unique<MyEvent>(&event_sink, i));
}
EXPECT_TRUE(event_source.EventsPending());
diff --git a/cryptohome/service.cc b/cryptohome/service.cc
index b8ad673..e66b47f 100644
--- a/cryptohome/service.cc
+++ b/cryptohome/service.cc
@@ -120,27 +120,18 @@
class TpmInitStatus : public CryptohomeEventBase {
public:
- TpmInitStatus()
- : took_ownership_(false),
- status_(false) { }
- virtual ~TpmInitStatus() { }
+ TpmInitStatus(bool took_ownership, bool status)
+ : took_ownership_(took_ownership), status_(status) {}
+ ~TpmInitStatus() override = default;
- virtual const char* GetEventName() const {
+ const char* GetEventName() const override {
return kTpmInitStatusEventType;
}
- void set_took_ownership(bool value) {
- took_ownership_ = value;
- }
-
bool get_took_ownership() {
return took_ownership_;
}
- void set_status(bool value) {
- status_ = value;
- }
-
bool get_status() {
return status_;
}
@@ -320,7 +311,8 @@
// DBusReply will take ownership of the |reply_str|.
std::unique_ptr<std::string> reply_str(new std::string);
reply.SerializeToString(reply_str.get());
- event_source_.AddEvent(new DBusReply(context, reply_str.release()));
+ event_source_.AddEvent(
+ std::make_unique<DBusReply>(context, reply_str.release()));
}
void Service::SendDBusErrorReply(DBusGMethodInvocation* context,
@@ -328,7 +320,7 @@
gint code,
const gchar* message) {
GError* error = g_error_new_literal(domain, code, message);
- event_source_.AddEvent(new DBusErrorReply(context, error));
+ event_source_.AddEvent(std::make_unique<DBusErrorReply>(context, error));
}
bool Service::FilterActiveMounts(
@@ -934,11 +926,8 @@
// can't do it prior to ownership.
InitializeInstallAttributes(true);
}
- // The event source will free this object
- TpmInitStatus* tpm_init_status = new TpmInitStatus();
- tpm_init_status->set_status(status);
- tpm_init_status->set_took_ownership(took_ownership);
- event_source_.AddEvent(tpm_init_status);
+ event_source_.AddEvent(
+ std::make_unique<TpmInitStatus>(took_ownership, status));
// Do attestation work after AddEvent because it may take long.
AttestationInitializeTpmComplete();
@@ -2106,7 +2095,7 @@
DircryptoMigrationStatus status,
uint64_t current_bytes,
uint64_t total_bytes) {
- event_source_.AddEvent(new DircryptoMigrationProgress(
+ event_source_.AddEvent(std::make_unique<DircryptoMigrationProgress>(
status, current_bytes, total_bytes));
}
diff --git a/cryptohome/service.h b/cryptohome/service.h
index ea82cc6..ad2a7a5 100644
--- a/cryptohome/service.h
+++ b/cryptohome/service.h
@@ -8,6 +8,7 @@
#include <map>
#include <memory>
#include <string>
+#include <utility>
#include <vector>
#include <base/atomic_sequence_num.h>
@@ -65,9 +66,9 @@
: mount_(mount), source_(source) { }
virtual ~MountTaskObserverBridge() { }
virtual bool MountTaskObserve(const MountTaskResult& result) {
- MountTaskResult *r = new MountTaskResult(result);
+ auto r = std::make_unique<MountTaskResult>(result);
r->set_mount(mount_);
- source_->AddEvent(r);
+ source_->AddEvent(std::move(r));
return true;
}
diff --git a/cryptohome/service_distributed.cc b/cryptohome/service_distributed.cc
index 9b233e0..1009c51 100644
--- a/cryptohome/service_distributed.cc
+++ b/cryptohome/service_distributed.cc
@@ -12,6 +12,8 @@
#include "tpm_manager/common/tpm_manager_constants.h"
#include "tpm_manager/common/tpm_ownership_dbus_interface.h"
+#include <utility>
+
using attestation::AttestationInterface;
using attestation::AttestationStatus;
@@ -174,14 +176,14 @@
void ServiceDistributed::ProcessStatusReply(int async_id,
const ReplyProtoType& reply) {
VLOG(1) << __func__;
- MountTaskResult* r = new MountTaskResult();
+ auto r = std::make_unique<MountTaskResult>();
VLOG(3) << "attestationd reply:"
<< " async_id=" << async_id << " status=" << reply.status();
VLOG_IF(1, reply.status() != AttestationStatus::STATUS_SUCCESS)
<< "Attestation daemon returned status " << reply.status();
r->set_sequence_id(async_id);
r->set_return_status(reply.status() == AttestationStatus::STATUS_SUCCESS);
- event_source_.AddEvent(r);
+ event_source_.AddEvent(std::move(r));
}
template <typename ReplyProtoType>
@@ -190,7 +192,7 @@
int async_id,
const ReplyProtoType& reply) {
VLOG(1) << __func__;
- MountTaskResult* r = new MountTaskResult();
+ auto r = std::make_unique<MountTaskResult>();
VLOG(3) << "attestationd reply:"
<< " async_id=" << async_id << " status=" << reply.status();
VLOG_IF(1, reply.status() != AttestationStatus::STATUS_SUCCESS)
@@ -199,7 +201,7 @@
r->set_return_status(reply.status() == AttestationStatus::STATUS_SUCCESS);
brillo::SecureBlob blob((reply.*func)());
r->set_return_data(blob);
- event_source_.AddEvent(r);
+ event_source_.AddEvent(std::move(r));
}
void ServiceDistributed::ProcessGetEndorsementInfoReply(
