Re: [libvirt] [PATCH] Implement DNS SRV record into the bridge driver

On Tue, Aug 09, 2011 at 05:50:55PM +0200, Michal Novotny wrote: > Hi, > this is the patch to the bridge driver and libvirt XML > file to include support for the SRV records in the DNS. > The syntax is based on DNSMasq man page and tests for > both xml2xml and xml2argv were added as well. > > Signed-off-by: Michal Novotny <minovotn(a)redhat.com&gt; > --- > docs/formatnetwork.html.in | 12 ++ > docs/schemas/network.rng | 30 +++++ > src/conf/network_conf.c | 125 ++++++++++++++++++++ > src/conf/network_conf.h | 14 ++ > src/network/bridge_driver.c | 18 +++ > .../nat-network-dns-srv-record.argv | 1 + > .../nat-network-dns-srv-record.xml | 26 ++++ > tests/networkxml2argvtest.c | 1 + > .../nat-network-dns-srv-record.xml | 26 ++++ > .../nat-network-dns-srv-record.xml | 26 ++++ > tests/networkxml2xmltest.c | 1 + > 12 files changed, 281 insertions(+), 1 deletions(-) > create mode 100644 tests/networkxml2argvdata/nat-network-dns-srv-record.argv > create mode 100644 tests/networkxml2argvdata/nat-network-dns-srv-record.xml > create mode 100644 tests/networkxml2xmlin/nat-network-dns-srv-record.xml > create mode 100644 tests/networkxml2xmlout/nat-network-dns-srv-record.xml > > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index ddfa77c..4f748c4 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -148,6 +148,7 @@ > &lt;mac address='00:16:3E:5D:C7:9E'/&gt; > &lt;dns&gt; > &lt;txt name="example" value="example value" /&gt; > + &lt;srv service="name" protocol="tcp" domain="test-domain-name" target="." port="1024" priority="10" weight="10"/&gt; > &lt;/dns&gt; > &lt;ip address="192.168.122.1" netmask="255.255.255.0"&gt; > &lt;dhcp&gt; > @@ -196,6 +197,17 @@ > <span class="since">Since 0.9.3</span> > </dd> > </dl> > + <dl> > + <dt><code>srv</code></dt> > + <dd>The <code>dns</code> element can have also 0 or more <code>srv</code> > + record elements. Each srv record element defines a DNS SRV record > + and has 2 mandatory and 4 optional attributes. The mandatory attributes > + are service name and protocol (tcp, udp) and you can define optional > + target, port, priority and weight arguments. The body of the element > + have to be set to some data and therefore it's a pair tag. > + <span class="since">Since 0.9.5</span> > + </dd> > + </dl> > </dd> > <dt><code>ip</code></dt> > <dd>The <code>address</code> attribute defines an IPv4 address in > diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng > index 1c44471..413698a 100644 > --- a/docs/schemas/network.rng > +++ b/docs/schemas/network.rng > @@ -138,6 +138,22 @@ > </element> > </zeroOrMore> > <zeroOrMore> > + <element name="srv"> > + <attribute name="service"><text/></attribute> > + <attribute name="protocol"> > + <choice> > + <value>tcp</value> > + <value>udp</value> > + </choice> > + </attribute> > + <attribute name="domain"><text/></attribute> > + <attribute name="target"><text/></attribute> > + <attribute name="port"><ref name="port-for-user"/></attribute>

> + <attribute name="priority"><ref name="short-int"/></attribute> > + <attribute name="weight"><ref name="short-int"/></attribute> > + </element> > + </zeroOrMore> > + <zeroOrMore> > <element name="host"> > <attribute name="ip"><ref name="ipv4Addr"/></attribute> > <oneOrMore> > @@ -206,6 +222,20 @@ > </element> > </define> > > + <define name="port-for-user"> > + <data type="integer"> > + <param name="minInclusive">1024</param> > + <param name="maxInclusive">65535</param> > + </data> > + </define>

