Re: [PATCH v3 1/2] livepatch: Add dynamic klp_object and klp_func iterators

[Josh Poimboeuf]

On Wed, Sep 27, 2017 at 11:41:29PM -0400, Jason Baron wrote:
> In preparation to introducing atomic replace, introduce iterators for
> klp_func and klp_object, such that objects and functions can be dynamically
> allocated (needed for atomic replace). This patch is intended to
> effectively be a no-op until atomic replace is introduced.

Hi Jason,

Sorry for coming in so late on this.  This will be a nice feature.  I
haven't gotten to the second patch yet, but I have a few comments
already.

1) Having both static *and* dynamic lists seems to add a lot of
   complexity and brittleness.

   The livepatch API is easier to use with static data structures, so I
   don't think we should change the API.  But I also don't think we
   should allow the API to overly constrain our internal representation
   of the data.

   What I'd *really* like to do is completely separate the user-supplied
   data structures from our internal data .  At registration time, we
   can convert all the arrays to dynamic lists, and then never touch the
   arrays again.  Then managing the lists is completely straightforward.

   That would also allow us to have an external public struct and an
   internal private struct, so the interface is more clear and we don't
   have to worry about any patch modules accidentally messing with our
   private data.

2) There seems to be a general consensus that the cumulative "replace"
   model is the best one.  In light of that, I wonder if we can simplify
   our code a bit.  Specifically, can we get rid of the func stack?

   If the user always uses replace, then the func stack will never have
   more than one func.  And we can reject a patch if the user doesn't
   use replace when they're trying to patch a function which has already
   been patched.

   Then the func stack doesn't have to be a stack, it can just be a
   single func.

   That would allow us to simplify the code in several places for the
   common case, yet still allow those who want to apply non-cumulative
   patches to do so most of the time.

   It's not a *huge* simplification, but we've already made livepatch so
   flexible that it's too complex to fit everything in your head at the
   same time, and any simplification we can get away with is well worth
   it IMO.  And once we have replace, I wonder if anybody would really
   miss the func stack.

Thoughts?

-- 
Josh