Clean up the addition of extra directories and/or files #125

weiny2 · 2025-03-10T20:54:51Z

The ability to add random files and directories to the root image had previously been hacked in via a secret 'special' directory location. This functionality is nice but lacking for a couple of reasons.

It is undocumented so even the original author forgot about its existence
It provides little flexibility

Remove the existing hidden feature and add --extra-dirs as a parameter which can specify a list of directories (or files) to be added to /root within the image.

This functionality was discussed in #124

This does not remove or clean up the rsync's but should make one of it's uses much more clear.

To ensure explicit usage for the user no default is provided. But directories will not be removed without a -r wipe.

The rsync'ing of ~/git/extra-scripts was hacked in before run_qemu was becoming a wider distributed tool. As such it is undocumented and could potentially cause odd or unexpected behavior. The addition of the user-extras provides the same functionality with more user control. Including being explicit for the users benefit. Remove extra-scripts rsync'ing. Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Depending on the use case run_qemu may need additional user space software. ndctl was specifically included and built within the root image due to the initial run_qemu use case to test pmem/dax/cxl. One option would be to hard code the addition of each user space source tree similar to ndctl. This is not desirable for a few reasons. 1) Additional source trees will start to bloat the root image. 2) ndctl was added with very specific directory locations. Some users have worked around this with symlinks but it is inflexible. 3) making source trees optional bloats the parameter space for run_qemu 4) building each user package is very specific to each package A better alternative is to allow the user to specify additional source trees; in different locations. This allows different kernel/user space source combinations in whatever combination the user requires at a specific time. Add the extra-dirs option. The extra-dirs option specifies a file containing a list of paths to user source trees. Each of these source trees is installed in the /root home directory of the guest image. Building of source trees is left to the user. Signed-off-by: Ira Weiny <ira.weiny@intel.com>

marc-hb · 2025-03-10T21:58:38Z

run_qemu.sh

+			fail "$extra_dirs not found"
+		fi
+		echo "Installing extra directories and files from: $extra_dirs"
+		for d in `cat "$extra_dirs"`; do


https://www.shellcheck.net/wiki/SC2006

shellcheck -x run_qemu.sh

Don't forget the -x otherwise there is a ton of false positives.

I always forget about shellcheck... I'll try it.

Coding shell scripts without running shellcheck is now a crime :-)

FWIW there are a few other warnings we should clean up if we are going to make that a requirement.

Absolutely! But shellcheck can catch codestyle issues and sometimes bugs even long before it is requirement :-) This only requires keep the number of warnings to a reasonable level not to drown the new ones.

marc-hb · 2025-03-10T21:59:42Z

run_qemu.sh

