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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org