Re: [gentoo-portage-dev] making aux_get more usable for vardbapi

[Brian Harring]

On Mon, Jan 23, 2006 at 07:44:44PM +0100, Marius Mauch wrote:
> On Mon, 23 Jan 2006 03:47:11 -0800
> Brian Harring <ferringb@g.o> wrote:
> > On Mon, Jan 23, 2006 at 11:16:03AM +0100, Marius Mauch wrote:
> > > On Wed, 11 Jan 2006 12:39:03 -0800
> > > Brian Harring <ferringb@g.o> wrote:
> > >
> > > > Regex you've got there allows for pulling the wrong text- recall,
> > > > ebd originally was doing grep based filtering (regex).  Had to
> > > > rewrite that in a major hurry since bash syntax (specifically
> > > > here ops) forces you to track state/constructs rather then just a
> > > > regex...
> > >
> > > Not really an issue in this case. First the code bails out if more
> > > than one match is found, so unless the metadata assignment is NOT
> > > found by it we don't get the wrong info.
> > > Also a mismatch in this special is so
> > > extremely unlikely that honestly I don't really care about it,
> > > especially as this is a one time conversion (might be different if
> > > I'd have added the on the fly extraction).
> >
> > Re-read that statement.  It's a one time conversion- meaning we
> > better get it right the first time, else the user's data is
> > effectively corrupted.  Forcing a full regen from the saved
> > environment is not a solution for fixing past corruptions either.
> >
> > If it were on the fly extraction, I wouldn't care quite as much- but
> > the fact this is an untracked change to the users data means we *do*
> > need to cover corner cases.
>
> Can't follow your thinking here. As said, the code won't corrupt any
> data, at worst it will tell the user that it couldn't extract some keys
> (and even that only if there would be such a stupid case which itself
> has a chance of like 10^-10 or so, or can you name a single case?).

Yeah, any extension of your code to pick up EAPI is going to false 
positive on somewhere around a quarter of my vdb nodes- I use a 
debug function from my bashrc for prefix testing (holdover from eapi 
testing days).  Why?  Because the eapi var didn't exist (thus no env 
assignment), the only possible match is in the middle of a here op-

debug_dump() {
	...
	echo <<<HERE >&2
EAPI="${EAPI-unset}"
PF="$PF"
CATEGORY="$CATEGORY"
HERE
	...
}

Setting EAPI to unset strikes me as corruption here, because now 
portage will *never* do anything with that vdb node- the eapi differs 
from what it supports (it's been set to effectively a random value), 
thus I have to fix the vdb entry myself if I ever want to get rid of 
it.

Guessing the response on that one is "well, you're using the bashrc", 
and yes, I am- the vdb code should be *robust*, not choking on a users 
debug code.

> So what package contains filter-env then?

Portage- been in cvs/svn for over a year now :)

> > Aside from that, if the code is in debate (as this is), I really
> > don't think it should get slid into svn 2 weeks later effectively
> > unchanged- didn't write that original email just for the hell of it :)
>
> As said, I disagree with your assessment of the situation. If you can
> name a single case where the code "breaks" or filter-env hits the tree
> I might reconsider, but not before.

Well, if you disagreed with the original response, continue the 
conversation prior to commiting- otherwise we see a commit, then a 
rebuttal a few hours later.  Not really how things should go for a 
contested piece of code (at least when the only two to weigh in our 
flat out opposed on it)- especially if the code's effect is nontrivial 
and it hasn't had any actual peer review (only comment was on your algo).

Issues with the code.
1) Scans for metadata that didn't exist at the time of the ebuild's 
installation, only possible match is unintended.  Not good.
2) your code is breaking upon *all* newlines, regardless if it's 
in the middle of a variable assignment.  Yes, bash does use $'' for 
assignment, but you're relying (hoping really) that the variable has 
been filtered by python portage and the ebuild defined var is stomped.  
This is a bad idea- description comes to mind.  Your code seems to be 
aware of this possibility also, since it's doing rather loose quote 
filtering.
3) Stop using regex all over the place.  split/join is a helluva lot 
faster, and collapses that nasty regex that attempts to remove 
whitespace down to two lines (a one time translate, and the split/join 
to kill whitespace).
4) Code specifically tries to find, and remove variable chars- 
this is *really* the wrong thing to do.  If you're trying to work 
around catching a var reference, either your parsing screwed up, or 
you're mangling a DESCRIPTION value that you shouldn't be hosing.
5) hardcoded vdb.
6) Non root aware VDB.
7) Only is aware of environment.bz2- older portage versions differed 
in this afaik (I *do* know PORT_ENV_FILE overloading for ebd I had to 
address this case already, so it's out there).
8) perms on the new files?
9) general code comments; 'if s != ""', use 'if s:'.  Catch the 
exceptions for check mode, don't let vdbkeyhandler nuke world file 
checking due to a potential exception.  Right now, your code will TB 
if ran by non-root for a check run rather then stating "got to be 
root".

In general, in looking through the parsing here there is a *lot* of 
chunks that look questionable- the scan for a var assignment for 
example, should *not* be stripping quotes on it's own.  The only 
benefit of that stripping is allowing for false positives (hardly a 
benefit).

Aside from code commentary, the real brass tacks here is that this 
code if used is going to be expanded for more then the trivial vars it 
handles now- in other words, this problematic parser will wind up 
being used months down the line for important vars with intrinsic issues 
still unaddressed.

And yes, what I'm pointing at is a corner case- that doesn't mean 
we cut corners on the solution *especially* when a proper tool 
exists for this already.  No tool?  I'd be a bit quieter, but the 
issues *are* resolved already, you just need to ditch the adhoc regex 
attempt (or write your own state aware parser).  It's a 20 minutes mod 
to fix my initial complaints on this- it might seem pointless, but the 
potential is there as is a fix, so address it.

~harring