[libvirt] [PATCH] network: Fix dnsmasq hostsfile creation logic and related tests

2011-06-28 Thread Matthias Bolte 
networkSaveDnsmasqHostsfile was added in 8fa9c2214247 (Apr 2010).
It has a force flag. If the dnsmasq hostsfile already exists force
needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
to false, networkDefine sets it to true. This results in the
hostsfile being written only in networkDefine in the common case.
If no error occurred networkSaveDnsmasqHostsfile returns true and
networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
command line.

networkSaveDnsmasqHostsfile was changed in 89ae9849f744 (24 Jun 2011)
to return a new dnsmasqContext instead of reusing one. This change broke
the logic of the force flag as now networkSaveDnsmasqHostsfile returns
NULL on error, but the early return -- if force was not set and the
hostsfile exists -- returns 0. This turned the early return in an error
case and networkBuildDnsmasqArgv didn't add the --dhcp-hostsfile option
anymore if the hostsfile already exists. It did because networkDefine
created the hostsfile already.

Then 9d4e2845d498 fixed the return 0 case in networkSaveDnsmasqHostsfile
but didn't apply the force option correctly to the new addnhosts file.
Now force doesn't control an early return anymore, but influences the
handling of the hostsfile context creation and dnsmasqSave is always
called now. This commit also added test cases that reveal several
problems. First, the tests now call functions that try to write the
dnsmasq config files to disk. If someone runs this tests as root this
might overwrite actively used dnsmasq config files, this is a no-go. Also
the tests depend onconfigure --localstatedir, this needs to be fixed as
well, because it makes the tests fail when localstatedir is different
from /var.

This patch does several things to fix this:

1) Move dnsmasqContext creation and saving out of networkBuildDnsmasqArgv
to the caller to separate the command line generation from the config
file writing. This makes the command line generation testable without the
risk of interfering with system files, because the tests just don't call
dnsmasqSave.

2) This refactoring of networkSaveDnsmasqHostsfile makes the force flag
useless as the saving happens somewhere else now. This fixes the wrong
usage of the force flag in combination with then newly added addnhosts
file by removing the force flag.

3) Adapt the wrong test cases to the correct behavior, by adding the
missing --dhcp-hostsfile option. Both affected tests contain DHCP host
elements but missed the necessary --dhcp-hostsfile option.

4) Rename networkSaveDnsmasqHostsfile to networkBuildDnsmasqHostsfile,
because it doesn't save the dnsmasqContext anymore.

5) Move all directory creations in dnsmasq context handling code from
the *New functions to dnsmasqSave to avoid directory creations in system
paths in the test cases.

6) Now that networkBuildDnsmasqArgv doesn't create the dnsmasqContext
anymore the test case can create one with the localstatedir that is
expected by the tests instead of the configure --localstatedir given one.
---
 src/network/bridge_driver.c|   71 +---
 src/network/bridge_driver.h|5 +-
 src/util/dnsmasq.c |   28 
 src/util/dnsmasq.h |1 +
 .../nat-network-dns-txt-record.argv|3 +-
 tests/networkxml2argvdata/nat-network.argv |3 +-
 tests/networkxml2argvtest.c|8 ++-
 7 files changed, 63 insertions(+), 56 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index aa170d6..f48fdcb 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -436,27 +436,17 @@ networkShutdown(void) {
 }
 
 
