11:58 a.m.
On 05/25/2012 11:59 PM, Eric Blake wrote:
> +<span class="since">Since 0.9.12</span>
s/12/13/ (hmm, sorry for the delays)
No problem.
> +# define virNWFilterSnoopLock() \
> + do { \
> + virMutexLock(&virNWFilterSnoopState.snoopLock); \
> + } while (0);
Ouch. Lose the trailing ';', or you will cause problems with:
if (foo)
virNWFilterSnoopLock();
else
bar;
Fixed this and three more times.
> +static virNWFilterSnoopReqPtr
> +virNWFilterSnoopReqNew(const char *ifkey)
> +{
> + virNWFilterSnoopReqPtr req;
> +
> + if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN - 1) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("virNWFilterSnoopReqNew called with invalid
"
> + "key \"%s\" (%u)"),
> + ifkey ? ifkey : "",
> + (unsigned int)strlen(ifkey));
In libvirt, it's safe and preferrable to use %zu and drop the cast when
dealing with size_t. (Gnulib is awesome.)
Fixed.
> +
> + pup = (struct udphdr *) ((char *) pip + (pip->ihl<< 2));
> + len -= pip->ihl<< 2;
> + if (len< 0)
> + return -2;
> +
> + pd = (virNWFilterSnoopDHCPHdrPtr) ((char *) pup + sizeof(*pup));
Two instances of type punning that you didn't convert. I hope these are
packed types, so that the compiler ensures we don't get any unaligned
accesses on platforms where it matters.
pip->ihl accesses 4 bits of a byte. So that should be ok.
The MAC header is typically (6+6+2) 14 bytes long. The IP header is a
multiple of 4 bytes, typically 20 bytes. UDP is always 8 bytes. So we
have 2 bytes alignment once we get to the DHCP header. Inside the DHCP
header we are accessing the 32bit IPv4 addresses and timeouts using
memcpy now and otherwise there are only byte accesses.
> + req->driver = driver;
> + req->techdriver = techdriver;
> + tmp = virNetDevGetIndex(ifname,&req->ifindex);
> + req->linkdev = linkdev ? strdup(linkdev) : NULL;
> + req->nettype = nettype;
> + req->ifname = strdup(ifname);
> + memcpy(req->macaddr, macaddr, sizeof(req->macaddr));
> + req->filtername = strdup(filtername);
> + req->vars = virNWFilterHashTableCreate(0);
> +
> + if (!req->ifname || !req->filtername || !req->vars || tmp< 0) {
> + virReportOOMError();
You don't detect strdup(linkdev) failure; is that intentional?
Ooops. Fixed.
> + goto exit_snoopreqput;
> + }
> +
> + /* check that all tools are available for applying the filters (late) */
> + if ( !techdriver->canApplyBasicRules()) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("IP parameter must be provided since "
> + "snooping the IP address does not work "
> + "possibly due to missing tools"));
Is this error user-visible? If so, VIR_ERR_INTERNAL_ERROR is probably
the wrong category; maybe VIR_ERR_CONFIG_UNSUPPORTED fits better (the
user is using $IP without configuring it properly).
Yes, it's user-visible. Fixed.
+
+ /* time intf ip dhcpserver */
+ snprintf(lbuf, sizeof(lbuf), "%u %s %s %s\n", ipl->timeout,
+ ifkey, ipstr, dhcpstr);
You're lucky that this doesn't overflow. A virAsprintf avoids the need
to audit whether 256 bytes will always be enough.
> +
> + len = strlen(lbuf);
The return value of snprintf/virAsprintf is the length; its faster to
grab the length on the first pass through the string than to go through
the string twice.
Fixed this and converted to use virAsprintf.
> +
> + if (safewrite(lfd, lbuf, len) != len) {
> + virReportSystemError(errno, "%s", _("lease file write
failed"));
> + ret = -1;
> + goto cleanup;
> + }
> +
> + (void) fsync(lfd);
Is there any problem if you delete the cast to void?
I change this to use ignore_value().
> +static void
> +virNWFilterSnoopSaveIter(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *data)
> +{
> + virNWFilterSnoopReqPtr req = payload;
> + int tfd = *(int *)data;
double spaces
> +/*
> + * Write all valid leases into a temporary file and then
> + * rename the file to the final file.
> + * Call this function with the SnoopLock held.
> + */
> +static void
> +virNWFilterSnoopLeaseFileRefresh(void)
> +{
> + int tfd;
> +
> + (void) unlink(TMPLEASEFILE);
You should report unlink() failure rather than ignore it, unless errno
is ENOENT.
Fixed.
> + /* lease file loaded, delete old one */
> + tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644);
> + if (tfd< 0) {
> + virReportSystemError(errno, _("open(\"%s\")"),
TMPLEASEFILE);
> + return;
> + }
> +
> + if (virNWFilterSnoopState.snoopReqs) {
> + /* clean up the requests */
> + virHashRemoveSet(virNWFilterSnoopState.snoopReqs,
> + virNWFilterSnoopPruneIter, NULL);
> + /* now save them */
> + virHashForEach(virNWFilterSnoopState.snoopReqs,
> + virNWFilterSnoopSaveIter, (void *)&tfd);
> + }
> +
> + VIR_FORCE_CLOSE(tfd);
I'd use VIR_CLOSE(fd) to check for and report errors.
Fixed. Skipping the renaming in case of an error.
+ }
+ ln++;
+ /* key len 55 = "VMUUID"+'-'+"MAC" */
+ if (sscanf(line, "%u %55s %16s %16s",&ipl.timeout,
+ ifkey, ipstr, srvstr)< 4) {
I'm trying to reduce the number of sscanf(%u) in our codebase.
*scanf
has undefined behavior on integer overflow, and if someone were to
intentionally corrupt our lease file (unlikely as that is - if they have
write permissions there they have write permissions to do much worse),
then using virStrToLong_* rather than *scanf will let us detect it. But
as you are not the lone offender, I can save it for a subsequent global
cleanup.
Then I'll cowardly dodge this one now.
> +
> + if (virSocketAddrParseIPv4(&ipl.ipAddress, ipstr)< 0) {
> + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
> + _("line %d corrupt ipaddr
\"%s\""),
> + ln, ipstr);
> + virNWFilterSnoopReqPut(req);
> + continue;
> + }
> + (void) virSocketAddrParseIPv4(&ipl.ipServer, srvstr);
Why are you reporting errors on one address and ignoring the other?
The DHCP server's IP address is (currently) recorded more for debugging
purposes but not as important for this code to work correctly.
I found a few things worth fixing, but I think the overall patch is
pretty sound. I'm okay if you get it into the tree now so it can get
wider exposure, and leave it up to you whether to post a v15 (if so,
post an incremental diff so I don't have to read the entire patch again)
or just push once you've made the fixes.
ACK.
I fixed a lot of the problems you mentioned.
For patch 3/5 I could use the ACK that DV gave on 4/19:
https://www.redhat.com/archives/libvir-list/2012-April/msg01009.html
I hope you'll have time to review the other 2 patches. 5/5 for example
makes it easier to verify what is going on since it adds the convenience
of displaying the current leases in the XML.
Thanks for the review.
I'll wait a bit until I push the patch.
Stefan