On 07/22/14 01:03, Nehal J Wani wrote:

This patch enables the helper program to detect event(s) triggered when there is a change in lease length or expiry and client-id. This transfers complete control of leases database to libvirt and obsoletes use of the lease database file (<network-name>.leases). That file will not be created, read, or written. This is achieved by adding the option --leasefile-ro to dnsmasq and passing a custom env var to leaseshelper, which helps us map events related to leases with their corresponding network bridges, no matter what the event be.

Also, this requires the addition of a new non-lease entry in our custom lease database: "server-duid". It is required to identify a DHCPv6 server.

Now that dnsmasq doesn't maintain its own leases database, it relies on our helper program to tell it about previous leases and server duid. Thus, this patch makes our leases program honor an extra action: "init", in which it sends the known info in a particular format to dnsmasq by printing it to stdout.

--- This is compatible with libvirt 1.2.6 as only additions have been introduced, and the old leases file (*.staus) will still be supported.

v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html

src/network/bridge_driver.c | 7 ++ src/network/leaseshelper.c | 152 +++++++++++++++++++++++++++++++++++++------- 2 files changed, 136 insertions(+), 23 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6a2e760..4363cd8 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1289,7 +1289,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,

cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + /* Libvirt gains full control of leases database */ + virCommandAddArgFormat(cmd, "--leasefile-ro"); virCommandAddArgFormat(cmd, "--dhcp-script=%s", leaseshelper_path); + virCommandAddEnvPair(cmd, "VIR_BRIDGE_NAME", network->def->bridge);

The invocation is now much nicer!

*cmdout = cmd; ret = 0; @@ -3432,6 +3435,10 @@ networkGetDHCPLeases(virNetworkPtr network, goto error; }

+ /* Ignore server-duid. It's not part of a lease */ + if (virJSONValueObjectHasKey(lease_tmp, "server-duid")) + continue;

[1]

+ if (!(mac_tmp = virJSONValueObjectGetString(lease_tmp, "mac-address"))) { /* leaseshelper program guarantees that lease will be stored only if * mac-address is known otherwise not */ diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index e4b5283..cc4b4ac 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c

@@ -89,6 +95,7 @@ enum virLeaseActionFlags { VIR_LEASE_ACTION_ADD, /* Create new lease */ VIR_LEASE_ACTION_OLD, /* Lease already exists, renew it */ VIR_LEASE_ACTION_DEL, /* Delete the lease */ + VIR_LEASE_ACTION_INIT, /* Tell dnsmasq of existing leases on restart */

VIR_LEASE_ACTION_LAST };

@@ -105,6 +112,7 @@ main(int argc, char **argv) char *pid_file = NULL; char *lease_entries = NULL; char *custom_lease_file = NULL; + char *server_duid = NULL; const char *ip = NULL; const char *mac = NULL; const char *iaid = virGetEnvAllowSUID("DNSMASQ_IAID"); @@ -112,20 +120,26 @@ main(int argc, char **argv) const char *interface = virGetEnvAllowSUID("DNSMASQ_INTERFACE"); const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES"); const char *hostname = virGetEnvAllowSUID("DNSMASQ_SUPPLIED_HOSTNAME"); + const char *server_duid_env = virGetEnvAllowSUID("DNSMASQ_SERVER_DUID"); const char *leases_str = NULL; long long currtime = 0; long long expirytime = 0; size_t i = 0; + size_t count_ipv6 = 0; + size_t count_ipv4 = 0; int action = -1; int pid_file_fd = -1; int rv = EXIT_FAILURE; int custom_lease_file_len = 0; bool add = false; + bool init = false; bool delete = false; virJSONValuePtr lease_new = NULL; virJSONValuePtr lease_tmp = NULL; virJSONValuePtr leases_array = NULL; virJSONValuePtr leases_array_new = NULL; + virJSONValuePtr *leases_ipv4 = NULL; + virJSONValuePtr *leases_ipv6 = NULL;

virSetErrorFunc(NULL, NULL); virSetErrorLogPriorityFunc(NULL); @@ -156,17 +170,18 @@ main(int argc, char **argv) } }

- if (argc != 4 && argc != 5) { + if (argc != 4 && argc != 5 && argc != 2) { /* Refer man page of dnsmasq --dhcp-script for more details */ usage(EXIT_FAILURE); }

/* Make sure dnsmasq knows the interface. The interface name is not known - * when dnsmasq (re)starts and throws 'del' events for expired leases. - * So, if any old lease has expired, it will be automatically removed the - * next time this program is invoked */ - if (!interface) - goto cleanup; + * via env variable set by dnsmasq when dnsmasq (re)starts and throws 'del' + * events for expired leases. So, libvirtd sets another env var for this + * purpose */ + if (!interface) { + interface = virGetEnvAllowSUID("VIR_BRIDGE_NAME");

If you upgrade libvirt with a running dnsmasq instance this variable won't be set at that point and if for some reason (although that shouldn't happen). I think we should exit if we don't know the interface as it will be used without checking below. I'd go with: if (!interface && !(interface = virGetEnvAllowSUID("VIR_BRIDGE_NAME"))) goto cleanup; That formatting would also avoid braces around a single line if.

+ }

ip = argv[3]; mac = argv[2]; @@ -185,9 +200,14 @@ main(int argc, char **argv) exptime[strlen(exptime) - 1] = '\0';

