Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2475343002:
Add UpstartClient (Closed)
|
Created:
4 years, 1 month ago by Roman Sorokin (ftl) Modified:
4 years, 1 month ago Reviewers:
hashimoto CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UpstartClient
Add StartAuthPolicyService into UpstartClient
StartAuthPolicyService would be used by chrome to start authpolicyd service
BUG=638663
Committed: https://crrev.com/58e0a2f05a6d6c67a7ea4290e7ce8d0380d95e57
Cr-Commit-Position: refs/heads/master@{#432239}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Created UpstartClient and moved StartService there #
Total comments: 19
Patch Set 3 : Addressed comments #Patch Set 4 : Wait for D-BUS response #
Total comments: 6
Patch Set 5 : nits #Patch Set 6 : Rebase #
Messages
Total messages: 31 (14 generated)
rsorokin@chromium.org changed reviewers: + hashimoto@chromium.org
Hi Hashimoto, please take a look.
https://codereview.chromium.org/2475343002/diff/1/chromeos/dbus/auth_policy_c... File chromeos/dbus/auth_policy_client.cc (right): https://codereview.chromium.org/2475343002/diff/1/chromeos/dbus/auth_policy_c... chromeos/dbus/auth_policy_client.cc:17: const char kUpstartServiceName[] = "com.ubuntu.Upstart"; AuthPolicyClient should deal only with AuthPolicy service. If you want to interact with Upstart, it should belong to a different client class.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by rsorokin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2475343002/diff/1/chromeos/dbus/auth_policy_c... File chromeos/dbus/auth_policy_client.cc (right): https://codereview.chromium.org/2475343002/diff/1/chromeos/dbus/auth_policy_c... chromeos/dbus/auth_policy_client.cc:17: const char kUpstartServiceName[] = "com.ubuntu.Upstart"; On 2016/11/08 00:12:26, hashimoto wrote: > AuthPolicyClient should deal only with AuthPolicy service. > If you want to interact with Upstart, it should belong to a different client > class. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add StartService into AuthPolicyClient StartService would be used by chrome to start authpolicyd service BUG=638663 ========== to ========== Add UpstartClient Add StartAuthPolicyService into UpstartClient StartAuthPolicyService would be used by chrome to start authpolicyd service BUG=638663 ==========
https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/fake_upst... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/fake_upst... chromeos/dbus/fake_upstart_client.h:20: void Init(dbus::Bus* bus) override; nit: Add a blank line. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = Please make proxy_ a member of this class to be consistent with other D-Bus client classes. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:37: writer.CloseContainer(&sub_writer); suggestion: How about writer.AppendArrayOfStrings(std::vector<std::string>()); ? https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:38: writer.AppendBool(false); // No waiting for response. IIUC this parameter changes the timing when the response is sent (i.e. when HandleUpstartCallback() is called), so "true" here would let us see the actual Start() result in the callback. Why "false" is preferred? https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:48: void HandleUpstartCallback(dbus::Response* response) { HandleUpstartCallback sounds ambiguous as basically everything in this class should be related to Upstart :) Maybe something like "HanldeStartResponse" is better? https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:51: return; nit: This line is redundant. Probably you should just use LOG_IF?
https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/fake_upst... File chromeos/dbus/fake_upstart_client.h (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/fake_upst... chromeos/dbus/fake_upstart_client.h:20: void Init(dbus::Bus* bus) override; On 2016/11/08 20:23:58, hashimoto wrote: > nit: Add a blank line. Done. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = On 2016/11/08 20:23:58, hashimoto wrote: > Please make proxy_ a member of this class to be consistent with other D-Bus > client classes. Hmm, what is the right way to do that? Since this proxy is specific to AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:37: writer.CloseContainer(&sub_writer); On 2016/11/08 20:23:59, hashimoto wrote: > suggestion: How about writer.AppendArrayOfStrings(std::vector<std::string>()); ? Done. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:38: writer.AppendBool(false); // No waiting for response. On 2016/11/08 20:23:58, hashimoto wrote: > IIUC this parameter changes the timing when the response is sent (i.e. when > HandleUpstartCallback() is called), so "true" here would let us see the actual > Start() result in the callback. > > Why "false" is preferred? I checked both options, but response is null in both cases (if startup failed for some reason). So not sure what is the advantage of waiting it. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:48: void HandleUpstartCallback(dbus::Response* response) { On 2016/11/08 20:23:58, hashimoto wrote: > HandleUpstartCallback sounds ambiguous as basically everything in this class > should be related to Upstart :) > > Maybe something like "HanldeStartResponse" is better? Done. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:51: return; On 2016/11/08 20:23:58, hashimoto wrote: > nit: This line is redundant. > > Probably you should just use LOG_IF? Done.
https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = On 2016/11/09 14:03:30, Roman Sorokin wrote: > On 2016/11/08 20:23:58, hashimoto wrote: > > Please make proxy_ a member of this class to be consistent with other D-Bus > > client classes. > > Hmm, what is the right way to do that? Since this proxy is specific to > AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? Ah, it seems this method is tricky. Where can I look at the documentation of this API? (or this is an undocumented API which shouldn't be used?) I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me 503. https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:38: writer.AppendBool(false); // No waiting for response. On 2016/11/09 14:03:30, Roman Sorokin wrote: > On 2016/11/08 20:23:58, hashimoto wrote: > > IIUC this parameter changes the timing when the response is sent (i.e. when > > HandleUpstartCallback() is called), so "true" here would let us see the actual > > Start() result in the callback. > > > > Why "false" is preferred? > > I checked both options, but response is null in both cases (if startup failed > for some reason). So not sure what is the advantage of waiting it. IIUC, without waiting, it results in ignoring errors which occurred after the response is sent back. So I thought using "true" here should allow us to be aware of more errors, thus it makes easy to debug by reading log files.
https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = On 2016/11/11 03:46:57, hashimoto wrote: > On 2016/11/09 14:03:30, Roman Sorokin wrote: > > On 2016/11/08 20:23:58, hashimoto wrote: > > > Please make proxy_ a member of this class to be consistent with other D-Bus > > > client classes. > > > > Hmm, what is the right way to do that? Since this proxy is specific to > > AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? > > Ah, it seems this method is tricky. > Where can I look at the documentation of this API? (or this is an undocumented > API which shouldn't be used?) > I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me > 503. FYI, another possible approach is to let the session manager emit an upstart event, and write an upstart conf file to handle the event and start the auth policy service. https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager...
On 2016/11/11 05:30:20, hashimoto wrote: > https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... > File chromeos/dbus/upstart_client.cc (right): > > https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... > chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = > On 2016/11/11 03:46:57, hashimoto wrote: > > On 2016/11/09 14:03:30, Roman Sorokin wrote: > > > On 2016/11/08 20:23:58, hashimoto wrote: > > > > Please make proxy_ a member of this class to be consistent with other > D-Bus > > > > client classes. > > > > > > Hmm, what is the right way to do that? Since this proxy is specific to > > > AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? > > > > Ah, it seems this method is tricky. > > Where can I look at the documentation of this API? (or this is an undocumented > > API which shouldn't be used?) > > I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me > > 503. > I looked here: http://upstart.ubuntu.com/cookbook/#to-start-a-job-via-d-bus > FYI, another possible approach is to let the session manager emit an upstart > event, and write an upstart conf file to handle the event and start the auth > policy service. > https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager... The problem is we need to start it from Chrome anyway (at least for the enrollment case - which would be triggered by custom shortcut)
https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:38: writer.AppendBool(false); // No waiting for response. On 2016/11/11 03:46:58, hashimoto wrote: > On 2016/11/09 14:03:30, Roman Sorokin wrote: > > On 2016/11/08 20:23:58, hashimoto wrote: > > > IIUC this parameter changes the timing when the response is sent (i.e. when > > > HandleUpstartCallback() is called), so "true" here would let us see the > actual > > > Start() result in the callback. > > > > > > Why "false" is preferred? > > > > I checked both options, but response is null in both cases (if startup failed > > for some reason). So not sure what is the advantage of waiting it. > > IIUC, without waiting, it results in ignoring errors which occurred after the > response is sent back. > So I thought using "true" here should allow us to be aware of more errors, thus > it makes easy to debug by reading log files. Done.
https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = On 2016/11/11 03:46:57, hashimoto wrote: > On 2016/11/09 14:03:30, Roman Sorokin wrote: > > On 2016/11/08 20:23:58, hashimoto wrote: > > > Please make proxy_ a member of this class to be consistent with other D-Bus > > > client classes. > > > > Hmm, what is the right way to do that? Since this proxy is specific to > > AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? > > Ah, it seems this method is tricky. > Where can I look at the documentation of this API? (or this is an undocumented > API which shouldn't be used?) > I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me > 503. I looked here: http://upstart.ubuntu.com/cookbook/#to-start-a-job-via-d-bus https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = On 2016/11/11 05:30:20, hashimoto wrote: > On 2016/11/11 03:46:57, hashimoto wrote: > > On 2016/11/09 14:03:30, Roman Sorokin wrote: > > > On 2016/11/08 20:23:58, hashimoto wrote: > > > > Please make proxy_ a member of this class to be consistent with other > D-Bus > > > > client classes. > > > > > > Hmm, what is the right way to do that? Since this proxy is specific to > > > AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? > > > > Ah, it seems this method is tricky. > > Where can I look at the documentation of this API? (or this is an undocumented > > API which shouldn't be used?) > > I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me > > 503. > > FYI, another possible approach is to let the session manager emit an upstart > event, and write an upstart conf file to handle the event and start the auth > policy service. > https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager... The problem is we need to start it from Chrome anyway (at least for the enrollment case - which would be triggered by custom shortcut)
lgtm https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/40001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:31: dbus::ObjectProxy* proxy = On 2016/11/14 13:54:19, Roman Sorokin wrote: > On 2016/11/11 03:46:57, hashimoto wrote: > > On 2016/11/09 14:03:30, Roman Sorokin wrote: > > > On 2016/11/08 20:23:58, hashimoto wrote: > > > > Please make proxy_ a member of this class to be consistent with other > D-Bus > > > > client classes. > > > > > > Hmm, what is the right way to do that? Since this proxy is specific to > > > AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member? > > > > Ah, it seems this method is tricky. > > Where can I look at the documentation of this API? (or this is an undocumented > > API which shouldn't be used?) > > I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me > > 503. > > I looked here: http://upstart.ubuntu.com/cookbook/#to-start-a-job-via-d-bus Thank you for giving me a pointer! https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: No (c) after Copyright. https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.h (right): https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: No (c) after Copyright. (see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...) Sorry for not noticing this in the previous reviews. https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.h:24: // Non-blocking signal to Upstart to start authpolicyd. Using the term "signal" here is confusing as it's not emitting a D-Bus signal, but just calling a method. Maybe just "Starts authpolicyd"?
https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.cc (right): https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/14 22:09:40, hashimoto wrote: > nit: No (c) after Copyright. Done. https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... File chromeos/dbus/upstart_client.h (right): https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/11/14 22:09:41, hashimoto wrote: > nit: No (c) after Copyright. > (see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...) > > Sorry for not noticing this in the previous reviews. Done. https://codereview.chromium.org/2475343002/diff/80001/chromeos/dbus/upstart_c... chromeos/dbus/upstart_client.h:24: // Non-blocking signal to Upstart to start authpolicyd. On 2016/11/14 22:09:41, hashimoto wrote: > Using the term "signal" here is confusing as it's not emitting a D-Bus signal, > but just calling a method. > Maybe just "Starts authpolicyd"? Done.
The CQ bit was checked by rsorokin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2475343002/#ps100001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chromeos/dbus/dbus_clients_browser.cc:
While running git apply --index -p1;
error: patch failed: chromeos/dbus/dbus_clients_browser.cc:18
error: chromeos/dbus/dbus_clients_browser.cc: patch does not apply
Patch: chromeos/dbus/dbus_clients_browser.cc
Index: chromeos/dbus/dbus_clients_browser.cc
diff --git a/chromeos/dbus/dbus_clients_browser.cc
b/chromeos/dbus/dbus_clients_browser.cc
index
c113f88826cd6c77c46aee3bd6cd4e03efd39f18..d3705fd21ab5c6bdb003c7869cedbf61989d734c
100644
--- a/chromeos/dbus/dbus_clients_browser.cc
+++ b/chromeos/dbus/dbus_clients_browser.cc
@@ -18,8 +18,10 @@
#include "chromeos/dbus/fake_easy_unlock_client.h"
#include "chromeos/dbus/fake_image_burner_client.h"
#include "chromeos/dbus/fake_lorgnette_manager_client.h"
+#include "chromeos/dbus/fake_upstart_client.h"
#include "chromeos/dbus/image_burner_client.h"
#include "chromeos/dbus/lorgnette_manager_client.h"
+#include "chromeos/dbus/upstart_client.h"
namespace chromeos {
@@ -57,6 +59,11 @@ DBusClientsBrowser::DBusClientsBrowser(bool use_real_clients)
{
lorgnette_manager_client_.reset(LorgnetteManagerClient::Create());
else
lorgnette_manager_client_.reset(new FakeLorgnetteManagerClient);
+
+ if (use_real_clients)
+ upstart_client_.reset(UpstartClient::Create());
+ else
+ upstart_client_.reset(new FakeUpstartClient);
}
DBusClientsBrowser::~DBusClientsBrowser() {}
@@ -71,6 +78,7 @@ void DBusClientsBrowser::Initialize(dbus::Bus* system_bus) {
easy_unlock_client_->Init(system_bus);
image_burner_client_->Init(system_bus);
lorgnette_manager_client_->Init(system_bus);
+ upstart_client_->Init(system_bus);
}
} // namespace chromeos
The CQ bit was checked by rsorokin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2475343002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add UpstartClient Add StartAuthPolicyService into UpstartClient StartAuthPolicyService would be used by chrome to start authpolicyd service BUG=638663 ========== to ========== Add UpstartClient Add StartAuthPolicyService into UpstartClient StartAuthPolicyService would be used by chrome to start authpolicyd service BUG=638663 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add UpstartClient Add StartAuthPolicyService into UpstartClient StartAuthPolicyService would be used by chrome to start authpolicyd service BUG=638663 ========== to ========== Add UpstartClient Add StartAuthPolicyService into UpstartClient StartAuthPolicyService would be used by chrome to start authpolicyd service BUG=638663 Committed: https://crrev.com/58e0a2f05a6d6c67a7ea4290e7ce8d0380d95e57 Cr-Commit-Position: refs/heads/master@{#432239} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/58e0a2f05a6d6c67a7ea4290e7ce8d0380d95e57 Cr-Commit-Position: refs/heads/master@{#432239} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
