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