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

Implement fullGetEnv function #1503

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Implement fullGetEnv function #1503

merged 2 commits into from
Jun 9, 2023

Conversation

michalbiesek
Copy link
Contributor

  • as a wrapper to call g_fn.getenv and a manual parsing of environ
  • when scoping bash calling getenv("SCOPE_CONF_PATH") will return NULL

Ref: #401

Fixes: #1502


// if g_fn.getenv was not yet defined try to parse environ
size_t l = scope_strchrnul(name, '=') - name;
if (l && !name[l] && environ)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we please use braces here for the if and for statements here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -265,7 +265,7 @@ cfgPathSearch(const char* cfgname)
char *
cfgPath(void)
{
const char* envPath = getenv("SCOPE_CONF_PATH");
const char* envPath = fullGetEnv("SCOPE_CONF_PATH");
Copy link
Contributor

@iapaddler iapaddler Jun 2, 2023

Choose a reason for hiding this comment

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

if we are using a fullGetEnv() then checkEnv() in utils.c should be using that, it seems?
there are numerous instances where getenv() is called directly. do those need to change?
checkEnv() is called in the constructor before initFn() is called to build the g_fn object.

for verification: ld.so must be resolving getenv() to bash:getenv() else fullSetEnv() would not be required for cfgPath(). accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My focus was to support the init problem - but You are right regarding above especially the part initFn and checkEnv - let's discuss it.

if (!scope_strncmp(name, *e, l) && l[*e] == '=')
return *e + l+1;
return NULL;
}
Copy link
Contributor

@iapaddler iapaddler Jun 2, 2023

Choose a reason for hiding this comment

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

assuming that the manual check of environ works, i haven't looked or tested yet, why use g_fn.getenv? is it needed?

on the other side of the same question; there is only place where a manual check of environ could be needed. let's not use it unless it's absolutely needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intentions are following:

  • I want to use g_fn.getenv() as first priority since this exact implementation of getenv function used by the application.
  • Manual check of environment is the implementation taken from our internal library. I treated this as last resort in case we did not able to resolve the g_fn.getenv function.

@michalbiesek
Copy link
Contributor Author

My init intentions after the discussion:

  • provide comment about the source of implementaiton fallback implementaiton of fullGetEnv
  • use fullGetEnv in every valid place in our code
  • get rid off g_fn.getenv and depend only on environ parsing directly - the fallback implementaiton

@michalbiesek michalbiesek force-pushed the fix-1502-env branch 2 times, most recently from bb20c7a to 0960c65 Compare June 5, 2023 07:49
- as a wrapper to call `g_fn.getenv` and a manual parsing
  of environ
- when scoping bash calling `getenv("SCOPE_CONF_PATH")`
  will return NULL

Ref: #401

Fixes: #1502
- use only environ variant
- replace getenv calls with fullGetEnv
@seanvaleo
Copy link
Collaborator

LGTM

@seanvaleo seanvaleo merged commit a8426bf into release/1.4 Jun 9, 2023
@michalbiesek michalbiesek deleted the fix-1502-env branch June 12, 2023 05:35
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.

Scoped bash does not appear to receive the correct configuration
3 participants