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 amqp property value that was being release early #1004

Merged
merged 2 commits into from
May 15, 2019
Merged

Conversation

jebrando
Copy link
Contributor

Checklist

  • I have read the [contribution guidelines] (https://github.com/Azure/azure-iot-sdk-c/blob/master/.github/CONTRIBUTING.md).
  • I added or modified the existing tests to cover the change (we do not allow our test coverage to go down).
  • If this is a modification that impacts the behavior of a public API
    • I edited the corresponding document in the devdoc folder and added or modified requirements.
  • I submitted this PR against the correct branch:
    • This pull-request is submitted against the master branch.
    • I have merged the latest master branch prior to submission and re-merged as needed after I took any feedback.
    • I have squashed my changes into one with a clear description of the change.

Reference/Link to the issue solved with this PR (if any)

Description of the problem

Description of the solution

Copy link
Contributor

@ewertons ewertons left a comment

Choose a reason for hiding this comment

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

Does not contain unit tests.

@massand
Copy link
Contributor

massand commented May 14, 2019

                else if (type == AMQP_TYPE_STRING)

Please copy the "when this is a string vs int" comment here #Resolved


Refers to: provisioning_client/src/prov_transport_amqp_common.c:364 in 06c122a. [](commit_id = 06c122a, deletion_comment = False)

@massand
Copy link
Contributor

massand commented May 14, 2019

                if (desc != NULL && amqpvalue_get_type(desc) == AMQP_TYPE_ULONG)

Questioning why this is a ULONG on error? Please add comment here and clarify with service. #Pending


Refers to: provisioning_client/src/prov_transport_amqp_common.c:254 in 06c122a. [](commit_id = 06c122a, deletion_comment = False)

@massand
Copy link
Contributor

massand commented May 14, 2019

                        amqp_info->retry_after_value = (uint32_t)desc_value;

I think its prudent to set this to UINT32_MAX here if there is a value larger here. #Resolved


Refers to: provisioning_client/src/prov_transport_amqp_common.c:259 in 06c122a. [](commit_id = 06c122a, deletion_comment = False)

@massand massand merged commit 3d9e978 into master May 15, 2019
@jebrando jebrando deleted the fix_amqp_prop branch May 16, 2019 18:24
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.

3 participants