Re: [PATCH] fdopendir(3)

On Mon, Apr 14, 2008 at 12:30:52PM -0700, Xin LI wrote:
> Kostik Belousov wrote:
> >On Sun, Apr 13, 2008 at 02:47:02PM -0700, Xin LI wrote:
> >>Hi,
> >>
> >>Any objection for the attached patch which implements fdopendir(3) that 
> >>is found in various other OSes?  Basically it splits __opendir2 into two 
> >>parts, and expose the second part which deals with fd to provide 
> >>fdopendir(3) functionalities.
> >
> >There are some problems with the DTF_REWIND and union mounts.
> >
> >I too implemented the fdopendir in the course of the *at() work after
> >the initial Roman Divacky submission. I put my patch at the
> >http://people.freebsd.org/~kib/misc/fdopendir.1.patch
> >
> >I postponed the commit for further testing and some more changes related
> >to the committed *at syscalls (mainly man pages, the patch awaits the
> >review).
> 
> Some observations.  __fdopendir2().  My feeling is that this is not 
> suitable to separate as a standard alone file as its sole users are 
> fdopendir() and opendir(), therefore, being static might be more 
> appropriate.  Do you have the intention to use it in somewhere else?  If 
Being static, it must be present in the same source file with the
callers. Since the static libraries (libXXX.a) try to avoid bringing
in non-used symbols by separating each symbol in the individual source
file, I put the __fdopendir2, opendir and fdopendir into the individual
files. Yes, use of the opendir or fdopendir would bring the __fdopendir2
in, but use of the opendir would not expose fdopendir extra.

> so we should really repocopy opendir.c to __fdopendir2.c (there is a 
> minor unnecessary.
I do intent to repocopy it. This cannot be represented in the patch.

> 
> Another thing is that the fd == -1 && (flags & DTF_REWIND) statement. 
> If this would be an internal routine then it sounds like to be better 
> represented as an assertion.  The caller should guarantee that the 
> assertion hold true (by design), and the runtime check seems to be 
> unnecessary (that's why I did not added these checks).
Agree.

> 
> So I think the major difference between your version and mine is whether 
> we wanted to expose __fdopendir2() outside libc?
Mine __fdopendir2() is also not exposed for the libc users, since it is
not present in the public symbol map.
nm DEV/src/lib/libc/libc.so.7 | grep __fdopendir2 
000c1160 t __fdopendir2
As you see, the symbol is local.