Attached should be a revised patch that addresses all of your suggestions. Cheers, rocky
On Tue, Feb 14, 2012 at 5:49 AM, Jim Meyering <j...@meyering.net> wrote: > Rocky Bernstein wrote: > > > On Mon, Feb 13, 2012 at 4:19 AM, Jim Meyering <j...@meyering.net> wrote: > > > > Rocky Bernstein wrote: > > > It's been a couple of months since I first sent this was sent > without nary > > > an ack. Comments? > > > > Hi Rocky, > > > > su is barely on life support in coreutils. > > Meaning that it's no longer really maintained. > > We stopped installing it (by default) back in the 2007-2008 > > time frame (6.9.90, 7.0). We nearly removed it altogether. > > > > Do you build/install coreutils yourself, or use it via a > distribution? > > If the latter, which distribution? > > > > CenOS 6.2 which seems to use coreutils 8.4 > > > > I don't remember looking carefully at your patch before, > > but glanced through it just now. Here are some suggestions > > if you'd like to pursue it: > > > > - use "error (0, 0, ...", not fprintf ( > > - indent with spaces, not TABs > > - use gnu indentation/formatting style (esp. wrt braces) > > - mention the change in NEWS > > - for a behavior change like this, we would like a test case > > > > The above seems all reasonable and easily doable. But are you suggesting > is even > > after getting this into the next coreutils, no OS is likely to use it? > > > > The basic idea is that a colleague and I were confused by the fact that > -l and > > -p conflict although this was not apparent in the documentation. I wound > up > > downloading the source code to understand fully. > > > > So something even as simple as mentioning this in doc/coreutils.texi > would have > > been helpful (which is the first part of that patch). > > > > Going further I realized that it is probably erroneous to supply both -l > -p and > > that can be easily warned. > > > > But if there is a better place such to report such problems or get code > > improved, let me know! Thanks. > > I see that Fedora still uses su from coreutils, too, > so this is a worthwhile change. >
diff --git a/NEWS b/NEWS index 51c44c7..08a7a8d 100644 --- a/NEWS +++ b/NEWS @@ -32,6 +32,9 @@ GNU coreutils NEWS -*- outline -*- [you might say this was introduced in coreutils-7.5, along with inotify support, but the GPFS magic number wasn't in the usual places then.] +** Changes in behavior + + su -l -p gives a warning that these options are incompatible. * Noteworthy changes in release 8.14 (2011-10-12) [stable] diff --git a/doc/coreutils.texi b/doc/coreutils.texi index c26a53d..d874519 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -15636,6 +15636,8 @@ environment variables except @env{TERM}, @env{HOME}, and @env{SHELL} directory. Prepend @samp{-} to the shell's name, intended to make it read its login startup file(s). +Note that this option and the next one @option{-m} are mutually exclusive. + @item -m @itemx -p @itemx --preserve-environment @@ -15654,6 +15656,8 @@ is not listed in the file @file{/etc/shells}, or in a compiled-in list if that file does not exist. Parts of what this option does can be overridden by @option{--login} and @option{--shell}. +Note that this option and the previous one @option{-l} are mutually exclusive. + @item -s @var{shell} @itemx --shell=@var{shell} @opindex -s diff --git a/src/su.c b/src/su.c index b1ba2a7..5daf1c4 100644 --- a/src/su.c +++ b/src/su.c @@ -400,6 +400,7 @@ main (int argc, char **argv) char *shell = NULL; struct passwd *pw; struct passwd pw_copy; + static bool seen_login_change_env = false ; initialize_main (&argc, &argv); set_program_name (argv[0]); @@ -427,12 +428,32 @@ main (int argc, char **argv) break; case 'l': - simulate_login = true; + if (seen_login_change_env && !simulate_login) + { + error (0, 0, + _("%s: already seen option --preserve-environment; --login ignored.\n"), + program_name); + } + else + { + simulate_login = true; + seen_login_change_env = true; + } break; case 'm': case 'p': - change_environment = false; + if (seen_login_change_env && change_environment) + { + error (0, 0, + _("%s: already seen option --login; --preserve-environment ignored.\n"), + program_name); + } + else + { + change_environment = false; + seen_login_change_env = true; + } break; case 's': diff --git a/tests/Makefile.am b/tests/Makefile.am index 23cb70f..09f1e31 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -500,6 +500,7 @@ TESTS = \ rmdir/fail-perm \ rmdir/ignore \ rmdir/t-slash \ + su/p-and-l \ tail-2/assert-2 \ tail-2/big-4gb \ tail-2/flush-initial \ diff --git a/tests/su/p-and-l b/tests/su/p-and-l new file mode 100644 index 0000000..37fae6f --- /dev/null +++ b/tests/su/p-and-l @@ -0,0 +1,74 @@ +#!/usr/bin/perl +# Test whether options -p and -l give a warning. + +# Copyright (C) 2012 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +## The following may be helpful in debugging +# use Enbugger; Enbugger->load_debugger('trepan'); + +use strict; use warnings; +(my $ME = $0) =~ s|.*/||; + +# Some older versions of Expect.pm (e.g. 1.07) lack the log_user method, +# so check for that, too. +eval { require Expect; Expect->require_version('1.11') }; +$@ + and CuSkip::skip "$ME: this script requires Perl's Expect package >=1.11\n"; + +{ + my $fail = 0; + my @tests = ( + ["su -l -p bogus", + qr/already seen option --login/], + ["su -p -l bogus", + qr/already seen option --preserve/], + ); + foreach my $ary (@tests) + { + my ($cmd, $expect) = @$ary; + my $exp = new Expect; + $exp->log_user(0); + $exp->spawn("$cmd") + or (warn "$ME: cannot run `$cmd': $!\n"), $fail=1, next; + my $spawn_ok; + my @found = $exp->expect(1, 'Password: '); + unless ($found[3] =~ $expect) { + warn "$ME: $cmd did not get expected warning: $expect\n"; + $fail = 1; + } + $exp->hard_close(); + } + @tests = ( + "su -l bogus", + "su -p bogus", + "su bogus", + ); + foreach my $cmd (@tests) + { + my $exp = new Expect; + $exp->log_user(0); + $exp->spawn("$cmd") + or (warn "$ME: cannot run `$cmd': $!\n"), $fail=1, next; + my $spawn_ok; + my @found = $exp->expect(1, 'Password: '); + if ($found[3] =~ qr/already seen option/) { + warn "$ME: $cmd should not get already-seen option warning\n"; + $fail = 1; + } + $exp->hard_close(); + } + exit $fail +}