6:14 p.m.
On 04/25/2012 06:59 AM, Stefan Berger wrote:
This patch adds DHCP snooping support to libvirt. The learning method
for
IP addresses is specified by setting the "CTRL_IP_LEARNING" variable to one of
"any" [default] (existing IP learning code), "none" (static only
addresses)
or "dhcp" (DHCP snooping).
Active leases are saved in a lease file and reloaded on restart or HUP.
The following interface XML activates and uses the DHCP snooping:
<interface type='bridge'>
<source bridge='virbr0'/>
<filterref filter='clean-traffic'>
<parameter name='CTRL_IP_LEARNING' value='dhcp'/>
</filterref>
</interface>
All filters containig the variable 'IP' are automatically adjusted when
s/containig/containing/
the VM receives an IP address via DHCP. However, multiple IP
addresses per
interface are silently ignored in this patch, thus only supporting one IP
address per interface. Multiple IP address support is added in a later
patch in this series.
Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com>
Signed-off-by: Stefan Berger <stefanb(a)linux.vnet.ibm.com>
---
Changes since v12:
- use pcap_create to open pcap to be able to set smaller buffer
than the 2 MB default buffer used by pcap which leads to processing
of huge backlog of messages if flooding occurrs
- use virAtomicInc/Dec
- use indep. rate controllers, one per direction to tune out of
flooding in each direction individually (even if flooding was to
occurr, libvirt doesn't use much CPU [0.3%])
s/occurr/occur/, although this note won't be in the final commit.
Index: libvirt-acl/docs/formatnwfilter.html.in
===================================================================
--- libvirt-acl.orig/docs/formatnwfilter.html.in
+++ libvirt-acl/docs/formatnwfilter.html.in
@@ -371,6 +371,118 @@
Further, the notation of $VARIABLE is short-hand for $VARIABLE[@0]. The
former notation always assumes the iterator with Id '0'.
<p>
+
+ <h3><a name="nwfelemsRulesAdvIPAddrDetection">Automatic IP
address detection</a></h3>
+ <p>
+ The detection of IP addresses used on a virtual machine's interface
+ is automatically activated if the variable <code>IP</code> is
referenced
+ but not value has been assign to it.
s/assign/assigned/
+ variable <code>DHCPSERVER</code> to the IP
address of a valid DHCP server
+ and provide filters that use this variable to filter incoming DHCP responses.
+ <br/><br/>
+ When DHCP snooping is enable and the DHCP lease expires,
s/enable/enabled/
+ the VM will no longer be able to use the IP address until it
acquires a
+ new, valid lease from a DHCP server. If the VM is migrated, it must get
+ a new valid DHCP lease to use an IP address (e.g., by
+ bringing the VM interface down and up again).
+ <br/><br/>
+ Note that automatic DHCP detection listens to the DHCP traffic
+ the VM exchanges with the DHCP server of the infrastructure. To avoid
+ denial-of-service attacks on libvirt, the evaluation of those packets
+ is rate-limited, meaning that a VM sending an excessive number of DHCP
+ packets per seconds on an interface will not have all of those packets
s/seconds/second/
+ evaluated and thus filters may not get adapted. Normal DHCP
client
+ behavior is assumed to send a low number of DHCP packets per second.
+ Further, it is important to setup approriate filters on all VMs in
s/approriate/appropriate/
+++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
Not a problem with your patch, per se, but we really should be using the
FSF-preferred form of LGPLv2+ boilerplate that uses a URL rather than a
street address (that's a global cleanup to all of libvirt).
+#include <config.h>
+
+#ifdef HAVE_LIBPCAP
+#include <pcap.h>
Needs indentation, to pass 'make syntax-check' if cppi is installed.
+
+#define VIR_FROM_THIS VIR_FROM_NWFILTER
+
+#ifdef HAVE_LIBPCAP
+
+#define LEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.leases"
And more instances of cppi complaints. I'll post a patch at the end...
+
+#define VIR_IFKEY_LEN ((VIR_UUID_STRING_BUFLEN) + (VIR_MAC_STRING_BUFLEN))
Technically over-parenthesized (unless VIR_UUID_STRING_BUFLEN is
under-parenthesized); you could get by with:
# define VIR_IFKEY_LEN \
(VIR_UUID_STRING_BUFLEN + VIR_MAC_STRING_BUFLEN)
but I don't mind leaving this line alone (it's easier to have a rule of
thumb of always using extra () in #defines than it is to think about
when () are necessary).
+
+struct _virNWFilterSnoopReq {
+ /*
+ * reference counter: while the req is on the
+ * publicSnoopReqs hash, the refctr may only
+ * be modified with the SnoopLock held
+ */
+ virAtomicInt refctr;
+
+ virNWFilterTechDriverPtr techdriver;
+ const char *ifname;
Why 'const char *'? Are we always going to point ifname to someone
else's (static) storage? Or are we going to strdup() our own copy, in
which case this should be 'char *', not 'const char *', as a hint that
we own the string? (Throughout the struct).
+ int ifindex;
+ const char *linkdev;
+ enum virDomainNetType nettype;
+ char ifkey[VIR_IFKEY_LEN];
+ unsigned char macaddr[VIR_MAC_BUFLEN];
Hmm, wondering if src/util/virmacaddr.h should typedef a MAC address.
[And while thinking about that, I just noticed that virMacAddrCompare
takes 'const char *', but virMacAddrFormat takes 'const unsigned char
*', so we aren't very consistent on whether we are using normal or
unsigned char for mac addrs.]
+ /*
+ * protect those members that can change while the
+ * req is on the public SnoopReq hash and
+ * at least one reference is held:
+ * - ifname
+ * - threadkey
+ * - start
+ * - end
+ * - a lease while it is on the list
+ * - threadStatus
+ * (for refctr, see above)
+ */
+ virMutex lock;
+};
+
+/*
+ * Note about lock-order:
+ * 1st: virNWFilterSnoopLock()
+ * 2nd: virNWFilterSnoopReqLock(req)
+ *
+ * Rationale: Former protects the SnoopReqs hash, latter its contents
Useful comments; thank you.
+
+typedef unsigned char Eaddr[6];
All the more reason that we should be typedef'ing a common MAC address
in virmacaddr.h.
+
+typedef struct _virNWFilterSnoopEthHdr virNWFilterSnoopEthHdr;
+typedef virNWFilterSnoopEthHdr *virNWFilterSnoopEthHdrPtr;
+
+struct _virNWFilterSnoopEthHdr {
+ Eaddr eh_dst;
+ Eaddr eh_src;
+ unsigned short eh_type;
We should be using uint16_t rather than trusting the size of 'unsigned
short',...
+ union {
+ unsigned char eu_data[0];
+ struct vlan_hdr {
+ unsigned short ev_flags;
+ unsigned short ev_type;
+ unsigned char ev_data[0];
Is a zero-length member really C99 compliant, or it is only a gcc extension?
+ } eu_vlh;
+ } eth_un;
+} ATTRIBUTE_PACKED;
...especially since you are packing this struct.
+
+#define eh_data eth_un.eu_data
+#define ehv_data eth_un.eu_vlh.ev_data
+#define ehv_type eth_un.eu_vlh.ev_type
+
+
+typedef struct _virNWFilterSnoopDHCPHdr virNWFilterSnoopDHCPHdr;
+typedef virNWFilterSnoopDHCPHdr *virNWFilterSnoopDHCPHdrPtr;
+
+struct _virNWFilterSnoopDHCPHdr {
+ uint8_t d_op;
+ uint8_t d_htype;
+ uint8_t d_hlen;
+ uint8_t d_hops;
+ uint32_t d_xid;
+ uint16_t d_secs;
+ uint16_t d_flags;
+ uint32_t d_ciaddr;
+ uint32_t d_yiaddr;
+ uint32_t d_siaddr;
+ uint32_t d_giaddr;
+ uint8_t d_chaddr[16];
+ char d_sname[64];
+ char d_file[128];
+ uint8_t d_opts[0];
Again a question about the validity of a 0-length member.
+
+#define MIN_VALID_DHCP_PKT_SIZE \
+ offsetof(virNWFilterSnoopEthHdr, eh_data) + \
+ sizeof(struct udphdr) + \
+ offsetof(virNWFilterSnoopDHCPHdr, d_opts)
Under-parenthesized.
+
+typedef struct _virNWFilterSnoopRateLimitConf virNWFilterSnoopRateLimitConf;
+typedef virNWFilterSnoopRateLimitConf *virNWFilterSnoopRateLimitConfPtr;
+
+struct _virNWFilterSnoopRateLimitConf {
+ time_t prev;
+ unsigned int pkt_ctr;
+ time_t burst;
+ const unsigned int rate;
const? Is this really a write-once structure?
+ const unsigned int burstRate;
+ const unsigned int burstInterval;
+};
+
+typedef struct _virNWFilterSnoopPcapConf virNWFilterSnoopPcapConf;
+typedef virNWFilterSnoopPcapConf *virNWFilterSnoopPcapConfPtr;
+
+struct _virNWFilterSnoopPcapConf {
+ pcap_t *handle;
+ const pcap_direction_t dir;
+ const char *filter;
Again, if we malloc filter, this should be 'char *', not 'const char *'.
+ virNWFilterSnoopRateLimitConf rateLimit; /* indep. rate limiters
*/
+ virAtomicInt qCtr; /* number of jobs in the worker's queue */
+ const unsigned int maxQSize;
Why is this const?
+static char *
+virNWFilterSnoopActivate(virThreadPtr thread)
+{
+ char *key;
+ unsigned long threadID = (unsigned long int)thread->thread;
You are not guaranteed that thread->thread can be converted cleanly to
an integral type; this smells like we are violating type encapsulation.
I'm worried that this cast will provoke compiler warnings on some
platforms. Can you get by with virThreadSelfID/virThreadID instead?
Or, since those functions are mainly for debugging purposes (and not
guaranteed to be unique on platforms that use 64-bit thread ids), what
are you really trying to do here that we should be supporting in
src/utils/threads.h?
+
+ if (virAsprintf(&key, "0x%0*lX", (int)sizeof(threadID)*2, threadID)
< 0) {
Why are you making this string have different length on 32- vs. 64-bit
platforms? Does it really have to be a fixed-width string?
+ virReportOOMError();
+ return NULL;
+ }
+
+ virNWFilterSnoopActiveLock();
+
+ if (virHashAddEntry(virNWFilterSnoopState.active, key, (void *)0x1) < 0) {
It looks like you are trying to make a map with a thread as the key; so
maybe the real thing to do here is make a utility function in
src/util/threads.h that can be used to hash a thread as a key without
you having to know the guts of how it works (and especially without you
having to stringize an integer representation of thread->thread).
+static bool
+virNWFilterSnoopIsActive(char *threadKey)
If we fix things to hash on a virThreadPtr instead of a stringized name
of the thread, then this function needs to change signature.
+/*
+ * virNWFilterSnoopListAdd - add an IP lease to a list
+ */
+static void
+virNWFilterSnoopListAdd(virNWFilterSnoopIPLeasePtr plnew,
+ virNWFilterSnoopIPLeasePtr *start,
+ virNWFilterSnoopIPLeasePtr *end)
+{
+ virNWFilterSnoopIPLeasePtr pl;
+
+ plnew->next = plnew->prev = 0;
Use NULL, not 0, for pointers. (I think newer gcc as shipped on F17 has
a warning that catches this; but I'm not sure if we are yet turning it
on, since I'm still on F16).
+/*
+ * virNWFilterSnoopListDel - remove an IP lease from a list
+ */
+static void
+virNWFilterSnoopListDel(virNWFilterSnoopIPLeasePtr ipl,
+ virNWFilterSnoopIPLeasePtr *start,
+ virNWFilterSnoopIPLeasePtr *end)
+{
+ if (ipl->prev)
+ ipl->prev->next = ipl->next;
+ else
+ *start = ipl->next;
+
+ if (ipl->next)
+ ipl->next->prev = ipl->prev;
+ else
+ *end = ipl->prev;
+
+ ipl->next = ipl->prev = 0;
Again, NULL, not 0, for pointers.
+/*
+ * virNWFilterSnoopInstallRule - install rule for a lease
+ *
+ * @instantiate: when calling this function in a loop, indicate
+ * the last call with 'true' here so that the
+ * rules all get instantiated
+ * Always calling this with 'true' is fine, but less
+ * efficient.
+ */
+static int
+virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
+ bool instantiate)
+{
+
+ if (virNWFilterHashTablePut(req->vars, NWFILTER_VARNAME_IP,
+ ipVar, 1) < 0) {
+ virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not add variable \""
+ NWFILTER_VARNAME_IP "\" to hashmap"));
'make syntax-check' doesn't like this unless you apply my patch:
https://www.redhat.com/archives/libvir-list/2012-April/msg01538.html
+/*
+ * virNWFilterSnoopReqLeaseTimerRun - run the IP lease timeout list
+ */
+static unsigned int
+virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReqPtr req)
+{
+ time_t now = time(0);
Should we be using src/util/virtime.h (with milliseconds) rather than
time() (with seconds)?
+
+ req->threadStatus = THREAD_STATUS_NONE;
+
+ if (virAtomicIntInit(&req->refctr) < 0 ||
+ virMutexInitRecursive(&req->lock) < 0 ||
+ virStrcpyStatic(req->ifkey, ifkey) == NULL ||
+ virCondInit(&req->threadStatusCond) < 0)
+ VIR_FREE(req);
Resource leak. If virMutexInitRecursive() succeeds, but virCondInit()
then fails, you've lost a mutex.
+
+ return req;
+}
+
+/*
+ * Free a snoop request unless it is still referenced.
+ * All its associated leases are also freed.
+ * The lease file is NOT rewritten.
+ */
+static void
+virNWFilterSnoopReqFree(virNWFilterSnoopReqPtr req)
+{
+ virNWFilterSnoopIPLeasePtr ipl;
+
+ if (!req)
+ return;
+
+ if (virAtomicIntRead(&req->refctr) != 0)
+ return;
+
+ /* free all leases */
+ for (ipl = req->start; ipl; ipl = req->start)
+ virNWFilterSnoopReqLeaseDel(req, &ipl->ipAddress, false);
+
+ /* free all req data */
+ VIR_FREE(req->ifname);
+ VIR_FREE(req->linkdev);
+ VIR_FREE(req->filtername);
+ virNWFilterHashTableFree(req->vars);
+
+ VIR_FREE(req);
Resource leak; you didn't clean up things like req->lock.
+
+/*
+ * virNWFilterSnoopReqRelease - hash table free function to kill a request
+ */
+static void
+virNWFilterSnoopReqRelease(void *req0, const void *name ATTRIBUTE_UNUSED)
+{
+ virNWFilterSnoopReqPtr req = (virNWFilterSnoopReqPtr) req0;
The cast is not necessary in C (void* can be assigned to anything other
pointer without a cast).
+/*
+ * Drop the reference to the Snoop request. Don't use the req
+ * after this call.
+ */
+static void
+virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr req)
+{
+ if (!req)
+ return;
+
+ virNWFilterSnoopLock()
How did this compile? Oh, because virNWFilterSnoopLock() is an unusual
macro. I would have made it be a 'do { ... } while (false)' idiom, to
make a semicolon mandatory in all callers, and prevent this unusual
looking construct.
+static int
+virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, int len,
+ int *pmtype, int *pleasetime)
+{
+ int oind, olen;
+ int oend;
+
+ olen = len - sizeof(*pd);
+ oind = 0;
+
+ if (olen < 4) /* bad magic */
+ return -1;
+
+ if (memcmp(dhcp_magic, pd->d_opts, sizeof(dhcp_magic)) != 0)
+ return -1; /* bad magic */
+
+ oind += sizeof(dhcp_magic);
+
+ oend = 0;
+
+ *pmtype = *pleasetime = 0;
+
+ while (oind < olen) {
+ switch (pd->d_opts[oind]) {
+ case DHCPO_LEASE:
Style nit: We have an inconsistent mix, but more of our code tends to
indent 'case' flush with 'switch' than code that indents 'case'
four
spaces beyond 'switch'.
+ if (olen - oind < 6)
+ goto malformed;
+ if (*pleasetime)
+ return -1; /* duplicate lease time */
+ *pleasetime =
+ ntohl(*(unsigned int *) (pd->d_opts + oind + 2));
This type-punning looks dangerous. It may generate unaligned accesses
which fail on some platforms. I think you need to build the int by
bitwise operations on 4 bytes, rather than using type-punning.
+
+ /* check for spoofed or packets not destined for this VM */
+ if (fromVM) {
+ if (memcmp(req->macaddr, pep->eh_src, 6) != 0)
+ return -2;
+ } else {
+ /*
+ * packets not destined for us can occurr while the bridge is
s/occurr/occur/
+ * learning the MAC addresses on ports
+ */
+ if (memcmp(req->macaddr, pep->eh_dst, 6) != 0)
+ return -2;
+ }
+
+ pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2));
More type-punning. I hope this doesn't back-fire.
+static pcap_t *
+virNWFilterSnoopDHCPOpen(const char *ifname, const unsigned char *mac,
+ const char *filter, pcap_direction_t dir)
+{
+ pcap_t *handle = NULL;
+ struct bpf_program fp;
+ char pcap_errbuf[PCAP_ERRBUF_SIZE];
+ char *ext_filter = NULL;
+ char macaddr[VIR_MAC_STRING_BUFLEN];
+ const char *ext;
+
+ virMacAddrFormat(mac, macaddr);
+
+ if (dir == PCAP_D_IN /* from VM */) {
+ /*
+ * don't want to hear about another VM's DHCP requests
+ *
+ * extend the filter with the macaddr of the VM; filter the
+ * more unlikely parameters first, then go for the MAC
+ */
+ ext = "and ether src";
+ } else {
+ ext = "and ether dst";
+ }
+
+ if (virAsprintf(&ext_filter, "%s %s %s", filter, ext, macaddr) < 0)
{
This is broken for translation. You did not mark the string "and ether
src" for translation.
I'd do:
if (dir == PCAP_D_IN) {
format = _("%s and ether src %s");
else
format = _("%s and ether dst %s");
virAsprintf(&ext_filter, format, filter, macaddr);
or something along those lines.
+ virReportOOMError();
+ return NULL;
+ }
+
+ handle = pcap_create(ifname, pcap_errbuf);
+
+ if (handle == NULL) {
+ virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+ _("pcap_create failed"));
If this is a normal user-visible error, it probably should not be
INTERNAL_ERROR.
+
+ if (pcap_compile(handle, &fp, ext_filter, 1, PCAP_NETMASK_UNKNOWN) != 0) {
+ virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+ _("pcap_compile: %s"), pcap_geterr(handle));
Is pcap_geterr() thread-safe? I know that later on you used strerror(),
which is NOT thread-safe, and which violates 'make syntax-check'.
+/*
+ * Submit a job to the worker thread doing the time-consuming work...
+ */
+static int
+virNWFilterSnoopDHCPDecodeJobSubmit(virThreadPoolPtr pool,
+ virNWFilterSnoopEthHdrPtr pep,
+ int len, pcap_direction_t dir,
+ virAtomicIntPtr qCtr)
I've run out of review time today. Here's what I had to add to get
'make syntax-check' to be happy, but there are a lot of other cleanups
I've mentioned above.
From adcdb61e42808821e81a2682ec49f05dbafb0048 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake(a)redhat.com>
Date: Mon, 30 Apr 2012 15:50:01 -0600
Subject: [PATCH] fixup to 2/5 dhcp
---
po/POTFILES.in | 1 +
src/nwfilter/nwfilter_dhcpsnoop.c | 74
+++++++++++++++++-------------------
src/nwfilter/nwfilter_dhcpsnoop.h | 2 +-
3 files changed, 37 insertions(+), 40 deletions(-)
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 4ea544b..0e64c96 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -53,6 +53,7 @@ src/node_device/node_device_hal.c
src/node_device/node_device_linux_sysfs.c
src/node_device/node_device_udev.c
src/nodeinfo.c
+src/nwfilter/nwfilter_dhcpsnoop.c
src/nwfilter/nwfilter_driver.c
src/nwfilter/nwfilter_ebiptables_driver.c
src/nwfilter/nwfilter_gentech_driver.c
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 1ed32ab..35a7908 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -42,7 +42,7 @@
#include <config.h>
#ifdef HAVE_LIBPCAP
-#include <pcap.h>
+# include <pcap.h>
#endif
#include <fcntl.h>
@@ -70,8 +70,8 @@
#ifdef HAVE_LIBPCAP
-#define LEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.leases"
-#define TMPLEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.ltmp"
+# define LEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.leases"
+# define TMPLEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.ltmp"
struct virNWFilterSnoopState {
/* lease file */
@@ -87,16 +87,16 @@ struct virNWFilterSnoopState {
virMutex activeLock; /* protects Active */
};
-#define virNWFilterSnoopLock() \
+# define virNWFilterSnoopLock() \
{ virMutexLock(&virNWFilterSnoopState.snoopLock); }
-#define virNWFilterSnoopUnlock() \
+# define virNWFilterSnoopUnlock() \
{ virMutexUnlock(&virNWFilterSnoopState.snoopLock); }
-#define virNWFilterSnoopActiveLock() \
+# define virNWFilterSnoopActiveLock() \
{ virMutexLock(&virNWFilterSnoopState.activeLock); }
-#define virNWFilterSnoopActiveUnlock() \
+# define virNWFilterSnoopActiveUnlock() \
{ virMutexUnlock(&virNWFilterSnoopState.activeLock); }
-#define VIR_IFKEY_LEN ((VIR_UUID_STRING_BUFLEN) +
(VIR_MAC_STRING_BUFLEN))
+# define VIR_IFKEY_LEN ((VIR_UUID_STRING_BUFLEN) +
(VIR_MAC_STRING_BUFLEN))
typedef struct _virNWFilterSnoopReq virNWFilterSnoopReq;
typedef virNWFilterSnoopReq *virNWFilterSnoopReqPtr;
@@ -191,9 +191,9 @@ struct _virNWFilterSnoopEthHdr {
} eth_un;
} ATTRIBUTE_PACKED;
-#define eh_data eth_un.eu_data
-#define ehv_data eth_un.eu_vlh.ev_data
-#define ehv_type eth_un.eu_vlh.ev_type
+# define eh_data eth_un.eu_data
+# define ehv_data eth_un.eu_vlh.ev_data
+# define ehv_type eth_un.eu_vlh.ev_type
typedef struct _virNWFilterSnoopDHCPHdr virNWFilterSnoopDHCPHdr;
@@ -219,25 +219,25 @@ struct _virNWFilterSnoopDHCPHdr {
/* DHCP options */
-#define DHCPO_PAD 0
-#define DHCPO_LEASE 51 /* lease time in secs */
-#define DHCPO_MTYPE 53 /* message type */
-#define DHCPO_END 255 /* end of options */
+# define DHCPO_PAD 0
+# define DHCPO_LEASE 51 /* lease time in secs */
+# define DHCPO_MTYPE 53 /* message type */
+# define DHCPO_END 255 /* end of options */
/* DHCP message types */
-#define DHCPDECLINE 4
-#define DHCPACK 5
-#define DHCPRELEASE 7
+# define DHCPDECLINE 4
+# define DHCPACK 5
+# define DHCPRELEASE 7
-#define MIN_VALID_DHCP_PKT_SIZE \
+# define MIN_VALID_DHCP_PKT_SIZE \
offsetof(virNWFilterSnoopEthHdr, eh_data) + \
sizeof(struct udphdr) + \
offsetof(virNWFilterSnoopDHCPHdr, d_opts)
-#define PCAP_PBUFSIZE 576 /* >= IP/TCP/DHCP headers */
-#define PCAP_READ_MAXERRS 25 /* retries on failing device */
-#define PCAP_FLOOD_TIMEOUT_MS 10 /* ms */
+# define PCAP_PBUFSIZE 576 /* >= IP/TCP/DHCP headers */
+# define PCAP_READ_MAXERRS 25 /* retries on failing device */
+# define PCAP_FLOOD_TIMEOUT_MS 10 /* ms */
typedef struct _virNWFilterDHCPDecodeJob virNWFilterDHCPDecodeJob;
typedef virNWFilterDHCPDecodeJob *virNWFilterDHCPDecodeJobPtr;
@@ -249,13 +249,13 @@ struct _virNWFilterDHCPDecodeJob {
virAtomicIntPtr qCtr;
};
-#define DHCP_PKT_RATE 10 /* pkts/sec */
-#define DHCP_PKT_BURST 50 /* pkts/sec */
-#define DHCP_BURST_INTERVAL_S 10 /* sec */
+# define DHCP_PKT_RATE 10 /* pkts/sec */
+# define DHCP_PKT_BURST 50 /* pkts/sec */
+# define DHCP_BURST_INTERVAL_S 10 /* sec */
-#define PCAP_BUFFERSIZE (DHCP_PKT_BURST * PCAP_PBUFSIZE / 2)
+# define PCAP_BUFFERSIZE (DHCP_PKT_BURST * PCAP_PBUFSIZE / 2)
-#define MAX_QUEUED_JOBS (DHCP_PKT_BURST + 2 * DHCP_PKT_RATE)
+# define MAX_QUEUED_JOBS (DHCP_PKT_BURST + 2 * DHCP_PKT_RATE)
typedef struct _virNWFilterSnoopRateLimitConf
virNWFilterSnoopRateLimitConf;
typedef virNWFilterSnoopRateLimitConf *virNWFilterSnoopRateLimitConfPtr;
@@ -597,8 +597,6 @@ virNWFilterSnoopReqNew(const char *ifkey)
return NULL;
}
- virNWFilterSnoopReqGet(req);
-
req->threadStatus = THREAD_STATUS_NONE;
if (virAtomicIntInit(&req->refctr) < 0 ||
@@ -607,6 +605,9 @@ virNWFilterSnoopReqNew(const char *ifkey)
virCondInit(&req->threadStatusCond) < 0)
VIR_FREE(req);
+ if (req)
+ virNWFilterSnoopReqGet(req);
+
return req;
}
@@ -1208,7 +1209,7 @@
virNWFilterSnoopRateLimit(virNWFilterSnoopRateLimitConfPtr rl)
{
time_t now = time(0);
int diff;
-#define IN_BURST(n,b) ((n)-(b) <= 1) /* bursts span 2 discreet seconds */
+# define IN_BURST(n,b) ((n)-(b) <= 1) /* bursts span 2 discreet seconds */
if (rl->prev != now && !IN_BURST(now, rl->burst)) {
rl->prev = now;
@@ -1754,9 +1755,7 @@ virNWFilterSnoopLeaseFileWrite(int lfd, const char
*ifkey,
len = strlen(lbuf);
if (safewrite(lfd, lbuf, len) != len) {
- virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
- _("lease file write failed: %s"),
- strerror(errno));
+ virReportSystemError(errno, "%s", _("lease file write
failed"));
ret = -1;
goto cleanup;
}
@@ -1865,9 +1864,7 @@ virNWFilterSnoopLeaseFileRefresh(void)
/* 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));
+ virReportSystemError(errno, _("open(\"%s\")"),
TMPLEASEFILE);
return;
}
@@ -1883,9 +1880,8 @@ virNWFilterSnoopLeaseFileRefresh(void)
VIR_FORCE_CLOSE(tfd);
if (rename(TMPLEASEFILE, LEASEFILE) < 0) {
- virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
- _("rename(\"%s\", \"%s\"):
%s"),
- TMPLEASEFILE, LEASEFILE, strerror(errno));
+ virReportSystemError(errno, _("rename(\"%s\",
\"%s\")"),
+ TMPLEASEFILE, LEASEFILE);
(void) unlink(TMPLEASEFILE);
}
virAtomicIntSet(&virNWFilterSnoopState.wLeases, 0);
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.h
b/src/nwfilter/nwfilter_dhcpsnoop.h
index 61ea894..9a1d797 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.h
+++ b/src/nwfilter/nwfilter_dhcpsnoop.h
@@ -22,7 +22,7 @@
*/
#ifndef __NWFILTER_DHCPSNOOP_H
-#define __NWFILTER_DHCPSNOOP_H
+# define __NWFILTER_DHCPSNOOP_H
int virNWFilterDHCPSnoopInit(void);
void virNWFilterDHCPSnoopShutdown(void);
--
1.7.7.6
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org