On Wed, Feb 18, 2015 at 11:00 AM, tito <farmat...@tiscali.it> wrote:
> On Wednesday 18 February 2015 09:14:42 you wrote:
>
>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp <lp...@kde.org> wrote:
>
>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp <lp...@kde.org> wrote:
>
>> >> I thought about this, too, when writing the patch, but I rejected it
>> >> because
>
>> >> it is not simpler, in fact more complex, it is also not the right layer
>> >> for
>
>> >> the change. In addition, it would also be somewhat slower to execute.
>
>> >
>
>> > Just to elaborate a bit more on this as I was sending that on my phone:
>
>> >
>
>> > 1) Why do you want to make a few lines more simpler to spare a few
>
>> > lines to introduce bad implementation and slower operation?
>
>> >
>
>> > 2) Also, the update_passwd function already has all the structure to
>
>> > handle this through the file, name and member variables. It looks like
>
>> > the natural place to utilize the existing infrastructure.
>
>> >
>
>> > 3) Basically, you would go through the groups once in the embedded
>
>> > list and then you would go through again rather than just doing the
>
>> > thing correct in the first place at the first iteration. So basically,
>
>> > you would have double embedded list iteration. Even if the group list
>
>> > was stored in memory for _each_ user, it would still slightly be
>
>> > slower and I would argue that wasting memory for potentially a big
>
>> > file could defeat busybox's purpose in the first place. Anyway,
>
>> > busybox's design goal should be to be as fast as possible. Personal
>
>> > preferences should neither slow it down, nor make it use more memory
>
>> > than needed to achieve the task.
>
>>
>
>> I would actually even go further than that, namely: the same-named
>
>> group deletion should probably be integrated into my loop so that one
>
>> embedded iteration could deal with the group file changes rather than
>
>> two separate.
>
>>
>
>
>
> Hi,
>
>
>
> you can try to write a patch, but i suspect that it could be difficult
>
> with the current update_password interface without breaking the
>
> deluser from group use case.
>
> If you opt for doing the changes in deluser instead some useful
>
> get_groups code is in the id applet and could be moved to libbb as
>
> function. This will of course be slower as more iterations are done
>
> through the passwd/group files.
I start to think that the best approach would be neither to use your
idea, nor mine, but actually a separate function doing the thing fast.
See my updated patch below which almost works, but it remains slow if
I try to inject the new code into the "line based" read and write
concept. Why don't we just create a new function?
commit 813e3db073aca7a55b9f3d4650055e7d1ed768b4
Author: Laszlo Papp <lp...@kde.org>
Date: Tue Feb 17 20:01:04 2015 +0000
Delete the user from all the groups for user deletion
diff --git a/libbb/update_passwd.c b/libbb/update_passwd.c
index a30af6f..24f949b 100644
--- a/libbb/update_passwd.c
+++ b/libbb/update_passwd.c
@@ -62,6 +62,8 @@ static void check_selinux_update_passwd(const char *username)
only if CONFIG_PASSWD=y and applet_name[0] == 'p' like in passwd
or if CONFIG_CHPASSWD=y and applet_name[0] == 'c' like in chpasswd
+ 8) delete a user from all groups : update_passwd(FILE, NULL, NULL, MEMBER)
+
This function does not validate the arguments fed to it
so the calling program should take care of that.
@@ -81,7 +83,7 @@ int FAST_FUNC update_passwd(const char *filename,
FILE *new_fp;
char *fnamesfx;
char *sfx_char;
- char *name_colon;
+ char *name_colon = 0;
unsigned user_len;
int old_fd;
int new_fd;
@@ -99,13 +101,15 @@ int FAST_FUNC update_passwd(const char *filename,
if (filename == NULL)
return ret;
- check_selinux_update_passwd(name);
+ if (name) check_selinux_update_passwd(name);
/* New passwd file, "/etc/passwd+" for now */
fnamesfx = xasprintf("%s+", filename);
sfx_char = &fnamesfx[strlen(fnamesfx)-1];
- name_colon = xasprintf("%s:", name);
- user_len = strlen(name_colon);
+ if (name) {
+ name_colon = xasprintf("%s:", name);
+ user_len = strlen(name_colon);
+ }
if (shadow)
old_fp = fopen(filename, "r+");
@@ -159,6 +163,19 @@ int FAST_FUNC update_passwd(const char *filename,
bb_perror_msg("warning: can't lock '%s'", filename);
lock.l_type = F_UNLCK;
+ if (!name && member) {
+ struct group* g;
+ while ((g = getgrent())) {
+ char **s= g->gr_mem;
+ char **d = s;
+ while (*s) {
+ if (strcmp(*s, member)) { *d = *s; ++d; }
+ ++s;
+ }
+ *d = 0;
+ }
+ }
+
/* Read current password file, write updated /etc/passwd+ */
changed_lines = 0;
while (1) {
@@ -167,7 +184,7 @@ int FAST_FUNC update_passwd(const char *filename,
line = xmalloc_fgetline(old_fp);
if (!line) /* EOF/error */
break;
- if (strncmp(name_colon, line, user_len) != 0) {
+ if (!name_colon || strncmp(name_colon, line, user_len) != 0) {
fprintf(new_fp, "%s\n", line);
goto next;
}
@@ -176,7 +193,7 @@ int FAST_FUNC update_passwd(const char *filename,
cp = line + user_len; /* move past name: */
#if ENABLE_FEATURE_ADDUSER_TO_GROUP || ENABLE_FEATURE_DEL_USER_FROM_GROUP
- if (member) {
+ if (name && member) {
/* It's actually /etc/group+, not /etc/passwd+ */
if (ENABLE_FEATURE_ADDUSER_TO_GROUP
&& applet_name[0] == 'a'
@@ -240,7 +257,7 @@ int FAST_FUNC update_passwd(const char *filename,
if (changed_lines == 0) {
#if ENABLE_FEATURE_ADDUSER_TO_GROUP || ENABLE_FEATURE_DEL_USER_FROM_GROUP
- if (member) {
+ if (name && member) {
if (ENABLE_ADDGROUP && applet_name[0] == 'a')
bb_error_msg("can't find %s in %s",
name, filename);
if (ENABLE_DELGROUP && applet_name[0] == 'd')
diff --git a/loginutils/deluser.c b/loginutils/deluser.c
index 01a9386..8219569 100644
--- a/loginutils/deluser.c
+++ b/loginutils/deluser.c
@@ -82,6 +82,9 @@ int deluser_main(int argc, char **argv)
do_delgroup:
/* "delgroup GROUP" or "delgroup USER GROUP" */
if (do_deluser < 0) { /* delgroup after deluser? */
+ pfile = bb_path_group_file;
+ if (update_passwd(pfile, NULL, NULL, name) == -1)
+ return EXIT_FAILURE;
gr = getgrnam(name);
if (!gr)
return EXIT_SUCCESS;
@@ -99,7 +102,6 @@ int deluser_main(int argc, char **argv)
}
//endpwent();
}
- pfile = bb_path_group_file;
if (ENABLE_FEATURE_SHADOWPASSWDS)
sfile = bb_path_gshadow_file;
}
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
