9:04 a.m.
On 04/30/2012 07:14 PM, Eric Blake wrote:
On 04/25/2012 06:59 AM, Stefan Berger wrote:
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
Not a problem with your patch, per se, but we really should be using the
FSF-preferred form of LGPLv2+ boilerplate that uses a URL rather than a
street address (that's a global cleanup to all of libvirt).
Will fix it.
> +#include<config.h>
> +
> +#ifdef HAVE_LIBPCAP
> +#include<pcap.h>
Needs indentation, to pass 'make syntax-check' if cppi is installed.
Of course I have cppi installed but 'make syntax-check' doesn't seem to
check these files.
> +
> +struct _virNWFilterSnoopReq {
> + /*
> + * reference counter: while the req is on the
> + * publicSnoopReqs hash, the refctr may only
> + * be modified with the SnoopLock held
> + */
> + virAtomicInt refctr;
> +
> + virNWFilterTechDriverPtr techdriver;
> + const char *ifname;
Why 'const char *'? Are we always going to point ifname to someone
else's (static) storage? Or are we going to strdup() our own copy, in
which case this should be 'char *', not 'const char *', as a hint that
we own the string? (Throughout the struct).
Right, this one should not be 'const'.
> + int ifindex;
> + const char *linkdev;
> + enum virDomainNetType nettype;
> + char ifkey[VIR_IFKEY_LEN];
> + unsigned char macaddr[VIR_MAC_BUFLEN];
Hmm, wondering if src/util/virmacaddr.h should typedef a MAC address.
[And while thinking about that, I just noticed that virMacAddrCompare
takes 'const char *', but virMacAddrFormat takes 'const unsigned char
*', so we aren't very consistent on whether we are using normal or
unsigned char for mac addrs.]
The compare function is ok -- it compares the two MAC addresses by
string (not binary).
So I'll introduce this typedef here
typedef unsigned char virMacAddr[VIR_MAC_BUFLEN];
but won't go into the API calls' signatures because that will require a
lot more work.
> + unsigned char eu_data[0];
> + struct vlan_hdr {
> + unsigned short ev_flags;
> + unsigned short ev_type;
> + unsigned char ev_data[0];
Is a zero-length member really C99 compliant, or it is only a gcc extension?
C99 seems to support [] rather than the [0]. So I'll adapt it to C99 but
also will drop this VLAN part here.
> +
> +typedef struct _virNWFilterSnoopRateLimitConf virNWFilterSnoopRateLimitConf;
> +typedef virNWFilterSnoopRateLimitConf *virNWFilterSnoopRateLimitConfPtr;
> +
> +struct _virNWFilterSnoopRateLimitConf {
> + time_t prev;
> + unsigned int pkt_ctr;
> + time_t burst;
> + const unsigned int rate;
const? Is this really a write-once structure?
Yes, the rate is assigned once during structure initialization. At least
for now it is a constant.
> + const unsigned int burstRate;
> + const unsigned int burstInterval;
> +};
> +
> +typedef struct _virNWFilterSnoopPcapConf virNWFilterSnoopPcapConf;
> +typedef virNWFilterSnoopPcapConf *virNWFilterSnoopPcapConfPtr;
> +
> +struct _virNWFilterSnoopPcapConf {
> + pcap_t *handle;
> + const pcap_direction_t dir;
> + const char *filter;
Again, if we malloc filter, this should be 'char *', not 'const char *'.
The filter is hard-coded.
> + virNWFilterSnoopRateLimitConf rateLimit; /* indep. rate
limiters */
> + virAtomicInt qCtr; /* number of jobs in the worker's queue */
> + const unsigned int maxQSize;
Why is this const?
Also this one we set once at structure initialization time.
> +static char *
> +virNWFilterSnoopActivate(virThreadPtr thread)
> +{
> + char *key;
> + unsigned long threadID = (unsigned long int)thread->thread;
You are not guaranteed that thread->thread can be converted cleanly to
an integral type; this smells like we are violating type encapsulation.
I'm worried that this cast will provoke compiler warnings on some
platforms. Can you get by with virThreadSelfID/virThreadID instead?
virThreadID looks like it could be the right function if it didn't
truncate the return value.
Or, since those functions are mainly for debugging purposes (and not
guaranteed to be unique on platforms that use 64-bit thread ids), what
are you really trying to do here that we should be supporting in
src/utils/threads.h?
We want to have a unique value associated with a thread in that map. As
long as that value is found in the map, the thread knowns it should keep
on monitoring the traffic. We can also use 'req' for that since that
lives as long as the thread does and so the pointer cannot appear again.
Here is what I changed the function to now so the lookup via the hash
table works as before:
static char *
virNWFilterSnoopActivate(virNWFilterSnoopReqPtr req)
{
char *key;
if (virAsprintf(&key, "%p", req) < 0) {
virReportOOMError();
return NULL;
}
[...]
> +/*
> + * virNWFilterSnoopReqLeaseTimerRun - run the IP lease timeout list
> + */
> +static unsigned int
> +virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReqPtr req)
> +{
> + time_t now = time(0);
Should we be using src/util/virtime.h (with milliseconds) rather than
time() (with seconds)?
The DHCP leases work on the granularity of seconds. So in this case
seconds are good enough I would say.
> +
> + /* free all leases */
> + for (ipl = req->start; ipl; ipl = req->start)
> + virNWFilterSnoopReqLeaseDel(req,&ipl->ipAddress, false);
> +
> + /* free all req data */
> + VIR_FREE(req->ifname);
> + VIR_FREE(req->linkdev);
> + VIR_FREE(req->filtername);
> + virNWFilterHashTableFree(req->vars);
> +
> + VIR_FREE(req);
Resource leak; you didn't clean up things like req->lock.
Fixed.
> + * learning the MAC addresses on ports
> + */
> + if (memcmp(req->macaddr, pep->eh_dst, 6) != 0)
> + return -2;
> + }
> +
> + pup = (struct udphdr *) ((char *) pip + (pip->ihl<< 2));
More type-punning. I hope this doesn't back-fire.
To be safe I now converted all 4 byte accesses to use memcpy(&nwint,
...) and then a subsequent ntohl(nwint).
> +static pcap_t *
> +virNWFilterSnoopDHCPOpen(const char *ifname, const unsigned char *mac,
> + const char *filter, pcap_direction_t dir)
> +{
> + pcap_t *handle = NULL;
> + struct bpf_program fp;
> + char pcap_errbuf[PCAP_ERRBUF_SIZE];
> + char *ext_filter = NULL;
> + char macaddr[VIR_MAC_STRING_BUFLEN];
> + const char *ext;
> +
> + virMacAddrFormat(mac, macaddr);
> +
> + if (dir == PCAP_D_IN /* from VM */) {
> + /*
> + * don't want to hear about another VM's DHCP requests
> + *
> + * extend the filter with the macaddr of the VM; filter the
> + * more unlikely parameters first, then go for the MAC
> + */
> + ext = "and ether src";
> + } else {
> + ext = "and ether dst";
> + }
> +
> + if (virAsprintf(&ext_filter, "%s %s %s", filter, ext, macaddr)<
0) {
This is broken for translation. You did not mark the string "and ether
src" for translation.
It is not meant for a user to read, but this extended filter string will
be compiled by the pcap library.
Actually that comparison for source and destination MAC address in the
DHCP packet parser can be removed due to the filtering by source and
dest. MAC addresses in the pcap library -- no need to do it twice.
> + virReportOOMError();
> + return NULL;
> + }
> +
> + handle = pcap_create(ifname, pcap_errbuf);
> +
> + if (handle == NULL) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("pcap_create failed"));
If this is a normal user-visible error, it probably should not be
INTERNAL_ERROR.
It will not be visible by the user.
> +
> + if (pcap_compile(handle,&fp, ext_filter, 1, PCAP_NETMASK_UNKNOWN) != 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("pcap_compile: %s"),
pcap_geterr(handle));
Is pcap_geterr() thread-safe? I know that later on you used strerror(),
which is NOT thread-safe, and which violates 'make syntax-check'.
Yes, it is thread-safe. It only accesses a buffer attached to 'handle',
where in turn handle is unique to the thread.
> +/*
> + * Submit a job to the worker thread doing the time-consuming work...
> + */
> +static int
> +virNWFilterSnoopDHCPDecodeJobSubmit(virThreadPoolPtr pool,
> + virNWFilterSnoopEthHdrPtr pep,
> + int len, pcap_direction_t dir,
> + virAtomicIntPtr qCtr)
I've run out of review time today. Here's what I had to add to get
'make syntax-check' to be happy, but there are a lot of other cleanups
I've mentioned above.
Thanks for the review so far. I would have caught the make syntax-check
stuff myself, but unfortunately it doesn't seem to look into the directory.
Stefan