[libvirt] [PATCH v2 1/5] Network: Add TXT record support for virtual DNS service
Laine Stump
laine at laine.org
Wed Apr 27 15:08:46 UTC 2011
Sorry for the long delay in responding to these patches. Unfortunately,
they will no longer apply with "git am", so it's difficult to look at
them "in-tree", but I think they need some revision anyway :-)
On 04/01/2011 06:45 AM, Michal Novotny wrote:
> Make: passed
> Make check: passed
> Make syntax-check: passed
>
> Hi,
> this is the patch to add support for adding TXT records to the
> DNS service running on the virtual network. This has been tested
> on Fedora-14 i386 box and tests are also added to RelaxNG schema
> and test XML files.
Since this comment will appear in the commit logs for all posterity, the
information about passing make check etc shouldn't really be here (that
should be implied by the fact that it's committed! :-) Also, the "this
is the patch" part and telling the exact setup of the testing are things
useful for annotations in the email, but not when pushing the patch, so
are more appropriate for the 0/5 email.
Conversely, in your 0/5 introduction, you gave an example of the new XML
- that would be useful in this commit comment so that someone could
easily search the git log to learn when support was added for this feature.
> Since spaces are not allowed for the TXT records in DNS they
> are rejected
> and "TXT records in DNS doesn't support spaces"
> error message is being output to the user.
You don't really need to say the exact log message in the commit comment
> It's been tested and checked/syntax-checked and everything was
> working fine.
Again, the sentence above should be in the introductory email, but is
superfluous here.
> Also, the formatnetwork HTML document has been altered to
> include those information about new DNS tag.
"Also the network XML documentation has been updated to describe the new
<dns> element."
> Michal
Again, signature not needed in a commit comment - it's implied in the
email address.
> Signed-off-by: Michal Novotny<minovotn at redhat.com>
> ---
> docs/formatnetwork.html.in | 24 ++++++-
> docs/schemas/network.rng | 12 +++
> src/conf/network_conf.c | 71 ++++++++++++++++++++
> src/conf/network_conf.h | 16 +++++
> src/network/bridge_driver.c | 15 ++++-
> .../nat-network-dns-txt-record.xml | 24 +++++++
> .../nat-network-dns-txt-record.xml | 24 +++++++
> tests/networkxml2xmltest.c | 1 +
> 8 files changed, 185 insertions(+), 2 deletions(-)
> create mode 100644 tests/networkxml2xmlin/nat-network-dns-txt-record.xml
> create mode 100644 tests/networkxml2xmlout/nat-network-dns-txt-record.xml
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index c6969eb..5211ed2 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -108,7 +108,10 @@
> The final set of elements define the addresses (IPv4 and/or
> IPv6, as well as MAC) to be assigned to the bridge device
> associated with the virtual network, and optionally enable DHCP
> - services.
> + services. The network creation also supports the TXT record in
> + the DNS to expose some information to the guest using this
> + record. This feature could be used in the similar way like DKIM
> + uses TXT records of DNS to expose public key.
> </p>
That needs some rewording, but should also be moved into its own
paragraph (see my next comment) anyway. Possibly it will end up being
something like:
dns
The dns element of a network contains configuration information for the
virtual network's DNS server. <span class="since">Since 0.9.1</span>
Currently supported elements are:
txt (note that I am changing the name from "txt-record" to "txt" - I
think the "record" is kind of implied)
A dns element can have 0 or more txt elements. Each txt element defines
a DNS TXT record and has two attributes, both required: a name that can
be queried via dns, and a value that will be returned when that name is
queried. names cannot contain embedded spaces or commas. value is a
single string that can contain multiple values which are
comma-separated. (Does each value create a separate TXT record with the
same name, or are they all together really a single value that happens
to have commas? I ask this because if it is the former, the
implementation may differ on other DNS servers - we want to avoid
embedding idiosyncracies of the dnsmasq implementation into our design!)
>
> <pre>
> @@ -120,6 +123,9 @@
> <host mac="00:16:3e:77:e2:ed" name="foo.example.com" ip="192.168.122.10" />
> <host mac="00:16:3e:3e:a9:1a" name="bar.example.com" ip="192.168.122.11" />
> </dhcp>
> +<dns>
> +<txt-record name="example" value="example value" />
> +</dns>
Since there is a single instance of dnsmasq listening on all IPs defined
for a network, and the txt records will be visible to all of them, I
think the DNS section should be one level up in the tree - at the same
level as IP rather than below it, ie:
<network>
<name>default</name>
<dns>
<txt name="example" value="example value" />
</dns>
...
</network>
> </ip>
> </network></pre>
>
> @@ -199,6 +205,22 @@
> element is used. The BOOTP options currently have to be the same
> for all address ranges and statically assigned addresses.<span
> class="since">Since 0.7.1 (<code>server</code> since 0.7.3).</span>
> +</dd><dt><code>dns</code></dt><dd>Also within the<code>ip</code> element
> + there is an optional<code>dns</code> element. The presence of this element
> + enables configuration and exposal of records in the DNS service on the
> + virtual network. It will further contain one or more<code>txt-record</code>
> + elements. The<code>dns</code> element is supported for both IPv4 and IPv6
> + networks.<span class="since">Since 0.9.1</span>
> +</dd>
> +<dt><code>txt-record</code></dt>
> +<dd>The<code>txt-record</code> element is the definition of TXT record for the
> + DNS service. There are two attributes that both have to be used for the TXT
> + record definition:<code>name</code> and<code>value</code>. The<code>name
> +</code>attribute doesn't support commas in it's value so the names with commas
> + will be rejected. This applies only to names of the TXT record and not values
> + since values (i.e.<code>value</code> contents) supports multiple values
> + separated by commas.
> +<span class="since">Since 0.9.1</span>
> </dd>
> </dl>
>
See my above comment. Note that some of what you say here doesn't apply
if you move the <dns> record up to the top level of the hierarchy in
<network>.
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 6d01b06..e27dace 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -136,6 +136,18 @@
> </optional>
> </element>
> </optional>
> +<optional>
> +<!-- Define the DNS related elements like TXT records
> + and other features -->
> +<element name="dns">
> +<zeroOrMore>
> +<element name="txt-record">
I prefer simply "txt" instead of "txt-record", since 1) it avoids having
a - in the name, 2) that's what it's called in a DNS zone file, 3) the
"-record" is really an artifact of the dnsmasq implementation (note that
dnsmasq inconsistently uses "srv-host" and "cname", along with
"ptr-record" and "naptr-record" - these are *all* "records" in the DNS
zone file.
> +<attribute name="name"><text/></attribute>
> +<attribute name="value"><text/></attribute>
> +</element>
> +</zeroOrMore>
> +</element>
> +</optional>
This will move up one level. I won't bother pointing out the other
places where code will change due to moving <dns> up in the hierarchy...
> </element>
> </zeroOrMore>
> </interleave>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index dcab9de..b7427d0 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -435,6 +435,60 @@ virNetworkDHCPRangeDefParseXML(const char *networkName,
> }
>
> static int
> +virNetworkDNSDefParseXML(virNetworkIpDefPtr def,
> + xmlNodePtr node)
> +{
> +
> + xmlNodePtr cur;
> + int result = -1;
> +
> + if (VIR_ALLOC(def->dns))
> + goto oom_error;
> +
> + cur = node->children;
> + while (cur != NULL) {
> + if (cur->type == XML_ELEMENT_NODE&&
> + xmlStrEqual(cur->name, BAD_CAST "txt-record")) {
> + char *name, *value;
> +
> + if (!(name = virXMLPropString(cur, "name"))) {
> + cur = cur->next;
> + continue;
> + }
> + if (!(value = virXMLPropString(cur, "value"))) {
> + VIR_FREE(name);
> + cur = cur->next;
> + continue;
> + }
> +
> + if (strchr(name, ' ') != NULL) {
> + virNetworkReportError(VIR_ERR_XML_DETAIL,
> + _("TXT record names in DNS doesn't support spaces"));
s/doesn't/don't/. Also, an error message is much more helpful if it
provides context - adding the name and value to the message would be
very useful.
> + return -1;
You leaked both name and value here.
> + }
> +
> + if (VIR_REALLOC_N(def->dns->txtrecords, def->dns->ntxtrecords + 1)< 0)
> + goto oom_error;
You again leaked name and value. (def->dns->txtrecords and def->dns will
be cleaned up by the caller as it destructs the def).
> + def->dns->txtrecords[def->dns->ntxtrecords].name = strdup(name);
> + def->dns->txtrecords[def->dns->ntxtrecords].value = strdup(value);
> + def->dns->ntxtrecords++;
> +
> + VIR_FREE(name);
> + VIR_FREE(value);
Instead of doing a strdup() of each of these, immediately followed by
freeing the original, you should just assign the original string into
the def.
> + }
> +
> + cur = cur->next;
> + }
> +
> + return 0;
> +
> +oom_error:
> + virReportOOMError();
> + return result;
I actually prefer having a single exit, with virReportOOMError() called
at the location of the failure, and your "return 0" turned into "ret =
0". You could even do a VIR_FREE(name); VIR_FREE(value); here (after
setting them to NULL when you move the pointer into the txtrecord
array). That way new error path code will never forget to free them, and
as a bonus the variable "ret" is actually used for something.
> +}
> +
> +static int
> virNetworkIPParseXML(const char *networkName,
> virNetworkIpDefPtr def,
> xmlNodePtr node,
> @@ -550,6 +604,12 @@ virNetworkIPParseXML(const char *networkName,
> goto error;
>
> } else if (cur->type == XML_ELEMENT_NODE&&
> + xmlStrEqual(cur->name, BAD_CAST "dns")) {
> + result = virNetworkDNSDefParseXML(def, cur);
> + if (result)
> + goto error;
> +
Okay, I can't stop myself :-) - this should be up a level, in
virNetworkParseXML().
> + } else if (cur->type == XML_ELEMENT_NODE&&
> xmlStrEqual(cur->name, BAD_CAST "tftp")) {
> char *root;
>
> @@ -828,6 +888,17 @@ virNetworkIpDefFormat(virBufferPtr buf,
>
> virBufferAddLit(buf, "</dhcp>\n");
> }
> + if ((def->dns != NULL)&& (def->dns->ntxtrecords)) {
> + int ii;
> +
> + virBufferAddLit(buf, "<dns>\n");
> + for (ii = 0 ; ii< def->dns->ntxtrecords ; ii++) {
> + virBufferVSprintf(buf, "<txt-record name='%s' value='%s' />\n",
> + def->dns->txtrecords[ii].name,
> + def->dns->txtrecords[ii].value);
> + }
> + virBufferAddLit(buf, "</dns>\n");
> + }
same.
>
> virBufferAddLit(buf, "</ip>\n");
>
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 281124b..5f47595 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -57,6 +57,20 @@ struct _virNetworkDHCPHostDef {
> virSocketAddr ip;
> };
>
> +typedef struct _virNetworkDNSTxtRecordsDef virNetworkDNSTxtRecordsDef;
> +typedef virNetworkDNSTxtRecordsDef *virNetworkDNSTxtRecordsDefPtr;
> +struct _virNetworkDNSTxtRecordsDef {
> + char *name;
> + char *value;
> +};
> +
> +struct virNetworkDNSDef {
> + unsigned int ntxtrecords;
> + virNetworkDNSTxtRecordsDefPtr txtrecords;
> +} virNetworkDNSDef;
> +
> +typedef struct virNetworkDNSDef *virNetworkDNSDefPtr;
> +
> typedef struct _virNetworkIpDef virNetworkIpDef;
> typedef virNetworkIpDef *virNetworkIpDefPtr;
> struct _virNetworkIpDef {
> @@ -75,6 +89,8 @@ struct _virNetworkIpDef {
> unsigned int nranges; /* Zero or more dhcp ranges */
> virNetworkDHCPRangeDefPtr ranges;
>
> + virNetworkDNSDefPtr dns; /* DNS related settings for DNSMasq */
> +
Move this into virNetworkDef (and no need to mention dnsmasq here, as
this file could be used by other implementations in the future).
> unsigned int nhosts; /* Zero or more dhcp hosts */
> virNetworkDHCPHostDefPtr hosts;
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index ea2bfd4..2e299f5 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -442,7 +442,6 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
> return 0;
> }
>
> -
> static int
> networkBuildDnsmasqArgv(virNetworkObjPtr network,
> virNetworkIpDefPtr ipdef,
> @@ -497,6 +496,20 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
> if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE)
> virCommandAddArg(cmd, "--dhcp-option=3");
>
> + if (ipdef->dns != NULL) {
> + int i;
> +
> + for (i = 0; i< ipdef->dns->ntxtrecords; i++) {
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> + virBufferVSprintf(&buf, "%s,%s",
> + ipdef->dns->txtrecords[i].name,
> + ipdef->dns->txtrecords[i].value);
> +
> + virCommandAddArgPair(cmd, "--txt-record", virBufferContentAndReset(&buf));
> + VIR_FREE(buf);
> + }
> + }
> +
> /*
> * --interface does not actually work with dnsmasq< 2.47,
> * due to DAD for ipv6 addresses on the interface.
> diff --git a/tests/networkxml2xmlin/nat-network-dns-txt-record.xml b/tests/networkxml2xmlin/nat-network-dns-txt-record.xml
> new file mode 100644
> index 0000000..d3e795d
> --- /dev/null
> +++ b/tests/networkxml2xmlin/nat-network-dns-txt-record.xml
> @@ -0,0 +1,24 @@
> +<network>
> +<name>default</name>
> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> +<forward dev='eth1' mode='nat'/>
> +<bridge name='virbr0' stp='on' delay='0' />
> +<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>
> +<dns>
> +<txt-record name='example' value='example value' />
> +</dns>
> +</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-txt-record.xml b/tests/networkxml2xmlout/nat-network-dns-txt-record.xml
> new file mode 100644
> index 0000000..d3e795d
> --- /dev/null
> +++ b/tests/networkxml2xmlout/nat-network-dns-txt-record.xml
> @@ -0,0 +1,24 @@
> +<network>
> +<name>default</name>
> +<uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> +<forward dev='eth1' mode='nat'/>
> +<bridge name='virbr0' stp='on' delay='0' />
> +<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>
> +<dns>
> +<txt-record name='example' value='example value' />
> +</dns>
> +</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 7805548..beb00ef 100644
> --- a/tests/networkxml2xmltest.c
> +++ b/tests/networkxml2xmltest.c
> @@ -90,6 +90,7 @@ mymain(int argc, char **argv)
> DO_TEST("nat-network");
> DO_TEST("netboot-network");
> DO_TEST("netboot-proxy-network");
> + DO_TEST("nat-network-dns-txt-record");
>
> return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
> }
More information about the libvir-list
mailing list