----- Original Message -----
> Code changes for this review have been pushed to the brolley/dev branch
> of the pcpfans repository. qa testing is underway.
Good stuff.
> > - API change - OK, cos its an internal.h routine only?
> Hope that's ok.
*nod*, fine by me.
> My understanding of SO_LINGER doesn't make it more or less applicable to
> a unix domain socket as opposed to a tcp one. As a compromise, I changed
> the code to set the SO_LINGER option first allowing your suggested
> short-circuit of the rest of the function.
Either way seems fine, if we need it we need it.
> Not according to my reading of the man page. It says that at most 'size'
Ah, but you didn't check this secret "man" page: ;)
http://msdn.microsoft.com/en-us/library/2ts7cx93%28v=vs.110%29.aspx
(which is the snprintf we end up in on Win32)
> > - localSocket{Path,Fd} don't need to be conditional AFAICT (?)
> > (simplifies other code later - less #ifdef sprawl, more readable).
> OK. I didn't know what the policy was for unused code, in the sense that
> these statics will never be modified and the tests involving them will
> always go the same way on platforms on which unix domain sockets are not
> supported. Some projects might prefer that these objects and the
> associated code not exist in those builds. I actually prefer it the way
> you want, so consider this done.
When in doubt I tend to apply the kernel rule of thumb, where fewer ifdefs
is usually best.
> making it now initialized at all call sites. I've added a
> (non-Dave-style) comment to this effect ...
Hah!
> > - second diagnostic should probably also report gid/egid given
> > expected filesystem permissions on PCP_RUN_DIR?
> Actually the second diagnostics was something I added in order to help
> me debug the problem of not being unable to unlink the socket path in
> the first place. I had intended it to be temporary. If you think it
> would be useful, then yes, the additional info should also be reported.
Your call, I could go either way. Kenj (aka Mr Diagnostics ;) will be
suggesting you keeping it I suspect - since you needed it once, likely
someone else will need it again someday.
> Well, I had them prefixed with '?' which makes them optional, but that's
> a non-issue now.
Ah, didn't know that - thanks for the tip.
cheers.
--
Nathan
|