[Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1

Reshetova, Elena elena.reshetova at intel.com
Mon Oct 22 12:49:04 UTC 2012


Hi Panu,

Thank you for  the comments! I will post the next version of patch with
corrections soon. My comments for non-cosmetic issues are below and of
course I will fix cosmetics also!

-----Original Message-----
From: Panu Matilainen [mailto:pmatilai at laiskiainen.org] 
Sent: Thursday, October 18, 2012 11:44 AM

>AFAICS, with the exception of the actual macro names used, this is
identical to rpmpluginsAddCollectionPlugin(). While not a real showstopper,
I've grown very weary of mopping up the consequences of copy-paste coding in
rpm codebase over the course of the last five years... Unless you foresee
this >significantly diverging from the collection plugin setup, I'd prefer
having these combined or at least made to use a common internal helper from
the start to avoid creeping copy-pasteism :)

Agree, will redo in the next submission.

>Probable copy-paste error, the description should be "pre", right? :)

Oh, yes, sorry, will fix. 

> diff --git a/lib/rpmte.c b/lib/rpmte.c index 35b8e3e..5972505 100644
> --- a/lib/rpmte.c
> +++ b/lib/rpmte.c
> @@ -941,7 +941,7 @@ int rpmteProcess(rpmte te, pkgGoal goal)
>       int scriptstage = (goal != PKG_INSTALL && goal != PKG_ERASE);
>       int test = (rpmtsFlags(te->ts) & RPMTRANS_FLAG_TEST);
>       int reset_fi = (scriptstage == 0 && test == 0);
> -    int failed = 1;
> +    int failed = 0;

>This isn't right, failure from rpmteOpen() would return success from
>rpmteProcess() with this change.

True, a nasty error, will fix.  
>
>       /* Dont bother opening for elements without pre/posttrans scripts */
>       if (goal == PKG_PRETRANS || goal == PKG_POSTTRANS) { @@ -955,7 
> +955,17 @@ int rpmteProcess(rpmte te, pkgGoal goal)
>       }
>
>       if (rpmteOpen(te, reset_fi)) {
> -	failed = rpmpsmRun(te->ts, te, goal);
> +
> +    	/* Run pre transaction element hook for all plugins */
> +    	if (goal != PKG_PRETRANS && goal != PKG_POSTTRANS) {
> +    		failed = rpmpluginsCallPsmPre(rpmtsPlugins(te->ts), te);
> +    	}
> +	if (!failed) {
> +		failed = rpmpsmRun(te->ts, te, goal);
> +        	/* Run post transaction element hook for all plugins*/
> +        	if (goal != PKG_PRETRANS && goal != PKG_POSTTRANS)
> +    			failed = rpmpluginsCallPsmPost(rpmtsPlugins(te->ts),
te);
> +		}

>Hmm, this might be simpler to handle inside rpmpsmRun() where the
pre/posttrans/verify cruft is already weeded out to a separate case. If left
here, I'd think the condition should be on "scriptstage" instead of
PKG_PRETRANS/POSTTRANS to avoid calling these hooks on verification.

Yes, I guess I'll better move them to psm.c inside rpmpsmRun(). Will look
cleaner since no additional conditions will be needed. 

>FWIW, the collection hooks kinda need to be here as they should be called
even if rpmteOpen() for the last collection elements failed (the logic is a
bit suspect but that's an unrelated issue), but these new hooks are
different.

Yes, I didn't touch the collection hooks, since it looks like the needs are
different between them and the new hooks. 

>> +static rpmRC rpmteSetupTransactionPlugins(rpmts ts)

>Shouldn't this be rpmtsSetup... not, rpmte? :) Probably just a copy-paste
leftover from collection plugins which are on the transaction elements.
Yeah, another copy-paste one, sorry :(

> +{
> +    rpmRC rc = 0;
> +    char *plugins = NULL, *plugin = NULL;
> +    char delims[] = ",";
> +
> +    plugins = rpmExpand("%{?__transaction_plugins}", NULL);
> +    if (!plugins || rstreq(plugins, "")) {
> +        rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_plugins
macro\n"));
> +	return -1;
> +    }

>Not having any plugins configured is not an error and should not complain,
in a "normal" setup. Obviously this has to do with wanting to enforce
security plugins being loaded but we need to find some other way to deal
with it. For now, "%{?__transaction_plugins}" evaluating to empty should
just quietly >return with success. (I do see the return code is ignored in
the caller, but the output from here is ugly and causes most of the
test-suite to fail)

Yes, actually this return code is a leftover from the previous case when I
tried to enforce the loading. It is amazing how much you can miss when
looking to the code after many times. Fresh eye is a bliss here! 

> +
> +    plugin = strtok(plugins, delims);
> +    while(plugin != NULL) {
> +	rpmlog(RPMLOG_DEBUG, _("plugin is %s\n"), plugin);
> +        if (!rpmpluginsPluginAdded(ts->plugins, (const char*)plugin)) {
> +	        rc = rpmpluginsAddTransactionPlugin(ts->plugins, (const
char*)plugin);
> +        }
> +        plugin = strtok(NULL, delims);
> +    }
> +    free(plugins);
> +    return rc;
> +}

>...but any configured plugins failing should indeed abort the thing, so the
caller should take not of the return code.
Yes.

>Once these mostly minor nits are fixed I think we're good to go. I'm
actually quite eager to have this in so I can start moving the selinux stuff
into a plugin :)

Will do my best in the next version! :)

Best Regards,
Elena.
-------------- 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/20121022/bf4033c0/attachment.p7s>


More information about the Rpm-maint mailing list