[libvirt] [libvirt PATCHv3 09/10] add leasefile support
Stefan Berger
stefanb at linux.vnet.ibm.com
Mon Oct 17 16:53:58 UTC 2011
On 10/12/2011 03:50 PM, David L Stevens wrote:
> This patch adds support for saving DHCP snooping leases to an on-disk
> file and restoring saved leases that are still active on restart.
>
> Signed-off-by: David L Stevens<dlstevens at us.ibm.com>
> ---
> src/nwfilter/nwfilter_dhcpsnoop.c | 370 +++++++++++++++++++++++++++++++++++--
> 1 files changed, 353 insertions(+), 17 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> index f784a29..eedf550 100644
> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -56,10 +56,21 @@
> #include "nwfilter_gentech_driver.h"
> #include "nwfilter_ebiptables_driver.h"
> #include "nwfilter_dhcpsnoop.h"
> +#include "configmake.h"
>
> #define VIR_FROM_THIS VIR_FROM_NWFILTER
>
> +#define LEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.leases"
> +#define TMPLEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.ltmp"
> +static int lease_fd = -1;
> +static int nleases = 0; /* number of active leases */
> +static int wleases = 0; /* number of written leases */
> +
> static virHashTablePtr SnoopReqs;
> +static pthread_mutex_t SnoopLock;
> +
> +#define snoop_lock() { pthread_mutex_lock(&SnoopLock); }
> +#define snoop_unlock() { pthread_mutex_unlock(&SnoopLock); }
>
> struct virNWFilterSnoopReq {
> virConnectPtr conn;
> @@ -90,7 +101,14 @@ struct iplease {
>
> static struct iplease *ipl_getbyip(struct iplease *start, uint32_t ipaddr);
> static void ipl_update(struct iplease *pl, uint32_t timeout);
> -
> +
> +static struct iflease *getiflease(const char *ifname);
> +static void lease_open(void);
> +static void lease_close(void);
> +static void lease_load(void);
> +static void lease_save(struct iplease *ipl);
> +static void lease_refresh(void);
> +static void lease_restore(struct virNWFilterSnoopReq *req);
>
> /*
> * ipl_ladd - add an IP lease to a list
> @@ -150,6 +168,7 @@ ipl_add(struct iplease *plnew)
> ipl_update(pl, plnew->ipl_timeout);
> return;
> }
> + nleases++;
> if (VIR_ALLOC(pl)< 0) {
> virReportOOMError();
> return;
> @@ -184,6 +203,7 @@ ipl_add(struct iplease *plnew)
> return;
> }
> ipl_tadd(pl);
> + lease_save(pl);
> }
>
> /*
> @@ -231,6 +251,7 @@ ipl_del(struct iplease *ipl)
> req = ipl->ipl_req;
>
> ipl_tdel(ipl);
> + lease_save(ipl);
>
> if (inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) {
> ipstr = strdup(ipbuf);
> @@ -248,6 +269,7 @@ ipl_del(struct iplease *ipl)
> _("ipl_del inet_ntop " "failed (0x%08X)"),
> ipl->ipl_ipaddr);
> VIR_FREE(ipl);
> + nleases--;
> }
>
> /*
> @@ -259,6 +281,7 @@ ipl_update(struct iplease *ipl, uint32_t timeout)
> ipl_tdel(ipl);
> ipl->ipl_timeout = timeout;
> ipl_tadd(ipl);
> + lease_save(ipl);
> return;
> }
>
> @@ -275,8 +298,6 @@ ipl_getbyip(struct iplease *start, uint32_t ipaddr)
> return pl;
> }
>
> -#define GRACE 5
> -
> /*
> * ipl_trun - run the IP lease timeout list
> */
> @@ -465,6 +486,19 @@ dhcpopen(const char *intf)
> return handle;
> }
>
> +/*
> + * snoopReqFree - release a snoop Req
> + */
> +static void
> +snoopReqFree(struct virNWFilterSnoopReq *req)
> +{
> + if (req->ifname)
> + VIR_FREE(req->ifname);
> + if (req->vars)
> + virNWFilterHashTableFree(req->vars);
> + VIR_FREE(req);
> +}
> +
> static void *
> virNWFilterDHCPSnoop(void *req0)
> {
> @@ -479,12 +513,19 @@ virNWFilterDHCPSnoop(void *req0)
> if (!handle)
> return 0;
>
> + /* restore any saved leases for this interface */
> + snoop_lock();
> + lease_restore(req);
> + snoop_unlock();
> +
> ifindex = if_nametoindex(req->ifname);
>
> while (1) {
> if (req->die)
> break;
> + snoop_lock();
> ipl_trun(req);
> + snoop_unlock();
>
> packet = (struct eth *) pcap_next(handle,&hdr);
>
> @@ -494,16 +535,18 @@ virNWFilterDHCPSnoop(void *req0)
> continue;
> }
>
> + snoop_lock();
> dhcpdecode(req, packet, hdr.caplen);
> + snoop_unlock();
> }
> +
> + snoop_lock();
> /* free all leases */
> for (ipl = req->start; ipl; ipl = req->start)
> ipl_del(ipl);
> + snoop_unlock();
>
> - /* free all req data */
> - VIR_FREE(req->ifname);
> - virNWFilterHashTableFree(req->vars);
> - VIR_FREE(req);
> + snoopReqFree(req);
> return 0;
> }
>
> @@ -518,9 +561,12 @@ virNWFilterDHCPSnoopReq(virConnectPtr conn,
> {
> struct virNWFilterSnoopReq *req;
>
> + snoop_lock();
> req = virHashLookup(SnoopReqs, ifname);
> - if (req)
> + snoop_unlock();
> + if (req) {
> return 0;
> + }
> if (VIR_ALLOC(req)< 0) {
> virReportOOMError();
> return 1;
> @@ -533,28 +579,30 @@ virNWFilterDHCPSnoopReq(virConnectPtr conn,
> req->ifname = strdup(ifname);
> req->vars = virNWFilterHashTableCreate(0);
> if (!req->vars) {
> + snoopReqFree(req);
Following the lookup into the hashtable above you cannot just free the
request. I suppose you'd first have to remove it from the hash table --
or did this maybe happen in the lines in between? This seems more like
fix to the previous patch? The snoopReqFree() should really be
introduced in the previous patch...
> virReportOOMError();
> return 1;
> }
> if (virNWFilterHashTablePutAll(vars, req->vars)) {
> virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> - _("virNWFilterDHCPSnoopReq: can't copy variables"
> - " on if %s"), ifname);
> + _("virNWFilterDHCPSnoopReq: can't copy "
> + "variables on if %s"), ifname);
> + snoopReqFree(req);
> return 1;
> }
> req->driver = driver;
> req->start = req->end = 0;
>
> if (virHashAddEntry(SnoopReqs, ifname, req)) {
> - VIR_FREE(req);
> + snoopReqFree(req);
> virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> - _("virNWFilterDHCPSnoopReq req add failed on"
> - "interface \"%s\""), ifname);
> + _("virNWFilterDHCPSnoopReq req add "
> + "failed oninterface \"%s\""), ifname);
> return 1;
> }
> if (pthread_create(&req->thread, NULL, virNWFilterDHCPSnoop, req) != 0) {
> (void) virHashRemoveEntry(SnoopReqs, ifname);
> - VIR_FREE(req);
> + (void) snoopReqFree(req);
no need to void the result since it doesn't return anything
> virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> _("virNWFilterDHCPSnoopReq pthread_create failed"
> " oninterface \"%s\""), ifname);
> @@ -577,12 +625,41 @@ freeReq(void *req0, const void *name ATTRIBUTE_UNUSED)
> req->die = 1;
> }
>
> +static void
> +lease_close(void)
> +{
> + (void) close(lease_fd);
> + lease_fd = -1;
> +}
Replace with VIR_FORCE_CLOSE().
> +
> +static void
> +lease_open(void)
> +{
> + lease_close();
> +
> + lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644);
> +}
> +
> int
> virNWFilterDHCPSnoopInit(void)
> {
> if (SnoopReqs)
> return 0;
Since locking is done below I'd test for SnoopReq with the snoop lock
being held otherwise you could (theoretically) have two threads running
into the code below, at least the code looks that way.
> +
> + if (pthread_mutex_init(&SnoopLock, 0)< 0)
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("pthread_mutex_init: %s"),
> + strerror(errno));
> + snoop_lock();
Move this lock to the top.
> +
> + lease_load();
> +
> + lease_open();
> +
> SnoopReqs = virHashCreate(0, freeReq);
> +
> + snoop_unlock();
> +
> if (!SnoopReqs) {
> virReportOOMError();
> return -1;
> @@ -595,8 +672,267 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
> {
> if (!SnoopReqs)
> return;
> - if (ifname)
> - virHashRemoveEntry(SnoopReqs, ifname);
> - else /* free all of them */
> + snoop_lock();
> + if (!ifname) { /* free all of them */
> virHashFree(SnoopReqs);
> + lease_refresh();
> + } else
> + virHashRemoveEntry(SnoopReqs, ifname);
> + lease_close();
> + snoop_unlock();
> +}
> +
> +
> +/* lease file handling */
> +
> +struct iflease {
> + char *ifl_ifname;
> + struct iplease *ifl_start;
> + struct iplease *ifl_end;
> + struct iflease *ifl_prev;
> + struct iflease *ifl_next;
> +};
> +
> +struct iflease *leases;
> +
> +static int
> +lease_write(int lfd, const char *ifname, struct iplease *ipl)
> +{
> + char lbuf[256],ipstr[16],dhcpstr[16];
> +
> + if (inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipstr, sizeof ipstr) == 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("inet_ntop(0x%08X) failed"), ipl->ipl_ipaddr);
> + return -1;
> + }
> + if (inet_ntop(AF_INET,&ipl->ipl_server, dhcpstr, sizeof dhcpstr) == 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("inet_ntop(0x%08X) failed"), ipl->ipl_server);
> + return -1;
> + }
> + /* time intf ip dhcpserver */
> + snprintf(lbuf, sizeof(lbuf), "%u %s %s %s\n", ipl->ipl_timeout,
> + ifname, ipstr, dhcpstr);
> + if (write(lfd, lbuf, strlen(lbuf))< 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("lease file write failed: %s"),
> + strerror(errno));
> + return -1;
> + }
> + (void) fsync(lfd);
What if this call fails?
> + return 0;
> +}
> +
> +static void
> +lease_save(struct iplease *ipl)
> +{
> + struct virNWFilterSnoopReq *req = ipl->ipl_req;
> + struct iflease *ifl;
> +
> + /* add to the lease file list */
> + ifl = getiflease(ipl->ipl_req->ifname);
> + if (ifl) {
> + struct iplease *ifipl = ipl_getbyip(ifl->ifl_start, ipl->ipl_ipaddr);
> +
> + if (ifipl) {
> + if (ipl->ipl_timeout) {
> + ifipl->ipl_timeout = ipl->ipl_timeout;
> + ifipl->ipl_server = ipl->ipl_server;
> + } else {
> + ipl_ldel(ifipl,&ifl->ifl_start,&ifl->ifl_end);
> + VIR_FREE(ifipl);
> + }
> + } else if (!VIR_ALLOC(ifipl)) {
> + ifipl->ipl_ipaddr = ipl->ipl_ipaddr;
> + ifipl->ipl_server = ipl->ipl_server;
> + ifipl->ipl_timeout = ipl->ipl_timeout;
> + ipl_ladd(ifipl,&ifl->ifl_start,&ifl->ifl_end);
> + }
> + }
> + if (lease_fd< 0)
> + lease_open();
> + if (lease_write(lease_fd, req->ifname, ipl)< 0)
> + return;
> + /* keep dead leases at< ~95% of file size */
> + if (++wleases>= nleases*20)
> + lease_load(); /* load& refresh lease file */
> +}
> +
> +static void
> +lease_restore(struct virNWFilterSnoopReq *req)
> +{
> + struct iflease *ifl;
> + struct iplease *ipl;
> +
> + ifl = getiflease(req->ifname);
> + if (!ifl)
> + return;
> + for (ipl = ifl->ifl_start; ipl; ipl = ipl->ipl_next) {
> + ipl->ipl_req = req;
> + ipl_add(ipl);
> + }
> +}
> +
> +static struct iflease *
> +getiflease(const char *ifname)
> +{
> + struct iflease *ifl;
> +
> + for (ifl=leases; ifl; ifl=ifl->ifl_next)
> + if (strcmp(ifname, ifl->ifl_ifname) == 0)
> + return ifl;
> + if (VIR_ALLOC(ifl)) {
> + virReportOOMError();
> + return 0;
> + }
> + ifl->ifl_ifname = strdup(ifname);
> + ifl->ifl_next = leases;
> + leases = ifl;
> + return ifl;
> +}
> +
> +static void
> +lease_refresh(void)
> +{
> + struct iflease *ifl;
> + struct iplease *ipl;
> + int tfd;
> +
> + (void) unlink(TMPLEASEFILE);
> + /* lease file loaded, delete old one */
> + tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644);
> + if (tfd< 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("open(\"%s\"): %s"),
> + TMPLEASEFILE, strerror(errno));
> + return;
> + }
> + for (ifl=leases; ifl; ifl=ifl->ifl_next)
> + for (ipl = ifl->ifl_start; ipl; ipl = ipl->ipl_next)
> + if (lease_write(tfd, ifl->ifl_ifname, ipl)< 0)
> + break;
In case of error, shouldn't it unlink the file and return and error ?
> + (void) close(tfd);
VIR_CLOSE() and handle error case ?
> + if (rename(TMPLEASEFILE, LEASEFILE)< 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("rename(\"%s\", \"%s\"): %s"),
> + TMPLEASEFILE, LEASEFILE, strerror(errno));
> + (void) unlink(TMPLEASEFILE);
> + }
> + wleases = 0;
> + lease_open();
> +}
> +
> +static void
> +LoadSnoopReqIter(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *data ATTRIBUTE_UNUSED)
> +{
> + struct virNWFilterSnoopReq *req = payload;
> + struct iplease *ipl, *ifipl;
> + struct iflease *ifl;
> +
> + ifl = getiflease(req->ifname);
> + if (!ifl)
> + return;
> + for (ipl = req->start; ipl; ipl = ipl->ipl_next) {
> + ifipl = ipl_getbyip(ifl->ifl_start, ipl->ipl_ipaddr);
> + if (ifipl) {
> + if (ifipl->ipl_timeout< ipl->ipl_timeout) {
> + ifipl->ipl_timeout = ipl->ipl_timeout;
> + ifipl->ipl_server = ipl->ipl_server;
> + }
> + continue;
> + }
> + if (VIR_ALLOC(ifipl)) {
> + virReportOOMError();
> + continue;
> + }
> + ifipl->ipl_ipaddr = ipl->ipl_ipaddr;
> + ifipl->ipl_server = ipl->ipl_server;
> + ifipl->ipl_timeout = ipl->ipl_timeout;
> + ipl_ladd(ifipl,&ifl->ifl_start,&ifl->ifl_end);
> + }
> +}
> +
> +static void
> +lease_load(void)
> +{
> + char line[256], ifname[16], ipstr[16], srvstr[16];
> + uint32_t ipaddr, svaddr;
> + FILE *fp;
> + int ln = 0;
> + time_t timeout, now;
> + struct iflease *ifl;
> + struct iplease *ipl;
> +
> + fp = fopen(LEASEFILE, "r");
Didn't find the fclose()..
> + time(&now);
> + while (fp&& fgets(line, sizeof(line), fp)) {
> + if (line[strlen(line)-1] != '\n') {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("lease_load lease file line %d corrupt"),
> + ln);
> + break;
> + }
> + ln++;
> + if (sscanf(line, "%lu %16s %16s %16s", (unsigned long *)&timeout,
> + ifname, ipstr, srvstr)< 4) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("lease_load lease file line %d corrupt"),
> + ln);
> + break;;
> + }
> + if (timeout&& timeout< now)
> + continue;
> + ifl = getiflease(ifname);
> + if (!ifl)
> + break;
> +
> + if (inet_pton(AF_INET, ipstr,&ipaddr)<= 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("line %d corrupt ipaddr \"%s\""),
> + ln, ipstr);
> + VIR_FREE(ipl);
> + continue;
> + }
> + (void) inet_pton(AF_INET, srvstr,&svaddr);
> +
> + ipl = ipl_getbyip(ifl->ifl_start, ipaddr);
> + if (ipl) {
> + if (timeout&& timeout< ipl->ipl_timeout)
> + continue; /* out of order lease? skip. */
> + ipl->ipl_timeout = timeout;
> + ipl->ipl_server = svaddr;
> + continue;
> + }
> + if (!timeout)
> + continue; /* don't add new lease deletions */
> + if (VIR_ALLOC(ipl)) {
> + virReportOOMError();
> + break;
> + }
> + ipl->ipl_ipaddr = ipaddr;
> + ipl->ipl_server = svaddr;
> + ipl->ipl_timeout = timeout;
> + ipl_ladd(ipl,&ifl->ifl_start,&ifl->ifl_end);
> + }
> + /* also load any active leases from memory, in case lease writes may
> + * have failed.
> + */
> + if (SnoopReqs)
> + virHashForEach(SnoopReqs, LoadSnoopReqIter, 0);
> + /* remove any deleted leases */
> + for (ifl = leases; ifl; ifl = ifl->ifl_next) {
> + struct iplease *iplnext;
> +
> + for (ipl = ifl->ifl_start; ipl; ipl = iplnext) {
> + iplnext = ipl->ipl_next;
> + if (ipl->ipl_timeout == 0) {
> + ipl_ldel(ipl,&ifl->ifl_start,&ifl->ifl_end);
> + VIR_FREE(ipl);
> + }
> + }
> + }
> +
> + lease_refresh();
> }
For every lease I guess you'd have to store the VLAN ID as well since
VMs on different VLANs can have the same IP address. I am not sure what
effect this has on the whole code. Maybe your snooping would have to
look at the MAC header and extract VLAN identifier(s) [nested!] as well
and store them here. I wonder about the MAC address that an IP address
is associated and whether that should be recorded as well and used for
determining whether a lease has expired.
Stefan
More information about the libvir-list
mailing list