Code review comment for lp:~csurbhi/ubuntu/natty/mountall/mountall-stop-timer

+ <allow send_destination="com.ubuntu.Mountall.Server"
+ send_interface="com.ubuntu.Mountall0_1.Server"
+ send_type="method_call" send_member="StopTimer" />
+ <allow send_destination="com.ubuntu.Mountall.Server"
+ send_interface="com.ubuntu.Mountall0_1.Server"
+ send_type="method_call" send_member="RestartTimers" />
+
+ <allow send_destination="com.ubuntu.Mountall.Server"
+ send_interface="com.ubuntu.Mountall0_1.Server.Job"
+ send_type="method_call" send_member="ChangeMountDevice" />

These methods should be root-only; just removing them from <policy context="default"> should be good enough.

+ <allow send_destination="com.ubuntu.Mountall.Server"
+ send_interface="com.ubuntu.Mountall0_1.Server.Job"

Remove ".Job".

+ <method name="RestartTimer">
+ <arg name="devname" type="s" direction="in" />
+ </method>

This is called RestartTimers in the .conf file. Which is correct?

Also, the .conf file has an entry for GetMountDevices, but that is missing from the .xml file.

- --package=$(PACAKGE) \
+ --package=$(PACKAGE) \

Thanks for fixing this, but there's another (probably copied-and-pasted) occurrence of $(PACAKGE) below. Could you fix that too?

=== added file 'src/control.c'
--- src/control.c 1970-01-01 00:00:00 +0000
+++ src/control.c 2011-06-30 18:09:32 +0000
@@ -0,0 +1,253 @@
+/* mountall
+ *
+ * Copyright © 2010 Canonical Ltd.
+ * Author: Surbhi A. Palande <email address hidden>

This is substantially copied from Upstart. That's fine - they're both copyright Canonical with the same licence, but it'd be good form to acknowledge Scott's co-authorship.

Otherwise I'm happy with this on visual inspection.