Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch

[Josh Poimboeuf]

On Wed, Jan 23, 2019 at 01:27:59PM -0500, Joe Lawrence wrote:
> > I wanted to ask why there is list_empty() and not klp_patch_enabled(), so
> > just to be sure... the patch was added to klp_patches list, so patch->list
> > is not empty (should not be). We could achieve the same by calling
> > !klp_patch_enabled() given its implementation, but it would look
> > counter-intuitive here.
> > 
> > The rest looks fine.
> > 
> > However, I am not sure if the outcome is better than what we have. Yes,
> > patch->enabled is not technically necessary and we can live with that (as
> > the patch proves). On the other hand, it gives the reader clear guidance
> > about the patch's state. klp_patch_enabled() is not a complete
> > replacement. We have to call list_empty() in __klp_enable_patch() or check
> > the original klp_target_state in klp_try_complete_transition().
> > 
> > I am not against the change, I am glad to see it is achievable, but I am
> > not sure if the code is better with it. Joe acked it. What do the others
> > think?
> 
> Let me qualify my ack -- I think minimizing the number of state variables
> like patch->enabled can help readability... on the other hand, deducing the
> same information from other properties like list-empty can be confusing, ie,
> klp_patch_enabled() is generally a lot clearer than
> list_empty(&patch->list)).
> 
> So I like this idea and would be interested to hear what folks think about
> the exception cases you pointed out.

I share Miroslav and Joe's ambivalence.  It's interesting to see that it
can be done, and normally I'd prefer to get rid of extraneous data
fields, but the patch doesn't reduce code, and it even makes the code
slightly more complex IMO, because klp_patch_enabled() doesn't always
work like you'd expect.

So while I suggested it to begin with, I'm going to go with a NACK :-)

-- 
Josh