[Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
Hi!
Some guys using dnsmasq in virtual machines and OpenStack use custom
dhcp_script to manage leases of clients.
However they complain if there is anything wrong with them, then are just told
broken pipe and no information.
We understand it should not produce any output under normal operation. But it
would be really helpful if at least anything was visible in logs. Especially
for errors happening under rare circumstances.
I have prepared patch to forward events from helper. It prevents SIGPIPE
receiving if script does write anything. And logs it from dnsmasq.
It seems very handy to me.
It was not simple to forward it to main log. I would like opinions if it is
useful or dangerous.
Do you consider it worth merging Simon?
Best Regards,
Petr
--
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com PGP: 65C6C973
From a7b740b12525b085efcab5d9c8a8260df2ae33dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?=
Date: Fri, 24 Mar 2017 18:23:53 +0100
Subject: [PATCH] Log output of dhcp_script called from handler
---
src/dnsmasq.c | 13
src/dnsmasq.h | 49 +++---
src/helper.c | 63 ---
3 files changed, 98 insertions(+), 27 deletions(-)
diff --git a/src/dnsmasq.c b/src/dnsmasq.c
index d2cc7cc..e2e42bf 100644
--- a/src/dnsmasq.c
+++ b/src/dnsmasq.c
@@ -1315,6 +1315,19 @@ static void async_event(int pipe, time_t now)
daemon->lease_change_command, strerror(ev.data));
break;
+#if defined(HAVE_SCRIPT) && defined(HAVE_DHCP)
+ case EVENT_SCRIPT_ERR:
+ if (ev.data != 0)
+ my_syslog(LOG_ERR, _("dhcp-script output: %s: %s"),
+strerror(ev.data), msg ? msg : "");
+else
+ my_syslog(LOG_DEBUG, _("dhcp-script output: %s"),
+msg ? msg : "");
+free(msg);
+ msg = NULL;
+ break;
+#endif
+
/* necessary for fatal errors in helper */
case EVENT_USER_ERR:
case EVENT_DIE:
diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 6b44e53..f51ae3b 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -145,30 +145,31 @@ struct event_desc {
int event, data, msg_sz;
};
-#define EVENT_RELOAD1
-#define EVENT_DUMP 2
-#define EVENT_ALARM 3
-#define EVENT_TERM 4
-#define EVENT_CHILD 5
-#define EVENT_REOPEN6
-#define EVENT_EXITED7
-#define EVENT_KILLED8
-#define EVENT_EXEC_ERR 9
-#define EVENT_PIPE_ERR 10
-#define EVENT_USER_ERR 11
-#define EVENT_CAP_ERR 12
-#define EVENT_PIDFILE 13
-#define EVENT_HUSER_ERR 14
-#define EVENT_GROUP_ERR 15
-#define EVENT_DIE 16
-#define EVENT_LOG_ERR 17
-#define EVENT_FORK_ERR 18
-#define EVENT_LUA_ERR 19
-#define EVENT_TFTP_ERR 20
-#define EVENT_INIT 21
-#define EVENT_NEWADDR 22
-#define EVENT_NEWROUTE 23
-#define EVENT_TIME_ERR 24
+#define EVENT_RELOAD 1
+#define EVENT_DUMP 2
+#define EVENT_ALARM 3
+#define EVENT_TERM 4
+#define EVENT_CHILD 5
+#define EVENT_REOPEN 6
+#define EVENT_EXITED 7
+#define EVENT_KILLED 8
+#define EVENT_EXEC_ERR 9
+#define EVENT_PIPE_ERR 10
+#define EVENT_USER_ERR 11
+#define EVENT_CAP_ERR12
+#define EVENT_PIDFILE13
+#define EVENT_HUSER_ERR 14
+#define EVENT_GROUP_ERR 15
+#define EVENT_DIE16
+#define EVENT_LOG_ERR17
+#define EVENT_FORK_ERR 18
+#define EVENT_LUA_ERR19
+#define EVENT_TFTP_ERR 20
+#define EVENT_INIT 21
+#define EVENT_NEWADDR22
+#define EVENT_NEWROUTE 23
+#define EVENT_TIME_ERR 24
+#define EVENT_SCRIPT_ERR 25
/* Exit codes. */
#define EC_GOOD0
diff --git a/src/helper.c b/src/helper.c
index 2b8164b..b777f77 100644
--- a/src/helper.c
+++ b/src/helper.c
@@ -14,6 +14,7 @@
along with this program. If not, see
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
On Fri, 24 Mar 2017 13:38:17 -0400 (EDT) Petr Mensik wrote: > Hi! > > Some guys using dnsmasq in virtual machines and OpenStack use custom > dhcp_script to manage leases of clients. However they complain if > there is anything wrong with them, then are just told broken pipe and > no information. > > We understand it should not produce any output under normal > operation. But it would be really helpful if at least anything was > visible in logs. Especially for errors happening under rare > circumstances. I have prepared patch to forward events from helper. > It prevents SIGPIPE receiving if script does write anything. And logs > it from dnsmasq. It seems very handy to me. > > It was not simple to forward it to main log. I would like opinions if > it is useful or dangerous. Do you consider it worth merging Simon? > > Best Regards, > Petr > -- > Petr Menšík > Software Engineer > Red Hat, http://www.redhat.com/ > email: pemen...@redhat.com PGP: 65C6C973 > you could also just put "exec >/var/log/whatever 2>&1" at the start of your script. hell, you can even do "exec > >(logger) 2>&1" if you want. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
Well yes, you can redirect output to any file you want. It is not configurable however. And it cannot reuse any logging configuration you already have. You cannot use journalctl to list errors for example. You have to create place for dnsmasq to write that log and rotate it in regular intervals. This solution is good for debugging your new script for the first time. It does not seem to good solution, if you want to log any problems in your long running service. Dnsmasq is used by libvirt for example, to provide DNS and DHCP on virtual subnet. -- Petr Menšík Software Engineer Red Hat, http://www.redhat.com/ email: pemen...@redhat.com PGP: 65C6C973 - Original Message - From: "Alex Xu" To: dnsmasq-discuss@lists.thekelleys.org.uk Sent: Friday, March 24, 2017 7:19:04 PM Subject: Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output On Fri, 24 Mar 2017 13:38:17 -0400 (EDT) Petr Mensik wrote: > Hi! > > Some guys using dnsmasq in virtual machines and OpenStack use custom > dhcp_script to manage leases of clients. However they complain if > there is anything wrong with them, then are just told broken pipe and > no information. > > We understand it should not produce any output under normal > operation. But it would be really helpful if at least anything was > visible in logs. Especially for errors happening under rare > circumstances. I have prepared patch to forward events from helper. > It prevents SIGPIPE receiving if script does write anything. And logs > it from dnsmasq. It seems very handy to me. > > It was not simple to forward it to main log. I would like opinions if > it is useful or dangerous. Do you consider it worth merging Simon? > > Best Regards, > Petr > -- > Petr Menšík > Software Engineer > Red Hat, http://www.redhat.com/ > email: pemen...@redhat.com PGP: 65C6C973 > you could also just put "exec >/var/log/whatever 2>&1" at the start of your script. hell, you can even do "exec > >(logger) 2>&1" if you want. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
I like this. Yes, I know you can do it with shell magic, but this is easier and what I would expect to happen. I've changed the patch quite a lot: 1) Don't go to large effort to report "never happen" errors from pipe(), just silently handle them in the same way as fork() 2) Don't do any of this when the -d debug flag is in effect, as it's already defined that the script gets stdin, stdout and stderr from the dnsmasq process in that case. 3) Expand the subject-based logging that already exists, DHCP stuff comes from dnsmasq-dhcp, script output comes from dhcp-script. That avoids the wordy preamble to every line otherwise. 4) Pull the copy-to-log code out of the loop wait()ing for processes to die, it makes more sense to iterate until the descriptors close, then reap child processes. 5) Rationalise conditional compilation stuff. There may be more of that in a subsequent commit. 6) Update the man page to reflect new reality (!) Any remaining bugs are mine, but Petr please check that I didn't break things. Cheers, Simon. On 24/03/17 17:38, Petr Mensik wrote: > Hi! > > Some guys using dnsmasq in virtual machines and OpenStack use custom > dhcp_script to manage leases of clients. > However they complain if there is anything wrong with them, then are just > told broken pipe and no information. > > We understand it should not produce any output under normal operation. But it > would be really helpful if at least anything was visible in logs. Especially > for errors happening under rare circumstances. > I have prepared patch to forward events from helper. It prevents SIGPIPE > receiving if script does write anything. And logs it from dnsmasq. > It seems very handy to me. > > It was not simple to forward it to main log. I would like opinions if it is > useful or dangerous. > Do you consider it worth merging Simon? > > Best Regards, > Petr > -- > Petr Menšík > Software Engineer > Red Hat, http://www.redhat.com/ > email: pemen...@redhat.com PGP: 65C6C973 > > > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
Hi Simon,
I like your changes. New shorter log is definitely helpful, separate log
section helps. The only bug I found is red white space highlight in the patch.
However I did yet another fix for remaining dhcp-script init action.
It completely ignored any error in structure and silently skipped the rest of
database. If there was any message in stdin of init script, it just died
silently.
The only thing that were visible was SIGPIPE from dhcp-script, because it did
not read whole database, if that signal was logged at all.
My new patch handles garbage in leases database. If the line is wrong, it logs
part of wrong line and skips the rest of init.
I thought about die in that case. I think it would be better for backward
compatibility to start with empty leases as before.
--
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com PGP: 65C6C973
- Original Message -
From: "Simon Kelley"
To: dnsmasq-discuss@lists.thekelleys.org.uk
Sent: Sunday, April 16, 2017 9:29:21 PM
Subject: Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
I like this. Yes, I know you can do it with shell magic, but this is
easier and what I would expect to happen.
I've changed the patch quite a lot:
1) Don't go to large effort to report "never happen" errors from pipe(),
just silently handle them in the same way as fork()
2) Don't do any of this when the -d debug flag is in effect, as it's
already defined that the script gets stdin, stdout and stderr from the
dnsmasq process in that case.
3) Expand the subject-based logging that already exists, DHCP stuff
comes from dnsmasq-dhcp, script output comes from dhcp-script. That
avoids the wordy preamble to every line otherwise.
4) Pull the copy-to-log code out of the loop wait()ing for processes to
die, it makes more sense to iterate until the descriptors close, then
reap child processes.
5) Rationalise conditional compilation stuff. There may be more of that
in a subsequent commit.
6) Update the man page to reflect new reality (!)
Any remaining bugs are mine, but Petr please check that I didn't break
things.
Cheers,
Simon.
On 24/03/17 17:38, Petr Mensik wrote:
> Hi!
>
> Some guys using dnsmasq in virtual machines and OpenStack use custom
> dhcp_script to manage leases of clients.
> However they complain if there is anything wrong with them, then are just
> told broken pipe and no information.
>
> We understand it should not produce any output under normal operation. But it
> would be really helpful if at least anything was visible in logs. Especially
> for errors happening under rare circumstances.
> I have prepared patch to forward events from helper. It prevents SIGPIPE
> receiving if script does write anything. And logs it from dnsmasq.
> It seems very handy to me.
>
> It was not simple to forward it to main log. I would like opinions if it is
> useful or dangerous.
> Do you consider it worth merging Simon?
>
> Best Regards,
> Petr
> --
> Petr Menšík
> Software Engineer
> Red Hat, http://www.redhat.com/
> email: pemen...@redhat.com PGP: 65C6C973
>
>
>
> ___
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
>
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
From cf2831d52884452f039e05cc16f6562cbe6db650 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?=
Date: Wed, 19 Apr 2017 19:32:54 +0200
Subject: [PATCH] Verify leases database format, log errors if present.
---
src/lease.c | 110 +++-
1 file changed, 65 insertions(+), 45 deletions(-)
diff --git a/src/lease.c b/src/lease.c
index fc6cbe9..77da638 100644
--- a/src/lease.c
+++ b/src/lease.c
@@ -21,67 +21,38 @@
static struct dhcp_lease *leases = NULL, *old_leases = NULL;
static int dns_dirty, file_dirty, leases_left;
-void lease_init(time_t now)
+static int read_leases(time_t now, FILE *leasestream)
{
unsigned long ei;
struct all_addr addr;
struct dhcp_lease *lease;
int clid_len, hw_len, hw_type;
- FILE *leasestream;
-
- leases_left = daemon->dhcp_max;
-
- if (option_bool(OPT_LEASE_RO))
-{
- /* run " init" once to get the
- initial state of the database. If leasefile-ro is
- set without a script, we just do without any
- lease database. */
-#ifdef HAVE_SCRIPT
- if (daemon->lease_change_command)
- {
- strcpy(daemon->dhcp_buff, daemon->lease_change_command);
- strcat(daemon->dhcp_buff, " init");
- leasestream = popen(daemon->dhcp_buff, "r");
- }
- else
-#endif
- {
- f
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
OK, there's a little subtlety here that needs to be taken into account. The lease file format was not originally designed to be extensible, so the code takes advantage (sort of) of the fact that lines which are associated with IPv6 leases make no sense to older versions of dnsmasq or even current versions compiled without IPv6 support. The order of lines when the file is written is always IPv4 leases duid IPv6 leases (I think duid may not get written in some cases - doesn't matter for the sake of this argument.) Therefore the behaviour of silently giving up when an unrecognisable line is encountered is a feature, not a bug, as it allows old or non-IPv6 dnsmasq binaries to work with newer lease files - they get the IPv4 leases and ignore the rest. Since the main problem you're trying to solve is unexpected error messages from the script, the solution to this is to keep the old behaviour when reading from a file, but the new behaviour when running the script init command. Given that we're only checking when using the init script, I think die()ing is the best thing to do when parsing fails. I've moved your patch on to do that, and to check ferror() on the stream, to catch IO-errors. Again, any new bugs are mine :) Cheers, Simon. On 19/04/17 19:14, Petr Mensik wrote: > Hi Simon, > > I like your changes. New shorter log is definitely helpful, separate log > section helps. The only bug I found is red white space highlight in the patch. > > However I did yet another fix for remaining dhcp-script init action. > It completely ignored any error in structure and silently skipped the rest of > database. If there was any message in stdin of init script, it just died > silently. > The only thing that were visible was SIGPIPE from dhcp-script, because it did > not read whole database, if that signal was logged at all. > My new patch handles garbage in leases database. If the line is wrong, it > logs part of wrong line and skips the rest of init. > I thought about die in that case. I think it would be better for backward > compatibility to start with empty leases as before. > > -- > Petr Menšík > Software Engineer > Red Hat, http://www.redhat.com/ > email: pemen...@redhat.com PGP: 65C6C973 > > - Original Message - > From: "Simon Kelley" > To: dnsmasq-discuss@lists.thekelleys.org.uk > Sent: Sunday, April 16, 2017 9:29:21 PM > Subject: Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output > > I like this. Yes, I know you can do it with shell magic, but this is > easier and what I would expect to happen. > > I've changed the patch quite a lot: > > 1) Don't go to large effort to report "never happen" errors from pipe(), > just silently handle them in the same way as fork() > > 2) Don't do any of this when the -d debug flag is in effect, as it's > already defined that the script gets stdin, stdout and stderr from the > dnsmasq process in that case. > > 3) Expand the subject-based logging that already exists, DHCP stuff > comes from dnsmasq-dhcp, script output comes from dhcp-script. That > avoids the wordy preamble to every line otherwise. > > 4) Pull the copy-to-log code out of the loop wait()ing for processes to > die, it makes more sense to iterate until the descriptors close, then > reap child processes. > > 5) Rationalise conditional compilation stuff. There may be more of that > in a subsequent commit. > > 6) Update the man page to reflect new reality (!) > > Any remaining bugs are mine, but Petr please check that I didn't break > things. > > > Cheers, > > Simon. > > > > On 24/03/17 17:38, Petr Mensik wrote: >> Hi! >> >> Some guys using dnsmasq in virtual machines and OpenStack use custom >> dhcp_script to manage leases of clients. >> However they complain if there is anything wrong with them, then are just >> told broken pipe and no information. >> >> We understand it should not produce any output under normal operation. But >> it would be really helpful if at least anything was visible in logs. >> Especially for errors happening under rare circumstances. >> I have prepared patch to forward events from helper. It prevents SIGPIPE >> receiving if script does write anything. And logs it from dnsmasq. >> It seems very handy to me. >> >> It was not simple to forward it to main log. I would like opinions if it is >> useful or dangerous. >> Do you consider it worth merging Simon? >> >> Best Regards, >> Petr >> -- >> Petr Menšík >> Software Engineer >> Red Hat, http://www.redhat.com/ >> email: pemen...@redhat.com PGP: 65C6C973 >> >> >> >&
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
Thank you for accepting that patches. I agree that some garbage is far more likely to appear in dhcp-script mode. I would myself welcome error log from wrong formatted lease file as well. If I understand it well, that file will be overwritten after the first lease created. If it contains wrong data, just log an error, but do not terminate. It should not surprise administrator that just disabled IPv6 support. Error message would be logged once at startup until the first lease is created. Then file will be rewritten without any IPv6 leases, because they were skipped during the reading. I would accept one error message as notification some leases are gone forever. I will have to ask whether failure to start on database corruption is considered a problem. It was silently ignoring all problems before. Now it fails to start the service completely if any error occurs. I think it relied on auto recovery with empty leases in the same way as with plain file. I think there should be a way to override default behavior. I will check for more opinions and get back with results. Thank you Simon. Cheers, Petr -- Petr Menšík Software Engineer Red Hat, http://www.redhat.com/ email: pemen...@redhat.com PGP: 65C6C973 - Original Message - From: "Simon Kelley" To: "Petr Mensik" Cc: dnsmasq-discuss@lists.thekelleys.org.uk Sent: Sunday, April 23, 2017 3:14:08 PM Subject: Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output OK, there's a little subtlety here that needs to be taken into account. The lease file format was not originally designed to be extensible, so the code takes advantage (sort of) of the fact that lines which are associated with IPv6 leases make no sense to older versions of dnsmasq or even current versions compiled without IPv6 support. The order of lines when the file is written is always IPv4 leases duid IPv6 leases (I think duid may not get written in some cases - doesn't matter for the sake of this argument.) Therefore the behaviour of silently giving up when an unrecognisable line is encountered is a feature, not a bug, as it allows old or non-IPv6 dnsmasq binaries to work with newer lease files - they get the IPv4 leases and ignore the rest. Since the main problem you're trying to solve is unexpected error messages from the script, the solution to this is to keep the old behaviour when reading from a file, but the new behaviour when running the script init command. Given that we're only checking when using the init script, I think die()ing is the best thing to do when parsing fails. I've moved your patch on to do that, and to check ferror() on the stream, to catch IO-errors. Again, any new bugs are mine :) Cheers, Simon. On 19/04/17 19:14, Petr Mensik wrote: > Hi Simon, > > I like your changes. New shorter log is definitely helpful, separate log > section helps. The only bug I found is red white space highlight in the patch. > > However I did yet another fix for remaining dhcp-script init action. > It completely ignored any error in structure and silently skipped the rest of > database. If there was any message in stdin of init script, it just died > silently. > The only thing that were visible was SIGPIPE from dhcp-script, because it did > not read whole database, if that signal was logged at all. > My new patch handles garbage in leases database. If the line is wrong, it > logs part of wrong line and skips the rest of init. > I thought about die in that case. I think it would be better for backward > compatibility to start with empty leases as before. > > -- > Petr Menšík > Software Engineer > Red Hat, http://www.redhat.com/ > email: pemen...@redhat.com PGP: 65C6C973 > > - Original Message - > From: "Simon Kelley" > To: dnsmasq-discuss@lists.thekelleys.org.uk > Sent: Sunday, April 16, 2017 9:29:21 PM > Subject: Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output > > I like this. Yes, I know you can do it with shell magic, but this is > easier and what I would expect to happen. > > I've changed the patch quite a lot: > > 1) Don't go to large effort to report "never happen" errors from pipe(), > just silently handle them in the same way as fork() > > 2) Don't do any of this when the -d debug flag is in effect, as it's > already defined that the script gets stdin, stdout and stderr from the > dnsmasq process in that case. > > 3) Expand the subject-based logging that already exists, DHCP stuff > comes from dnsmasq-dhcp, script output comes from dhcp-script. That > avoids the wordy preamble to every line otherwise. > > 4) Pull the copy-to-log code out of the loop wait()ing for processes to > die, it makes more sense to iterate until the descriptors close, then > reap child processes. > &
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
On 24/04/17 15:42, Petr Mensik wrote: > Thank you for accepting that patches. I agree that some garbage is > far more likely to appear in dhcp-script mode. I would myself welcome > error log from wrong formatted lease file as well. If I understand it > well, that file will be overwritten after the first lease created. If > it contains wrong data, just log an error, but do not terminate. That seems sensible. Change committed. > > It should not surprise administrator that just disabled IPv6 support. > Error message would be logged once at startup until the first lease > is created. Then file will be rewritten without any IPv6 leases, > because they were skipped during the reading. I would accept one > error message as notification some leases are gone forever. > Agreed. > I will have to ask whether failure to start on database corruption is > considered a problem. It was silently ignoring all problems before. > Now it fails to start the service completely if any error occurs. I > think it relied on auto recovery with empty leases in the same way as > with plain file. I think there should be a way to override default > behavior. I will check for more opinions and get back with results. > Any news on this? Cheers, Simon. > Thank you Simon. > > Cheers, Petr > > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
Hi Simon, sorry for a delay. We concluded that we do not like die on parse error even for dhcp script. It does always start up now unless script returns nonzero exit status. With this change, it would never start, until you were able to figure out what is wrong with your script. Then fix the cause. Until that, no-one will receive any lease as they did before. When assigning addresses to dhclient that had his address before, dnsmasq will offer requested address if it is not leased. If you start with clear leases, many machines will receive correct addresses again. Dnsmasq is used as small server for containers or virtual hosts on the local machine. I think it is quite useful they will always (try to) start. I think it should be possible to disable dying on parse errors. I think change from totally ignoring parsing errors to being not able to skip them is problematic for real world administrators. I would prefer current behavior as default, but with possible configuration override. I think it would be useful if return code suggested there is problem with lease database. I could then move old leases file and retry with empty database from the startup script. What do you think? Dne 29.4.2017 v 00:03 Simon Kelley napsal(a): > On 24/04/17 15:42, Petr Mensik wrote: >> Thank you for accepting that patches. I agree that some garbage is >> far more likely to appear in dhcp-script mode. I would myself welcome >> error log from wrong formatted lease file as well. If I understand it >> well, that file will be overwritten after the first lease created. If >> it contains wrong data, just log an error, but do not terminate. > > That seems sensible. Change committed. >> >> It should not surprise administrator that just disabled IPv6 support. >> Error message would be logged once at startup until the first lease >> is created. Then file will be rewritten without any IPv6 leases, >> because they were skipped during the reading. I would accept one >> error message as notification some leases are gone forever. >> > Agreed. > >> I will have to ask whether failure to start on database corruption is >> considered a problem. It was silently ignoring all problems before. >> Now it fails to start the service completely if any error occurs. I >> think it relied on auto recovery with empty leases in the same way as >> with plain file. I think there should be a way to override default >> behavior. I will check for more opinions and get back with results. >> > > Any news on this? > > > Cheers, > > Simon. > >> Thank you Simon. >> >> Cheers, Petr >> >> > > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
On 09/05/17 18:49, Petr Menšík wrote: > Hi Simon, > > sorry for a delay. We concluded that we do not like die on parse error > even for dhcp script. It does always start up now unless script returns > nonzero exit status. With this change, it would never start, until you > were able to figure out what is wrong with your script. Then fix the > cause. Until that, no-one will receive any lease as they did before. > > When assigning addresses to dhclient that had his address before, > dnsmasq will offer requested address if it is not leased. If you start > with clear leases, many machines will receive correct addresses again. > Dnsmasq is used as small server for containers or virtual hosts on the > local machine. I think it is quite useful they will always (try to) start. > > I think it should be possible to disable dying on parse errors. I think > change from totally ignoring parsing errors to being not able to skip > them is problematic for real world administrators. > > I would prefer current behavior as default, but with possible > configuration override. I think it would be useful if return code > suggested there is problem with lease database. I could then move old > leases file and retry with empty database from the startup script. > > What do you think? I think that just logging a warning is best. I don't want to add yet another obscure config option. Cheers, Simon. > > Dne 29.4.2017 v 00:03 Simon Kelley napsal(a): >> On 24/04/17 15:42, Petr Mensik wrote: >>> Thank you for accepting that patches. I agree that some garbage is >>> far more likely to appear in dhcp-script mode. I would myself welcome >>> error log from wrong formatted lease file as well. If I understand it >>> well, that file will be overwritten after the first lease created. If >>> it contains wrong data, just log an error, but do not terminate. >> >> That seems sensible. Change committed. >>> >>> It should not surprise administrator that just disabled IPv6 support. >>> Error message would be logged once at startup until the first lease >>> is created. Then file will be rewritten without any IPv6 leases, >>> because they were skipped during the reading. I would accept one >>> error message as notification some leases are gone forever. >>> >> Agreed. >> >>> I will have to ask whether failure to start on database corruption is >>> considered a problem. It was silently ignoring all problems before. >>> Now it fails to start the service completely if any error occurs. I >>> think it relied on auto recovery with empty leases in the same way as >>> with plain file. I think there should be a way to override default >>> behavior. I will check for more opinions and get back with results. >>> >> >> Any news on this? >> >> >> Cheers, >> >> Simon. >> >>> Thank you Simon. >>> >>> Cheers, Petr >>> >>> >> >> > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Logging of dhcp_script output
Dne 10.5.2017 v 00:14 Simon Kelley napsal(a):
> On 09/05/17 18:49, Petr Menšík wrote:
>> Hi Simon,
>>
>> sorry for a delay. We concluded that we do not like die on parse error
>> even for dhcp script. It does always start up now unless script returns
>> nonzero exit status. With this change, it would never start, until you
>> were able to figure out what is wrong with your script. Then fix the
>> cause. Until that, no-one will receive any lease as they did before.
>>
>> When assigning addresses to dhclient that had his address before,
>> dnsmasq will offer requested address if it is not leased. If you start
>> with clear leases, many machines will receive correct addresses again.
>> Dnsmasq is used as small server for containers or virtual hosts on the
>> local machine. I think it is quite useful they will always (try to) start.
>>
>> I think it should be possible to disable dying on parse errors. I think
>> change from totally ignoring parsing errors to being not able to skip
>> them is problematic for real world administrators.
>>
>> I would prefer current behavior as default, but with possible
>> configuration override. I think it would be useful if return code
>> suggested there is problem with lease database. I could then move old
>> leases file and retry with empty database from the startup script.
>>
>> What do you think?
>
> I think that just logging a warning is best. I don't want to add yet
> another obscure config option.
>
> Cheers,
>
> Simon.
>
Thanks. Would be sufficient to only remove condition with die (patch
included)?
I think it is configurable by script anyway. If you have well written
script, you should know when something is not as it should be. If you
return nonzero code from script, dnsmasq will die. If you don't, it will
log an error but start. I think it is simple yet powerful.
>>
>> Dne 29.4.2017 v 00:03 Simon Kelley napsal(a):
>>> On 24/04/17 15:42, Petr Mensik wrote:
Thank you for accepting that patches. I agree that some garbage is
far more likely to appear in dhcp-script mode. I would myself welcome
error log from wrong formatted lease file as well. If I understand it
well, that file will be overwritten after the first lease created. If
it contains wrong data, just log an error, but do not terminate.
>>>
>>> That seems sensible. Change committed.
It should not surprise administrator that just disabled IPv6 support.
Error message would be logged once at startup until the first lease
is created. Then file will be rewritten without any IPv6 leases,
because they were skipped during the reading. I would accept one
error message as notification some leases are gone forever.
>>> Agreed.
>>>
I will have to ask whether failure to start on database corruption is
considered a problem. It was silently ignoring all problems before.
Now it fails to start the service completely if any error occurs. I
think it relied on auto recovery with empty leases in the same way as
with plain file. I think there should be a way to override default
behavior. I will check for more opinions and get back with results.
>>>
>>> Any news on this?
>>>
>>>
>>> Cheers,
>>>
>>> Simon.
>>>
Thank you Simon.
Cheers, Petr
>>>
>>>
>>
>
>
From 4194ffba65a38e76f473d34d3b68d5b6a459a30b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?=
Date: Wed, 10 May 2017 16:13:07 +0200
Subject: [PATCH] Do not die on parse error but require nonzero exit status for
it.
---
src/lease.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/src/lease.c b/src/lease.c
index 5afb99b..56881b4 100644
--- a/src/lease.c
+++ b/src/lease.c
@@ -202,12 +202,6 @@ void lease_init(time_t now)
sprintf(daemon->dhcp_buff, "%d", WEXITSTATUS(rc));
die(_("lease-init script returned exit code %s"), daemon->dhcp_buff, WEXITSTATUS(rc) + EC_INIT_OFFSET);
}
-
- /* Only die if we stopped reading due to a non-parsed line when running script,
- this is expected behaviour when reading from a file, if the file was written with IPv6 data
- and we are not compiled to understand that. */
- if (!readok)
- die(_("failed to read lease-init script output"), NULL, EC_FILE);
}
#endif
--
2.9.3
signature.asc
Description: OpenPGP digital signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
