[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv5 1/4] net-dhcp-leases: Implement the public APIs



On Tue, Nov 26, 2013 at 02:35:58AM +0530, Nehal J Wani wrote:
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 5aad75c..20ea40a 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> +
> +typedef struct _virNetworkDHCPLeases virNetworkDHCPLeases;
> +typedef virNetworkDHCPLeases *virNetworkDHCPLeasesPtr;
> +struct _virNetworkDHCPLeases {

s/Leases/Lease/ -  each struct only stores a single lease

> +    char *interface;            /* Network interface name */
> +    long long expirytime;       /* Seconds since epoch */
> +    int type;                   /* virIPAddrType */
> +    char *mac;                  /* MAC address */
> +    char *iaid;                 /* IAID */
> +    char *ipaddr;               /* IP address */
> +    unsigned int prefix;        /* IP address prefix */
> +    char *hostname;             /* Hostname */
> +    char *clientid;             /* Client ID or DUID */
> +};

> @@ -2019,6 +2022,7 @@ libvirt_setuid_rpc_client_la_SOURCES = 		\
>  		util/virtypedparam.c		\
>  		util/viruri.c			\
>  		util/virutil.c			\
> +		util/virmacaddr.c		\
>  		util/viruuid.c			\
>  		conf/domain_event.c		\
>  		rpc/virnetsocket.c		\

Don't add this here.

> diff --git a/src/libvirt.c b/src/libvirt.c
> index eff44eb..9a0b872 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -68,6 +68,7 @@
>  #include "virstring.h"
>  #include "virutil.h"
>  #include "virtypedparam.h"
> +#include "virmacaddr.h"
>  
>  #ifdef WITH_TEST
>  # include "test/test_driver.h"

