Commit 9ab75810 authored by Mike Hibler's avatar Mike Hibler

Optimize the case of a "usermod -G" which changes only a few groups.

The handling of group file changes via the -G option is abysmal.
It creates a new copy of the installed group file on every group line
changed, fsync'ing after the new copy is made. On top of that, it implements
a new group list by first removing the user from all existing groups and then
reading them to every group in the new list, thereby maximizing the number of
group lines changed!

On the Emulab mothership where, for example, the "geniuser" user is in
some 450+ groups, adding them to a new group entailed changing 450+ lines
twice, resulting in over 900 copies of the 2000 line group file being
written. This took about 17 minutes.

The change here is modest, just check where the group line needs to be
changed or not before doing anything. In the case above, adding a single
group for a user, we only write the group file once. This takes about 0.8
seconds.
parent 18729d86
diff -Nur pw.orig/Makefile pw/Makefile
--- pw.orig/Makefile 2015-08-25 08:41:11.000000000 -0600
+++ pw/Makefile 2016-11-22 12:46:28.248147000 -0700
@@ -8,6 +8,8 @@
grupd.c pwupd.c fileupd.c psdate.c \
bitmap.c cpdir.c rm_r.c
+#CFLAGS+=-DOLDCODE_COMPAT
+
WARNS?= 3
DPADD= ${LIBCRYPT} ${LIBUTIL} ${LIBSBUF}
diff -Nur pw.orig/pw_user.c pw/pw_user.c
--- pw.orig/pw_user.c 2015-08-25 08:41:11.000000000 -0600
+++ pw/pw_user.c 2016-11-22 11:04:11.272539000 -0700
@@ -636,6 +636,7 @@
if (mode == M_ADD || getarg(args, 'G') != NULL) {
int i, j;
+#ifdef OLDCODE_COMPAT
/* First remove the user from all group */
SETGRENT();
while ((grp = GETGRENT()) != NULL) {
@@ -668,8 +669,55 @@
chggrent(cnf->groups[i], grp);
free(grp);
}
- }
+#else
+ /*
+ * N.B. every chggrent() call causes a copy of the entire group
+ * file, so minimize the number of calls we make!
+ */
+ SETGRENT();
+ while ((grp = GETGRENT()) != NULL) {
+ int oingroup, ningroup;
+ char group[MAXLOGNAME];
+ strlcpy(group, grp->gr_name, MAXLOGNAME);
+
+ /* Are we supposed to be in this group? */
+ ningroup = 0;
+ for (i = 0; cnf->groups[i] != NULL; i++) {
+ if (strcmp(cnf->groups[i], group) == 0) {
+ ningroup = 1;
+ break;
+ }
+ }
+
+ /* Are we already in this group? */
+ oingroup = -1;
+ for (i = 0; grp->gr_mem && grp->gr_mem[i] != NULL; i++) {
+ if (strcmp(grp->gr_mem[i], pwd->pw_name) == 0) {
+ oingroup = i;
+ break;
+ }
+ }
+
+ /* If we found the user and they should not be here, remove */
+ if (oingroup >= 0 && ningroup == 0) {
+ for (j = oingroup; grp->gr_mem[j] != NULL; j++)
+ grp->gr_mem[j] = grp->gr_mem[j+1];
+ chggrent(group, grp);
+ }
+
+ /* If we did not find the user and they should be here, add */
+ else if (oingroup < 0 && ningroup != 0) {
+ grp = gr_add(grp, pwd->pw_name);
+ if (grp != NULL) {
+ chggrent(group, grp);
+ free(grp);
+ }
+ }
+ }
+ ENDGRENT();
+#endif
+ }
/* go get a current version of pwd */
pwd = GETPWNAM(name);
diff -Nur pw.orig/Makefile pw/Makefile
--- pw.orig/Makefile 2016-11-21 11:09:19.249036000 -0700
+++ pw/Makefile 2016-11-22 06:51:45.802580000 -0700
@@ -8,6 +8,9 @@
grupd.c pwupd.c psdate.c bitmap.c cpdir.c rm_r.c strtounum.c \
pw_utils.c
+# Allow -O option to do usermod group changes the old way
+CFLAGS+= -DOLDCODE_COMPAT
+
WARNS?= 3
DPADD= ${LIBCRYPT} ${LIBUTIL} ${LIBSBUF}
diff -Nur pw.orig/pw_user.c pw/pw_user.c
--- pw.orig/pw_user.c 2016-05-27 09:23:45.000000000 -0600
+++ pw/pw_user.c 2016-11-22 06:51:30.409582000 -0700
@@ -1486,7 +1486,12 @@
struct passwd *pwd;
struct group *grp;
StringList *groups = NULL;
+#ifdef OLDCODE_COMPAT
+ char args[] = "C:qn:u:c:d:e:p:g:G:mM:l:k:s:w:L:h:H:NPYy:O";
+ int oldcode = 0;
+#else
char args[] = "C:qn:u:c:d:e:p:g:G:mM:l:k:s:w:L:h:H:NPYy:";
+#endif
const char *cfg;
char *gecos, *homedir, *grname, *name, *newname, *walk, *skel, *shell;
char *passwd, *class, *nispasswd;
@@ -1605,6 +1610,11 @@
case 'Y':
nis = true;
break;
+#ifdef OLDCODE_COMPAT
+ case 'O':
+ oldcode = 1;
+ break;
+#endif
}
}
@@ -1666,12 +1676,27 @@
if (grname && pwd->pw_uid != 0) {
grp = GETGRNAM(grname);
- if (grp == NULL)
+ if (grp == NULL) {
grp = GETGRGID(pw_checkid(grname, GID_MAX));
+ grname = grp->gr_name;
+ }
if (grp->gr_gid != pwd->pw_gid) {
pwd->pw_gid = grp->gr_gid;
edited = true;
}
+ /* if grname is in group list, remove it */
+ if (groups && sl_find(groups, grname)) {
+ StringList *ngroups = NULL;
+ if (groups->sl_cur > 1) {
+ ngroups = sl_init();
+ for (i = 0; i < groups->sl_cur; i++) {
+ if (strcmp(grname, groups->sl_str[i]))
+ sl_add(ngroups, groups->sl_str[i]);
+ }
+ }
+ sl_free(groups, 0);
+ groups = ngroups;
+ }
}
if (password_days >= 0 && pwd->pw_change != password_days) {
@@ -1740,31 +1765,77 @@
perform_chgpwent(name, pwd, nis ? nispasswd : NULL);
/* Now perform the needed changes concern groups */
if (groups != NULL) {
- /* Delete User from groups using old name */
+#ifdef OLDCODE_COMPAT
+ if (oldcode) {
+ /* Delete User from groups using old name */
+ SETGRENT();
+ while ((grp = GETGRENT()) != NULL) {
+ if (grp->gr_mem == NULL)
+ continue;
+ for (i = 0; grp->gr_mem[i] != NULL; i++) {
+ if (strcmp(grp->gr_mem[i] , name) != 0)
+ continue;
+ for (j = i; grp->gr_mem[j] != NULL ; j++)
+ grp->gr_mem[j] = grp->gr_mem[j+1];
+ chggrent(grp->gr_name, grp);
+ break;
+ }
+ }
+ ENDGRENT();
+ /* Add the user to the needed groups */
+ for (i = 0; i < groups->sl_cur; i++) {
+ grp = GETGRNAM(groups->sl_str[i]);
+ grp = gr_add(grp, pwd->pw_name);
+ if (grp == NULL)
+ continue;
+ chggrent(grp->gr_name, grp);
+ free(grp);
+ }
+ goto finished;
+ }
+#endif
+
+ /*
+ * N.B. every chggrent() call causes a copy of the entire group
+ * file, so minimize the number of calls we make!
+ */
SETGRENT();
while ((grp = GETGRENT()) != NULL) {
- if (grp->gr_mem == NULL)
- continue;
- for (i = 0; grp->gr_mem[i] != NULL; i++) {
- if (strcmp(grp->gr_mem[i] , name) != 0)
- continue;
- for (j = i; grp->gr_mem[j] != NULL ; j++)
+ int oingroup, ningroup;
+
+ /* Are we supposed to be in this group? */
+ ningroup = (sl_find(groups, grp->gr_name) != NULL) ? 1 : 0;
+
+ /* Are we already in this group? */
+ oingroup = -1;
+ for (i = 0; grp->gr_mem && grp->gr_mem[i] != NULL; i++) {
+ if (strcmp(grp->gr_mem[i], name) == 0) {
+ oingroup = i;
+ break;
+ }
+ }
+
+ /* If we found the user and they should not be here, remove */
+ if (oingroup >= 0 && ningroup == 0) {
+ for (j = oingroup; grp->gr_mem[j] != NULL; j++)
grp->gr_mem[j] = grp->gr_mem[j+1];
chggrent(grp->gr_name, grp);
- break;
+ }
+
+ /* If we did not find the user and they should be here, add */
+ else if (oingroup < 0 && ningroup != 0) {
+ grp = gr_add(grp, pwd->pw_name);
+ if (grp != NULL) {
+ chggrent(grp->gr_name, grp);
+ free(grp);
+ }
}
}
ENDGRENT();
- /* Add the user to the needed groups */
- for (i = 0; i < groups->sl_cur; i++) {
- grp = GETGRNAM(groups->sl_str[i]);
- grp = gr_add(grp, pwd->pw_name);
- if (grp == NULL)
- continue;
- chggrent(grp->gr_name, grp);
- free(grp);
- }
}
+#ifdef OLDCODE_COMPAT
+ finished:
+#endif
/* In case of rename we need to walk over the different groups */
if (newname) {
SETGRENT();
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment