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

Misc cleanups #147

Merged
merged 9 commits into from
Sep 27, 2023
Merged

Misc cleanups #147

merged 9 commits into from
Sep 27, 2023

Conversation

bmarzins
Copy link
Member

I've gotten distracted with other things, so I haven't made much progress on the remove work yet, but here are some fixes that I've accumulated while looking through the code to understand what needs to be done. I'm not quite sure if the final commit is the correct solution (erroring out instead of just skipping the failed sysfs attribute and going on). But the most likely cause of this seems to be that the sysfs device has disappeared. Right now this happens during remove events, but even when those have a different call path, the sysfs device could still get removed while we are processing an earlier uevent. in this case, I don't see much point in continuing to update the database for the device. But if you'd rather the code skip adding that attribute and continue with the function, I can change it.

In sid_resource_create() we can fail after the resource is allocated,
but before we initialize the lists. In this case we were calling
list_iterate_items_safe_back() on res->children and res->event_sources
before they were initialized.
service_link_add_notification(), service_link_remove_notification(), and
service_link_group_add_member() cannot fail, but they still return an
integer. _create_service_link_group() even checks this integer, although
its failure path doesn't correctly cleanup the allocated service link.
Instead of adding code to cleanup the service link on an error that
cannot happen, just change the functions to not return anything.
Make sure that the message size is larger than the header before
memcpying the header, so we don't access possibly unallocated memory.
If we are doing a remove, count will be zero here.
Calling _destroy_key() with a NULL key is fine, so we can always call it
on failure here.
If SID fails to get the sysfs dm sysfs values, likely because the
device has been removed from sysfs, fail instead of using uninitialized
memory for the key.
@prajnoha
Copy link
Member

...the sysfs device could still get removed while we are processing an earlier uevent. in this case, I don't see much point in continuing to update the database for the device. But if you'd rather the code skip adding that attribute and continue with the function, I can change it.

Yup, right now, we should skip. I'm just fiddling with the "ready" state for devices, so later on, we will also cover this situation (when we expect certain part/attribute to exist on the system, but it's not there, obviously because the device has been removed in the meantime) by marking the device with proper state and erroring out, giving a chance for SID to react somehow (expecting a subsequent event to be "REMOVE" or restart the operation as if it was "REMOVE" etc.).

Thanks for the patches!

@prajnoha prajnoha merged commit 9d90234 into sid-project:main Sep 27, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants