[Rpm-maint] [PATCH 5/5] Add a generic plugin for use by simple Collections

[Steve Lawrence]

On Mon, 2010-06-21 at 15:15 +0300, Panu Matilainen wrote: 
> On Fri, 18 Jun 2010, Panu Matilainen wrote:
> >
> >> +rpmRC COLLHOOK_POST_ANY_FUNC(rpmts ts, const char * collname, const char * 
> >> options)
> >> +{
> >> +	int rc = RPMRC_FAIL;
> >> +
> >> +	if (rpmChrootSet(rpmtsRootDir(ts)) || rpmChrootIn()) {
> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > [snip]
> >> +	if (rpmChrootOut() || rpmChrootSet(NULL)) {
> >                           ^^^^^^^^^^^^^^^^^^^^^
> >
> > These look highly suspect, rpmChrootSet() is done already from rpmtsSetup() 
> > and rpmtsFinish(), everything between those shouldn't need anything more than 
> > just rpmChrootIn() and rpmChrootOut(). Calling rpmChrootSet(NULL) makes it 
> > forget the cwd so further rpmChrootIn/Out calls will fail.
> 
> Oh, another thing wrt chroots: do you have some specific reason to leave 
> the chroot handling for the plugins to handle by themselves, instead of 
> just doing it in rpmtsRunCollection()?
> 

Parts of the SELinux plugin need to be run outside of the chroot. We're
still finishing this up, but when the plugin is run, it iterates through
all transaction elements with policy and extracts the necessary policy
information (policy names, data, etc). The reading of this data needs to
be done outside of the chroot. Ideally, we wouldn't need to do this, but
this made the most sense in order to keep as much as the SELinux
specific code contained in the plugin as possible.

> Entering braindump mode...
> 
> And actually looking at rpmtsRunCollection() and how its called etc... 
> seems to me the whole thing could be buried inside rpmte.c around 
> rpmteProcess() so rpmteLastInCollectionAdd() and friends wouldn't be 
> needed even in internal API.
> 

I think we'll still need rpmteCollection() and rpmteHasCollection() to
be public for the SELinux plugin, but the other collection helpers can
be removed/hidden.

> Errors from collection hooks is something to think about too. Currently 
> rpm only considers elements failed when %pre, %preun or payload processing 
> fails, but does callback notifications for all scriptlet failures (with 
> warning/error indicator). The collections patch set makes collection-hook 
> failures to show up in rpmtsRun() return code as errors while not going 
> through the other mechanisms (marking elements failed, notifying through 
> callback). Which makes the already fuzzy semantics even fuzzier :)
> 

So, should a collection failure mark the te that caused that collection
action to be run as failed? Even though the failure isn't really part of
the te? Maybe just the RPMCALLBACK_COLLECTION_ERROR is enough, and let
the callback handler deal with it?


We have no problems with any of your other comments. We should have a
updated patchset out to you shortly.

- Steve