<tt>David Stevens/Beaverton/IBM@IBMUS wrote on 03/26/2012 04:25:48 PM: </tt> <tt>> > This patch adds DHCP snooping support to libvirt. The learning method for > IP addresses is specified by setting the "ip_learning" variable to one of > "any" [default] (existing IP learning code), "none" (static only addresses) > or "dhcp" (DHCP snooping). > > Active leases are saved in a lease file and reloaded on restart or HUP. > > Changes since v6: > - replace pthread_cancel() with synchronous cancelation method > > Signed-off-by: David L Stevens <dlstevens@us.ibm.com> > --- [...] > + > +static char * > +SnoopActivate(pthread_t thread) > +{ > + int len; > + char *key; > + > + len = sizeof(thread)*2 + 3; /* "0x"+'\0' */ > + if (VIR_ALLOC_N(key, len)) { > + virReportOOMError(); > + return NULL; > + } > + (void) snprintf(key, len, "0x%0*lX", sizeof(thread)*2, > + (unsigned long int)thread); </tt> <tt>> + key[len-1] = '\0'; </tt> <tt>Why does this string have to be closed here? You should use virAsprintf() rather than the malloc+snprintf.</tt> <tt>> + > + active_lock(); > + if (virHashAddEntry(Active, key, strdup("1"))) > + VIR_FREE(key);</tt> <tt>Check strdup() for NULL.</tt> <tt> > +} > + > +/* > + * ipl_install - install rule for a lease > + */ > +static int > +ipl_install(struct iplease *ipl) > +{ > + char ipbuf[20]; /* dotted decimal IP > addr string */</tt> <tt>Use INET_ADDRSTRLEN, which is 16 and corresponds to exactly the number of bytes needed. Also you use 16 further below.</tt> <tt> > + int rc; > + virNWFilterVarValuePtr ipVar; > + > + if (!inet_ntop(AF_INET, &ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("ipl_install inet_ntop " "failed (0x%08X)"), </tt> <tt>Why are there two separate strings?</tt> <tt>> + > +/* > + * ipl_trun - run the IP lease timeout list > + */ > +static unsigned int > +ipl_trun(struct virNWFilterSnoopReq *req) > +{ > + uint32_t now;</tt> <tt>time_t ?</tt> <tt>> + > +unsigned char dhcp_magic[4] = { 99, 130, 83, 99 };</tt> <tt>static const unsigned char dhcp_magic[4] ...</tt> <tt>> + pip = (struct iphdr *) pep->eh_data; > + len -= sizeof(*pep);</tt> <tt>before you used sizeof xyz and here sizeof(xyz). Can you converge to one style? Preferably the latter.</tt> <tt>Check len for < 0 to not step into memory that may not have been allocated (for example pip->ihl).</tt> <tt>> + > +#define PBUFSIZE 576 /* >= IP/TCP/DHCP headers */ > +#define TIMEOUT 30 /* secs */ > + > +static pcap_t * > +dhcpopen(const char *intf) > +{ > + pcap_t *handle = NULL; > + struct bpf_program fp; > + char filter[64]; > + char pcap_errbuf[PCAP_ERRBUF_SIZE]; > + time_t start; > + > + start = time(0); > + while (handle == NULL && time(0) - start < TIMEOUT) > + handle = pcap_open_live(intf, PBUFSIZE, 0, POLL_INTERVAL, > pcap_errbuf); > + > + if (handle == NULL) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("pcap_open_live: %s"), pcap_errbuf); > + return 0; > + } > + > + sprintf(filter, "port 67 or dst port 68"); > + if (pcap_compile(handle, &fp, filter, 1, PCAP_NETMASK_UNKNOWN) != 0) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("pcap_compile: %s"), pcap_geterr(handle)); > + return 0; > + } > + if (pcap_setfilter(handle, &fp) != 0) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("pcap_setfilter: %s"), pcap_geterr(handle)); </tt> <tt>pcap_freecode()...</tt> <tt>> + > +static void * > +virNWFilterDHCPSnoop(void *req0) > +{ > + struct virNWFilterSnoopReq *req = req0; > + pcap_t *handle; > + struct pcap_pkthdr *hdr; > + struct eth *packet; > + int ifindex; > + int errcount; > + char *threadkey; > + > + handle = dhcpopen(req->ifname); > + if (!handle) > + return 0; > + > + req->threadkey = SnoopActivate(pthread_self()); > + threadkey = strdup(req->threadkey);</tt> <tt>Check for NULL.</tt> <tt>> + > +int > +virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, > + const char *ifname, > + const char *linkdev, > + enum virDomainNetType nettype, > + const unsigned char *vmuuid, > + const unsigned char *macaddr, > + const char *filtername, > + virNWFilterHashTablePtr filterparams, > + virNWFilterDriverStatePtr driver) > +{ > + struct virNWFilterSnoopReq *req; > + bool isnewreq; > + char ifkey[VIR_IFKEY_LEN]; > + pthread_t thread; > + > + ifkeyFormat(ifkey, vmuuid, macaddr); > + snoop_lock(); > + req = virHashLookup(SnoopReqs, ifkey); > + isnewreq = req == NULL; > + if (!isnewreq) { > + if (req->threadkey) { > + snoop_unlock(); > + return 0; > + } > + } else { > + req = newreq(ifkey); > + if (!req) { > + snoop_unlock(); > + return 1;</tt> <tt>All functions now return -1 on error.</tt> <tt> > + } > + } > + > + req->techdriver = techdriver; > + req->ifindex = if_nametoindex(ifname); > + req->linkdev = linkdev ? strdup(linkdev) : NULL; > + req->nettype = nettype; > + req->ifname = strdup(ifname); > + memcpy(req->macaddr, macaddr, sizeof(req->macaddr)); > + req->filtername = strdup(filtername); > + if (req->filtername == NULL) {</tt> <tt>Also check req->ifname for NULL.</tt> <tt> > + snoop_unlock(); > + snoopReqFree(req); > + virReportOOMError(); > + return 1; > + }</tt> <tt>return -1;</tt> <tt>> + > +static void > +lease_close(void) > +{ > + VIR_FORCE_CLOSE(lease_fd); > +} > + > +static void > +lease_open(void) > +{ > + lease_close(); > + > + lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644);</tt> <tt>Does this have to be a global variable?</tt> <tt> > +} > + > +int > +virNWFilterDHCPSnoopInit(void) > +{ > + if (SnoopReqs) > + return 0; > + > + snoop_lock(); > + IfnameToKey = virHashCreate(0, NULL); > + SnoopReqs = virHashCreate(0, freeReq); > + if (!SnoopReqs) { > + snoop_unlock(); </tt> <tt>free IfnameToKey? Have a common exit for error cases .</tt> <tt>> + virReportOOMError(); > + return -1; > + } > + lease_load(); > + lease_open(); > + > + snoop_unlock(); > + > + active_lock(); > + Active = virHashCreate(0, 0); > + if (!Active) { > + active_unlock(); > + virReportOOMError(); </tt> <tt>free hash tables above?</tt> <tt>> + return -1; > + } > + active_unlock(); > + return 0; > +} > + > +static int > +lease_write(int lfd, const char *ifkey, struct iplease *ipl) > +{ > + char lbuf[256],ipstr[16],dhcpstr[16]; > +</tt> <tt>See above regarding the '16'. Space after comma.</tt> <tt> > + > +static struct virNWFilterSnoopReq * > +newreq(const char *ifkey) > +{ > + struct virNWFilterSnoopReq *req; > + > + if (VIR_ALLOC(req) < 0) { > + virReportOOMError(); > + return NULL; > + } > + strncpy(req->ifkey, ifkey, sizeof req->ifkey);</tt> <tt>?? req->ifkey is NULL.</tt> <tt>Also see Hacking for usage of strncpy. Do not use it.</tt> <tt> > + > + return req; > +} > + > +static void > +SaveSnoopReqIter(void *payload, > + const void *name ATTRIBUTE_UNUSED, > + void *data) > +{ > + struct virNWFilterSnoopReq *req = payload; > + int tfd = (int)data;</tt> <tt>This gives the case of different size error (on 64bit) as I had said in previous patch reviews already.</tt> <tt>Use *(int *)data.</tt> <tt> > + struct iplease *ipl; > + > + /* clean up orphaned, expired leases */ > + if (!req->threadkey) { > + uint32_t now; </tt> <tt>time_t</tt> <tt>> + > + now = time(0); > + for (ipl = req->start; ipl; ipl = ipl->ipl_next) > + if (ipl->ipl_timeout < now) > + ipl_del(req, ipl->ipl_ipaddr , 0); > + if (!req->start) { > + snoopReqFree(req); > + return; > + } > + } > + for (ipl = req->start; ipl; ipl = ipl->ipl_next) > + (void) lease_write(tfd, req->ifkey, ipl); > +} > + > +static void > +lease_refresh(void) > +{ > + int tfd; > + > + (void) unlink(TMPLEASEFILE); > + /* lease file loaded, delete old one */ > + tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644); > + if (tfd < 0) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("open(\"%s\"): %s"), > + TMPLEASEFILE, strerror(errno)); > + return; > + } > + if (SnoopReqs) > + virHashForEach(SnoopReqs, SaveSnoopReqIter, (void *)tfd); > + (void) close(tfd);</tt> <tt>Use VIR_FORCE_CLOSE as already shown in previous reviews.</tt> <tt> > + if (rename(TMPLEASEFILE, LEASEFILE) < 0) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("rename(\"%s\", \"%s\"): %s"), > + TMPLEASEFILE, LEASEFILE, strerror(errno)); > + (void) unlink(TMPLEASEFILE); > + } > + wleases = 0; > + lease_open(); > +} > + > + > +static void > +lease_load(void) > +{ > + char line[256], ifkey[VIR_IFKEY_LEN], ipstr[16], srvstr[16];</tt> <tt>Same comment as above.</tt> <tt> > + struct iplease ipl; > + struct virNWFilterSnoopReq *req; > + time_t now; > + FILE *fp; > + int ln = 0; > + > + fp = fopen(LEASEFILE, "r"); > + time(&now); > + while (fp && fgets(line, sizeof(line), fp)) { > + if (line[strlen(line)-1] != '\n') { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("lease_load lease file line %dcorrupt"), > + ln); > + break; > + } > + ln++; > + /* key len 55 = "VMUUID"+'-'+"MAC" */ > + if (sscanf(line, "%u %55s %16s %16s", &ipl.ipl_timeout, > + ifkey, ipstr, srvstr) < 4) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("lease_load lease file line %dcorrupt"), > + ln); > + break;;</tt> <tt>Remove ';'</tt> <tt> > + } > + if (ipl.ipl_timeout && ipl.ipl_timeout < now) > + continue; > + req = virHashLookup(SnoopReqs, ifkey); > + if (!req) { > + req = newreq(ifkey); > + if (!req) > + break; > + if (virHashAddEntry(SnoopReqs, ifkey, req)) { > + snoopReqFree(req); > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("lease_load req add failed on " > + "interface \"%s\""), ifkey); > + continue; > + } > + } > + > + if (inet_pton(AF_INET, ipstr, &ipl.ipl_ipaddr) <= 0) { > + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, > + _("line %d corrupt ipaddr \"%s\""), > + ln, ipstr); > + continue; > + } > + (void) inet_pton(AF_INET, srvstr, &ipl.ipl_server); > + ipl.ipl_req = req; > + > + if (ipl.ipl_timeout) > + ipl_add(&ipl, 0); > + else > + ipl_del(req, ipl.ipl_ipaddr, 0); > + } > + if (fp != NULL) > + (void) fclose(fp); > + lease_refresh(); > +} > + </tt> <tt> Stefan</tt>