On Saturday 14 November 2009, Jérémy Bobbio wrote:
> On Wed, Nov 11, 2009 at 09:38:22PM +0100, Frans Pop wrote:
> Those comments are only based on reading your changes against the C
> code. But here they are:
Thanks a lot for the review Jérémy! Nice to see you're still lurking.
Attached a new version of the most important changes, including
improvements from most of your comments. It even seems to work the same as
the previous version :-)
BTW, you can't see it in this patch, but I do use your 'alingn' capability
to separate codename and suite in the dialog (see screenshot in original
mail).
Could you go through it once more and maybe look closely at freeing release
and other vars again?
Thanks,
FJP
> > + char line[80];
> Are "Releases" lines defined not be hold more than 80 characters?
> Proper boundary checking seems to be done hereafter, so I'm just
> wondering.
Here's a typical Release file:
http://ftp.nl.debian.org/debian/dists/lenny/Release
So yes, they can be longer than 80, but not the lines we care about. And as
we don't actually read the file, but only a grep of the lines we want,
there is no problem.
> That might be useless given the size of the Release file, but you could
> exit the while loop as soon as (release.name != NULL && release.suite !=
> NULL).
Same. As we grep only the exact two lines we want that's not needed.
> > + if ((release.name || release.suite) && !(release.status &
> > IS_VALID))
>
> Up to you, but I would tend to explicitely compare against NULL for all
> those tests.
Done.
> Can't think straight about it, but I think there is no need
> for the parenthesis around the ||.
AFAIK && has higher prio than ||, so they are needed.
> get_release() calls validate_codename() and validate_codename() calls
> get_release(). I like mutually recursive functions in Haskell, but
> it might be better to avoid them in C, I'd say. ;)
This I have not changed yet. The idea is that given the tests and the
content of Release files, you can never get an endless recursion.
validate_codename() should get called one time maximum.
I will think about this a bit more though.
> > + if (cn_status >= 0) {
>
> I would use != here instead: cn_value holds flags, so think of it has a
> numerical value (by using 'greater than') is misleading.
In the old version I did return -1 from validate_codename(). The filed had
a double usage as flags (if >= 0) and error code (if < 0). This is now
fixed after implementing your later suggestions though.
> I suggest to change the code a little with the following prototyes:
>
> static int get_release(struct release_t * release, const char * name);
> ^^^ ^^^^^
> success or failure no changes is ever done to 'name'
This gave me quite a bit of trouble to find the correct syntax, but I got
there in the end. Does look better now, but I'd appreciate a check if I
got it right.
> releases will always be allocated to the same size. Which means there
> is no need to play malloc/free games and just define releases with
> something like:
>
> static struct release_t releases[MAXRELEASES];
Done.
> It would also need another static variable that would hold the actual
> number of releases already parsed (and if == 0, then releases have not
> been parsed yet).
Not needed IMO. When releases gets used .name is always set to NULL for the
last entry as terminator.
> I can't see where 'suite' needs to be modified. Wouldn't it be possible
> to use suites[i] directly and drop the extra free() calls?
Done.
> > + release = get_release(suite);
> > [...]
> > static int choose_suite(void) {
> > [...]
> > + for (i=0; releases[i].name != NULL; i++) {
> > + char *name;
> > +
> > + if (releases[i].status & GET_SUITE)
> > + name = strdup(releases[i].suite);
> > + else
> > + name = strdup(releases[i].name);
> > +
> > + choices_c[i] = strdup(name);
> > + if (strcmp(name, releases[i].name) != 0)
> > + asprintf(&choices[i], "%s - %s", releases[i].name, name);
> > + else
> > + choices[i] = strdup(name);
> > + if (releases[i].status & IS_DEFAULT) {
> > + debconf_set(debconf, DEBCONF_BASE "suite", name);
> > + have_default = 1;
> > + }
> > +
> > + free(name);
> > + }
>
> There is probably extra strdup() as well here.
Not that I can see; 'name' is needed I think. What were you thinking of?
Can I maybe just do 'choices_c[i] = name' and 'choices[i] = name' instead
of duplicating the string?
But then I can't free name where I do now, right?
> > + * Data structure containing information on releases supported by a
> > mirror + */
> > +struct release_t {
> > + char *name;
> > + char *suite;
> > + int status;
> > +};
>
> Actually, I wonder about calling that struct 'release_t', as this does
> not define a full type (_t) but only a struct name. Using 'struct
> release' might be less confusing.
Yeah, but it's consistent with how mirror_t is defined...
diff --git a/packages/choose-mirror/choose-mirror.c b/packages/choose-mirror/choose-mirror.c
index afee209..47573e1 100644
--- a/packages/choose-mirror/choose-mirror.c
+++ b/packages/choose-mirror/choose-mirror.c
@@ -27,6 +27,9 @@ int show_progress = 1;
/* Are we installing from a CD that includes base system packages? */
static int base_on_cd = 0;
+/* Available releases (suite/codename) on the mirror. */
+static struct release_t releases[MAXRELEASES];
+
/*
* Returns a string on the form "DEBCONF_BASE/protocol/supplied". The
* calling function is responsible for freeing the string afterwards.
@@ -148,6 +151,219 @@ static char *get_default_suite(void) {
return suite;
}
+void log_invalid_release(const char *name, const char *field) {
+ di_log(DI_LOG_LEVEL_WARNING,
+ "broken mirror: invalid %s in Release file for %s", field, name);
+}
+
+static int get_release(struct release_t *release, const char *name);
+
+/*
+ * Try to fetch a Release file using its codename and, if successful, check
+ * that it matches the Release file that was fetched using the suite.
+ */
+static int validate_codename(int *status, const char *codename, const char *suite) {
+ struct release_t release;
+ int ret = 0;
+
+ if (get_release(&release, codename)) {
+ *status = release.status;
+ if (strcmp(release.suite, suite) != 0)
+ *status &= ~IS_VALID;
+ ret = 1;
+ }
+
+ free(release.name);
+ free(release.suite);
+
+ return ret;
+}
+
+/*
+ * Fetch a Release file, extract its Suite and Codename and check its valitity.
+ */
+static int get_release(struct release_t *release, const char *name) {
+ char *command;
+ FILE *f = NULL;
+ char *hostname, *directory;
+ char line[80];
+ char buf[SUITE_LENGTH];
+
+ hostname = add_protocol("hostname");
+ debconf_get(debconf, hostname);
+ free(hostname);
+ hostname = strdup(debconf->value);
+ directory = add_protocol("directory");
+ debconf_get(debconf, directory);
+ free(directory);
+ directory = strdup(debconf->value);
+
+ asprintf(&command, "wget -q %s://%s%s/dists/%s/Release -O - | grep -E '^(Suite|Codename):'",
+ protocol, hostname, directory, name);
+ di_log(DI_LOG_LEVEL_DEBUG, "command: %s", command);
+ f = popen(command, "r");
+ free(command);
+ 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;
+
+ if (line[strlen(line) - 1] == '\n')
+ line[strlen(line) - 1] = '\0';
+ if ((value = strstr(line, ": ")) != NULL) {
+ strncpy(buf, value + 2, SUITE_LENGTH - 1);
+ buf[SUITE_LENGTH - 1] = '\0';
+ if (strncmp(line, "Codename:", 9) == 0)
+ release->name = strdup(buf);
+ if (strncmp(line, "Suite:", 6) == 0)
+ release->suite = strdup(buf);
+ }
+ }
+ if (release->name != NULL && strcmp(release->name, name) == 0)
+ release->status |= IS_VALID | GET_CODENAME;
+ if (release->suite != NULL && strcmp(release->suite, name) == 0)
+ release->status |= IS_VALID | GET_SUITE;
+
+ if ((release->name != NULL || release->suite != NULL) &&
+ !(release->status & IS_VALID))
+ log_invalid_release(name, "Suite or Codename");
+
+ /* Check if release can also be gotten using codename */
+ if ((release->status & IS_VALID) && release->name != NULL &&
+ !(release->status & GET_CODENAME)) {
+ int cn_status;
+ 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");
+ }
+ }
+ }
+
+ /* In case there is no Codename field */
+ if ((release->status & IS_VALID) && release->name == NULL)
+ release->name = strdup(name);
+
+ // di_log(DI_LOG_LEVEL_DEBUG, "get_release(): %s -> %s:%s (0x%x)",
+ // name, release->suite, release->name, release->status);
+ }
+
+ pclose(f);
+
+ return (release->name != NULL);
+}
+
+static int find_releases(void) {
+ int nbr_suites = sizeof(suites)/SUITE_LENGTH;
+ int i, r = 0;
+ int bad_mirror = 0, have_default = 0;
+ struct release_t release;
+ char *default_suite;
+
+ default_suite = get_default_suite();
+ if (default_suite == NULL)
+ di_log(DI_LOG_LEVEL_ERROR, "no default release specified");
+
+ if (show_progress) {
+ debconf_progress_start(debconf, 0, nbr_suites,
+ DEBCONF_BASE "checking_title");
+ debconf_progress_info(debconf,
+ DEBCONF_BASE "checking_download");
+ }
+
+ /* Get releases for all suites */
+ if (! base_on_cd) {
+ for (i=0; i < nbr_suites && r < MAXRELEASES; i++) {
+ if (get_release(&release, suites[i])) {
+ if (release.status & IS_VALID) {
+ if (strcmp(release.name, default_suite) == 0 ||
+ strcmp(release.suite, default_suite) == 0) {
+ release.status |= IS_DEFAULT;
+ have_default = 1;
+ }
+ /* Only list oldstable if it's the default */
+ if (strcmp(suites[i], "oldstable") != 0 ||
+ (release.status & IS_DEFAULT))
+ releases[r++] = release;
+ } else {
+ bad_mirror = 1;
+ break;
+ }
+ }
+
+ if (show_progress)
+ debconf_progress_step(debconf, 1);
+ }
+ if (r == MAXRELEASES)
+ di_log(DI_LOG_LEVEL_ERROR, "array overflow: more releases than allowed by MAXRELEASES");
+ if (! bad_mirror && r == 0)
+ di_log(DI_LOG_LEVEL_INFO, "mirror does not have any suite symlinks");
+ }
+
+ /* Try to get release using the default "suite" */
+ if (! bad_mirror && (base_on_cd || ! have_default)) {
+ if (get_release(&release, default_suite)) {
+ if (release.status & IS_VALID) {
+ release.status |= IS_DEFAULT;
+ releases[r++] = release;
+ have_default = 1;
+ } else {
+ bad_mirror = 1;
+ }
+ } else {
+ di_log(DI_LOG_LEVEL_WARNING,
+ "mirror does not support the specified release (%s)",
+ default_suite);
+ }
+ if (r == MAXRELEASES)
+ di_log(DI_LOG_LEVEL_ERROR, "array overflow: more releases than allowed by MAXRELEASES");
+ }
+
+ /* Mark end of list */
+ releases[r].name = NULL;
+
+ if (show_progress) {
+ debconf_progress_step(debconf, nbr_suites);
+ debconf_progress_stop(debconf);
+ }
+
+ if (r == 0 || bad_mirror) {
+ free(default_suite);
+
+ debconf_input(debconf, "critical", DEBCONF_BASE "bad");
+ if (debconf_go(debconf) == 30)
+ exit(10); /* back up to menu */
+ else
+ return 1; /* back to beginning of questions */
+ }
+
+ if (! base_on_cd && ! have_default) {
+ debconf_subst(debconf, DEBCONF_BASE "no-default",
+ "RELEASE", default_suite);
+ free(default_suite);
+
+ debconf_input(debconf, "critical", DEBCONF_BASE "no-default");
+ if (debconf_go(debconf) == 30) {
+ exit(10); /* back up to menu */
+ } else {
+ debconf_get(debconf, DEBCONF_BASE "no-default");
+ if (strcmp(debconf->value, "false"))
+ return 1; /* back to beginning of questions */
+ }
+ } else {
+ free(default_suite);
+ }
+
+ return 0;
+}
+
/*
* Using the current debconf settings for a mirror, figure out which suite
* to use from the mirror and set mirror/suite.
@@ -466,62 +686,90 @@ static int check_mirror(void) {
}
static int choose_suite(void) {
+ char **choices_c, **choices, *list;
+ int i, ret;
+ int have_default = 0;
+
+ ret = find_releases();
+ if (ret)
+ return ret;
+
+ /* 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;
+
+ if (releases[i].status & GET_SUITE)
+ name = strdup(releases[i].suite);
+ else
+ name = strdup(releases[i].name);
+
+ choices_c[i] = strdup(name);
+ if (strcmp(name, releases[i].name) != 0)
+ asprintf(&choices[i], "%s - %s", releases[i].name, name);
+ else
+ choices[i] = strdup(name);
+ if (releases[i].status & IS_DEFAULT) {
+ debconf_set(debconf, DEBCONF_BASE "suite", name);
+ have_default = 1;
+ }
+
+ free(name);
+ }
+
+ choices_c[i] = NULL;
+ list = debconf_list(choices_c);
+ debconf_subst(debconf, DEBCONF_BASE "suite", "CHOICES-C", list);
+ choices[i] = NULL;
+ list = debconf_list(choices);
+ debconf_subst(debconf, DEBCONF_BASE "suite", "CHOICES", list);
+ free(choices_c);
+ free(choices);
+ free(list);
+
/* If the base system can be installed from CD, don't allow to
* select a different suite
*/
+ if (! have_default)
+ debconf_fset(debconf, DEBCONF_BASE "suite", "seen", "false");
if (! base_on_cd)
- debconf_input(debconf, "medium", DEBCONF_BASE "suite");
+ debconf_input(debconf, have_default ? "medium" : "critical",
+ DEBCONF_BASE "suite");
+
return 0;
}
-/* Get the codename for the selected suite. */
-int get_codename (void) {
- char *command;
- FILE *f = NULL;
- char *hostname, *directory, *suite = NULL;
- int ret = 1;
-
- hostname = add_protocol("hostname");
- debconf_get(debconf, hostname);
- free(hostname);
- hostname = strdup(debconf->value);
- directory = add_protocol("directory");
- debconf_get(debconf, directory);
- free(directory);
- directory = strdup(debconf->value);
+/* Set the codename for the selected suite. */
+int set_codename (void) {
+ char *suite;
+ int i;
/* As suite has been determined previously, this should not fail */
debconf_get(debconf, DEBCONF_BASE "suite");
if (strlen(debconf->value) > 0) {
suite = strdup(debconf->value);
- asprintf(&command, "wget -q %s://%s%s/dists/%s/Release -O - | grep ^Codename: | cut -d' ' -f 2",
- protocol, hostname, directory, suite);
- di_log(DI_LOG_LEVEL_DEBUG, "command: %s", command);
- f = popen(command, "r");
- free(command);
-
- if (f != NULL) {
- char buf[SUITE_LENGTH];
- if (fgets(buf, SUITE_LENGTH - 1, f)) {
- if (buf[strlen(buf) - 1] == '\n')
- buf[strlen(buf) - 1] = '\0';
- debconf_set(debconf, DEBCONF_BASE "codename", buf);
- di_log(DI_LOG_LEVEL_INFO, "codename set to: %s", buf);
- ret = 0;
+ for (i=0; releases[i].name != NULL; i++) {
+ if (strcmp(releases[i].name, suite) == 0 ||
+ strcmp(releases[i].suite, suite) == 0) {
+ char *codename;
+
+ if (releases[i].status & GET_CODENAME)
+ codename = releases[i].name;
+ else
+ 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;
}
}
- pclose(f);
- }
- free(hostname);
- free(directory);
- if (suite)
free(suite);
+ }
- if (ret != 0)
- di_log(DI_LOG_LEVEL_ERROR, "Error getting codename");
- return ret;
+ return 0;
}
/* Check if the mirror carries the architecture that's being installed. */
@@ -590,9 +838,8 @@ int main (int argc, char **argv) {
validate_mirror,
choose_proxy,
set_proxy,
- check_mirror,
choose_suite,
- get_codename,
+ set_codename,
check_arch,
NULL,
};
Attachment:
signature.asc
Description: This is a digitally signed message part.