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@xxxxxxxxxx>
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxx>
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list