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