+{
+	local p=$1
+
+	p=$(eval echo $p)


What does this line do?

This really sent me down a rabit hole which I could not resolve any other way.

If you don't do the eval on the $arg* variable any shell expansion will not happen. So without that eval this:

--extra-dirs=~/foo

Will not resolve to your home dir. I tried for a while to get it to work. For some reason this only happens with the argbash variables and not with other variables defined in the script. Running eval made the shell expansion work as expected. So I went with it.

OK, I missed that shell expansion was an intended feature. It's really not a small thing that should be found by chance in line 155 and nowhere else.

Hm I don't love the eval.. (also is the echo redundant anyway?)

Does something like this work?

paths=($_arg_extra_dirs) for paths in ${paths[@]}; do <stuff> done

marc-hb · 2025-03-10T22:04:33Z

run_qemu.sh

@@ -144,6 +144,21 @@ distro_vars="${script_dir}/${_distro}_vars.sh"

 pushd "$_arg_working_dir" > /dev/null || fail "couldn't cd to $_arg_working_dir"

+# save the working directory
+working_dir=$(pwd)


Why save it again? It's already in "$_arg_working_dir"

pwd guarentees we have that directory's full path after doing the push where ever _arg_working_dir was.

FWIW AFAIK no one uses _arg_working_dir. So it is probably almost untested. But this seemed safe.

Ah yes of course. Then please:

Suggested change

working_dir=$(pwd)

abs_working_dir=$(pwd)

Please also drop or change the comment, it threw me off.

marc-hb · 2025-03-10T22:07:35Z

parser_generator.m4

@@ -34,6 +34,7 @@ exit 11  #)Created by argbash-init v2.9.0
 # ARG_OPTIONAL_BOOLEAN([debug], [v], [Debug script problems (enables set -x)], )
 # ARG_OPTIONAL_BOOLEAN([ndctl-build], , [Enable ndctl build in root image], [on])
 # ARG_OPTIONAL_BOOLEAN([kern-selftests], , [Enable kernel selftest build in root image (Warning: This option can take a long time and requires many support packages on the host; including some 32 bit)], [off])
+# ARG_OPTIONAL_SINGLE([extra-dirs], , [Specify a file with a list of directories (or files) to be copied to the /root directory within the image.], [])


You need to say that:

Relative directories inside that file are relative to --working-dir (which defaults to .)

symbolic links are resolved (NOT recursively like rsync -L does, but that's impossible to summarize clearly and fairly intuitive so don't say "not recursively")

I was trying to keep the help small. But yea it is probably worth noting that any relative paths are to the working directory. I'll update it.

Also: shell expansion that I mostly miss in the first review. It's really not a small thing.

marc-hb · 2025-03-10T22:07:58Z

run_qemu.sh

 	if [[ $_arg_ndctl_build == "on" ]]; then
 		if [ -n "$ndctl" ]; then
 			rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl
 			prepare_ndctl_build # create mkosi.postinst which compiles
 		fi
 	fi

+	if [[ "$_arg_extra_dirs" ]]; then


Suggested change

if [[ "$_arg_extra_dirs" ]]; then

if [[ -n "$_arg_extra_dirs" ]]; then

No we are not checking for the file here. See comment above about the lack of shell expansion for _arg_extra_dirs. This check is simply for the value being set. If it is set we must canonicalize it before we can check for the file. Probably worth a comment.

-n is not checking for the file either. It's a string operator. It's more explicit, clearer and generally safer because variables can start with special characters (like -)

I thought the quotes would save us from special characters... :-/ ok I'll add it. Sorry was thinking -f.

I thought the quotes would save us from special characters... :-/

They may or may not depending on which test instruction you use etc. It's complicated. I just keep it simple and always use -n.

Same with quotes: "When in doubt, double-quote every expansion in your shell commands."
https://mywiki.wooledge.org/Quotes#I.27m_Too_Lazy_to_Read.2C_Just_Tell_Me_What_to_Do

marc-hb · 2025-03-10T22:09:13Z

run_qemu.sh

 	if [[ $_arg_ndctl_build == "on" ]]; then
 		if [ -n "$ndctl" ]; then
 			rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl
 			prepare_ndctl_build # create mkosi.postinst which compiles
 		fi
 	fi

+	if [[ "$_arg_extra_dirs" ]]; then
+		extra_dirs=$(make_canonical_path $_arg_extra_dirs)


I think make_canonical_path should not be used here, otherwise it's incompatible with shell completion?

On the contrary it is required for shell expansion. See comment about the if check above.

Shell expansion is a pretty big thing, a much bigger "feature" IMHO than "canonical_path". The function name should reflect that.

Ok one could make the argument you are making. But the function returnes the fully qualified canonical path. That is it's purpose. I submit that passing "~/foo" should resolve to "/home//foo" and that is what the user expects. The fact that eval is used to do that is irrelavant to what the user expects.

Let me ask this another way. How else might "~" be expected to be interpreted if passed to this function?

BTW this also works through eval:

$HOME/foo

Whatever happens what I don't want is to have to repeat this logic over and over for each path presented.

We could insist that all paths be absolute in the config file... Maybe that would be ok... dunno?

I actually like that idea. absolute paths only.

Let me ask this another way. How else might "~" be expected to be interpreted if passed to this function?

This is really not the question. The question is: "who knows what ELSE happens when using eval and shell expansion?" And the answer is: barely anyone!

BTW this also works through eval:

My point exactly! And many, many other things: https://www.gnu.org/software/bash/manual/html_node/Shell-Expansions.html
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html

We could insist that all paths be absolute in the config file..

If you mean: dropping shell expansion and eval then this would for sure simplify things tremendously. There is a reason why people say "eval is evil" :-)

But... dropping relative paths sounds like throwing the baby with the eval bathwater. A very easy way to deal with relative paths is to temporarily change directory back to whatever base you decide. Something like:

for src_line in... ( cd $working_dir rsync -a "$src_line" ... ) done

If a relative "$src_line" is not found, rsync will just fail with a useful error message.

marc-hb · 2025-03-10T22:14:27Z

run_qemu.sh

+			fail "$extra_dirs not found"
+		fi
+		echo "Installing extra directories and files from: $extra_dirs"
+		for d in `cat "$extra_dirs"`; do


Suggested change

for d in `cat "$extra_dirs"`; do

cat "$extra_dirs" | while read d; do

read is guaranteed to read line by line whereas for ... in ... splits on any whitespace.

