Re: [PATCH v2] meta: refactored flock usage

[Claudius Heine <claudius.heine.ext@siemens.com>]

On 12/03/2019 11.20, Jan Kiszka wrote:
> On 09.03.19 20:08, Claudius Heine wrote:
>> Hi Jan,
>>
>> Quoting Jan Kiszka (2019-03-08 18:49:48)
>>> On 14.02.19 17:08, [ext] claudius.heine.ext@siemens.com wrote:
>>>> From: Claudius Heine <ch@denx.de>
>>>>
>>>> Currently much care has to be taken in order to correctly escape 
>>>> strings
>>>> inside flock commands. And there is also on instance where this was
>>>> incorrectly used (isar-bootstrap.inc).
>>>>
>>>> The usage of flock was changed to no longer require single or double
>>>> ticks. Instead commands are run inside a subshell.
>>>>
>>>> Signed-off-by: Claudius Heine <ch@denx.de>
>>>> ---
>>>>    meta/classes/buildchroot.bbclass                    |  6 ++++--
>>>>    meta/classes/wic-img.bbclass                        |  6 ++++--
>>>>    meta/recipes-core/isar-bootstrap/isar-bootstrap.inc | 10 +++++++---
>>>>    3 files changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/meta/classes/buildchroot.bbclass 
>>>> b/meta/classes/buildchroot.bbclass
>>>> index 0d4ff4e..c017b25 100644
>>>> --- a/meta/classes/buildchroot.bbclass
>>>> +++ b/meta/classes/buildchroot.bbclass
>>>> @@ -22,7 +22,8 @@ python __anonymous() {
>>>>    MOUNT_LOCKFILE = "${BUILDCHROOT_DIR}/mount.lock"
>>>>    buildchroot_do_mounts() {
>>>> -    sudo flock ${MOUNT_LOCKFILE} -c ' \
>>>> +    sudo -s <<'EOSUDO'
>>>> +        ( flock 9
>>>>            set -e
>>>>            if ! grep -q ${BUILDCHROOT_DIR}/isar-apt /proc/mounts; then
>>>>                mount --bind ${REPO_ISAR_DIR}/${DISTRO} 
>>>> ${BUILDCHROOT_DIR}/isar-apt
>>>> @@ -36,5 +37,6 @@ buildchroot_do_mounts() {
>>>>            # Refresh /etc/resolv.conf at this chance
>>>>            cp -L /etc/resolv.conf ${BUILDCHROOT_DIR}/etc
>>>> -        '
>>>> +        ) 9>${MOUNT_LOCKFILE}
>>>> +EOSUDO
>>>>    }
>>>> diff --git a/meta/classes/wic-img.bbclass 
>>>> b/meta/classes/wic-img.bbclass
>>>> index 76602d8..5367a37 100644
>>>> --- a/meta/classes/wic-img.bbclass
>>>> +++ b/meta/classes/wic-img.bbclass
>>>> @@ -87,14 +87,16 @@ do_build[stamp-extra-info] = 
>>>> "${DISTRO}-${DISTRO_ARCH}"
>>>>    do_wic_image() {
>>>>        buildchroot_do_mounts
>>>> -    sudo flock ${MOUNT_LOCKFILE} -c ' \
>>>> +    sudo -s <<'EOSUDO'
>>>> +        ( flock 9
>>>>            for dir in ${BBLAYERS} ${STAGING_DIR} 
>>>> ${ISARROOT}/scripts; do
>>>>                mkdir -p ${BUILDCHROOT_DIR}/$dir
>>>>                if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 
>>>> 2>&1; then
>>>>                    mount --bind --make-private $dir 
>>>> ${BUILDCHROOT_DIR}/$dir
>>>>                fi
>>>>            done
>>>> -        '
>>>> +        ) 9>${MOUNT_LOCKFILE}
>>>> +EOSUDO
>>>>        export FAKEROOTCMD=${FAKEROOTCMD}
>>>>        export BUILDDIR=${BUILDDIR}
>>>>        export MTOOLS_SKIP_CHECK=1
>>>> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc 
>>>> b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
>>>> index 234d339..9c82184 100644
>>>> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
>>>> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
>>>> @@ -207,14 +207,16 @@ isar_bootstrap() {
>>>>            esac
>>>>            shift
>>>>        done
>>>> -    debootstrap_args="--verbose --variant=minbase 
>>>> --include='${DISTRO_BOOTSTRAP_BASE_PACKAGES}'"
>>>> +    debootstrap_args="--verbose --variant=minbase 
>>>> --include=${DISTRO_BOOTSTRAP_BASE_PACKAGES}"
>>>
>>> Are you sure that the removal of '' is correct here as well? What 
>>> binds that
>>> package list to --include=?
>>
>> Well the package list should be ',' seperated otherwise debootstrap
>> would not accept it. With my change it would look at "'locales'" (with
>> ') and would not find that package, since it does not exist. Having
>> those ' there is pretty misleading and wrong.
> 
> But the package list may contain spaces as well, no?

debootstrap manpage:

        --include=alpha,beta
               Comma separated list of packages which will be added to
               download  and  extract lists.


If it does, than that is AFAIK undocumented and should not be done.

Claudius

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de