[libvirt] [PATCHv5 1/4] net-dhcp-leases: Implement the public APIs
Daniel P. Berrange
berrange at redhat.com
Thu Dec 12 11:44:44 UTC 2013
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 :|
More information about the libvir-list
mailing list