With the following value in my /etc/rc.conf, "service isc-dhcpd restart" will fail due to invalid command line arguments. dhcpd_flags="-4 -q" For some reason, "stop" and "start" both work. But, "restart" invokes dhcpd_checkconfig() which performs a sed that breaks the above value of dhcpd_flags. Portion of sh -x of the restart operation shows the problem: + rc_flags='-4 -q -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2 -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2’ + rc_flags_mod='-4 -q -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2 -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2’ + echo '-4 -q -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2 -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2’ + sed -Ee 's/(^-q | -q | -q$)//‘ + rc_flags_mod='-4-cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2 -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2’ + /usr/local/sbin/dhcpd -t -q -4-cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2 -cf /usr/local/etc/dhcpd.conf -lf /var/db/dhcpd/dhcpd.leases -pf /var/run/dhcpd/dhcpd.pid -user dhcpd -group dhcpd int1 int2 I think replacing "-q" and surrounding spaces with "//" (empty string) is an error. If also removing the spaces around the argument you ae removing, you need to replace the sequence with space to avoid the above problem.
Created attachment 253805 [details] fix operational part of the problem Attached is an easy way to avoid the problem during the actual configcheck run.
Created attachment 253822 [details] Remove the problematic logic This is all completely pointless, there is no reason to remove -q from rc_flags.
Removing all of it is one way to deal with this, but it's certainly not pointless code. What it is, however, is futile, because stripping -q in all possible combos to show the error is not possible with reasonable amounts of code. That being said I'm happy to adjust the code according to suggestions, but we can also chip in to all do patches instead of communicating about it first. Cheers, Franco
If the point is to force error messages to be displayed, you can trivially patch server/dhcpd.c to add an undocumented -Q option which cancels -q, and just place that after ${rc_flags}.
To be frank I agree with your patch. I just find the assertiveness towards given code a bit impractical because somebody made the effort to put it in and it mostly works. The obsoleted patch proposed a clean fix for checkconfig to avoid running into operational issues, but mileage obviously varies with the error message returned in the modified command invoke. Both patches are much better than modifying the dhcpd code to accommodate the current behaviour. Cheers, Franco
Created attachment 253837 [details] Add unquiet flag Like I said, adding an unquiet flag is trivial and far more robust than trying to edit rc_flags.
Given that ISC is unlikely to be releasing new versions of isc-dhcpd, the patch to add a -Q option is probably the best route.
I suggest adding a comment in the rc.d (isc-dhcpd.in) indicating that the -Q needs to be at the end for a reason. My first glance at that patch I was thinking "Hey, that should be up front with the other hard-coded options", then realized why that would silently be a problem.
Mmmmmm? That prevents the startup message from being repeated twice when -q is not in use, right? Also, the number of times -q is specified can change some behavior, so it seems to prevent that as well. Isn't it the sed that needs to be changed? For example, make it tight to the following extent. sed -Ee 's/(^-q[[:space:]]|[[:space:]]-q[[:>:]]|[[:space:]]-q$)//'
(In reply to Tatsuki Makino from comment #9) No, the -q option is idempotent, specifying it multiple times will make no difference. And the point of my patch is to remove the need for sed. (Come to think of it, on the first line of the diff, -q should be at the end, but that's a minor issue)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=6725d10536142ea378a9207aecb99c2d2e1a9229 commit 6725d10536142ea378a9207aecb99c2d2e1a9229 Author: Dag-Erling Smørgrav <des@FreeBSD.org> AuthorDate: 2024-10-02 10:37:30 +0000 Commit: Dag-Erling Smørgrav <des@FreeBSD.org> CommitDate: 2024-10-02 10:37:30 +0000 net/isc-dhcp44-server: Fix checkconfig command. This adds an undocumented -Q flag which cancels out any -q option that may be present in dhcpd_flags, avoiding the need for a fragile sed command. It also adds a comment explaining why we are running the check twice and what the -q and -Q are for. PR: 281701 net/isc-dhcp44-server/Makefile | 2 +- net/isc-dhcp44-server/files/isc-dhcpd.in | 19 +++++++++---------- .../files/patch-server_dhcpd.c (new) | 12 ++++++++++++ 3 files changed, 22 insertions(+), 11 deletions(-)
(In reply to Dag-Erling Smørgrav from comment #10) Yes. The duplication of -q has not meant anything since around 3.1 when this checkconfig was added. I've been looking into it :) The problem with this seems to be that for some reason it is not at the time of start, but at the time of restart. Is there something wrong with running checkconfig on start all the time, like apache24? This can be done using service isc-dhcpd configtest command to run only dhcpd_checkconfig. It might be better to be able to do this with -q for start and -Q for configtest. It seems that the results of dhcpd -t are always output to syslog. It may not be good to have a passage that is run twice. Well, I've been using this without worrying too much about it until now :)
You could add a `dhcpd_checkconfig` line to the `dhcpd_precmd()` function, but that's a separate issue, please open a separate ticket.