On 13/09/13 05:53, 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 | 193 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3a8be90..2cdea56 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -109,6 +109,36 @@ static int networkPlugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface); static int networkUnplugBandwidth(virNetworkObjPtr net, virDomainNetDefPtr iface); +/** + * VIR_NETWORK_DHCPLEASE_LENGTH_MAX: + * + * Macro providing the maximum length of an entry in the leases file + * Refer: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q3/007402.html + */ +#define VIR_NETWORK_DHCPLEASE_LENGTH_MAX 2048 + +/** + * VIR_NETWORK_DHCPLEASE_PARAMS: + * + * Macro providing the maximum number of parameters in an entry in + * the leases file + */ +#define VIR_NETWORK_DHCPLEASE_FIELDS 5
s/DHCPLEASE/DHCP_LEASE/,
+ +static int networkGetDHCPLeases(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags); + +static int networkGetDHCPLeasesForMAC(virNetworkPtr network, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags); + +static int networkGetDHCPLeasesHelper(virNetworkPtr network, + virNetworkObjPtr obj, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags);
Not a big deal, but these function declarations can be removed.
static virNetworkDriverStatePtr driverState = NULL;
@@ -2980,6 +3010,167 @@ cleanup: return ret; }
+static int +networkGetDHCPLeases(virNetworkPtr network, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + virNetworkObjPtr obj; + + virCheckFlags(0, -1); + + obj = networkObjFromNetwork(network); + + if (virNetworkGetDHCPLeasesEnsureACL(network->conn, obj->def) < 0) + goto cleanup; + + rv = networkGetDHCPLeasesHelper(network, obj, NULL, leases, flags); + +cleanup: + if (obj) + virNetworkObjUnlock(obj); + return rv; +} + +static int +networkGetDHCPLeasesForMAC(virNetworkPtr network, + const char *mac, + virNetworkDHCPLeasesPtr **leases, + unsigned int flags) +{ + int rv = -1; + virNetworkObjPtr obj; + + virCheckFlags(0, -1); + + obj = networkObjFromNetwork(network); + + if (virNetworkGetDHCPLeasesForMACEnsureACL(network->conn, obj->def) < 0) + goto cleanup; + + rv = networkGetDHCPLeasesHelper(network, obj, mac, leases, flags); + +cleanup: + if (obj) + virNetworkObjUnlock(obj); + return rv; +} + +/* This function parese the leases file generated by dnsmasq.
s/generated by/of/,
+ * Example content: + * :::::::::::::: + * /var/lib/libvirt/dnsmasq/TestNetwork1.leases + * ::::::::::::::
Not good comment style, especially when the real comment has ":" character, I'd like write it as: * 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 * *
+ * 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, + unsigned int flags) +{ + int rv = -1; + size_t i = 0; + char *leasefile; + FILE *fp = NULL; + size_t nleases = 0; + char **leaseparams;
To be consistent with the macro name, this should be lease_fields or leasefields.
+ virNetworkDHCPLeasesPtr *leases_ret = NULL; + char dhcpentry[VIR_NETWORK_DHCPLEASE_LENGTH_MAX];
s/DHCPLEASE/DHCP_LEASE/,
+ + virCheckFlags(0, -1); +
Redundant flags checking. You already did it in both networkGetDHCPLeases and networkGetDHCPLeasesForMAC
+ if (!network) { + virReportError(VIR_ERR_NO_NETWORK, "%s", + _("no network with matching name")); + return -1; + } + + /* Retrive 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 error;
s/error/cleanup/, since you don't have to free the lease_ret if fails at this point.
+ } + + while (fgets(dhcpentry, sizeof(dhcpentry), fp) != NULL) { + virNetworkDHCPLeasesPtr lease = NULL; + + /* Remove newline */ + dhcpentry[strlen(dhcpentry) - 1] = '\0'; + + /* split the lease line */ + leaseparams = virStringSplit(dhcpentry, " ", VIR_NETWORK_DHCPLEASE_FIELDS); + + if (virStringListLength(leaseparams) != VIR_NETWORK_DHCPLEASE_FIELDS) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Number of lease params aren't equal to: %d"), + VIR_NETWORK_DHCPLEASE_FIELDS); + goto error; + } + + if (mac && STRNEQ(mac, leaseparams[1])) + continue; + + /* We don't know the total number of leases yet */
This comment can be removed, it doesn't tell what the code does clearly, instead, it's confused. The code explain itself clearly.
+ if (VIR_EXPAND_N(leases_ret, nleases, 1) < 0) + goto error; + + if (VIR_ALLOC(leases_ret[nleases - 1]) < 0) + goto error; + + lease = leases_ret[nleases - 1]; + + /* Convert expirytime here */ + if (virStrToLong_ll(leaseparams[0], NULL, 10, &(lease->expirytime)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to convert lease expiry time to integer: %s"), + leaseparams[0]); + goto error; + } + + /* Hardcoded, as dnsmasq uses ipv4 */ + lease->type = VIR_IP_ADDR_TYPE_IPV4; + lease->prefix = virSocketAddrGetNumNetmaskBits(&obj->def->ips->netmask);
Hm, virSocketAddrGetNumNetmaskBits is used as a general purpose function, but its name is too specific. But it doesn't relate with this patch.
+ + if ((VIR_STRDUP(lease->mac, leaseparams[1]) < 0) || + (VIR_STRDUP(lease->ipaddr, leaseparams[2]) < 0) || + (VIR_STRDUP(lease->hostname, leaseparams[3]) < 0) || + (VIR_STRDUP(lease->clientid, leaseparams[4]) < 0)) + goto error; + } + + if (mac && !leases_ret) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no lease with matching MAC address: %s"), mac); + goto error; + } + + if (leases_ret) { + /* trim the array to the final size */
Confused comment, you are not trimming anything. Consider about: /* 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; +}
static virNetworkDriver networkDriver = { "Network", @@ -3004,6 +3195,8 @@ static virNetworkDriver networkDriver = { .networkSetAutostart = networkSetAutostart, /* 0.2.1 */ .networkIsActive = networkIsActive, /* 0.7.3 */ .networkIsPersistent = networkIsPersistent, /* 0.7.3 */ + .networkGetDHCPLeases = networkGetDHCPLeases, /* 1.1.3 */ + .networkGetDHCPLeasesForMAC = networkGetDHCPLeasesForMAC, /* 1.1.3 */ };
static virStateDriver networkStateDriver = {