[Rpm-maint] Plugin ponderings

Reshetova, Elena elena.reshetova at intel.com
Wed Dec 5 11:31:43 UTC 2012


Hi,

Sorry for the delay: got buried with other stuff :(

Here is the corrected version of the patch:
- cleaned up hygiene
- removed any additional "goto" labels: for tsm made it like you suggested 
with just variable, for psm, it seemed cleaner to put the psm calls inside of 
if(), but I can also make it with variable or name goto label, smth like 
"postpsm".
- removed the void checking of postsm hook return value (I wanted actually to 
do it before, but it slipped through :()

Best Regards,
Elena.



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 -
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Making-pre-post-tsm-psm-hooks-more-consistent.patch
Type: application/octet-stream
Size: 6688 bytes
Desc: not available
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20121205/c66a5080/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 7220 bytes
Desc: not available
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20121205/c66a5080/attachment.p7s>


More information about the Rpm-maint mailing list