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@xxxxxxxxxx> >>> --- >>> 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@xxxxxxxxxx>, RHCE, Red Hat Virtualization | libvirt-php bindings | php-virt-control.org -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list