[PATCH 0/1] V2: Replace retry logic in useradd with flock

[PATCH 0/1] V2: Replace retry logic in useradd with flock

[kai.kang]

From: Kai Kang <kai.kang@windriver.com>

V2:

* Use $SYSROOT/etc as flock file

The following changes since commit da13f0bce5d1b3c52f5716ae8d104372a820e5f9:

  buildhistory.bbclass: remove out-dated information on request (2016-02-15 17:47:08 +0000)

are available in the git repository at:

  git://git.pokylinux.org/poky-contrib kangkai/flock
  http://git.pokylinux.org/cgit.cgi/poky-contrib/log/?h=kangkai/flock

Kai Kang (1):
  useradd_base.bbclass: fix simultaneous with flock

 meta/classes/useradd.bbclass      |   8 +-
 meta/classes/useradd_base.bbclass | 165 ++++++++++----------------------------
 2 files changed, 48 insertions(+), 125 deletions(-)

-- 
2.7.0

[PATCH 1/1] useradd_base.bbclass: fix simultaneous with flock

[kai.kang]

From: Kai Kang <kai.kang@windriver.com>

When perform useradd during populate sysroot, it locks files passwd.lock
and group.lock at same time. And then it meets a dead lock issue
randomly.

Use flock to reslove it by using an universal lock file for all the
user and group related operations.

