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 -
_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
