David Stevens/Beaverton/IBM@IBMUS wrote on 05/09/2011 04:11:19 PM:
+ */ + +#include <config.h> + +#ifdef HAVE_LIBPCAP +#include <pcap.h> +#endif
This is the only place you check for HAVE_LIBPCAP. Basically when it's not available the snooping doesn't work and VMs referencing filters that have the variable 'IP' no user-given value cannot start. The part checking again for HAVE_LIBPCAP further down seems to be missing.
+virNWFilterDHCPSnoopReq(virConnectPtr conn, + virNWFilterTechDriverPtr techdriver ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + virNWFilterDefPtr filter ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + virNWFilterHashTablePtr vars ATTRIBUTE_UNUSED, + virNWFilterDriverStatePtr driver ATTRIBUTE_UNUSED) +{ + struct virNWFilterSnoopReq *req; + + req = virHashLookup(SnoopReqs, ifname);
Presumably multiple threads can be here, so you'll have to protect the hash table 'SnoopReqs' from concurrent accesses -- everywhere.
+ if (req) + return 0; + if (VIR_ALLOC(req) < 0) { + virReportOOMError(); + return 1; + } + + req->conn = conn; + req->techdriver = techdriver; + req->nettype = nettype; + req->filter = filter; + req->ifname = strdup(ifname); + req->vars = virNWFilterHashTableCreate(0); + if (!req->vars) { + virReportOOMError(); + return 1; + } + if (virNWFilterHashTablePutAll(vars, req->vars)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq: can't copy variables" + " on if %s"), ifname); + return 1; + } + req->driver = driver; + req->start = req->end = 0; + + if (virHashAddEntry(SnoopReqs, ifname, req)) {
Also protect it here.
+ VIR_FREE(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq req add failed on" + "interface \"%s\""), ifname); + return 1; + } + if (pthread_create(&req->thread, NULL, virNWFilterDHCPSnoop, req) != 0) { + (void) virHashRemoveEntry(SnoopReqs, ifname);
... and here ...
+ VIR_FREE(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq pthread_create failed" + " oninterface \"%s\""), ifname); + return 1; + } + return 0; +} + +/* + * freeReq - hash table free function to kill a request + */ +static void +freeReq(void *req0, const void *name ATTRIBUTE_UNUSED) +{ + struct virNWFilterSnoopReq *req = (struct virNWFilterSnoopReq *) req0; + + if (!req) + return; + + req->die = 1; +} + +int +virNWFilterDHCPSnoopInit(void) +{ + if (SnoopReqs) + return 0; + SnoopReqs = virHashCreate(0, freeReq); + if (!SnoopReqs) { + virReportOOMError(); + return -1; + } + return 0; +} + +void +virNWFilterDHCPSnoopEnd(const char *ifname) +{ + if (!SnoopReqs) + return; + if (ifname) + virHashRemoveEntry(SnoopReqs, ifname);
.. and again protect here. Since SnoopReqs is a global variable, maybe wrap the hash table operations into functions of their own. Great work! It's overall pretty complex. I'll try it at some point. Regards, Stefan