[Rpm-maint] Plugin ponderings

[Panu Matilainen]

On 11/30/2012 08:11 PM, Reshetova, Elena wrote:
> Hi,
>
> I made a new small patch to clean up the existing pre/post hooks as we
> discussed this week. After this is cleaned up, I can get back to fsm hooks.
> After our recent discussion I think I need to reconsider places of some hooks,
> for example post hook, or maybe make two post hooks?
> One when file is placed to tpm location (time to label the file) and another
> one after fsm commit. What do you think?

One small "general patch hygiene" issue:

-           if (rpmpluginsCallPsmPre(ts->plugins, te) == RPMRC_FAIL)
-               break;
+           if (rpmpluginsCallPsmPre(ts->plugins, te) == RPMRC_FAIL) {
+               goto exit;
+           }

One-liner if statements are generally ok with or without the braces, but 
please dont add them (or remove existing ones) unnecessarily, it makes 
the actual change harder to see for weary eyes on a quick glance, 
compared to:

             if (rpmpluginsCallPsmPre(ts->plugins, te) == RPMRC_FAIL)
-               break;
+               goto exit;

Both the rpmpsmRun() and rpmtsRun() changes basically achieve what we 
discussed, presumably with "minimum possible changes" goal, but overall 
clarify is IMO more important that absolute bary minimum changes. Eg 
jumping within a switch-case, to a label called "exit", nowhere near the 
exit of the function seems a bit iffy :)

Similarly the "exit1" label in rpmtsRun() is, if nothing else, not very 
descriptive :) Might be clearer to just add a variable to track TsmPre 
hook execution and call rpmpluginsCallTsmPost() based on that tracking 
variable from the existing exit-label, that way you dont need to worry 
about which exit point you need to jump to in a given point of the 
function. In fact, that'd also make the desired semantics very explicit 
in the code itself, so it'd probably make sense in the psm part too 
(details obviously differ a bit there).

There's not much point in checking for the result code of TsmPost now, 
might just as well make it just:

     /* Run post transaction hook for all plugins */
     rpmpluginsCallTsmPost(ts->plugins, ts, rc);

Then there's a question of what, if anything, to do about the various 
special modes of transactions, such as RPMTRANS_FLAG_TEST and 
RPMTRANS_FLAG_BUILD_PROBS. In a sense it might be good to just not call 
plugins at all on these cases (I can envision several plugins forgetting 
to properly handle those) but then there are perfectly legal cases where 
plugins might want to be aware of test-transactions (eg to deny 
something early). Perhaps its best left as it is from that POV, ie let 
plugins figure out the correct actions for a given set of flags.

> Best Regards,
> Elena.
>
> P.S. At least hope my indentation issues are over :)

Indeed, I don't see a single indentation issue in this patch. Good, now 
we can properly concentrate on the more important stuff :)

	- Panu -