[Rpm-maint] FSM hooks for rpm plugin

[Panu Matilainen]

On 01/22/2013 04:36 PM, Reshetova, Elena wrote:
> Hi,
>
Hi,

> Long time again since I replied :( Unfortunately had to resolve a number of
> other issues and wanted to attach smth already to this mail as opposite to
> just "reply".

No worries.

> I have started from FSM hooks as you indicated and I am including the
> initial version of patch for review based on our discussion.
>
> I have two hooks: fileOpen and fileClose and call them separately for
> install and erase. I had to make a number of choices while writing this
> patch, let's see if they were good ones :)
> Some details:
>
> - I tried to keep the logic of other hooks: if pre_hook is called, post_hook
> is also called with the result of the operation. However, it is a bit
> trickier in fsm case. For that purpose, I moved the fileclose hook in
> installation out of fsmCommit() that we can nicely pass the result to the
> hook.
>    I also think it looks better from symmetry point of view, but it does now
> perfom labelling of a file (if it happens inside of a plugin) not exactly at
> the same place where Selinux currently does it.

The exact place of current selinux labeling doesn't really matter, 
that's not an issue.

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.

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.

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 :)

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.

>
> - I also made it that result from fileclose hook is ignored currently for
> the same reason as for post_tsm and post_psm hooks: what can rpm do after
> file has been committed even if plugin is unhappy?

Yup. This is fine with me.

> -The tricky part is what to do with the result code of fileOpen hook. In
> principle, this can be the place to abort installation/erasure of a concrete
> file in case smth really terrible happened (can't even think what can
> happen). Normally plugins should not abort anything on this hook (as we
> discussed) and if they do, then smth is wrong in plugin.  On the other hand,
> rpm itself is physically able to abort at that point and even does it in
> cases for example if smth wrong with the archive unpacking. So, I am not
> really sure what to do with the return code in this case.

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. Unfortunately 
there's no way to enforce this kind of semantic in the API we'll just 
have to settle for documenting the intention I guess.

> - I was also thinking that it is probably not worth making it initially more
> complicated and adding additional hooks, like for handling the temporal
> files, because they can't really help fully with the security part: we might
> succeed setting whatever label on tpm file, but fail a second after on real
> file, or not succeed setting a label even on tmp file. I guess these hooks
> can be added on demand or simply later if the strong need comes.

Fine with me, better get the basics working first and if it turns out 
that's not enough, then we can think what to do about it.

	- Panu -