Hi!
On Wed, Nov 11, 2009 at 09:38:22PM +0100, Frans Pop wrote:
> I would also very much appreciate if someone could carefully review the
> attached patch [2] as my C foo is extremely limited. Things to check are:
> - memory allocation freeing of variables
> - string manipulation
> - things that can be done smarter
Those comments are only based on reading your changes against the C
code. But here they are:
> [...]
> +static struct release_t get_release(char *name) {
> + char *command;
> + FILE *f = NULL;
> + char *hostname, *directory;
> + 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.
> + char buf[SUITE_LENGTH];
> + struct release_t release;
> + int cn_status;
> +
> + 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';
> + value = strstr(line, ": ");
> + if (value) {
Combining those last two lines into
if ((value = strstr(line, ": ")) != NULL) {
might be a little more idiomatic. But that's up to you.
> + strncpy(buf, value + 2, strlen(value) -2 + 1);
^^
missing a space here
Using strncpy() by calculating string length using strlen() does not
improve code robustness. Using strcpy() would lead to the same result.
But as you are writing the string to buf for which length is already
known to be SUITE_LENGTH, that would be the correct limit (minus 1) to
pass to strncpy().
> + buf[strlen(value) - 2 + 1] = '\0';
With a correct parameter to strncpy(), you could set buf[SUITE_LENGTH-1]
to '\0' in order to be sure that buf is null-terminated.
> + if (strncmp(line, "Codename:", 9) == 0)
> + release.name = strdup(buf);
> + if (strncmp(line, "Suite:", 6) == 0)
> + release.suite = strdup(buf);
> + }
> + }
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).
> + if (release.name && strcmp(release.name, name) == 0)
> + release.status |= IS_VALID | GET_CODENAME;
> + if (release.suite && strcmp(release.suite, name) == 0)
> + release.status |= IS_VALID | GET_SUITE;
> +
> + 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. Can't think straight about it, but I think there is no need
for the parenthesis around the ||.
> + log_invalid_release(name, "Suite or Codename");
> +
> + /* Check if release can also be gotten using codename */
> + if ((release.status & IS_VALID) &&
> + release.name && !(release.status & GET_CODENAME)) {
> + cn_status = validate_codename(release.name, name);
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. ;)
> + 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.
> + if (!(cn_status & IS_VALID)) {
> + release.status &= ~IS_VALID;
> + log_invalid_release(name, "Codename");
> + } else {
> + release.status |= (cn_status & GET_CODENAME);
> + }
> + }
> + }
> +
> + /* In case there is no Codename field */
> + if ((release.status & IS_VALID) && ! release.name)
> + 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;
Bad API for C: returning release that way will copy the full struct
around, which is considered a bad thing to do. That returned struct will
also contain two newly allocated strings (by the strdup() earlier) that
must be freed by the caller; something not obvious when the caller is
not responsible for allocating the struct.
find_releases() actually forgets to free those two strings.
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'
The caller will be responsible for 'release' allocation, get_release()
will fill its field with proper value. Caller is also responsible to
free .name and .suite field.
validate_codename should also be modified in a similar way... and avoid
to call get_release() as said earlier. Defining cn_release could
probably also be removed from get_release() by accessing status field
directly.
> +}
> +
> +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");
> + }
> +
> + if (releases)
> + free(releases);
> + releases = malloc(MAXRELEASES * sizeof(struct release_t));
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];
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).
> + /* Get releases for all suites */
> + if (! base_on_cd) {
> + for (i=0; i < nbr_suites; i++) {
> + char *suite;
> +
> + suite = strdup(suites[i]);
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?
> + 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.
> [...]
> --- a/packages/choose-mirror/mirrors.h
> +++ b/packages/choose-mirror/mirrors.h
> @@ -10,16 +10,33 @@ struct mirror_t {
> };
>
> /*
> + * 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.
Looks like an interesting improvement though! :)
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature