On Thu, Nov 19, 2009 at 09:46:45PM +0000, Frans Pop wrote:
> Log:
> Rewrite release (suite) selection
> […]
> + /* Initialize releases; also ensures NULL termination for .name */
That comment is misleading, I'd say. If I understood correctly, NULL as
releases[x].name means that x is the last set release in releases. As
.name is a char*, ensuiring NULL termination can be understood as
ensuiring NULL termination of the string…
> + memset(&releases, 0, MAXRELEASES * sizeof(struct release_t));
As you pointed out, it would probably be better to use 'sizeof
(releases)' instead of manually calculating the size.
> […]
> + memset(&release, 0, sizeof(struct release_t));
Same here.
> […]
> + memset(&release, 0, sizeof(struct release_t));
Same here.
> […]
> + if (release.name)
> + free(release.name);
> + if (release.suite)
> + free(release.suite);
free() is defined to do nothing on NULL pointers, IIRC, so you can drop
those ifs.
> + memset(choices, 0, MAXRELEASES * sizeof(char *));
> + memset(choices_c, 0, MAXRELEASES * sizeof(char *));
As earlier.
> + /* Arrays can never overflow as we've already checked releases */
> + for (i=0; releases[i].name != NULL; i++) {
> + char *name;
name is not modified, so 'const char * name' would be better.
> +
> + if (releases[i].status & GET_SUITE)
> + name = strdup(releases[i].suite);
> + else
> + name = strdup(releases[i].name);
No need to copy the string, releases[i].suite or releases[i].name will
not change before the question is asked. (And debconf_subst() will do a
copy already.)
> +
> + choices_c[i] = strdup(name);
Same here.
> + if (strcmp(name, releases[i].name) != 0)
> + asprintf(&choices[i], "%s - %s", releases[i].name, name);
> + else
> + choices[i] = strdup(name);
And here.
> + if (releases[i].status & IS_DEFAULT)
> + debconf_set(debconf, DEBCONF_BASE "suite", name);
> +
> + free(name);
And no free() if you only reference releases' strings.
> + for (i=0; choices[i] != NULL; i++) {
> + free(choices_c[i]);
> + free(choices[i]);
> + }
> +
And no need for this too.
Ok, nothing more to say, I think. :) /me goes back to more lurking ;)
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature