On Thursday 21 October 2010 01:07:59 Denys Vlasenko wrote:
> On Tuesday 05 October 2010 22:38, Tito wrote:
> > > > Why? Who said that group foo needs to be removed when user foo is being
> > > > removed?
> > > > Do other deluser utilities do this?
> > > Hi
> > > Yes at least on my Debian system if i create a user:
> > >
> > > adduser prova
> > > Adding user `prova' ...
> > > Adding new group `prova' (1006) ...
> > > Adding new user `prova' (1004) with group `prova' ...
> > > The home directory `/home/prova' already exists. Not copying from
> > > `/etc/skel'.
> > > Enter new UNIX password:
> > > Retype new UNIX password:
> > > passwd: password updated successfully
> > > Changing the user information for prova
> > > Enter the new value, or press ENTER for the default
> > > Full Name []:
> > > Room Number []:
> > > Work Phone []:
> > > Home Phone []:
> > > Other []:
> > > Is the information correct? [Y/n] debian:/home/tito/Desktop#
> > >
> > >
> > > grep prova /etc/pashadowetc/shadow /etc/group /etc/gshadow
> > > /etc/passwd:prova:x:1004:1006:,,,:/home/prova:/bin/bash
> > > /etc/shadow:prova:$6$I..Q9V.z$aoa7lBKlI68heRLDh9E0oMm8DUiBFO9MfBZgOYYjr/09v0Z1nPipjDPxz7TbbuwLmRslWZVdpI3Wr/G4IxDrl1:14886:0:99999:7:::
> > > /etc/group:prova:x:1006:
> > > /etc/gshadow:prova:!::
> > >
> > > deluser prova
> > > Removing user `prova' ...
> > > Warning: group `prova' has no more members.
> > > Done.
> > > grep prova /etc/passwd /etc/shadow /etc/group /etc/gshadow
> > > debian:/home/tito/Desktop#
> > >
> > >
> > > adduser command creates a group with the same name as the username,
> > > and makes this group the primary group for that user. This is called a
> > > user private group (UPG)
> > >
> > > So i test if a group with the same name exists and delete it,
> > > Maybe a check if the group is empty could be added but as
> > > we don't check for that at all in the current implementation
> > > and happily remove non-empty groups I've skipped that.
> > >
>
>
>
> > > > + if (!member && getpwnam(name))
> > > > + bb_error_msg_and_die("is %s's
> > > > primary group", name);
> > > >
> > > > This looks unreadable. Can be rewritten to do the same,
> > > > but make more sense to human reader, as:
> > > >
> > > > if (!member) {
> > > > if (getpwnam(name) = 0) {
> > > > bb_error_msg_and_die("is %s's primary group",
> > > > name);
> > > > }
> > > > }
> > > >
> > >
> > > Can change it if you like it that way, for my personal taste just 3
> > > wasted lines :-)
> > >
> > > Should be:
> > >
> > > if (!member) {
> > > if (getpwnam(name)) {
> > > bb_error_msg_and_die("is %s's primary group", name);
> > > }
> > > }
> > >
> > > We check if getpwnam returns a struct passwd and in this case
> > > it is a primary group (group with the same name of a user).
> > >
> > > > The message also needs to be better. This:
> > > >
> > > > deluser: is foo's primary group: <strerror here>
> > > >
> > > > is incomprehensible. What are you trying to say?
> > >
> > > Original error message on my system is:
> > >
> > > delgroup tito
> > > /usr/sbin/delgroup: `tito' still has `tito' as their primary group!
> > >
> > > I shortened it a little to:
> > >
> > > delgroup: is tito's primary group
> > >
> > > Could be changed. As i'm not a native english speaker I'm open
> > > to suggestions about a short and easy to understand error message
> > > for this use case.
>
> Yes, "`tito' still has `tito' as their primary group!" is understandable,
> while "is tito's primary group" isn't.
Hi,
can be changed.
> Moreover, I think the check for username==groupname is wrong.
>
> Primary group is the group whose GID is listed as 4th field in /etc/passwd
> for some user:
>
> syslog:x:54:50::/:
> ^^
> You can't delete group with gid 50 while there is at least one such user.
> It has nothing to do with names of user and group being the same.
>
>From man adduser:
By default, each user in Debian GNU/Linux is given a corresponding
group with the same name.
So when deleting a user we look if there is a group with corresponding name.
If the user is part of a different group like:
tito:users
the group is not removed.
Maybe a more correct definition could be: "User Private Groups", so the error
message could be changed accordingly.
Red Hat Linux uses a user private group (UPG) scheme, which makes UNIX groups
much easier to use.
The UPG scheme does not add or change anything in the standard UNIX way of
handling groups;
it simply offers a new convention. Whenever you create a new user, by default,
he or she has a unique group.
The scheme works as follows:
User Private Group
Every user has a primary group; the user is the only member of that group.
Any idea for the best and shortest error message?
or should i just use:
"`tito' still has `tito' as their primary group!"
> > Here v4,
> >
> > just cosmetical changes for readability.
>
> My problem with this patch is that it goes from:
>
> if (ENABLE_DELUSER && applet_name[3] == 'u') {
> /* deluser USER */
> if (update_passwd(bb_path_passwd_file, argv[1], NULL, NULL) <
> 0)
> return EXIT_FAILURE;
> if (ENABLE_FEATURE_SHADOWPASSWDS)
> if (update_passwd(bb_path_shadow_file, argv[1], NULL,
> NULL) < 0)
> return EXIT_FAILURE;
> } else if (ENABLE_DELGROUP) {
> /* delgroup ... */
> if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || argc != 3) {
> /* delgroup GROUP */
> if (update_passwd(bb_path_group_file, argv[1], NULL,
> NULL) < 0)
> return EXIT_FAILURE;
> if (ENABLE_FEATURE_SHADOWPASSWDS)
> if (update_passwd(bb_path_gshadow_file,
> argv[1], NULL, NULL) < 0)
> return EXIT_FAILURE;
> } else {
> /* delgroup USER GROUP */
> if (update_passwd(bb_path_group_file, argv[2], NULL,
> argv[1]) < 0)
> return EXIT_FAILURE;
> if (ENABLE_FEATURE_SHADOWPASSWDS)
> if (update_passwd(bb_path_gshadow_file,
> argv[2], NULL, argv[1]) < 0)
> return EXIT_FAILURE;
> }
> }
>
> to:
>
> switch (argc) {
> case 3:
> if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || do_deluser)
> break;
> name = argv[2];
> member = argv[1];
> /* Fallthrough */
> case 2:
> if (geteuid())
>
> bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
> if (do_deluser) {
> xgetpwnam(name);
> if (ENABLE_FEATURE_SHADOWPASSWDS)
> sfile = bb_path_shadow_file;
> } else {
> do_delgroup:
> xgetgrnam(name);
> if (!member) {
> if(getpwnam(name) != NULL) {
> bb_error_msg_and_die("is %s's
> primary group", name);
> }
> }
> pfile = bb_path_group_file;
> if (ENABLE_FEATURE_SHADOWPASSWDS)
> sfile = bb_path_gshadow_file;
> }
> do {
> if (update_passwd(pfile, name, NULL, member)
> == -1)
> return EXIT_FAILURE;
> pfile = sfile;
> sfile = NULL;
> } while (pfile);
>
> if (ENABLE_DELGROUP && do_deluser) {
> do_deluser = 0;
> goto do_delgroup;
> }
> return EXIT_SUCCESS;
> }
>
>
> This looks like a big step backwards wrt readability.
> All comments are gone. Why?
Looked trivial to me, mainly because I wrote it ;-)
but I can add some.
>
> Can at least
> if (geteuid())
>
> bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
> be moved out of this switch statement from hell,
> to make it a bit easier to grok?
No, it cannot be moved outside because it must be triggered when
doing deluser or delgroup (argc = 2)
and when doing deluser from group (argc = 3)
if uid != 0
> Why "deluser USER" behavior depends on whether delgroup is enabled?
Because if delgroup is enabled I would like to remove the user's private group.
> If I build two busyboxes, one only with deluser, and another only with
> delgroup,
> why they work differently from one with both applets??
>
>
Because if both functionalities are available I will use both when doing
deluser.
Will send a fully commented patch later today.
Ciao,
Tito
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
