Hi Johannes, On Tue, Jul 11, 2006 at 10:41:44AM +0200, Johannes Winkelmann wrote:
Second - pkgutils have [security] flaw. One package can seriously damage whole system without any warnings. Is conflicts catching should be done in pkgutils or not is arguable, but we have some sort of it already, thus we should make it more consistent. I assume you're refering to #63 here, right? The symlink bug #15 doesn't qualify as a security bug in my opinion.
Right.
Also, does it really fix #15, i.e. follow symlinks? I might have looked over it too quickly, but I didn't find any indication.
I can't reproduce bug#15's test case, thus I assume it is fixed. If there another test case exist, I can test it and try to fix.
Have you though about other cases which could be potentially annoying to people, e.g. when they manually adjust permissions?
Maybe I should introduce DONTCARE_{DIR,FILE}_PERMISSIONS config entry later. But really I can't imagine the case when someone have to change permissions on say /var or /bin/ls. And if they doing it, with current pkgutils they are risking to lost their changes on the package update.
Other than that, if the potential "false positives" aren't too bad (analysis needed), it seems like a feature worth having and even adding a couple of lines. I haven't looked at the output it provides in case of a conflict, we should make sure it's clear to the user.
This patch fixes bugs mentioned in subject.
The patch is about the same size as summary sizes of all *.cc files. But! Please, don't be scared, there are really small amount of logic changes. Then it should be a small patch :-)
Mmm.. no. :-) That is a point of refactoring: you change small piece of code, and whole code needs to be changed here and there, because of initial change. Like an avalanche. If it was trivial change, yes it will be a small patch. But this is not so trivial change, but in the same it do not touch algorithms logic.
Most line-consuming changes are: new types, old types typedef'ed, long functions splitting, new debugging code, pkgadd.cc changes regarding indentation and long lines. I'd appreciate if you could split it up.
It's hard to do, because things is somewhat twisted. If it's really important you to read changes splitted, I will try.
I've reviewed it quickly, and while I like the ideas, some of the refactorings seem to be personal preference: the indentation changes, typedefs etc. and while this doesn't mean that those changes should be rejected, it seems more sensible to look at those changes isolated, and independent from the bug fix. If you could provide individual patches (debug changes, indentation, nesting removal etc.) we can decide on a per patch basis whether we agree there, and eventually come up with a merged version we can all live with (hopefully), and which supports mode-conflicts.
As a final remark, I find your use of typedefs slightly excessive here; I usually prefer to write the real type instead of looking up typedefs, especially as there are so many typedefs with similar names: file_t, files_t, filenames_t, fileconflict_t, fileconflicts_t I haven't really thought a lot about it yet, it just kinda "smells"; maybe some naming changes might help already.
Yes, I use and like typedefs. So do Per as far as I can see, because he introduced packages_t in pkgutil.h, but for unknown reason he do not use it in many places of code. I suppose he just don't find enough time to switch all set<> to proper types. This what I've done. For the most of time stl algorithms do not care which type you use, be it vector, deque or set. It's all containers, which is nameless, and it's up to programmer to name it. The code will rather ugly and obscure without typedefs. That is why typedef invented, btw. ;-)
Other than that, the patch is very clean, c++ish, and I look forward to discuss the changes in more detail.
Thanks.
Thanks for your work so far. Regards, Johannes
-- Anton (irc: bd2)