[pacman-dev] [PATCH 2/2] New feature: files verification
Dan McGee
dpmcgee at gmail.com
Tue Jul 21 23:44:12 EDT 2009
On Sat, Jul 18, 2009 at 1:09 PM, Xavier Chantry<shiningxc at gmail.com> wrote:
> From: Charly COSTE <changaco at laposte.net>
>
> A new option "-Qk" which checks if all packages files are really on the
> system (i.e. not accidentally deleted).
>
> This implements FS#13877
>
> Signed-off-by: Charly COSTE <changaco at laposte.net>
> [Xav : don't ignore tmp, don't repeat package names in quiet output, add
> errno message in verbose output]
> Signed-off-by: Xavier Chantry <shiningxc at gmail.com>
OK, I'm going to finally review this patch enough that we can get it
in. Hopefully we can make a team effort to get it fixed up.
> ---
> doc/pacman.8.txt | 4 ++
> src/pacman/conf.h | 1 +
> src/pacman/pacman.c | 7 +++-
> src/pacman/query.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++---
> 4 files changed, 118 insertions(+), 8 deletions(-)
>
> diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt
> index af85a15..ccff167 100644
> --- a/doc/pacman.8.txt
> +++ b/doc/pacman.8.txt
> @@ -196,6 +196,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.
> +
Good.
> *-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 39802ca..6523d49 100644
> --- a/src/pacman/conf.h
> +++ b/src/pacman/conf.h
> @@ -51,6 +51,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 7f86489..48d45ad 100644
> --- a/src/pacman/pacman.c
> +++ b/src/pacman/pacman.c
> @@ -108,6 +108,7 @@ static void usage(int op, const char * const myname)
> printf(_(" -e, --explicit list packages explicitly installed [filter]\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) [filter]\n"));
> printf(_(" -o, --owns <file> query the package that owns <file>\n"));
> @@ -345,6 +346,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'},
> @@ -473,7 +475,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 4997202..62eca53 100644
> --- a/src/pacman/query.c
> +++ b/src/pacman/query.c
> @@ -334,10 +334,94 @@ 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];
> +
> + 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);
> + 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++;
> + /* 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, f, strerror(errno));
> + 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) {
> + 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 +447,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 +456,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 +481,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 +497,13 @@ int pacman_query(alpm_list_t *targets)
> continue;
> }
>
> +
^^^ necessary?
> 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 +511,10 @@ int pacman_query(alpm_list_t *targets)
> pkg = NULL;
> }
> }
> + if(config->op_q_check){
Please match brace style. You're missing space between the ) and the {
closing the line. You did this in more than one place.
> + check(pkgs);
> + alpm_list_free(pkgs);
> + }
Hmm, the last few diff hunks are quite interesting. Really wish we
didn't have to special case this so much. Why not call check() from
display() so it fits in with the rest of the logic? Ahh, because we
used a homegrown progress function...I said it way back when I was
never a fan of this, unfortunately, and no one got the hint.
>
> return(ret);
> }
> --
> 1.6.3.3
So I didn't even type up all of my thoughts, but I did go and hack at
the patch a bit instead. At first I was just going to submit a
supplemental patch, but instead I've reworked it enough that I'll
submit in one piece as a reply to this email.
-Dan
More information about the pacman-dev
mailing list