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

Fix websocket transfer callback userdata & server name in tls_options #313

Merged
merged 5 commits into from Aug 3, 2023

Conversation

xiazhvera
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023

Codecov Report

Patch coverage: 9.09% and project coverage change: -0.09% ⚠️

Comparison is base (f09d932) 82.17% compared to head (2e4ee3c) 82.08%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
- Coverage   82.17%   82.08%   -0.09%     
==========================================
  Files          20       20              
  Lines        8661     8670       +9     
==========================================
  Hits         7117     7117              
- Misses       1544     1553       +9     
Files Changed Coverage Δ
source/v5/mqtt5_to_mqtt3_adapter.c 75.42% <9.09%> (-0.59%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiazhvera xiazhvera changed the title Fix user data for 5-to-3-Adapter websocket transfer callback Fix websocket transfer callback userdata & server name in tls_options Aug 3, 2023
Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

Fix n ship

@@ -326,6 +340,13 @@ static struct aws_mqtt_adapter_connect_task *s_aws_mqtt_adapter_connect_task_new
connect_task->clean_session = connection_options->clean_session;

return connect_task;

error:
aws_ref_count_release(&adapter->internal_refs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, let's move the acquire after the last control flow that can error out. Having to release refs in error blocks is something to avoid if possible.

Copy link
Contributor Author

@xiazhvera xiazhvera Aug 3, 2023

Choose a reason for hiding this comment

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

The task is using the adapter->allocator. If we do not acquire the adapter beforehand, would there be chance that that the allocator get released with the adapter?

Copy link
Contributor

Choose a reason for hiding this comment

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

no

@@ -314,6 +314,20 @@ static struct aws_mqtt_adapter_connect_task *s_aws_mqtt_adapter_connect_task_new
if (connection_options->tls_options) {
aws_tls_connection_options_copy(&connect_task->tls_options, connection_options->tls_options);
Copy link
Contributor

@sfod sfod Aug 3, 2023

Choose a reason for hiding this comment

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

This call can fail. Since s_aws_mqtt_adapter_connect_task_new is already changed, maybe add a check for this call as well?

@xiazhvera xiazhvera merged commit b777be4 into main Aug 3, 2023
31 checks passed
@xiazhvera xiazhvera deleted the 5to3ws branch August 3, 2023 23:08
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