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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list