Code review comment for lp:~brendan-donegan/checkbox/bug1066094

On 16/10/12 14:52, Marc Tardif wrote:
>> - output = check_output(['/sbin/ifconfig', '-s', '-a'])
>> + output = check_output(['ifconfig', '-s', '-a'])
> Thank you for changing the path to ifconfig and the other commands from
> absolute to just their names!
You're welcome :)
>
>> - for interface in self.interfaces:
>> + for iface in self.interfaces:
> What was wrong with the name "interface"?
Not a lot, just that it avoids having to split certain statements across
multiple lines. I reckoned iface is a fairly common contraction of
interface so it shouldn't be confusing
>
>> try:
>> - check_output(['/sbin/ifconfig', interface, 'up'])
>> + check_output(['ifconfig', iface, 'up'])
>> except CalledProcessError:
> This will still output the stderr of ifconfig to the terminal. The
> exception raised by the check_output function only has one output
> attribute, so you need to pipe stderr into stdout:
This is an existing problem with the script as far as I know, but it
would be ok to fix it now.
>
> check_output(
> ['/sbin/ifconfig', interface, 'up'],
> stderr=subprocess.STDOUT)
>
>> - logging.error('Unable to bring up interface {0!r}'
>> - .format(interface))
>> + message = 'Unable to bring up interface {0!r}'.format(iface)
>> +
>> + if os.getuid():
>> + message += self.SUPERUSER_MSG
>> +
>> + message += '.'
>> +
>> + print(message, file=sys.stderr)
>> + logging.error(message)
>> raise
> How do you know that just because check_output raised a CalledProcessError
> that it was because of missing superuser privileges? Here's another error
> that has nothing to do with privileges:
>
> $ ifconfig eth8 up
> eth8: ERROR while getting interface flags: No such device
>
> So, once you piped stderr into stdout, you should use the output variable
> in the exception to provide a useful and accurate error message to the
> user:
>
> message = "Unable to bring up interface {0!r}:\n".format(interface)
> message += exception.output.decode("utf-8")
> print(message, file=sys.stderr)
> logging.error(message)
> raise
Sounds like a fine method, I'll give it a try
>
> the same applies for the other calls that assumer that any error is because
> of missing superuser privileges.
>
>> - default='/var/log',
>> + default='.',
>> help='The path to the log directory. \
>> Default is /var/log')
> The problem here is that the default kwarg was changed but the help wasn't
> changed accordingly. The solution would be to use %(default)s in the help
> instead of hard coding a duplicate value.
I fixed this specific issue but in principle where defaults are
mentioned in help text that convention should be used, so I'll change it.
>