Add os:getenv/1 #1805

mat-hek · 2025-08-20T09:53:30Z

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

pguyot · 2025-08-20T20:08:11Z

POSIX says that getenv is not thread safe. You should hold global->env_spinlock. See nif_erlang_localtime.

mat-hek · 2025-08-21T14:14:42Z

Done ✅

src/libAtomVM/nifs.c

pguyot · 2025-08-22T20:38:22Z

src/libAtomVM/nifs.c

+
+    if (IS_NULL_PTR(env_var_value_tmp)) {
+        result = FALSE_ATOM;
+        goto nif_os_getenv_1_unlock_return;


I think we'd rather duplicate the spin unlock and have a return than a goto. At least it's the most common pattern here. See AVMCCS-L011 in c coding style guide.

pguyot · 2025-08-22T20:39:47Z

src/libAtomVM/nifs.c

+        goto nif_os_getenv_1_unlock_return;
+    }
+
+    strcpy(env_var_value, env_var_value_tmp);


This looks like strdup(3) to me, even if you use the strlen later on. You could just strdup, release the spinlock and then test for memory error. If you want to strlen first, you can strndup with len + 1.

pguyot · 2025-08-22T20:45:04Z

tests/libs/estdlib/test_os.erl

+    true =
+        case atomvm:platform() of
+            generic_unix ->
+                is_list(os:getenv("PATH"));


You could test that it starts with / or contains / which probably is always true.

pguyot · 2025-08-22T20:54:06Z

src/libAtomVM/nifs.c

+    int ok;
+    const char *env_var = interop_list_to_utf8_string(env_var_list, &ok);
+    if (UNLIKELY(!ok)) {
+        RAISE_ERROR(OUT_OF_MEMORY_ATOM);


We usually raise badarg here instead of out of memory even if both reasons would yield 0.

pguyot · 2025-08-22T20:54:23Z

src/libAtomVM/nifs.c

+    VALIDATE_VALUE(env_var_list, term_is_list);
+
+    int ok;
+    const char *env_var = interop_list_to_utf8_string(env_var_list, &ok);


I'm not sure env_var is freed.

I'm not sure env_var is freed.

Where is it freed?

I've mistaken it with env_var_value, which also wasn't always freed 🫠 Now both should be fixed

pguyot · 2025-08-22T20:55:36Z

tests/libs/estdlib/test_os.erl

+            _ ->
+                true
+        end,
+    false = os:getenv("NON_EXISTENT_PATH_VAR"),


Since you do utf8, you could try a codepoint > 255.

Also what happens if string is empty or improper?

pguyot · 2025-08-22T20:57:14Z

libs/estdlib/src/os.erl

+%% @doc     Get an environment variable value if defined
+%% @end
+%%-----------------------------------------------------------------------------
+-spec getenv(Name :: string()) -> string() | false.


OTP uses env_var_name() and env_var_value() that are nonempty_string().

pguyot · 2025-08-25T10:53:45Z

tests/libs/estdlib/test_os.erl

+
+test_os_getenv() ->
+    ok =
+        case atomvm:platform() of


This test is executed by BEAM to assert compatibility, you need erlang:system_info(machine).

AFAIR this is to check if we can expect the PATH env variable to be there. Not sure how erlang:system_info(machine) would help, since it seems to always return the string "ATOM" 🤔

You need to check erlang:system_info(machine) returns "ATOM" before calling atomvm:platform(). On the BEAM: "BEAM" and os:type() can be used.
Anyway tests must be runnable on the BEAM, so this code must be changed.

bettio

Fairly good, only minor changes are required.

bettio · 2025-09-05T23:36:43Z

tests/libs/estdlib/test_os.erl

+
+test_os_getenv() ->
+    ok =
+        case atomvm:platform() of


You need to check erlang:system_info(machine) returns "ATOM" before calling atomvm:platform(). On the BEAM: "BEAM" and os:type() can be used.
Anyway tests must be runnable on the BEAM, so this code must be changed.

bettio · 2025-09-05T23:45:56Z

src/libAtomVM/nifs.c

+    smp_spinlock_lock(&ctx->global->env_spinlock);
+#endif
+
+    char *env_var_value = getenv(env_var);


I suggest using const char *.

bettio · 2025-09-05T23:46:32Z

src/libAtomVM/nifs.c

+        return FALSE_ATOM;
+    }
+
+    env_var_value = strdup(env_var_value);


