More bugfixes and remove infrastructure #148
Merged
Conversation
This file contains 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
When _delta_step_calc() is building the delta buffers, it can add entries to the delta->final and delta->minus buffers that point to data stored in the old vvalue (delta->plus will only ever point to new data, which isn't stored in the old vvalue). When _kv_delta_set finally writes the new vvalue from delta->final, the old vvalue is freed. This means that there can be entries in delta->final and delta->minus that point to freed memory. The same thing can happen to abs_delta->plus and abs_delta->minus after those are written out. Where this is actually visible is when a device is removed and the device hierarchy is updated. In this case, delta->minus ends up with entries pointing to freed memory. If that memory is overwritten, it will effect both the DELTA_WITH_DIFF and DELTA_WITH_REL output. The easiest way to avoid this is to run _kv_delta_set() inside a transaction, so the old values don't get freed until the end, when the transaction is ended.
_refresh_device_partition_hierarchy_from_sysfs() always assumed that the partition was being added.
If SID fails to get the module name at all, it will not run the ident functions of the block modules. However, it will run all the other scan phase functions. In all these other scan phase functions, the block modules will be run, regardless of whether the device has a type module, even if the module name couldn't be determined. Make the ident phase also run the block modules, even if the module name can't be determined, to be consistent.
Instead of using the regular scan phases for removes, use a separate set, with just the same initialization and cleanup as a regular scan. The main remove is done in two phases. The first calls the new module function, scan_remove for the appropriate type and block modules. These are currently all stubs. The second phase removes the core SID kvstore values. Currently this just removes the device from the device hierarchy in the kvstore.
|
Yup, that looks good. The "fix freed memory access in _kv_delta_set" - wow, that was quite a bug, slipped through the cracks. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I still keep having other things on my plate that are keeping me from being able to finish up the remove work. Instead, I'll just send you what I currently have, so you can see the direction I'm heading. I've split out the remove work from the regular scan work, although it still uses the "usid scan" command, since there is a lot of code overlap with regular scans. It currently doesn't do anything special on removes. That's what I'm working on next. Also, here are fixes for two bugs that were keeping resyncing the device hierarchy from working on removes.