[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4] Implement DNS SRV record into the bridge driver



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]