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

mgmt: Add support for un-registering a group along with the same support in img_mgmt #108

Merged
merged 3 commits into from
Feb 10, 2021

Conversation

vrahane
Copy link
Contributor

@vrahane vrahane commented Feb 10, 2021

  • Add support for un-registering a group at runtime
  • Add support for un-registering an image management group at runtime
  • Re-entrancy to be handled by caller since the API modifies a list

Vipul Rahane and others added 3 commits February 10, 2021 09:46
- Add support for unregistering command group at runtime
- Add support for unregistering img_mgmt_group at runtime
@vrahane vrahane changed the title img_mgmt: Add support for un-registering a group mgmt: Add support for un-registering a group along with the same support in img_mgmt Feb 10, 2021
Copy link
Contributor

@mlaz mlaz left a comment

Choose a reason for hiding this comment

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

LGTM

@vrahane vrahane merged commit fb489e1 into apache:master Feb 10, 2021
@sjanc
Copy link
Contributor

sjanc commented Feb 11, 2021

@utzig
Copy link
Member

utzig commented Feb 11, 2021

https://travis-ci.org/github/apache/mynewt-nimble/jobs/758517372

this breaks build...

About time CI is added to this repo...

Copy link
Member

@utzig utzig left a comment

Choose a reason for hiding this comment

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

@vrahane Please fix the issues in a new PR.

{
struct mgmt_group *curr = mgmt_group_list, *prev;

if (curr && curr == group) {
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to test curr here because if it is NULL it won't be equal to group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I keep checking current everywhere is just to be safe since there was no NULL check for group.

void
mgmt_unregister_group(struct mgmt_group *group)
{
struct mgmt_group *curr = mgmt_group_list, *prev;
Copy link
Member

Choose a reason for hiding this comment

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

Please check that group is not NULL

Copy link
Contributor Author

@vrahane vrahane Feb 11, 2021

Choose a reason for hiding this comment

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

mgmt_register_group() does not do it either.

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 think I know the reason why the NULL check is not there, it’s because memory for the groups is not allocated here, it is a structure defined in the static memory that does not change. Nonetheless, does not hurt to add the check here.

}

while (curr && curr != group) {
prev = curr;
Copy link
Member

Choose a reason for hiding this comment

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

If curr is valid and equal to group, prev will remain uninitialized.

Copy link
Contributor Author

@vrahane vrahane Feb 11, 2021

Choose a reason for hiding this comment

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

curr will never be equal to group initially here because we take care of that condition above.

@vrahane
Copy link
Contributor Author

vrahane commented Feb 11, 2021

https://travis-ci.org/github/apache/mynewt-nimble/jobs/758517372

this breaks build...

Thank you @sjanc, somehow, I did not see any build issues on the zephyr build locally. @utzig ’s PR will fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants