-
Notifications
You must be signed in to change notification settings - Fork 125
Add file:get_cwd/0 #1806
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 file:get_cwd/0 #1806
Conversation
19e7b57
to
e2ae3ca
Compare
AFAIK, |
@pguyot indeed it's not, but we need it because it's called by the OTP internals, and not only by the file server |
Which module calls it? I searched OTP source code (for |
@pguyot well, now I can't find it either... changed to |
src/libAtomVM/nifs.c
Outdated
} | ||
term result_tuple = term_alloc_tuple(2, &ctx->heap); | ||
term reason = UNDEFINED_ATOM; | ||
switch (errno) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POSIX error code are already standard in OTP, so we don't have to invent new codes:
https://www.erlang.org/doc/apps/kernel/file.html#t:posix/0
Also we have a functions that handles them, that is posix_errno_to_term
.
src/libAtomVM/nifs.c
Outdated
UNUSED(argv) | ||
|
||
char cwd[PATH_MAX]; | ||
if (IS_NULL_PTR(getcwd(cwd, PATH_MAX))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PATH_MAX
is not defined on STM32 and RPi Pico, also more broadly getcwd()
is POSIX but not standard C, so I don't expect it to be available on all platforms.
We might use some kind of HAVE_CWD
define to check if a platform has it or not, as we are already doing with open()
and other POSIX functions.
Question: should we move this to posix_nifs.c
? CC @pguyot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, if we test for HAVE_GETCWD because it's posix and not C, I'd say it makes sense to move it to posix_nifs.c
.
src/libAtomVM/nifs.c
Outdated
UNUSED(argc) | ||
UNUSED(argv) | ||
|
||
char cwd[PATH_MAX]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PATH_MAX
might be quite big, I'm not sure this is safe for stack allocation.
src/libAtomVM/nifs.c
Outdated
#include <stdio.h> | ||
#include <string.h> | ||
#include <time.h> | ||
#include <unistd.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mention later, I'm worried we are leaking unistd and POSIX stuff into platform agnostic nifs.c.
true = is_valid_path(Path), | ||
ok. | ||
|
||
is_valid_path(<<"/", _/binary>>) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll regret this the day we are going to test this on a platform that is not unix ;)
Let's check platform.
7692b6d
to
980951e
Compare
% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later | ||
% | ||
|
||
-module(test_file_get_cwd). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have just a ŧest_file
module as we are doing with other modules in our tests.
In order to get this done in a pain-free manner I suggest merging #1807 first, then we can rebase this, add relevant tests to test_file
and squash everything.
53f1a4b
to
ea6d34e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything good, let's squash everything into a single commit.
898b61c
to
d580122
Compare
@bettio done. BTW, can't you do squash and merge on github? Would save us one step ;) |
src/libAtomVM/nifs.c
Outdated
|
||
static const struct Nif make_ref_nif = { | ||
static const struct Nif make_ref_nif = | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice this. Let's rewrite this as:
static const struct Nif make_ref_nif = {
So clang-format check will pass.
define_if_function_exists(libAtomVM unlink "unistd.h" PRIVATE HAVE_UNLINK) | ||
define_if_function_exists(libAtomVM execve "unistd.h" PRIVATE HAVE_EXECVE) | ||
define_if_function_exists(libAtomVM closefrom "unistd.h" PRIVATE HAVE_CLOSEFROM) | ||
define_if_function_exists(libAtomVM getcwd "unistd.h" PUBLIC HAVE_GETCWD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using define_if_symbol_exists
for PATH_MAX
: on Pico it is failing with the following error:
/home/runner/work/AtomVM/AtomVM/src/libAtomVM/posix_nifs.c: In function 'nif_file_get_cwd_0':
/home/runner/work/AtomVM/AtomVM/src/libAtomVM/posix_nifs.c:953:24: error: 'PATH_MAX' undeclared (first use in this function)
953 | char *cwd = malloc(PATH_MAX);
| ^~~~~~~~
So we need to define HAVE_PATH_MAX and we'll have the following:
#if defined(HAVE_GETCWD) && defined(HAVE_PATH_MAX)
static term nif_file_get_cwd_0(Context *ctx, int argc, term argv[])
9311e6e
to
1075f7b
Compare
These changes are made under both the "Apache 2.0" and the "GNU Lesser General Public License 2.1 or later" license terms (dual license). SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later Added nif implementation for getcwd. Signed-off-by: Mateusz Front <[email protected]>
1075f7b
to
8c2d206
Compare
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later