Re: [RFC] choose-mirror rewrite - please review/test
On Monday 16 November 2009, Frans Pop wrote:
> I think the attached incremental patch addresses all your comments.
Eh, wrong commit. Correct patch attached.
commit 888015f06c122e1fa723f14bb69a527b2ef7585b
Author: Frans Pop <fjp@debian.org>
Date: Sun Nov 15 23:51:38 2009 +0100
More improvements from Jeremy
diff --git a/packages/choose-mirror/choose-mirror.c b/packages/choose-mirror/choose-mirror.c
index 45c4cf5..43e7344 100644
--- a/packages/choose-mirror/choose-mirror.c
+++ b/packages/choose-mirror/choose-mirror.c
@@ -159,22 +159,29 @@ void log_invalid_release(const char *name, const char *field) {
static int get_release(struct release_t *release, const char *name);
/*
- * Try to fetch a Release file using its codename and, if successful, check
+ * Try to fetch a Release file using its codename; if successful, check
* that it matches the Release file that was fetched using the suite.
+ * Returns false only if an invalid Release file was found.
*/
-static int validate_codename(int *status, const char *codename, const char *suite) {
- struct release_t release;
- int ret = 0;
+static int validate_codename(struct release_t *s_release) {
+ struct release_t cn_release;
+ int ret = 1;
- if (get_release(&release, codename)) {
- *status = release.status;
- if (strcmp(release.suite, suite) != 0)
- *status &= ~IS_VALID;
- ret = 1;
+ memset(&cn_release, 0, sizeof(struct release_t));
+
+ /* s_release->name is the codename to check */
+ if (get_release(&cn_release, s_release->name)) {
+ if ((cn_release.status & IS_VALID) &&
+ strcmp(cn_release.suite, s_release->suite) == 0) {
+ s_release->status |= (cn_release.status & GET_CODENAME);
+ } else {
+ s_release->status &= ~IS_VALID;
+ ret = 0;
+ }
}
- free(release.name);
- free(release.suite);
+ free(cn_release.name);
+ free(cn_release.suite);
return ret;
}
@@ -206,9 +213,6 @@ static int get_release(struct release_t *release, const char *name) {
free(hostname);
free(directory);
- release->name = NULL;
- release->suite = NULL;
- release->status = 0;
if (f != NULL) {
while (fgets(line, sizeof(line), f) != NULL) {
char *value;
@@ -235,17 +239,9 @@ static int get_release(struct release_t *release, const char *name) {
/* Check if release can also be gotten using codename */
if ((release->status & IS_VALID) && release->name != NULL &&
- !(release->status & GET_CODENAME)) {
- int cn_status = 0;
- if (validate_codename(&cn_status, release->name, name)) {
- if (cn_status & IS_VALID) {
- release->status |= (cn_status & GET_CODENAME);
- } else {
- release->status &= ~IS_VALID;
- log_invalid_release(name, "Codename");
- }
- }
- }
+ !(release->status & GET_CODENAME))
+ if (! validate_codename(release))
+ log_invalid_release(name, "Codename");
/* In case there is no Codename field */
if ((release->status & IS_VALID) && release->name == NULL)
@@ -257,7 +253,13 @@ static int get_release(struct release_t *release, const char *name) {
pclose(f);
- return (release->name != NULL);
+ if (release->name != NULL) {
+ return 1;
+ } else {
+ if (release->suite)
+ free(release->suite);
+ return 0;
+ }
}
static int find_releases(void) {
@@ -277,9 +279,13 @@ static int find_releases(void) {
DEBCONF_BASE "checking_download");
}
+ /* Initialize releases; also ensures NULL termination for .name */
+ memset(&releases, 0, MAXRELEASES * sizeof(struct release_t));
+
/* Get releases for all suites */
if (! base_on_cd) {
for (i=0; i < nbr_suites && r < MAXRELEASES; i++) {
+ memset(&release, 0, sizeof(struct release_t));
if (get_release(&release, suites[i])) {
if (release.status & IS_VALID) {
if (strcmp(release.name, default_suite) == 0 ||
@@ -303,6 +309,7 @@ static int find_releases(void) {
/* Try to get release using the default "suite" */
if (! bad_mirror && (base_on_cd || r == 0)) {
+ memset(&release, 0, sizeof(struct release_t));
if (get_release(&release, default_suite)) {
if (release.status & IS_VALID) {
release.status |= IS_DEFAULT;
@@ -313,9 +320,6 @@ static int find_releases(void) {
}
}
- /* Mark end of list */
- releases[r].name = NULL;
-
if (show_progress) {
debconf_progress_step(debconf, nbr_suites);
debconf_progress_stop(debconf);
@@ -324,6 +328,11 @@ static int find_releases(void) {
free(default_suite);
if (r == 0 || bad_mirror) {
+ if (release.name)
+ free(release.name);
+ if (release.suite)
+ free(release.suite);
+
debconf_input(debconf, "critical", DEBCONF_BASE "bad");
if (debconf_go(debconf) == 30)
exit(10); /* back up to menu */
@@ -652,16 +661,18 @@ static int check_mirror(void) {
}
static int choose_suite(void) {
- char **choices_c, **choices, *list;
+ char *choices_c[MAXRELEASES], *choices[MAXRELEASES], *list;
int i, ret;
ret = find_releases();
if (ret)
return ret;
+ /* Also ensures NULL termination */
+ memset(choices, 0, MAXRELEASES * sizeof(char *));
+ memset(choices_c, 0, MAXRELEASES * sizeof(char *));
+
/* Arrays can never overflow as we've already checked releases */
- choices_c = malloc(MAXRELEASES * sizeof(char*));
- choices = malloc(MAXRELEASES * sizeof(char*));
for (i=0; releases[i].name != NULL; i++) {
char *name;
@@ -681,16 +692,18 @@ static int choose_suite(void) {
free(name);
}
- choices_c[i] = NULL;
list = debconf_list(choices_c);
debconf_subst(debconf, DEBCONF_BASE "suite", "CHOICES-C", list);
- choices[i] = NULL;
+ free(list);
list = debconf_list(choices);
debconf_subst(debconf, DEBCONF_BASE "suite", "CHOICES", list);
- free(choices_c);
- free(choices);
free(list);
+ for (i=0; choices[i] != NULL; i++) {
+ free(choices_c[i]);
+ free(choices[i]);
+ }
+
/* If the base system can be installed from CD, don't allow to
* select a different suite
*/
@@ -722,7 +735,6 @@ int set_codename (void) {
codename = releases[i].suite;
debconf_set(debconf, DEBCONF_BASE "codename", codename);
di_log(DI_LOG_LEVEL_INFO, "codename set to: %s", codename);
- free(codename);
break;
}
}
@@ -787,6 +799,7 @@ int check_arch (void) {
}
int main (int argc, char **argv) {
+ int i;
/* Use a state machine with a function to run in each state */
int state = 0;
int (*states[])() = {
@@ -827,5 +840,11 @@ int main (int argc, char **argv) {
else
state++;
}
+
+ for (i=0; releases[i].name != NULL; i++) {
+ free(releases[i].name);
+ free(releases[i].suite);
+ }
+
return (state >= 0) ? 0 : 10; /* backed all the way out */
}
Reply to: