changed addReccasterEnvVars to linked list implementation#117
changed addReccasterEnvVars to linked list implementation#117madelinespark wants to merge 6 commits intoChannelFinder:masterfrom
Conversation
|
| string_list_t *new_list = malloc(sizeof(string_list_t)); | ||
| if (new_list == NULL) { | ||
| errlogSevPrintf(errlogMajor, "Error in addReccasterEnvVars - malloc error for creating linked list node"); | ||
| break; | ||
| } | ||
| new_list->item_str = strdup(argv[i]); |
There was a problem hiding this comment.
fyi. These two allocations could be combined into one. For example: https://github.com/epics-base/epics-base/blob/e4fbd83c93a68a1d9cb863ff341735388d246347/modules/libcom/src/iocsh/afterIocRunning.c#L62-L65
const size_t cmd_len = strlen(cmd) + 1;
struct cmditem *const item = mallocMustSucceed(sizeof(struct cmditem) + cmd_len, "afterIocRunning");
item->cmd = (char *)(item + 1);
memcpy(item->cmd, cmd, cmd_len);Also, iteration could be done in a more compact fashion: https://github.com/epics-base/epics-base/blob/e4fbd83c93a68a1d9cb863ff341735388d246347/modules/libcom/src/iocsh/initHooks.c#L64-L65
for(cur = ellFirst(&functionList); cur; cur = ellNext(cur)) {
const initHookLink *fn = CONTAINER(cur, initHookLink, node);
| epicsMutexUnlock(self->lock); | ||
|
|
||
| epicsMutexMustLock(self->lock); |
There was a problem hiding this comment.
Do we need this lock/unlock?
client/castApp/src/dbcb.c
Outdated
| for (i = 0; !ret && i < caster->extra_envs.count; i++) { | ||
| string_list_t *envptr = (string_list_t *)ellNth(&caster->extra_envs, i); | ||
| const char *val = getenv(envptr->item_str); | ||
| if (val && val[0] != '\0') | ||
| ret = casterSendInfo(caster, 0, caster->extra_envs[i], val); | ||
| ret = casterSendInfo(caster, 0, envptr->item_str, val); | ||
| if (ret) | ||
| casterMsg(caster, "Error sending env %s", caster->extra_envs[i]); | ||
| casterMsg(caster, "Error sending env %s", envptr->item_str); |
There was a problem hiding this comment.
I think this iteration can more cleanly be done over the list as opposed to over the count and selecting the N-th element; this makes it an N^2 iteration over the list instead of a one-time pass.
| const char *val = getenv(envptr->item_str); | ||
| ELLNODE *env = ellFirst(&caster->extra_envs); | ||
| while (!ret && env != NULL) { | ||
| string_list_t *temp = (string_list_t *)env; |
There was a problem hiding this comment.
I don't love the variable name temp; envptr was better, and penvvar or somethign would be better yet (and more consistent; see how ellFirst/ellNext is used in EPICS base)
There was a problem hiding this comment.
string_list_t *envptr = CONTAINER(env, string_list_t, node);CONTAINER comes from dbDefs.h, and is intended for these structure "base" to "derived" casts.
There was a problem hiding this comment.
... Corrected missing first argument
| } | ||
| if(found_dup) { | ||
| continue; | ||
| for(i=0; i < argc; i++) { |
There was a problem hiding this comment.
Lots of duplication in this loop and in the one for exclude patterns, just a thought...
| string_list_t *envptr = (string_list_t *)ellNth(&caster->extra_envs, i); | ||
| const char *val = getenv(envptr->item_str); | ||
| ELLNODE *env = ellFirst(&caster->extra_envs); | ||
| while (!ret && env != NULL) { |
There was a problem hiding this comment.
I see that this change has addressed my previous comment about the loop iteration. Consider squashing these two commits?
|
completed via #120 |
4 participants
NOTE: #112 needs to be merged first