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

[jhuang0 <jackie.huang@windriver.com>]

On 7/23/2012 6:09 PM, Richard Purdie wrote:
> 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.

Hi Richard,

This patch you replied is the first one I sent with whitespace issues, I 
didn't change the function update_useradd_package.

>
> 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.

You're right, I didn't think about that it will be script running on the 
target device and I just tested for tow different contexts:
1) Before do_install
2) At do_populate_sysroot_setscene when installing from sstate packages

>
> python functions should also be 4 space indented.
>

I will change with 4 space indent.

Thanks,
Jackie


> Cheers,
>
> Richard
>
>
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
>

-- 
Jackie Huang
WIND RIVER | China Development Center
MSN:jackielily@hotmail.com
Tel: +86 8477 8594
Mobile: +86 138 1027 4745