Messages in this thread |  | | Date | Wed, 16 Aug 2017 15:20:32 -0500 | From | Josh Poimboeuf <> | Subject | Re: [PATCH v3] add (un)patch callbacks |
| |
On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote: > v3: > > - livepatch.h > - drop obj->patched checks from pre/post-(un)patch funcs, > add preceding comment and note about obj->patched assumptions > - move core.c :: klp_is_module() to here > > - klp_complete_transition() > - fix "else if (klp_target_state == KLP_UNPATCHED)" case > - combine conditional syntax when avoiding module_put for immediate > patches > - add check for klp_is_object_loaded to avoid callbacks for any > unloaded modules (necessary after removing obj->patched checks in > livepatch.h) > > - Documentation > - added Josh's use-cases blurb in intro > - s/Callbacks are only executed/A callbacks is only executed/ > > - livepatch-callbacks-demo.c > - whitespace cleanup > > I also wrote a quick test script (see below) to exercise some of the > load/unload/enable/disable/error status combinations. I'm not sure > about some of the behaviors, most notably test6 with regard to > post-unpatch-callbacks as executed on a cancelled transition. (See > results and comments further below.)
Yeah, that doesn't seem right. Maybe in case of a pre-patch callback error, we should only call post-unpatch callbacks for those objects whose pre-patch callbacks were successfully called (and returned zero). That would mean tracking on a per-object basis which objects had their pre-patch callbacks called (successfully).
That would give the patch module a post-unpatch chance to tear down anything it had set up in the pre-patch callback.
And the behavior should be documented.
> Also, maybe it's just my reading of the log, but would it be clearer if > the "(un)patching ... complete" messages indicated that they are > referring to a transaction? It's a bit confusing to see "unpatching ... > complete" before the pre-unpatch-callbacks ever execute. Not a big > deal, but I can send a follow up patch if others agree.
Hm. I'm thinking this highlights the fact that the pre-unpatch callback is being called in the wrong place. It should actually be called before the unpatching transition starts. When called from klp_unpatch_objects(), the new code is no longer running, so it's effectively post-patch instead of pre-patch.
Another random thought: maybe we should show the "patching complete" message *after* the post-patch callback is run. That would be more honest with the user, as technically, the post-patch callback is part of the patching process.
And a similar comment for the "unpatching complete" message.
-- Josh
|  |