11 Jul
2006
11 Jul
'06
5:43 p.m.
Hi,
On Tue, Jul 11, 2006 at 19:24:48 +0400, Anton wrote:
> Hi Johannes,
>
> On Tue, Jul 11, 2006 at 10:41:44AM +0200, Johannes Winkelmann wrote:
[...]
> > 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.
Having thought about it for a while I think there's a whole family of
mode changes which would cause an unwanted conflict:
- a port changing one of its binaries to be suid root (or not suid root
anymore), or any other mode change
- a port (e.g. a daemon) changing to be run as nobody from root, thus
requiring its data file to be owned by nobody:nobody
- same problem: a port starting to use a particular group (think: video,
audio etc.) which previously used 'root'
- human errors where a directory wasn't writable or a file not
executable, and fixed by the maintainer by changing a mode/ownership
of some files or directories
Especially the last is likely to happen to us also in the future, and
the current version of pkgutils handles those wanted and intentional
mode/ownership changes gracefully (which is a nice way to say it, it
obviously just ignores it).
Considering that, I'm not sure if the conflict mode should be on by
default. It definitely makes sense for maintainers to avoid such mode
changes by mistake, and it protects users from malicious ports, however
I'm not convinced yet whether protecting from the theoretical thread is
worth removing the functionality to make fixes to mode/ownership without
special interaction.
> > > 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: [...]
Well, I meant the bugfix which could be a small(er) patch when separated
from the refactorings.
> > > 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.
I'd tend to disagree that it's hard, but maybe I'm looking at it from a
different point of view: From a quick glance, I see the following quite
distinct refactorings:
- replace NDEBUG/cerr with dbg macros
- introduce typedefs for existing template types
- extract method in config parsing
- extract resolve_conflicts()
- make make_keep_list a template
- extract pkginfo_t out of pkgutil class
- change indentation in void pkgadd::run() which looks like it could be
done before any other changes (didn't investigate that closer,
though).
In addition, there's the bugfix itself. That would be eight patches
which all introduce one change (be it logical or cosmetical), and none
of them would be 200+ lines.
> If it's really
> important you to read changes splitted, I will try.
It seems quite important, for two reasons:
- isolated changes are easier to understand, and therefore potential
problems/errors are easier to spot
- we can reject one particular change without rejecting the whole
feature
[...]
> 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.
Or one could say that he decided to use the proper types throughout the
code, and forgot to switch the package_t typedef. Only he can tell.
> 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.
I can only agree to some extend here. While the algorithm might work
either way, it can still have a performance impact. So if efficiency is
a criteria, the type matters. Also, depending on the problem it matters
a lot whether a container allows duplicate values etc.
In those cases, it's important to know which type we're dealing with,
and my argument is that it's easier to keep track of type if they're not
hidden by typedefs.
> The code will rather ugly and obscure without typedefs. That is why typedef
> invented, btw. ;-)
Well, if I see
std::pair<string, string> file;
I can immediately tell the type of 'file'. If it's
file_t file
I have to look it up.
So if you ask me, it's the typedefs which are obscure, since they hide
information. Don't get me wrong, typedefs are handy, but in cases like
- void db_rm_pkg(const string& name, const set<string>& keep_list);
+ void db_rm_pkg(const string& name, const filenames_t& keep_list);
I strongly prefer the explicit form. Also, for the record, I don't
consider the template notation ugly, although I'm aware that not
everyone shares this opinion :-).
Regards, Johannes
--
Johannes Winkelmann mailto:jw@smts.ch
Zurich, Switzerland http://jw.smts.ch