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@xxxxxxxxxx>
> ---
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list