This is an automated email from the ASF dual-hosted git repository. sjanc pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mynewt-core.git
commit 6546730041adc0119ef24bfa79c4b92a19fc7518 Author: Szymon Janc <[email protected]> AuthorDate: Tue Oct 6 19:16:40 2026 +0200 mgmt/mcumgr/mgmt: Fix group list corruption on unregister When last group in list was unregistered, mgmt_group_list_end was set to the removed group instead of the previous one. Group registered after that was linked to the removed group and was not reachable from the list. When the only group was unregistered list end was not cleared at all. This also adds unit test for groups registration. Assisted-by: Claude:claude-opus-5-5 --- mgmt/mcumgr/mgmt/selftest/pkg.yml | 31 ++++++++++ mgmt/mcumgr/mgmt/selftest/src/mgmt_test.c | 98 +++++++++++++++++++++++++++++++ mgmt/mcumgr/mgmt/src/mgmt.c | 20 ++++--- 3 files changed, 140 insertions(+), 9 deletions(-) diff --git a/mgmt/mcumgr/mgmt/selftest/pkg.yml b/mgmt/mcumgr/mgmt/selftest/pkg.yml new file mode 100644 index 000000000..323bf45a4 --- /dev/null +++ b/mgmt/mcumgr/mgmt/selftest/pkg.yml @@ -0,0 +1,31 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +pkg.name: mgmt/mcumgr/mgmt/selftest +pkg.type: unittest +pkg.description: "Unit tests for mcumgr management group registration." +pkg.author: "Apache Mynewt <[email protected]>" +pkg.homepage: "http://mynewt.apache.org/"; +pkg.keywords: + +pkg.deps: + - "@apache-mynewt-core/sys/log/stub" + - '@apache-mynewt-core/sys/console/stub' + - '@apache-mynewt-core/test/testutil' + - '@apache-mynewt-core/mgmt/mcumgr/mgmt' diff --git a/mgmt/mcumgr/mgmt/selftest/src/mgmt_test.c b/mgmt/mcumgr/mgmt/selftest/src/mgmt_test.c new file mode 100644 index 000000000..5e99c7f77 --- /dev/null +++ b/mgmt/mcumgr/mgmt/selftest/src/mgmt_test.c @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "os/mynewt.h" +#include "testutil/testutil.h" +#include "mgmt/mgmt.h" + +static int +mgmt_test_handler(struct mgmt_ctxt *ctxt) +{ + return 0; +} + +static const struct mgmt_handler mgmt_test_handlers[] = { + [0] = { mgmt_test_handler, NULL }, +}; + +#define MGMT_TEST_GROUP(id) \ + { \ + .mg_handlers = mgmt_test_handlers, \ + .mg_handlers_count = 1, \ + .mg_group_id = (id), \ + } + +static struct mgmt_group group_a = MGMT_TEST_GROUP(MGMT_GROUP_ID_PERUSER + 0); +static struct mgmt_group group_b = MGMT_TEST_GROUP(MGMT_GROUP_ID_PERUSER + 1); +static struct mgmt_group group_c = MGMT_TEST_GROUP(MGMT_GROUP_ID_PERUSER + 2); +static struct mgmt_group group_d = MGMT_TEST_GROUP(MGMT_GROUP_ID_PERUSER + 3); + +static bool +mgmt_test_registered(const struct mgmt_group *group) +{ + return mgmt_find_handler(group->mg_group_id, 0) != NULL; +} + +TEST_CASE_SELF(mgmt_test_case_register_unregister) +{ + mgmt_register_group(&group_a); + mgmt_register_group(&group_b); + mgmt_register_group(&group_c); + TEST_ASSERT(mgmt_test_registered(&group_a)); + TEST_ASSERT(mgmt_test_registered(&group_b)); + TEST_ASSERT(mgmt_test_registered(&group_c)); + + /* Remove last group and add new one at the end of list. */ + mgmt_unregister_group(&group_c); + TEST_ASSERT(!mgmt_test_registered(&group_c)); + mgmt_register_group(&group_d); + TEST_ASSERT(mgmt_test_registered(&group_a)); + TEST_ASSERT(mgmt_test_registered(&group_b)); + TEST_ASSERT(mgmt_test_registered(&group_d)); + + /* Remove first group. */ + mgmt_unregister_group(&group_a); + TEST_ASSERT(!mgmt_test_registered(&group_a)); + TEST_ASSERT(mgmt_test_registered(&group_b)); + TEST_ASSERT(mgmt_test_registered(&group_d)); + + /* Remove all groups, list must be usable afterwards. */ + mgmt_unregister_group(&group_d); + mgmt_unregister_group(&group_b); + TEST_ASSERT(!mgmt_test_registered(&group_b)); + TEST_ASSERT(!mgmt_test_registered(&group_d)); + mgmt_register_group(&group_c); + TEST_ASSERT(mgmt_test_registered(&group_c)); + + /* Unregistering group that is not registered is no-op. */ + mgmt_unregister_group(&group_a); + TEST_ASSERT(mgmt_test_registered(&group_c)); +} + +TEST_SUITE(mgmt_test_suite) +{ + mgmt_test_case_register_unregister(); +} + +int +main(int argc, char **argv) +{ + mgmt_test_suite(); + return tu_any_failed; +} diff --git a/mgmt/mcumgr/mgmt/src/mgmt.c b/mgmt/mcumgr/mgmt/src/mgmt.c index d652089ac..18d2aa558 100644 --- a/mgmt/mcumgr/mgmt/src/mgmt.c +++ b/mgmt/mcumgr/mgmt/src/mgmt.c @@ -80,24 +80,26 @@ mgmt_unregister_group(struct mgmt_group *group) return; } - if (curr == group) { - mgmt_group_list = curr->mg_next; - return; - } - while (curr && curr != group) { prev = curr; curr = curr->mg_next; } - if (!prev || !curr) { + if (!curr) { return; } - prev->mg_next = curr->mg_next; - if (curr->mg_next == NULL) { - mgmt_group_list_end = curr; + if (prev) { + prev->mg_next = curr->mg_next; + } else { + mgmt_group_list = curr->mg_next; + } + + if (mgmt_group_list_end == curr) { + mgmt_group_list_end = prev; } + + curr->mg_next = NULL; } static struct mgmt_group *