yea probably better. cat just worked initially... :-D

marc-hb · 2025-03-10T22:17:51Z

run_qemu.sh

+			if [[ -e $d ]]; then
+				rsync -a "$d" mkosi.extra/root/
+			else
+				fail "$d not found"


Suggested change

fail "$d not found"

fail "extra-dir directory %s not found" "$d"

Actually I missed that you are trying to use printf style here. Why?

More flexible, safer, more consistent. But no big deal :-D

marc-hb · 2025-03-10T23:37:10Z

BTW, this looks useful and a very welcome cleanup but... I will probably not use it :-)

Packages: add sshfs #126

marc-hb

I missed that shell expansion was an intended feature. Is this only for expanding ~ because rsync does not support it? I thought it did...

EDIT: Shell expansion will not just expand ~, it will also expand *, maybe others and it will be whitespace incompatible.

marc-hb · 2025-03-12T18:41:49Z

run_qemu.sh

@@ -144,6 +144,21 @@ distro_vars="${script_dir}/${_distro}_vars.sh"

 pushd "$_arg_working_dir" > /dev/null || fail "couldn't cd to $_arg_working_dir"

+# save the working directory
+working_dir=$(pwd)


Ah yes of course. Then please:

Suggested change

working_dir=$(pwd)

abs_working_dir=$(pwd)

Please also drop or change the comment, it threw me off.

marc-hb · 2025-03-12T18:48:57Z

run_qemu.sh

 	if [[ $_arg_ndctl_build == "on" ]]; then
 		if [ -n "$ndctl" ]; then
 			rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl
 			prepare_ndctl_build # create mkosi.postinst which compiles
 		fi
 	fi

+	if [[ "$_arg_extra_dirs" ]]; then


-n is not checking for the file either. It's a string operator. It's more explicit, clearer and generally safer because variables can start with special characters (like -)

marc-hb · 2025-03-12T18:50:09Z

run_qemu.sh

 	if [[ $_arg_ndctl_build == "on" ]]; then
 		if [ -n "$ndctl" ]; then
 			rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl
 			prepare_ndctl_build # create mkosi.postinst which compiles
 		fi
 	fi

+	if [[ "$_arg_extra_dirs" ]]; then
+		extra_dirs=$(make_canonical_path $_arg_extra_dirs)


Shell expansion is a pretty big thing, a much bigger "feature" IMHO than "canonical_path". The function name should reflect that.

marc-hb · 2025-03-12T18:50:55Z

parser_generator.m4

@@ -34,6 +34,7 @@ exit 11  #)Created by argbash-init v2.9.0
 # ARG_OPTIONAL_BOOLEAN([debug], [v], [Debug script problems (enables set -x)], )
 # ARG_OPTIONAL_BOOLEAN([ndctl-build], , [Enable ndctl build in root image], [on])
 # ARG_OPTIONAL_BOOLEAN([kern-selftests], , [Enable kernel selftest build in root image (Warning: This option can take a long time and requires many support packages on the host; including some 32 bit)], [off])
+# ARG_OPTIONAL_SINGLE([extra-dirs], , [Specify a file with a list of directories (or files) to be copied to the /root directory within the image.], [])


Also: shell expansion that I mostly miss in the first review. It's really not a small thing.

marc-hb · 2025-03-12T18:51:51Z

run_qemu.sh

+{
+	local p=$1
+
+	p=$(eval echo $p)


OK, I missed that shell expansion was an intended feature. It's really not a small thing that should be found by chance in line 155 and nowhere else.

marc-hb · 2025-03-12T18:52:46Z

run_qemu.sh

+			fail "$extra_dirs not found"
+		fi
+		echo "Installing extra directories and files from: $extra_dirs"
+		for d in `cat "$extra_dirs"`; do


Coding shell scripts without running shellcheck is now a crime :-)

marc-hb · 2025-03-12T18:53:28Z

run_qemu.sh

+			if [[ -e $d ]]; then
+				rsync -a "$d" mkosi.extra/root/
+			else
+				fail "$d not found"


More flexible, safer, more consistent. But no big deal :-D

weiny2 · 2025-03-12T19:46:06Z

I missed that shell expansion was an intended feature. Is this only for expanding ~ because rsync does not support it? I thought it did...

EDIT: Shell expansion will not just expand ~, it will also expand *, maybe others and it will be whitespace incompatible.

