Re: [PATCH 1/1] useradd.bbclass: use locking of bb.utils to avoid lock race issue of useradd/groupadd

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Sun, 2012-07-22 at 14:53 +0800, jackie.huang@windriver.com wrote:
> From: Jackie Huang <jackie.huang@windriver.com>
> 
> A race condition can occur when adding users and groups to the
> passwd and group files, in [YOCTO #1794], 10 times retry added
> but it is not fixed completely.
> 
> This fix re-writes the useradd_preinst and useradd_sysroot with
> python and use locking of bb.utils to lock the passwd and group
> files before executing useradd/groupadd commands to avoid the
> lock race themselves.
> 
> [YOCTO #2779]
> 
> Signed-off-by: Jackie Huang <jackie.huang@windriver.com>
> ---
>  meta/classes/useradd.bbclass |  284 ++++++++++++++++++------------------------
>  1 files changed, 124 insertions(+), 160 deletions(-)
> 
> diff --git a/meta/classes/useradd.bbclass b/meta/classes/useradd.bbclass
> index bb8f42b..ed5ed69 100644
> --- a/meta/classes/useradd.bbclass
> +++ b/meta/classes/useradd.bbclass
> @@ -14,126 +14,90 @@ USERADDDEPENDS_virtclass-nativesdk = ""
>  # c) As the preinst script in the target package at do_rootfs time
>  # d) As the preinst script in the target package on device as a package upgrade
>  #
> -useradd_preinst () {
> -OPT=""
> -SYSROOT=""
> -
> -if test "x$D" != "x"; then

[...]

> -	done
> -fi
> +def useradd_preinst(d):
> +	import re
> +

[...]


> -    # Add the user/group preinstall scripts and RDEPENDS requirements
> -    # to packages specified by USERADD_PACKAGES
> -    if not bb.data.inherits_class('nativesdk', d):
> -        useradd_packages = d.getVar('USERADD_PACKAGES', True) or ""
> -        for pkg in useradd_packages.split():
> -            update_useradd_package(pkg)
> +	def update_useradd_package(pkg):
> +		bb.debug(1, 'adding user/group calls to preinst for %s' % pkg)
> +
> +		"""
> +		useradd preinst is appended here because pkg_preinst may be
> +		required to execute on the target. Not doing so may cause
> +		useradd preinst to be invoked twice, causing unwanted warnings.
> +		"""
> +		preinst = d.getVar('pkg_preinst_%s' % pkg, True) or d.getVar('pkg_preinst', True)
> +		if not preinst:
> +			preinst = '#!/bin/sh\n'
> +		preinst += d.getVar('useradd_preinst', True)


This looks like we're adding the contents of the useradd_preinst
function (changed from shell to python) to a script headed with
"#!/bin/sh"? Python script with a /bin/sh header isn't a good idea.

We can't really depend on python being on the target device to add users
in the postinstall script either.

So in summary, this patch needs a lot more thought and hasn't been well
tested.

python functions should also be 4 space indented.

Cheers,

Richard