Re: [PATCH V13 2/5] nwfilter: add DHCP snooping

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]