/* Check if it is an IPv6 lease */ - if (virGetEnvAllowSUID("DNSMASQ_IAID")) { + if (iaid) { mac = virGetEnvAllowSUID("DNSMASQ_MAC"); clientid = argv[2]; + + if (server_duid_env) { + if (VIR_STRDUP(server_duid, server_duid_env) < 0) + goto cleanup;

The server_duid string is only read hereafter and never ever modified so no need to STRDUP it ...

+ } }

if (virAsprintf(&custom_lease_file, @@ -264,6 +284,8 @@ main(int argc, char **argv) goto cleanup; } } + } else if (action == VIR_LEASE_ACTION_INIT) { + init = true;

The init variable is used just once, you could write the condition inline to save the variable and this condition.

} else { fprintf(stderr, _("Unsupported action: %s\n"), virLeaseActionTypeToString(action)); @@ -294,6 +316,7 @@ main(int argc, char **argv) i = 0; while (i < virJSONValueArraySize(leases_array)) { const char *ip_tmp = NULL; + const char *server_duid_tmp = NULL; long long expirytime_tmp = -1;

if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { @@ -302,6 +325,20 @@ main(int argc, char **argv) goto cleanup; }

+ if ((server_duid_tmp + = virJSONValueObjectGetString(lease_tmp, "server-duid"))) { + /* Control reaches here atmost once */ + if (!server_duid) { + /* Control reaches here when the 'action' is not for an + * ipv6 lease or, for some weird reason the env var + * DNSMASQ_SERVER_DUID wasn't set*/ + if (VIR_STRDUP(server_duid, server_duid_tmp) < 0) + goto cleanup;

Same here, the STRDUP is useless.

+ } + i++; + continue; + } + if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) || (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -328,39 +365,108 @@ main(int argc, char **argv) goto cleanup; }

+ /* Store pointers to ipv4 and ipv6 leases */ + if (strchr(ip_tmp, ':')) + ignore_value(VIR_APPEND_ELEMENT(leases_ipv6, count_ipv6, lease_tmp)); + else + ignore_value(VIR_APPEND_ELEMENT(leases_ipv4, count_ipv4, lease_tmp)); + ignore_value(virJSONValueArraySteal(leases_array, i)); } } }

- if (add) { - if (virJSONValueArrayAppend(leases_array_new, lease_new) < 0) { + if (init) {

This part looks like it would benefit from converting to: switch ((enum virLeaseActionFlags) action) { ...

+ /* Man page of dnsmasq says: the script (helper program, in our case) + * should write the saved state of the lease database, in dnsmasq + * leasefile format, to stdout and exit with zero exit code, when + * called with argument init. Format: + * #For all ipv4 leases: + * Expiry_time MAC_address IP_address Hostname Client-id + * #If DHCPv6 is present: + * duid Server_DUID + * #For all ipv6 leases: + * Expiry_time IAID IP_address Hostname Client-DUID */ + long long expirytime_tmp = -1; + for (i = 0; i < count_ipv4; i++) { + lease_tmp = leases_ipv4[i]; + virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp); + printf("%lld %s %s %s %s\n", + expirytime_tmp, + virJSONValueObjectGetString(lease_tmp, "mac-address"), + virJSONValueObjectGetString(lease_tmp, "ip-address"), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "hostname")), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "client-id"))); + } + if (server_duid) { + printf("duid %s\n", server_duid); + for (i = 0; i < count_ipv6; i++) { + lease_tmp = leases_ipv6[i]; + virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp); + printf("%lld %s %s %s %s\n", + expirytime_tmp, + virJSONValueObjectGetString(lease_tmp, "iaid"), + virJSONValueObjectGetString(lease_tmp, "ip-address"), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "hostname")), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "client-id"))); + } + } + }

closing brace and else shall be on the same line

+ else { + if (add) { + if (virJSONValueArrayAppend(leases_array_new, lease_new) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + lease_new = NULL; + } + + if (server_duid) { + if (!(lease_new = virJSONValueNewObject())) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + + if (virJSONValueObjectAppendString(lease_new, + "server-duid", server_duid) < 0) + goto cleanup;

Hmm this is really odd. Why is the "server_duid" stored as a part of the leases array as it's a completely separate info and occuring only once? Unfortunately, looking at the format of the lease file the array of leases is the top level element. Is there a way we could (without complicating the code too much) convert it to a object so that it's extensible? Storing the duid separately will also avoid the hunk [1].

+ + if (virJSONValueArrayAppend(leases_array_new, lease_new) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + lease_new = NULL; + } + + if (!(leases_str = virJSONValueToString(leases_array_new, true))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to create json")); + _("empty json array")); goto cleanup; } - lease_new = NULL; - }

- if (!(leases_str = virJSONValueToString(leases_array_new, true))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("empty json array")); - goto cleanup; + /* Write to file */ + if (virFileRewrite(custom_lease_file, 0644, + customLeaseRewriteFile, &leases_str) < 0) + goto cleanup; }

- /* Write to file */ - if (virFileRewrite(custom_lease_file, 0644, - customLeaseRewriteFile, &leases_str) < 0) - goto cleanup; - rv = EXIT_SUCCESS;

cleanup: if (pid_file_fd != -1) virPidFileReleasePath(pid_file, pid_file_fd); + for (i = 0; i < count_ipv4; i++) + VIR_FREE(leases_ipv4); + for (i = 0; i < count_ipv6; i++) + VIR_FREE(leases_ipv6);

VIR_FREE(pid_file); VIR_FREE(exptime); + VIR_FREE(server_duid);

And an unnecessary free.

+ VIR_FREE(lease_entries);

The above FREE fixes an existing memory leak, and it's not mentioned in the commit message. Best will be to split that into a separate patch with a separate commit message.

VIR_FREE(custom_lease_file); virJSONValueFree(lease_new); virJSONValueFree(leases_array);

The change to using the ENV variable has turned out great, unfortunately there's a problem with the lease file JSON we need to clear before proceeding. Peter