Hi Pawel,
On 22/08/2019 12:02, Wieczorkiewicz, Pawel wrote:
On 22. Aug 2019, at 12:29, Julien Grall <julien.grall@xxxxxxx <mailto:julien.grall@xxxxxxx>> wrote:
On 21/08/2019 09:19, Pawel Wieczorkiewicz wrote:
More generally, I am not very comfortable to see panic() in the middle of the code. Could you explain why panic is the best solution over reverting the work?
Yes. Production-ready hotpatches must not contain inconsistent hooks or functions-to-be-applied/reverted.
The goal here is to detect such hotpatches and fail hard immediately highlighting the fact that such hotpatch
is broken.
Aside the len = 0 that you are going to fix. How would this condition happen? Are you going to add code that will potentially trigger the panic?
The default livepatch code path is very conservative (to the extent it does not even need this check and panic).
But, with the changes of this series, one can potentially construct a hotpatch that upon load would trigger the panic
(or without the panic, would silently leave the Xen code in memory in an inconsistent state). This obviously must not
happen in production, so the new livepatch feature should be used with care.The changes make livepatch more
flexible and universal, yet when using new features, more care is needed.
However, when someone is developing a hotpatch or is using the new feature for debugging or for whatever
non-production-related reason, it is very helpful to detect immediately any sort of undefined state.
I have been there myself when I was working on the changes presented here, and thought that would be a good idea
to add this checks permanently.
Also, when something changes in the future in the livepatch implementation, those checks could potentially highlight
any inconsistencies.
The inconsistent application of a hotpatch (some function applied, some reverted while other left behined) leaves
the system in a very bad state. I think the best what we could do here is panic() to enable post-mortem analysis
of what went wrong and avoid leaking such system into production.
Thank you for the explanation here (and on IRC). May I ask some documentation regarding the panic in at least commit message? Ideally, this would explain why the panic the most sensible solution.
Yes, certainly. I will add that.
