[libvirt] [PATCH v2] leaseshelper: improvements to support all events
Peter Krempa
pkrempa at redhat.com
Tue Jul 22 15:20:04 UTC 2014
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140722/78c6ee30/attachment-0001.sig>
More information about the libvir-list
mailing list