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 :|