On Sat, Jun 21, 2008 at 4:56 PM, James Carlson <james.d.carlson at sun.com> wrote: > Mike Gerdts writes: >> I would like to get some eyes on my proposed fix for 6717067. >> >> http://cr.opensolaris.org/~mgerdts/6717067/ > > 217: Too bad X11 has 'bogus share' disease ... I think I would have > used /usr/X11/man here instead (as that's really the proper path > for a man page), but it probably doesn't matter. (By "bogus > share," I'm referring to the fact that /usr/share is a special > mount location defined in filesystem(5) ... putting your own > directory under there is reasonable, but putting "share" into > some far-flung path makes no sense at all. Nobody's going to > share a bunch of random directories. In short, "/usr/share/foo/" > is right, and "/usr/foo/share" is wrong.)
I agree, mostly. I chose /usr/X11/share/man because that is a directory, whereas /usr/X11/man is a symlink. They both get you to the same place, but /usr/X11/share/man seemed to be chosen as the "right" place. To be honest, I am just thrilled that the "right" place for X executables is not /usr/bin/X11/*. > 3186: the logic appears to be backwards here. The original code used > "!=0" for the stat() check -- it was returning NULL when the > directory being stat'd didn't exist. This new code is returning > NULL when the directory _does_ exist, and non-NULL when it > doesn't. > > The code should be simply: > > if (stat(mand, &sb) == 0 && S_ISDIR(sb.st_mode)) > return (mand); > > free(mand); > return (NULL); Oops. Fixed. > Other than that, it looks like this file doesn't (or shouldn't) pass > C-style, as it has non-compliant function definitions. Oh, well. > Don't bother fixing that if you don't feel up to it. "cstyle -pP man.c" doesn't complain. A read of http://opensolaris.org/os/community/documentation/getting_started_docs/cstyle.ms.pdf suggests that this may be what you are referring to: static void freev(char **v) { should be: static void freev(char **v) { I've fixed those. Is there something else I should be looking for? FWIW, there are also a bunch of continuation line warnings (cstyle -c). Since Roland has indicated that he will be suggesting a project to tackle some rather significant work with the man subsystem, I will leave this for now. I have updated the webrev at http://cr.opensolaris.org/~mgerdts/6717067/ Thanks for the review! Mike -- Mike Gerdts http://mgerdts.blogspot.com/
