okay, some comments to the comments... On Thu, Nov 19, 2009 at 07:44:51AM +0100, Guillem Jover wrote: > On Mon, 2009-11-09 at 23:26:27 +0100, sean finney wrote: > > On Fri, Nov 06, 2009 at 08:48:43AM +0100, Guillem Jover wrote: > > > > +/** > > > > + * @file conffiledb.h > > > > + * > > > [...] > > > > + */ > > > > > > Missing blank line > > > > clarification: after what? > > Between the comment and the preprocessor directive. Okay, fixed. > > > > +void conffiledb_remove(const char *pkg, conffiledb_base base); > > > > > > > +void conffiledb_commit(const char *pkg, conffiledb_base from_base, > > > > + conffiledb_base to_base); > > > > > > These two should be conffile based and not pkg or base based. This way > > > we can guarantee atomicity, which we cannot when managing directories > > > as a whole. > > > > discussion: > > > > namely, while working on a conffile-by-conffile basis ensures atomicity > > at the file level, there's a bigger window where things can go wrong > > compared to renaming the directories. > > How so? there's more operations where disk/inodes could run out and time during which the user/system could kill the process, etc. and supporting being able to clean up from such a state seems a bit messy. so i was thinking on a less granular level wrt overall conffile db state. > > *however*, one idea did come > > up in the discussion with jonathan, that might be a nicer alternative. > > combining it with what you've suggested in this thread, how about > > the layout: > > > > <admindir>/conffiles/<pkg>/<pkgversion>/<hash>.<basetype> > > > > since we're passing struct pkginfo's around instead of package names, we > > get the version information for free. > > What would be the advantage? it's quite a bit simpler, for starters. using the version (which we get from the struct pkginfo) eliminates the need for having the new/current/tmp directories, and the files once unpacked never need to be moved, which kinda sidesteps the above atomicity problems; they all had to do with the "commit" operation, which using the above i believe would be a no-op. so this would result in removing a pretty hefty chunk of code from the patch as well as the atomicity problems afaict. namely the commit function would be gone, as well as anything related to the conffiledb_base and conffiledb_base_dir stuff. and wrt the symlinks issue: > > anyway, with > > a layout like above, in the <pkg> directory we could have symlinks which could > > be updated to point at the respective <pkgversion> subdirectory instead > > of actually moving files/directories around. what do you think? > > I think this just will complicate things a bit more, as you'd have to > take care of updating the symlinks, ensuring they don't ever become > dangling, and removing old files from the db, at which point you could > have just handled the files themselves and be done with it. the more i think about it the more i think they're not necessary. so i withdraw my suggestion for using them. if using the above layout, which directory was "current" etc can be determined implicitly by dpkg's knowledge of the state of the package (so having them would be merely reflecting information we already know). > > clarification: not sure what you mean here: between dpkg-db.h and main.h? > > Right. Okay, fixed. > > > Why are you storing resolved (in the merge patch), how are you planning > > > to use it afterwards? > > > > it was a suggestion from buxy or mrvn, at this point my memory is a bit > > fuzzy. if you don't think it's useful then it can be removed. > > I read those mails, but I'm not entirely sure that's what was suggested. could be that i mis-interpreted the suggestion. should i gut the stuff in the meantime, until i'm sufficiently cluebatted? > Well yeah, I agree that in general using descriptive names for > constants is better than just using the literals there, but in this > case I think it's more clear and compact to use the octal value, as > it's something we use pretty often when dealing with file system > perms on the command line on Unix systems, for example. Okay, "fixed". > Also regarding comments, too many, or too obvious can also be pretty > distracting. Usually if the code is self descriptive (small functions, > good named variables, etc) there should not be much need for many > inline comments. Of course there's always tricky sections or specific > pieces that might deserve explanations why it's done that way or some > overall logic, etc, but certainly not detailing what each line is > doing. Noted. I don't normally comment this verbosely but as the code in question would be up for discussion i wanted to make it as clear as possible what was going on in the different places. the _commit function was certainly overboard. > > this is just a general habit of mine... even when the variable > > is initialized a couple lines later. it doesn't have any cost > > since the compiler removes it and as a general practice prevents > > various refactorings/etc from removing what seems like an invariant. > > but fixed in this case, anyway. > > Personally I just find it distracting, as you have to work out if it > was intended or if there's any discrepancy which one is the right one. Okay, i'll "do as the romans" then :) sean
Attachment:
signature.asc
Description: Digital signature