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

fix mktemp linker warnings when using GNU linker #132

Merged
merged 3 commits into from
Dec 27, 2019

Conversation

stephen-sorley
Copy link
Contributor

Use mkdtemp() to create a temporary directory with a unique name,
then add a constant filename to the path after that. This allows
us to construct a unique path without any race conditions, and
without having to use mktemp().

Hopefully doing it this way avoids the issues you were having
with getting mkstemp() to work in #43.

@coveralls
Copy link

coveralls commented Dec 5, 2019

Coverage Status

Coverage decreased (-0.009%) to 83.31% when pulling c366d04 on stephen-sorley:fix-mktemp into 809af56 on tbeu:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 82.837% when pulling e0749d2 on stephen-sorley:fix-mktemp into d255262 on tbeu:master.

Copy link
Owner

@tbeu tbeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR which finally might solve the annoying warning and all the previous attempts to get rid of it. I added some comments to your changes and will merge after treatment.

src/mat.c Outdated Show resolved Hide resolved
src/mat.c Outdated Show resolved Hide resolved
src/mat.c Outdated Show resolved Hide resolved
src/mat.c Outdated Show resolved Hide resolved
@tbeu
Copy link
Owner

tbeu commented Dec 8, 2019

@svillemot The changes look very good to me now. Before merging I'd like to get your feedback. Do you see any issues with mkdtemp on Debian platforms? Should we enable mkdtemp only for POSIX systems? Thanks a lot.

@svillemot
Copy link
Contributor

No problem for using mkdtemp() on Debian. But you may indeed have to disable it for non-POSIX systems (for example, I don’t think it is available under MinGW).

@stephen-sorley
Copy link
Contributor Author

@tbeu Any further changes that you need me to make to this?

What platforms do you support besides Windows/Linux/macOS? Apart from Windows, looks like AIX is the main one that's missing mkdtemp:
http://lists.llvm.org/pipermail/llvm-dev/2010-August/034081.html

Solaris provides it, however:
https://docs.oracle.com/cd/E36784_01/html/E36874/mkdtemp-3c.html

@tbeu
Copy link
Owner

tbeu commented Dec 21, 2019

@tbeu Any further changes that you need me to make to this?

No, it all looks fine. I only was reluctant to merge since @svillemot mentioned MinGW. I need to give it one more try after I observed no issues when I tested it locally last time.

Use mkdtemp() to create a temporary directory with a unique name,
then add a constant filename to the path after that. This allows
us to construct a unique path without any race conditions, and
without having to use mktemp().
(1) Now creates the temporary directory in /tmp on Linux, instead of in
    the current working directory.

(2) Will now delete the temporary directory as well before the function
    returns (was only deleting the temporary file before).

(3) Now uses a buffer on the stack instead of malloc() when constructing
    the paths, to avoid memory leaks due to Mat_Critical() never returning
    to caller, and to optimize for better performance.
@tbeu
Copy link
Owner

tbeu commented Dec 26, 2019

I only was reluctant to merge since @svillemot mentioned MinGW. I need to give it one more try after I observed no issues when I tested it locally last time.

I rebased (and force-pushed) your branch to confirm that it builds with MinGW.

What platforms do you support besides Windows/Linux/macOS? Apart from Windows, looks like AIX is the main one that's missing mkdtemp: http://lists.llvm.org/pipermail/llvm-dev/2010-August/034081.html

I have no chance to build on AIX. I believe we should exclude it from the two readme files, where AIX is mentioned as supported platform. OK for you?

@stephen-sorley
Copy link
Contributor Author

Yeah, excluding AIX sounds good to me. I don't have any way to test on it, either.

@tbeu tbeu merged commit 1f6ff27 into tbeu:master Dec 27, 2019
tbeu added a commit that referenced this pull request Dec 27, 2019
As discussed in #132

[ci skip]
@tbeu
Copy link
Owner

tbeu commented Dec 27, 2019

Thanks for the pull request.

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.

4 participants