You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2021/02/10 17:59:38 UTC
[GitHub] [mynewt-mcumgr] vrahane opened a new pull request #108: img_mgmt: Add support for un-registering a group
vrahane opened a new pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108
- 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574681140
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
+ mgmt_group_list = curr->mg_next;
+ return;
+ }
+
+ while (curr && curr != group) {
+ prev = curr;
Review comment:
Agreed, this needs a fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574680306
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
Review comment:
The only reason I keep checking current everywhere is just to be safe since there was no NULL check for group.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane merged pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane merged pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574702947
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
+ mgmt_group_list = curr->mg_next;
+ return;
+ }
+
+ while (curr && curr != group) {
+ prev = curr;
Review comment:
curr will never be equal to group initially here because we take care of that condition above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574677976
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
Review comment:
Yes, agreed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574676640
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
Review comment:
mgmt_register_group() does not do it either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574677976
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
Review comment:
Yes, agreed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] utzig commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
utzig commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574476913
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
Review comment:
There is no need to test `curr` here because if it is NULL it won't be equal to `group`
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
Review comment:
Please check that `group` is not NULL
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
+ mgmt_group_list = curr->mg_next;
+ return;
+ }
+
+ while (curr && curr != group) {
+ prev = curr;
Review comment:
If `curr` is valid and equal to `group`, `prev` will remain uninitialized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane edited a comment on pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane edited a comment on pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#issuecomment-777666374
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574702947
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
+ mgmt_group_list = curr->mg_next;
+ return;
+ }
+
+ while (curr && curr != group) {
+ prev = curr;
Review comment:
curr will never be equal to group here because we take care of that condition above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574676640
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
Review comment:
Happy to do that, the only reason I did not add that is because mgmt_reguster_group() does not do it either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574681140
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
+
+ if (curr && curr == group) {
+ mgmt_group_list = curr->mg_next;
+ return;
+ }
+
+ while (curr && curr != group) {
+ prev = curr;
Review comment:
Agreed, this needs a fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#issuecomment-777666374
> https://travis-ci.org/github/apache/mynewt-nimble/jobs/758517372
>
> this breaks build...
Thank you @sjanc, I did not see any build issues on the zephyr build locally. @utzig ’s PR will fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] utzig commented on pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
utzig commented on pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#issuecomment-777423490
> https://travis-ci.org/github/apache/mynewt-nimble/jobs/758517372
>
> this breaks build...
About time CI is added to this repo...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574676640
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
Review comment:
mgmt_reguster_group() does not do it either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] vrahane commented on a change in pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
vrahane commented on a change in pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#discussion_r574706150
##########
File path: mgmt/src/mgmt.c
##########
@@ -71,6 +71,31 @@ mgmt_streamer_free_buf(struct mgmt_streamer *streamer, void *buf)
streamer->cfg->free_buf(buf, streamer->cb_arg);
}
+void
+mgmt_unregister_group(struct mgmt_group *group)
+{
+ struct mgmt_group *curr = mgmt_group_list, *prev;
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-mcumgr] sjanc commented on pull request #108: mgmt: Add support for un-registering a group along with the same support in img_mgmt
Posted by GitBox <gi...@apache.org>.
sjanc commented on pull request #108:
URL: https://github.com/apache/mynewt-mcumgr/pull/108#issuecomment-777418259
https://travis-ci.org/github/apache/mynewt-nimble/jobs/758517372
this breaks build...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org