[pacman-dev] [PATCH] New feature: files verification

[changaco]

Dan McGee wrote :

> 1. Please enable the pre-commit hook in git:
> mv .git/hooks/pre-commit.sample .git/hooks/pre-commit
> This will ensure you don't have the following that I caught when
> applying the patch locally:
> 
> dmcgee at galway ~/projects/pacman (master)
> $ git am -3 < /tmp/pacman-verify.patch
> Applying: New feature: files verification
> /home/dmcgee/projects/pacman/.git/rebase-apply/patch:160: trailing whitespace.
> warning: 1 line adds whitespace errors.
Done.

> I think -k would be a better option, after seeing Aaron's comments.
> Why don't we change it to that instead.
Done. I replaced every "verify" by "check".

> You missed adding it to the query directions, somewhere around line
> 110 in pacman.c. We will also want to document it in the
> doc/pacman.8.txt manpage.
Yes I ignored the doc part when I wrote the code, it's done now.

> Please stick with the naming conventions in the rest of the code- no
> camel case necessary. Just 'st' is the preferred name for a struct
> stat. This also can be made local to the for(files) loop.
Fixed.

> No automatic GC in C. You are leaking like a sieve here:
> ==26832== 43,564 bytes in 1,512 blocks are definitely lost in loss record 3 of 3
> ==26832==    at 0x4C2391E: malloc (in
> /usr/lib/valgrind/amd64-linux/vgpreload_memcheck.so)
> ==26832==    by 0x40852C: verify (query.c:353)
> ==26832==    by 0x408B69: pacman_query (query.c:485)
> ==26832==    by 0x407A6B: main (pacman.c:944)
> 
> Valgrind is awesome. Compile pacman, run ./src/pacman/pacman at least
> once, then do the following:
> 
> dmcgee at galway ~/projects/pacman (master)
> $ valgrind ./src/pacman/.libs/lt-pacman -Qy glibc pacman-git
> 
> And it should give you a fairly concise summary of memory leaks. See
> here for more: http://www.toofishes.net/blog/using-valgrind-c-programming/
> 
Thanks, you're right it's awesome. :)

> This logic isn't always correct. See lib/libalpm/add.c, line 309:
> snprintf(filename, PATH_MAX, "%s%s", handle->root, entryname);
> 
> You cannot assume the root path is always '/'. In addition, it would
> probably be better to just use a static buffer of length PATH_MAX like
> that code does or you will be spending a tremendous amount of time
> mallocing (and currently leaking memory).
Fixed.

> What has files in /tmp?
Well they are temporary files so you can't consider that the package is damaged if they are not here.
I added this because of some fonts package which has files in /tmp.

> Hmm. I like the interactivity of this, but I'm not completely sure
> what you having going on here. You write to stderr but flush stdout?
Those were just errors due to the fact that I only wrote in stdout when I first wrote the code and I forgot to change.

> In addition, the '°'
> symbol means nothing to me- this could be a cultural thing though. :)
Yes I didn't realise it was French, and according to Wikipedia it isn't even correct in French ...
I removed it.

> It would probably also be best to use a format like we do in
> callback.c, line 399, which is something like this:
> checking package integrity...
> (1/1) checking for file conflicts                   [#####################] 100%
> (1/1) installing emacs                              [#####################] 100%
> 
> I am referring to the last two lines here. We can probably even use
> this callback function to do the dirty work of printing status and a
> progress bar for us.
> 
> I am also a fan of output only when errors happen, besides a progress
> bar. For instance, look at the output of testdb:
> $ testdb
> Checking the integrity of the local database in /var/lib/pacman/
> missing dependency for mysql : mysql-clients>=5.1.32
> 
> If there is any output at all, it is important as it is telling you problems.
If we go for progress bars then I guess it would be a line by package and the bar would be for the files.
I'll look into that and post a new version of the patch.

> Since we don't actually do any repair, can we just call pkgs2repair
> "damaged"? That also kills the numeral from the variable name, which I
> at least don't find very appealing.
>
> pkgs2repair is also never freed. Memory leak.
>
> Memory leak!
All fixed.

> I think your logic is a bit weird here, as you are trying to
> incorporate both filters and a verify- note that we end up displaying
> both pieces of information:
> 
> $ ./src/pacman/.libs/lt-pacman -Qy glibc pacman-git
> glibc 2.9-4
> pacman-git 20090116-1
> file n°1512 (package 2/2): OK
> No damaged packages.
> 
> We should probably only do verification and not print the names of the
> packages and run them through filters, unless anyone else has an
> opinion.
Good point, fixed.

> I commented a lot, but don't think I hate your patch. I definitely
> like this idea.
I get that and it's me who has to thank you for commenting because it helps me get better in C.