-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Rework slam function #536
Rework slam function #536
Conversation
Need to check if there are time restrictions for the CI (reached tests regarding steganography)
Didn't fail on a test and they run without error locally. |
@Narrat I need some help after solving conflicts with master. Maybe there is something I've done wrong? Beware here I force pushed my conflict resolution over your old branch, so better save your own local copy of it for reference. I like very much that we have a test now and also the |
Roger. I will take a look |
I'm a bit puzzled :D
doesn't do its job. Or is the comment a reminder, that it needs to be done if one wants to use Just rebased onto loop handling:
Original state of the PR:
Two ways:
To get it working I will go with 2 for the time being. |
In dyne#504 list_processes() got reworked in a way to avoid parsing process output as this had interesting side-effects. Back then I mentioned the same behaviour existing in slam_tomb() which should probably be changed too. This PR addresses that. Firstly it will use list_processes() from within slam_tomb(), as this is in principal overlapping functionality. For this list_processes() needed to be adjusted. It now has a return value which can indicate if there were processes. Secondly the order of execution was changed in slam_tomb(). Before it would process one process and work through the signals until this process was killed. Now it will take a signal and issue a kill for all processes found.
Force pushed the adjusted branch. Now the checks are happy again:
|
In general umount_tomb and slam_tomb shared a lot of similar code. Main difference being, that the latter additionally searched for processes and would still call umount_tomb if the processes could be killed. umount_tomb would then again search with the provided name for the relevant tomb in list_tomb_mounts, which should be obsolete at this point. Therefore the decision to reduce slam_tomb in functionality. It would only work on a supplied tombname and tombmount, look if there are processes and is called from within umount_tomb. (Theoretical tombname could be removed) Calling tomb with slam or close sets a flag, which will decide if that part in umount_tomb will be executed.
The additional force push because I forgot to remove a temporary output |
many thanks! looks good even with the small hack on tombname, still less invasive and feasible for now. I agree we could look into making the |
Two things get in this PR addressed:
Commit 1:
slam_tomb(): don't parse process output and rework
In #504 list_processes() got reworked in a way to avoid parsing process
output as this had interesting side-effects.
Back then I mentioned the same behaviour existing in slam_tomb() which
should probably be changed too. This PR addresses that.
Firstly it will use list_processes() from within slam_tomb(), as this is
in principal overlapping functionality. For this list_processes() needed
to be adjusted. It now has a return value which can indicate if there
were processes.
Secondly the order of execution was changed in slam_tomb(). Before it
would process one process and work through the signals until this
process was killed. Now it will take a signal and issue a kill for all
processes found.
Commit 2:
slam_tomb: simplify and rename to _kill_processes
In general umount_tomb and slam_tomb shared a lot of similar code.
Main difference being, that the latter additionally searched for
processes and would still call umount_tomb if the processes could
be killed.
umount_tomb would then again search with the provided name for the
relevant tomb in list_tomb_mounts, which should be obsolete at this
point.
Therefore the decision to reduce slam_tomb in functionality. It would
only work on a supplied tombname and tombmount, look if there are
processes and is called from within umount_tomb.
(Theoretical tombname could be removed)
Calling tomb with slam or close sets a flag, which will decide if
that part in umount_tomb will be executed.
And the final PR for the time being :)