[pacman-dev] Fwd: [BUG] chk_filedifference is buggy!!

[Nagy Gabor]

Idézés Dan McGee <dpmcgee at gmail.com>:

> 2008/2/11 Nagy Gabor <ngaba at bibl.u-szeged.hu>:
> > > Nagy, we've discussed this before in the past. *Please* do not just
> > > point out problems with the expectation that we will fix it. I know
> > > you are a very competent coder, and I'd appreciate you contributing
> > > solutions rather than inferring our code is shit. Saying things like
> > > "totally needless" does not start things off on the right foot with
> > > Aaron and I, who have really tried hard to clean up some questionable
> > > code. We do make mistakes.
> >
> > Oh, I didn't want to say, that your code is shit (however: serious).
> Instead,
> > that function is quite tricky, and not easy to understand at all (that's
> why I
> > didn't provide a patch yet...: 1. I'm also quite tired/ill nowadays => I
> don't
> > trust on myself now; 2. I want to do a bigger rewrite of that part)
> >
> > The word "totally needless" referred to the following: there is (should be)
> no
> > difference between empty and n-element list, so that pB==NULL check is not
> > needed <- the incorrect handling of pB==NULL was a special case of this
> BUG.
> >
> > > diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
> > > index c093705..9ef5c9f 100644
> > > --- a/lib/libalpm/conflict.c
> > > +++ b/lib/libalpm/conflict.c
> > > @@ -279,6 +279,12 @@ static alpm_list_t
> > > *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
> > >                         }
> > >           }
> > >         }
> > > +       /* ensure we have completely emptied pA */
> > > +       while(pA) {
> > > +               const char *strA = pA->data;
> > > +               ret = alpm_list_add(ret, strdup(strA));
> > > +               pA = pA->next;
> > > +       }
> > >
> > >         return(ret);
> > >  }
> > >
> >
> > This seems OK, but the previous code filters out directories, so this is
> needed
> > here too (this was missed in the old pB==NULL handler part, too).
> 
> Ahh, you are correct. If you would have said this originally, then I
> would understand why this problem was not easy at first sight. :)
> 
> > > This patch fixes the revised upgrade040 and 041 pactests, as well as
> > > upgrade011 as attached. It doesn't fix upgrade046.py. Any thoughts for
> > > that issue? I'm not familiar enough with the whole program flow there
> > > yet.
> > >
> >
> > Yes, for fixing upgrade046.py bigger efforts are needed (however, that is
> > probably rarer issue). The problem is, when you use --force, the
> > _alpm_db_find_fileconflicts is not called at all. And this file relocation
> > "stuff" (skip_remove) is computed in that function; so this computation is
> not
> > done at all with --force.
> 
> Yeah, interesting. We may have to rework this somehow...hmm.
> 
A possible hotfix:
We may call _alpm_db_find_fileconflicts() with --force too (which is in fact a
fileconflict PLUS filerelocation check), and simply ignore its reported
fileconflict errors.

One more small but must-mention issue:
See upgrade041.py and conflict.c:428 and consider the following case: user
deleted /usr/share/file by hand [this is not forbidden, however not suggested],
then this filerelocation simply won't be detected and the issue appear again.

Long run:
Fixing these issues is not easy at all (I mean cleaning up this part: moving
filerelocation stuff to add.c and fix these issues), I'm afraid of a possible
appearing slowdown (see the last issue for example).
On the other hand, transaction rollback will eliminate this whole filerelocation
computation, since then we will do the "final step" (the REAL removal and and
install) at transaction level, not in "package level", so we will just see:
file.OLD-PID, file.NEW-PID at the end or something like this. And this fact is
quite unmotivating :-P
Transaction rollback can also easily fix trans001.py and fileconflict00?.py so
this is just an other reason for implementing it (however, that will be
difficult too ;-P and personally I'm not a big fan of that, but I must admit
that it is needed.)

Bye


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/