> +int
> +virNetworkGetDHCPLeasesForMAC(virNetworkPtr network,
> +                              const char *mac,
> +                              virNetworkDHCPLeasesPtr **leases,
> +                              unsigned int flags)
> +{
> +    virConnectPtr conn;
> +    virMacAddr addr;
> +
> +    VIR_DEBUG("network=%p, mac=%s, leases=%p, flags=%x",
> +               network, mac, leases, flags);
> +
> +    virResetLastError();
> +
> +    if (leases)
> +        *leases = NULL;
> +
> +    virCheckNonNullArgGoto(mac, error);
> +
> +    if (!VIR_IS_CONNECTED_NETWORK(network)) {
> +        virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    /* Validate the MAC address */
> +    if (virMacAddrParse(mac, &addr) < 0) {
> +        virReportInvalidArg(mac, "%s",
> +                            _("Given MAC Address doesn't comply "
> +                              "with the standard (IEEE 802) format"));
> +        goto error;
> +    }

Don't do this here - it is the job of driver APIs todo semantic
validation of parameters.


> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index fe9b497..f1a9707 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -639,4 +639,11 @@ LIBVIRT_1.1.3 {
>          virConnectGetCPUModelNames;
>  } LIBVIRT_1.1.1;
>  
> +LIBVIRT_1.1.4 {
> +    global:
> +        virNetworkDHCPLeaseFree;
> +        virNetworkGetDHCPLeases;
> +        virNetworkGetDHCPLeasesForMAC;
> +} LIBVIRT_1.1.3;

Can move this into the 1.2.1 version block now.

> +/*
> + * Use this when passing possibly-NULL strings to printf-a-likes.
> + */
> +# define NULL_STR(s) ((s) ? (s) : "*")

I'd suggest calling this EMPTY_STR to avoid confusion with
the existing NULLSTR macro which prints "(null)"

> +
> +int
> +main(int argc, char **argv) {
> +
> +    /* Doesn't hurt to check */
> +    if (argc < 4)
> +        return -1;

You should print an error message to stderr along with
usage text.

> +
> +    const char *action = argv[1];
> +    const char *interface = NULL_STR(getenv("DNSMASQ_INTERFACE"));
> +    const char *expirytime = NULL_STR(getenv("DNSMASQ_LEASE_EXPIRES"));
> +    const char *mac = argv[2];
> +    const char *ip = argv[3];
> +    const char *iaid = NULL_STR(getenv("DNSMASQ_IAID"));
> +    const char *hostname = NULL_STR(getenv("DNSMASQ_SUPPLIED_HOSTNAME"));
> +    const char *clientid = NULL_STR(getenv("DNSMASQ_CLIENT_ID"));

All use of getenv is banned in libvirt code - please make sure
to run 'make syntax-check' and 'make check'. Use virGetEnvAllowSUID
here

> +    const char *leases_str = NULL;
> +    char *lease_file = NULL;
> +    char *lease_entries = NULL;
> +    char *lease_entry = NULL;
> +    char **lease_fields = NULL;
> +    bool delete = false;
> +    bool add = false;
> +    int rv = -1;
> +    int lease_file_len = 0;
> +    FILE *fp = NULL;
> +    virBuffer buf_new_lease = VIR_BUFFER_INITIALIZER;
> +    virBuffer buf_all_leases = VIR_BUFFER_INITIALIZER;

You must call the following before invoking any other libvir
APIs at all

    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 (getenv("DNSMASQ_IAID")) {
> +        mac = NULL_STR(getenv("DNSMASQ_MAC"));
> +        clientid = argv[2];
> +    }
> +
> +    /* Make sure the file exists. If not, 'touch' it */
> +    fp = fopen(lease_file, "a+");
> +    fclose(fp);

We have a virFileTouch method todo this.

> +
> +    /* 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 */
> +                virBufferAsprintf(&buf_new_lease, "%s %s %s %s %s %s\n",
> +                                  expirytime, mac, iaid, ip, hostname, clientid);
> +
> +                if (virBufferError(&buf_new_lease)) {
> +                    virBufferFreeAndReset(&buf_new_lease);
> +                    virReportOOMError();
> +                    goto cleanup;
> +                }
> +            }
> +        }
> +    }
> +
> +    lease_entry = lease_entries[0] == '\0' ? NULL : lease_entries;
> +
> +    while (lease_entry) {
> +        int nfields = 0;
> +
> +        char *eol = strchr(lease_entry, '\n');
> +        *eol = '\0';
> +
> +        /* Split the lease line */
> +        if (!(lease_fields = virStringSplit(lease_entry, " ",
> +                                            VIR_NETWORK_DHCP_LEASE_FIELDS)))
> +            goto cleanup;
> +
> +        nfields = virStringListLength(lease_fields);
> +
> +        /* Forward lease_entry to the next lease */
> +        lease_entry = strchr(lease_entry, '\0');
> +        if (lease_entry - lease_entries + 1 < lease_file_len)
> +            lease_entry++;
> +        else
> +            lease_entry = NULL;
> +
> +        if (nfields != VIR_NETWORK_DHCP_LEASE_FIELDS)
> +            goto cleanup;
> +
> +        if (delete && STREQ(lease_fields[3], ip))
> +            continue;
> +        else {
> +            virBufferAsprintf(&buf_all_leases, "%s %s %s %s %s %s\n",
> +                              lease_fields[0], lease_fields[1], lease_fields[2],
> +                              lease_fields[3], lease_fields[4], lease_fields[5]);
> +
> +            if (virBufferError(&buf_all_leases)) {
> +                virBufferFreeAndReset(&buf_all_leases);
> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +        }
> +    }
> +
> +    if (add)
> +        virBufferAsprintf(&buf_all_leases, "%s", virBufferContentAndReset(&buf_new_lease));
> +

Need virBufferError check on buf_all_leases here

> +    rv = 0;
> +
> +    /* Write to file */
> +    leases_str = virBufferContentAndReset(&buf_all_leases);
> +    if (!leases_str)
> +        leases_str = "";
> +
> +    if (virFileWriteStr(lease_file, leases_str, 0) < 0)
> +        rv = -1;
> +
> +cleanup:
> +    VIR_FREE(lease_file);
> +    VIR_FREE(lease_entries);
> +    if (lease_fields)
> +        virStringFreeList(lease_fields);
> +    return rv;
> +}


I'd suggest that the lease helper program should be added in a separate
patch from the public API, sine there's no real dependancy between
them.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]