Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Libc.File constructor that takes a RawFD #10535

Merged
merged 5 commits into from Mar 21, 2015
Merged

Add a Libc.File constructor that takes a RawFD #10535

merged 5 commits into from Mar 21, 2015

Conversation

Keno
Copy link
Member

@Keno Keno commented Mar 16, 2015

Twice this week, I've came across instances where C libraries wanted
a FILE*, but I only had an FD. Since we have Libc.File, we might as
well have this method too.

[Opening PR to have Appveyor look this over]

Twice this week, I've came across instances where C libraries wanted
a FILE*, but I only had an FD. Since we have Libc.File, we might as
well have this method too.
@IainNZ
Copy link
Member

IainNZ commented Mar 16, 2015

Hah @mlubin this is very related what I was asking about the other day.

Could be good to mention how to deal with FILE* in the C part of the manual.

@tkelman
Copy link
Contributor

tkelman commented Mar 17, 2015

This should really be considered a bug in these libraries... that's a horribly non-portable thing to do.

@Keno
Copy link
Member Author

Keno commented Mar 17, 2015

While working on this I am wondering if the conversion to LibC.FILE steal the file descriptor from the IOStream. The problem I'm thinking off is that it's not clear who should close the stream. If we add a close method to LibC.FILE, then we probably should check it's return value and throw an error. Now, it so happens that the IOStream doesn't really care if the FD is valid when closing, so we could just say "If you create a CFILE from an IOStream, always close the CFILE first", but that doesn't seem great, especially considering the IOStream may be closed by GC. Thoughts? cc @JeffBezanson

@JeffBezanson
Copy link
Sponsor Member

Stuff like this is always pretty ugly. I'm not sure there is a good solution. Perhaps we should not have the method that constructs a FILE from an arbitrary IO.

@Keno
Copy link
Member Author

Keno commented Mar 17, 2015

I'd be fine with keeping convert(::Type{Libc.File}, s::IOStream), but I'm leaning towards you not being allowed to use the IOStream afterwards. If you want other behavior, you can always use Libc.FILE(fd(s), LibC.modestr(s)) (though then we should probably export modestr).

@JeffBezanson and I decided that was the best way to go about handling the issue that
fdopen wants to take ownership of the file descriptor. In this way the two objects
are independent and everything should work in the general case. Also add a close method
to Libc.FILE, which can now always be safely called (assuming of course that it isn't
closed by the C function it gets passed to).

Finally, move RawFD and dup to Libc, since they're part of the C library anyway.
Keno added a commit that referenced this pull request Mar 21, 2015
Add a Libc.File constructor that takes a RawFD
@Keno Keno merged commit 1d84f9f into master Mar 21, 2015
@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 22, 2015

This should really be considered a bug in these libraries... that's a horribly non-portable thing to do.

this does seem to have the same issue as malloc/free: you don't know if your library is linked against the same libc as julia, so it seems like this usage may sometimes corrupt the program.

(edit: although, on windows, the same issue also exists for file descriptors since the canonical object is a HANDLE)

@tkelman tkelman deleted the kf/cfilefd branch April 19, 2015 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants