Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

readlink -e is not portable and fails with busybox #32

Open
jameshilliard opened this issue Feb 22, 2020 · 13 comments
Open

readlink -e is not portable and fails with busybox #32

jameshilliard opened this issue Feb 22, 2020 · 13 comments

Comments

@jameshilliard
Copy link

When calling e2scrub_all on a system using busybox the readlink command fails due to busybox's readlink not supporting the -e option. See here for details.

if ! readlink -q -s -e /dev/mapper/*.e2scrub* > /dev/null; then

# SERVICE_MODE=1 /sbin/e2scrub_all -A -r
readlink: invalid option -- 'e'
BusyBox v1.31.1 (2020-02-20 16:09:26 MST) multi-call binary.

Usage: readlink [-fnv] FILE

Display the value of a symlink

	-f	Canonicalize by following all symlinks
	-n	Don't add newline
	-v	Verbose
@tytso
Copy link
Owner

tytso commented Feb 22, 2020

I consider this more a bug for busybox than e2scrub_all. It will be less work to fix busybox to support readlink -e than to workaround the failure of busybox to support readlink -e.

@jameshilliard
Copy link
Author

@tytso Maybe there is an alternative command that does the same thing but is portable with busybox, would realpath work here at all?

@tytso
Copy link
Owner

tytso commented Mar 21, 2020

Nope, busybox's realpath is not a complete replacement of readlink -q -s -e. On what distribution are we trying to have the insane combination of (a) busybox and (b) trying to use e2scrub?

Maybe what we can do is to have e2scrub check for busybox and just have it fail with a "insane system configuration"? :-)

@jameshilliard
Copy link
Author

On what distribution are we trying to have the insane combination of (a) busybox and (b) trying to use e2scrub?

I was running into this issue with buildroot.

@jameshilliard
Copy link
Author

Maybe for portability it would make sense to just rewrite e2scrub_all to be a small helper binary as opposed to a shell script.

@tytso
Copy link
Owner

tytso commented Mar 21, 2020

Send me a patch and I'll consider it.

I'm quite sure that a patch to fix busybox so it supports readlink -e is going to be much easier.

@tytso
Copy link
Owner

tytso commented Mar 21, 2020

Buildroot is building X.org. So why the heck are you trying to make e2scrub_all work with busybox? What is your use case where you are insisting on using busybox and also expect e2scrub_all to work? If you want this, you're going to have to supply the patch.

@jameshilliard
Copy link
Author

Buildroot is building X.org.

How is X.org related to e2scrub_all?

So why the heck are you trying to make e2scrub_all work with busybox?

It's being invoked by this systemd service on startup, is that a bad idea?

What is your use case where you are insisting on using busybox and also expect e2scrub_all to work?

Busybox is very common on embedded systems in general due to its small size. Isn't e2scrub_all supposed to be ran on startup on systems with ext4 filesystems to check for corruption?

@jameshilliard
Copy link
Author

What's the point of the -e option anyways here? I'm not seeing how it affects the exit code at all, can we not just use readlink -q -s /dev/mapper/*.e2scrub* instead?

@tytso
Copy link
Owner

tytso commented Mar 21, 2020

readlink -e will not return any text if the symlink points to a non-existent file:

% ln -s this-does-not-exist test-file
% readlink -e test-file
% readlink test-file
this-does-not-exist

In answer to your question, e2scrub_all is optionally run periodically, if configured in /etc/e2scrub.conf, to take a LVM snapshot of a LV's containing ext[234] file systems so that fsck can be run on the snapshot. It is designed for data server systems that are up for months or years, and where it is considered desirable (because there is critical data on the file system) to run periodic file system checks without needing to do a reboot. e2scrub_all is also run (with different command line options) on a reboot to clean up any stale LVM snapshots which might been left over after the system crashes in the middle of an e2scrub run --- either one run manually by running e2scrub, or an automated, periodic run using the systemd timer unit.

If you are on an embedded device, and you don't have LVM devices, there's nothing which says you have to enable the systemd services. My point about buildroot supporting X.org is that if you are supporting huge packages like X, why the hell are you caring about trying to boot systems using busybox. I guess the answer is you are trying to support everything? It sounds like to me you have three options.

  1. Fix busybox so it supports readlink -e
  2. Unconditionally suppress e2scrub's systemd units on buildroot
  3. Only enable e2scrub's systemd units if you are not using busybox, and are properly using coreutils.

@jameshilliard
Copy link
Author

readlink -e will not return any text if the symlink points to a non-existent file

But we don't care about text at all, only exit code right? I guess readlink -e also affects the exit code?

It is designed for data server systems that are up for months or years, and where it is considered desirable (because there is critical data on the file system) to run periodic file system checks without needing to do a reboot.

Buildroot is often used with long life systems with long uptime as well(common for embedded systems).

My point about buildroot supporting X.org is that if you are supporting huge packages like X, why the hell are you caring about trying to boot systems using busybox.

Buildroot is a cross compilation environment designed to be extremely customizable so that it can be used on an extremely wide variety of hardware, everything from from business cards to high end x86_64 server hardware. X.org like most other buildroot packages is entirely optional, my buildroot firmware builds for example do not use X.org.

  1. Only enable e2scrub's systemd units if you are not using busybox, and are properly using coreutils.

I guess for now this probably makes the most sense.

The correct way to suppress systemd service installation is to just pass --with-systemd-unit-dir=no to configure right? I can just submit a patch to buildroot suppressing systemd service installation for e2fsprogs when any of the following BR2_PACKAGE_LVM2(lvcreate), BR2_PACKAGE_BASH(mapfile) BR2_PACKAGE_UTIL_LINUX_BINARIES(lsblk) and BR2_PACKAGE_COREUTILS(readlink) is not selected to be installed.

@tytso
Copy link
Owner

tytso commented Sep 17, 2020

readlink -e will not return any text if the symlink points to a non-existent file

But we don't care about text at all, only exit code right? I guess readlink -e also affects the exit code?

Yes, that's correct.

@tianyuanhao Some embedded systems will consider tune2fs, e2label, debugfs, etc., similarly useless. Typically the packaging scripts will simply choose not to install those binaries / shell scripts / man pages that aren't useful for that particular packaging use case. I don't see what the value would be to adding Yet Another Configure Option for that particular purpose.

But as I've said before, if you really want a feature (and I consider this either a feature request for e2fsprogs, or a bug report for busybox) feel free to send me a patch, and I'll consider it. Trying to integrate random pieces of software together (and busybox is way more random than most) is a distribution responsibility, not an upstream maintainer responsibility.

@jameshilliard
Copy link
Author

@tianyuanhao Not sure we want to unconditionally disable it when it should be possible make the install conditional on its dependencies being present.

The following e2fsprogs configure options seem to be what we should be using to control installation based on what packages and init system are configured in buildroot:

--with-crond-dir=no
--with-udev-rules-dir=no
--with-systemd-unit-dir=no

In general we like to override automatic configure options anyways, at least those that have behavior which varies based on other optional packages as we want their installation to be deterministic and not vary depending on package installation order, configure options that require other packages at install time should have those other packages added as dependencies to e2fsprogs in buildroot if they are selected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants