On Thu, Aug 11, 2011 at 10:13:34AM +0800, Daniel Veillard wrote:
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@redhat.com> --- 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 @@ <mac address='00:16:3E:5D:C7:9E'/> <dns> <txt name="example" value="example value" /> + <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> @@ -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>
Actually I just looked at http://tools.ietf.org/html/rfc2782
+ <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>
and the full set of allowed port values is correct for an SRV record page 3: Port The port on this target host of this service. The range is 0- 65535. This is a 16 bit unsigned integer in network byte order. This is often as specified in Assigned Numbers but need not be. is that a limitation of dnsmasq ?
+ <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.
and protocol must be checked for being "udp" or "tcp" only One of the warning raised in the RFC is that there is often a limit of 512 bytes for DNS UDP messages so maybe the service name lenght should be checked too, along with the characters used.
+ 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.
Also I think we need to either verify the service string on input or use virBufferEscapeString to serialize it back,
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 :-)
Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/