expanding "*" is not going to work anyway if you don't pass the correct relative path. Part of the issue is that when this code runs you are in $working_dir/qbuild. I'm trying to make the user not have to understand exactly where this is run and during what step. Passing in * may be something we want to make work... dunno. But making the user read and understand the internals of run-qemu.sh to know how to specify things is even harder than getting an error on "*" processing.

Not sure why you say whitespace is incompatible. It seems to work for me.

14:45:10 > ls ./qbuild/mkosi.extra/root/white\ space/
foo
14:45:13 > cat ~/runkymoo-dirs
~/dev/rasdaemon
~/dev/ras-tools
$HOME/dev/numactl
/home/iweiny/dev/runkymoo-test/white space

weiny2 · 2025-03-12T20:04:24Z

Ok leaning toward this simplifed version which requires absolute paths in the config file.

'*' is not supported.

Ok I don't get how to put preformatted text in here... ok got it.

15:01:25 > git di
diff --git a/run_qemu.sh b/run_qemu.sh
index 1d39f9a0a02b..3e9ba4062ff2 100755
--- a/run_qemu.sh
+++ b/run_qemu.sh
@@ -144,22 +144,6 @@ distro_vars="${script_dir}/${_distro}_vars.sh"
 
 pushd "$_arg_working_dir" > /dev/null || fail "couldn't cd to $_arg_working_dir"
 
-# save the working directory
-working_dir=$(pwd)
-
-# make a path canonical to the working dir
-make_canonical_path()
-{
-       local p=$1
-
-       # ensure bash expansion on $p
-       p=$(eval echo "$p")
-       pushd "$working_dir" > /dev/null
-       p=$(realpath "$p")
-       popd > /dev/null
-       echo "$p"
-}
-
 set_valid_mkosi_ver()
 {
        "$mkosi_bin" --version
@@ -1173,14 +1157,14 @@ make_rootfs()
        fi
 
        # Check if extra-dirs is specified prior to making it canonical
-       if [[ "$_arg_extra_dirs" ]]; then
-               extra_dirs=$(make_canonical_path "$_arg_extra_dirs")
+       if [[ -n "$_arg_extra_dirs" ]]; then
+               extra_dirs=$(eval echo "$_arg_extra_dirs")
                if [[ ! -f $extra_dirs ]]; then
                        fail "$extra_dirs not found"
                fi
                echo "Installing extra directories and files from: $extra_dirs"
                while read d; do
-                       d=$(make_canonical_path "$d")
+                       d=$(eval echo "$d")
                        echo "     $d"
                        if [[ -e $d ]]; then
                                rsync -a "$d" mkosi.extra/root/

marc-hb · 2025-03-12T20:11:09Z

expanding "*" is not going to work anyway if you don't pass the correct relative path. Part of the issue is that when this code runs you are in $working_dir/qbuild.

My point exactly: expanding * (and other things) is extremely brittle, error-prone and confusing. The main problem is (funny enough) not whether users want expansion or not, it's making sure the behavior is consistent and predictable.

Ok leaning toward this simplifed version which requires absolute paths in the config file.

I miss how relative paths are related to shell expansion, let me take a closer look.

marc-hb

Not sure why you say whitespace is incompatible. It seems to work for me.

It works in simple cases because echo accepts a variable number of arguments. But you can find plenty of counter-examples, here's the first one I can think of:

a="multiple    spaces"
echo $a
multiple spaces
echo "$a"
multiple    spaces

https://mywiki.wooledge.org/Quotes#I.27m_Too_Lazy_to_Read.2C_Just_Tell_Me_What_to_Do

Also: in general prefer printf over echo, it's more predictable
https://www.shellcheck.net/wiki/SC2028

marc-hb · 2025-03-12T22:51:28Z

run_qemu.sh

 	if [[ $_arg_ndctl_build == "on" ]]; then
 		if [ -n "$ndctl" ]; then
 			rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl
 			prepare_ndctl_build # create mkosi.postinst which compiles
 		fi
 	fi

+	if [[ "$_arg_extra_dirs" ]]; then


I thought the quotes would save us from special characters... :-/

They may or may not depending on which test instruction you use etc. It's complicated. I just keep it simple and always use -n.

Same with quotes: "When in doubt, double-quote every expansion in your shell commands."
https://mywiki.wooledge.org/Quotes#I.27m_Too_Lazy_to_Read.2C_Just_Tell_Me_What_to_Do

marc-hb · 2025-03-12T23:02:24Z

run_qemu.sh

 	if [[ $_arg_ndctl_build == "on" ]]; then
 		if [ -n "$ndctl" ]; then
 			rsync "${rsync_opts[@]}" "$ndctl/" mkosi.extra/root/ndctl
 			prepare_ndctl_build # create mkosi.postinst which compiles
 		fi
 	fi

+	if [[ "$_arg_extra_dirs" ]]; then
+		extra_dirs=$(make_canonical_path $_arg_extra_dirs)


Let me ask this another way. How else might "~" be expected to be interpreted if passed to this function?

This is really not the question. The question is: "who knows what ELSE happens when using eval and shell expansion?" And the answer is: barely anyone!

BTW this also works through eval:

My point exactly! And many, many other things: https://www.gnu.org/software/bash/manual/html_node/Shell-Expansions.html
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html

We could insist that all paths be absolute in the config file..

If you mean: dropping shell expansion and eval then this would for sure simplify things tremendously. There is a reason why people say "eval is evil" :-)

