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