Open
Conversation
Telling it to use the same number of threads as in cts-cli (one fewer than the number of processors) speeds it up dramatically.
It shouldn't be, but clang's static analysis gets tripped up thinking it's possible, so we might as well add a check.
…rwrite. I think the idea here was to make sure that 'type' would always have something set, just in case the pcmk__graph_next enum gained some more values. However, we have the compiler flags enabled to warn about that so there should be no way through the switch statement without setting it to something.
It was complaining that 'text' was getting set and then immediately overwritten with some other value. This can be solved by just getting rid of the variable.
These shouldn't be strictly necessary, but clang can't figure them out and thinks NULL pointer dereferences are possible. In general, I'm not a fan of adding these sorts of obvious checks. On the other hand, I'm less a fan of having static analysis enabled but then ignoring its results.
If fopen returns success, errno will be undefined. If getline reads a file that is empty, the loop will never get a chance to set errno to 0, in which case the test will be checking an undefined value. Instead, rearrange the loop so that errno is always set to 0.
Telling it to use the same number of threads as in cts-cli (one fewer than the number of processors) speeds it up dramatically.
…ode. I'm not sure whether or not this is a real bug. The IPC client freeing code is a little bit of a mess so it's hard to tell. However, clang thinks there is something here so just to be safe, check that api is not NULL before freeing it.
We don't want to assign the return value of g_list_append back into the hash table (note the comments above), but assigning its value to the list variable causes static analysis to complain about a dead assignment. Just turn off the warning for this one line.
…plies. Multiple static analysis tools think that it's impossible to reach the pcmk__rsc_trace call at the bottom with reason still set to its initial value of "allowed". I'm not convinced of that, but regardless we can still avoid the dead assignment by using pcmk__s instead.
Static analysis can't figure out that filename can't be NULL in the else block, so I'm just going to add a test ahead of time. In general I dislike rearranging code like this just to please static analysis tools, but I prefer doing that to having useless results full of false positives.
This function has a nested fgets loop for reading long descriptions that extend over multiple lines. If the inner fgets hits EOF or an error, it'll just continue and the outer fgets will be in some indeterminate state. Instead, handle those possibilities. I think there's likely a lot that could be done to this function to make it more robust, but this will have to do for now.
Contributor
This looks possible, but even if not, the code could use some clarifying. |
Contributor
Author
|
The last patch addresses memory leaks from running multiple 10-run ctslab sessions through valgrind. It is definitely not an exhaustive check of memory leaks by any means (perhaps I should also run regression tests through it), but I think it's going to catch the most important leak. The only other things that valgrind reports are things we already knew about but haven't been able to address, for instance: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are fixes for issues found by static analysis. There are still three outstanding results from clang that I haven't decided what to do about (if anything):
rsc->flagsis a NULL pointer dereference, which is impossible.args->verbosityis a NULL pointer dereference, which is impossible.pcmk__new_common_argsexits on error.search, which is pretty hard to decide if that's the case or not.