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>