Hello,
While writing a few tests for userspec (below), I was surprised to
re-learn that chown USER_NAME: has a special meaning. It is a
shorthand for chown USER_NAME:+$(id -g USER_NAME) ...
I had expected it to be equivalent to this:
chown USER_NAME ...
Since the above behavior is not specified by POSIX, and
is IMHO, counter-intuitive, I propose to change it. However,
it is documented both in coreutils and in cpio's manuals.
Here's the patch, followed by the coreutils part,
but I suppose I can't really apply them as-is.
Rather, I may make "chown USER_NAME: ..." warn-for-now yet
continue to work and "chown NUMERIC: ..." work like "chown NUMERIC ...".
Then, in say two years, I'd make the actual change.
Sergio, would you accept a similar preparatory patch for cpio?
Opinions?
BTW, just realized that changing userspec semantics like this
would require a NEWS update, too... But we're not there yet.
Also, regardless, I now expect to adjust this new test
module to test the current behavior.
>From b86a5bd9960c33b753f7963ceb1502a34255f3f4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Wed, 2 Dec 2009 20:01:36 +0100
Subject: [PATCH 1/2] userspec: do not treat USER: specially
* lib/userspec.c: Adjust semantics not to treat USER: specially.
GNU chown has had a "feature" whereby specifying "USER:" would make
chown change owner to the non-numeric "USER", and at the same time
change the group to the login group of USER. By a similar token,
since it's not generally possible to map a numeric USER to a user
name, chown would reject a numeric "USER:". Now, "USER:" is
treated the same as "USER", for both numeric and non-numeric USER.
---
ChangeLog | 11 +++++++++++
lib/userspec.c | 21 +++++----------------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 6eec830..c4098c0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2009-12-02 Jim Meyering <meyer...@redhat.com>
+
+ userspec: do not treat USER: specially
+ * lib/userspec.c: Adjust semantics not to treat USER: specially.
+ GNU chown has had a "feature" whereby specifying "USER:" would make
+ chown change owner to the non-numeric "USER", and at the same time
+ change the group to the login group of USER. By a similar token,
+ since it's not generally possible to map a numeric USER to a user
+ name, chown would reject a numeric "USER:". Now, "USER:" is
+ treated the same as "USER", for both numeric and non-numeric USER.
+
2009-12-01 Jim Meyering <meyer...@redhat.com>
fts: fts_open: do not let an empty string cause immediate failure
diff --git a/lib/userspec.c b/lib/userspec.c
index 0388cb1..e12b567 100644
--- a/lib/userspec.c
+++ b/lib/userspec.c
@@ -105,7 +105,6 @@ parse_with_separator (char const *spec, char const
*separator,
{
static const char *E_invalid_user = N_("invalid user");
static const char *E_invalid_group = N_("invalid group");
- static const char *E_bad_spec = N_("invalid spec");
const char *error_msg;
struct passwd *pwd;
@@ -158,22 +157,12 @@ parse_with_separator (char const *spec, char const
*separator,
pwd = (*u == '+' ? NULL : getpwnam (u));
if (pwd == NULL)
{
- bool use_login_group = (separator != NULL && g == NULL);
- if (use_login_group)
- {
- /* If there is no group,
- then there may not be a trailing ":", either. */
- error_msg = E_bad_spec;
- }
+ unsigned long int tmp;
+ if (xstrtoul (u, NULL, 10, &tmp, "") == LONGINT_OK
+ && tmp <= MAXUID && (uid_t) tmp != (uid_t) -1)
+ unum = tmp;
else
- {
- unsigned long int tmp;
- if (xstrtoul (u, NULL, 10, &tmp, "") == LONGINT_OK
- && tmp <= MAXUID && (uid_t) tmp != (uid_t) -1)
- unum = tmp;
- else
- error_msg = E_invalid_user;
- }
+ error_msg = E_invalid_user;
}
else
{
--
1.6.6.rc0.324.gb5bf2
>From 33cf9804ac5593a267d9c76482d5a5c85b01db84 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Sat, 28 Nov 2009 11:51:08 +0100
Subject: [PATCH 2/2] userspec-tests: test the userspec module
* tests/test-userspec.c: New file.
* modules/userspec-tests: Likewise.
---
ChangeLog | 4 ++
modules/userspec-tests | 10 ++++
tests/test-userspec.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 153 insertions(+), 0 deletions(-)
create mode 100644 modules/userspec-tests
create mode 100644 tests/test-userspec.c
diff --git a/ChangeLog b/ChangeLog
index c4098c0..20720d0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -34,6 +34,10 @@
2009-11-28 Jim Meyering <meyer...@redhat.com>
+ userspec: add unit tests
+ * tests/test-userspec.c: New file.
+ * modules/userspec-tests: Likewise.
+
userspec: depend on the inttostr module, too
* modules/userspec (Depends-on): Add inttostr.
diff --git a/modules/userspec-tests b/modules/userspec-tests
new file mode 100644
index 0000000..97bcbdb
--- /dev/null
+++ b/modules/userspec-tests
@@ -0,0 +1,10 @@
+Files:
+tests/test-userspec.c
+
+Depends-on:
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-userspec
+check_PROGRAMS += test-userspec
diff --git a/tests/test-userspec.c b/tests/test-userspec.c
new file mode 100644
index 0000000..7cc9909
--- /dev/null
+++ b/tests/test-userspec.c
@@ -0,0 +1,139 @@
+/* Test userspec.c
+ Copyright (C) 2009 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/>. */
+
+/* Written by Jim Meyering. */
+
+#include <config.h>
+
+#include "userspec.h"
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <sys/types.h>
+
+struct test
+{
+ char const *in;
+ uid_t uid;
+ gid_t gid;
+ char *user_name;
+ char *group_name;
+ char const *result;
+};
+
+static struct test T[] =
+ {
+ { "", -1, -1, "", "", NULL},
+ { ":", -1, -1, "", "", NULL},
+ { "0:0", 0, 0, "", "", NULL},
+ { ":1", -1, 1, "", "", NULL},
+ { "1:", 1, -1, "", "", NULL},
+ { "1", 1, -1, "", "", NULL},
+ { "+0:", 0, -1, "", "", NULL},
+ { ":+0", -1, 0, "", "", NULL},
+ { "22:42", 22, 42, "", "", NULL},
+ { "4294967295:4294967295", 0, 0, NULL, NULL, "invalid user"},
+ { "0:4294967295", 0, 0, NULL, NULL, "invalid group"},
+ { ":4294967295", 0, 0, NULL, NULL, "invalid group"},
+ { "4294967295:0", 0, 0, NULL, NULL, "invalid user"},
+ { "4294967295:", 0, 0, NULL, NULL, "invalid user"},
+ { "4294967296:4294967296", 0, 0, NULL, NULL, "invalid user"},
+ { "0:4294967296", 0, 0, NULL, NULL, "invalid group"},
+ { ":4294967296", 0, 0, NULL, NULL, "invalid group"},
+ { "4294967296:0", 0, 0, NULL, NULL, "invalid user"},
+ { "4294967296:", 0, 0, NULL, NULL, "invalid user"},
+
+ { NULL, 0, 0, NULL, NULL, ""}
+ };
+
+#define STREQ(a, b) (strcmp (a, b) == 0)
+
+static char const *
+maybe_null (char const *s)
+{
+ return s ? s : "NULL";
+}
+
+static bool
+same_diag (char const *s, char const *t)
+{
+ if (s == NULL && t == NULL)
+ return true;
+ if (s == NULL || t == NULL)
+ return false;
+ return STREQ (s, t);
+}
+
+int
+main (void)
+{
+ unsigned int i;
+ int fail = 0;
+
+ for (i = 0; T[i].in; i++)
+ {
+ uid_t uid = (uid_t) -1;
+ gid_t gid = (gid_t) -1;
+ char *user_name;
+ char *group_name;
+ char const *diag = parse_user_spec (T[i].in, &uid, &gid,
+ &user_name, &group_name);
+ free (user_name);
+ free (group_name);
+ if (!same_diag (diag, T[i].result))
+ {
+ printf ("%s return value mismatch: got %s, expected %s\n",
+ T[i].in, maybe_null (diag), maybe_null (T[i].result));
+ fail = 1;
+ continue;
+ }
+
+ if (diag)
+ continue;
+
+ if (uid != T[i].uid || gid != T[i].gid)
+ {
+ printf ("%s mismatch (-: expected uid,gid; +:actual)\n"
+ "-%3lu,%3lu\n+%3lu,%3lu\n",
+ T[i].in,
+ (unsigned long int) T[i].uid,
+ (unsigned long int) T[i].gid,
+ (unsigned long int) uid,
+ (unsigned long int) gid);
+ fail = 1;
+ }
+
+ if (!diag && !T[i].result)
+ continue;
+
+ {
+ printf ("%s diagnostic mismatch (-: expected uid,gid; +:actual)\n"
+ "-%s\n+%s\n",
+ T[i].in, T[i].result, diag);
+ fail = 1;
+ }
+ }
+
+ return fail;
+}
+
+/*
+Local Variables:
+indent-tabs-mode: nil
+End:
+*/
--
1.6.6.rc0.324.gb5bf2
>From 7be609101fbbaf325f6dc91908fb61f7d7a62590 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Wed, 2 Dec 2009 19:33:40 +0100
Subject: [PATCH] chown: do not treat a "USER:" spec specially
GNU chown has had a "feature" whereby specifying "USER:" would make it
change owner to the non-numeric "USER", and at the same time change
the group to the login group of USER. As a consequence, since it's
not always possible to map a numeric USER to a user name, chown would
reject a numeric "USER:". Now, "USER:" is treated the same as "USER",
for both numeric and non-numeric USER.
* src/chown.c (usage): Adjust description.
* tests/chown/separator: Do not require "chown 0: ." to fail.
* doc/coreutils.texi (chown invocation): Remove a paragraph.
* NEWS (Changes in behavior): Mention it.
---
NEWS | 6 ++++++
doc/coreutils.texi | 5 -----
src/chown.c | 3 +--
tests/chown/separator | 5 +----
4 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/NEWS b/NEWS
index 9f7bf2e..36e95ee 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,12 @@ GNU coreutils NEWS -*-
outline -*-
the presence of the empty string argument.
[bug introduced in coreutils-8.0]
+** Changes in behavior
+
+ chown no longer rejects a spec of the form "NUMERIC_USER_ID:",
+ and no longer treats "U:" as "U:G" where U is a non-numeric user name
+ and G is that user's login group. Now, "U:" is treated the same as "U".
+
* Noteworthy changes in release 8.1 (2009-11-18) [stable]
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3721bee..50c85dc 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9453,11 +9453,6 @@ chown invocation
group name or numeric group ID), with no spaces between them, the group
ownership of the files is changed as well (to @var{group}).
-...@item ow...@samp{:}
-If a colon but no group name follows @var{owner}, that user is
-made the owner of the files and the group of the files is changed to
-...@var{owner}'s login group.
-
@item @samp{:}group
If the colon and following @var{group} are given, but the owner
is omitted, only the group of the files is changed; in this case,
diff --git a/src/chown.c b/src/chown.c
index 8104afb..2d5f3d5 100644
--- a/src/chown.c
+++ b/src/chown.c
@@ -130,8 +130,7 @@ one takes effect.\n\
fputs (VERSION_OPTION_DESCRIPTION, stdout);
fputs (_("\
\n\
-Owner is unchanged if missing. Group is unchanged if missing, but changed\n\
-to login group if implied by a `:' following a symbolic OWNER.\n\
+Owner is unchanged if missing. Group is unchanged if missing.\n\
OWNER and GROUP may be numeric as well as symbolic.\n\
"), stdout);
printf (_("\
diff --git a/tests/chown/separator b/tests/chown/separator
index 6e717f6..6c3ed17 100755
--- a/tests/chown/separator
+++ b/tests/chown/separator
@@ -56,10 +56,7 @@ for u in $id_u "$id_un" ''; do
*) seps=': .' ;;
esac
for sep in $seps; do
- case $u$sep$g in
- [0-9]*$sep) chown "$u$sep$g" . 2> /dev/null && fail=1 ;;
- *) chown "$u$sep$g" . || fail=1 ;;
- esac
+ chown "$u$sep$g" . || fail=1
done
done
done
--
1.6.6.rc0.324.gb5bf2
