Date: Fri, 18 Aug 2017 16:09:41 +0200
From: Petr Mladek <pmladek@suse.com>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Joe Lawrence <joe.lawrence@redhat.com>, live-patching@vger.kernel.org,
        linux-kernel@vger.kernel.org, Jessica Yu <jeyu@kernel.org>,
        Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
        Chris J Arges <chris.j.arges@canonical.com>
Subject: Re: [PATCH v3] add (un)patch callbacks
Message-ID: <20170818140941.GD25223@pathway.suse.cz>
References: <1502911024-16143-1-git-send-email-joe.lawrence@redhat.com>
 <20170816202032.lpmi2ucklnq5gzqv@treble>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170816202032.lpmi2ucklnq5gzqv@treble>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1996
Lines: 44

On Wed 2017-08-16 15:20:32, Josh Poimboeuf wrote:
> On Wed, Aug 16, 2017 at 03:17:03PM -0400, Joe Lawrence wrote:
> > 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.

All this makes sense.

> > 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.

Makes sense as well.

Best Regards,
Petr