| Message ID | 20171217183042.24471-1-roman@advem.lv |
|---|---|
| State | Accepted |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [LEDE-DEV,1/3] busybox: enable flock by default | expand |
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@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
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@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. Adding flock and procd_lock is intended to replace all init custom locks and others. (and fix the racing problem once and for all) Regards, Roman
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@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. 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. > 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: 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. Mathias
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@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
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
This is needed for procd init script protection to work. flock adds 4248 bytes to stripped busybox binary. Signed-off-by: Roman Yeryomin <roman@advem.lv> --- package/utils/busybox/Config-defaults.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)