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

Use sysrc to modify rc.conf #708

Closed
wants to merge 1 commit into from
Closed

Use sysrc to modify rc.conf #708

wants to merge 1 commit into from

Conversation

jlduran
Copy link
Contributor

@jlduran jlduran commented Aug 28, 2019

touch is required in case there is no rc.conf

@bdrewery
Copy link
Member

Is there a benefit here that I'm missing?
The old code was simple and has no executions. Now there's 2 and extra dependencies.

@jlduran
Copy link
Contributor Author

jlduran commented Aug 28, 2019

I see... Will you be willing to accept a patch with the hostname surrounded with double quotation marks instead?

Is there a benefit here that I'm missing?

For example, if the overlaydir contains an rc.conf with the hostname already added, or something similar.

@jlduran
Copy link
Contributor Author

jlduran commented Aug 30, 2019

As it has been marked for discussion:

I have pushed another revision, using sysrc's -R switch to set the relative root directory. Also removed the touch, however, if there is no rc.conf (or hostname) defined, sysrc will complain (awk: can't open file /etc/rc.conf), I don't know how to address this (> /dev/null 2>&1)? cc/ @freebsdfrau

Thanks!

@freebsdfrau
Copy link

That's a bug. I worked up the following patch to solve it:

--- /usr/share/bsdconfig/sysrc.subr.orig        2019-08-30 13:16:04.604403000 +0000
+++ /usr/share/bsdconfig/sysrc.subr        2019-08-30 13:14:55.058956000 +0000
@@ -560,7 +560,7 @@ f_sysrc_set()
         #
         if [ "$not_found" ]; then
                 # Add a newline if missing before appending to the file
-                awk 'BEGIN { wc = 0 } NR == 1 {
+                [ ! -e "$file" ] || awk 'BEGIN { wc = 0 } NR == 1 {
                         (cmd = "wc -l " FILENAME) | getline
                         close(cmd)
                         wc = $1

@bdrewery
Copy link
Member

bdrewery commented Dec 4, 2019

Well there's a reason for simplicity. Would have to create the file to avoid the bug.
Anyway #731 will likely conflict with this change since it's changing this line of code and moving it elsewhere. Converting to sysrc after that PR could still be done.

@freebsdfrau
Copy link

freebsdfrau commented Dec 4, 2019

Well there's a reason for simplicity. Would have to create the file to avoid the bug.
Anyway #731 will likely conflict with this change since it's changing this line of code and moving it elsewhere. Converting to sysrc after that PR could still be done.

Merged to head 42 hours ago

https://svnweb.freebsd.org/base?view=revision&revision=355280

I intend to MFC back to stable/11

@jlduran
Copy link
Contributor Author

jlduran commented May 20, 2021

Closing this PR, as it will be addressed in #731. Also stable/11 is about to EOL in a few months.

Thank you!

@jlduran jlduran closed this May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants