On Wed, Feb 18, 2015 at 3:08 PM, tito <[email protected]> wrote: > On Wednesday 18 February 2015 15:13:21 you wrote: > >> On Wed, Feb 18, 2015 at 12:09 PM, Laszlo Papp <[email protected]> wrote: > >> > On Wed, Feb 18, 2015 at 11:00 AM, tito <[email protected]> wrote: > >> >> On Wednesday 18 February 2015 09:14:42 you wrote: > >> >> > >> >>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp <[email protected]> wrote: > >> >> > >> >>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp <[email protected]> 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? > >> > >> Actually, the locking and swapping logic would be common anyway which > >> already resides in update_passwd, so if that reusable component is not > >> moved to a separate function to be reused, we could use put our > >> conditional magic into the while loop. I am preparing with that change > >> now. > >> > >> By the way, why does busybox use /etc/file+ and /etc/file- for this > >> logic as opposed to ... say: mkstemp? > > > > So other instances of programs that want to change > > file know that it is in use (file+ existing) and wait?
Is that a valid reasoning? The file ought to be opened in locked mode all the time when starting the operation, in my opinion, in which case this does not apply. Furthermore, if I precreate the "lock" file like that, it is easy to break the applet by just creating the file beforehand. man 2 flock (LOCK_EX) man 3 mkstemp > > > >> > commit 813e3db073aca7a55b9f3d4650055e7d1ed768b4 > >> > Author: Laszlo Papp <[email protected]> > >> > 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 > [email protected] > http://lists.busybox.net/mailman/listinfo/busybox _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
