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

Add -p option to mknod.c and use it with sys #2181

Merged
merged 3 commits into from
Jan 12, 2025

Conversation

floriangit
Copy link
Contributor

This replaces #2180 as per your suggestions. Using sys now even on a fully installed medium will now only output around 20 lines which fits to the old terminal sizes (80x25).

@ghaerr
Copy link
Owner

ghaerr commented Jan 12, 2025

Thanks @floriangit, well done!

On a small code formatting note, would you mind changing the new if statements on lines 19-22 to match the existing formatting convention of using a space after the if, to match the rest of the file? Thank you.

@floriangit
Copy link
Contributor Author

well done!

thanks! Added the spaces now.

@ghaerr
Copy link
Owner

ghaerr commented Jan 12, 2025

Thanks for enhancing sys to work for updating as well as initial installation @floriangit!

@ghaerr ghaerr merged commit 2e20e1f into ghaerr:master Jan 12, 2025
@floriangit
Copy link
Contributor Author

one day I will rename sys...:P

@ghaerr
Copy link
Owner

ghaerr commented Jan 12, 2025

It was initially suggested by someone, saying DOS used "sys" ... Obviously we're not emulating DOS here. What possible other names are you thinking?

@floriangit
Copy link
Contributor Author

update, upgrade, install? After all it is now kind of an upgrader for me at least.

@floriangit
Copy link
Contributor Author

then again, I thought it would be too tiresome to rename that now, since one has to know overall ELKS. Because grepping for sys to find out where it's used is a very time-consuming thing, I guess.

@ghaerr
Copy link
Owner

ghaerr commented Jan 12, 2025

I kind of like 'install'. The community has been used to 'sys' for more than a few years, but I would agree 'sys' only means something to folks familiar with DOS. Renaming ideas should probably be mentioned on #2157.

@floriangit
Copy link
Contributor Author

floriangit commented Jan 12, 2025

Yes, I thought same. Since it's not (yet) anything like a self-hosted updater, it's still an installer involving two separate media, so install sounds good.

@ghaerr
Copy link
Owner

ghaerr commented Jan 13, 2025

@floriangit,

I just realized that I think we have the new return value for the mknod -p option incorrect: instead of return 1 for the three cases, I think it needs to be return 0 (which means OK). A return 0 is an exit(0) which is interpreted by the shell as OK. Return 1 will stop shell execution if stop on errors is set (off by default).

To test this, add set -e (stop on error) to bin/sys, and try updating. The mknod -p will return 1 and halt execution. And the set -e should stay in the shell script anyways, so that errors aren't ignored.

Please test and let me know what you find, thanks!

@floriangit
Copy link
Contributor Author

My line of thinking was that with -p returning 1 instead of 0: "The user asked me to preserve a file, but he also asked me to overwrite that file, so I indicate 1 instead of 0, so he has a chance to recognize my silent return".
Then again, I do understand your point (All is OK!) and looking at mkdir -p returning 0 when directory exists, I agree and will change the return's to 0.

As for set -e that's a bit of a mixed bag. In sys also makeboot is called, which does not return 0 on success, but rather the FS type, since this is how it's designed. So I cannot globally add set -e in sys since that will stop the script at that point. I could do it after those lines, but I personally would not overrate set -e since it silently returns 1, but gives no warning whatsoever anyhow. The last lines of output of sys was:
System copied
so if I would not work on it right now, I would fail to see any error....

@ghaerr
Copy link
Owner

ghaerr commented Jan 13, 2025

As for set -e that's a bit of a mixed bag. In sys also makeboot is called, which does not return 0 on success, but rather the FS type

Thanks for pointing that out, I forgot about that.

My current take is that running shell scripts without set -e is unsafe, unless any error is not important. In our case, the user will definitely want to know whether there was success or not in installation. Thus, set -e must be set.

Next, the convention in shell scripts is a command returns "true" (no error) on exit/return 0, while "false" is anything else. A "false" return will stop the script execution if set -e is set. Thus our mknod -p must follow that convention.

HOWEVER, since you've rightly pointed out that our design requires makeboot to return an FSTYPE (yes, an earlier kluge since there was otherwise no way to get the mounted filesystem type), then I think we need to set +e before makeboot and set -e afterwards - at least until the makeboot returning FSTYPE design is enhanced using another method.

To your earlier points about writing a C program versus a shell script for installation or updating - our current method shows a weak point in the design which would be more easily handled in C. However, given the ease in scripting most of the solution, perhaps we should think of more/better ways to communicate between the kernel and shell scripts for installation issues, then take an overview design pass after we see what more might be required.

@floriangit
Copy link
Contributor Author

floriangit commented Jan 16, 2025

Hi @ghaerr,

Agreed first about return 0 in mknod -p and set -e in sys.
I shortly tried to set it after makeboot but it seems to error out somewhere else (as said: alas set -e is silent except the error code with $?). Would need to debug.

Which made me step back. (Still agreeing on mknod -p to exit with 0):
set -e makes sense to safely execute scripts that can run silently and really need to succeed. But as sys is a standalone installer and we don't know whether the user checks its return value with echo $? after execution and as there is no error message it makes no sense as-is to me. It's not clear cut for a user. Wrapping sys with a install script that checks its return and cries "FAILED" is of course an option. Which would mean more bytes, heap and stack (your dilemma? haha).

I would commit the return 0 for mknod -p next days and wait for thoughts.

The general discussion re. sys can continue in the separate thread. Other contributors are much more experienced so maybe something creative comes up there. I don't really mind "the name" that much, since I watched the Dilbert episode of "the name" ;-).

@ghaerr
Copy link
Owner

ghaerr commented Jan 17, 2025

we don't know whether the user checks its return value with echo $? after execution and as there is no error message it makes no sense as-is to me.

See #2186 - the issue isn't what the sys script returns to the user, but rather halting the sys script on any error, but also allowing makeboot to return a filesystem type as its return code, and not having that filesystem type stop script execution, but only for that single script statement.

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

Successfully merging this pull request may close these issues.

2 participants