dracut-install cleanup #1796

nabijaczleweli · 2022-04-27T17:47:57Z

Changes

See individual commit messages; draft because on top of #1794

marcosfrm · 2022-04-29T19:21:03Z

src/install/dracut-install.c

-
-        realpath_p = realpath(target_dir_p, NULL);
-        if (realpath_p == NULL) {
+        if (realpath(target_dir_p, relative_from) == NULL) {


relative_from as a temporary buffer to build realfrom and realtarget is fine, but its name is confusing. Maybe rename it to buf or whatever? (comments below will need update in this case)

stale · 2022-05-30T21:16:58Z

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

nabijaczleweli · 2022-05-30T21:30:03Z

bad bot

nabijaczleweli · 2022-06-09T23:54:38Z

rebased

marcosfrm · 2022-06-14T23:48:47Z

src/install/dracut-install.c

+        if (vfork() == 0) {
+                dup2(fds[1], 1);
+                dup2(fds[1], 2);
+                execlp(ldd, ldd, fullsrcpath, (char *)NULL);


This will break at least the "command not found" case, that will be considered successful (ret = 0). However current popen() code is also broken for non-english locales: sh and ldd output is internationalized and error messages do not match.

Wait, this actually depends on the "command not found" bash output? sh -c obviously outputs sh: 1: ldd: not found, like it has for the past 30+ years. Wild. Added a "command not found" write to compensate.

The latter fixed by setting LC_ALL=C.

I wonder if checking wstatus could simplify things, like:

pid_t pid = fork(); if (pid == 0) { dup2(fds[1], 1); dup2(fds[1], 2); putenv("LC_ALL=C"); execlp(ldd, ldd, fullsrcpath, (char *)NULL); _exit(EXIT_FAILURE); } close(fds[1]); while (waitpid(pid, &ret, 0) < 0) { if (errno != EINTR) { ret = 1; break; } } if (ret != 0) { log_error("ERROR: failed to run '%s %s'", ldd, fullsrcpath); return 1; } while (getline(&buf, &linesize, fptr) >= 0) { ...

(dropped err)

Then clean up strstr() calls below, removing the ones which correspond to unclean exit codes.

well, there's three strings

"you do not have execution permission"
"unable to find sysroot"
"command not found"

The last one is obviously from bash, but (my) ldd (or ld.so) doesn't contain the other two. Are they all foldable together to $? != 0?

"you do not have execution permission" is generated by ldd itself:

$ cp -p /usr/bin/true . $ chmod -x true $ LANG=C ldd true ldd: warning: you do not have execution permission for `./true' linux-vdso.so.1 (0x00007ffe08fd3000) libc.so.6 => /lib64/libc.so.6 (0x00007fe9f8a28000) /lib64/ld-linux-x86-64.so.2 (0x00007fe9f8c38000)

The exit code is 0... maybe ignore the warning? Dunno.

"unable to find sysroot" appears to come from https://github.com/crosstool-ng/crosstool-ng/blob/8fa98eeeff9bc53478d97ef722f366fea151ae64/scripts/xldd.in#L170 , so it must stay I think.

Oddly, I don't see it on bullseye:

$ cp /bin/rm . $ chmod -x rm $ ldd ./rm linux-vdso.so.1 (0x00007ffdef1d4000) libselinux.so.1 => /lib/x86_64-linux-gnu/libselinux.so.1 (0x00007f70fc5fb000) libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f70fc42e000) libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f70fc2ea000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f70fc125000) libpcre2-8.so.0 => /lib/x86_64-linux-gnu/libpcre2-8.so.0 (0x00007f70fc08d000) libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f70fc087000) /lib64/ld-linux-x86-64.so.2 (0x00007f70fc649000) libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f70fc06b000) libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f70fc049000)

And grepping through ldd there's a "you have no read" but no "you have no execute".

Would love a hard opinion on this.

I am on Fedora 36 here btw. "you do not have execution permission" was added in

b4dc22c

hence it must stay too.

Or, if the noexec case makes ldd exit code != 0, then drop it. Needs testing.

Turns out this is a Debian patch, all/local-ldd.diff:

- test -x "$file" || echo 'ldd:' $"\ -warning: you do not have execution permission for" "\`$file'" >&2

Applied != 0 => ret += 1; retained all except for "command not found"

marcosfrm · 2022-06-16T02:32:37Z

src/install/dracut-install.c

-        wait(&err);
+        while (waitpid(ldd_pid, &err, 0) == -1) {
+            if (errno != EINTR) {
+                if (err != 0) {


waitpid() returns the terminated pid (> 0) when sucessfull, but its exit status still can be abnormal: non-zero exit code, killed by signal... hence err != 0 check and error message print need to be outside the loop[*]. If waitpid() < 0 and errno != EINTR things are broken and we must assume everything failed: err in this case cannot be trusted (I think most implementations do not change it, not sure). Set it to anything non-zero.

write(2, "command not found", ...) is not needed. Also, to print meaningful exit codes/signals in the error message, it would need WIFEXITED, WEXITSTATUS, WIFSIGNALED, WTERMSIG, macros. Overkill in my opinion.

[*] cp() function (normal_copy label) is buggy in this regard too, because those log_error() will not run in case of normal errors, when waitpid() succeeds (missing cp binary, etc).

I was clearly in no state to be writing this last night, because this is an obvious error; I've fixed this and cp.

…nstall

marcosfrm · 2022-08-13T22:52:38Z

LGTM

marcosfrm · 2022-09-07T12:03:55Z

Can this go in to get more testing?

stale · 2022-10-08T06:34:24Z

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

nabijaczleweli · 2022-10-08T11:08:59Z

bad bot

stale · 2022-11-09T05:46:10Z

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

marcosfrm · 2022-11-11T10:16:05Z

ping

nabijaczleweli · 2022-11-11T12:34:22Z

pong. looks fine on my end?

stale · 2022-12-11T14:11:18Z

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

nabijaczleweli · 2022-12-11T14:16:50Z

so true, bestie

LaszloGombos · 2022-12-11T15:51:00Z

Personally, I would prefer having #1987 around to help out for these reviews. Not sure if someone there is a way to do a run just for this PR.

johannbg · 2022-12-21T16:36:43Z

unresolved conversation and un-requested re-reviews ( @marcosfrm acked in comment section not in the review process ) has the tendency to get pr ignored from those of us that are quite busy anyways let's not merge this until in 059 milestone

aafeijoo-suse

Everything I was able to check and test went well, so let's merge this.

nabijaczleweli requested review from haraldh, danimo and johannbg as code owners April 27, 2022 17:47

nabijaczleweli force-pushed the clean branch from 7b42a90 to 1fd4bd2 Compare April 27, 2022 17:59

nabijaczleweli marked this pull request as draft April 27, 2022 18:00

github-actions bot added the modules Issue tracker for all modules label Apr 27, 2022

nabijaczleweli force-pushed the clean branch 3 times, most recently from 63c7bf4 to fe30eb0 Compare April 27, 2022 21:52

marcosfrm mentioned this pull request Apr 28, 2022

feat(install): better relative symlinks #1788

Closed

marcosfrm reviewed Apr 29, 2022

View reviewed changes

nabijaczleweli force-pushed the clean branch from ee6bcec to 09bd6f6 Compare April 29, 2022 19:37

stale bot added the stale communication is stuck label May 30, 2022

stale bot removed the stale communication is stuck label May 30, 2022

nabijaczleweli force-pushed the clean branch from 09bd6f6 to 09a865c Compare June 9, 2022 23:54

nabijaczleweli marked this pull request as ready for review June 9, 2022 23:54

github-actions bot removed the modules Issue tracker for all modules label Jun 9, 2022

marcosfrm reviewed Jun 14, 2022

View reviewed changes

nabijaczleweli force-pushed the clean branch 4 times, most recently from 60a6afc to e5d9df8 Compare June 15, 2022 20:21

marcosfrm reviewed Jun 16, 2022

View reviewed changes

nabijaczleweli force-pushed the clean branch 4 times, most recently from 7943cd0 to bb96d3d Compare June 16, 2022 16:39

nabijaczleweli added 7 commits August 13, 2022 14:11

chore: remove src/install/hashmap.lo

2d30251

perf(dracut-install): multiple single-character strstr()s -> strpbrk()

6b1062f

perf(dracut-install): stat() w/unused buf -> access(F_OK) in dracut-i…

b9bdd79

…nstall

perf(dracut-install): don't strdup() excessively for dracut_install()

2b95e30

perf(dracut-install): don't reallocate {src,dst}path in hmac_install()

d86145b

perf(dracut-install): don't strdup() environment block

832bead

refactor(dracut-install): strerror(errno) -> %m

13d9399

nabijaczleweli force-pushed the clean branch from 4563643 to 13d9399 Compare August 13, 2022 12:11

stale bot added the stale communication is stuck label Oct 8, 2022

stale bot removed the stale communication is stuck label Oct 8, 2022

stale bot added the stale communication is stuck label Nov 9, 2022

stale bot removed the stale communication is stuck label Nov 11, 2022

stale bot added the stale communication is stuck label Dec 11, 2022

stale bot removed the stale communication is stuck label Dec 11, 2022

johannbg approved these changes Dec 21, 2022

View reviewed changes

johannbg added this to the dracut-059 milestone Dec 21, 2022

aafeijoo-suse added a commit to aafeijoo-suse/dracut that referenced this pull request Dec 28, 2022

https://github.com/dracutdevs/dracut/pull/1796

47fd601

aafeijoo-suse approved these changes Dec 28, 2022

View reviewed changes

aafeijoo-suse merged commit 94dcac7 into dracutdevs:master Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dracut-install cleanup #1796

dracut-install cleanup #1796

nabijaczleweli commented Apr 27, 2022 •

edited

Loading

marcosfrm Apr 29, 2022

stale bot commented May 30, 2022

nabijaczleweli commented May 30, 2022

nabijaczleweli commented Jun 9, 2022

marcosfrm Jun 14, 2022 •

edited

Loading

nabijaczleweli Jun 15, 2022

marcosfrm Jun 15, 2022

nabijaczleweli Jun 15, 2022

marcosfrm Jun 15, 2022

nabijaczleweli Jun 15, 2022

marcosfrm Jun 15, 2022

marcosfrm Jun 15, 2022

nabijaczleweli Jun 15, 2022

marcosfrm Jun 16, 2022

nabijaczleweli Jun 16, 2022

marcosfrm commented Aug 13, 2022

marcosfrm commented Sep 7, 2022

stale bot commented Oct 8, 2022

nabijaczleweli commented Oct 8, 2022

stale bot commented Nov 9, 2022

marcosfrm commented Nov 11, 2022

nabijaczleweli commented Nov 11, 2022

stale bot commented Dec 11, 2022

nabijaczleweli commented Dec 11, 2022

LaszloGombos commented Dec 11, 2022

johannbg commented Dec 21, 2022

aafeijoo-suse left a comment

dracut-install cleanup #1796

dracut-install cleanup #1796

Conversation

nabijaczleweli commented Apr 27, 2022 • edited Loading

Changes

Choose a reason for hiding this comment

stale bot commented May 30, 2022

nabijaczleweli commented May 30, 2022

nabijaczleweli commented Jun 9, 2022

marcosfrm Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosfrm commented Aug 13, 2022

marcosfrm commented Sep 7, 2022

stale bot commented Oct 8, 2022

nabijaczleweli commented Oct 8, 2022

stale bot commented Nov 9, 2022

marcosfrm commented Nov 11, 2022

nabijaczleweli commented Nov 11, 2022

stale bot commented Dec 11, 2022

nabijaczleweli commented Dec 11, 2022

LaszloGombos commented Dec 11, 2022

johannbg commented Dec 21, 2022

aafeijoo-suse left a comment

Choose a reason for hiding this comment

nabijaczleweli commented Apr 27, 2022 •

edited

Loading

marcosfrm Jun 14, 2022 •

edited

Loading