Re: [pacman-dev] Pacman doesn't handle properly the multiple satisfiers, localdb corruption
Oh yes, please, clean that update_depends function, trying to read it almost killed me. (so go with case 1 I guess). OK, but one more question: tranc.c:461: if(trans->packages && trans->type == PM_TRANS_TYPE_REMOVE) { if(_alpm_pkg_find(dep->name, handle->trans->packages)) { continue; } } Should I keep this or remove? I prefer remove, but I'm not sure. Pro (for keeping): this reduces the slow disk write in many cases (usually there is dependency between the removed packages) Contra: this breaks libalpm hierarchy, it assumes that the remove_commit will be successful; if not, or user break or... then the database can become corrupted (however, extra requiredby is not a big trouble, as you said), so this is a bit unsafe. Bye, ngaba
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
On Mon, Jul 16, 2007 at 05:13:25PM +0200, ngaba@petra.hos.u-szeged.hu wrote:
Oh yes, please, clean that update_depends function, trying to read it almost killed me. (so go with case 1 I guess). OK, but one more question: tranc.c:461: if(trans->packages && trans->type == PM_TRANS_TYPE_REMOVE) { if(_alpm_pkg_find(dep->name, handle->trans->packages)) { continue; } } Should I keep this or remove? I prefer remove, but I'm not sure. Pro (for keeping): this reduces the slow disk write in many cases (usually there is dependency between the removed packages) Contra: this breaks libalpm hierarchy, it assumes that the remove_commit will be successful; if not, or user break or... then the database can become corrupted (however, extra requiredby is not a big trouble, as you said), so this is a bit unsafe.
Yes indeed, it might be safer to remove. Besides, there was a comment that was removed at some point (not sure when), but it was there at the point where that update_depends function was created. This function was actually refactored from several places, so that's a good thing, but in the same time, a part of it was duplicated, I fail to see why. http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=92ad5565128d4... the comment I'm referring to : + /* XXX: this is a hack...if this dependency is in the transaction targets + * of a remove transaction no need to update its requiredby info: + * it is in the process of being removed (if not already done!) */ + /* TODO I wonder if we can just skip this, the few extra operations won't be + * a huge deal either way */ + if(handle->trans && handle->trans->packages + && handle->trans->type == PM_TRANS_TYPE_REMOVE) { + if(_alpm_pkg_isin(dep.name, handle->trans->packages)) { + continue; + } + } and duplicated part is everything after this : /* Ensure package has the right newpkg */ which became this in the current code: /* this is cheating... we call this function to populate the package */
Yes indeed, it might be safer to remove. OK, my patch attached, please test it. Bye, ngaba
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
On Mon, Jul 16, 2007 at 07:25:50PM +0200, ngaba@petra.hos.u-szeged.hu wrote:
Yes indeed, it might be safer to remove. OK, my patch attached, please test it. Bye, ngaba
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index a18dc1f..5c94141 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -844,6 +844,9 @@ int _alpm_pkg_splitname(const char *target, char *name, char *version, int witha
/* scan the local db to fill in requiredby field of package, * used when we want to install or add a package */ + +/* A depends on B through n depends <=> A listed in B's requiredby n times */ +/* n == 0 or 1 in almost all cases */
I failed to see why we would require to put the same package more than once in requiredby. I think it works that way, but it isn't necessary, is it? At least I couldn't find a pactest for a case where this would be needed. Is there one? Actually, I've nothing against these multiple requiredby, but I think Dan doesn't like it. I believe it's mostly because of that bug where requiredby were added wrongly multiple times. While in this case, having them multiple times would actually have a meaning (multiple depends), and anyway like you said, it's a corner case that would probably never happen in practice :) So I actually like this patch in its current state a lot (as well as the other one for add_commit). But I think these may still need some review/thoughts.
I failed to see why we would require to put the same package more than once in requiredby. I think it works that way, but it isn't necessary, is it? At least I couldn't find a pactest for a case where this would be needed. Is there one? Well, this is so rare as you said (AFAIK no real example yet) that you can simply ignore it (I just put that comment there to remember us). I've chosen this solution because this is not broken imho (the same function does the requiredby add/remove) and this wouldn't worth the O(number of requiredby entries) check for all inserting. Anyway, in the late future (pacman4 ;-) we should optimize the dependency storing... Bye, ngaba
---------------------------------------------------------------- This message was sent using IMP, the Internet Messaging Program.
On Tue, Jul 17, 2007 at 09:34:28PM +0200, ngaba@petra.hos.u-szeged.hu wrote:
I failed to see why we would require to put the same package more than once in requiredby. I think it works that way, but it isn't necessary, is it? At least I couldn't find a pactest for a case where this would be needed. Is there one? Well, this is so rare as you said (AFAIK no real example yet) that you can simply ignore it (I just put that comment there to remember us). I've chosen this solution because this is not broken imho (the same function does the requiredby add/remove) and this wouldn't worth the O(number of requiredby entries) check for all inserting.
That check isn't a big deal on the performance side, it's totally negligible compared to the rest. It's also a sanity check to avoid getting duplicated requiredby entries. But I find your solution elegant, and it extends cleanly to rare cases where you can have valid duplicated requiredby entries (which means that check doesn't really fit there anymore). I don't think that sanity check is very important anyway (there are probably much more important ones in other places that are missing). Besides, for the current broken databases that have invalid duplicated requiredby entries, it's probably very easy to find these, and it is very easy to fix them, by just reinstalling the package.
participants (2)
-
ngaba＠petra.hos.u-szeged.hu
-
Xavier