Re: Call for tests: C rewrite of update-alternatives
Hi!
On Mon, 2010-06-07 at 22:22:00 +0200, Raphael Hertzog wrote:
> On Mon, 07 Jun 2010, Guillem Jover wrote:
> > 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.
I've just got used to seeing them, and actually when I use an
editor which does not show them, I feel naked. :)
> > > +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).
(Well I think that match is wrong then :)
> 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.
Personally I think that using tabs for indentation and spaces for
aligning, is the only style which is sane and democratic! :), it has
the nicest properties of them all, it just seems to me tools might be
lacking.
It preserves the structure of the text when quoting, it does too when
changing the tab size (so it caters for different tastes). If you have
them visible, it also helps (like if it was a ruler) see where each
indentation level begins. Using tabs (instead of spaces) for indenting
is the easiest way to guarantee there's always the correct amount of
spacing (not more or less, as in a missing space).
I don't think a checker will be able to catch all stylistic problems
anyway w/o understanding C in some cases, and having taste in others :).
Some constructs might be just plain ugly, and such a tool will not be
able to spot them either. A bug report to indent would be more
worthwhile (something I should have done long time ago), then we
could just ask people to run indent over it with a set of options.
I don't actually mind entirely meanwhile having to fix those after the
fact, or when applying them myself. So I'd really like to keep the
style as it is. And just for reference (but I think I might have
mentioned this before) I consider the current mix of tabs and spaces
in the perl code base just broken, but as Frank (at the time) and
you (later on) seemed to be fine with it, it just didn't seem worth to
try to push for a change there.
> [ 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.
Ah true!
> > 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.
Perfect!
> > > + 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.
It will be empty if the line starts with a literal NUL character (w/ or
w/o newline).
> > > +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? :)
Hmm, maybe something like (completely untested):
static char string[1024];
size_t cur_len;
string[0] = '\0';
cur_len = 0;
for (i = 0; i < argc; i++) {
size_t arg_len = strlen(argv[i]);
if (cur_len + arg_len + 2 > sizeof(string))
break;
if (cur_len) {
strcpy(string + cur_len, " ");
cur_len++;
}
strcpy(string + cur_len, argv[i]);
cur_len += arg_len;
}
? But mostly because we have to compute string's length anyway,
otherwise it might not even be worth it in this kind of case, more
so if it would make the code slightly more difficult to read.
> I pushed an updated and rebased branch at the same place with all
> required corrections. I will merge it to master soon.
I've skimmed over the branch. The asprintf might fail so it would need
to get wrapped too.
Also few more minor comments, it seems in alternative_set_selection(),
the “if (alternative_has_choice(a, choice)) {” could be folded into the
previous else as “} else if (...) {”. I try to avoid loops with empty
body constructs, which look a bit odd, in case of “for ( ; a; b) ;” I
switch them to “while (a)\n\tb;” because it also promotes the post
expression to be the main action, which more clearly reflects the
intent of the loop. The case of while loops with empty bodies usually
needs code reestructuration, which depending on if it clarifies or
complicates the code might not be worth it.
thanks,
guillem
Reply to: