[libvirt] [PATCH v3] leaseshelper: improvements to support all events
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 (*.status) will still be supported. v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known. v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html src/network/bridge_driver.c | 3 + src/network/leaseshelper.c | 132 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 26 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 965fdec..b578b3a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1288,7 +1288,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); *cmdout = cmd; ret = 0; diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..e984cbb 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -50,6 +50,12 @@ */ #define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX (32 * 1024 * 1024) +/* + * Use this when passing possibly-NULL strings to printf-a-likes. + * Required for unknown parameters during init call. + */ +#define EMPTY_STR(s) ((s) ? (s) : "*") + static const char *program_name; /* Display version information. */ @@ -65,7 +71,7 @@ usage(int status) if (status) { fprintf(stderr, _("%s: try --help for more details\n"), program_name); } else { - printf(_("Usage: %s add|old|del mac|clientid ip [hostname]\n" + printf(_("Usage: %s add|old|del|init mac|clientid ip [hostname]\n" "Designed for use with 'dnsmasq --dhcp-script'\n" "Refer to man page of dnsmasq for more details'\n"), program_name); @@ -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 }; @@ -96,7 +103,7 @@ enum virLeaseActionFlags { VIR_ENUM_DECL(virLeaseAction); VIR_ENUM_IMPL(virLeaseAction, VIR_LEASE_ACTION_LAST, - "add", "old", "del"); + "add", "old", "del", "init"); int main(int argc, char **argv) @@ -112,20 +119,24 @@ 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 = 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 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,16 +167,17 @@ 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) + * 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"))) goto cleanup; ip = argv[3]; @@ -176,6 +188,10 @@ main(int argc, char **argv) if (argc == 5) hostname = argv[4]; + /* In case hostname is still unkown, use the last known one */ + if (!hostname) + hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME"); + if (VIR_STRDUP(exptime, exptime_tmp) < 0) goto cleanup; @@ -185,7 +201,7 @@ 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]; } @@ -235,7 +251,6 @@ main(int argc, char **argv) delete = true; if (action == VIR_LEASE_ACTION_ADD || action == VIR_LEASE_ACTION_OLD) { - add = true; /* Create new lease */ if (!(lease_new = virJSONValueNewObject())) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -260,11 +275,13 @@ main(int argc, char **argv) goto cleanup; if (clientid && virJSONValueObjectAppendString(lease_new, "client-id", clientid) < 0) goto cleanup; + if (server_duid && virJSONValueObjectAppendString(lease_new, "server-duid", server_duid) < 0) + goto cleanup; if (expirytime && virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) goto cleanup; } } - } else { + } else if (action != VIR_LEASE_ACTION_INIT) { fprintf(stderr, _("Unsupported action: %s\n"), virLeaseActionTypeToString(action)); exit(EXIT_FAILURE); @@ -294,7 +311,7 @@ main(int argc, char **argv) i = 0; while (i < virJSONValueArraySize(leases_array)) { const char *ip_tmp = NULL; - long long expirytime_tmp = -1; + const char *server_duid_tmp = NULL; if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -303,14 +320,13 @@ main(int argc, char **argv) } if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) || - (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0)) { + (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime_tmp < currtime) { + if (expirytime < currtime) { i++; continue; } @@ -321,6 +337,30 @@ main(int argc, char **argv) continue; } + /* Store pointers to ipv4 and ipv6 leases */ + if (strchr(ip_tmp, ':')) { + /* This is an ipv6 lease */ + ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv6, count_ipv6, lease_tmp)); + if ((server_duid_tmp + = virJSONValueObjectGetString(lease_tmp, "server-duid"))) { + 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*/ + server_duid = server_duid_tmp; + } + } else { + /* Inject server-duid into those ipv6 leases which + * didn't have it previously, for example, those + * created by leaseshelper from libvirt 1.2.6 */ + if (virJSONValueObjectAppendString(lease_tmp, "server-duid", server_duid) < 0) + goto cleanup; + } + } else { + /* This is an ipv4 lease */ + ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv4, count_ipv4, lease_tmp)); + } + /* Move old lease to new array */ if (virJSONValueArrayAppend(leases_array_new, lease_tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -333,31 +373,71 @@ main(int argc, char **argv) } } - if (add) { + switch ((enum virLeaseActionFlags) action) { + case VIR_LEASE_ACTION_INIT: + /* 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: + * $expirytime $mac $ip $hostname $clientid # For all ipv4 leases + * duid $server-duid # If DHCPv6 is present + * $expirytime $iaid $ip $hostname $clientduid # For all ipv6 leases */ + for (i = 0; i < count_ipv4; i++) { + lease_tmp = leases_ipv4[i]; + virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime); + printf("%lld %s %s %s %s\n", + expirytime, + 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); + printf("%lld %s %s %s %s\n", + expirytime, + virJSONValueObjectGetString(lease_tmp, "iaid"), + virJSONValueObjectGetString(lease_tmp, "ip-address"), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "hostname")), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "client-id"))); + } + } + break; + + case VIR_LEASE_ACTION_OLD: + case VIR_LEASE_ACTION_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 (!(leases_str = virJSONValueToString(leases_array_new, true))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("empty json array")); - goto cleanup; - } + default: + 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); -- 1.9.3
On Mon, Aug 4, 2014 at 1:29 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> 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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
src/network/bridge_driver.c | 3 + src/network/leaseshelper.c | 132 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 965fdec..b578b3a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c
Ping!
On Mon, Aug 11, 2014 at 1:14 AM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
On Mon, Aug 4, 2014 at 1:29 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> 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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
src/network/bridge_driver.c | 3 + src/network/leaseshelper.c | 132 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 26 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 965fdec..b578b3a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c
Ping!
Ping! -- Nehal J Wani
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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
JFI: I did a basic testing and it's working fine for me so far.
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..e984cbb 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c
@@ -176,6 +188,10 @@ main(int argc, char **argv) if (argc == 5) hostname = argv[4];
+ /* In case hostname is still unkown, use the last known one */
s/unkown/unknown/
+ if (!hostname) + hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME"); +
Roman Bogorodskiy
On Wed, Aug 20, 2014 at 4:23 PM, Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
JFI: I did a basic testing and it's working fine for me so far.
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..e984cbb 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c
@@ -176,6 +188,10 @@ main(int argc, char **argv) if (argc == 5) hostname = argv[4];
+ /* In case hostname is still unkown, use the last known one */
s/unkown/unknown/
+ if (!hostname) + hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME"); +
Roman Bogorodskiy
Ping! -- Nehal J Wani
On Wed, Aug 20, 2014 at 4:23 PM, Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
JFI: I did a basic testing and it's working fine for me so far.
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..e984cbb 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c
@@ -176,6 +188,10 @@ main(int argc, char **argv) if (argc == 5) hostname = argv[4];
+ /* In case hostname is still unkown, use the last known one */
s/unkown/unknown/
+ if (!hostname) + hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME"); +
Roman Bogorodskiy
Ping! -- Nehal J Wani
On 08/04/14 09:59, 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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
src/network/bridge_driver.c | 3 + src/network/leaseshelper.c | 132 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 26 deletions(-)
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..e984cbb 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c
@@ -260,11 +275,13 @@ main(int argc, char **argv) goto cleanup; if (clientid && virJSONValueObjectAppendString(lease_new, "client-id", clientid) < 0) goto cleanup; + if (server_duid && virJSONValueObjectAppendString(lease_new, "server-duid", server_duid) < 0) + goto cleanup; if (expirytime && virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) goto cleanup; } } - } else { + } else if (action != VIR_LEASE_ACTION_INIT) { fprintf(stderr, _("Unsupported action: %s\n"), virLeaseActionTypeToString(action)); exit(EXIT_FAILURE); @@ -294,7 +311,7 @@ main(int argc, char **argv) i = 0; while (i < virJSONValueArraySize(leases_array)) { const char *ip_tmp = NULL; - long long expirytime_tmp = -1; + const char *server_duid_tmp = NULL;
if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -303,14 +320,13 @@ main(int argc, char **argv) }
if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) || - (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0)) { + (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime_tmp < currtime) { + if (expirytime < currtime) { i++; continue; } @@ -321,6 +337,30 @@ main(int argc, char **argv) continue; }
+ /* Store pointers to ipv4 and ipv6 leases */ + if (strchr(ip_tmp, ':')) { + /* This is an ipv6 lease */
Is there a need to separate them? Can't we just flush them out from the json array in the order they are stored?
+ ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv6, count_ipv6, lease_tmp));
It would save us the need to copy the leases separately even if dnsmasq isn't initializing. Also you should probably check the return value, otherwise you will drop leases silently on OOM.
+ if ((server_duid_tmp + = virJSONValueObjectGetString(lease_tmp, "server-duid"))) { + 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*/ + server_duid = server_duid_tmp; + } + } else { + /* Inject server-duid into those ipv6 leases which + * didn't have it previously, for example, those + * created by leaseshelper from libvirt 1.2.6 */ + if (virJSONValueObjectAppendString(lease_tmp, "server-duid", server_duid) < 0) + goto cleanup;
This part is required all the time as our format wasn't complete.
+ } + } else { + /* This is an ipv4 lease */ + ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv4, count_ipv4, lease_tmp));
Again, do we need to sort them explicitly?
+ } + /* Move old lease to new array */ if (virJSONValueArrayAppend(leases_array_new, lease_tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -333,31 +373,71 @@ main(int argc, char **argv) } }
- if (add) { + switch ((enum virLeaseActionFlags) action) { + case VIR_LEASE_ACTION_INIT: + /* 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: + * $expirytime $mac $ip $hostname $clientid # For all ipv4 leases + * duid $server-duid # If DHCPv6 is present + * $expirytime $iaid $ip $hostname $clientduid # For all ipv6 leases */
Here you could just go through the json array and format each lease.
+ for (i = 0; i < count_ipv4; i++) { + lease_tmp = leases_ipv4[i]; + virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime); + printf("%lld %s %s %s %s\n", + expirytime, + 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); + printf("%lld %s %s %s %s\n", + expirytime, + virJSONValueObjectGetString(lease_tmp, "iaid"), + virJSONValueObjectGetString(lease_tmp, "ip-address"), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "hostname")), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "client-id"))); + } + } + break; + + case VIR_LEASE_ACTION_OLD: + case VIR_LEASE_ACTION_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 (!(leases_str = virJSONValueToString(leases_array_new, true))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("empty json array")); - goto cleanup; - } + default: + 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);
Otherwise looks good. Peter
On Thu, Sep 11, 2014 at 8:38 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On 08/04/14 09:59, 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 (*.status) will still be supported.
v3: * Add server-duid as an entry in the lease object for every ipv6 lease. * Remove unnecessary variables and double copies. * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
src/network/bridge_driver.c | 3 + src/network/leaseshelper.c | 132 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 26 deletions(-)
diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..e984cbb 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c
@@ -260,11 +275,13 @@ main(int argc, char **argv) goto cleanup; if (clientid && virJSONValueObjectAppendString(lease_new, "client-id", clientid) < 0) goto cleanup; + if (server_duid && virJSONValueObjectAppendString(lease_new, "server-duid", server_duid) < 0) + goto cleanup; if (expirytime && virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) goto cleanup; } } - } else { + } else if (action != VIR_LEASE_ACTION_INIT) { fprintf(stderr, _("Unsupported action: %s\n"), virLeaseActionTypeToString(action)); exit(EXIT_FAILURE); @@ -294,7 +311,7 @@ main(int argc, char **argv) i = 0; while (i < virJSONValueArraySize(leases_array)) { const char *ip_tmp = NULL; - long long expirytime_tmp = -1; + const char *server_duid_tmp = NULL;
if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -303,14 +320,13 @@ main(int argc, char **argv) }
if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) || - (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime_tmp) < 0)) { + (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime_tmp < currtime) { + if (expirytime < currtime) { i++; continue; } @@ -321,6 +337,30 @@ main(int argc, char **argv) continue; }
+ /* Store pointers to ipv4 and ipv6 leases */ + if (strchr(ip_tmp, ':')) { + /* This is an ipv6 lease */
Is there a need to separate them? Can't we just flush them out from the json array in the order they are stored?
+ ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv6, count_ipv6, lease_tmp));
It would save us the need to copy the leases separately even if dnsmasq isn't initializing.
I am only copying the pointer to the lease and not the lease itself.
Also you should probably check the return value, otherwise you will drop leases silently on OOM.
Agreed.
+ if ((server_duid_tmp + = virJSONValueObjectGetString(lease_tmp, "server-duid"))) { + 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*/ + server_duid = server_duid_tmp; + } + } else { + /* Inject server-duid into those ipv6 leases which + * didn't have it previously, for example, those + * created by leaseshelper from libvirt 1.2.6 */ + if (virJSONValueObjectAppendString(lease_tmp, "server-duid", server_duid) < 0) + goto cleanup;
This part is required all the time as our format wasn't complete.
"server-duid" should be a part of every ipv6 lease. It doesn't make sense to add it to an ipv4 lease. Hence the above hunk is required only when we find an ipv6 lease without server-duid.
+ } + } else { + /* This is an ipv4 lease */ + ignore_value(VIR_APPEND_ELEMENT_COPY(leases_ipv4, count_ipv4, lease_tmp));
Again, do we need to sort them explicitly?
I am not sorting anything!
+ } + /* Move old lease to new array */ if (virJSONValueArrayAppend(leases_array_new, lease_tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -333,31 +373,71 @@ main(int argc, char **argv) } }
- if (add) { + switch ((enum virLeaseActionFlags) action) { + case VIR_LEASE_ACTION_INIT: + /* 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: + * $expirytime $mac $ip $hostname $clientid # For all ipv4 leases + * duid $server-duid # If DHCPv6 is present + * $expirytime $iaid $ip $hostname $clientduid # For all ipv6 leases */
Here you could just go through the json array and format each lease.
I considered that, but then since the requirement of DNSMASQ is to print ipv4 leases first and then ipv6 leases, I would have to iterate over the JSON array twice! (Writing to the file would be a third iteration). I thought the extra space wouldn't matter as long as we are not increasing the runtime. Also, I am only copying the pointer to the leases in the JSON array, and not the complete lease itself. Do you still think this is a bad idea?
+ for (i = 0; i < count_ipv4; i++) { + lease_tmp = leases_ipv4[i]; + virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime); + printf("%lld %s %s %s %s\n", + expirytime, + 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); + printf("%lld %s %s %s %s\n", + expirytime, + virJSONValueObjectGetString(lease_tmp, "iaid"), + virJSONValueObjectGetString(lease_tmp, "ip-address"), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "hostname")), + EMPTY_STR(virJSONValueObjectGetString(lease_tmp, "client-id"))); + } + } + break; + + case VIR_LEASE_ACTION_OLD: + case VIR_LEASE_ACTION_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 (!(leases_str = virJSONValueToString(leases_array_new, true))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("empty json array")); - goto cleanup; - } + default: + 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);
Otherwise looks good.
Peter
Thanks for reviewing the patch. If we decide to remove the 'pointer copy' stuff, then I'll send v4. If not, then I'll handle the OOM cases and then send v4. -- Nehal J Wani
On 09/11/2014 10:21 AM, Nehal J Wani wrote:
Here you could just go through the json array and format each lease.
I considered that, but then since the requirement of DNSMASQ is to print ipv4 leases first and then ipv6 leases, I would have to iterate over the JSON array twice! (Writing to the file would be a third iteration). I thought the extra space wouldn't matter as long as we are not increasing the runtime. Also, I am only copying the pointer to the leases in the JSON array, and not the complete lease itself. Do you still think this is a bad idea?
Going through the array twice is not a problem algorithmically (that's still O(n), with just a slightly larger constant). Sorting, on the other hand, is O(n log n) minimum complexity. Sometimes, simplicity of multiple loops outweighs complexity of trying to optimize prematurely.
Otherwise looks good.
Peter
Thanks for reviewing the patch. If we decide to remove the 'pointer copy' stuff, then I'll send v4. If not, then I'll handle the OOM cases and then send v4.
At this point, I don't have any strong opinion on which approach is better, so go ahead and send v4 with the version you like. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Nehal J Wani -
Peter Krempa -
Roman Bogorodskiy