[libvirt] [PATCHv3 3/4] net-dhcp-leases: Private implementation inside network driver

[Eric Blake]

On 09/15/2013 11:49 PM, Nehal J Wani wrote:
> By querying the driver for the path of the leases file for the given virtual
> network and parsing it to retrieve info.
> 
> src/network/bridge_driver.c:
>   * Implement networkGetDHCPLeases
>   * Implement networkGetDHCPLeasesForMAC
>   * Implement networkGetDHCPLeasesHelper
> 
> ---
>  src/network/bridge_driver.c | 177 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 177 insertions(+)
> 

>  }
>  
> +/* This function parese the leases file of dnsmasq.

s/parese/parses/

> + *
> + * An example of leases file content:
> + *
> + * 1379024255 52:54:00:20:70:3d 192.168.105.240 * *
> + * 1379023351 52:54:00:b1:70:19 192.168.105.201 * *
> + */
> +static int
> +networkGetDHCPLeasesHelper(virNetworkPtr network,
> +                           virNetworkObjPtr obj,
> +                           const char *mac,
> +                           virNetworkDHCPLeasesPtr **leases)
> +{
> +    int rv = -1;
> +    size_t i = 0;
> +    size_t nleases = 0;
> +    char *leasefile;
> +    char **lease_fields;
> +    char dhcpentry[VIR_NETWORK_DHCP_LEASE_LENGTH_MAX];
> +    virNetworkDHCPLeasesPtr *leases_ret = NULL;
> +    FILE *fp = NULL;
> +
> +    if (!network) {
> +        virReportError(VIR_ERR_NO_NETWORK, "%s",
> +                       _("no network with matching name"));
> +        return -1;
> +    }

Dead code (network is always non-NULL if you get here).

> +
> +    /* Retrieve leases file location */
> +    leasefile = networkDnsmasqLeaseFileNameDefault(network->name);
> +    if (!(fp = fopen(leasefile, "r"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Unable to open leases file: %s"), leasefile);
> +        goto cleanup;
> +    }
> +
> +    while (fgets(dhcpentry, sizeof(dhcpentry), fp) != NULL) {

Rather than hand-rolling your file reading, I would suggest using
virFileReadAll(leasefile, len, &buf), and then loop over the returned
buffer.  That gives you nicer guarantees about handling things like
interrupts that fgets() might get confused on.

> +        virNetworkDHCPLeasesPtr lease = NULL;
> +
> +        /* Remove newline */
> +        dhcpentry[strlen(dhcpentry) - 1] = '\0';
> +
> +        /* Split the lease line */
> +        lease_fields = virStringSplit(dhcpentry, " ", VIR_NETWORK_DHCP_LEASE_FIELDS);
> +
> +        if (virStringListLength(lease_fields) != VIR_NETWORK_DHCP_LEASE_FIELDS) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Number of lease params aren't equal to: %d"),
> +                           VIR_NETWORK_DHCP_LEASE_FIELDS);
> +            goto error;
> +        }
> +
> +        if (mac && STRNEQ(mac, lease_fields[1]))

This may not do what you want - remember, 'mac' is specified by the
user, whereas lease_fields[1] is specified by dnsmasq.  While we can
reasonably assume that dnsmasq always uses a canonical MAC
representation (such as 52:54:00:1a:0a:6d), the user may have passed in
a non-canonical form that we can still parse and recognize as the same
address (such as 52:54:00:1A:0a:6D, or even 52:54:0:1a:a:6d).  You want
to use virMacAddrCompare() instead of STRNEQ.

> +            continue;

Memleak.  lease_fields is allocated each iteration of the loop, so it
must be freed each iteration of the loop.

> +
> +        if (VIR_EXPAND_N(leases_ret, nleases, 1) < 0)
> +            goto error;
> +
> +        if (VIR_ALLOC(leases_ret[nleases - 1]) < 0)
> +            goto error;

Rather than trying to allocate it into the destination array, it might
be better to allocate into a local variable, and only on successful
population of the entire element, use VIR_INSERT_ELEMENT to move it into
the array at that time.

> +
> +        lease = leases_ret[nleases - 1];
> +
> +        /* Convert expirytime here */
> +        if (virStrToLong_ll(lease_fields[0], NULL, 10, &(lease->expirytime)) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Unable to convert lease expiry time to integer: %s"),
> +                           lease_fields[0]);
> +            goto error;
> +        }
> +
> +        /* Hardcoded, as dnsmasq uses ipv4 */
> +        lease->type = VIR_IP_ADDR_TYPE_IPV4;
> +        lease->prefix = virSocketAddrGetIpPrefix(&obj->def->ips->address,
> +                                                 &obj->def->ips->netmask,
> +                                                 obj->def->ips->prefix);
> +
> +        if ((VIR_STRDUP(lease->mac, lease_fields[1]) < 0) ||
> +            (VIR_STRDUP(lease->ipaddr, lease_fields[2]) < 0) ||
> +            (VIR_STRDUP(lease->hostname, lease_fields[3]) < 0) ||
> +            (VIR_STRDUP(lease->clientid, lease_fields[4]) < 0))
> +            goto error;
> +    }

Transfer semantics rather than VIR_STRDUP might save some memory pressure.

> +
> +    if (mac && !leases_ret) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("no lease with matching MAC address: %s"), mac);
> +        goto error;
> +    }
> +
> +    if (leases_ret) {
> +        /* NULL terminated array */
> +        ignore_value(VIR_REALLOC_N(leases_ret, nleases + 1));
> +        *leases = leases_ret;
> +        leases_ret = NULL;
> +        rv = nleases;
> +    }
> +
> +cleanup:
> +    VIR_FORCE_FCLOSE(fp);
> +    VIR_FREE(leasefile);
> +    return rv;
> +
> +error:
> +    if (leases_ret) {
> +        for (i = 0; i < nleases; i++)
> +            virNetworkDHCPLeaseFree(leases_ret[i]);
> +    }
> +    VIR_FREE(leases_ret);
> +    goto cleanup;
> +}
> +


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130916/42858954/attachment-0001.sig>