[Merge] lp:~ogra/phablet-tools/phablet-tools-phablet-config-changes into lp:phablet-tools

[Martin Pitt]

I have some inline code comments which need to be addressed.

However, even with a fixed is_remote_root() this doesn't seem to work with the non-root adb, I still get AppArmor "DENIED" errors.

Diff comments:

> === modified file 'phablet-config'
> --- phablet-config	2014-06-19 15:10:15 +0000
> +++ phablet-config	2014-08-21 12:55:47 +0000
> @@ -100,23 +100,44 @@
>          sys.exit(1)
>  
>  
> +def is_remote_root(adb):
> +    if adb.shell("id -r -u"):
> +        return 0
> +    return 1
> +

Returning ints for boolean functions is very un-Pythonic. Also, this logic doesn't work as the shell() command always returns a non-empty string ("0" or "1000"), which always counts as True. Thus this function always returns False. I suggest

    return adb.shell("id -ru").strip() == "0"

which is more explicit, clearer, and returns a proper bool. But see below, maybe you don't need it at all.

> +
>  def _handle_autopilot(adb, args):
> -    if args.dbus_probe == 'enable':
> -        rfile = '/usr/share/autopilot-touch/apparmor/click.rules'
> -        adb.shell('aa-clickhook -f --include=%s' % rfile)
> +    if not is_remote_root(adb):
> +        dbusarg = 'false'
> +        if args.dbus_probe == 'enable':
> +            dbusarg = 'true'
> +        dbus_call = 'dbus-send --system --print-reply --dest=com.canonical.PropertyService ' \
> +                    '/com/canonical/PropertyService com.canonical.PropertyService.SetProperty ' \
> +                    'string:autopilot boolean:%s' % dbusarg
> +        adb.shell(dbus_call, False)

Is there any reason why this can't just always use the D-BUS call, even as root? Having two different ways of doing this is harder to test and maintain.

>      else:
> -        adb.shell('aa-clickhook -f')
> +        if args.dbus_probe == 'enable':
> +            rfile = '/usr/share/autopilot-touch/apparmor/click.rules'
> +            adb.shell('aa-clickhook -f --include=%s' % rfile)
> +        else:
> +            adb.shell('aa-clickhook -f')
>  
>  
>  def _handle_writable_image(adb, args):
> -    fname = '/userdata/.writable_image'
> -    try:
> -        adb.shell('test -e %s' % fname, False)
> -    except CalledProcessError as e:
> -        adb.shell('touch %s' % fname, False)
> -        adb.reboot()
> -        adb.wait_for_device()
> -        adb.wait_for_network()
> +    if not is_remote_root(adb):
> +        dbus_call = 'dbus-send --system --print-reply --dest=com.canonical.PropertyService ' \
> +                    '/com/canonical/PropertyService com.canonical.PropertyService.SetProperty ' \
> +                    'string:writable boolean:true'
> +        adb.shell(dbus_call, False)

Likewise, this should also work as root?

> +    else:
> +        fname = '/userdata/.writable_image'
> +        try:
> +            adb.shell('test -e %s' % fname, False)
> +        except CalledProcessError as e:
> +            adb.shell('touch %s' % fname, False)
> +            adb.reboot()
> +            adb.wait_for_device()
> +            adb.wait_for_network()
>  
>      try:
>          if args.package_dir:
> 


-- 
https://code.launchpad.net/~ogra/phablet-tools/phablet-tools-phablet-config-changes/+merge/231711
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~ogra/phablet-tools/phablet-tools-phablet-config-changes into lp:phablet-tools.