[libvirt] [PATCH] Implement DNS SRV record into the bridge driver
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> + <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) { + 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; + 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); + + 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); + } + 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; + } + + 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"); -- 1.7.4.4
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> + <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.
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; + }
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 :-) thanks ! 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/
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/
Thanks for your review, Daniel. I already sent v2 of the patch now ;-) Michal On 08/11/2011 04:42 AM, Daniel Veillard wrote:
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
-- Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat Virtualization | libvirt-php bindings | php-virt-control.org
On 08/10/2011 10:13 PM, Daniel Veillard wrote:
On Tue, Aug 09, 2011 at 05:50:55PM +0200, Michal Novotny wrote:
+ 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.
A nice side effect would be that it would get the ctxt passed down through virNetworkDNSDefParseXML(), which could then use virXPathNodeSet to get the list of all srv records (and txt records and host records) at once, rather than picking them out in a loop. (not for doing now, but would be a nice cleanup in the future).
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@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");
participants (3)
-
Daniel Veillard -
Laine Stump -
Michal Novotny