[libvirt] [PATCH v4] Add helper program to create custom leases

[Eric Blake]

On 03/17/2014 03:30 PM, 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".
> 


> @@ -243,6 +253,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
>                        virNetworkObjPtr net)
>  {
>      char *leasefile = NULL;
> +    char *customleasefile = NULL;
>      char *radvdconfigfile = NULL;
>      char *configfile = NULL;
>      char *radvdpidbase = NULL;
> @@ -261,6 +272,9 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
>      if (!(leasefile = networkDnsmasqLeaseFileName(def->name)))
>          goto cleanup;
>  
> +    if (!(customleasefile = networkDnsmasqLeaseFileNameCustom(def->bridge)))
> +        goto cleanup;
> +

Memory leak - customleasefile is malloc'd memory, but the cleanup label
never frees it.

> @@ -1120,6 +1135,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
>  
>      cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
>      virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
> +
> +    /* This helper is used to create custom leases file for libvirt */
> +    virCommandAddArgFormat(cmd, "--dhcp-script=%s", LIBEXECDIR "/libvirt_leaseshelper");

This is a bit hard-coded, and won't play nicely with ./run.  Ideally, we
should be constructing the name so that if argv[0] is an uninstalled
in-tree binary, then we convert to a name relative to the build tree
instead of LIBEXECDIR; that way, when using ./run, we test the
just-built libvirt_leaseshelper instead of a pre-installed version.

> +++ b/src/network/leaseshelper.c
> @@ -0,0 +1,328 @@
> +/*
> + * leasehelper.c: Helper program to create custom leases file
> + *
> + * Copyright (C) 2014 Red Hat, Inc.

You're allowed to claim copyright if you'd like; you don't have to
assign it all to Red Hat.


> +
> +ATTRIBUTE_NORETURN static void
> +usage(int status)
> +{
> +    if (status) {
> +        fprintf(stderr, _("%s: try --help for more details\n"), program_name);
> +    } else {
> +        printf(_("Usage: %s ACTION MAC|CLIENTID IP HOSTNAME\n"
> +                 "   or: %s ACTION MAC|CLIENTID IP\n"),

Could be compressed to one line as "%s ACTION MAC|CLIENTID IP
[HOSTNAME]".  Maybe worth listing the set of valid ACTION verbs.


> +    size_t i = 0;
> +    int size = 0;

Usually something named 'size' is of type 'size_t' - but I guess I'll
have to see how you use it.

> +
> +    if (argc != 4 && argc != 5) {
> +        /* Refer man page of dnsmasq --dhcp-script for more details */

This comment might also be useful as actual output of the --help option.

> +    /* Check if it is an IPv6 lease */
> +    if (virGetEnvAllowSUID("DNSMASQ_IAID")) {
> +        mac = virGetEnvAllowSUID("DNSMASQ_MAC");
> +        clientid=argv[2];

Space around =

> +    }
> +
> +    if (virAsprintf(&custom_lease_file, "%s/%s.status", LOCALSTATEDIR
> +                    "/lib/libvirt/dnsmasq/", interface) < 0)

Unusual line break; I would have done:

if (virAsprintf(&custom_lease_file, "%s/%s.status",
                LOCALSTATEDIR "/lib/libvirt/dnsmasq/",
                interface) < 0)

or even:

if (virAsprintf(&custom_lease_file,
                LOCALSTATEDIR "/lib/libvirt/dnsmasq/%s.status",
                interface) < 0)

> +
> +    if (STREQ(action, "add") || STREQ(action, "old") || STREQ(action, "del")) {
> +        if (mac || STREQ(action, "del")) {

This is a lot of repeated STREQ.  It might be nicer if you use
VIR_ENUM_IMPL earlier on, and convert the incoming string to an enum
value; then the rest of your code will use integer comparisons instead
of repeat string comparisons.


> +
> +                if ((iaid && virJSONValueObjectAppendString(lease_new, "iaid",
> +                                                            iaid) < 0) ||
> +                    (ip && virJSONValueObjectAppendString(lease_new, "ip-address",
> +                                                          ip) < 0) ||

This works, but it's harder to debug (gdb is rather picky about how it
puts breakpoints across chains of && and || when optimization gets
involved).  It may be a bit nicer to have multiple if statements:

if (iaid && virJSON...() < 0)
    goto cleanup;
if (ip && virJSON...() < 0)
    goto cleanup;
...

> +
> +        if ((size = virJSONValueArraySize(leases_array)) < 0) {

Ah, so size really is an 'int'.  You were right here.


> +    for (i = 0; i < size; i++) {

> +        /* Check whether lease has expired or not */
> +        if (exptime_tmp < (long long) time(NULL))

This calls time() in a loop, which can get a bit expensive on some
hardware, as well as a bit unpredictable (the time changes over the
course of the loop).  Better might be to call time() outside the loop,
then compare against that value inside the loop.

Looks like you are converging on something worth applying.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140320/a22728c9/attachment-0001.sig>