[Rpm-maint] FSM hooks for rpm plugin
pmatilai at laiskiainen.org
Sat Feb 2 09:38:42 UTC 2013
On 01/28/2013 02:47 PM, Reshetova, Elena wrote:
> I am attaching the next version of the patch with modifications explained
> inline below.
>> The patch looks deceptively simple :) In principle things look ok to me,
> the issues I see have mostly to do with the symmetry part:
>> In fsmMkdirs(), the FileOpen() hook is not called, only
> rpmpluginsCallFsmFileClose(). If we promise symmetry, we need to stick to it
> here too. Looks like just an oversight and trivial to fix.
> Yes, fixed now: I was looking to install/remove mostly and forgot about
> symmetry on this one.
I've started to wonder about the hook names though: "open" and "close"
seem out of place especially here (and to lesser degree elsewhere).
Perhaps these hooks should simply follow the pre/post pattern even in
the names, ie FILE_PRE and FILE_POST. I think it'd be more descriptive
wrt what they actually do, and leave open and close free for possible
later use for the cases where a kind of open+close (files from payload
stream) is actually occurring.
>> rpmPackageFilesRemove() looks like symmetry is indeed guaranteed, but the
> problem here is that the hooks will get called whether the file/directory is
> actually removed or not, and the hook does has no way of knowing it: passing
> FSM_PKGERASE (or FSM_PKGINSTALL on install) is not sufficient. Either we
>> need to pass fsm->action (those FA_CREATE, FA_ERASE, FA_SKIP etc things) to
> the hooks, or alternatively only call the hooks when an actual action is
> taken. Or even both: a plugin might want to know if eg a backup is taken.
>> Not sure which option is the best here: from plugin POV it'd certainly be
> simpler to not have to worry about files that rpm has decided to skip
> entirely (there shouldn't be need to label etc such things). But then there
> might be use-cases where plugins would want to know the fate of each file,
> including >skipped. It's not exactly hard to do "if (XFA_SKIPPED(action))"
> in plugins that dont care about skipped files either.
> I think it would make sense to call a plugin anyway but supply the action.
> It is indeed easy for plugin to filter actions it wants to do smth about and
> it is hard to see all potential use cases. So, I am passing the action now
> as a parameter and then it is up to plugin to react on this.
Yup, makes sense. It does expose some fairly murky internals (notably
action on RPMFILE_GHOST items not always matching what is actually done
to them, possibly something else as well) to plugins, but the problem
here is the murky internals, not the information being passed to
plugins. So we'll need a bit of cleaning in the internals but that's for
the better anyway.
>> And then there's the PITA also known as rpmPackageFilesInstall(). There are
> at least two cases where FileClose() hook could be skipped despite
>> FileOpen() getting called (early "return" before expandRegular() and a
> "break" in the post-processing to rmdir/unlink failed file. At least the
> latter should be reasonably easy to refactor, the early return case ..
>> guess I need to actually figure out what on earth its about in the first
> place :)
> Hm... I guess both can be fixed by manually calling fileclosed() hook before
> return or break (like in this version of the patcj), but it doesn't look
> that clean and nice. :( The verify() function before early return is hard to
> read for me, it seems to do million things and even called a number of times
> during installation. Another option would be to call fileOpen() hook later,
> but then one can't stop cleanly in emergency cases.
This gets quite ugly indeed, better refactor the surrounding code (which
is ugly in itself) to allow for nice clean pairing. Oh and btw, this
version of the patch introduces a new asymmetry case: if the open-hook
fails, close-hook wont get called. It'll need something like open-hook
just flagging postpone without terminating the loop, and adjusting the
rest of the code to deal with it. I'll have a look at this, how hard can
it be ;)
>> And like said, the same question whether to call the hooks for skipped
> files is true here as well, but probably somewhat trickier as there are more
> details and cases to handle.
> Would passing the action to all hooks help to it?
Yup, passing the action like done in this patch version makes sense for
>> I think the plugins should be allowed to cause an abort here, just like
>> rpm itself can, in case of fatal unexpected failure. Unexpected is the
>> key really: what I complained about was the MSM plugin *planning* to
>> fail here on unresolved file conflicts, which is something that should
>> be handled before anything gets installed or removed.
> Yes, I agree and I think the only reason before was the signature check hook
> wasn't called early enough, but this is one of the next todo items to do it
> properly :)
- Panu -
More information about the Rpm-maint