DAOS-18527 cart: Handling of string env limits#17466
Conversation
- CRT_ENV_STR_MAX_SIZE introduced, set to 1024. All string envs are checked against this limit at the data_init() time. - Additional checks added for domain/interface not to exceed this limit, as those can be passed via crt_init_opt. Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>
|
Ticket title is 'CART: cart limits interface/domain strings to 64 characters, resulting in errors' |
|
Test stage Functional Hardware Medium Verbs Provider MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-17466/1/execution/node/1309/log |
Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>
| * env checks limit strings to CRT_ENV_STR_MAX_SIZE, but a domain can | ||
| * be passed as an init argument | ||
| */ | ||
| if (strlen(na_cfg->noc_domain) + 1 >= CRT_ENV_STR_MAX_SIZE) { |
There was a problem hiding this comment.
you don't need parentheses here ? I'm surprised the compiler does not complain if it does not.
There was a problem hiding this comment.
why ? math operations take precedence over comparisons
|
Test stage Functional Hardware Medium Verbs Provider MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-17466/4/execution/node/1359/log |
|
Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-17466/6/display/redirect |
1 similar comment
|
Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-17466/6/display/redirect |
|
|
||
| if (interface) { | ||
| D_STRNDUP(na_cfg->noc_interface, interface, 64); | ||
| D_STRNDUP(na_cfg->noc_interface, interface, CRT_ENV_STR_MAX_SIZE); |
There was a problem hiding this comment.
should we try to make this more robust?
this is not necessarily an env variable, and sometimes it can contain multiple interfaces like the issue Johann had, so like CRT_ENV_STR_MAX_SIZE * number of interfaces (seprated be , or : forgot what it is here)
There was a problem hiding this comment.
im not opposed to this as is.. i think 1k is probably enough to cover 60 interfaces/domains or more :-)
There was a problem hiding this comment.
I am mixed on this. On one hand, i agree that having individual limits would be better, but then we would have to do similar for other strings as well such as domains, provider and ports (for multiprovider case) and possibly others (like auth keys).
If we change this to be CRT_ENV_STR_MAX_SIZE * num_interfaces here, then we would have to allow the similar sized string to be passed via D_INTERFACE environment variable as well. This would require changing each cart env declaration to specify an individual limit.
In addition, we dont know the number of interfaces ahead of time, and we only access envs once they are all copied out by cart (at init time), so even at best it would need to be some static amount such as CRT_ENV_STR_MAX_SIZE * max_num_interfaces for D_INTERFACE and CRT_ENV_STR_MAX_SIZE * max_num_providers for D_PROVIDER, etc..
With all that i feel that having a single max value big enough for all input strings is much cleaner for now.
5 participants
CRT_ENV_STR_MAX_SIZE introduced, set to 1024. All string envs are checked against this limit at the data_init() time.
Additional checks added for domain/interface not to exceed this limit, as those can be passed via crt_init_opt.
Steps for the author:
After all prior steps are complete: