8000 [collectd 6] format_stackdriver: When JSON generation fails, reset the generator … by octo · Pull Request #4268 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[collectd 6] format_stackdriver: When JSON generation fails, reset the generator … #4268

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 1, 2024
Merged

[collectd 6] format_stackdriver: When JSON generation fails, reset the generator … #4268

merged 1 commit into from
Feb 1, 2024

Conversation

octo
Copy link
Member
@octo octo commented Feb 1, 2024 •
edited
Loading

… because it is in an unknown state.

Whenever the JSON generation fails, the code returns early, leaving the generator in an unknown and likely broken state, such as an opened map that we will never close. This happens all throughout the file. This change will reset the generator state in case of an error so that we're back in a well defined state.

Additionally, a free/alloc cycle was replaced with clear+reset, which are much cheaper operations.

ChangeLog: format_stackdriver: An error path that could leave the JSON generator in an undefined state has been fixed.

@octo octo added the Fix A pull request fixing a bug label Feb 1, 2024
@octo octo requested a review from a team as a code owner February 1, 2024 15:59
@collectd-bot collectd-bot added this to the 6.0 milestone Feb 1, 2024
@octo octo mentioned this pull request Feb 1, 2024
Merged
eero-t
eero-t approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor
@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

Approved.

In addition to calling the new reset function when create fails, out->gen free+alloc is now replaced with clear+reset, I guess on assumption that it's faster?

@octo octo force-pushed the 6/format_stackdriver branch from d0ad7cd to 0f36c32 Compare February 1, 2024 16:31
@octo
Copy link
Member Author
octo commented Feb 1, 2024

In addition to calling the new reset function when create fails, out->gen free+alloc is now replaced with clear+reset, I guess on assumption that it's faster?

Yeah, added that to the PR description, too. clear+reset essentially boils down to a memset(). free+alloc is a much more expensive operation.

@octo octo added the Automerge Labels PRs to be merged by a bot once approved label Feb 1, 2024
@collectd-bot collectd-bot merged commit 262d228 into collectd:collectd-6.0 Feb 1, 2024
@octo octo mentioned this pull request Feb 1, 2024
Merged
@eero-t
Copy link
Contributor
eero-t commented Feb 1, 2024

Looking at the whole file closer:

  • sd_output_initialize() can fail and return error without closing map, but its callers do not even check its return value
  • in addition to sd_output_add(), also sd_format_metric_descriptor() calls internal format_*() functions (that open maps, and return on errors without closing them). When an error is returned, sd_format_metric_descriptor() calls yajl_gen_free(gen), is that enough?

@octo octo deleted the 6/format_stackdriver branch February 2, 2024 09:52
@octo octo mentioned this pull request Feb 2, 2024
Merged
@octo
Copy link
Member Author
octo commented Feb 2, 2024
  • sd_output_initialize() can fail and return error without closing map, but its callers do not even check its return value

Good call: #4270 (and #4269)

  • sd_format_metric_descriptor() calls internal format_*() functions (that open maps, and return on errors without closing them). When an error is returned, sd_format_metric_descriptor() calls yajl_gen_free(gen), is that enough?

Yes, that's fine. That function does not use the generator state in sd_output_t that is shared across calls, but uses a generator that is allocated for this one function only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eero-t eero-t eero-t approved these changes

Assignees
No one assigned
Labels
Automerge Labels PRs to be merged by a bot once approved Fix A pull request fixing a bug
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@octo @eero-t @collectd-bot
0