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" <si...@thekelleys.org.uk> To: "Petr Mensik" <pemen...@redhat.com> 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 <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" <si...@thekelleys.org.uk> > 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 > _______________________________________________ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
