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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list