I suggest doing: char *env_var_value_copy = strdup(env_var_value); instead of reassigning.

bettio

Just a few minor notes.
By the way: UNLIKELY(IS_NULL_PTR(x)) is redundant and can be simplified to IS_NULL_PTR(x)

bettio · 2025-09-11T20:56:30Z

src/libAtomVM/nifs.c

+    smp_spinlock_unlock(&ctx->global->env_spinlock);
+#endif
+
+    if (UNLIKELY(IS_NULL_PTR(env_var_value))) {


One last thing: UNLIKELY(IS_NULL_PTR(x)) is redundant: IS_NULL_PTR is a shorthand for UNLIKELY(x == 0).

bettio · 2025-09-11T21:00:34Z

src/libAtomVM/nifs.c

@@ -229,657 +235,549 @@ DECLARE_MATH_NIF_FUN(sqrt)
 DECLARE_MATH_NIF_FUN(tan)
 DECLARE_MATH_NIF_FUN(tanh)

-static const struct Nif binary_at_nif =
-{
+static const struct Nif binary_at_nif = {


I would avoid to merge this with all this unrelated noise, still I understand the struggle. So I can start aligning nifs.c more to our .clang-format. git rebase should handle this for you.

Sorry, it was unintentional, VS Code doesn't respect clang-ignore for some reason. Yet formatting nifs.c (and opcodesswitch.h :P) would be much appreciated ;)

bettio

Everything looks good now. Let's rebase and squash commits together, after that I will merge it.

Signed-off-by: Mateusz Front <[email protected]>

pguyot requested changes Aug 21, 2025

View reviewed changes

src/libAtomVM/nifs.c Show resolved Hide resolved

mat-hek requested a review from pguyot August 22, 2025 13:19

mat-hek force-pushed the mf/os-getenv branch from 48f5373 to d5ad0ec Compare August 22, 2025 13:20

pguyot requested changes Aug 22, 2025

View reviewed changes

mat-hek force-pushed the mf/os-getenv branch 3 times, most recently from 06be48c to 7e201ca Compare August 25, 2025 09:37

mat-hek requested a review from pguyot August 25, 2025 09:39

mat-hek force-pushed the mf/os-getenv branch from 7e201ca to 94a0300 Compare August 25, 2025 10:27

pguyot reviewed Aug 25, 2025

View reviewed changes

mat-hek force-pushed the mf/os-getenv branch from 42b5faa to 33379a7 Compare August 25, 2025 12:25

mat-hek requested a review from pguyot August 25, 2025 12:25

bettio requested changes Sep 5, 2025

View reviewed changes

mat-hek force-pushed the mf/os-getenv branch 3 times, most recently from 1930be4 to 5d6238f Compare September 11, 2025 16:24

mat-hek requested a review from bettio September 11, 2025 18:51

bettio requested changes Sep 11, 2025

View reviewed changes

mat-hek force-pushed the mf/os-getenv branch from f784360 to a104b8e Compare September 12, 2025 08:04

bettio approved these changes Sep 13, 2025

View reviewed changes

bettio requested changes Sep 13, 2025

View reviewed changes

mat-hek force-pushed the mf/os-getenv branch from a104b8e to 3894ec8 Compare September 15, 2025 08:25

Add os:getenv

0bf2417

Signed-off-by: Mateusz Front <[email protected]>

mat-hek force-pushed the mf/os-getenv branch from 3894ec8 to 0bf2417 Compare September 15, 2025 08:26

mat-hek requested a review from bettio September 15, 2025 11:35

Add os:getenv/1 #1805

Are you sure you want to change the base?

Add os:getenv/1 #1805

Uh oh!

Conversation

mat-hek commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pguyot commented Aug 20, 2025

Uh oh!

mat-hek commented Aug 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bettio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bettio Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bettio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bettio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mat-hek commented Aug 20, 2025 •

edited

Loading

bettio Sep 5, 2025 •

edited

Loading