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: Audio: Fix dead code issue in VCS #35394

Merged
merged 1 commit into from May 20, 2021

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented May 18, 2021

The common API functions in VCS had dead code in specific
configurations, causing coverity issues. Fixed by this
commit.

Signed-off-by: Emil Gydesen emil.gydesen@nordicsemi.no

Fixes #35131
Fixes #35129
Fixes #35128
Fixes #35125
Fixes #35124
Fixes #35123
Fixes #35122
Fixes #35121
Fixes #35120
Fixes #35132

@joerchan
Copy link
Contributor

Woah, 10 for 1. What a deal.

@Thalley
Copy link
Collaborator Author

Thalley commented May 18, 2021

Woah, 10 for 1. What a deal.

That's the way it is when code is basically copy-pasted :D I briefly considered a commit for each, but with such a small change I think it made sense to group them :)

@joerchan
Copy link
Contributor

@Thalley Are you sure you aren't just trading one coverity warning for another now?

If the issue is that coverity sees that the return cannot be reached due to the ifdefs, wouldn't it just now say instead that the assign to -ENOTSUP would be always overwritten?

I'm thinking it might have been better to just close these is invalid/intentional.

Maybe using IS_ENABLED instead of ifdef would have improved it.

@Thalley
Copy link
Collaborator Author

Thalley commented May 18, 2021

@Thalley Are you sure you aren't just trading one coverity warning for another now?

If the issue is that coverity sees that the return cannot be reached due to the ifdefs, wouldn't it just now say instead that the assign to -ENOTSUP would be always overwritten?

I'm thinking it might have been better to just close these is invalid/intentional.

Maybe using IS_ENABLED instead of ifdef would have improved it.

@joerchan
Damn, I think you are right.
The bt_vcs_client calls can easily be guarded by a IS_ENABLED instead, but the server code blocks expects the vcs_inst struct to be declared, which it wouldn't be, so it requires a bit more to be able to use IS_ENABLED for the server.

We might need to just close these as invalid.

@joerchan
Copy link
Contributor

Ok, then I think closing the issue without any changes is the best, coverity does not have the overview of the feature defines, so it cannot see that the code is only dead under certain circumstances.

@Thalley Thalley force-pushed the coverity_vcs_dead_code branch 2 times, most recently from c15f668 to 05dbc63 Compare May 18, 2021 12:57
The common API functions in VCS had dead code in specific
configurations, causing coverity issues. Fixed by this
commit.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
@Thalley
Copy link
Collaborator Author

Thalley commented May 18, 2021

@joerchan I've tried another design that should work and not cause coverity issues.

@MaureenHelm MaureenHelm added this to the v2.6.0 milestone May 18, 2021
@MaureenHelm MaureenHelm added the bug The issue is a bug, or the PR is fixing a bug label May 18, 2021
@carlescufi carlescufi merged commit 03a2c29 into zephyrproject-rtos:main May 20, 2021
@Thalley Thalley deleted the coverity_vcs_dead_code branch June 1, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment