Fix a design bug concerning struct user_info.

It was a bad idea to include the accounting data (#files, #dirs,
information about the user only.

So move the accounting data to user_summary_info. This allows
to get rid of the ugly uid_hash_table_sort_idx construct and of
sort_hash_table(). These were only needed because of the broken
design.  We now never sort the hash table but allocate an array of
user_summary_info structures on each query and sort that array instead.

This patch not only simplifies code but also fixes a real bug noted by
Sebastian Stark: If the user_summary was requested more than once in
interactive mode, the old code computed incorrect values because the
above mentioned accounting fields were only initialized once. The new
code gets this right automatically because a fresh array is created
on each query.

diff --git a/create.c b/create.c
index 65567e03ee592065cfd4126ac6398b8890dd71e6..87b0b9a0198a471e0a5a886dbce84a09c49f4901 100644 (file)
--- a/create.c
+++ b/create.c
@@ -134,8 +134,6 @@ static int scan_dir(char *dirname, uint64_t *parent_dir_num)
                ret = create_user_table(uid, &ui);
                if (ret < 0)
                        goto out;
-               ui->bytes += size;
-               ui->files++;
                ret = update_user_row(ui->table, this_dir_num, &size);
                if (ret < 0)
                        goto out;

diff --git a/select.c b/select.c
index 44178805122bbd2afddaceb0d15de4918cb95d58..e30697ee9f0dc51d629d6ccbfaa789b6428e996a 100644 (file)
--- a/select.c
+++ b/select.c
@@ -145,15 +145,23 @@ struct user_list_format_info {
 
 struct user_summary_info {
        struct user_info *ui;
+       /** Total number of files owned by this user. */
+       uint64_t files;
+       /** Total number of bytes owned by this user. */
+       uint64_t bytes;
+       /** Total number of directories that contain at least one file */
+       uint64_t dirs;
        int ret;
        int osl_errno;
        regex_t *preg;
        int inverse_matching;
 };
 
-struct user_summary_line_info {
+struct user_summary_loop_data {
+       unsigned num_admissible_users;
+       struct user_summary_info *usis;
+       struct user_summary_info *current;
        struct format_info *fi;
-       uint32_t count;
 };
 
 static FILE *output_file;
@@ -444,12 +452,12 @@ static int user_summary_loop_function(struct osl_row *row, void *data)
        ret = get_num_user_files(row, usi->ui, &num);
        if (ret < 0)
                goto err;
-       usi->ui->files += num;
+       usi->files += num;
        ret = get_num_user_bytes(row, usi->ui, &num);
        if (ret < 0)
                goto err;
-       usi->ui->bytes += num;
-       usi->ui->dirs++;
+       usi->bytes += num;
+       usi->dirs++;
        return 1;
 err:
        usi->ret = ret;
@@ -457,92 +465,106 @@ err:
        return ret;
 }
 
-static int compute_user_summary(struct user_info *ui, __a_unused void *data)
+static int compute_user_summary(struct user_info *ui, void *data)
 {
-       struct user_summary_info usi = {.ui = ui};
-       int ret = compile_regex(&usi.preg, &usi.inverse_matching);
+       struct user_summary_loop_data *usld = data;
+       struct user_summary_info *usi = usld->current++;
+       int ret = compile_regex(&usi->preg, &usi->inverse_matching);
 
        if (ret < 0)
                return ret;
-       ret = adu_loop_reverse(ui->table, UT_BYTES, &usi, user_summary_loop_function,
-               &usi.ret, &usi.osl_errno);
-       free_regex(usi.preg);
+       usi->ui = ui;
+       ret = adu_loop_reverse(ui->table, UT_BYTES, usi, user_summary_loop_function,
+               &usi->ret, &usi->osl_errno);
+       free_regex(usi->preg);
        return ret;
 }
 
-static int print_user_summary_line(struct user_info *ui, void *data)
+static int print_user_summary_line(struct user_summary_info *usi,
+               struct format_info *fi)
 {
-       struct user_summary_line_info *usli = data;
+       struct user_info *ui = usi->ui;
        union atom_value values[] = {
                [usa_pw_name] = {.string_value = ui->pw_name?
                        ui->pw_name : "?"},
                [usa_uid] = {.num_value = (long long unsigned)ui->uid},
-               [usa_dirs] = {.num_value = (long long unsigned)ui->dirs},
-               [usa_files] = {.num_value = (long long unsigned)ui->files},
-               [usa_size] = {.num_value =  (long long unsigned)ui->bytes}
+               [usa_dirs] = {.num_value = (long long unsigned)usi->dirs},
+               [usa_files] = {.num_value = (long long unsigned)usi->files},
+               [usa_size] = {.num_value =  (long long unsigned)usi->bytes}
        };
-       char *buf;
-       int ret = -E_LOOP_COMPLETE;
+       char *buf = format_items(fi, values);
+       int ret = output("%s", buf);
 
-       if (!usli->count)
-               return ret;
-
-       buf = format_items(usli->fi, values);
-       ret = output("%s", buf);
        free(buf);
-       usli->count--;
        return ret;
 }
 
-static int name_comp(struct user_info *a, struct user_info *b)
+static int name_comp(const void *a, const void *b)
 {
-       char *x = a->pw_name;
-       char *y = b->pw_name;
+       const struct user_summary_info *x = a, *y = b;
+       char *n1 = x->ui->pw_name;
+       char *n2 = y->ui->pw_name;
 
-       if (!x)
+       if (!n1)
                return 1;
-       if (!y)
+       if (!n2)
                return -1;
-       return strcmp(x, y);
+       return strcmp(n1, n2);
 }
 
-static int uid_comp(struct user_info *a, struct user_info *b)
+static int uid_comp(const void *a, const void *b)
 {
-       return -NUM_COMPARE(a->uid, b->uid);
+       const struct user_summary_info *x = a, *y = b;
+       return -NUM_COMPARE(x->ui->uid, y->ui->uid);
 }
 
-static int dir_count_comp(struct user_info *a, struct user_info *b)
+static int dir_count_comp(const void *a, const void *b)
 {
-       return NUM_COMPARE(a->dirs, b->dirs);
+       const struct user_summary_info *x = a, *y = b;
+       return NUM_COMPARE(x->dirs, y->dirs);
 }
 
-static int file_count_comp(struct user_info *a, struct user_info *b)
+static int file_count_comp(const void *a, const void *b)
 {
-       return NUM_COMPARE(a->files, b->files);
+       const struct user_summary_info *x = a, *y = b;
+       return NUM_COMPARE(x->files, y->files);
 }
 
-static int size_comp(struct user_info *a, struct user_info *b)
+static int size_comp(const void *a, const void *b)
 {
-       return NUM_COMPARE(a->bytes, b->bytes);
+       const struct user_summary_info *x = a, *y = b;
+       return NUM_COMPARE(x->bytes, y->bytes);
+}
+
+static int count_admissible_users(__a_unused struct user_info *ui, void *data)
+{
+       struct user_summary_loop_data *usld = data;
+       usld->num_admissible_users++;
+       return 1;
 }
 
 static int print_user_summary(struct format_info *fi)
 {
-       int ret;
-       int (*comp)(struct user_info *a, struct user_info *b);
-       struct user_summary_line_info usli = {
-               .fi = fi,
-               .count = select_conf.limit_arg
-       };
+       int i, ret;
+       int (*comp)(const void *a, const void *b);
+       struct user_summary_loop_data usld = { .fi = fi};
        char *header = select_conf.header_given? select_conf.header_arg :
                "User summary\n";
 
        ret = output("%s", header);
        if (ret < 0)
                return ret;
-       ret = for_each_admissible_user(compute_user_summary, NULL);
+       ret = for_each_admissible_user(count_admissible_users, &usld);
        if (ret < 0)
                return ret;
+       if (usld.num_admissible_users == 0)
+               return 1;
+       usld.usis = adu_calloc(usld.num_admissible_users
+               * sizeof(struct user_summary_info));
+       usld.current = usld.usis;
+       ret = for_each_admissible_user(compute_user_summary, &usld);
+       if (ret < 0)
+               goto out;
        switch (select_conf.user_summary_sort_arg) {
        case user_summary_sort_arg_name:
                comp = name_comp;
@@ -563,10 +585,15 @@ static int print_user_summary(struct format_info *fi)
                comp = size_comp;
                break;
        }
-       sort_hash_table(comp);
-       ret = for_each_admissible_user(print_user_summary_line, &usli);
-       if (ret == -E_LOOP_COMPLETE)
-               ret = 1;
+       qsort(usld.usis, usld.num_admissible_users,
+               sizeof(struct user_summary_info), comp);
+       for (i = 0; i < usld.num_admissible_users; i++) {
+               if (select_conf.limit_arg >= 0 && i > select_conf.limit_arg)
+                       break;
+               print_user_summary_line(usld.usis + i, usld.fi);
+       }
+out:
+       free(usld.usis);
        return ret;
 }
 

diff --git a/user.c b/user.c
index bde47a7e79b87cc50d5813361d13cfc86746f272..026d65be741fdc3a100c24f6cce379f8eaa124e0 100644 (file)
--- a/user.c
+++ b/user.c
@@ -52,9 +52,6 @@ static struct user_info *uid_hash_table;
 /** This is always a power of two. It is set in create_hash_table(). */
 static uint32_t uid_hash_table_size;
 
-/* Array of indices to the entries of \a uid_hash_table. */
-static int *uid_hash_table_sort_idx;
-
 /** The number of used slots in the hash table. */
 static uint32_t num_uids;
 
@@ -325,8 +322,7 @@ int for_each_admissible_user(int (*func)(struct user_info *, void *),
        assert(uid_hash_table);
        for (i = 0; i < uid_hash_table_size; i++) {
                int ret;
-               struct user_info *ui = uid_hash_table +
-                       uid_hash_table_sort_idx[i];
+               struct user_info *ui = uid_hash_table + i;
 
                if (!ui_used(ui) || !ui_admissible(ui))
                        continue;
@@ -349,14 +345,9 @@ int for_each_admissible_user(int (*func)(struct user_info *, void *),
  */
 void create_hash_table(unsigned bits)
 {
-       int i;
-
        uid_hash_table_size = 1 << bits;
        uid_hash_table = adu_calloc(uid_hash_table_size *
                sizeof(struct user_info));
-       uid_hash_table_sort_idx = adu_malloc(uid_hash_table_size * sizeof(int));
-       for (i = 0; i < uid_hash_table_size; i++)
-               uid_hash_table_sort_idx[i] = i;
 }
 
 /**
@@ -394,8 +385,6 @@ void close_user_tables(void)
        }
        free(uid_hash_table);
        uid_hash_table = NULL;
-       free(uid_hash_table_sort_idx);
-       uid_hash_table_sort_idx = NULL;
 }
 
 /*
@@ -465,32 +454,6 @@ static char *get_uid_list_name(void)
 {
        return make_message("%s/uid_list", conf.database_dir_arg);
 }
-
-static int (*hash_table_comparator)(struct user_info *a, struct user_info *b);
-
-static int comp_wrapper(const void *a, const void *b)
-{
-       struct user_info *x = uid_hash_table + *(unsigned *)a;
-       struct user_info *y = uid_hash_table + *(unsigned *)b;
-       return hash_table_comparator(x, y);
-}
-
-/**
- * Sort the hash table according to a given comparator.
- *
- * \param comp The comparator.
- *
- * The comparator is a user-defined function which must return 1, 0, or -1.
- *
- * \sa qsort(3).
- */
-void sort_hash_table(int (*comp)(struct user_info *, struct user_info *))
-{
-       hash_table_comparator = comp;
-       qsort(uid_hash_table_sort_idx, uid_hash_table_size,
-               sizeof(*uid_hash_table_sort_idx), comp_wrapper);
-}
-
 /**
  * Open the osl tables for all admissible uids.
  *

diff --git a/user.h b/user.h
index acd0b8d85243220e42a09ec972233a238c4d393b..a6fbbf6381953140e2cac54464988917bd38aa07 100644 (file)
--- a/user.h
+++ b/user.h
@@ -28,12 +28,6 @@ struct user_info {
        char *pw_name;
        /** The user table of this user.*/
        struct osl_table *table;
-       /** Total number of files owned by this user. */
-       uint64_t files;
-       /** Total number of bytes owned by this user. */
-       uint64_t bytes;
-       /** Total number of directories that contain at least one file */
-       uint64_t dirs;
        /** The description of the user table. */
        struct osl_table_description *desc;
 };
@@ -46,8 +40,6 @@ int read_uid_file(void);
 int write_uid_file(void);
 
 void create_hash_table(unsigned bits);
-void sort_hash_table(int (*comp)(struct user_info *, struct user_info *));
-
 int for_each_admissible_user(int (*func)(struct user_info *, void *),
                void *data);
 int parse_uid_arg(const char *orig_arg, struct uid_range **ur);