[libvirt] [libvirt PATCHv7 1/1] add DHCP snooping

[Daniel P. Berrange]

On Mon, Mar 26, 2012 at 01:25:48PM -0700, David L Stevens wrote:
> This patch adds DHCP snooping support to libvirt. The learning method for
> IP addresses is specified by setting the "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.
> 
> Changes since v6:
> - replace pthread_cancel() with synchronous cancelation method
> 
> Signed-off-by: David L Stevens <dlstevens at us.ibm.com>
> ---
>  docs/formatnwfilter.html.in            |   17 +
>  src/Makefile.am                        |    2 +
>  src/nwfilter/nwfilter_dhcpsnoop.c      | 1139 ++++++++++++++++++++++++++++++++
>  src/nwfilter/nwfilter_dhcpsnoop.h      |   38 ++
>  src/nwfilter/nwfilter_driver.c         |    6 +
>  src/nwfilter/nwfilter_gentech_driver.c |   59 ++-
>  6 files changed, 1248 insertions(+), 13 deletions(-)
>  create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.c
>  create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.h
> 
> diff --git a/docs/formatnwfilter.html.in b/docs/formatnwfilter.html.in
> index 9cb7644..ad10765 100644
> --- a/docs/formatnwfilter.html.in
> +++ b/docs/formatnwfilter.html.in

> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> new file mode 100644
> index 0000000..8e5dcc5
> --- /dev/null
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c

> +#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 virHashTablePtr IfnameToKey;
> +static pthread_mutex_t SnoopLock = PTHREAD_MUTEX_INITIALIZER;
> +
> +/* snooper thread management */
> +
> +static virHashTablePtr Active;
> +static pthread_mutex_t ActiveLock = PTHREAD_MUTEX_INITIALIZER;

I'd prefer to see all these random variables put into a
'struct virNWFilterSnoopState', and have a single global
variable, or even better, pass an instance of the struct
into the methods that require it.


> +SnoopActivate(pthread_t thread)
> +SnoopCancel(char **ThreadKey)
> +SnoopIsActive(char *ThreadKey)

> +#define snoop_lock()    { pthread_mutex_lock(&SnoopLock); }
> +#define snoop_unlock()  { pthread_mutex_unlock(&SnoopLock); }

> +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 *ifkey);
> +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(struct iplease *plnew, struct iplease **start, struct iplease **end)

> +ipl_tadd(struct iplease *plnew)

> +ipl_install(struct iplease *ipl)

> +ipl_add(struct iplease *plnew, bool update_leasefile)

> +ipl_ldel(struct iplease *ipl, struct iplease **start, struct iplease **end)

> +ipl_tdel(struct iplease *ipl)

> +ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr, bool update_leasefile)

> +ipl_update(struct iplease *ipl, uint32_t timeout)

> +ipl_getbyip(struct iplease *start, uint32_t ipaddr)

> +ipl_trun(struct virNWFilterSnoopReq *req)

> +struct eth {

> +struct dhcp {

> +dhcp_getopt(struct dhcp *pd, int len, int *pmtype, int *pleasetime)

> +dhcpdecode(struct virNWFilterSnoopReq *req, struct eth *pep, int len)

> +dhcpopen(const char *intf)

> +snoopReqFree(struct virNWFilterSnoopReq *req)

> +virNWFilterDHCPSnoop(void *req0)

> +ifkeyFormat(char *ifkey, const unsigned char *vmuuid,
> +            unsigned const char *macaddr)

> +virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
> +                        const char *ifname,
> +                        const char *linkdev,
> +                        enum virDomainNetType nettype,
> +                        const unsigned char *vmuuid,
> +                        const unsigned char *macaddr,
> +                        const char *filtername,
> +                        virNWFilterHashTablePtr filterparams,
> +                        virNWFilterDriverStatePtr driver)

> +freeReq(void *req0, const void *name ATTRIBUTE_UNUSED)

> +lease_close(void)

> +lease_open(void)

> +virNWFilterDHCPSnoopInit(void)

> +virNWFilterDHCPSnoopEnd(const char *ifname)

> +lease_write(int lfd, const char *ifkey, struct iplease *ipl)

> +lease_save(struct iplease *ipl)

> +newreq(const char *ifkey)

> +SaveSnoopReqIter(void *payload,
> +                 const void *name ATTRIBUTE_UNUSED,
> +                 void *data)

> +lease_refresh(void)

> +lease_load(void)

> +lease_restore(struct virNWFilterSnoopReq *req)


The number of different function / struct naming schemes used here
is out of control. Please change everything to use 'virNWFilter'
or 'virNWFilterDHCPSnoop' as a prefix, and avoid '_' in function
or struct names.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|