-static dnsmasqContext*
-networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
-virNetworkDNSDefPtr dnsdef,
-char *name,
-bool force)
+static int
+networkBuildDnsmasqHostsfile(dnsmasqContext *dctx,
+ virNetworkIpDefPtr ipdef,
+ virNetworkDNSDefPtr dnsdef)
 {
 unsigned int i, j;
 
-dnsmasqContext *dctx = dnsmasqContextNew(name,
- DNSMASQ_STATE_DIR);
-if (dctx == NULL) {
-virReportOOMError();
-goto cleanup;
-}
-
-if (!(! force  virFileExists(dctx-hostsfile-path))) {
-for (i = 0; i  ipdef-nhosts; i++) {
-virNetworkDHCPHostDefPtr host = (ipdef-hosts[i]);
-if ((host-mac)  VIR_SOCKET_HAS_ADDR(host-ip))
-dnsmasqAddDhcpHost(dctx, host-mac, host-ip, host-name);
-}
+for (i = 0; i  ipdef-nhosts; i++) {
+virNetworkDHCPHostDefPtr host = (ipdef-hosts[i]);
+if ((host-mac)  VIR_SOCKET_HAS_ADDR(host-ip))
+dnsmasqAddDhcpHost(dctx, host-mac, host-ip, host-name);
 }
 
 if (dnsdef) {
@@ -469,15 +459,7 @@

Re: [libvirt] [PATCH] network: Fix dnsmasq hostsfile creation logic and related tests

2011-06-28 Thread Eric Blake 
On 06/28/2011 05:10 AM, Matthias Bolte wrote:
 networkSaveDnsmasqHostsfile was added in 8fa9c2214247 (Apr 2010).
 It has a force flag. If the dnsmasq hostsfile already exists force
 needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
 to false, networkDefine sets it to true. This results in the
 hostsfile being written only in networkDefine in the common case.
 If no error occurred networkSaveDnsmasqHostsfile returns true and
 networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
 command line.

Thanks for the detailed commit message - it was a huge help in reviewing.

The patch is a bit big, but it fixes both a real code problem and the
remaining test failure, so it is appropriate for inclusion in 0.9.3.

ACK.

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: Fix dnsmasq hostsfile creation logic and related tests

2011-06-28 Thread Michal Novotny 
On 06/28/2011 01:10 PM, Matthias Bolte wrote:
 networkSaveDnsmasqHostsfile was added in 8fa9c2214247 (Apr 2010).
 It has a force flag. If the dnsmasq hostsfile already exists force
 needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
 to false, networkDefine sets it to true. This results in the
 hostsfile being written only in networkDefine in the common case.
 If no error occurred networkSaveDnsmasqHostsfile returns true and
 networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
 command line.

 networkSaveDnsmasqHostsfile was changed in 89ae9849f744 (24 Jun 2011)
 to return a new dnsmasqContext instead of reusing one. This change broke
 the logic of the force flag as now networkSaveDnsmasqHostsfile returns
 NULL on error, but the early return -- if force was not set and the
 hostsfile exists -- returns 0. This turned the early return in an error
 case and networkBuildDnsmasqArgv didn't add the --dhcp-hostsfile option
 anymore if the hostsfile already exists. It did because networkDefine
 created the hostsfile already.

 Then 9d4e2845d498 fixed the return 0 case in networkSaveDnsmasqHostsfile
 but didn't apply the force option correctly to the new addnhosts file.
 Now force doesn't control an early return anymore, but influences the
 handling of the hostsfile context creation and dnsmasqSave is always
 called now. This commit also added test cases that reveal several
 problems. First, the tests now call functions that try to write the
 dnsmasq config files to disk. If someone runs this tests as root this
 might overwrite actively used dnsmasq config files, this is a no-go. Also
 the tests depend onconfigure --localstatedir, this needs to be fixed as
 well, because it makes the tests fail when localstatedir is different
 from /var.

 This patch does several things to fix this:

 1) Move dnsmasqContext creation and saving out of networkBuildDnsmasqArgv
 to the caller to separate the command line generation from the config
 file writing. This makes the command line generation testable without the
 risk of interfering with system files, because the tests just don't call
 dnsmasqSave.

 2) This refactoring of networkSaveDnsmasqHostsfile makes the force flag
 useless as the saving happens somewhere else now. This fixes the wrong
 usage of the force flag in combination with then newly added addnhosts
 file by removing the force flag.

 3) Adapt the wrong test cases to the correct behavior, by adding the
 missing --dhcp-hostsfile option. Both affected tests contain DHCP host
 elements but missed the necessary --dhcp-hostsfile option.

 4) Rename networkSaveDnsmasqHostsfile to networkBuildDnsmasqHostsfile,
 because it doesn't save the dnsmasqContext anymore.

 5) Move all directory creations in dnsmasq context handling code from
 the *New functions to dnsmasqSave to avoid directory creations in system
 paths in the test cases.

 6) Now that networkBuildDnsmasqArgv doesn't create the dnsmasqContext
 anymore the test case can create one with the localstatedir that is
 expected by the tests instead of the configure --localstatedir given one.
 ---
  src/network/bridge_driver.c|   71 
 +---
  src/network/bridge_driver.h|5 +-
  src/util/dnsmasq.c |   28 
  src/util/dnsmasq.h |1 +
  .../nat-network-dns-txt-record.argv|3 +-
  tests/networkxml2argvdata/nat-network.argv |3 +-
  tests/networkxml2argvtest.c|8 ++-
  7 files changed, 63 insertions(+), 56 deletions(-)

 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index aa170d6..f48fdcb 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -436,27 +436,17 @@ networkShutdown(void) {
  }
  
  
 -static dnsmasqContext*
 -networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
 -virNetworkDNSDefPtr dnsdef,
 -char *name,
 -bool force)
 +static int
 +networkBuildDnsmasqHostsfile(dnsmasqContext *dctx,
 + virNetworkIpDefPtr ipdef,
 + virNetworkDNSDefPtr dnsdef)
  {
  unsigned int i, j;
  
 -dnsmasqContext *dctx = dnsmasqContextNew(name,
 - DNSMASQ_STATE_DIR);
 -if (dctx == NULL) {
 -virReportOOMError();
 -goto cleanup;
 -}
 -
 -if (!(! force  virFileExists(dctx-hostsfile-path))) {
 -for (i = 0; i  ipdef-nhosts; i++) {
 -virNetworkDHCPHostDefPtr host = (ipdef-hosts[i]);
 -if ((host-mac)  VIR_SOCKET_HAS_ADDR(host-ip))
 -dnsmasqAddDhcpHost(dctx, host-mac, host-ip, host-name);
 -}
 +for (i = 0; i  ipdef-nhosts; i++) {
 +virNetworkDHCPHostDefPtr host = (ipdef-hosts[i]);
 +if ((host-mac)  VIR_SOCKET_HAS_ADDR(host-ip))
 +

Re: [libvirt] [PATCH] network: Fix dnsmasq hostsfile creation logic and related tests

2011-06-28 Thread Matthias Bolte 
2011/6/28 Eric Blake ebl...@redhat.com:
 On 06/28/2011 05:10 AM, Matthias Bolte wrote:
 networkSaveDnsmasqHostsfile was added in 8fa9c2214247 (Apr 2010).
 It has a force flag. If the dnsmasq hostsfile already exists force
 needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
 to false, networkDefine sets it to true. This results in the
 hostsfile being written only in networkDefine in the common case.
 If no error occurred networkSaveDnsmasqHostsfile returns true and
 networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
 command line.

 Thanks for the detailed commit message - it was a huge help in reviewing.

 The patch is a bit big, but it fixes both a real code problem and the
 remaining test failure, so it is appropriate for inclusion in 0.9.3.

 ACK.


Thanks, pushed.

-- 
Matthias Bolte
http://photron.blogspot.com

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list