Skip to content
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

* MDF [core/sp/supplemental] Handle the case when nni_zalloc fails #592

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RanMaoyi
Copy link
Contributor

fixes #589

@RanMaoyi RanMaoyi self-assigned this Jun 28, 2023
@RanMaoyi RanMaoyi added the bug Something isn't working label Jun 28, 2023
@@ -199,6 +228,12 @@ dbtree_get_tree(dbtree *db, void *(*cb)(uint32_t pipe_id))
dbtree_info **ret_line_ping = NULL;
for (size_t i = 0; i < cvector_size(nodes); i++) {
dbtree_info *vn = nni_zalloc(sizeof(dbtree_info));
if (vn == NULL) {
/* All vn are pushed in ret_line_ping vector*/
cvector_free(ret_line_ping);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check that using cvector_free to release memory is correct or not ? @JaylinYu

Copy link
Member

@JaylinYu JaylinYu Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesnt need this?
@lee-emqx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you noticed any issues with cvector_free?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had any issues yet.
Just curious about the rationality of this approach :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When dealing with pointer arrays such as ret_line_ping, it's important to remember to free its members before freeing the array itself (use cvector_free). This ensures that memory is properly deallocated and avoids any potential issues.

Copy link
Contributor Author

@RanMaoyi RanMaoyi Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked at the cvector_free function and did not free all the members, so I added the following code. @JaylinYu @lee-emqx

for (int j = 0; j < cvector_size(ret_line_ping); j++) {
	nni_free(ret_line_ping[j], sizeof(dbtree_info));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vn's member also need :P

* Use free() to release memory, because of the len
* is too difficult to obtain.
*/
free(topic_queue[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe nni_strfree is more proper here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix to nni_strfree

@@ -199,6 +228,12 @@ dbtree_get_tree(dbtree *db, void *(*cb)(uint32_t pipe_id))
dbtree_info **ret_line_ping = NULL;
for (size_t i = 0; i < cvector_size(nodes); i++) {
dbtree_info *vn = nni_zalloc(sizeof(dbtree_info));
if (vn == NULL) {
/* All vn are pushed in ret_line_ping vector*/
cvector_free(ret_line_ping);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When dealing with pointer arrays such as ret_line_ping, it's important to remember to free its members before freeing the array itself (use cvector_free). This ensures that memory is properly deallocated and avoids any potential issues.

@JaylinYu JaylinYu marked this pull request as draft June 30, 2023 07:06
@JaylinYu JaylinYu added enhancement New feature or request and removed bug Something isn't working labels Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A lot of nni_zalloc didn't handle failure cases(From Code Review)
3 participants