AU: Beginnings of dbus support.

AU: Beginnings of dbus support.

The AU will be a daemon that runs as root. Non-root will communicate
via dbus with the updater to do things such as: query status, request
forced or full updates, etc.

New files for dbus:

UpdateEngine.conf - security configuration
dbus_constants.h - common constants
dbus_service.* - The object exposed over dbus
org.chromium.UpdateEngine.service - the dbus service file
udpate_attempter.* - Refactored this out of main.cc
update_engine_client.cc - Simple command line utility to interact with
Update Engine over dbus. Whereas Update Engine runs as root, this tool
runs as non-root user.

[Daniel Erat, 2010-04-24 01:25:53]

The code looks okay to me, but you should get someone who knows more about D-Bus
to take a look.

http://codereview.chromium.org/1733013/diff/1/2
File src/platform/update_engine/SConstruct (right):

http://codereview.chromium.org/1733013/diff/1/2#newcode42
src/platform/update_engine/SConstruct:42: #dbus-binding-tool --mode=glib-client
--prefix=update_engine_service update_engine.xml >update_engine_dbus_bindings.h
delete these?

http://codereview.chromium.org/1733013/diff/1/2#newcode48
src/platform/update_engine/SConstruct:48: source: array containing the source
.xml file
s/array/list/

http://codereview.chromium.org/1733013/diff/1/2#newcode101
src/platform/update_engine/SConstruct:101: # -Wclobbered
delete these?

http://codereview.chromium.org/1733013/diff/1/4
File src/platform/update_engine/dbus_constants.h (right):

http://codereview.chromium.org/1733013/diff/1/4#newcode8
src/platform/update_engine/dbus_constants.h:8: static const char* const
kUpdateEngineServiceName = "org.chromium.UpdateEngine";
can these go inside of a namespace?

http://codereview.chromium.org/1733013/diff/1/5
File src/platform/update_engine/dbus_service.cc (right):

http://codereview.chromium.org/1733013/diff/1/5#newcode7
src/platform/update_engine/dbus_service.cc:7: #include
"chromeos/obsolete_logging.h"
remove this in favor of base/logging.h

http://codereview.chromium.org/1733013/diff/1/5#newcode14
src/platform/update_engine/dbus_service.cc:14: LOG(INFO) <<
"update_engine_service_finalize()";
DLOG(INFO) would probably be better for these, or just delete them if you no
longer need them

http://codereview.chromium.org/1733013/diff/1/7
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/1733013/diff/1/7#newcode59
src/platform/update_engine/main.cc:59: LOG(FATAL) << "Got retult code " <<
request_name_ret << " from requesting "
s/retult/result/

http://codereview.chromium.org/1733013/diff/1/7#newcode60
src/platform/update_engine/main.cc:60: "name, but expected " <<
DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER;
line should start with "<<"

http://codereview.chromium.org/1733013/diff/1/7#newcode78
src/platform/update_engine/main.cc:78: // logging::InitLogging("logfile.txt",
what's the reason for commenting this out?

http://codereview.chromium.org/1733013/diff/1/9
File src/platform/update_engine/update_attempter.cc (right):

http://codereview.chromium.org/1733013/diff/1/9#newcode41
src/platform/update_engine/update_attempter.cc:41: // shared_ptr<InstallAction>
install_action(  // re-add
add TODO for this and other commented-out lines

http://codereview.chromium.org/1733013/diff/1/9#newcode104
src/platform/update_engine/update_attempter.cc:104: // TODO(adlr): implement
this method.
NOTIMPLEMENTED() is a handy macro from logging.h to use in situations like
these; it LOG(ERROR)s when the code is called

http://codereview.chromium.org/1733013/diff/1/10
File src/platform/update_engine/update_attempter.h (right):

http://codereview.chromium.org/1733013/diff/1/10#newcode19
src/platform/update_engine/update_attempter.h:19: UpdateAttempter(GMainLoop
*loop)
explicit

http://codereview.chromium.org/1733013/diff/1/12
File src/platform/update_engine/update_engine_client.cc (right):

http://codereview.chromium.org/1733013/diff/1/12#newcode18
src/platform/update_engine/update_engine_client.cc:18: "Force an update, even
over an expensive network.");
indent to line up with opening paren

http://codereview.chromium.org/1733013/diff/1/12#newcode24
src/platform/update_engine/update_engine_client.cc:24: const char*
ErrorMessage(const GError* error) {
GetErrorMessage() or DecodeError() or something would be a more descriptive name

http://codereview.chromium.org/1733013/diff/1/12#newcode101
src/platform/update_engine/update_engine_client.cc:101: if
(!CheckForUpdates(FLAGS_force_update)) {
just use CHECK(CheckForUpdates...)?

[adlr, 2010-04-26 18:27:09]

Dan, please take another look. If you're happy, I'll reach out to another
developer to look at the dbus parts.

Thanks,

http://codereview.chromium.org/1733013/diff/1/2
File src/platform/update_engine/SConstruct (right):

http://codereview.chromium.org/1733013/diff/1/2#newcode42
src/platform/update_engine/SConstruct:42: #dbus-binding-tool --mode=glib-client
--prefix=update_engine_service update_engine.xml >update_engine_dbus_bindings.h
On 2010/04/24 01:25:53, Daniel Erat wrote:
> delete these?

Done.

http://codereview.chromium.org/1733013/diff/1/2#newcode48
src/platform/update_engine/SConstruct:48: source: array containing the source
.xml file
On 2010/04/24 01:25:53, Daniel Erat wrote:
> s/array/list/

Done.

http://codereview.chromium.org/1733013/diff/1/2#newcode101
src/platform/update_engine/SConstruct:101: # -Wclobbered
On 2010/04/24 01:25:53, Daniel Erat wrote:
> delete these?

Put them back in. I can't recall the reason I took them out.

http://codereview.chromium.org/1733013/diff/1/4
File src/platform/update_engine/dbus_constants.h (right):

http://codereview.chromium.org/1733013/diff/1/4#newcode8
src/platform/update_engine/dbus_constants.h:8: static const char* const
kUpdateEngineServiceName = "org.chromium.UpdateEngine";
On 2010/04/24 01:25:53, Daniel Erat wrote:
> can these go inside of a namespace?

Done.

http://codereview.chromium.org/1733013/diff/1/5
File src/platform/update_engine/dbus_service.cc (right):

http://codereview.chromium.org/1733013/diff/1/5#newcode7
src/platform/update_engine/dbus_service.cc:7: #include
"chromeos/obsolete_logging.h"
On 2010/04/24 01:25:53, Daniel Erat wrote:
> remove this in favor of base/logging.h

Done.

http://codereview.chromium.org/1733013/diff/1/5#newcode14
src/platform/update_engine/dbus_service.cc:14: LOG(INFO) <<
"update_engine_service_finalize()";
On 2010/04/24 01:25:53, Daniel Erat wrote:
> DLOG(INFO) would probably be better for these, or just delete them if you no
> longer need them

Done.

http://codereview.chromium.org/1733013/diff/1/7
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/1733013/diff/1/7#newcode59
src/platform/update_engine/main.cc:59: LOG(FATAL) << "Got retult code " <<
request_name_ret << " from requesting "
On 2010/04/24 01:25:53, Daniel Erat wrote:
> s/retult/result/

Done.

http://codereview.chromium.org/1733013/diff/1/7#newcode60
src/platform/update_engine/main.cc:60: "name, but expected " <<
DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER;
On 2010/04/24 01:25:53, Daniel Erat wrote:
> line should start with "<<"

Done.

http://codereview.chromium.org/1733013/diff/1/7#newcode78
src/platform/update_engine/main.cc:78: // logging::InitLogging("logfile.txt",
On 2010/04/24 01:25:53, Daniel Erat wrote:
> what's the reason for commenting this out?

It was crashing, but I figured out why and fixed it.

http://codereview.chromium.org/1733013/diff/1/9
File src/platform/update_engine/update_attempter.cc (right):

http://codereview.chromium.org/1733013/diff/1/9#newcode41
src/platform/update_engine/update_attempter.cc:41: // shared_ptr<InstallAction>
install_action(  // re-add
On 2010/04/24 01:25:53, Daniel Erat wrote:
> add TODO for this and other commented-out lines

Done.

http://codereview.chromium.org/1733013/diff/1/9#newcode104
src/platform/update_engine/update_attempter.cc:104: // TODO(adlr): implement
this method.
On 2010/04/24 01:25:53, Daniel Erat wrote:
> NOTIMPLEMENTED() is a handy macro from logging.h to use in situations like
> these; it LOG(ERROR)s when the code is called

Done.

http://codereview.chromium.org/1733013/diff/1/10
File src/platform/update_engine/update_attempter.h (right):

http://codereview.chromium.org/1733013/diff/1/10#newcode19
src/platform/update_engine/update_attempter.h:19: UpdateAttempter(GMainLoop
*loop)
On 2010/04/24 01:25:53, Daniel Erat wrote:
> explicit

Done.

http://codereview.chromium.org/1733013/diff/1/12
File src/platform/update_engine/update_engine_client.cc (right):

http://codereview.chromium.org/1733013/diff/1/12#newcode18
src/platform/update_engine/update_engine_client.cc:18: "Force an update, even
over an expensive network.");
On 2010/04/24 01:25:53, Daniel Erat wrote:
> indent to line up with opening paren

Done.

http://codereview.chromium.org/1733013/diff/1/12#newcode24
src/platform/update_engine/update_engine_client.cc:24: const char*
ErrorMessage(const GError* error) {
On 2010/04/24 01:25:53, Daniel Erat wrote:
> GetErrorMessage() or DecodeError() or something would be a more descriptive
name

Done.

http://codereview.chromium.org/1733013/diff/1/12#newcode101
src/platform/update_engine/update_engine_client.cc:101: if
(!CheckForUpdates(FLAGS_force_update)) {
On 2010/04/24 01:25:53, Daniel Erat wrote:
> just use CHECK(CheckForUpdates...)?

Hm, I guess this could go either way. Usually I understand CHECK failures to
mean that an assumption of the programmer has been violated, whereas LOG(FATAL)
means more that the program will terminate with a message, but more along the
lines that this is somewhat sanctioned by the programmer. What do you think?

[Daniel Erat, 2010-04-26 18:40:53]

LGTM

http://codereview.chromium.org/1733013/diff/1/12
File src/platform/update_engine/update_engine_client.cc (right):

http://codereview.chromium.org/1733013/diff/1/12#newcode101
src/platform/update_engine/update_engine_client.cc:101: if
(!CheckForUpdates(FLAGS_force_update)) {
On 2010/04/26 18:27:10, adlr wrote:
> On 2010/04/24 01:25:53, Daniel Erat wrote:
> > just use CHECK(CheckForUpdates...)?
> 
> Hm, I guess this could go either way. Usually I understand CHECK failures to
> mean that an assumption of the programmer has been violated, whereas
LOG(FATAL)
> means more that the program will terminate with a message, but more along the
> lines that this is somewhat sanctioned by the programmer. What do you think?

Not a huge deal, but I think of them as both having the same purpose and prefer
CHECK() since it saves at least one line and produces a more-precise message
about the problem.

[adlr, 2010-04-26 19:20:51]

Will, can you take a quick look at the dbus specific parts of this? derat alread
LGTMed the rest.

Thanks very much,
-andrew

[Will Drewry, 2010-04-26 20:16:32]

So this lgtm but does have a good bit of code duplication given the dbus code in
common (chromeos::dbus).  It'd be nice to encapsulate some of the C/glib stuff
away using that or the coming dbus-c++ stuff for cromo.

I was also curious how this fits in with the engine itself.  Will this daemon
just pick up local status and do calls to the network or will it actually touch
the updater process.  (We can discuss it out of cl too.)  I just want to make
sure I understand what it looks like in a more complete way.

thanks!
will

http://codereview.chromium.org/1733013/diff/24001/25006
File src/platform/update_engine/main.cc (right):

http://codereview.chromium.org/1733013/diff/24001/25006#newcode37
src/platform/update_engine/main.cc:37: void
SetupDbusService(UpdateEngineService* service) {
FWIW, some of the other services (cryptohomed session_manager) are using
AbstractDbusService which provides the code below.

[rochberg and co are also working on stripping exceptions out of dbus-c++ so
that we can use it in a style-compliant way.  You might want to check in on them
later when you add more services.]

http://codereview.chromium.org/1733013/diff/24001/25006#newcode77
src/platform/update_engine/main.cc:77: g_type_init();
Should be ::g_type_init();

http://codereview.chromium.org/1733013/diff/24001/25006#newcode79
src/platform/update_engine/main.cc:79: dbus_g_thread_init();
I guess this is all threaded with glib-friendly threads?

http://codereview.chromium.org/1733013/diff/24001/25006#newcode98
src/platform/update_engine/main.cc:98:
dbus_g_object_type_install_info(UPDATE_ENGINE_TYPE_SERVICE,
All this stuff is also handled in chromeos::dbus::AbstractDbusService except for
g_type_init().  Did it not make sense to use an UpdateEngineService class to
wrap this.  (E.g., was GMainLoop encapsulation not working out, etc)

http://codereview.chromium.org/1733013/diff/24001/25006#newcode106
src/platform/update_engine/main.cc:106: g_timeout_add(0,
&chromeos_update_engine::SetupInMainLoop, &update_attempter);
If the whole thingt was wrapped up in a class, you could probably just call this
once the loop is up.  I guess it depends on if there is some lag.

http://codereview.chromium.org/1733013/diff/24001/25007
File src/platform/update_engine/org.chromium.UpdateEngine.service (right):

http://codereview.chromium.org/1733013/diff/24001/25007#newcode3
src/platform/update_engine/org.chromium.UpdateEngine.service:3:
Exec=/home/adlr/trunk/src/platform/update_engine/update_engine
Update with the permanent home? :)

http://codereview.chromium.org/1733013/diff/24001/25011
File src/platform/update_engine/update_engine_client.cc (right):

http://codereview.chromium.org/1733013/diff/24001/25011#newcode5
src/platform/update_engine/update_engine_client.cc:5: #include <gflags/gflags.h>
Are we adding more deps on gflags or moving over to libbase's commandline.h?

http://codereview.chromium.org/1733013/diff/24001/25011#newcode6
src/platform/update_engine/update_engine_client.cc:6: #include <glib.h>
Since this is C, it should probably come before gflags.

http://codereview.chromium.org/1733013/diff/24001/25011#newcode43
src/platform/update_engine/update_engine_client.cc:43: proxy =
dbus_g_proxy_new_for_name_owner(bus,
there's code in common for this too (seanparent's chromeos::dbus::Proxy  and
chromeos::dbus::BusConnection)