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

Bluetooth: shell: Fix null check in cmd_conn_phy_update #34203

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

kaiser-ren
Copy link
Contributor

@kaiser-ren kaiser-ren commented Apr 11, 2021

When I used ./tests/bluetooth/shell, if there is not any connections established but perform bt phy-update, it can cause a fatal error because of conn-check-missing, null check for default_conn pointer is necessary in case causing a system failure.

Fixes #34201

@kaiser-ren
Copy link
Contributor Author

Try to fix #34201

@kaiser-ren kaiser-ren marked this pull request as draft April 11, 2021 14:39
Copy link
Contributor

@cvinayak cvinayak left a comment

Choose a reason for hiding this comment

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

Other commands like data length update, connection update, etc... all have this issue. Its best to address them all.

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

I generally agree with @cvinayak regarding adding this everywhere, but I'm perfectly content to fix those in other PRs.

Copy link
Contributor

@joerchan joerchan left a comment

Choose a reason for hiding this comment

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

The merge commits must be removed.

@Thalley
Copy link
Collaborator

Thalley commented Apr 12, 2021

@kaiser-ren Btw, you can link this PR to the issue by using one of the Github closing keywords in the pull request description: https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

That way, when this is merged, the issue is automatically marked as done.

@kaiser-ren
Copy link
Contributor Author

@kaiser-ren Btw, you can link this PR to the issue by using one of the Github closing keywords in the pull request description: https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

That way, when this is merged, the issue is automatically marked as done.

Thanks. Apologize that this is my first zephyr pull request, it may take some time.

@kaiser-ren kaiser-ren marked this pull request as ready for review April 15, 2021 00:47
@kaiser-ren
Copy link
Contributor Author

Here is a CI error, Commit e8d025fca8:
3: UC4 Line exceeds max length (109>72): "For Link Layer PHY update, null check for default_conn pointer is necessary in case causing a system failure."

I'm not sure how can I modify the commit message? And it's already being public on Zephyr repo.

@Thalley
Copy link
Collaborator

Thalley commented Apr 15, 2021

Here is a CI error, Commit e8d025fca8: 3: UC4 Line exceeds max length (109>72): "For Link Layer PHY update, null check for default_conn pointer is necessary in case causing a system failure."
I'm not sure how can I modify the commit message? And it's already being public on Zephyr repo.

You can modify your commit message but doing either git commit --amend or using git rebase -i

@joerchan
Copy link
Contributor

@kaiser-ren You should read up on the contributing guidelines for the project, that will help you get this PR ready.
https://docs.zephyrproject.org/latest/contribute/index.html#contribution-workflow

@kaiser-ren
Copy link
Contributor Author

Here is a CI error, Commit e8d025fca8: 3: UC4 Line exceeds max length (109>72): "For Link Layer PHY update, null check for default_conn pointer is necessary in case causing a system failure."
I'm not sure how can I modify the commit message? And it's already being public on Zephyr repo.

You can modify your commit message but doing either git commit --amend or using git rebase -i

Thanks, fixed.

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Please also reduce the commits to just one commit.

@joerchan
Copy link
Contributor

@kaiser-ren The commit is empty.

@kaiser-ren
Copy link
Contributor Author

Apologize very deeply to make this PR mess, finally, make CI happy except buildkite is still ongoing. @Thalley, already merged the commit into one.
This 1st PR makes me learn a lot, thanks for your guide and patient.

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Looks alright to me

Comment on lines 2145 to 2147
shell_error(shell,
"%s: at least, one connection is required\n",
shell->ctx->active_cmd.syntax);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation seems slight off, but not something I would block this PR for

@@ -2141,6 +2141,13 @@ static int cmd_conn_phy_update(const struct shell *shell, size_t argc,
struct bt_conn_le_phy_param param;
int err;

if (default_conn == NULL) {
shell_error(shell,
"%s: at least, one connection is required\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

\n is redundant. shell_error adds in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nordic-krch .
@nordic-krch and @Thalley , after this PR, I will create another PR to add the null check for conn_update() and conn_data_length_update(), how about I fix "\n" at the next PR and we keep this approval process going? Sound good to every approver?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think that you can remove \n in this PR, if there is no critical changes others reviewers don't need to re-review and approvals stays.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @nordic-krch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger, I've already done it.

Null check in case of system failure.

Signed-off-by: Kai Ren <renkaikaiser@163.com>
@jhedberg jhedberg merged commit de16299 into zephyrproject-rtos:master Apr 20, 2021
@kaiser-ren kaiser-ren deleted the #34201 branch April 21, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal error when perform "bt phy-update" if there is not any connections at ./tests/bluetooth/shell
7 participants