2:31 p.m.
On 12/05/2011 07:03 AM, Michal Novotny wrote:
Hi,
this is the fourth version of my SRV record for DNSMasq patch rebased
for the current codebase 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. This is basically un-reviewed version 3 of this patch rebased to
the current codebase with changed version information to 0.9.9 as it's
supposed to go in 0.9.9 or later because of current 0.9.8 features
freeze.
Also, the second part of this patch is fixing the networkxml2argv test
to pass both checks, i.e. both unit tests and also syntax check.
Please review,
Michal
@@ -217,6 +230,19 @@
</element>
</define>
+ <define name='unsignedShort'>
+ <data type='integer'>
+ <param name="minInclusive">0</param>
+ <param name="maxInclusive">65535</param>
+ </data>
+ </define>
I'm a bit surprised we didn't have a common definition for this before.
It looks like docs/schemas/nwfilter.rng could be made shorter if we
moved this to a common file between the two, but that can be a separate
cleanup.
@@ -553,8 +562,99 @@ error:
}
static int
+virNetworkDNSSrvDefParseXML(virNetworkDNSDefPtr def,
+ xmlNodePtr cur,
+ xmlXPathContextPtr ctxt)
+{
+ char *domain;
+ char *service;
+ char *protocol;
+ char *target;
+ int port;
+ int priority;
+ int weight;
+ int ret = 0;
+ char xpath[1024] = { 0 };
I'm not a fan of a fixed-width array,...
+ /* Following attributes are optional but we had to make sure
their NULL above */
s/their/they're/
+ if ((target = virXMLPropString(cur, "target"))
&& (domain = virXMLPropString(cur, "domain"))) {
+ snprintf(xpath, sizeof(xpath),
"string(//network/dns/srv[@service='%s']/@port)", service);
...along with an unchecked(!) snprintf into that array. It might be
better to modify ctxt->node (for example, see virDomainDiskDefParseXML),
and construct your query directly relative to the node containing the
srv in question.
+error:
+ VIR_FREE(domain);
+ VIR_FREE(service);
+ VIR_FREE(protocol);
+ VIR_FREE(target);
+
+ ret = 1;
Should this be -1?
@@ -1146,6 +1251,27 @@ virNetworkDNSDefFormat(virBufferPtr buf,
def->txtrecords[i].value);
}
+ for (i = 0 ; i < def->nsrvrecords ; i++) {
+ if (def->srvrecords[i].service && def->srvrecords[i].protocol) {
+ virBufferAsprintf(buf, " <srv service='%s'
protocol='%s' ",
Drop the trailing space here...
+ def->srvrecords[i].service,
+ def->srvrecords[i].protocol);
+
+ if (def->srvrecords[i].domain)
+ virBufferAsprintf(buf, "domain='%s' ",
def->srvrecords[i].domain);
...and use leading space rather than trailing space for each of these...
+ if (def->srvrecords[i].target)
+ virBufferAsprintf(buf, "target='%s' ",
def->srvrecords[i].target);
+ if (def->srvrecords[i].port)
+ virBufferAsprintf(buf, "port='%d' ",
def->srvrecords[i].port);
+ if (def->srvrecords[i].priority)
+ virBufferAsprintf(buf, "priority='%d' ",
def->srvrecords[i].priority);
+ if (def->srvrecords[i].weight)
+ virBufferAsprintf(buf, "weight='%d' ",
def->srvrecords[i].weight);
+
+ virBufferAsprintf(buf, "/>\n");
so that this line does not result in a space prior to the />.
--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\
+--dhcp-lease-max=253 \
+--dhcp-no-override \
+--dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile
This changes the file to add a trailing newline, where it used to end
without one (thanks to the backslash at the end). This has a negative
consequence...
+++ b/tests/networkxml2argvtest.c
@@ -18,6 +18,7 @@
static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
char *inXmlData = NULL;
char *outArgvData = NULL;
+ char *actual_tmp = NULL;
char *actual = NULL;
int ret = -1;
virNetworkDefPtr dev = NULL;
@@ -47,9 +48,17 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char
*outargv) {
if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0)
goto fail;
- if (!(actual = virCommandToString(cmd)))
+ if (!(actual_tmp = virCommandToString(cmd)))
goto fail;
+ if (VIR_ALLOC_N( actual, (strlen(actual_tmp) + 3) * sizeof(char) ) < 0)
Style. Libvirt code tends not to use spaces before or after nested
layers of arguments. This would be:
if (VIR_ALLOC_N(actual, (strlen(actual_tmp) + 3) * sizeof(char)) < 0)
That said, sizeof(char) is ALWAYS 1, so multiplying by sizeof(char) is
useless. And why the +3? Since you are only adding one byte (plus room
for trailing NUL), this could be:
if (VIR_ALLOC_N(actual, strlen(actual_tmp) + 2) < 0)
but see below why I think we can get away without this malloc in the
first place.
+ goto fail;
+
+ /* A little hack to make it working for both check and syntax-check */
+ memcpy(actual, actual_tmp, strlen(actual_tmp));
+ actual[ strlen(actual_tmp) ] = '\n';
+ actual[ strlen(actual_tmp) + 1 ] = 0;
...as mentioned above, your change means that there is now a trailing
newline in the expected but not in the actual. Do we really need a
trailing newline in the expected? And if so, it's more efficient to
remove the trailing newline from expected than it is to alloc and memcpy
the actual just to add a newline (that is, it's more efficient to hack
on expected to remove newline than it is to hack actual to add newline).
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org