On Sun, Nov 15, 2009 at 12:32:35AM +0100, Frans Pop wrote:
> 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 :-)
Great. :)
> Could you go through it once more and maybe look closely at freeing release
> and other vars again?
Here we go:
> diff --git a/packages/choose-mirror/choose-mirror.c b/packages/choose-mirror/choose-mirror.c
> [...]
> +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);
Not obvious, but this is fine as get_release() starts by setting both to
NULL. I tend to explicitely start this kind of code by using something
like:
memset(&release, 0, sizeof (struct release_t))
But that's a matter of choice...
> [...]
> + /* 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");
> + }
> + }
How about moving the if block inside validate_codename()? Thus you
would be able to pass whole release that needs to be updated. Once
again, a matter of taste, though.
> [...]
> + return (release->name != NULL);
No need for the parenthesis.
> [...]
> +static int find_releases(void) {
> [...]
Looks fine. I would probably not do it that way, but it looks fine.
One thing that is not usually done is the full copy of 'struct
release_t' from 'release' to 'releases'. But that's fine as it is.
> [...]
> static int choose_suite(void) {
> [...]
> + char **choices_c, **choices, *list;
> [...]
> + choices_c = malloc(MAXRELEASES * sizeof(char*));
> + choices = malloc(MAXRELEASES * sizeof(char*));
Once again, candidates being statically allocated. And as those arrays
are only used to build a list suitable for debconf_list, you can even
declare the char 'const'.
> + 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?
Exactly.
> But then I can't free name where I do now, right?
Right. But those strings need to be released when cleaning up
'releases', see below.
> +
> + 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);
Previously allocated list has been lost: memory leak. Should be free()'d
after given to debconf_subst().
> [...]
> +int set_codename (void) {
> [...]
> + 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;
That call to free() actually releases the memory for one string or
another of the strings reachable by 'releases'. See below.
> [...]
> }
If I got it right, 'releases' become useless after set_codename(). So
all allocated .name and .suite should all free()'d at that point.
> > 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.
Thanks for the insight.
> > 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.
I was only suggesting this as an optimisation, but yes, not really
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.
Actually I had missed the fact that Release files might differ. So
that's far from the worst path to take.
> [...]
> > 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.
You never access the array before filling meaningful value, so it's
indeed fine; but it has been observed that unitialized memory issues can
be hard to debug, hence the suggestion. :)
> [...]
> > 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...
True. Same comments actually apply to mirror_t. :)
Cheers,
--
Jérémy Bobbio .''`.
lunar@debian.org : :Ⓐ : # apt-get install anarchism
`. `'`
`-
Attachment:
signature.asc
Description: Digital signature