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
From cf2831d52884452f039e05cc16f6562cbe6db650 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
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 "<lease_change_script> 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
-	{
-          file_dirty = dns_dirty = 0;
-          return;
-        }
+  int found = 0;
+  int items;
+
+  *daemon->dhcp_buff3 = *daemon->dhcp_buff2 = '\0';
+
+  /* client-id max length is 255 which is 255*2 digits + 254 colons
+     borrow DNS packet buffer which is always larger than 1000 bytes
 
-    }
-  else
-    {
-      /* NOTE: need a+ mode to create file if it doesn't exist */
-      leasestream = daemon->lease_stream = fopen(daemon->lease_file, "a+");
-      
-      if (!leasestream)
-	die(_("cannot open or create lease file %s: %s"), daemon->lease_file, EC_FILE);
-      
-      /* a+ mode leaves pointer at end. */
-      rewind(leasestream);
-    }
-  
-  /* client-id max length is 255 which is 255*2 digits + 254 colons 
-     borrow DNS packet buffer which is always larger than 1000 bytes 
-  
      Check various buffers are big enough for the code below */
 
 #if (DHCP_BUFF_SZ < 255) || (MAXDNAME < 64) || (PACKETSZ+MAXDNAME+RRFIXEDSZ  < 764)
-# error Buffer size breakage in leasefile parsing. 
+# error Buffer size breakage in leasefile parsing.
 #endif
 
-  if (leasestream)
-    while (fscanf(leasestream, "%255s %255s", daemon->dhcp_buff3, daemon->dhcp_buff2) == 2)
+    while ((items=fscanf(leasestream, "%255s %255s", daemon->dhcp_buff3, daemon->dhcp_buff2)) == 2)
       {
+	*daemon->namebuff = *daemon->dhcp_buff = *daemon->packet = '\0';
 #ifdef HAVE_DHCP6
 	if (strcmp(daemon->dhcp_buff3, "duid") == 0)
 	  {
 	    daemon->duid_len = parse_hex(daemon->dhcp_buff2, (unsigned char *)daemon->dhcp_buff2, 130, NULL, NULL);
+	    if (daemon->duid_len < 0)
+	      return 0;
 	    daemon->duid = safe_malloc(daemon->duid_len);
 	    memcpy(daemon->duid, daemon->dhcp_buff2, daemon->duid_len);
+            found = 1;
 	    continue;
 	  }
 #endif
@@ -90,7 +61,7 @@ void lease_init(time_t now)
 	
 	if (fscanf(leasestream, " %64s %255s %764s",
 		   daemon->namebuff, daemon->dhcp_buff, daemon->packet) != 3)
-	  break;
+	    return 0;
 	
 	clid_len = 0;
 	if (strcmp(daemon->packet, "*") != 0)
@@ -109,6 +80,7 @@ void lease_init(time_t now)
 	    
 	    if (strcmp(daemon->dhcp_buff, "*") !=  0)
 	      lease_set_hostname(lease, daemon->dhcp_buff, 0, get_domain(lease->addr), NULL);
+	    found = 1;
 	  }
 #ifdef HAVE_DHCP6
 	else if (inet_pton(AF_INET6, daemon->namebuff, &addr.addr.addr6))
@@ -131,11 +103,12 @@ void lease_init(time_t now)
 		lease_set_iaid(lease, iaid);
 		if (strcmp(daemon->dhcp_buff, "*") !=  0)
 		  lease_set_hostname(lease, daemon->dhcp_buff, 0, get_domain6((struct in6_addr *)lease->hwaddr), NULL);
+		found = 1;
 	      }
 	  }
 #endif
 	else
-	  break;
+	  return 0;
 
 	if (!lease)
 	  die (_("too many stored leases"), NULL, EC_MISC);
@@ -155,7 +128,54 @@ void lease_init(time_t now)
 	/* set these correctly: the "old" events are generated later from
 	   the startup synthesised SIGHUP. */
 	lease->flags &= ~(LEASE_NEW | LEASE_CHANGED);
+	*daemon->dhcp_buff3 = *daemon->dhcp_buff2 = '\0';
       }
+    return (items == 0 || items == EOF) && found;
+}
+
+void lease_init(time_t now)
+{
+  FILE *leasestream;
+
+  leases_left = daemon->dhcp_max;
+
+  if (option_bool(OPT_LEASE_RO))
+    {
+      /* run "<lease_change_script> 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
+	{
+          file_dirty = dns_dirty = 0;
+          return;
+        }
+
+    }
+  else
+    {
+      /* NOTE: need a+ mode to create file if it doesn't exist */
+      leasestream = daemon->lease_stream = fopen(daemon->lease_file, "a+");
+
+      if (!leasestream)
+	die(_("cannot open or create lease file %s: %s"), daemon->lease_file, EC_FILE);
+
+      /* a+ mode leaves pointer at end. */
+      rewind(leasestream);
+    }
+
+  if (leasestream && !read_leases(now, leasestream))
+    my_syslog(MS_DHCP | LOG_ERR, _("aborting lease init, invalid line: %s %s %s %s *"),
+              daemon->dhcp_buff3, daemon->dhcp_buff2,
+              daemon->namebuff, daemon->dhcp_buff);
   
 #ifdef HAVE_SCRIPT
   if (!daemon->lease_stream)
-- 
2.9.3

_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to