[libvirt] [PATCHv2 3/4] net-dhcp-leases: Private implementation inside network driver
Osier Yang
jyang at redhat.com
Fri Sep 13 10:03:47 UTC 2013
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 = {
More information about the libvir-list
mailing list