[YOCTO #9022]

Signed-off-by: Kai Kang <kai.kang@windriver.com>
---
 meta/classes/useradd.bbclass      |   8 +-
 meta/classes/useradd_base.bbclass | 165 ++++++++++----------------------------
 2 files changed, 48 insertions(+), 125 deletions(-)

diff --git a/meta/classes/useradd.bbclass b/meta/classes/useradd.bbclass
index c960656..4503020 100644
--- a/meta/classes/useradd.bbclass
+++ b/meta/classes/useradd.bbclass
@@ -38,6 +38,8 @@ if test "x$D" != "x"; then
 	export PSEUDO_PASSWD="$SYSROOT:${STAGING_DIR_NATIVE}"
 fi
 
+FLOCK_FILE=$SYSROOT/etc
+
 # If we're not doing a special SSTATE/SYSROOT install
 # then set the values, otherwise use the environment
 if test "x$UA_SYSROOT" = "x"; then
@@ -57,7 +59,7 @@ if test "x`echo $GROUPADD_PARAM | tr -d '[:space:]'`" != "x"; then
 	opts=`echo "$GROUPADD_PARAM" | cut -d ';' -f 1`
 	remaining=`echo "$GROUPADD_PARAM" | cut -d ';' -f 2-`
 	while test "x$opts" != "x"; do
-		perform_groupadd "$SYSROOT" "$OPT $opts" 10
+		perform_groupadd "$SYSROOT" "$OPT $opts"
 		if test "x$opts" = "x$remaining"; then
 			break
 		fi
@@ -73,7 +75,7 @@ if test "x`echo $USERADD_PARAM | tr -d '[:space:]'`" != "x"; then
 	opts=`echo "$USERADD_PARAM" | cut -d ';' -f 1`
 	remaining=`echo "$USERADD_PARAM" | cut -d ';' -f 2-`
 	while test "x$opts" != "x"; do
-		perform_useradd "$SYSROOT" "$OPT $opts" 10
+		perform_useradd "$SYSROOT" "$OPT $opts"
 		if test "x$opts" = "x$remaining"; then
 			break
 		fi
@@ -89,7 +91,7 @@ if test "x`echo $GROUPMEMS_PARAM | tr -d '[:space:]'`" != "x"; then
 	opts=`echo "$GROUPMEMS_PARAM" | cut -d ';' -f 1`
 	remaining=`echo "$GROUPMEMS_PARAM" | cut -d ';' -f 2-`
 	while test "x$opts" != "x"; do
-		perform_groupmems "$SYSROOT" "$OPT $opts" 10
+		perform_groupmems "$SYSROOT" "$OPT $opts"
 		if test "x$opts" = "x$remaining"; then
 			break
 		fi
diff --git a/meta/classes/useradd_base.bbclass b/meta/classes/useradd_base.bbclass
index ab3cd35..66a9848 100644
--- a/meta/classes/useradd_base.bbclass
+++ b/meta/classes/useradd_base.bbclass
@@ -4,7 +4,7 @@
 
 # The following functions basically have similar logic.
 # *) Perform necessary checks before invoking the actual command
-# *) Invoke the actual command, make retries if necessary
+# *) Invoke the actual command with flock
 # *) Error out if an error occurs.
 
 # Note that before invoking these functions, make sure the global variable
@@ -13,26 +13,16 @@
 perform_groupadd () {
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing groupadd with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing groupadd with [$opts]"
 	local groupname=`echo "$opts" | awk '{ print $NF }'`
 	local group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
 	if test "x$group_exists" = "x"; then
-		local count=0
-		while true; do
-			eval $PSEUDO groupadd $opts || true
-			group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
-			if test "x$group_exists" = "x"; then
-				bbwarn "${PN}: groupadd command did not succeed. Retrying..."
-			else
-				break
-			fi
-			count=`expr $count + 1`
-			if test $count = $retries; then
-				bbfatal "${PN}: Tried running groupadd command $retries times without success, giving up"
-			fi
-                        sleep $count
-		done
+		opts=`echo $opts | sed s/\'/\"/g`
+		eval flock -x -w 100 $FLOCK_FILE -c \'$PSEUDO groupadd $opts\' || true
+		group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
+		if test "x$group_exists" = "x"; then
+			bbfatal "${PN}: groupadd command did not succeed."
+		fi
 	else
 		bbnote "${PN}: group $groupname already exists, not re-creating it"
 	fi
@@ -41,26 +31,16 @@ perform_groupadd () {
 perform_useradd () {
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing useradd with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing useradd with [$opts]"
 	local username=`echo "$opts" | awk '{ print $NF }'`
 	local user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
 	if test "x$user_exists" = "x"; then
-	       local count=0
-	       while true; do
-		       eval $PSEUDO useradd $opts || true
-		       user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
-		       if test "x$user_exists" = "x"; then
-			       bbwarn "${PN}: useradd command did not succeed. Retrying..."
-		       else
-			       break
-		       fi
-		       count=`expr $count + 1`
-		       if test $count = $retries; then
-				bbfatal "${PN}: Tried running useradd command $retries times without success, giving up"
-		       fi
-		       sleep $count
-	       done
+		opts=`echo $opts | sed s/\'/\"/g`
+		eval flock -x -w 100 $FLOCK_FILE -c  \'$PSEUDO useradd $opts\' || true
+		user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
+		if test "x$user_exists" = "x"; then
+			bbfatal "${PN}: useradd command did not succeed."
+		fi
 	else
 		bbnote "${PN}: user $username already exists, not re-creating it"
 	fi
@@ -69,8 +49,7 @@ perform_useradd () {
 perform_groupmems () {
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing groupmems with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing groupmems with [$opts]"
 	local groupname=`echo "$opts" | awk '{ for (i = 1; i < NF; i++) if ($i == "-g" || $i == "--group") print $(i+1) }'`
 	local username=`echo "$opts" | awk '{ for (i = 1; i < NF; i++) if ($i == "-a" || $i == "--add") print $(i+1) }'`
 	bbnote "${PN}: Running groupmems command with group $groupname and user $username"
@@ -84,25 +63,11 @@ perform_groupmems () {
 	fi
 	local mem_exists="`grep "^$groupname:[^:]*:[^:]*:\([^,]*,\)*$username\(,[^,]*\)*" $rootdir/etc/group || true`"
 	if test "x$mem_exists" = "x"; then
-		local count=0
-		while true; do
-			eval $PSEUDO groupmems $opts || true
-			mem_exists="`grep "^$groupname:[^:]*:[^:]*:\([^,]*,\)*$username\(,[^,]*\)*" $rootdir/etc/group || true`"
-			if test "x$mem_exists" = "x"; then
-				bbwarn "${PN}: groupmems command did not succeed. Retrying..."
-			else
-				break
-			fi
-			count=`expr $count + 1`
-			if test $count = $retries; then
-				if test "x$gshadow" = "xno"; then
-					rm -f $rootdir${sysconfdir}/gshadow
-					rm -f $rootdir${sysconfdir}/gshadow-
-				fi
-				bbfatal "${PN}: Tried running groupmems command $retries times without success, giving up"
-			fi
-			sleep $count
-		done
+		eval flock -x -w 100 $FLOCK_FILE -c \'$PSEUDO groupmems $opts\' || true
+		mem_exists="`grep "^$groupname:[^:]*:[^:]*:\([^,]*,\)*$username\(,[^,]*\)*" $rootdir/etc/group || true`"
+		if test "x$mem_exists" = "x"; then
+			bbfatal "${PN}: groupmems command did not succeed."
+		fi
 	else
 		bbnote "${PN}: group $groupname already contains $username, not re-adding it"
 	fi
@@ -115,26 +80,15 @@ perform_groupmems () {
 perform_groupdel () {
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing groupdel with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing groupdel with [$opts]"
 	local groupname=`echo "$opts" | awk '{ print $NF }'`
 	local group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
 	if test "x$group_exists" != "x"; then
-		local count=0
-		while true; do
-			eval $PSEUDO groupdel $opts || true
-			group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
-			if test "x$group_exists" != "x"; then
-				bbwarn "${PN}: groupdel command did not succeed. Retrying..."
-			else
-				break
-			fi
-			count=`expr $count + 1`
-			if test $count = $retries; then
-				bbfatal "${PN}: Tried running groupdel command $retries times without success, giving up"
-			fi
-			sleep $count
-		done
+		eval flock -x -w 100 $FLOCK_FILE -c \'$PSEUDO groupdel $opts\' || true
+		group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
+		if test "x$group_exists" != "x"; then
+			bbfatal "${PN}: groupdel command did not succeed."
+		fi
 	else
 		bbnote "${PN}: group $groupname doesn't exist, not removing it"
 	fi
@@ -143,26 +97,15 @@ perform_groupdel () {
 perform_userdel () {
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing userdel with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing userdel with [$opts]"
 	local username=`echo "$opts" | awk '{ print $NF }'`
 	local user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
 	if test "x$user_exists" != "x"; then
-	       local count=0
-	       while true; do
-		       eval $PSEUDO userdel $opts || true
-		       user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
-		       if test "x$user_exists" != "x"; then
-			       bbwarn "${PN}: userdel command did not succeed. Retrying..."
-		       else
-			       break
-		       fi
-		       count=`expr $count + 1`
-		       if test $count = $retries; then
-				bbfatal "${PN}: Tried running userdel command $retries times without success, giving up"
-		       fi
-		       sleep $count
-	       done
+		eval flock -x -w 100 $FLOCK_FILE -c \'$PSEUDO userdel $opts\' || true
+		user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
+		if test "x$user_exists" != "x"; then
+			bbfatal "${PN}: userdel command did not succeed."
+		fi
 	else
 		bbnote "${PN}: user $username doesn't exist, not removing it"
 	fi
@@ -174,25 +117,14 @@ perform_groupmod () {
 	set +e
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing groupmod with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing groupmod with [$opts]"
 	local groupname=`echo "$opts" | awk '{ print $NF }'`
 	local group_exists="`grep "^$groupname:" $rootdir/etc/group || true`"
 	if test "x$group_exists" != "x"; then
-		local count=0
-		while true; do
-			eval $PSEUDO groupmod $opts
-			if test $? != 0; then
-				bbwarn "${PN}: groupmod command did not succeed. Retrying..."
-			else
-				break
-			fi
-			count=`expr $count + 1`
-			if test $count = $retries; then
-				bbfatal "${PN}: Tried running groupmod command $retries times without success, giving up"
-			fi
-			sleep $count
-		done
+		eval flock -x -w 100 $FLOCK_FILE -c \'$PSEUDO groupmod $opts\'
+		if test $? != 0; then
+			bbwarn "${PN}: groupmod command did not succeed."
+		fi
 	else
 		bbwarn "${PN}: group $groupname doesn't exist, unable to modify it"
 	fi
@@ -204,25 +136,14 @@ perform_usermod () {
 	set +e
 	local rootdir="$1"
 	local opts="$2"
-	local retries="$3"
-	bbnote "${PN}: Performing usermod with [$opts] and $retries times of retry"
+	bbnote "${PN}: Performing usermod with [$opts]"
 	local username=`echo "$opts" | awk '{ print $NF }'`
 	local user_exists="`grep "^$username:" $rootdir/etc/passwd || true`"
 	if test "x$user_exists" != "x"; then
-	       local count=0
-	       while true; do
-		       eval $PSEUDO usermod $opts
-		       if test $? != 0; then
-			       bbwarn "${PN}: usermod command did not succeed. Retrying..."
-		       else
-			       break
-		       fi
-		       count=`expr $count + 1`
-		       if test $count = $retries; then
-				bbfatal "${PN}: Tried running usermod command $retries times without success, giving up"
-		       fi
-		       sleep $count
-	       done
+		eval flock -x -w 100 $FLOCK_FILE -c \'$PSEUDO usermod $opts\'
+		if test $? != 0; then
+			bbfatal "${PN}: usermod command did not succeed."
+		fi
 	else
 		bbwarn "${PN}: user $username doesn't exist, unable to modify it"
 	fi
-- 
2.7.0

Re: [PATCH 1/1] useradd_base.bbclass: fix simultaneous with flock

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On 16 February 2016 at 07:14, <kai.kang@windriver.com> wrote:

> +FLOCK_FILE=$SYSROOT/etc
>

useradd_base is also used by extrausers.bbclass, so it would be best if
useradd_base decided where to use as a lock ($rootdir/${sysconfdir} sounds
sensible) instead of expecting that the caller sets FLOCK_FILE and
mysteriously failing if it wasn't set.

Also if we're locking and waiting properly instead of using timeouts and
loops, can we remove the "does it exist?" check before the operation (eg
groupadd -f will exit 0 if the group exists) and after (with proper locking
that shouldn't be happening).

Ross

[-- Attachment #2: Type: text/html, Size: 1109 bytes --]

Re: [PATCH 1/1] useradd_base.bbclass: fix simultaneous with flock

[Kang Kai]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

On 2016年02月16日 20:09, Burton, Ross wrote:
>
> On 16 February 2016 at 07:14, <kai.kang@windriver.com 
> <mailto:kai.kang@windriver.com>> wrote:
>
>     +FLOCK_FILE=$SYSROOT/etc
>
>
> useradd_base is also used by extrausers.bbclass, so it would be best 
> if useradd_base decided where to use as a lock ($rootdir/${sysconfdir} 
> sounds sensible) instead of expecting that the caller sets FLOCK_FILE 
> and mysteriously failing if it wasn't set.

OK. Sounds good.

>
> Also if we're locking and waiting properly instead of using timeouts 
> and loops, can we remove the "does it exist?" check before the 
> operation (eg groupadd -f will exit 0 if the group exists) and after 
> (with proper locking that shouldn't be happening).

I prefer to keep such check. There is more than one case that causes 
operation error such as pass wrong parameters. The messages are helpful 
for developers.

Thanks,
Kai

>
> Ross


-- 
Regards,
Neil | Kai Kang


[-- Attachment #2: Type: text/html, Size: 2656 bytes --]

Re: [PATCH 1/1] useradd_base.bbclass: fix simultaneous with flock

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

On 17 February 2016 at 05:40, Kang Kai <Kai.Kang@windriver.com> wrote:

> I prefer to keep such check. There is more than one case that causes
> operation error such as pass wrong parameters. The messages are  helpful
> for developers.
>


In that case the groupadd will return an error code.  What situations with
proper locking will result in groupadd running successfully but the group
not being added?

Ross

[-- Attachment #2: Type: text/html, Size: 836 bytes --]

Re: [PATCH 1/1] useradd_base.bbclass: fix simultaneous with flock

[Kang Kai]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On 2016年02月18日 07:49, Burton, Ross wrote:
>
> On 17 February 2016 at 05:40, Kang Kai <Kai.Kang@windriver.com 
> <mailto:Kai.Kang@windriver.com>> wrote:
>
>     I prefer to keep such check. There is more than one case that
>     causes operation error such as pass wrong parameters. The messages
>     are  helpful for developers.
>
>
> In that case the groupadd will return an error code.  What situations 
> with proper locking will result in groupadd running successfully but 
> the group not being added?

Sorry for late reply.

I think I understand your meanings. V3 will be sent.

--Kai


>
> Ross


-- 
Regards,
Neil | Kai Kang


[-- Attachment #2: Type: text/html, Size: 2065 bytes --]