[libvirt] [libvirt PATCHv5 2/2] add leasefile support
Stefan Berger
stefanb at linux.vnet.ibm.com
Wed Nov 16 13:41:46 UTC 2011
On 11/10/2011 03:28 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 | 270 +++++++++++++++++++++++++++++++++----
> 1 files changed, 243 insertions(+), 27 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> index 8a37a6f..918ad7b 100644
> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -55,10 +55,18 @@
> #include "nwfilter_gentech_driver.h"
> #include "nwfilter_ebiptables_driver.h"
> #include "nwfilter_dhcpsnoop.h"
> +#include "virfile.h"
> +#include "configmake.h"
>
> #define VIR_FROM_THIS VIR_FROM_NWFILTER
>
> #ifdef HAVE_LIBPCAP
> +
> +#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;
> @@ -76,6 +84,7 @@ struct virNWFilterSnoopReq {
> const char *filtername;
> virNWFilterHashTablePtr vars;
> virNWFilterDriverStatePtr driver;
> + bool running;
> /* start and end of lease list, ordered by lease time */
> struct iplease *start;
> struct iplease *end;
> @@ -96,7 +105,15 @@ 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 virNWFilterSnoopReq *newreq(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_restore(struct virNWFilterSnoopReq *req);
> +static void lease_refresh(void);
>
> /*
> * ipl_ladd - add an IP lease to a list
> @@ -187,7 +204,7 @@ ipl_install(struct iplease *ipl)
> * ipl_add - create or update an IP lease
> */
> static void
> -ipl_add(struct iplease *plnew)
> +ipl_add(struct iplease *plnew, bool update_leasefile)
> {
> struct iplease *pl;
> struct virNWFilterSnoopReq *req = plnew->ipl_req;
> @@ -195,6 +212,8 @@ ipl_add(struct iplease *plnew)
> pl = ipl_getbyip(req->start, plnew->ipl_ipaddr);
> if (pl) {
> ipl_update(pl, plnew->ipl_timeout);
> + if (update_leasefile)
> + lease_save(pl);
> return;
> }
> /* support for multiple addresses requires the ability to add filters
> @@ -212,11 +231,14 @@ ipl_add(struct iplease *plnew)
> }
> *pl = *plnew;
>
> - if (ipl_install(pl)< 0) {
> + if (req->running&& ipl_install(pl)< 0) {
> VIR_FREE(pl);
> return;
> }
> ipl_tadd(pl);
> + nleases++;
> + if (update_leasefile)
> + lease_save(pl);
> }
>
> /*
> @@ -252,7 +274,7 @@ ipl_tdel(struct iplease *ipl)
> * ipl_del - delete an IP lease
> */
> static void
> -ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr)
> +ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr, bool update_leasefile)
> {
> struct iplease *ipl;
>
> @@ -262,13 +284,18 @@ ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr)
>
> ipl_tdel(ipl);
>
> - /* for multiple address support, this needs to remove those rules
> - * referencing "IP" with ipl's ip value.
> - */
> - if (req->techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, NULL)) {
> - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "ipl_ldel failed");
> + if (update_leasefile) {
> + lease_save(ipl);
> +
> + /*
> + * for multiple address support, this needs to remove those rules
> + * referencing "IP" with ipl's ip value.
> + */
> + if (req->techdriver->applyDHCPOnlyRules(req->ifname,req->macaddr,NULL))
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "ipl_ldel failed");
> }
> VIR_FREE(ipl);
> + nleases--;
> }
>
> /*
> @@ -308,7 +335,7 @@ ipl_trun(struct virNWFilterSnoopReq *req)
>
> now = time(0);
> while (req->start&& req->start->ipl_timeout<= now)
> - ipl_del(req, req->start->ipl_ipaddr);
> + ipl_del(req, req->start->ipl_ipaddr, 1);
> return 0;
> }
>
> @@ -467,11 +494,11 @@ dhcpdecode(struct virNWFilterSnoopReq *req, struct eth *pep, int len)
>
> switch (mtype) {
> case DHCPACK:
> - ipl_add(&ipl);
> + ipl_add(&ipl, 1);
> break;
> case DHCPDECLINE:
> case DHCPRELEASE:
> - ipl_del(req, ipl.ipl_ipaddr);
> + ipl_del(req, ipl.ipl_ipaddr, 1);
> break;
> default:
> break;
> @@ -521,7 +548,7 @@ snoopReqFree(struct virNWFilterSnoopReq *req)
> /* free all leases */
> snoop_lock();
> for (ipl = req->start; ipl; ipl = req->start)
> - ipl_del(req, ipl->ipl_ipaddr);
> + ipl_del(req, ipl->ipl_ipaddr, 0);
> snoop_unlock();
>
> /* free all req data */
> @@ -547,6 +574,8 @@ virNWFilterDHCPSnoop(void *req0)
>
> ifindex = if_nametoindex(req->ifname);
>
> + lease_restore(req);
> +
> while (1) {
> if (req->die)
> break;
> @@ -583,23 +612,22 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
> {
> struct virNWFilterSnoopReq *req;
> pthread_t thread;
> + bool isnewreq;
>
> snoop_lock();
> req = virHashLookup(SnoopReqs, ifname);
> - snoop_unlock();
> - if (req)
> - return 0;
> -
> - if (VIR_ALLOC(req)< 0) {
> - virReportOOMError();
> - return 1;
> - }
> -
> - req->ifname = strdup(ifname);
> - if (req->ifname == NULL) {
> - snoopReqFree(req);
> - virReportOOMError();
> - return 1;
> + isnewreq = req == NULL;
> + if (!isnewreq) {
> + if (req->running) {
> + snoop_unlock();
> + return 0;
> + }
> + snoop_unlock();
> + } else {
> + snoop_unlock();
> + req = newreq(ifname);
> + if (!req)
> + return 1;
> }
>
> req->techdriver = techdriver;
> @@ -634,8 +662,10 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
> }
> req->driver = driver;
>
> + req->running = 1;
> +
> snoop_lock();
> - if (virHashAddEntry(SnoopReqs, ifname, req)) {
> + if (isnewreq&& virHashAddEntry(SnoopReqs, ifname, req)) {
> snoop_unlock();
> virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> _("virNWFilterDHCPSnoopReq req add failed on "
> @@ -669,6 +699,20 @@ freeReq(void *req0, const void *name ATTRIBUTE_UNUSED)
> req->die = 1;
> }
>
> +static void
> +lease_close(void)
> +{
> + VIR_FORCE_CLOSE(lease_fd);
> +}
> +
> +static void
> +lease_open(void)
> +{
> + lease_close();
> +
> + lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644);
> +}
> +
> int
> virNWFilterDHCPSnoopInit(void)
> {
> @@ -687,6 +731,8 @@ virNWFilterDHCPSnoopInit(void)
> virReportOOMError();
> return -1;
> }
> + lease_load();
> + lease_open();
> snoop_unlock();
> return 0;
> }
> @@ -709,10 +755,180 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
> virReportOOMError();
> return;
> }
> + lease_load();
> }
> + lease_close();
> snoop_unlock();
> }
>
> +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);
> + return 0;
> +}
> +
> +static void
> +lease_save(struct iplease *ipl)
> +{
> + struct virNWFilterSnoopReq *req = ipl->ipl_req;
> +
> + 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 struct virNWFilterSnoopReq *
> +newreq(const char *ifname)
> +{
> + struct virNWFilterSnoopReq *req;
> +
> + if (VIR_ALLOC(req)< 0) {
> + virReportOOMError();
> + return NULL;
> + }
> + req->ifname = strdup(ifname);
> +
> + return req;
> +}
> +
> +static void
> +SaveSnoopReqIter(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *data)
> +{
> + struct virNWFilterSnoopReq *req = payload;
> + int tfd = (int)data;
> + struct iplease *ipl;
> +
> + for (ipl = req->start; ipl; ipl = ipl->ipl_next)
> + (void) lease_write(tfd, req->ifname, ipl);
> +}
> +
> +static void
> +lease_refresh(void)
> +{
> + 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;
> + }
> + if (SnoopReqs)
> + virHashForEach(SnoopReqs, SaveSnoopReqIter, (void *)tfd);
To avoid the warnings pass '&tfd' and adjust the iterator accordingly.
nwfilter/nwfilter_dhcpsnoop.c: In function 'SaveSnoopReqIter':
nwfilter/nwfilter_dhcpsnoop.c:826:15: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
nwfilter/nwfilter_dhcpsnoop.c: In function 'lease_refresh':
nwfilter/nwfilter_dhcpsnoop.c:848:53: warning: cast to pointer from
integer of different size [-Wint-to-pointer-cast]
> + (void) close(tfd);
Use VIR_FORCE_CLOSE().
> + 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
> +lease_load(void)
> +{
> + char line[256], ifname[16], ipstr[16], srvstr[16];
> + struct iplease ipl;
> + struct virNWFilterSnoopReq *req;
> + time_t now;
> + FILE *fp;
> + int ln = 0;
> +
> + fp = fopen(LEASEFILE, "r");
> + 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, "%u %16s %16s %16s",&ipl.ipl_timeout,
> + ifname, ipstr, srvstr)< 4) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("lease_load lease file line %d corrupt"),
> + ln);
> + break;;
> + }
> + if (ipl.ipl_timeout&& ipl.ipl_timeout< now)
> + continue;
> + req = virHashLookup(SnoopReqs, ifname);
> + if (!req) {
> + req = newreq(ifname);
> + if (!req)
> + break;
> + if (virHashAddEntry(SnoopReqs, ifname, req)) {
> + snoopReqFree(req);
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("lease_load req add failed on "
> + "interface \"%s\""), ifname);
> + continue;
> + }
> + }
> +
> + if (inet_pton(AF_INET, ipstr,&ipl.ipl_ipaddr)<= 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("line %d corrupt ipaddr \"%s\""),
> + ln, ipstr);
> + continue;
> + }
> + (void) inet_pton(AF_INET, srvstr,&ipl.ipl_server);
> + ipl.ipl_req = req;
> +
> + if (ipl.ipl_timeout)
> + ipl_add(&ipl, 0);
> + else
> + ipl_del(req, ipl.ipl_ipaddr, 0);
> + }
> + if (fp != NULL)
> + (void) fclose(fp);
Use VIR_FORCE_FCLOSE(). No checking for fp != NULL necessary then.
> + lease_refresh();
> +}
> +
> +static void
> +lease_restore(struct virNWFilterSnoopReq *req)
> +{
> + struct iplease *ipl;
> +
> + for (ipl=req->start; ipl; ipl=ipl->ipl_next)
> + (void) ipl_install(ipl);
> +}
> +
> #else /* HAVE_LIBPCAP */
> int
> virNWFilterDHCPSnoopInit(void)
The code looks good now, but please fix the nits. I'll test it more
later today.
When I start a VM that was previously running and thus still has a lease
and restart libvirt while it is booting, libvirtd seems to automatically
rebuilds the filters to the previous leased IP address. Can we
invalidate the previous leases once new detection starts? Also, I would
rather NOT use the name of the interface as a criterion to disable
previous leases but the MAC address of the interface since the name of
the interface can change (and another VM can now have 'vnet0'). I think
the MAC address that an IP address is associated with should go into the
lease file rather than the interface name. Would it be possible to
change it in that direction?
Stefan
More information about the libvir-list
mailing list