[PATCH] 2nd attempt at deluser/delgroup size reduction and improvements

[Denys Vlasenko]

On Sun, Nov 7, 2010 at 10:25 AM, Tito <farmatito at tiscali.it> wrote:
>> I am saying that "delgroup foo" (delgroup, not deluser)
>> should not check whether *user named foo* exists;
>> but should check that deleting group foo doesn't
>> leave users with "deleted" GIDs.
>
>> Example:
>>
>> /etc/passwd
>> haldaemon:x:68:490:HAL daemon:/:/sbin/nologin
>> foo:x:496:1234::/:/sbin/nologin
>>
>> /etc/group
>> foo:490:
>>
>> What "standard" delgroup foo will do? I suspect it will
>> complain that haldaemon user's primary GID is 490
>> and therefore group foo can't be deleted.
>
> adduser prova
> Adding user `prova' ...
> Adding new group `prova' (1006) ...
> Adding new user `prova' (1004) with group `prova' ...
> adduser prova2 --ingroup prova
> Adding user `prova2' ...
> Adding new user `prova2' (1005) with group `prova' ...

And what grep prova /etc/passwd /etc/group shows
after these?

> Test case 1: Removing user prova:
>
> deluser prova

Awww, my brainzzzz... Why do you delete *a user*?
We are trying to determine what is the correct behavior
of *delgroup*, right?

> Removing user `prova' ...
> Warning: group `prova' has no more members.

Seems like an erroneous message. Group prova
should still have at least prova2 user.

Looks like code only checks /etc/group line
whether it contains additional usernames:
prova:x:1006:<any names here?>
but not /etc/passwd for users with GID=1006.
I'd say this is a bug.

> userdel: Cannot remove group prova which is a primary group for another user.

Right, this means that prova2 line looks like this:
prova2:x:1005:1006:...........

> Done.
> grep prova /etc/group
> prova:x:1006:
>
> Test case 2: Removing user prova2:
>
> deluser prova2
> Removing user `prova2' ...
> Warning: group `prova' has no more members.
> Done.

Attention. See _which_ group it deleted here?
It did not try to delete a group with the same name as user,
it looked at GID (2006), found which group it is (prova),
and then deleted it.

I was right in the second bug comment, we don't do
this correctly too.

> grep prova /etc/group
> prova:x:1006:

So you are saying group still exists?

> Test case 3: Removing group prova:
>
> delgroup prova
> /usr/sbin/delgroup: `prova' still has `prova' as their primary group!

Wait a sec. User prova should already be deleted, no?
I'm confused...

> Test case 4: Removing group prova after removal of user prova:
>
> deluser prova
> Removing user `prova' ...
> Warning: group `prova' has no more members.
> userdel: Cannot remove group prova which is a primary group for another user.
> Done.
> delgroup prova
> /usr/sbin/delgroup: `prova2' still has `prova' as their primary group!

Yes, now we see that they don't have our BUG #1.
They correctly check passwd.



>> If you remove haldaemon line from /etc/passwd,
>> delgroup foo will succeed despite the fact that _user_ foo exists.
>> Because in this example, _user_ foo and _group_ foo
>> are completely unrelated.
>>
>> Our version gets this wrong, I think.
>
> In some cases of course it does. To fix this
> will be rather expensive in size:
>
> if (doing_deluser) {
>        get gid
>        delete_user
>        walk through passwd file

No, why would you want the passwd file?
it seems they just try "delgroup <group_with_gid_of_deleted_user>"

>        if users with same gid
>                complain and do nothing to group file

This will be done as part of "delgroup <group_with_gid_of_deleted_user>"
operation.

>        else
>                if username == groupname // is UserPrivateGroup
>                        delete_group

Where is the evidence it works like that, *by name*?

> if (doing_delgroup) {
>        get_gid
>        // NO - check if group is empty
>        walk through passwd file
>        if users with same gid
>                complain and do nothing to group file
> }

This is correct.


> case argc2
>        if (deluser) {
>                setup deluser
>                gid = get_gid
>        } else {
> do_delgroup:
>        /* delgroup */
>                setup delgroup
>                if (no_gid)
>                        gid = get_gid
>                // NO - check if group is empty
>                walk through passwd file
>                if users with same gid > 0
>                        complain and do nothing to group file
>                else if (doing_delgroup after deluser /* needs a flag ? */) {
>                        if username != groupname // not UserPrivateGroup
>                                do_nothing to group file
>                }
>        }
>
>        do_deletions_loop
>
>        if (deluser ) {
>                        set_a_flag
>                        goto do_delgroup
>        }

Not entirely correct, see comments above.
-- 
vda