[LEDE-DEV] [PATCH 1/3] busybox: enable flock by default

[Roman Yeryomin]

On 2017-12-18 20:47, Mathias Kresin wrote:
> 18.12.2017 10:48, Roman Yeryomin:
>> On 2017-12-18 09:58, Mathias Kresin wrote:
>>> 17.12.2017 19:30, Roman Yeryomin:
>>>> This is needed for procd init script protection to work.
>>>> flock adds 4248 bytes to stripped busybox binary.
>>>> 
>>>> Signed-off-by: Roman Yeryomin <roman at advem.lv>
>>>> ---
>>>>   package/utils/busybox/Config-defaults.in | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/package/utils/busybox/Config-defaults.in
>>>> b/package/utils/busybox/Config-defaults.in
>>>> index 2a8d9dd397..6fc5093055 100644
>>>> --- a/package/utils/busybox/Config-defaults.in
>>>> +++ b/package/utils/busybox/Config-defaults.in
>>>> @@ -1497,7 +1497,7 @@ config BUSYBOX_DEFAULT_FINDFS
>>>>       default n
>>>>   config BUSYBOX_DEFAULT_FLOCK
>>>>       bool
>>>> -    default n
>>>> +    default y
>>>>   config BUSYBOX_DEFAULT_FDFLUSH
>>>>       bool
>>>>       default n
>>>> 
>>> 
>>> We have a custom (f)lock command in LEDE [0], which is used for
>>> example during wireless detect.
>>> 
>>> I only had a brief lock at your patch series, but the existing lock
>>> command might do the job.
>>> 
>>> I've no idea why we have a custom lock command instead of using the
>>> busybox provided flock. But shipping two (f)lock implementations at
>>> the same time seem to be a waste of flash space.
>>> 
>>> Mathias
>>> 
>>> [0]
>>> https://git.lede-project.org/?p=source.git;a=blob;f=package/utils/busybox/patches/220-add_lock_util.patch;hb=60a39e8f5af7ed710c5c62b131fd9df6519b64e4
>>> 
>> 
>> 
>> I think we should use stock flock instead of patching and adding 
>> another
>> custom wheel.
>> If something doesn't work in stock flock we should fix that and send 
>> the
>> fix upstream.
> 
> I had a closer look at the commit history to understand why we have a
> custom (f)lock implementation. It is as simple as our custom lock dates
> around 2006, while the busybox flock applet was added 2010.

oh, I see, thanks for looking into this

> At least I'm fine to switch to the upstream flock. As implied by using
> the word "switch", our custom lock applet should be removed at the same
> time.

I would insist on "switch" to be done as different patch set, since this 
one main purpose is slightly different.
I can submit that.

>> Adding flock and procd_lock is intended to replace all init custom 
>> locks
>> and others.
>> (and fix the racing problem once and for all)
> 
> Locks aren't used only for init scripts. Within the packages directory
> the lock applet is used in:

I would say in current condition, in init scripts, locks are not used at 
all, only some workarounds.

> package/network/config/ltq-vdsl-app/files/dsl_cpe_pipe.sh
> package/base-files/files/lib/preinit/30_failsafe_wait
> package/base-files/files/lib/preinit/40_run_failsafe_hook
> package/base-files/files/sbin/sysupgrade
> 
> Most likely there are more scripts using lock.

Good to know, thanks.


Regards,
Roman