But... dropping relative paths sounds like throwing the baby with the eval bathwater. A very easy way to deal with relative paths is to temporarily change directory back to whatever base you decide. Something like:

for src_line in... ( cd $working_dir rsync -a "$src_line" ... ) done

If a relative "$src_line" is not found, rsync will just fail with a useful error message.

marc-hb · 2025-03-12T23:04:56Z

run_qemu.sh

+			fail "$extra_dirs not found"
+		fi
+		echo "Installing extra directories and files from: $extra_dirs"
+		for d in `cat "$extra_dirs"`; do


Absolutely! But shellcheck can catch codestyle issues and sometimes bugs even long before it is requirement :-) This only requires keep the number of warnings to a reasonable level not to drown the new ones.

marc-hb · 2025-03-13T00:59:54Z

After a quick "brainstorm" with @stellarhopper I think we have a simple and effective idea: add TWO new command line options offering two different user interfaces to the same feature:

--extra-dir=~/dir1/ --extra-dir=$SOMEWHERE/dir2/ ... # thanks to ARG_OPTIONAL_REPEATED
--extra-dirs-from=list_of_already_expanded_dirs_and_files.txt

All these directories and files get combined in a single list and fed to a single rsync loop.

The first, command-line option leverages the same old shell expansion that people are very used to. But it's not "scalable" to a large number of directories.
The second, from "file option" offers this scalability. It requires "manual" expansion but that's a minor inconvenience, small compared to creating the file anyway. Relative files may be possible; I think that is a separate topic.
There is no eval or any other shell expansion complexity to maintain in run_qemu.sh
No duplication of code: all inputs get concatenated.

@weiny2 , thoughts?

weiny2 added 2 commits March 7, 2025 10:00

weiny2 requested a review from stellarhopper as a code owner March 10, 2025 20:54

marc-hb reviewed Mar 10, 2025

View reviewed changes

marc-hb reviewed Mar 12, 2025

View reviewed changes

marc-hb mentioned this pull request Mar 12, 2025

Too many "mkosi.extra/" cooks spoil the broth #124

Open

marc-hb reviewed Mar 12, 2025

View reviewed changes

	if [[ "$_arg_extra_dirs" ]]; then
	if [[ -n "$_arg_extra_dirs" ]]; then

	for d in `cat "$extra_dirs"`; do
	cat "$extra_dirs" \| while read d; do

	fail "$d not found"
	fail "extra-dir directory %s not found" "$d"

Clean up the addition of extra directories and/or files #125

Are you sure you want to change the base?

Clean up the addition of extra directories and/or files #125

Conversation

weiny2 commented Mar 10, 2025

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

stellarhopper Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weiny2 Mar 12, 2025 • 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

marc-hb Mar 12, 2025 • 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

marc-hb Mar 12, 2025 • 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

marc-hb commented Mar 10, 2025

marc-hb left a comment • 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

weiny2 commented Mar 12, 2025 • edited by marc-hb Loading

weiny2 commented Mar 12, 2025 • edited Loading

marc-hb commented Mar 12, 2025 • edited Loading

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

marc-hb Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Mar 13, 2025 • edited Loading

stellarhopper Mar 12, 2025 •

edited

Loading

weiny2 Mar 12, 2025 •

edited

Loading

marc-hb Mar 12, 2025 •

edited

Loading

marc-hb Mar 12, 2025 •

edited

Loading

marc-hb left a comment •

edited

Loading

weiny2 commented Mar 12, 2025 •

edited by marc-hb

Loading

weiny2 commented Mar 12, 2025 •

edited

Loading

marc-hb commented Mar 12, 2025 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb Mar 12, 2025 •

edited

Loading

marc-hb Mar 12, 2025 •

edited

Loading

marc-hb commented Mar 13, 2025 •

edited

Loading