Re: Call for tests: C rewrite of update-alternatives
On Mon, 07 Jun 2010, Guillem Jover wrote:
> > +static const char *admdir = "/var/lib/dpkg/alternatives";
>
> admdir should not be hardcoded, concat to ADMINDIR, deriving it instead
> from Makefile.am with “-DADMINDIR=\"$(admindir)\"”.
Done and done the same for SYSCONFDIR in that case (for
/etc/alternatives) and LOGDIR for the log file.
> In vim I use something like:
>
> ,---
> let c_space_errors=1
> set list
> set listchars=tab:»-,trail:·
> `---
>
> to assist me spot those.
Thanks, I find this setting too much annoying. I just added this to catch
common mistakes:
let c_space_errors=1
highlight EightSpaces ctermbg=lightred guibg=lightred
match EightSpaces /\(^\|\t\) /
I find this less intrusive than seeing weird chars for all tabs.
> > +static int
> > +filter_altdir(const struct dirent *entry)
> > +{
> > + if (strcmp(entry->d_name, ".") == 0 ||
> > + strcmp(entry->d_name, "..") == 0 ||
> > + (strlen(entry->d_name) > strlen(DPKG_TMP_EXT) &&
> > + strcmp(entry->d_name + strlen(entry->d_name) -
> > + strlen(DPKG_TMP_EXT), DPKG_TMP_EXT) == 0))
>
> There's one tab instead of spaces in the last strlen; tabs to indent,
> spaces to align (recurring).
Can we revisit this requirement? I think it's useless and hard to
detect/fix automatically (on the contrary using spaces here is catched
as error by the simple syntax highlighting rule mentionned above).
The time spent reviewing those is not time usefully spent IMO. And I plan
to write a check_patch.pl script that should catch all stylistic issues we
want to see fixed and I don't see how I could enforce this one.
[ alternative_reset ]
> > + }
> > + alt->modified = false;
>
> The current funciton name does not seem to quite fit with the current
> functionality, reinitialize to NULL the struct members after freeing
> them. This is also safer in case we try to reuse or free again some
> freed pointer.
That's already the case. "while (alt-><member>)" means the loop will
end when the member is NULL, so they are effectively set to NULL in the
process.
> I'd rather not see such hardcoded limits, as there's systems were file
> names are not restricted. OTOH the rest of the dpkg code base has
> similar limits for conffile paths for example and others, and dpkg is
> currently also limited by the tar path limit, although u-a can be used
> to manage non-dpkg-managed paths.
Ok, used a dynamic buffer that I grow on demand.
> > + line = fgets(buf, sizeof(buf), ctx->fh);
> > + if (line == NULL) {
> > + if (errno)
> > + ctx->bad_format(ctx, _("while reading %s: %s"),
> > + name, strerror(errno));
> > + ctx->bad_format(ctx, _("unexpected end of file while trying "
> > + "to read %s"), name);
> > + }
> > +
> > + len = strlen(line);
> > + if (line[len - 1] != '\n') {
>
> The line could be empty so this would access out of bounds (and will
> most probably cause a crash on fortify enabled builds).
No, if the line is empty (not even a newline), fgets() returns NULL.
> > +static const char *
> > +get_argv_string(int argc, char **argv)
> > +{
> > + static char string[1024];
> > + int i;
> > +
> > + string[0] = '\0';
> > + for (i = 1; i < argc; i++) {
> > + if (strlen(string) + strlen(argv[i]) + 2 > sizeof(string))
> > + break;
> > + if (strlen(string))
> > + strcat(string, " ");
> > + strcat(string, argv[i]);
>
> Not really performance critical, but this seems a bit wasteful. :)
Any nice suggestion on how to implement it with less CPU cycles? :)
> > + } else if (strcmp(action, "config") == 0) {
> > + if (alternative_choices_count(a) == 0) {
> > + pr(_("There is no program which provides %s."),
> > + a->master_name);
> > + pr(_("Nothing to configure."));
> > + } else if (skip_auto && a->status == ALT_ST_AUTO) {
> > + alternative_display_user(a);
> > + } else if (alternative_choices_count(a) == 1 &&
> > + a->status == ALT_ST_AUTO &&
> > + alternative_has_current_link(a)) {
> > + char *cur = alternative_get_current(a);
>
> This can be const.
Nope, instead it should be freed.
I pushed an updated and rebased branch at the same place with all
required corrections. I will merge it to master soon.
Cheers,
--
Raphaël Hertzog
Like what I do? Sponsor me: http://ouaza.com/wp/2010/01/05/5-years-of-freexian/
My Debian goals: http://ouaza.com/wp/2010/01/09/debian-related-goals-for-2010/
Reply to: