pcp
[Top] [All Lists]

Re: [pcp] NSS/NSPR review notes

To: pcp@xxxxxxxxxxx
Subject: Re: [pcp] NSS/NSPR review notes
From: Dave Brolley <brolley@xxxxxxxxxx>
Date: Thu, 30 Aug 2012 14:49:31 -0400
In-reply-to: <21006055.2971170.1346311577682.JavaMail.root@xxxxxxxxxx>
References: <21006055.2971170.1346311577682.JavaMail.root@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0
Hi Nathan,

Wow, thanks for the merge and for the good catches while doing it! I have had a look and don't see anything missing. I look forward to the full test results. Is it difficult to setup/run the test suite? I probably should be able to do it myself as we progress.

Dave

On 08/30/2012 03:26 AM, Nathan Scott wrote:
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>