Merge lp:~lool/linaro-image-tools/optional-sudo into lp:linaro-image-tools/11.11

[Loïc Minier]

[James Westby (james-w)]

Hi,

I like the change, but it would be good to have a test if possible.

Thanks,

James

review: Approve

[lp:~lool/linaro-image-tools/optional-sudo]

276. By Loïc Minier on 2011-01-29

Mock getuid() when testing cmd_runner and test both the sudo needed and sudo
not needed cases.

[Loïc Minier (lool)]

I update the cmd_runner tests in r276 now; I realize that we ought to change the whole testsuite to assume "regular user" (getuid() != 0), but changing the Popen mock class to mock os.getuid() required changing the MockSomething fixture in non-trivial ways, so I'd like to consult with salgado on doing this nicely.

We could either change cmd_runner to take a force_sudo/never_sudo flag if that's useful, or we could have a new type of mock class which allows mocking many things; in fact, I had started such a class a while ago, but it's not trivial to make something generic and elegant, notably distinguishing between args you want to mock as non-existent (delattr()) versus args you want to mock to None (setattr(None)). We'd need a slightly more complex interface that I'm comfortable to add without him.

[Guilherme Salgado (salgado)]

It's not clear to me why we ought to change the whole testsuite to assume "regular user". Is it because when building a package the tests will be executed as root and that would now cause the ones that use cmd_runner.Popen() to fail?

[Loïc Minier (lool)]

When building a package, the tests will be run as a regular user; perhaps under fakeroot, I didn't check.

But I think the uid of the calling user shouldn't matter, we should build the testsuite to work whatever the uid. I'm proposing we change the testsuite to setup things so that it looks like a regular user is running it so that we get the chance to include sudo in the expect commands; that way, we include the "this piece needs to be run as root" information in all the Popen tests.

So I'm suggesting that whatever the calling user, we should divert getuid() to return non-zero, and we should expect commands to look like:
[sudo command-requiring-root,
command-not-requiring-root]
etc.

[Loïc Minier (lool)]

I just found a stronger argument for avoiding sudo: this allows strace-ing the whole of linaro-media-create: first become root, then strace. strace doesn't work across sudo because it's setuid.

=== modified file 'linaro-hwpack-install'
--- linaro-hwpack-install	2011-01-28 19:50:48 +0000
+++ linaro-hwpack-install	2011-01-29 19:21:18 +0000
@@ -38,6 +38,11 @@
 APT_GET_OPTIONS="Dir::Etc::SourceList=${SOURCES_LIST_FILE}"
 SUPPORTED_FORMATS="1.0"  # A space-separated list of hwpack formats.
 
+sudo="sudo"
+if [ $(id -u) -eq 0 ]; then
+    sudo=""
+fi
+
 die() {
   echo -e "$@"
   exit 1
@@ -79,7 +84,7 @@
   # Ensure our temp dir and apt sources are removed.
   echo -n "Cleaning up ..."
   rm -rf $TEMP_DIR
-  sudo apt-get update -qq
+  $sudo apt-get update -qq
   echo "Done"
 }
 
@@ -128,14 +133,14 @@
   done < $stripped_file
 
   if [ $should_install -eq 1 ]; then
-    sudo cp $file /etc/apt/sources.list.d/hwpack.$filename
+    $sudo cp $file /etc/apt/sources.list.d/hwpack.$filename
   fi
 done
 
 # Import the OpenPGP keys for the files installed above.
 for filename in $(ls "${HWPACK_DIR}"/sources.list.d.gpg/); do
   file="${HWPACK_DIR}"/sources.list.d.gpg/$filename
-  sudo apt-key add $file
+  $sudo apt-key add $file
 done
 
 # Add one extra apt source for the packages included in the hwpack and make
@@ -145,7 +150,7 @@
 cat /etc/apt/sources.list >> "$SOURCES_LIST_FILE"
 
 echo "Updating apt package lists ..."
-sudo apt-get -o "$APT_GET_OPTIONS" update -q
+$sudo apt-get -o "$APT_GET_OPTIONS" update -q
 
 echo -n "Installing packages ..."
 
@@ -199,11 +204,11 @@
   done
 fi
 
-sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" install ${packages}
+$sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" install ${packages}
 
 if [ "$DEP_PACKAGE_PRESENT" == "yes" ]; then
   if [ -n "${to_be_installed}" ]; then
-    sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" markauto ${to_be_installed}
+    $sudo apt-get $FORCE_OPTIONS -o "$APT_GET_OPTIONS" markauto ${to_be_installed}
   fi
 fi
 

=== modified file 'linaro-media-create'
--- linaro-media-create	2011-01-28 19:50:48 +0000
+++ linaro-media-create	2011-01-29 19:21:18 +0000
@@ -22,7 +22,6 @@
 import os
 import sys
 import tempfile
-from subprocess import Popen
 
 from linaro_media_create.boards import (
     board_configs,
@@ -42,7 +41,10 @@
     ensure_command,
     is_arm_host,
     )
-from linaro_media_create import get_args_parser
+from linaro_media_create import (
+    cmd_runner,
+    get_args_parser
+    )
 
 # Just define the global variables
 TMP_DIR = None
@@ -60,18 +62,20 @@
     Before doing so, make sure BOOT_DISK and ROOT_DISK are not mounted.
     """
     devnull = open('/dev/null', 'w')
-    # Use raw subprocess.Popen as we don't want to stop in case the
-    # commands end with a non-zero return code.
-    if BOOT_DISK is not None:
-        Popen(['sudo', 'umount', BOOT_DISK],
-              stdout=devnull, stderr=devnull).wait()
-    if ROOT_DISK is not None:
-        Popen(['sudo', 'umount', ROOT_DISK],
-              stdout=devnull, stderr=devnull).wait()
+    # ignore non-zero return codes
+    try:
+        if BOOT_DISK is not None:
+            cmd_runner.Popen(['umount', BOOT_DISK],
+                  stdout=devnull, stderr=devnull, as_root=True).wait()
+        if ROOT_DISK is not None:
+            cmd_runner.Popen(['umount', ROOT_DISK],
+                  stdout=devnull, stderr=devnull, as_root=True).wait()
+    except SubcommandNonZeroReturnValue:
+        pass
     # Remove TMP_DIR as root because some files written there are
     # owned by root.
     if TMP_DIR is not None:
-        Popen(['sudo', 'rm', '-rf', TMP_DIR]).wait()
+        cmd_runner.Popen(['rm', '-rf', TMP_DIR], as_root=True).wait()
 
 
 def ensure_required_commands(args):

=== modified file 'linaro_media_create/cmd_runner.py'
--- linaro_media_create/cmd_runner.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/cmd_runner.py	2011-01-29 19:21:18 +0000
@@ -37,10 +37,10 @@
     """
     assert isinstance(args, (list, tuple)), (
         "The command to run must be a list or tuple, found: %s" % type(args))
-    # TODO: We might want to always use 'sudo -E' here to avoid problems like
-    # https://launchpad.net/bugs/673570
-    if as_root:
+    if as_root and os.getuid() != 0:
         args = args[:]
+        # TODO: We might want to always use 'sudo -E' here to avoid problems
+        # like https://launchpad.net/bugs/673570
         args.insert(0, 'sudo')
     return Popen(args, stdin=stdin, stdout=stdout, stderr=stderr)
 

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-01-29 19:21:18 +0000
@@ -412,11 +412,18 @@
         self.assertEqual(0, proc.returncode)
         self.assertEqual([['foo', 'bar', 'baz']], fixture.mock.calls)
 
-    def test_run_as_root(self):
+    def test_run_as_root_with_sudo(self):
         fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        self.useFixture(MockSomethingFixture(os, 'getuid', lambda: 1000))
         cmd_runner.run(['foo', 'bar'], as_root=True).wait()
         self.assertEqual([['sudo', 'foo', 'bar']], fixture.mock.calls)
 
+    def test_run_as_root_as_root(self):
+        fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        self.useFixture(MockSomethingFixture(os, 'getuid', lambda: 0))
+        cmd_runner.run(['foo', 'bar'], as_root=True).wait()
+        self.assertEqual([['foo', 'bar']], fixture.mock.calls)
+
     def test_run_succeeds_on_zero_return_code(self):
         proc = cmd_runner.run(['true'])
         # Need to wait() here as we're using the real Popen.