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

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(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> 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