pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895661902
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yes we could add check for the incoming parameter too. The man page is a bit
ambiguous in my opinion about this.
For `setenv()`it makes perfect sense to check for the incoming 'name'
parameter.
For `unsetenv()` it is ambiguous. The check is for 'name' parameter
validity, but no such env variable can exist, as setenv() checks for parameter
validity. As such name does not exist, should the function return success ?
EDIT: There seem to be contradicting man pages on this also, but no page
specifically **prohibits** testing for the incoming parameter 'name' for
validity and returning EINVAL for a bad parameter is allowed by all the
versions I could find, so it is safer/better to do the check!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