> + <define name="short-int"> > + <data type="integer"> > + <param name="minInclusive">0</param> > + <param name="maxInclusive">127</param> > + </data> > + </define> > + > <define name='addr-family'> > <data type='string'> > <param name="pattern">(ipv4)|(ipv6)</param> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index b11c482..120b149 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -137,6 +137,14 @@ static void virNetworkDNSDefFree(virNetworkDNSDefPtr def) > } > VIR_FREE(def->hosts); > } > + if (def->nsrvrecords) { ^^^^ please remove the tab above before commit, are you sure you ran "make syntax-check" ? > + while (def->nsrvrecords--) { > + VIR_FREE(def->srvrecords[def->nsrvrecords].domain); > + VIR_FREE(def->srvrecords[def->nsrvrecords].service); > + VIR_FREE(def->srvrecords[def->nsrvrecords].protocol); > + VIR_FREE(def->srvrecords[def->nsrvrecords].target); > + } > + } > VIR_FREE(def); > } > } > @@ -552,6 +560,107 @@ error: > } > > static int > +virNetworkDNSSrvDefParseXML(virNetworkDNSDefPtr def, > + xmlNodePtr cur) > +{ > + char *domain = NULL; > + char *service = NULL; Somehow I guess Clang or other tools will complain about the extra initialization here, but IMHO that's fine. An option would be to move the def->srvrecords allocation before the fetch of service attribute. > + char *protocol = NULL; > + char *target = NULL; > + char *portString = NULL; > + char *priorityString = NULL; > + char *weightString = NULL; > + int port; > + int priority; > + int weight; > + int ret = 0; > + > + if (!(service = virXMLPropString(cur, "service"))) { > + virNetworkReportError(VIR_ERR_XML_DETAIL, > + "%s", _("Missing required service attribute in dns srv record")); > + goto error; > + } > + if (!(protocol = virXMLPropString(cur, "protocol"))) { > + virNetworkReportError(VIR_ERR_XML_DETAIL, > + _("Missing required protocol attribute in dns srv record '%s'"), service); > + goto error; > + } > + > + target = virXMLPropString(cur, "target"); > + domain = virXMLPropString(cur, "domain"); > + portString = virXMLPropString(cur, "port"); > + priorityString = virXMLPropString(cur, "priority"); > + weightString = virXMLPropString(cur, "weight"); > + > + if (VIR_REALLOC_N(def->srvrecords, def->nsrvrecords + 1) < 0) { > + virReportOOMError(); > + goto error; > + } > + > + if (portString && > + virStrToLong_i(portString, NULL, 10, &port) < 0) { > + virNetworkReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot parse 'port' attribute")); > + goto error; > + } > + > + if (priorityString && > + virStrToLong_i(priorityString, NULL, 10, &priority) < 0) { > + virNetworkReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot parse 'priority' attribute")); > + goto error; > + } > + > + if (weightString && > + virStrToLong_i(weightString, NULL, 10, &weight) < 0) { > + virNetworkReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot parse 'weight' attribute")); > + goto error; > + } Hum, using virXPathInt() using the XPath expressions "@port", "@priority" and "@weight" would probably allow to simplify all this quite a bit, but you would have to extend virNetworkDNSDefParseXML() to carry the xpath context from virNetworkDefParseXML() and update the node for it. > + def->srvrecords[def->nsrvrecords].domain = domain; > + def->srvrecords[def->nsrvrecords].service = service; > + def->srvrecords[def->nsrvrecords].protocol = protocol; > + def->srvrecords[def->nsrvrecords].target = target; > + def->srvrecords[def->nsrvrecords].port = port; > + def->srvrecords[def->nsrvrecords].priority = priority; > + def->srvrecords[def->nsrvrecords].weight = weight; Hum, the range values, service and target should all be checked you need at least the set of minimal check you provided in the rng to be done in the C parsing.

> + def->nsrvrecords++; > + > + goto cleanup; > + > +error: > + if (domain) > + VIR_FREE(domain); > + if (service) > + VIR_FREE(service); > + if (protocol) > + VIR_FREE(protocol); > + if (target) > + VIR_FREE(target); > + > + ret = 1; > + > +cleanup: > + if (portString) > + VIR_FREE(portString); > + if (priorityString) > + VIR_FREE(priorityString); > + if (weightString) > + VIR_FREE(weightString); > + > + domain = NULL; > + service = NULL; > + protocol = NULL; > + target = NULL; > + portString = NULL; > + priorityString = NULL; > + weightString = NULL; > + > + return ret; > +} > + > +static int > virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef, > xmlNodePtr node) > { > @@ -598,6 +707,11 @@ virNetworkDNSDefParseXML(virNetworkDNSDefPtr *dnsdef, > name = NULL; > value = NULL; > } else if (cur->type == XML_ELEMENT_NODE && > + xmlStrEqual(cur->name, BAD_CAST "srv")) { > + ret = virNetworkDNSSrvDefParseXML(def, cur); > + if (ret < 0) > + goto error; > + } else if (cur->type == XML_ELEMENT_NODE && > xmlStrEqual(cur->name, BAD_CAST "host")) { > ret = virNetworkDNSHostsDefParseXML(def, cur); > if (ret < 0) > @@ -1147,6 +1261,17 @@ virNetworkDNSDefFormat(virBufferPtr buf, > def->txtrecords[i].value); > } > > + for (i = 0 ; i < def->nsrvrecords ; i++) { > + virBufferAsprintf(buf, " <srv service='%s' protocol='%s' domain='%s' target='%s' port='%d' priority='%d' weight='%d' />\n", > + def->srvrecords[i].service, > + def->srvrecords[i].protocol, > + def->srvrecords[i].domain, > + def->srvrecords[i].target, > + def->srvrecords[i].port, > + def->srvrecords[i].priority, > + def->srvrecords[i].weight); > + } > + Hum, if attributes are optional is that a good idea to serialize back the default value when saving the XML, I would think the answer is no. But that requires to use special values when parsing like -1 or NULL and check them here. In any case it seems this code would fail and print for example target="(null)" or crash if target wasn't specified in the input XML.

> > diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h > index 869085e..08e08b2 100644 > --- a/src/conf/network_conf.h > +++ b/src/conf/network_conf.h > @@ -67,6 +67,18 @@ struct _virNetworkDNSTxtRecordsDef { > char *value; > }; > > +typedef struct _virNetworkDNSSrvRecordsDef virNetworkDNSSrvRecordsDef; > +typedef virNetworkDNSSrvRecordsDef *virNetworkDNSSrvRecordsDefPtr; > +struct _virNetworkDNSSrvRecordsDef { > + char *domain; > + char *service; > + char *protocol; > + char *target; > + int port; > + int priority; > + int weight; > +}; > + > struct _virNetworkDNSHostsDef { > virSocketAddr ip; > int nnames; > @@ -80,6 +92,8 @@ struct _virNetworkDNSDef { > virNetworkDNSTxtRecordsDefPtr txtrecords; > unsigned int nhosts; > virNetworkDNSHostsDefPtr hosts; > + unsigned int nsrvrecords; > + virNetworkDNSSrvRecordsDefPtr srvrecords; > }; > > typedef struct _virNetworkDNSDef *virNetworkDNSDefPtr; > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index c90db63..e7f3b26 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -559,6 +559,24 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, > virCommandAddArgPair(cmd, "--txt-record", record); > VIR_FREE(record); > } > + > + for (i = 0; i < dns->nsrvrecords; i++) { > + char *record = NULL; > + if (virAsprintf(&record, "%s.%s.%s,%s,%d,%d,%d", > + dns->srvrecords[i].service, > + dns->srvrecords[i].protocol, > + dns->srvrecords[i].domain, > + dns->srvrecords[i].target, > + dns->srvrecords[i].port, > + dns->srvrecords[i].priority, > + dns->srvrecords[i].weight) < 0) { > + virReportOOMError(); > + goto cleanup; > + } okay, but this fully need to be checked for default values... > + virCommandAddArgPair(cmd, "--srv-host", record); > + VIR_FREE(record); > + } > } > > /* > diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv > new file mode 100644 > index 0000000..2ea9809 > --- /dev/null > +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv > @@ -0,0 +1 @@ > +/usr/sbin/dnsmasq --strict-order --bind-interfaces --conf-file= --except-interface lo --srv-host=name.tcp.test-domain-name,.,1024,10,10 --listen-address 192.168.122.1 --listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 --listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 --dhcp-range 192.168.122.2,192.168.122.254 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases --dhcp-lease-max=253 --dhcp-no-override --dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile > \ No newline at end of file > diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.xml b/tests/networkxml2argvdata/nat-network-dns-srv-record.xml > new file mode 100644 > index 0000000..4be85b5 > --- /dev/null > +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.xml > @@ -0,0 +1,26 @@ > +<network> > + <name>default</name> > + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> > + <forward dev='eth1' mode='nat'> > + <interface dev='eth1'/> > + </forward> > + <bridge name='virbr0' stp='on' delay='0' /> > + <dns> > + <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10' /> need to add more SRV records to test for optional attributes handling > + </dns> > + <ip address='192.168.122.1' netmask='255.255.255.0'> > + <dhcp> > + <range start='192.168.122.2' end='192.168.122.254' /> > + <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> > + <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> > + </dhcp> > + </ip> > + <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> > + </ip> > + <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> > + </ip> > + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> > + </ip> > + <ip family='ipv4' address='10.24.10.1'> > + </ip> > +</network> > diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c > index 4a11d6f..a9da613 100644 > --- a/tests/networkxml2argvtest.c > +++ b/tests/networkxml2argvtest.c > @@ -120,6 +120,7 @@ mymain(void) > DO_TEST("netboot-network"); > DO_TEST("netboot-proxy-network"); > DO_TEST("nat-network-dns-txt-record"); > + DO_TEST("nat-network-dns-srv-record"); > DO_TEST("nat-network-dns-hosts"); > > return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); > diff --git a/tests/networkxml2xmlin/nat-network-dns-srv-record.xml b/tests/networkxml2xmlin/nat-network-dns-srv-record.xml > new file mode 100644 > index 0000000..4be85b5 > --- /dev/null > +++ b/tests/networkxml2xmlin/nat-network-dns-srv-record.xml > @@ -0,0 +1,26 @@ > +<network> > + <name>default</name> > + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> > + <forward dev='eth1' mode='nat'> > + <interface dev='eth1'/> > + </forward> > + <bridge name='virbr0' stp='on' delay='0' /> > + <dns> > + <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10' /> > + </dns> > + <ip address='192.168.122.1' netmask='255.255.255.0'> > + <dhcp> > + <range start='192.168.122.2' end='192.168.122.254' /> > + <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> > + <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> > + </dhcp> > + </ip> > + <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> > + </ip> > + <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> > + </ip> > + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> > + </ip> > + <ip family='ipv4' address='10.24.10.1'> > + </ip> > +</network> > diff --git a/tests/networkxml2xmlout/nat-network-dns-srv-record.xml b/tests/networkxml2xmlout/nat-network-dns-srv-record.xml > new file mode 100644 > index 0000000..4be85b5 > --- /dev/null > +++ b/tests/networkxml2xmlout/nat-network-dns-srv-record.xml > @@ -0,0 +1,26 @@ > +<network> > + <name>default</name> > + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> > + <forward dev='eth1' mode='nat'> > + <interface dev='eth1'/> > + </forward> > + <bridge name='virbr0' stp='on' delay='0' /> > + <dns> > + <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10' /> > + </dns> > + <ip address='192.168.122.1' netmask='255.255.255.0'> > + <dhcp> > + <range start='192.168.122.2' end='192.168.122.254' /> > + <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10' /> > + <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11' /> > + </dhcp> > + </ip> > + <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> > + </ip> > + <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> > + </ip> > + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> > + </ip> > + <ip family='ipv4' address='10.24.10.1'> > + </ip> > +</network> > diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c > index 5cdbedb..8ee8e0e 100644 > --- a/tests/networkxml2xmltest.c > +++ b/tests/networkxml2xmltest.c > @@ -87,6 +87,7 @@ mymain(void) > DO_TEST("netboot-network"); > DO_TEST("netboot-proxy-network"); > DO_TEST("nat-network-dns-txt-record"); > + DO_TEST("nat-network-dns-srv-record"); > DO_TEST("nat-network-dns-hosts"); > DO_TEST("8021Qbh-net"); > DO_TEST("direct-net"); I like the principle and directions of the patch but there is some needed checks and cleanup before pushing it :-)