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