[libvirt] [PATCH v2] Add helper program to create custom leases
Introduce helper program to catch events from dnsmasq and maintain a custom lease file per network. It supports dhcpv4 and dhcpv6. The file is saved as "<interface-name>.status". Each lease contains the following info: <expiry-time (epoch time)> <mac> <iaid> <ip-address> <hostname> <clientid> Example of custom leases file content: [ { "expiry-time": "1390775837", "mac-address": "52:54:00:93:8c:63", "iaid": "*", "ip-address": "192.168.150.209", "hostname": "iit-ad885e4aa1", "client-id": "01:52:54:00:44:7c:d7" }, { "expiry-time": "1390775950", "mac-address": "52:54:00:7b:6f:ba", "iaid": "8089530", "ip-address": "2001:db8:ca2:2:1::6d", "hostname": "*", "client-id": "00:04:76:00:cf:ae:b3:0b:fc:cd:0e:22:2e:97:76:65:74:ec" } ] src/Makefile.am: * Add options to compile the helper program src/network/bridge_driver.c: * Introduce networkDnsmasqLeaseFileNameCustom() * Invoke helper program along with dnsmasq * Delete the .status file when corresponding n/w is destroyed. src/util/leaseshelper.c * Helper program to create the custom lease file --- v2: * Changed format to JSON v1: * Refer: https://www.redhat.com/archives/libvir-list/2014-January/msg00626.html src/Makefile.am | 20 ++++ src/network/bridge_driver.c | 19 ++++ src/util/leaseshelper.c | 271 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 310 insertions(+) create mode 100644 src/util/leaseshelper.c diff --git a/src/Makefile.am b/src/Makefile.am index 7844efa..1fa6263 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -840,6 +840,9 @@ STORAGE_HELPER_DISK_SOURCES = \ UTIL_IO_HELPER_SOURCES = \ util/iohelper.c +UTIL_LEASES_HELPER_SOURCES = \ + util/leaseshelper.c + # Network filters NWFILTER_DRIVER_SOURCES = \ nwfilter/nwfilter_driver.h nwfilter/nwfilter_driver.c \ @@ -2410,6 +2413,23 @@ libvirt_iohelper_CFLAGS = \ $(NULL) endif WITH_LIBVIRTD +if WITH_LIBVIRTD +libexec_PROGRAMS += libvirt_leaseshelper +libvirt_leaseshelper_SOURCES = $(UTIL_LEASES_HELPER_SOURCES) +libvirt_leaseshelper_LDFLAGS = \ + $(NULL) +libvirt_leaseshelper_LDADD = \ + libvirt_util.la \ + ../gnulib/lib/libgnu.la +if WITH_DTRACE_PROBES +libvirt_leaseshelper_LDADD += libvirt_probes.lo +endif WITH_DTRACE_PROBES + +libvirt_leaseshelper_CFLAGS = \ + $(PIE_CFLAGS) \ + $(NULL) +endif WITH_LIBVIRTD + if WITH_STORAGE_DISK if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_parthelper diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0b43a67..5440aef 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -148,6 +148,16 @@ networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName = networkDnsmasqLeaseFileNameDefault; static char * +networkDnsmasqLeaseFileNameCustom(const char *bridge) +{ + char *leasefile; + + ignore_value(virAsprintf(&leasefile, "%s/%s.status", + driverState->dnsmasqStateDir, bridge)); + return leasefile; +} + +static char * networkDnsmasqConfigFileName(const char *netname) { char *conffile; @@ -183,6 +193,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, virNetworkObjPtr net) { char *leasefile = NULL; + char *customleasefile = NULL; char *radvdconfigfile = NULL; char *configfile = NULL; char *radvdpidbase = NULL; @@ -201,6 +212,9 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, if (!(leasefile = networkDnsmasqLeaseFileName(def->name))) goto cleanup; + if (!(customleasefile = networkDnsmasqLeaseFileNameCustom(def->bridge))) + goto cleanup; + if (!(radvdconfigfile = networkRadvdConfigFileName(def->name))) goto cleanup; @@ -217,6 +231,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver, /* dnsmasq */ dnsmasqDelete(dctx); unlink(leasefile); + unlink(customleasefile); unlink(configfile); /* radvd */ @@ -1063,6 +1078,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + + /* This helper is used to create cutom leases file for libvirt */ + virCommandAddArgFormat(cmd, "--dhcp-script=%s", LIBEXECDIR "/libvirt_leaseshelper"); + *cmdout = cmd; ret = 0; cleanup: diff --git a/src/util/leaseshelper.c b/src/util/leaseshelper.c new file mode 100644 index 0000000..a9aaf3f --- /dev/null +++ b/src/util/leaseshelper.c @@ -0,0 +1,271 @@ +/* + * leasehelper.c: Helper program to create custom leases file + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Nehal J Wani <nehaljw.kkd1@gmail.com> + * + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> + +#include "virutil.h" +#include "virthread.h" +#include "virfile.h" +#include "virbuffer.h" +#include "virstring.h" +#include "virerror.h" +#include "viralloc.h" +#include "virjson.h" +#include "configmake.h" + +#define VIR_FROM_THIS VIR_FROM_NETWORK + +/** + * VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX: + * + * Macro providing the upper limit on the size of leases file + */ +#define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX 2097152 + +/* + * Use this when passing possibly-NULL strings to printf-a-likes. + */ +# define EMPTY_STR(s) ((s) ? (s) : "*") + +int +main(int argc, char **argv) { + + /* Doesn't hurt to check */ + if (argc < 4) { + /* Refer man page of dnsmasq --dhcp-script for more details */ + fprintf(stderr, "Usage: $program $action ${mac|clientid} $ip\n"); + return -1; + } + + char *lease_file = NULL; + char *lease_entries = NULL; + const char *ip = argv[3]; + const char *mac = argv[2]; + const char *action = argv[1]; + const char *program_name = argv[0]; + const char *iaid = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_IAID")); + const char *clientid = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_CLIENT_ID")); + const char *interface = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_INTERFACE")); + const char *exptime = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES")); + const char *hostname = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_SUPPLIED_HOSTNAME")); + const char *leases_str = NULL; + size_t i = 0; + int rv = -1; + int size = 0; + int lease_file_len = 0; + FILE *fp = NULL; + bool add = false; + bool delete = false; + virJSONValuePtr lease_new; + virJSONValuePtr lease_tmp; + virJSONValuePtr leases_array; + virJSONValuePtr lease_new_tmp; + virJSONValuePtr leases_array_new; + + if (setlocale(LC_ALL, "") == NULL || + bindtextdomain(PACKAGE, LOCALEDIR) == NULL || + textdomain(PACKAGE) == NULL) { + fprintf(stderr, _("%s: initialization failed\n"), program_name); + exit(EXIT_FAILURE); + } + + if (virThreadInitialize() < 0 || + virErrorInitialize() < 0) { + fprintf(stderr, _("%s: initialization failed\n"), program_name); + exit(EXIT_FAILURE); + } + + if (virAsprintf(&lease_file, "%s/%s.status", LOCALSTATEDIR + "/lib/libvirt/dnsmasq/", interface) < 0) + goto cleanup; + + if (virGetEnvAllowSUID("DNSMASQ_IAID")) { + mac = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_MAC")); + clientid = argv[2]; + } + + /* Make sure dnsmasq knows the interface, otherwise something is wrong */ + if (STREQ(interface, "*")) + goto cleanup; + + /* Make sure the file exists. If not, 'touch' it */ + if (virFileTouch(lease_file, 0644) < 0) + goto cleanup; + + /* Read entire contents */ + if ((lease_file_len = virFileReadAll(lease_file, + VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, + &lease_entries)) < 0) { + goto cleanup; + } + + if (STREQ(action, "add") || STREQ(action, "old") || STREQ(action, "del")) { + if (mac || STREQ(action, "del")) { + /* Delete the corresponding lease */ + delete = true; + if (STREQ(action, "add") || STREQ(action, "old")) { + add = true; + /* Enter new lease */ + if (!(lease_new = virJSONValueNewObject())) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + if (virJSONValueObjectAppendString(lease_new, "expiry-time", + exptime) < 0 || + virJSONValueObjectAppendString(lease_new, "mac-address", + mac) < 0 || + virJSONValueObjectAppendString(lease_new, "iaid", + iaid) < 0 || + virJSONValueObjectAppendString(lease_new, "ip-address", + ip) < 0 || + virJSONValueObjectAppendString(lease_new, "hostname", + hostname) < 0 || + virJSONValueObjectAppendString(lease_new, "client-id", + clientid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + } + } + } + + /* Check for previous leases */ + if (lease_file_len) { + if (!(leases_array = virJSONValueFromString(lease_entries))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid json in file: %s"), lease_file); + goto cleanup; + } + + if ((size = virJSONValueArraySize(leases_array)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("couldn't fetch array of leases")); + goto cleanup; + } + } + + if (!(leases_array_new = virJSONValueNewArray())) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + + for (i = 0; i < size; i++) { + const char *ip_tmp = NULL; + const char *mac_tmp = NULL; + const char *iaid_tmp = NULL; + const char *exptime_tmp = NULL; + const char *clientid_tmp = NULL; + const char *hostname_tmp = NULL; + long long expirytime_tmp = 0; + + if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse json")); + goto cleanup; + } + + if (!(iaid_tmp = virJSONValueObjectGetString(lease_tmp, "iaid")) || + !(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) || + !(mac_tmp = virJSONValueObjectGetString(lease_tmp, "mac-address")) || + !(hostname_tmp = virJSONValueObjectGetString(lease_tmp, "hostname")) || + !(clientid_tmp = virJSONValueObjectGetString(lease_tmp, "client-id")) || + !(exptime_tmp = virJSONValueObjectGetString(lease_tmp, "expiry-time"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse json")); + goto cleanup; + } + + if (virStrToLong_ll(exptime_tmp, NULL, 10, &expirytime_tmp) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to convert lease expiry time to integer: %s"), + exptime_tmp); + goto cleanup; + } + + /* Check whether lease has expired or not */ + if (expirytime_tmp < (long long) time(NULL)) + continue; + else if (delete && STREQ(ip_tmp, ip)) + continue; + else { + if (!(lease_new_tmp = virJSONValueNewObject())) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + if (virJSONValueObjectAppendString(lease_new_tmp, "expiry-time", + exptime_tmp) < 0 || + virJSONValueObjectAppendString(lease_new_tmp, "mac-address", + mac_tmp) < 0 || + virJSONValueObjectAppendString(lease_new_tmp, "iaid", + iaid_tmp) < 0 || + virJSONValueObjectAppendString(lease_new_tmp, "ip-address", + ip_tmp) < 0 || + virJSONValueObjectAppendString(lease_new_tmp, "hostname", + hostname_tmp) < 0 || + virJSONValueObjectAppendString(lease_new_tmp, "client-id", + clientid_tmp) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + if (virJSONValueArrayAppend(leases_array_new, lease_new_tmp) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + } + } + + if (add) { + if (virJSONValueArrayAppend(leases_array_new, lease_new) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + } + + rv = 0; + + /* Write to file */ + leases_str = virJSONValueToString(leases_array_new, true); + if (!leases_str) + leases_str = ""; + + if (virFileWriteStr(lease_file, leases_str, 0) < 0) + rv = -1; + +cleanup: + VIR_FREE(lease_file); + virJSONValueFree(lease_new); + virJSONValueFree(leases_array); + virJSONValueFree(lease_new_tmp); + virJSONValueFree(leases_array_new); + return rv; +} -- 1.8.1.4
On Mon, Jan 27, 2014 at 2:17 PM, Nehal J Wani <nehaljw.kkd1@gmail.com> wrote:
Introduce helper program to catch events from dnsmasq and maintain a custom lease file per network. It supports dhcpv4 and dhcpv6. The file is saved as "<interface-name>.status".
Each lease contains the following info: <expiry-time (epoch time)> <mac> <iaid> <ip-address> <hostname> <clientid>
Example of custom leases file content: [ { "expiry-time": "1390775837", "mac-address": "52:54:00:93:8c:63", "iaid": "*", "ip-address": "192.168.150.209", "hostname": "iit-ad885e4aa1", "client-id": "01:52:54:00:44:7c:d7" }, { "expiry-time": "1390775950", "mac-address": "52:54:00:7b:6f:ba", "iaid": "8089530", "ip-address": "2001:db8:ca2:2:1::6d", "hostname": "*", "client-id": "00:04:76:00:cf:ae:b3:0b:fc:cd:0e:22:2e:97:76:65:74:ec" } ]
src/Makefile.am: * Add options to compile the helper program
src/network/bridge_driver.c: * Introduce networkDnsmasqLeaseFileNameCustom() * Invoke helper program along with dnsmasq * Delete the .status file when corresponding n/w is destroyed.
src/util/leaseshelper.c * Helper program to create the custom lease file
--- v2: * Changed format to JSON
v1: * Refer: https://www.redhat.com/archives/libvir-list/2014-January/msg00626.html
src/Makefile.am | 20 ++++ src/network/bridge_driver.c | 19 ++++ src/util/leaseshelper.c | 271 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 310 insertions(+) create mode 100644 src/util/leaseshelper.c
Ping! -- Nehal J Wani
On 01/27/2014 01:47 AM, Nehal J Wani wrote:
Introduce helper program to catch events from dnsmasq and maintain a custom lease file per network. It supports dhcpv4 and dhcpv6. The file is saved as "<interface-name>.status".
Each lease contains the following info: <expiry-time (epoch time)> <mac> <iaid> <ip-address> <hostname> <clientid>
Example of custom leases file content: [ { "expiry-time": "1390775837",
In JSON, this is probably better represented as an int rather than string.
"mac-address": "52:54:00:93:8c:63", "iaid": "*",
JSON is pretty flexible as a key/value lookup; omitting a key rather than setting it to a sentinel value of '*' actually makes the parser easier (it's easier to ask "is the key present" than it is "lookup the key, now strcmp with "*" to see if the lease file didn't have anything to tell us).
"ip-address": "192.168.150.209", "hostname": "iit-ad885e4aa1", "client-id": "01:52:54:00:44:7c:d7" },
Everything else as string makes sense.
@@ -2410,6 +2413,23 @@ libvirt_iohelper_CFLAGS = \ $(NULL) endif WITH_LIBVIRTD
+if WITH_LIBVIRTD
We can drop the endif/if on the same condition, and just make the existing block longer. Then again, I'm trying to figure out when the leases helper should be build. Is it always built for exactly the same conditions that libvirtd is built? I think we are pretty safe to say that only libvirtd will be registering this helper binary with dnsmasq. On the other hand, I think it is possible to build libvirtd without qemu support, or more generically, in a setup where we will not be using dnsmasq. Thinking aloud, maybe the real condition should be: if WITH_LIBVIRTD existing iohelper stuff if WITH_DNSMASQ leasehelper stuff end WITH_DNSMASQ end WITH_LIBVIRTD which also would require a configure.ac setting of an automake conditional that says whether we are compiling with dnsmasq support. Looking at existing configure.ac, we merely check for a path to dnsmasq, and assume that we either learn the absolute path to use at configure time, or that it will be found on PATH at runtime, without needing to configure it, so on that front, your use of WITH_LIBVIRTD may be sufficient.
+libexec_PROGRAMS += libvirt_leaseshelper +libvirt_leaseshelper_SOURCES = $(UTIL_LEASES_HELPER_SOURCES) +libvirt_leaseshelper_LDFLAGS = \ + $(NULL)
It's okay to omit this if we don't have anything to list.
+++ b/src/network/bridge_driver.c
@@ -1063,6 +1078,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + + /* This helper is used to create cutom leases file for libvirt */
s/cutom/custom/
+ virCommandAddArgFormat(cmd, "--dhcp-script=%s", LIBEXECDIR "/libvirt_leaseshelper"); + *cmdout = cmd;
So this hooks up the helper, but nothing is parsing the result yet? That's still okay for an incremental patch, but it would be nice to also see the consumer code.
+++ b/src/util/leaseshelper.c @@ -0,0 +1,271 @@ +/* + * leasehelper.c: Helper program to create custom leases file + * + * Copyright (C) 2013 Red Hat, Inc.
It's now 2014, and while attributing Red Hat copyright for the portions of this file that you copied from elsewhere, you're also entitled to claim copyright for your new file if you'd like.
+ +/** + * VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX: + * + * Macro providing the upper limit on the size of leases file + */ +#define VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX 2097152
I like spelling this (2 * 1024 * 1024), as that's a bit more obvious that it is 2M.
+ +/* + * Use this when passing possibly-NULL strings to printf-a-likes. + */ +# define EMPTY_STR(s) ((s) ? (s) : "*")
Why not reuse our existing NULLSTR macro?
+ +int +main(int argc, char **argv) {
Style: leading { of a function body on its own line.
+ + /* Doesn't hurt to check */ + if (argc < 4) {
Why not also reject argc > 4? I am also a strong believer that EVERY app should support --help and --version (even if only to say that the app is useless on its own). iohelper.c at least does: if (argc > 1 && STREQ(argv[1], "--help")) usage(EXIT_SUCCESS); but doesn't support --version, so you can argue that libvirt isn't consistent on always doing this, but it's still nicer to not propagate the bad examples.
+ /* Refer man page of dnsmasq --dhcp-script for more details */ + fprintf(stderr, "Usage: $program $action ${mac|clientid} $ip\n"); + return -1;
-1 gets truncated to a return value of 255, but that is generally a bad idea. I'd much rather see 'return EXIT_FAILURE' or 'exit(EXIT_FAILURE)'. Also, your error message is not translated. I guess we can argue that since the helper app is only ever useful internally, then hard-coding it to the C locale doesn't hurt. But even iohelper.c took care to call setlocale()/bindtextdomain()/textdomain() before printing any error messages, which were marked for translation.
+ const char *program_name = argv[0];
Although we require a C99 compiler, and therefore allow declarations after statements, we tend to avoid them; you should swap this block to occur before your check for proper argc.
+ const char *iaid = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_IAID")); + const char *clientid = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_CLIENT_ID")); + const char *interface = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_INTERFACE")); + const char *exptime = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES")); + const char *hostname = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_SUPPLIED_HOSTNAME"));
By storing "*" in iaid now, you've lost whether it was not provided in the environment. I'd rather see the output modified to omit keys that were not provided, but for that to work, you want to leave iaid NULL if it was not found in the environment.
+ virJSONValuePtr lease_new; + virJSONValuePtr lease_tmp;
Uninitialized...
+ if (setlocale(LC_ALL, "") == NULL || + bindtextdomain(PACKAGE, LOCALEDIR) == NULL || + textdomain(PACKAGE) == NULL) { + fprintf(stderr, _("%s: initialization failed\n"), program_name); + exit(EXIT_FAILURE); + }
Ah, you DO set up translations. You'll need to sink your argc check below here, and mark that string for translation. You'll also need to separate the declaration of ip from the actual assigning it from argv[3], so that you aren't dereferencing beyond array bounds.
+ + if (virThreadInitialize() < 0 || + virErrorInitialize() < 0) { + fprintf(stderr, _("%s: initialization failed\n"), program_name); + exit(EXIT_FAILURE); + } + + if (virAsprintf(&lease_file, "%s/%s.status", LOCALSTATEDIR + "/lib/libvirt/dnsmasq/", interface) < 0) + goto cleanup;
...and on error, the cleanup label unconditionally frees lease_new and friends. That's a crash waiting to happen (the chance of being out of memory this early after startup is remote, but it will probably trigger Coverity warnings if you don't fix it).
+ + if (virGetEnvAllowSUID("DNSMASQ_IAID")) { + mac = EMPTY_STR(virGetEnvAllowSUID("DNSMASQ_MAC")); + clientid = argv[2]; + } + + /* Make sure dnsmasq knows the interface, otherwise something is wrong */ + if (STREQ(interface, "*")) + goto cleanup;
Is it worth writing a diagnostic to stderr before cleaning up? I know it is dnsmasq and not libvirt that calls this app, so I don't know whether error messages printed here will be propagated back to libvirt, but it's probably worth trying to be verbose about any error exit. For that matter, you may want to copy what tools/virt-login-shell.c does: virSetErrorFunc(NULL, NULL); virSetErrorLogPriorityFunc(NULL); that way, errors printed by libvirt functions such as virAsprintf are SURE to go to stderr.
+ + /* Make sure the file exists. If not, 'touch' it */ + if (virFileTouch(lease_file, 0644) < 0) + goto cleanup;
This feels wrong. The file should already be created by libvirtd before it spawned dnsmasq.
+ + /* Read entire contents */ + if ((lease_file_len = virFileReadAll(lease_file, + VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, + &lease_entries)) < 0) { + goto cleanup; + } + + if (STREQ(action, "add") || STREQ(action, "old") || STREQ(action, "del")) { + if (mac || STREQ(action, "del")) { + /* Delete the corresponding lease */ + delete = true; + if (STREQ(action, "add") || STREQ(action, "old")) { + add = true; + /* Enter new lease */ + if (!(lease_new = virJSONValueNewObject())) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + if (virJSONValueObjectAppendString(lease_new, "expiry-time", + exptime) < 0 || + virJSONValueObjectAppendString(lease_new, "mac-address", + mac) < 0 || + virJSONValueObjectAppendString(lease_new, "iaid", + iaid) < 0 || + virJSONValueObjectAppendString(lease_new, "ip-address", + ip) < 0 || + virJSONValueObjectAppendString(lease_new, "hostname", + hostname) < 0 || + virJSONValueObjectAppendString(lease_new, "client-id", + clientid) < 0) {
Again, I'd rather omit keys for values that were not provided by dnsmasq.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to create json")); + goto cleanup; + } + } + } + }
Shouldn't you error out if 'action' was not one of the three strings you know how to handle? That way, if future dnsmasq adds capabilities, we get logging that we didn't know what to do with it.
+ + /* Check whether lease has expired or not */ + if (expirytime_tmp < (long long) time(NULL)) + continue; + else if (delete && STREQ(ip_tmp, ip))
The else is redundant, because the if called continue.
+ continue; + else {
Again. And dropping the else and the indentation will fix your style issue (if either side of if/else used {}, then both sides should use it).
+ if (virJSONValueObjectAppendString(lease_new_tmp, "expiry-time", + exptime_tmp) < 0 ||
You should append the integer value of expirytime_tmp rather than a string.
+ rv = 0;
Changing your return value to success prior to writing is fishy. I'd defer this to the very end, when you know you succeeded. Also, EXIT_SUCCESS is nicer than hard-coded 0.
+ /* Write to file */ + leases_str = virJSONValueToString(leases_array_new, true); + if (!leases_str) + leases_str = "";
Writing an empty string when we encountered an error feels fishy. We should instead report failure if leases_str is NULL.
+ + if (virFileWriteStr(lease_file, leases_str, 0) < 0)
Hmmm. This is not atomic. If your program gets killed mid-write, it will leave the half-written new stuff at the front and the old data at the back. You want to use virFileRewrite() instead, because that interface guarantees via rename() that the official file name will be either the old contents or the new contents.
+ rv = -1;
Again, -1 is not a good return value. EXIT_FAILURE is better.
+ +cleanup: + VIR_FREE(lease_file); + virJSONValueFree(lease_new); + virJSONValueFree(leases_array); + virJSONValueFree(lease_new_tmp); + virJSONValueFree(leases_array_new); + return rv; +}
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Nehal J Wani