Re: [libvirt] [PATCH 8/9] add DHCP snooping support to nwfilter
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
Attachments:
- attachment.html (text/html — 6.5 KB)