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