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

[Reshetova, Elena]

Hi,

Please find the corrected patch in the attachment.
I really spend quite some time fighting indentation :( , but let's hope it
succeeded now (I am not that sure since it so much changing when I open it
with different editors/settings).
I was trying to use only soft tabs: 4 spaces, but in places where I had to
integrated in existing code it behaves unpredictable because of hard tabs
that I think in some places aren't of same size (but maybe this is my
inability to use editors).

Best Regards,
Elena.


-----Original Message-----
From: rpm-maint-bounces at lists.rpm.org
[mailto:rpm-maint-bounces at lists.rpm.org] On Behalf Of Reshetova, Elena
Sent: Monday, October 22, 2012 3:49 PM
To: Panu Matilainen
Cc: rpm-maint at lists.rpm.org
Subject: Re: [Rpm-maint] [PATCH 1/2] Extending rpm plugin interface, part 1

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: 0001-Extending-rpm-plugin-interface-part-1.patch
Type: application/octet-stream
Size: 16773 bytes
Desc: not available
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20121026/641ca3f9/attachment-0001.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/20121026/641ca3f9/attachment-0001.p7s>