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 <[email protected]> wrote:
> Rocky Bernstein wrote:
>
> > On Mon, Feb 13, 2012 at 4:19 AM, Jim Meyering <[email protected]> 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
+}
