[pacman-dev] [PATCH] New feature: files verification
Dan McGee
dpmcgee at gmail.com
Sun Mar 29 17:19:27 EDT 2009
On Sun, Mar 29, 2009 at 3:50 PM, changaco <changaco at laposte.net> wrote:
>
>> > 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.
>
>
>> Part of me would like to see:
>> Checking glibc 2.9-4... OK
>> Checking pacman-git 20090116... OK
>> ...etc...
>> Verification Complete
>> 1512 files checked (2 packages)
>>
>> But... that could be a lot of output. It might be cleaner to see:
>>
>> Checking for missing files for installed packages
>> # if any errors, output like:
>> pacman-git: Missing file /etc/pacman.conf
>> # endif
>> Verification Complete: 1512 files (2 packages)
>
> I tried with the progress bars and it's definitively too much output.
>
> I don't like when there is too much but on the other hand I like to know what is being done so my proposition is:
> (567/895) checking glibc ...
> overwritten each time to keep the output short and removed at the end because we don't care once it is finished.
>
> I propose to follow your propositions, Aaron, for both errors and end message:
> openldap: Missing file /var/lib/openldap/openldap-data/DB_CONFIG.example
> Check complete: 253202 files (988 packages)
>
> Then if there are damaged packages:
> Damaged packages: kdemod-kdewebdev-kommander openldap
>
> A complete example with no error could then be:
> Check complete: 253202 files (988 packages)
> pretty simple and compact don't you think ? :)
>
> and with errors:
> kdemod-kdewebdev-kommander: Missing file /usr/share/applnk/.hidden/
> kdemod-kdewebdev-kommander: Missing file /usr/share/applnk/.hidden/kommander.desktop
> openldap: Missing file /var/lib/openldap/openldap-data/
> openldap: Missing file /var/lib/openldap/openldap-data/DB_CONFIG.example
> Check complete: 253202 files (988 packages)
> Damaged packages: kdemod-kdewebdev-kommander openldap
>
> Note that this last example is what I actually get when running test as non-root user (denied access to the files) and the previous one what I get when running in root. I think we should probably force root, if you can point me where that should be done I could add it to the patch.
>
> Finally when adding the quiet option we get something like this:
> kdemod-kdewebdev-kommander /usr/share/applnk/.hidden/
> kdemod-kdewebdev-kommander /usr/share/applnk/.hidden/kommander.desktop
> openldap /var/lib/openldap/openldap-data/
> openldap /var/lib/openldap/openldap-data/DB_CONFIG.example
> kdemod-kdewebdev-kommander openldap
>
> If there are no errors then nothing is printed.
>
> Also note that the list of missing files is in stderr and the list of damaged package in stdout, this allows easy bash use of the output.
>
>
>
>
> Concerning memory leaks I got rid of them but now I get an error when pacman cleans up which seems to be due to the FREELIST(damaged) at the end of check() in pacman.c and I don't know what I can do.
> Here's what valgrind tells me:
> ==10905== Invalid free() / delete / delete[]
> ==10905== at 0x4023E3A: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
> ==10905== by 0x403FBC4: _alpm_pkg_free (package.c:825)
> ==10905== by 0x403001E: alpm_list_free_inner (alpm_list.c:71)
> ==10905== by 0x4035411: _alpm_db_free_pkgcache (cache.c:72)
> ==10905== by 0x4037AA8: _alpm_db_free (db.c:345)
> ==10905== by 0x4037B6C: _alpm_db_unregister (db.c:89)
> ==10905== by 0x4037BEC: alpm_db_unregister_all (db.c:107)
> ==10905== by 0x402FE95: alpm_release (alpm.c:68)
> ==10905== by 0x804D888: cleanup (pacman.c:202)
> ==10905== by 0x804E864: main (pacman.c:960)
> ==10905== Address 0x42771f0 is 0 bytes inside a block of size 27 free'd
> ==10905== at 0x4023E3A: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
> ==10905== by 0x403001E: alpm_list_free_inner (alpm_list.c:71)
> ==10905== by 0x804F8A6: check (query.c:402)
> ==10905== by 0x805010E: pacman_query (query.c:441)
> ==10905== by 0x804EA84: main (pacman.c:947)
> ==10905==
> ==10905== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 27 from 1)
> ==10905== malloc/free: in use at exit: 0 bytes in 0 blocks.
> ==10905== malloc/free: 516,031 allocs, 516,033 frees, 15,576,777 bytes allocated.
> ==10905== For counts of detected errors, rerun with: -v
> ==10905== All heap blocks were freed -- no leaks are possible.
>
>
Nice! I'll address it below.
$ ./src/pacman/pacman -Qk glibc
Check complete: 1407 files (1 packages)
*** glibc detected ***
/home/dmcgee/projects/pacman/src/pacman/.libs/lt-pacman:
munmap_chunk(): invalid pointer: 0x0000000000814780 ***
======= Backtrace: =========
/lib/libc.so.6[0x7f9e4c7eb8b8]
/home/dmcgee/projects/pacman/lib/libalpm/.libs/libalpm.so.3[0x7f9e4d954153]
/home/dmcgee/projects/pacman/lib/libalpm/.libs/libalpm.so.3(alpm_list_free_inner+0x3a)[0x7f9e4d9453aa]
/home/dmcgee/projects/pacman/lib/libalpm/.libs/libalpm.so.3[0x7f9e4d949967]
/home/dmcgee/projects/pacman/lib/libalpm/.libs/libalpm.so.3[0x7f9e4d94bbb8]
/home/dmcgee/projects/pacman/lib/libalpm/.libs/libalpm.so.3(alpm_db_unregister_all+0x55)[0x7f9e4d94bce5]
/home/dmcgee/projects/pacman/lib/libalpm/.libs/libalpm.so.3(alpm_release+0x3f)[0x7f9e4d94522f]
/home/dmcgee/projects/pacman/src/pacman/.libs/lt-pacman[0x406a4c]
/home/dmcgee/projects/pacman/src/pacman/.libs/lt-pacman[0x407885]
/lib/libc.so.6(__libc_start_main+0xe6)[0x7f9e4c797546]
/home/dmcgee/projects/pacman/src/pacman/.libs/lt-pacman[0x404a99]
> Here's the new patch:
Please don't do this in the future. Instead, reply to this email with
the patch (git-send-email prompts you for a message ID which is a
handy feature). Otherwise all of the above text ends up in the commit
message when I try to use git-am to automate applying patches.
>
>
>
>
> >From 6c1cac35ce62d7f97bdc65c410b575a718ef6d5f Mon Sep 17 00:00:00 2001
> From: Charly COSTE <changaco at laposte.net>
> Date: Sun, 29 Mar 2009 22:04:00 +0200
> Subject: [PATCH] New feature: files verification
>
> A new option "-Qk" which checks if the files owned by a/some/all package(s) really are on the system (i.e. not accidentally deleted).
>
> Signed-off-by: Charly COSTE <changaco at laposte.net>
> ---
> doc/pacman.8.txt | 5 ++
> src/pacman/conf.h | 1 +
> src/pacman/pacman.c | 11 ++++-
> src/pacman/query.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++-----
> 4 files changed, 108 insertions(+), 12 deletions(-)
>
> diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt
> index c574872..f76b7ad 100644
> --- a/doc/pacman.8.txt
> +++ b/doc/pacman.8.txt
> @@ -181,6 +181,11 @@ Query Options[[QO]]
> '\--info' or '-i' flags will also display the list of backup files and
> their modification states.
>
> +*-k \--check*::
> + Check that the files owned by the package(s) are present on the system,
> + that they haven't been deleted. Given no argument it will check all the
> + packages installed.
Redundant language, but your native language is not English, so no worries.
How about:
Check that all files owned by the given packages(s) are present on the
system. If packages are not specified, check all installed packages.
> +
> *-l, \--list*::
> List all files owned by a given package. Multiple packages can be
> specified on the command line.
> diff --git a/src/pacman/conf.h b/src/pacman/conf.h
> index 466d983..50907db 100644
> --- a/src/pacman/conf.h
> +++ b/src/pacman/conf.h
> @@ -50,6 +50,7 @@ typedef struct __config_t {
> unsigned short op_q_search;
> unsigned short op_q_changelog;
> unsigned short op_q_upgrade;
> + unsigned short op_q_check;
>
> unsigned short op_s_clean;
> unsigned short op_s_downloadonly;
> diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
> index 59916d6..8a54d6e 100644
> --- a/src/pacman/pacman.c
> +++ b/src/pacman/pacman.c
> @@ -107,6 +107,7 @@ static void usage(int op, const char * const myname)
> printf(_(" -e, --explicit list all packages explicitly installed\n"));
> printf(_(" -g, --groups view all members of a package group\n"));
> printf(_(" -i, --info view package information (-ii for backup files)\n"));
> + printf(_(" -k, --check check that the files owned by the package(s) are present\n"));
> printf(_(" -l, --list list the contents of the queried package\n"));
> printf(_(" -m, --foreign list installed packages not found in sync db(s)\n"));
> printf(_(" -o, --owns <file> query the package that owns <file>\n"));
> @@ -343,6 +344,7 @@ static int parseargs(int argc, char *argv[])
> {"help", no_argument, 0, 'h'},
> {"info", no_argument, 0, 'i'},
> {"dbonly", no_argument, 0, 'k'},
> + {"check", no_argument, 0, 'k'},
> {"list", no_argument, 0, 'l'},
> {"foreign", no_argument, 0, 'm'},
> {"nosave", no_argument, 0, 'n'},
> @@ -471,7 +473,10 @@ static int parseargs(int argc, char *argv[])
> case 'g': (config->group)++; break;
> case 'h': config->help = 1; break;
> case 'i': (config->op_q_info)++; (config->op_s_info)++; break;
> - case 'k': config->flags |= PM_TRANS_FLAG_DBONLY; break;
> + case 'k':
> + config->flags |= PM_TRANS_FLAG_DBONLY;
> + config->op_q_check = 1;
> + break;
> case 'l': config->op_q_list = 1; break;
> case 'm': config->op_q_foreign = 1; break;
> case 'n': config->flags |= PM_TRANS_FLAG_NOSAVE; break;
> @@ -510,7 +515,9 @@ static int parseargs(int argc, char *argv[])
> config->flags |= PM_TRANS_FLAG_DOWNLOADONLY;
> config->flags |= PM_TRANS_FLAG_NOCONFLICTS;
> break;
> - case 'y': (config->op_s_sync)++; break;
> + case 'y':
> + (config->op_s_sync)++;
> + break;
Unnecessary change now, let's just revert these to the original line.
> case '?': return(1);
> default: return(1);
> }
> diff --git a/src/pacman/query.c b/src/pacman/query.c
> index 0d48638..6ec58f5 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -35,6 +35,7 @@
> #include "package.h"
> #include "conf.h"
> #include "util.h"
> +#include "callback.h"
>
> extern pmdb_t *db_local;
>
> @@ -334,10 +335,79 @@ static void display(pmpkg_t *pkg)
> }
> }
>
> +static void check_display_progress(int j, int pkgs_n, const char *pkgname){
> + char message[getcols()];
> + int len = snprintf(message, getcols(), "(%i/%i) %s %s ...", (int)j, (int)pkgs_n, _("checking"), pkgname);
> + fprintf(stderr, "\r%s %*s", message, getcols()-len-1, " ");
> + fflush(stderr);
> +}
> +
> +// We loop through the packages and for each packages we loop through files to check if they exist
> +static void check(alpm_list_t *pkgs)
> +{
> + alpm_list_t *i, *files, *file, *damaged = NULL;
> + pmpkg_t *pkg;
> + int pkgs_n = alpm_list_count(pkgs);
> + int j = 0, allfiles = 0;
> + for(i = pkgs; i; i = alpm_list_next(i)) {
> + j++;
> + pkg = alpm_list_getdata(i);
> + const char *pkgname = alpm_pkg_get_name(pkg);
> + if(filter(pkg)) {
> + files = alpm_pkg_get_files(pkg);
> + if(!config->quiet) {
> + check_display_progress(j, pkgs_n, pkgname);
> + }
> + for(file = files; file; file = alpm_list_next(file)){
> + struct stat st;
> + char *x = alpm_list_getdata(file);
> + char f[PATH_MAX];
> + snprintf(f, PATH_MAX, "%s%s", alpm_option_get_root(), x);
It will be slightly more efficient if you call alpm_option_get_root()
once outside of the for loops, and then use the pointer returned here
instead.
> + allfiles++;
> + if(strncmp(f, "/tmp", 4)==0){
> + continue; // ignore files in /tmp
> + }
Did you explain why this is necessary? I think all special case
handling should be dropped, and this check is not correct anyway as it
assumes a root of '/'.
> + if(lstat(f,&st) != 0) { // we use lstat to prevent errors from symbolic links
> + if(config->quiet) {
> + fprintf(stderr, "%s %s\n", pkgname, f);
> + fflush(stderr);
> + } else {
> + fprintf(stderr, "\r%s: %s %s\n", pkgname, _("Missing file"), f);
> + check_display_progress(j, pkgs_n, pkgname);
> + fflush(stderr);
> + }
> + if(alpm_list_find_ptr(damaged, pkgname) == NULL) {
> + damaged = alpm_list_add(damaged, (void*)pkgname);
> + }
> + }
> + }
> + }
> + }
> + if(!config->quiet) {
> + char message[getcols()];
> + int len = snprintf(message, getcols(), "\r%s: %i %s (%i %s)\n", _("Check complete"), allfiles, _("files"), pkgs_n, _("packages"));
> + fprintf(stderr, "\r%s %*s", message, getcols()-len-1, " ");
> + fflush(stderr);
> + }
> + if(alpm_list_count(damaged) > 0) {
> + if(!config->quiet){
> + fprintf(stderr, "\r%s ", _("Damaged packages:"));
> + fflush(stderr);
> + }
> + for(i = damaged; i; i = alpm_list_next(i)) {
> + fprintf(stdout, "%s ", (char*)alpm_list_getdata(i));
> + }
> + fprintf(stdout, "\n");
> + fflush(stdout);
> + }
> +
> + FREELIST(damaged);
You get your double free() because you definitely don't want to be
using this macro. What this macro does is free both the list objects
AND their contents. However, when you build the list, you are not
duplicating the list contents, so freeing the list contents here is
something we don't want to do- we only want to free the list itself.
This line should just be a:
alpm_list_free(damaged);
> +}
> +
> int pacman_query(alpm_list_t *targets)
> {
> int ret = 0;
> - alpm_list_t *i;
> + alpm_list_t *i, *pkgs = NULL;
>
> /* First: operations that do not require targets */
>
> @@ -363,18 +433,20 @@ int pacman_query(alpm_list_t *targets)
> }
>
> /* operations on all packages in the local DB
> - * valid: no-op (plain -Q), list, info
> + * valid: no-op (plain -Q), list, info, check
> * invalid: isfile, owns */
> if(targets == NULL) {
> if(config->op_q_isfile || config->op_q_owns) {
> pm_printf(PM_LOG_ERROR, _("no targets specified (use -h for help)\n"));
> return(1);
> - }
> -
> - for(i = alpm_db_get_pkgcache(db_local); i; i = alpm_list_next(i)) {
> - pmpkg_t *pkg = alpm_list_getdata(i);
> - if(filter(pkg)) {
> - display(pkg);
> + } else if(config->op_q_check) {
> + check(alpm_db_get_pkgcache(db_local));
> + } else {
> + for(i = alpm_db_get_pkgcache(db_local); i; i = alpm_list_next(i)) {
> + pmpkg_t *pkg = alpm_list_getdata(i);
> + if(filter(pkg)) {
> + display(pkg);
> + }
> }
> }
> return(0);
> @@ -389,7 +461,7 @@ int pacman_query(alpm_list_t *targets)
> }
>
> /* operations on named packages in the local DB
> - * valid: no-op (plain -Q), list, info */
> + * valid: no-op (plain -Q), list, info, check */
> for(i = targets; i; i = alpm_list_next(i)) {
> char *strname = alpm_list_getdata(i);
> pmpkg_t *pkg = NULL;
> @@ -406,8 +478,14 @@ int pacman_query(alpm_list_t *targets)
> continue;
> }
>
> +
> if(filter(pkg)) {
> - display(pkg);
> + if(config->op_q_check){
> + pkgs = alpm_list_add(pkgs, pkg);
> + }
> + else{
Add a space before the { and put the "} else {" on one line.
> + display(pkg);
> + }
> }
So with this logic, we end up calling filter() twice for every
package, as you also called it in the check() function above. I
believe that is not necessary, and the filter() call in check should
be dropped. However, that breaks the no explicit packages logic.
I'm not sure how you want to fix or clean it up, but we shouldn't filter twice.
>
> if(config->op_q_isfile) {
> @@ -415,6 +493,11 @@ int pacman_query(alpm_list_t *targets)
> pkg = NULL;
> }
> }
> + if(config->op_q_check){
> + check(pkgs);
> + }
> +
> + FREELIST(pkgs);
s/FREELIST/alpm_list_free/
And this free can really be moved inside the if() statement right
above it, as any adds are protected by the same case.
>
> return(ret);
> }
> --
> 1.6.2.1
A few random tests:
dmcgee at galway ~/projects/pacman (master)
$ ./src/pacman/pacman -Qtdk
Check complete: 1296 files (693 packages)
The packages count is definitely wrong there, and looks like it is
wrong in several other filter tests (e.g. pacman -Qtek).
I'm still not a fan of the flickering progress bar. After you make a
few of these other adjustments I may see what I can come up with.
-Dan
More information about the pacman-dev
mailing list