From: Andre Noll <maan@systemlinux.org>
Date: Wed, 17 Dec 2008 22:32:21 +0000 (+0100)
Subject: Fix a design bug concerning struct user_info.
X-Git-Tag: v0.1.0~21
X-Git-Url: http://git.tue.mpg.de/?a=commitdiff_plain;h=bd0166ae6242deafc9b3436199965420517033b5;p=adu.git

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 65567e0..87b0b9a 100644
--- 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 4417880..e30697e 100644
--- 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 bde47a7..026d65b 100644
--- 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 acd0b8d..a6fbbf6 100644
--- 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);