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

b4.4 bug: trap failed json conversion in stream_client_dump #1486

Closed
wants to merge 1 commit into from

Conversation

baallan
Copy link
Collaborator

@baallan baallan commented Oct 29, 2024

fix #1475

@baallan baallan changed the title trap failed json conversion in stream_client_dump b4.4 bug: trap failed json conversion in stream_client_dump Oct 29, 2024
@baallan baallan added this to the v4.4.5 milestone Oct 29, 2024
@tom95858
Copy link
Collaborator

@baallan, I think we need to look at fixing this more generally. The fmt_status function is called a couple dozen times inside ldmsd_controller. The fmt_status() failure is the result of the server (ldmsd) returning an error instead of a valid result. Most of the code patterns are:

rc, msg = self.conn.some_status_function(...)

If the rc != 0, then the result is an error message and should not be assumed to be JSON. The caller can then return without attempting to decode the string. The issue with this is that this convention is not uniformly implemented in ldmsd_controller, e.g. in the code you changed. The bug was not that the JSON couldn't be decoded, but that the rc was not consulted before attempting to do so. To fix this properly, we need to look through the 23 fmt_status() call sites and ensure that the rc is always consulted.

@tom95858 tom95858 closed this Oct 29, 2024
@baallan
Copy link
Collaborator Author

baallan commented Oct 30, 2024

@tom95858 i did notice that fmt_status is used rather inconsistently, but that many sites had some kind of guard code. Picking a preferred handling style and refactoring the entire file to apply it is beyond the scope of both the reported bug and the fix. Let's not let better be the enemy of good in this case. If you are closing this because ogc will be applying a better fix during the current release milestone, then that is fine.

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.

2 participants