pcp
[Top] [All Lists]

Re: [pcp] NSS/NSPR review notes

To: Dave Brolley <brolley@xxxxxxxxxx>
Subject: Re: [pcp] NSS/NSPR review notes
From: Nathan Scott <nathans@xxxxxxxxxx>
Date: Thu, 30 Aug 2012 03:26:17 -0400 (EDT)
Cc: pcp@xxxxxxxxxxx
In-reply-to: <289008570.2441917.1346217319343.JavaMail.root@xxxxxxxxxx>
Reply-to: Nathan Scott <nathans@xxxxxxxxxx>
Hi Dave,

----- Original Message -----
> ----- Original Message -----
> > On 08/23/2012 11:39 PM, Nathan Scott wrote:
> > > ...
> *nod* - good plan re doing that testing.  Since there's a bit of
> rework required around the ipc table, I was thinking of pulling
> out just the api-wrapper type changes (so, a no-op change) and
> testing/merging those (by hand, probably, not git merge on the
> nss branch, which is starting to look a bit like its done a few
> rounds with Mike Tyson).

OK, wow, what a marathon.  Glad we went this way though, a few more
lurking issues did fall out...

__pmFcntlGetFlags() has accidentally replaced two different fcntl()
calls with just the one.  Subtle, but F_GETFL != F_GETFD (the final
letter differs - FL is file status flags, FD file descriptor flags).
Huh, I've never noticed that - but they definitely map to different
codes in fcntl.h.  I've created new separate interfaces to help to
clarify this.  Likewise for the SetFlags() variant.

__pmAuxConnectPMCDPort() had an error path (__pmCreateSocket fail)
that introduced a "HostEntBuffer" memleak.  Likewise in the perl
wrapper module (local_reconnector, I think).

Naming convention for fields in impl.h structures appears to be all
lower case with underscores ... made a note there re __pmFdSet.

Found memleaks on all of the calls to __pmSockAddrInToString.


I also renamed the "mysocklen_t" (old school) type to __pmSockLen
which matches the new type names better.

Finally, I left a note in the (now empty, ready for you) section
for NSS code - I think it'll be better to go with fewer #ifdefs;
so instead of...

__pmFunction()
{
#ifdef NSS
   ...
#else 
   ...
#endif
   ... (usually 1 or 0 lines here) ...
}

to have two totally distinct implementations (so, one #ifdef only,
not one in each function).

Running it through the QA tests now.  Hmm - was looking good while I
was writing this, but then 062 just failed (__pmAccAddClient related?
"host 29: __pmAccAddClient returns denyOpsResult 0x0 (expected 0xdfffffff)"
style messages).  Will finish off the run and get the full results &
a merge tree up on oss, and try catch you in the morning to discuss.

cheers.

--
Nathan

<Prev in Thread] Current Thread [Next in Thread>