Hi, 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. >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. >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. >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? >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 :) Best Regards, Elena.
0001-Adding-FSM-file-hooks.patch
Description: Binary data
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
