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
Minor make adjustments, fix warnings #199
Conversation
LOG(FATAL) << "Need to set enviroment variable AWS_ACCESS_KEY_ID to use S3"; | ||
} | ||
if (seckey == NULL) { | ||
if (seckey == NULL || !*seckey) { |
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.
@mli is this correct?
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.
will getenv return '\0'?
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.
My implementation returns NULL (Ubuntu). However, the documentation is not specific on this and states that it returns NULL if the environment variable is not found. However, I can put an empty variable in the environment:
[coolivie@DEV:]export FOO=]env | grep FOO
[coolivie@DEV:
FOO=
[coolivie@DEV:~]
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.
By googling that, I see cases of some implementations returning an empty string.
Also, this example does not appear to check:
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/stdlib/getenv.c?rev=1.19&content-type=text/x-cvsweb-markup
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.
Note that this Apple Open Source putenv and setenv both will insert and retrieve empty values:
(I compiled and tried this code and does, in fact, return an empty string)
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.
ok I'll merge this since it looks like it wouldn't hurt.
No description provided.