[pacman-dev] [PATCH] New feature: files verification
From 7c0348fa499f55968764fef4c7bb3d3d7245932b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Sat, 28 Mar 2009 12:36:23 +0100 Subject: [PATCH] New feature: files verification
A new option "-Qy" which checks if the files owned by a/some/all package(s) really are on the system (i.e. not accidentally deleted). http://bugs.archlinux.org/task/13877 Signed-off-by: Charly COSTE <changaco@laposte.net> --- src/pacman/conf.h | 1 + src/pacman/pacman.c | 6 +++- src/pacman/query.c | 87 +++++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 466d983..01a1828 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_verify; unsigned short op_s_clean; unsigned short op_s_downloadonly; diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 59916d6..e1d13a4 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -360,6 +360,7 @@ static int parseargs(int argc, char *argv[]) {"verbose", no_argument, 0, 'v'}, {"downloadonly", no_argument, 0, 'w'}, {"refresh", no_argument, 0, 'y'}, + {"verify", no_argument, 0, 'y'}, {"noconfirm", no_argument, 0, 1000}, {"config", required_argument, 0, 1001}, {"ignore", required_argument, 0, 1002}, @@ -510,7 +511,10 @@ 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)++; + config->op_q_verify = 1; + break; case '?': return(1); default: return(1); } diff --git a/src/pacman/query.c b/src/pacman/query.c index 0d48638..2a7dba5 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -334,10 +334,70 @@ static void display(pmpkg_t *pkg) } } +// We loop through the packages and for each packages we loop through files to check if they exist +static void verify(alpm_list_t *pkgs) +{ + alpm_list_t *i, *files, *file, *pkgs2repair; + pmpkg_t *pkg; + struct stat stFileInfo; + int pkgs_n = alpm_list_count(pkgs); + int j = 0, k = 0; + for(i = pkgs; i; i = alpm_list_next(i)) { + j++; + pkg = alpm_list_getdata(i); + char *pkgname = alpm_pkg_get_name(pkg); + if(filter(pkg)) { + files = alpm_pkg_get_files(pkg); + for(file = files; file; file = alpm_list_next(file)){ + char *x = alpm_list_getdata(file); + char *f = malloc (strlen(x)+2); + strcpy(f,"/"); + strcat(f, x); + k++; + if(strncmp(f, "/tmp", 4)==0){ + continue; // ignore files in /tmp + } + if(lstat(f,&stFileInfo) == 0) { // we use lstat to prevent errors from symbolic links + if(!config->quiet){ + fprintf(stderr, "\rfile n°%i (package %i/%i): OK", k, j, pkgs_n); fflush(stdout); + } + } + else { + if(config->quiet){ + fprintf(stderr, "%s %s\n", pkgname, f); fflush(stdout); + } else { + fprintf(stderr, "\rfile n°%i (package %i/%i): Missing file owned by \"%s\": %s\n", k, j, pkgs_n, pkgname, f); fflush(stdout); + } + if(alpm_list_find_ptr(pkgs2repair, pkgname) == NULL) { + pkgs2repair = alpm_list_add(pkgs2repair, pkgname); + } + } + } + } + } + if(!config->quiet){ + fprintf(stderr, "\n"); + } + if(alpm_list_count(pkgs2repair) > 0) { + if(!config->quiet){ + fprintf(stderr, "Damaged packages: "); + } + for(i = pkgs2repair; i; i = alpm_list_next(i)) { + fprintf(stdout, "%s ", alpm_list_getdata(i)); + } + } else { + if(!config->quiet){ + fprintf(stderr, "No damaged packages."); + } + } + fprintf(stdout, "\n"); + fflush(stdout); +} + int pacman_query(alpm_list_t *targets) { int ret = 0; - alpm_list_t *i; + alpm_list_t *i, *pkgs; /* First: operations that do not require targets */ @@ -363,18 +423,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, verify * 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_verify) { + verify(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 +451,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, verify */ for(i = targets; i; i = alpm_list_next(i)) { char *strname = alpm_list_getdata(i); pmpkg_t *pkg = NULL; @@ -406,8 +468,12 @@ int pacman_query(alpm_list_t *targets) continue; } + if(filter(pkg)) { display(pkg); + if(config->op_q_verify){ + pkgs = alpm_list_add(pkgs, pkg); + } } if(config->op_q_isfile) { @@ -415,6 +481,9 @@ int pacman_query(alpm_list_t *targets) pkg = NULL; } } + if(config->op_q_verify){ + verify(pkgs); + } return(ret); } -- 1.6.2.1 Créez votre adresse électronique prenom.nom@laposte.net 1 Go d'espace de stockage, anti-spam et anti-virus intégrés.
On Sat, Mar 28, 2009 at 8:52 AM, changaco <changaco@laposte.net> wrote:
From 7c0348fa499f55968764fef4c7bb3d3d7245932b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Sat, 28 Mar 2009 12:36:23 +0100 Subject: [PATCH] New feature: files verification
A new option "-Qy" which checks if the files owned by a/some/all package(s) really are on the system (i.e. not accidentally deleted). http://bugs.archlinux.org/task/13877
I like this, in theory, but I wonder if "y" would be confusing, as the user may expect -Qy to do something similar to -Sy
On Sat, Mar 28, 2009 at 8:52 AM, changaco <changaco@laposte.net> wrote:
From 7c0348fa499f55968764fef4c7bb3d3d7245932b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Sat, 28 Mar 2009 12:36:23 +0100 Subject: [PATCH] New feature: files verification
A new option "-Qy" which checks if the files owned by a/some/all package(s) really are on the system (i.e. not accidentally deleted). http://bugs.archlinux.org/task/13877
I like this, in theory, but I wonder if "y" would be confusing, as the user may expect -Qy to do something similar to -Sy
Firstly I wanted to say that I'm just a student and that it is the first patch I make so thank you for the support. Secondly about the -Qy problem I thought of that afterwards, maybe -Qk standing for the last letter of "check" would be better. That is an easy change to make. Finally, my main concern about this patch is that it works great when querying the entire database (i.e. "pacman -Qy") but I get a segmentation fault when querying some given packages (i.e. "pacman -Qy glibc pacman"). As I'm not an expert in C I was hoping that if somebody had some time he could help me fix this.
On Sat, Mar 28, 2009 at 8:52 AM, changaco <changaco@laposte.net> wrote:
From 7c0348fa499f55968764fef4c7bb3d3d7245932b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Sat, 28 Mar 2009 12:36:23 +0100 Subject: [PATCH] New feature: files verification
A new option "-Qy" which checks if the files owned by a/some/all package(s) really are on the system (i.e. not accidentally deleted). http://bugs.archlinux.org/task/13877
Signed-off-by: Charly COSTE <changaco@laposte.net>
Two initial comments before we get into the meat of the patch: 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@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. 2. Ensure you build with ./configure --enable-debug, which turns on the gcc -Wall -Werror options: cc1: warnings being treated as errors query.c: In function ‘verify’: query.c:348: error: initialization discards qualifiers from pointer target type (you should make this a 'const') query.c:386: error: format ‘%s’ expects type ‘char *’, but argument 3 has type ‘void *’ (you need an explicit (char *) cast here) make[2]: *** [query.o] Error 1 You'll see a few more errors pop up after fixing this, so you've been warned. :P
--- src/pacman/conf.h | 1 + src/pacman/pacman.c | 6 +++- src/pacman/query.c | 87 +++++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 84 insertions(+), 10 deletions(-)
diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 466d983..01a1828 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_verify;
unsigned short op_s_clean; unsigned short op_s_downloadonly; diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 59916d6..e1d13a4 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -360,6 +360,7 @@ static int parseargs(int argc, char *argv[]) {"verbose", no_argument, 0, 'v'}, {"downloadonly", no_argument, 0, 'w'}, {"refresh", no_argument, 0, 'y'}, + {"verify", no_argument, 0, 'y'}, I think -k would be a better option, after seeing Aaron's comments. Why don't we change it to that instead.
{"noconfirm", no_argument, 0, 1000}, {"config", required_argument, 0, 1001}, {"ignore", required_argument, 0, 1002}, @@ -510,7 +511,10 @@ 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)++; + config->op_q_verify = 1; + break; case '?': return(1); default: return(1); }
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.
diff --git a/src/pacman/query.c b/src/pacman/query.c index 0d48638..2a7dba5 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -334,10 +334,70 @@ static void display(pmpkg_t *pkg) } }
+// We loop through the packages and for each packages we loop through files to check if they exist +static void verify(alpm_list_t *pkgs) +{ + alpm_list_t *i, *files, *file, *pkgs2repair; Some of these will be uninitialized on their first access, but using ./configure --enable-debug will show you this.
+ pmpkg_t *pkg; + struct stat stFileInfo; 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.
+ int pkgs_n = alpm_list_count(pkgs); + int j = 0, k = 0; + for(i = pkgs; i; i = alpm_list_next(i)) { + j++; + pkg = alpm_list_getdata(i); + char *pkgname = alpm_pkg_get_name(pkg); + if(filter(pkg)) { + files = alpm_pkg_get_files(pkg); + for(file = files; file; file = alpm_list_next(file)){ + char *x = alpm_list_getdata(file); + char *f = malloc (strlen(x)+2); 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@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/
+ strcpy(f,"/"); + strcat(f, x); 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).
+ k++; + if(strncmp(f, "/tmp", 4)==0){ + continue; // ignore files in /tmp + } What has files in /tmp?
+ if(lstat(f,&stFileInfo) == 0) { // we use lstat to prevent errors from symbolic links + if(!config->quiet){ + fprintf(stderr, "\rfile n°%i (package %i/%i): OK", k, j, pkgs_n); fflush(stdout); 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? (And keep statements to one per line, please.) In addition, the '°' symbol means nothing to me- this could be a cultural thing though. :)
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.
+ } + } + else { + if(config->quiet){ + fprintf(stderr, "%s %s\n", pkgname, f); fflush(stdout); + } else { + fprintf(stderr, "\rfile n°%i (package %i/%i): Missing file owned by \"%s\": %s\n", k, j, pkgs_n, pkgname, f); fflush(stdout); + } + if(alpm_list_find_ptr(pkgs2repair, pkgname) == NULL) { + pkgs2repair = alpm_list_add(pkgs2repair, pkgname); + } + } + } + } + } + if(!config->quiet){ + fprintf(stderr, "\n"); + } + if(alpm_list_count(pkgs2repair) > 0) { + if(!config->quiet){ + fprintf(stderr, "Damaged packages: "); + } + for(i = pkgs2repair; i; i = alpm_list_next(i)) { 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.
+ fprintf(stdout, "%s ", alpm_list_getdata(i)); + } pkgs2repair is also never freed. Memory leak.
+ } else { + if(!config->quiet){ + fprintf(stderr, "No damaged packages."); + } + } + fprintf(stdout, "\n"); + fflush(stdout); +} + int pacman_query(alpm_list_t *targets) { int ret = 0; - alpm_list_t *i; + alpm_list_t *i, *pkgs;
/* First: operations that do not require targets */
@@ -363,18 +423,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, verify * 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_verify) { + verify(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 +451,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, verify */ for(i = targets; i; i = alpm_list_next(i)) { char *strname = alpm_list_getdata(i); pmpkg_t *pkg = NULL; @@ -406,8 +468,12 @@ int pacman_query(alpm_list_t *targets) continue; }
+ if(filter(pkg)) { display(pkg); + if(config->op_q_verify){ + pkgs = alpm_list_add(pkgs, pkg); Memory leak! + } }
if(config->op_q_isfile) { @@ -415,6 +481,9 @@ int pacman_query(alpm_list_t *targets) pkg = NULL; } } + if(config->op_q_verify){ + verify(pkgs); + }
return(ret); }
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. I commented a lot, but don't think I hate your patch. I definitely like this idea. -Dan
On Sat, Mar 28, 2009 at 4:12 PM, Dan McGee <dpmcgee@gmail.com> wrote:
$ ./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.
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)
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@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@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.
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. Here's the new patch: From 6c1cac35ce62d7f97bdc65c410b575a718ef6d5f Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@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@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. + *-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; 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); + allfiles++; + if(strncmp(f, "/tmp", 4)==0){ + continue; // ignore files in /tmp + } + 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); +} + 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{ + display(pkg); + } } 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); return(ret); } -- 1.6.2.1
On Sun, Mar 29, 2009 at 3:50 PM, changaco <changaco@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@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@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@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
On Sun, Mar 29, 2009 at 4:19 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Sun, Mar 29, 2009 at 3:50 PM, changaco <changaco@laposte.net> wrote:
Here's the new patch:
I made a few changes of my own to your patch, I'll throw them in below just as an FYI. The biggest change was killing snprintf() as that was the bottleneck when looping files. This is now fast as heck when all file inodes are in the kernel cache. I also converted your // comments to the /* */ style, which is all we use in the codebase. dmcgee@galway ~/projects/pacman (master) $ time ./src/pacman/.libs/lt-pacman -Qk filesystem: Missing file /var/games/ Check complete: 181758 files (693 packages) Damaged packages: filesystem real 0m0.583s user 0m0.113s sys 0m0.413s $ git diff | cat diff --git a/src/pacman/query.c b/src/pacman/query.c index 6ec58f5..db29fe0 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -342,13 +342,27 @@ static void check_display_progress(int j, int pkgs_n, const char *pkgname){ fflush(stderr); } -// We loop through the packages and for each packages we loop through files to check if they exist +/* Loop through the packages. For each package, + * 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; + const char *root; + int pkgs_n, j = 0, allfiles = 0; + size_t rootlen; + char f[PATH_MAX]; + + pkgs_n = alpm_list_count(pkgs); + root = alpm_option_get_root(); + rootlen = strlen(root); + if(rootlen + 1 > PATH_MAX) { + /* we are in trouble here */ + pm_printf(PM_LOG_ERROR, _("root path too long\n")); + return; + } + strcpy(f, root); + for(i = pkgs; i; i = alpm_list_next(i)) { j++; pkg = alpm_list_getdata(i); @@ -361,13 +375,14 @@ static void check(alpm_list_t *pkgs) 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); - allfiles++; - if(strncmp(f, "/tmp", 4)==0){ - continue; // ignore files in /tmp + if(rootlen + 1 + strlen(x) > PATH_MAX) { + pm_printf(PM_LOG_WARNING, _("file path too long\n")); + continue; } - if(lstat(f,&st) != 0) { // we use lstat to prevent errors from symbolic links + strcpy(f + rootlen, x); + allfiles++; + /* use lstat to prevent errors from symlinks */ + if(lstat(f, &st) != 0) { if(config->quiet) { fprintf(stderr, "%s %s\n", pkgname, f); fflush(stderr); @@ -377,7 +392,7 @@ static void check(alpm_list_t *pkgs) fflush(stderr); } if(alpm_list_find_ptr(damaged, pkgname) == NULL) { - damaged = alpm_list_add(damaged, (void*)pkgname); + damaged = alpm_list_add(damaged, (char*)pkgname); } } } @@ -385,7 +400,8 @@ static void check(alpm_list_t *pkgs) } 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")); + 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); } @@ -401,7 +417,7 @@ static void check(alpm_list_t *pkgs) fflush(stdout); } - FREELIST(damaged); + alpm_list_free(damaged); } int pacman_query(alpm_list_t *targets) @@ -482,8 +498,7 @@ int pacman_query(alpm_list_t *targets) if(filter(pkg)) { if(config->op_q_check){ pkgs = alpm_list_add(pkgs, pkg); - } - else{ + } else { display(pkg); } } @@ -493,11 +508,12 @@ int pacman_query(alpm_list_t *targets) pkg = NULL; } } + if(config->op_q_check){ check(pkgs); + alpm_list_free(pkgs); } - FREELIST(pkgs); return(ret); }
From ee827c85cb9c4cd42ff42d8d624923076d62459b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Mon, 30 Mar 2009 18:48:15 +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@laposte.net> --- doc/pacman.8.txt | 4 ++ src/pacman/conf.h | 1 + src/pacman/pacman.c | 7 +++- src/pacman/query.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 126 insertions(+), 8 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index c574872..f208816 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -181,6 +181,10 @@ Query Options[[QO]] '\--info' or '-i' flags will also display the list of backup files and their modification states. +*-k \--check*:: + Check that all files owned by the given package(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..f6efce3 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; diff --git a/src/pacman/query.c b/src/pacman/query.c index 0d48638..c0d8df0 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -334,10 +334,102 @@ 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); +} + +/* Loop through the packages. For each package, + * 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; + const char *root; + int pkgs_n, j = 0, allfiles = 0; + size_t rootlen; + char f[PATH_MAX]; + char tmp[PATH_MAX]; + + pkgs_n = alpm_list_count(pkgs); + root = alpm_option_get_root(); + rootlen = strlen(root); + if(rootlen + 1 > PATH_MAX) { + /* we are in trouble here */ + pm_printf(PM_LOG_ERROR, _("root path too long\n")); + return; + } + strcpy(f, root); + strcpy(tmp, root); + strcpy(tmp + rootlen, "tmp"); + int tmplen = strlen(tmp); + + for(i = pkgs; i; i = alpm_list_next(i)) { + j++; + pkg = alpm_list_getdata(i); + const char *pkgname = alpm_pkg_get_name(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); + if(rootlen + 1 + strlen(x) > PATH_MAX) { + pm_printf(PM_LOG_WARNING, _("file path too long\n")); + continue; + } + strcpy(f + rootlen, x); + allfiles++; + if(strncmp(f, tmp, tmplen)==0){ + continue; + } + /* use lstat to prevent errors from symlinks */ + if(lstat(f,&st) != 0) { + 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, (char*)pkgname); + } + } + } + } + if(!config->quiet) { + char message[getcols()]; + int len = snprintf(message, getcols(), "%s: %i %s (%i %s)", + _("Check complete"), allfiles, _("files"), pkgs_n, _("packages")); + fprintf(stderr, "\r%s %*s\n", 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); + } + + alpm_list_free(damaged); +} + int pacman_query(alpm_list_t *targets) { int ret = 0; - alpm_list_t *i; + alpm_list_t *i, *pkgs = NULL; + pmpkg_t *pkg = NULL; /* First: operations that do not require targets */ @@ -363,7 +455,7 @@ 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) { @@ -372,11 +464,19 @@ int pacman_query(alpm_list_t *targets) } for(i = alpm_db_get_pkgcache(db_local); i; i = alpm_list_next(i)) { - pmpkg_t *pkg = alpm_list_getdata(i); + pkg = alpm_list_getdata(i); if(filter(pkg)) { - display(pkg); + if(config->op_q_check) { + pkgs = alpm_list_add(pkgs, pkg); + } else { + display(pkg); + } } } + if(config->op_q_check){ + check(pkgs); + alpm_list_free(pkgs); + } return(0); } @@ -389,10 +489,9 @@ 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; if(config->op_q_isfile) { alpm_pkg_load(strname, 1, &pkg); @@ -406,8 +505,13 @@ 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 { + display(pkg); + } } if(config->op_q_isfile) { @@ -415,6 +519,10 @@ int pacman_query(alpm_list_t *targets) pkg = NULL; } } + if(config->op_q_check){ + check(pkgs); + alpm_list_free(pkgs); + } return(ret); } -- 1.6.2.1
changaco wrote :
From ee827c85cb9c4cd42ff42d8d624923076d62459b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Mon, 30 Mar 2009 18:48:15 +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@laposte.net> --- doc/pacman.8.txt | 4 ++ src/pacman/conf.h | 1 + src/pacman/pacman.c | 7 +++- src/pacman/query.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 126 insertions(+), 8 deletions(-)
...
No feedback on this new version of the patch ?
changaco wrote :
changaco wrote :
From ee827c85cb9c4cd42ff42d8d624923076d62459b Mon Sep 17 00:00:00 2001
From: Charly COSTE <changaco@laposte.net> Date: Mon, 30 Mar 2009 18:48:15 +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@laposte.net> --- doc/pacman.8.txt | 4 ++ src/pacman/conf.h | 1 + src/pacman/pacman.c | 7 +++- src/pacman/query.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 126 insertions(+), 8 deletions(-)
...
No feedback on this new version of the patch ?
Not even a little message to tell me if there are things I still need to work on ?
On Wed, Apr 08, 2009 at 12:46:22AM +0200, changaco wrote:
Not even a little message to tell me if there are things I still need to work on ?
Seems pacman-dev is in vacation mode, or I suspect that they are busy with some other things lately. :D
Loui Chang wrote :
On Wed, Apr 08, 2009 at 12:46:22AM +0200, changaco wrote:
Not even a little message to tell me if there are things I still need to work on ?
Seems pacman-dev is in vacation mode, or I suspect that they are busy with some other things lately. :D
Ok, thx. ;)
changaco wrote :
From ee827c85cb9c4cd42ff42d8d624923076d62459b Mon Sep 17 00:00:00 2001 From: Charly COSTE <changaco@laposte.net> Date: Mon, 30 Mar 2009 18:48:15 +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@laposte.net> --- doc/pacman.8.txt | 4 ++ src/pacman/conf.h | 1 + src/pacman/pacman.c | 7 +++- src/pacman/query.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 126 insertions(+), 8 deletions(-)
Anybody still interested in my patch ?
Dan McGee wrote :
On Sun, Mar 29, 2009 at 3:50 PM, changaco <changaco@laposte.net> wrote:
+*-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.
Works for me, changed in the new patch.
@@ -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.
That's right, changed too.
+ 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 '/'.
I fixed that with the same method you used for the root in your patch. Should I remove the test or not ?
+ 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);
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.
+ 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.
All fixed according to your patch.
+ 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.
A few random tests:
dmcgee@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.
Problem fixed. See the new patch for details.
changaco schrieb:
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.
If a package has files in /tmp, it should be reported here, as it is definitely a bug in the package! It is not pacman's responsibility to fix this, but the packager's.
2009/3/30 Thomas Bächler <thomas@archlinux.org>:
changaco schrieb:
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.
If a package has files in /tmp, it should be reported here, as it is definitely a bug in the package! It is not pacman's responsibility to fix this, but the packager's.
I guess he's talking about ttf-ms-fonts. The files that initially get installed in /tmp are extracted/installed properly afterwards by the .install script. I'm not sure why this is done but it might be a license restriction.
Eric Bélanger wrote:
2009/3/30 Thomas Bächler <thomas@archlinux.org>:
changaco schrieb:
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.
If a package has files in /tmp, it should be reported here, as it is definitely a bug in the package! It is not pacman's responsibility to fix this, but the packager's.
I guess he's talking about ttf-ms-fonts. The files that initially get installed in /tmp are extracted/installed properly afterwards by the .install script. I'm not sure why this is done but it might be a license restriction.
Correct. We can only distribute the un-extracted files. Allan
Allan McRae wrote :
Eric Bélanger wrote: 2009/3/30 Thomas Bächler <thomas@archlinux.org>:
changaco wrote:
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.
If a package has files in /tmp, it should be reported here, as it is definitely a bug in the package! It is not pacman's responsibility to fix this, but the packager's.
I guess he's talking about ttf-ms-fonts. The files that initially get installed in /tmp are extracted/installed properly afterwards by the .install script. I'm not sure why this is done but it might be a license restriction.
Correct. We can only distribute the un-extracted files.
Allan
Ok pacman shouldn't have to worry about /tmp but as we cannot distribute the extracted files we have to find a solution, we can't just ignore the fact that this option would show ttf-ms-fonts as damaged. As "normal" packages shouldn't put files in /tmp I think it is a good choice to ignore it. Alternatively we could just display a message when the missing files are in /tmp and let the user choose what to do. Or if someone has a better idea: share it ...
On Mon, Mar 30, 2009 at 11:21 AM, changaco <changaco@laposte.net> wrote:
Allan McRae wrote :
Eric Bélanger wrote: 2009/3/30 Thomas Bächler <thomas@archlinux.org>:
changaco wrote:
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.
If a package has files in /tmp, it should be reported here, as it is definitely a bug in the package! It is not pacman's responsibility to fix this, but the packager's.
I guess he's talking about ttf-ms-fonts. The files that initially get installed in /tmp are extracted/installed properly afterwards by the .install script. I'm not sure why this is done but it might be a license restriction.
Correct. We can only distribute the un-extracted files.
Allan
Ok pacman shouldn't have to worry about /tmp but as we cannot distribute the extracted files we have to find a solution, we can't just ignore the fact that this option would show ttf-ms-fonts as damaged. As "normal" packages shouldn't put files in /tmp I think it is a good choice to ignore it. Alternatively we could just display a message when the missing files are in /tmp and let the user choose what to do. Or if someone has a better idea: share it ...
The package is damaged. The better option is NOT to ignore it and fix the broken package, not try to hack a solution into pacman. If *anything* on the system deletes a file a package knows about, it is a problem. -Dan
Dan McGee wrote:
On Mon, Mar 30, 2009 at 11:21 AM, changaco <changaco@laposte.net> wrote:
Allan McRae wrote :
Eric Bélanger wrote: 2009/3/30 Thomas Bächler <thomas@archlinux.org>:
changaco wrote: > > 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.
If a package has files in /tmp, it should be reported here, as it is definitely a bug in the package! It is not pacman's responsibility to fix this, but the packager's.
I guess he's talking about ttf-ms-fonts. The files that initially get installed in /tmp are extracted/installed properly afterwards by the .install script. I'm not sure why this is done but it might be a license restriction.
Correct. We can only distribute the un-extracted files.
Allan
Ok pacman shouldn't have to worry about /tmp but as we cannot distribute the extracted files we have to find a solution, we can't just ignore the fact that this option would show ttf-ms-fonts as damaged. As "normal" packages shouldn't put files in /tmp I think it is a good choice to ignore it. Alternatively we could just display a message when the missing files are in /tmp and let the user choose what to do. Or if someone has a better idea: share it ...
The package is damaged. The better option is NOT to ignore it and fix the broken package, not try to hack a solution into pacman.
If *anything* on the system deletes a file a package knows about, it is a problem.
-Dan _______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
I get your point but how do we fix the package ? We make empty files ?
participants (7)
-
Aaron Griffin
-
Allan McRae
-
changaco
-
Dan McGee
-
Eric Bélanger
-
Loui Chang
-
Thomas Bächler