[libvirt] [PATCH] Implement DNS SRV record into the bridge driver
Laine Stump
laine at laine.org
Thu Aug 11 04:10:04 UTC 2011
On 08/09/2011 11:50 AM, 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 at 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
I prefer to not use "you" in documentation. Instead maybe say "and the
optional attributes are target, port, priority, and weight."
(I've been thinking, it would be useful to reference the appropriate RFC
for each of these record types.)
> + 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.
I don't understand that last sentence.
> +<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>
We could maybe be more restrictive with this - it's basically anything
allowed in a domain name, plus "_".
> +<attribute name="protocol">
> +<choice>
> +<value>tcp</value>
> +<value>udp</value>
> +</choice>
> +</attribute>
> +<attribute name="domain"><text/></attribute>
> +<attribute name="target"><text/></attribute>
<text/> is a reasonable start for these two. But both are domain names,
so they can be restricted to only characters allowed in a domain name,
i.e. <ref name="dnsName"/> (defined in basictypes.rng)
> +<attribute name="port"><ref name="port-for-user"/></attribute>
You've defined a special "port-for-user" type that only allows
unprivileged ports (1024-65535), but the RFC (2782) doesn't have that
limitation - it says anything 0-65535 is okay. I think we should allow
anything the RFC says is legal.
> +<attribute name="priority"><ref name="short-int"/></attribute>
> +<attribute name="weight"><ref name="short-int"/></attribute>
You've defined priority and weight as type "short-int", and defined
short-int as 0-127. However, in the SRV RFC (rfc2782), it says that both
of these attributes can also take any value between 0 and 65535.
> +</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>
In addition to correcting the overly restrictive range, rather than
setting a minInclusive, you could just make type="unsignedInt" and set
maxInclusive.
> +</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) {
> + 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);
> + }
> + }
You need to VIR_FREE(def->srvrecords) here (do it outside the "if
(def->nsrcrecords)", just in case the block was allocated but no records
were added (e.g. some sort of failure during parse). (Hmm, actually I
think that would be a good cleanup patch for the record types that are
already there. So I just sent it to the list :-)
> VIR_FREE(def);
> }
> }
> @@ -552,6 +560,107 @@ error:
> }
>
> static int
> +virNetworkDNSSrvDefParseXML(virNetworkDNSDefPtr def,
> + xmlNodePtr cur)
> +{
> + char *domain = NULL;
> + char *service = NULL;
> + 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;
> + }
> +
> + 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;
> + 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);
make syntax-check would have told you that you shouldn't put the "if
(domain)" etc prior to all these VIR_FREEs - VIR_FREE already checks for
NULL, so just call it straight.
> +
> + ret = 1;
> +
> +cleanup:
> + if (portString)
> + VIR_FREE(portString);
> + if (priorityString)
> + VIR_FREE(priorityString);
> + if (weightString)
> + VIR_FREE(weightString);
Same here - drop the conditional.
> +
> + domain = NULL;
> + service = NULL;
> + protocol = NULL;
> + target = NULL;
> + portString = NULL;
> + priorityString = NULL;
> + weightString = NULL;
These are all dead stores (and VIR_FREE sets them all to NULL anyway).
> +
> + 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);
> + }
> +
As Daniel said, the optional attributes shouldn't be output if they
weren't set during the parse (which could mean setting special values
for the ones that are integers; maybe it would be simpler to just store
them as strings, and simply check that they are valid numbers rather
than going all the way and converting them - after all, libvirt never
does anything with those integers except convert them back into strings
to insert in the dnsmasq commandline).
> if (def->nhosts) {
> int ii, j;
>
> 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;
> + }
Again, if they're optional and were missing during parse, don't output
them here.
> + 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' />
> +</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");
More information about the libvir-list
mailing list