On Thu, Dec 12, 2013 at 5:14 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
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.
The code didn't compile without making the above addition. It kept throwing the error: ../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-libvirt.o): In function `virNetworkGetDHCPLeasesForMAC': /home/Wani/Hobby/libvirt/src/libvirt.c:22187: undefined reference to `virMacAddrParse'
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.
Do you mean, I need to put this check inside networkGetDHCPLeasesForMAC in src/network/bridge_driver.c ?
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
I did run both the commands. Unfortunately, they didn't detect that I had been using getenv().
+ 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); }
Just a suggestion: It would be good if someone adds this to hacking.html
+ + 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.
Would do that in the next series. Thanks for reviewing the series. I'll continue to furnish 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 :|
-- Nehal J Wani