Re: [RFC] choose-mirror rewrite - please review/test
On Sunday 15 November 2009, Jérémy Bobbio wrote:
> Here we go:
Many thanks again for the review.
I think the attached incremental patch addresses all your comments.
Could you check one last time if I got the memsets and freeing right now?
One question. I now do:
memset(&releases, 0, MAXRELEASES * sizeof(struct release_t));
Could that (and same for choices arrays) maybe be simplified to just:
memset(&releases, 0, sizeof(releases));
?
Cheers,
FJP
> 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.
Yep. Now that I'm actually start to get passing stuff by reference that
makes sense. Before it was simply beyond me to do it that way :-P
> > + return (release->name != NULL);
>
> No need for the parenthesis.
But rewritten to fix another possible memory leak.
> If I got it right, 'releases' become useless after set_codename(). So
> all allocated .name and .suite should all free()'d at that point.
As it's a global variable I think the best place to free it is simply at
the end. Does it really make any difference there (except for cleaner
code), given that choose-mirror will be terminated anyway?
> > > 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. :)
It also means that I don't feel very guilty about leaving it as is ;-)
commit f8eee46d4d645f9f7a4514a88864ed5285ff1bbb
Author: Frans Pop <fjp@debian.org>
Date: Sun Nov 15 21:44:00 2009 +0100
Change progress message to better what's happening
Especially with the new code multiple Release files get downloaded.
diff --git a/packages/choose-mirror/debian/choose-mirror-bin.templates-in b/packages/choose-mirror/debian/choose-mirror-bin.templates-in
index c59ec8c..021214d 100644
--- a/packages/choose-mirror/debian/choose-mirror-bin.templates-in
+++ b/packages/choose-mirror/debian/choose-mirror-bin.templates-in
@@ -33,7 +33,7 @@ _Description: Checking the Debian archive mirror
Template: mirror/checking_download
Type: text
# :sl1:
-_Description: Downloading the Release file...
+_Description: Downloading Release files...
Template: mirror/no-default
Type: boolean
Reply to: