DAOS-6979 csum: Fixes detected by NLT #4915
Conversation
- Now Using DF_RC where appropriate. - Renamed pointer for allocation so NLT can detect where the allocation is freed. Signed-off-by: Ryon Jensen <ryon.jensen@intel.com>
ryon-jensen
requested review from
jolivier23,
liuxuezhao,
jgmoore-or and
rpadma2
|
Is this ready for gatekeeping now? |
|
The failure is also seen on master (DAOS-6997). Di is working a fix, but don't think this patch is blocked by it, so requesting force land. |
| D_FREE(iod_csum->ic_data); | ||
| } | ||
|
|
||
| rc = proc_struct_dcs_csum_info(proc, &iod_csum->ic_akey); | ||
| if (unlikely(rc)) { | ||
| D_FREE(iod_csum->ic_data); |
There was a problem hiding this comment.
minor, D_FREE(iod_csum->ic_data) possibly call twice in L216 and L221, D_FREE will not check NULL internally, so better to check if it is NULL here.
There was a problem hiding this comment.
I disagree. Both free() and D_FREE() accept NULL, and in fact we encourage calling D_FREE with null rather than checking to improve readability of both the code and the logs. D_FREE() will set the pointer to NULL so as long as the same variable is called twice it's OK because the first one will have free it but the second one won't.
There was a problem hiding this comment.
From the commit message:
Renamed pointer for allocation so NLT can detect
where the allocation is freed.
NLT does not take into consideration the name of the pointer, just the value when doing the free check, although it logs the name. I suspect you've renamed something here that you didn't need to.
These changes are good, and in line with how we want errors to be reported, however I also notice that there is no change in NLT results in CI so how did you go about finding them? It may be that you have some additional testing you'd like to add to NLT that managed to expose these error paths?
| if (iod_csum->ic_data == NULL) | ||
| return -DER_NOMEM; |
There was a problem hiding this comment.
This would have resulted in a segfault here before presumably?
There was a problem hiding this comment.
Yes, if memory failed to be allocated then a segfault would have probably occurred when ic_data was attempted to be accessed.
There was a problem hiding this comment.
This warrants it's own ticket and a 1.2 backport then I assume?
There was a problem hiding this comment.
Yes, probably would have, and I debated this with myself, but I ended up lumping all the fixes for the issues reported by NLT into one patch. I will work on the backport.
| D_FREE(iod_csum->ic_data); | ||
| } | ||
|
|
||
| rc = proc_struct_dcs_csum_info(proc, &iod_csum->ic_akey); | ||
| if (unlikely(rc)) { | ||
| D_FREE(iod_csum->ic_data); |
There was a problem hiding this comment.
I disagree. Both free() and D_FREE() accept NULL, and in fact we encourage calling D_FREE with null rather than checking to improve readability of both the code and the logs. D_FREE() will set the pointer to NULL so as long as the same variable is called twice it's OK because the first one will have free it but the second one won't.
|
Got approval on behalf of gatekeeper, but need gatekeeper to land :-) ... Requesting gatekeeper again. |
@ashleypittman - The NLT errors were found on another PR where checksums are enabled by default. The purpose of that PR is to just run CI with checksums, but won't actually be landed (at least for a while), and I wanted to get these fixes in now. |
- Now Using DF_RC where appropriate. - Renamed pointer for allocation so NLT can detect where the allocation is freed. Signed-off-by: Ryon Jensen <ryon.jensen@intel.com>
where the allocation is freed.
Signed-off-by: Ryon Jensen ryon.jensen@intel.com