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

[Panu Matilainen]

On 10/17/2012 03:50 PM, Elena Reshetova wrote:
> This change adds a new type of the rpm plugin, called transaction plugin and a set of initial hooks for this plugin.
> The hooks are:
>
> PLUGINHOOK_TSM_PRE
> Pre-transaction hook that is called before an rpm transaction begins
> PLUGINHOOK_TSM_POST
> Post-transaction hook that is called after an rpm transaction ends
> PLUGINHOOK_PSM_PRE
> Pre-transaction-element hook that is called before an rpm transaction-element is processed
> PLUGINHOOK_PSM_POST
> Post-transaction-element hook that is called after an rpm transaction-element is processed
> PLUGINHOOK_SCRIPT_SETUP
> Per-script hook that is called once for each rpm mainainers script that is present in the package
>
> Each hook is called for every plugin that have this hook registered.
> The avaliable transaction plugins can be specified in macros.in via transaction_plugins element.

Okay, as this seems like more of an actual patch submission than 
preliminary thing for sake of discussion, I'm switching into 
nitpick-mode :) Overall this looks quite okay, most of the nits are on 
cosmetic details that I previously either didn't notice or simply didn't 
mention as it wasn't clear what parts you were going to further polish, 
but there are a couple of "real" issues too.

> diff --git a/lib/rpmplugins.c b/lib/rpmplugins.c
> index 9098aa5..bb44f34 100644
> --- a/lib/rpmplugins.c
> +++ b/lib/rpmplugins.c
> @@ -110,6 +110,39 @@ rpmRC rpmpluginsAddCollectionPlugin(rpmPlugins plugins, const char *name)
>       return rc;
>   }
>
> +rpmRC rpmpluginsAddTransactionPlugin(rpmPlugins plugins, const char *name)
> +{
> +    char *path;
> +    char *options;
> +    int rc = RPMRC_FAIL;
> +
> +    path = rpmExpand("%{?__transaction_",name, "}", NULL);
> +    if (!path || rstreq(path, "")) {
> +	rpmlog(RPMLOG_INFO, _("Failed to expand %%__transaction_%s macro\n"), name);
> +	goto exit;
> +    }
> +
> +    /* split the options from the path */
> +#define SKIPSPACE(s)    { while (*(s) &&  risspace(*(s))) (s)++; }
> +#define SKIPNONSPACE(s) { while (*(s) && !risspace(*(s))) (s)++; }
> +    options = path;
> +    SKIPNONSPACE(options);
> +    if (risspace(*options)) {
> +	*options = '\0';
> +	options++;
> +	SKIPSPACE(options);
> +    }
> +    if (*options == '\0') {
> +	options = NULL;
> +    }
> +
> +    rc = rpmpluginsAdd(plugins, name, path, options);
> +
> +  exit:
> +    _free(path);
> +    return rc;
> +}
> +

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 :)

> +
> +rpmRC rpmpluginsCallTsmPre(rpmPlugins plugins, rpmts ts)
> +{
> +    rpmRC (*hookFunc)(rpmts);
> +    int i;
> +    rpmRC rc = RPMRC_OK;
> +    const char *name = NULL;
> +
> +    for (i = 0; i < plugins->count; i++) {
> +	name = plugins->names[i];
> +    	RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_PRE);
> +    	if ( hookFunc(ts) == RPMRC_FAIL )
> +		rc = RPMRC_FAIL;
> +    }
> +
> +    return rc;
> +}
> +
> +rpmRC rpmpluginsCallTsmPost(rpmPlugins plugins, rpmts ts)
> +{
> +    rpmRC (*hookFunc)(rpmts);
> +    int i;
> +    rpmRC rc = RPMRC_OK;
> +    const char *name = NULL;
> +
> +    for (i = 0; i < plugins->count; i++) {
> +	name = plugins->names[i];
> +    	RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_POST);
> +    	if ( hookFunc(ts) == RPMRC_FAIL )
> +		rc = RPMRC_FAIL;

Indentation is off for the entire for-block in this and all the other 
rpmpluginsCall*() functions added (which seem to cry for a common helper 
function but since macros are involved...) The rpm indentation style is 
basically a combination of soft tabs (of width 4) and hard tabs with 
minimal number of each to get to the desired indentation level, which is 
four characters deeper than the previous indentation.
Also spaces dont belong before or after parenthesis (in the if).

'indent -kr' fixes these but makes "wrong" changes on some other aspects 
as rpm isn't "pure" K&R style. One of these days I'll need to figure out 
the magic incantation of indent switches to have it output rpm style. 
Would make everybody's lives easier :)

> +/** \ingroup rpmplugins
> + * Call the post transaction element plugin hook
                ^^^^
> + * @param plugins	plugins structure
> + * @param te		processed transaction element
> + * @return		RPMRC_OK on success, RPMRC_FAIL otherwise
> + */
> +rpmRC rpmpluginsCallPsmPre(rpmPlugins plugins, rpmte te);

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


>   	if (xx == 0) {
> -	    xx = execv(argv[0], argv);
> +                /* Run script setup hook for all plugins */
> +                if (rpmpluginsCallScriptSetup(plugins, argv[0]) != RPMRC_FAIL) {
> +		        xx = execv(argv[0], argv);
> +                }

Cosmetics: indentation is off here.


> 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.

>
>       /* 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.

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.


> +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.

> +{
> +    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)

> +
> +    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.

Cosmetics: the indentation is off in this function, various mixtures of 
spaces that should be a single tab + overly deep indentation for the 
rpmpluginsAddTransactionPlugin() line.

> +
>   int rpmtsRun(rpmts ts, rpmps okProbs, rpmprobFilterFlags ignoreSet)
>   {
>       int rc = -1; /* assume failure */
> @@ -1441,11 +1467,19 @@ int rpmtsRun(rpmts ts, rpmps okProbs, rpmprobFilterFlags ignoreSet)
>   	goto exit;
>       }
>
> +

Cosmetics: Unnecessary empty line added.

> +    rc = rpmteSetupTransactionPlugins(ts);
> +
>       rpmtsSetupCollections(ts);
>
>       /* Check package set for problems */
>       tsprobs = checkProblems(ts);
>
> +    /* Run pre transaction hook for all plugins */
> +    if ( rpmpluginsCallTsmPre(ts->plugins, ts) == RPMRC_FAIL) {
            ^^
Cosmetics: extra space after parenthesis.

> +	goto exit;
> +    }
> +
>       /* Run pre-transaction scripts, but only if there are no known
>        * problems up to this point and not disabled otherwise. */
>       if (!((rpmtsFlags(ts) & (RPMTRANS_FLAG_BUILD_PROBS|RPMTRANS_FLAG_NOPRE))
> @@ -1489,6 +1523,11 @@ int rpmtsRun(rpmts ts, rpmps okProbs, rpmprobFilterFlags ignoreSet)
>   	runTransScripts(ts, PKG_POSTTRANS);
>       }
>
> +    /* Run post transaction hook for all plugins */
> +    if ( rpmpluginsCallTsmPost(ts->plugins, ts) == RPMRC_FAIL) {
            ^^
Cosmetics: extra space after parenthesis.

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 :)

	- Panu -