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.
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.
> 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?
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?
Why "deluser USER" behavior depends on whether delgroup is enabled?
If I build two busyboxes, one only with deluser, and another only with delgroup,
why they work differently from one with both applets??
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
