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-boun...@lists.rpm.org [mailto:rpm-maint-boun...@lists.rpm.org] On Behalf Of Reshetova, Elena Sent: Monday, October 22, 2012 3:49 PM To: Panu Matilainen Cc: rpm-maint@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:pmati...@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.
0001-Extending-rpm-plugin-interface-part-1.patch
Description: Binary data
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
