[libvirt] [PATCH] Implement DNS SRV record into the bridge driver
Daniel Veillard
veillard at redhat.com
Thu Aug 11 02:42:18 UTC 2011
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 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
> > + 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 at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list