FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Debian > Debian dpkg

 
 
LinkBack Thread Tools
 
Old 05-26-2010, 07:01 PM
Raphael Hertzog
 
Default Call for tests: C rewrite of update-alternatives

Hi,

I just completed the C rewrite of update-alternatives. It passes the test
suite (which covers many use cases) but I would still like to have some
beta-testers before I push that to master.

So please test what's in my pu/u-a-c-rewritre branch and report back:
http://git.debian.org/?p=users/hertzog/dpkg.git;a=shortlog;h=refs/heads/pu/u-a-c-rewrite
git://git.debian.org/~hertzog/dpkg.git

Code reviews are welcome too but note that the available functions
clearly map what was in the perl version and the available API is not
super-clean. In the longer term (post-squeeze) I intend to fix that so
that we can reuse that API within dpkg and fix bugs like #25759.

Thanks!

Cheers,
--
Raphal 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/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100526190110.GB6201@rivendell">http://lists.debian.org/20100526190110.GB6201@rivendell
 
Old 05-27-2010, 07:59 AM
Peter Krefting
 
Default Call for tests: C rewrite of update-alternatives

Raphael Hertzog:


So please test what's in my pu/u-a-c-rewritre branch and report back:
http://git.debian.org/?p=users/hertzog/dpkg.git;a=shortlog;h=refs/heads/pu/u-a-c-rewrite
git://git.debian.org/~hertzog/dpkg.git


Will you move the translations over along with it (since scripts have their
own namespace for translations), or should we translators do that manually
on the next update?


--
// Peter - http://www.softwolves.pp.se/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: alpine.DEB.2.00.1005270858500.22019@ds9.cixit.se"> http://lists.debian.org/alpine.DEB.2.00.1005270858500.22019@ds9.cixit.se
 
Old 05-27-2010, 08:11 AM
Raphael Hertzog
 
Default Call for tests: C rewrite of update-alternatives

On Thu, 27 May 2010, Peter Krefting wrote:
> Raphael Hertzog:
>
> >So please test what's in my pu/u-a-c-rewritre branch and report back:
> >http://git.debian.org/?p=users/hertzog/dpkg.git;a=shortlog;h=refs/heads/pu/u-a-c-rewrite
> >git://git.debian.org/~hertzog/dpkg.git
>
> Will you move the translations over along with it (since scripts
> have their own namespace for translations), or should we translators
> do that manually on the next update?

Nothing needs to be done. update-alternatives's translations were always
part of dpkg's po file since it's part of dpkg and not dpkg-dev.

I also kept all strings the same (even if it was inconvenient sometimes).
There are some new strings due to error checking but I reused strings for
similar errors that were already used in dpkg.

Cheers,
--
Raphal 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/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100527081129.GE11787@rivendell">http://lists.debian.org/20100527081129.GE11787@rivendell
 
Old 05-27-2010, 08:15 AM
Peter Krefting
 
Default Call for tests: C rewrite of update-alternatives

Raphael Hertzog:

Nothing needs to be done. update-alternatives's translations were always
part of dpkg's po file since it's part of dpkg and not dpkg-dev.


Ah, good. I thought they were in the scripts PO file, but didn't actually
check first. Sorry about that :-)


I also kept all strings the same (even if it was inconvenient sometimes).
There are some new strings due to error checking but I reused strings for
similar errors that were already used in dpkg.


Nice, thank you!

--
// Peter - http://www.softwolves.pp.se/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: alpine.DEB.2.00.1005270914340.22019@ds9.cixit.se"> http://lists.debian.org/alpine.DEB.2.00.1005270914340.22019@ds9.cixit.se
 
Old 05-27-2010, 10:19 AM
Raphael Hertzog
 
Default Call for tests: C rewrite of update-alternatives

On Wed, 26 May 2010, Raphael Hertzog wrote:
> So please test what's in my pu/u-a-c-rewritre branch and report back:
> http://git.debian.org/?p=users/hertzog/dpkg.git;a=shortlog;h=refs/heads/pu/u-a-c-rewrite
> git://git.debian.org/~hertzog/dpkg.git

I have built packages for people who don't want to build from git:
http://people.debian.org/~hertzog/packages/dpkg_1.15.8_i386.deb
http://people.debian.org/~hertzog/packages/dpkg_1.15.8.dsc

I mentionned those in a blog post too to reach a broader audience:
http://www.ouaza.com/wp/2010/05/27/rewriting-update-alternatives-in-c/

Cheers,
--
Raphal 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/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100527101918.GA554@rivendell">http://lists.debian.org/20100527101918.GA554@rivendell
 
Old 06-07-2010, 02:18 AM
Guillem Jover
 
Default Call for tests: C rewrite of update-alternatives

Hi!

On Wed, 2010-05-26 at 21:01:10 +0200, Raphael Hertzog wrote:
> I just completed the C rewrite of update-alternatives. It passes the test
> suite (which covers many use cases) but I would still like to have some
> beta-testers before I push that to master.

Perfect!

> So please test what's in my pu/u-a-c-rewritre branch and report back:
> http://git.debian.org/?p=users/hertzog/dpkg.git;a=shortlog;h=refs/heads/pu/u-a-c-rewrite
> git://git.debian.org/~hertzog/dpkg.git
>
> Code reviews are welcome too but note that the available functions
> clearly map what was in the perl version and the available API is not
> super-clean. In the longer term (post-squeeze) I intend to fix that so
> that we can reuse that API within dpkg and fix bugs like #25759.

Ok so as this is a direct conversion from perl to C, I'll only comment
on things that are regressions compared to the perl code, coding style
issues and similar, further refactoring can happen later on, and I'd
like to avoid intermixing those. For recurring issues I'll try to
mention them just once. There's maybe some stuff I've missed but this
will do for now . So here's a code review:

> diff --git a/utils/update-alternatives.c b/utils/update-alternatives.c
> new file mode 100644
> index 0000000..1b6e4ff
> --- /dev/null
> +++ b/utils/update-alternatives.c

> +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)"”.

> +/* Action to perform */
> +static const char *action = "";

Make this NULL, then you don't need to strlen() == 0 on it.

> +/* Skip alternatives properly configured in auto mode (for --config) */
> +static int skip_auto = 0;
> +static int verbosemode = 0;
> +static int force = 0;

Prefix these flags with opt_ so that they are somewhat namespaced.

> +static char* pass_opts[MAX_OPTS];

Space between type and asterisk (recurring).

> +static void DPKG_ATTR_NORET

You can mark this and the rest of reporting functions with
DPKG_ATTR_PRINTF or DPKG_ATTR_VPRINTF.

> +error(char const *fmt, ...)
> +{
> + va_list ap;

Name these ‘args’ instead of ap, I've now made the existing code
consistent with this (recurring).

> + fprintf(stderr, "%s: %s: ", progname, _("error"));
> + va_start(ap, fmt);

Uses spaces instead of a tab (recurring).

In vim I use something like:

,---
let c_space_errors=1
set list
set listchars=tab:»-,trail:·
`---

to assist me spot those.

> + vfprintf(stderr, fmt, ap);
> + va_end(ap);
> + fprintf(stderr, "
");
> + exit(2);
> +}

> +static void
> +pr(char const *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vprintf(fmt, ap);
> + va_end(ap);
> + printf("
");
> +}
> +
> +

Two blank lines (recurring).

> +static char *
> +xstrdup(const char *str)
> +{
> + char * new_str;

Additional space after asterisk (recurring).

> +static void
> +set_action(const char* new_action)
> +{
> + if (strlen(action)) {
> + badusage(_("two commands specified: --%s and --%s"), action, new_action);
> + }

Unneeded curly braces.

> + action = new_action;
> +}
> +
> +static FILE *fh_log = NULL;
> +
> +static void
> +log_msg(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + /* XXX: the C rewrite must use the std function to get the
> + * filename from /etc/dpkg/dpkg.cfg or from command line. */

This comment does not make sense anymore, just remove or update.

> + if (fh_log == NULL && (access(log_file, W_OK) == 0 || errno == ENOENT)) {

No need to access() here, just try to fopen(), and do not consider an
error if errno == ENOENT or EACCESS, only warn or ignore for the others.

> + fh_log = fopen(log_file, "a");
> + if (fh_log == NULL) {
> + error(_("cannot append to %s: %s"), log_file, strerror(errno));
> + }
> + }
> +
> + if (fh_log) {
> + char timestamp[64];
> + time_t now;

Missing blank line between variable declarations and code (recurring).

> + time(&now);
> + strftime(timestamp, 64, "%Y-%m-%d %H:%M:%S", localtime(&now));

Use sizeof instead of hardcoding the sizes, it's safer in case of future
changes (recurring).

> + fprintf(fh_log, "%s %s: ", timestamp, progname);
> + va_start(ap, fmt);
> + vfprintf(fh_log, fmt, ap);
> + va_end(ap);
> + fprintf(fh_log, "
");
> + }
> +}
> +
> +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).

> + return 0;
> + return 1;
> +}

> +static int
> +spawn(const char *prog, const char *args[])
> +{
> + const char **cmd;
> + int i = 0;
> + pid_t pid, r;
> + int status;
> +
> + while (args[i++]);
> + cmd = xmalloc(sizeof(char*) * (i + 2));
> + cmd[0] = prog;
> + i = 0;
> + while (args[i]) {
> + cmd[i + 1] = args[i];
> + i++;
> + }

With this kind of pattern, just use a for () (recurring).

> + cmd[i + 1] = NULL;
> +
> + pid = fork();
> + if (pid == -1)
> + error(_("fork failed"));
> + if (pid == 0) {
> + execvp(prog, (char * const *)cmd);
> + error(_("failed to execute %s: %s"), prog, strerror(errno));
> + }
> + while ((r = waitpid(pid, &status, 0)) == -1 && errno == EINTR) ;
> + if (r != pid)
> + error(_("wait for subprocess %s failed"), prog);
> + free(cmd);
> +
> + return status;
> +}

> +static void
> +subcall(int count, ...)
> +{
> + va_list ap;
> + const char **cmd;
> + int res, i = 0, j;
> +
> + cmd = xmalloc(sizeof(*cmd) * (nb_opts + count + 1));
> + for (j = 0; j < nb_opts; j++)
> + cmd[i++] = pass_opts[j];
> + va_start(ap, count);
> + while (count-- > 0)
> + cmd[i++] = va_arg(ap, char*);
> + va_end(ap);
> + cmd[i++] = NULL;

Instead of using count to specify how many arguments, just use a NULL
sentinel at the end. The function can be marked DPKG_ATTR_SENTINEL
then. You'll just need to do a first pass to count the arguments.

It could be argued that as this function should actually go at some
point, and u-a should just call internal functions instead of invoking
itself, there's no point in doing this change, but keeping the count
and the arguments in sync is a recipe for disaster, so better let the
computer keep track of it.

> + res = spawn(prog_path, cmd);
> + if (WIFEXITED(res) && WEXITSTATUS(res) == 0)
> + return;
> + if (WIFEXITED(res))
> + exit(WEXITSTATUS(res));
> + exit(128);
> +}

> +void
> +fileset_free(struct fileset *fs) {

Curly brace on the next line.

> + struct slave_file *slave = fs->slaves;
> + free(fs->master_file);
> + while (slave) {
> + struct slave_file *next = slave->next;
> + free(slave->name);
> + if (slave->file)
> + free(slave->file);

No need to check for NULL, free() is a noop in that case (recurring).

> + free(slave);
> + slave = next;
> + }
> + free(fs);
> +}
> +
> +void
> +fileset_add_slave(struct fileset *fs, const char* name, const char* file)
> +{
> + struct slave_file *sl, *cur, *prev = NULL;
> +
> + /* Replace existing first */
> + cur = fs->slaves;
> + while (cur) {
> + if (strcmp(cur->name, name) == 0) {
> + if (cur->file)
> + free(cur->file);
> + cur->file = xstrdup(file);
> + return;
> + }
> + prev = cur;
> + cur = cur->next;
> + }

Using for () with this pattern is more natural as well (recurring).

> + /* Otherwise add new at the end */
> + sl = xmalloc(sizeof(*sl));
> + sl->next = NULL;
> + sl->name = xstrdup(name);
> + sl->file = xstrdup(file);
> + if (prev)
> + prev->next = sl;
> + else
> + fs->slaves = sl;
> +}
> +
> +const char*
> +fileset_get_slave(struct fileset *fs, const char *name)
> +{
> + struct slave_file *slave;
> +
> + if (fs->slaves == NULL)
> + return NULL;

No need for this check, if fs->slaves is NULL the loop will not get
executed anyway.

> + for (slave = fs->slaves; slave; slave = slave->next) {
> + if (strcmp(slave->name, name) == 0)
> + return slave->file;
> + }
> + return NULL;
> +}

> +void
> +alternative_reset(struct alternative *alt)
> +{
> + struct slave_link *slave;
> + struct fileset *fs;
> + struct commit_operation *commit_op;
> +
> + if (alt->master_link) {
> + free(alt->master_link);
> + alt->master_link = NULL;
> + }
> + while (alt->slaves) {
> + slave = alt->slaves;
> + alt->slaves = slave->next;
> + free(slave->name);
> + free(slave->link);
> + free(slave);

This can be refactored into a function to free a slave_link, which can
be used also in alternative_save().

> + }
> + while (alt->choices) {
> + fs = alt->choices;
> + alt->choices = fs->next;
> + fileset_free(fs);
> + }
> + while (alt->commit_ops) {
> + commit_op = alt->commit_ops;
> + alt->commit_ops = commit_op->next;
> + if (commit_op->arg_a)
> + free(commit_op->arg_a);
> + if (commit_op->arg_b)
> + free(commit_op->arg_b);
> + free(commit_op);

This can also be refactored too into a function used also in
alternative_commit().

> + }
> + 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.

> +}

> +static int
> +compare_fileset (const void *va, const void *vb)

No space after the function name (recurring).

> +{
> + struct fileset *a, *b;
> + a = *((struct fileset **) va);
> + b = *((struct fileset **) vb);

No space before the cast. You can make these const, and assign in the
declaration as in pkg_sorter_by_name() (recurring).

> + assert(a && a->master_file);
> + assert(b && b->master_file);
> + return strcmp(a->master_file, b->master_file);
> +}

> +void
> +alternative_add_slave(struct alternative *a, const char *slave_name, const char *slave_link)

Wrap the arguments into the next line.

> +bool
> +alternative_remove_choice(struct alternative *a, const char *file)
> +{
> + struct fileset *fs = a->choices;
> + struct fileset *fs_dropped;
> +
> + if (fs == NULL)
> + return false;

Not really needed as the other similar case.

> + if (strcmp(fs->master_file, file) == 0) {
> + a->choices = fs->next;
> + fileset_free(fs);
> + a->modified = true;
> + return true;
> + }

And this can be folded into the next for () loop, as long as you
operate on fs instead of fs->next, and keep a pointer for prev.

> + while (fs->next) {
> + if (strcmp(fs->next->master_file, file) == 0) {
> + fs_dropped = fs->next;
> + fs->next = fs->next->next;
> + fileset_free(fs_dropped);
> + a->modified = true;
> + return true;
> + }
> + fs = fs->next;
> + }
> +
> + return false;
> +}

> +struct altdb_context {
> + FILE *fh;
> + const char *filename;
> + void (*bad_format)(struct altdb_context *, const char *format, ...);
> + jmp_buf on_error;
> +};

> +char *
> +altdb_get_line(struct altdb_context *ctx, const char *name)
> +{
> + char buf[1024];

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.

I've added now compat code for asprintf, which does not help for the
fgets case, but does for other cases of hardcoded buffer sizes.

In any case, if you don't feel like tackling this right now, at least
use macros instead of literals for the values (recurring).

> + char *line;
> + size_t len;
> +
> + errno = 0;
> +
> + 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] != '
') {

The line could be empty so this would access out of bounds (and will
most probably cause a crash on fortify enabled builds).

> + ctx->bad_format(ctx, _("line too long or not terminated while "
> + "trying to read %s"), name);
> + }
> + line[len - 1] = '';
> +
> + return xstrdup(line);
> +}

> +bool
> +alternative_load(struct alternative *a, bool must_not_die)
> +{
> + struct altdb_context ctx;
> + struct stat st;
> + char fn[1024], *line;
> + bool modified = false;
> +
> + /* Initialize parse context */
> + if (setjmp(ctx.on_error)) {
> + if (ctx.fh)
> + fclose(ctx.fh);
> + alternative_reset(a);
> + return false;
> + }
> + if (must_not_die)
> + ctx.bad_format = altdb_interrupt_parsing;
> + else
> + ctx.bad_format = altdb_parse_error;
> + snprintf(fn, sizeof(fn), "%s/%s", admdir, a->master_name);

This one can be easily replaced with asprintf (recurring).

> + ctx.filename = fn;
> +
> + /* Verify the alternative exists */
> + if (stat(ctx.filename, &st) == -1) {
> + if (errno == ENOENT)
> + return false;
> + else
> + error(_("unable to stat %s: %s"), ctx.filename,
> + strerror(errno));
> + }
> + if (st.st_size == 0) {
> + return false;
> + }
> +
> + /* Open the database file */
> + ctx.fh = fopen(ctx.filename, "r");
> + if (ctx.fh == NULL)
> + error(_("unable to read %s: %s"), ctx.filename, strerror(errno));
> +
> + /* Start parsing mandatory attributes (link+status) of the alternative */
> + alternative_reset(a);
> + line = altdb_get_line(&ctx, _("status"));
> + if (strcmp(line, "auto") != 0 && strcmp(line, "manual") != 0)
> + ctx.bad_format(&ctx, _("invalid status"));
> + alternative_set_status(a, (strcmp(line, "auto") == 0) ?
> + ALT_ST_AUTO : ALT_ST_MANUAL);

Align with spaces.

> + free(line);

It seems there's some cases of unneeded strdups in the setters, as the
argument then needs to be freed. Probably better to just change the
setters to expect an allocated string and just assign it.

> + line = altdb_get_line(&ctx, _("master link"));
> + alternative_set_link(a, line);
> + free(line);
> +
> + /* Parse the description of the slaves links of the alternative */
> + while(strlen(line = altdb_get_line(&ctx, _("slave name")))) {

For clarity move assignment and later strlen check inside body, so
that we can have a more meaningful vaiable name too, with a smaller
scope (recurring).

Missing space after while (recurring).

> + char * link = altdb_get_line(&ctx, _("slave link"));
> + struct slave_link *sl;
> + /* XXX: Memory leak on error */
> + if (alternative_has_slave(a, line))
> + ctx.bad_format(&ctx, _("duplicate slave %s"), line);
> + if (strcmp(link, a->master_link) == 0)
> + ctx.bad_format(&ctx, _("slave link same as main link %s"),
> + link);
> + for(sl = a->slaves; sl; sl = sl->next) {
> + if (strcmp(link, sl->link) == 0)
> + ctx.bad_format(&ctx, _("duplicate slave link %s"),
> + link);
> + }
> + alternative_add_slave(a, line, link);
> + free(line);
> + free(link);
> + }
> +
> + /* Parse the available choices in the alternative */
> + while(strlen(line = altdb_get_line(&ctx, _("master file")))) {
> + struct fileset *fs;
> + struct slave_link *sl;
> + struct stat st;
> +
> + for (fs = a->choices; fs; fs = fs->next) {
> + if (strcmp(fs->master_file, line) == 0)
> + ctx.bad_format(&ctx, _("duplicate path %s"), line);
> + }
> + if (stat(line, &st) != -1) {

Try to have first the block dealing with error conditions. Also it's
clearer (visually faster) to have positive checks for things that are
error codes (instead of boolean values).

> + char *endptr, *prio;
> + long int iprio;
> +
> + prio = altdb_get_line(&ctx, _("priority"));
> + iprio = strtol(prio, &endptr, 10);
> + if (*endptr != '')
> + ctx.bad_format(&ctx, _("priority of %s: %s"),
> + line, prio);
> + fs = fileset_new(line, (int) iprio);
> + for (sl = a->slaves; sl; sl = sl->next) {
> + fileset_add_slave(fs, sl->name,
> + altdb_get_line(&ctx, _("slave file")));
> + }

If the indentation gets too deep, that usually means this needs to be
split into a new function, probably one for each of the two while parser
loops in this function.

> + alternative_add_choice(a, fs);
> + } else {
> + if (errno != ENOENT)
> + error(_("cannot stat %s: %s"), line,
> + strerror(errno));
> + /* File not found - remove. */
> + if (! must_not_die)

No space after unary operators (recurring).

> + warning(_("alternative %s (part of link group "
> + "%s) doesn't exist. Removing from list "
> + "of alternatives."), line,
> + a->master_name);
> + altdb_get_line(&ctx, _("priority"));
> + for (sl = a->slaves; sl; sl = sl->next)
> + altdb_get_line(&ctx, _("slave file"));
> + modified = true;
> + }

Leaks line.

> + }
> +
> + /* Close database file */
> + if (fclose(ctx.fh))
> + error(_("unable to close %s: %s"), ctx.filename, strerror(errno));
> +
> + /* Initialize the modified field which has been erroneously changed
> + * by the various alternative_(add|set)_* calls */
> + a->modified = modified; /* false unless a choice has been auto-cleaned */

Merge the lateral comment to the one above the code.

> +
> + return true;
> +}
> +
> +
> +void
> +alternative_save(struct alternative *a, const char *file)
> +{
> + struct altdb_context ctx;
> + struct slave_link *sl = a->slaves, *prevsl = NULL;

Split the variable name with underscore to make it visually clearer,
something like sl_prev.

> + struct fileset *fs;
> +
> + /* Cleanup unused slaves before writing admin file. */
> + while (sl) {
> + int has_slave = 0;

This can be a bool...

> + for (fs = a->choices; fs; fs = fs->next) {
> + if (fileset_has_slave(fs, sl->name))
> + has_slave++;

... as this can skip right away on first match.

> + }
> + if (has_slave == 0) {
> + struct slave_link *slrm;
> + verbose(_("discarding obsolete slave link %s (%s)."),
> + sl->name, sl->link);
> + if (prevsl)
> + prevsl->next = sl->next;
> + else
> + a->slaves = sl->next;
> + slrm = sl;
> + sl = sl->next;
> + free(slrm->name);
> + free(slrm->link);
> + free(slrm);
> + } else {
> + prevsl = sl;
> + sl = sl->next;
> + }
> + }
[...]
> +}

> +struct fileset*
> +alternative_get_best(struct alternative *a)
> +{
> + struct fileset *fs, *best;
> + best = a->choices;
> + for (fs = a->choices; fs; fs = fs->next)

Just assign “best = fs = a->choices”.

> + if (fs->priority > best->priority)
> + best = fs;
> + return best;
> +}

> +char *
> +alternative_get_current(struct alternative *a)
> +{
> + char curlink[1024], file[1024];
> + ssize_t size;
> + snprintf(curlink, 1024, "%s/%s", altdir, a->master_name);
> + curlink[1023] = '';
> +
> + if (! alternative_has_current_link(a))
> + return NULL;

> + size = readlink(curlink, file, 1024);

Allocate these dynamically, the size can be retrieved from an lstat
st_size member (recurring).

> + if (size == -1)
> + error(_("readlink(%s) failed: %s"), curlink, strerror(errno));
> + file[min(1023, size)] = '';
> + return xstrdup(file);
> +}

> +const char *
> +alternative_select_choice(struct alternative *a)
> +{
> + char *current, *ret, selection[128];
> + struct fileset *best, *fs;
> + long int len, index;

These two should be int not “long int”, as they are being used as
variables for format width and precision. This will cause run-time
problems on 64-bit architectures. I'm fairly certain this should cause
a warning, I've changed the build system to always enable the additional
compiler-warnings now, so we don't forget to pass it in the future.

> + current = alternative_get_current(a);
> + best = alternative_get_best(a);
> + assert(best);
> +
> + while (true) {

Use “for (;” to be consistent with the rest of the code base (recurring).

> + const char *mark;
[...]
> + if (*ret == '') {
> + /* Look up by index */
> + if (index == 0) {
> + alternative_set_status(a, ALT_ST_AUTO);
> + ret = best->master_file;
> + goto finish_choice;
> + }
> + fs = a->choices;
> + while (--index) {
> + fs = fs->next;
> + if (! fs)
> + break;
> + }

This can be easily a for () loop.

> + if (fs) {
> + alternative_set_status(a, ALT_ST_MANUAL);
> + ret = fs->master_file;
> + goto finish_choice;
> + }
> + } else {
> + /* Look up by name */
> + for (fs = a->choices; fs; fs = fs->next) {
> + if (strcmp(fs->master_file, selection) == 0) {
> + alternative_set_status(a, ALT_ST_MANUAL);
> + ret = fs->master_file;
> + goto finish_choice;
> + }
> + }
> + }
> + }
> +finish_choice:
> + if (current)
> + free(current);

I strongly try to avoid gotos, as they can be easily circumvented by
way of an additional function, by splitting the resource managment from
the actual processing, and then making the caller release the resource
on error code from the inner function. That's why having variables with
the smallest scope helps with seeing this kind of refactoring
possibility.

The only annying case is when one needs to pass a huge amount of state
to the inner function.

> + return ret;
> +}

> +void
> +alternative_add_commit_op(struct alternative *a, enum opcode opcode,
> + const char *arg_a, const char *arg_b)
> +{
> + struct commit_operation *op, *cur;
> +
> + op = xmalloc(sizeof(*op));
> + op->opcode = opcode;
> + op->arg_a = arg_a ? xstrdup(arg_a) : NULL;
> + op->arg_b = arg_b ? xstrdup(arg_b) : NULL;

xstrdup could be changed to handle NULL.

> +}

> +static const char *
> +get_argv_string(int argc, char **argv)
> +{
> + static char string[1024];
> + int i;
> +
> + string[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.

> + }
> + return string;
> +}

> +/*
> + * Main program
> + */
> +
> +#define MISSING_ARGS(nb) (argc < i + nb + 1)
> +
> +int
> +main(int argc, char **argv)
> +{

> + /* Load infos about all alternatives to be able to check for mistakes. */
> + alt_map_obj = alternative_map_new(NULL, NULL);
> + alt_map_links = alternative_map_new(NULL, NULL);
> + alt_map_parent = alternative_map_new(NULL, NULL);
> + struct dirent **table;

No C99 intermixed code and declarations (recurring);

> + count = get_all_alternatives(&table);
> + for (i = 0; i < count; i++) {
> + struct slave_link *sl;
> + struct alternative *a = alternative_new(table[i]->d_name);
> + if (! alternative_load(a, 1)) {

Use true instead of 1.

> + alternative_free(a);
> + free(table[i]);
> + continue;
> + }
[...]
> + }

> + /* Check that caller don't mix links between alternatives and don't mix
> + * alternatives between slave/master, and that the various parameters
> + * are fine. */
> + if (strcmp(action, "install") == 0) {
[...]
> + if (index(inst_alt->master_name, ' ') ||
> + index(inst_alt->master_name, ' ') ||
> + index(inst_alt->master_name, '/'))
> + error(_("alternative name (%s) must not contain '/' "
> + "and spaces."), inst_alt->master_name);

Use strpbrk instead.

> + for (sl = inst_alt->slaves; sl; sl = sl->next) {
[...]
> + if (index(sl->name, ' ') || index(sl->name, ' ') ||
> + index(sl->name, '/'))
> + error(_("alternative name (%s) must not contain '/' "
> + "and spaces."), sl->name);

Use strpbrk here too.

> + }
> + }

> + } else if (strcmp(action, "get-selections") == 0) {
> + struct alternative_map *am = alt_map_obj;
> + char *current;
> + while (am && am->item) {
> + current = alternative_get_current(am->item);
> + printf("%-30s %-8s %s
", am->key,
> + alternative_status_string(am->item->status),
> + (current != NULL) ? current : "");

Just do “current ? current : ""”.

> + if (current)
> + free(current);
> + am = am->next;
> + }
> + exit(0);
> + } else if (strcmp(action, "set-selections") == 0) {

> + } 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.

> + pr(_("There is only one alternative in link group %s: %s"),
> + a->master_name, cur);
> + pr(_("Nothing to configure."));
> + } else {
> + new_choice = alternative_select_choice(a);
> + }
> + } else if (strcmp(action, "remove") == 0) {
[...]
> + return 0;
> +}

Oh, forgot to mention in some of the code, sometimes it's too compact,
try to split with a blank line (but w/o abusing them) whenever a logical
block has finished, so that they can be more easily delimited visually.

regards,
guillem


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100607021856.GA988@gaara.hadrons.org">http://lists.debian.org/20100607021856.GA988@gaara.hadrons.org
 
Old 06-07-2010, 08:22 PM
Raphael Hertzog
 
Default 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 /(^| ) /

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] != '
') {
>
> 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] = '';
> > + 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/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100607202200.GA3464@rivendell">http://lists.debian.org/20100607202200.GA3464@rivendell
 
Old 06-09-2010, 08:10 PM
Guillem Jover
 
Default 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 /(^| ) /
>
> 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] != '
') {
> >
> > 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] = '';
> > > + 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] = '';
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)
b;” 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


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100609201047.GA12408@gaara.hadrons.org">http://lists.debian.org/20100609201047.GA12408@gaara.hadrons.org
 
Old 06-09-2010, 09:38 PM
Raphael Hertzog
 
Default Call for tests: C rewrite of update-alternatives

Hi,

On Wed, 09 Jun 2010, Guillem Jover wrote:
> > > 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 /(^| ) /
> >
> > 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.

Eventually I went for:
set listchars=tab:→*,trail:·

It's much less intrusive and is acceptable for me.

> 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 agree it's nice but I really don't care of people who try to use tabsize
different from the standard of 8 chars and the occasionnal bad indent
shown while quoting the code in a mail is really not much of a burden.

When I do a review, I concentrate on the code not on what kind of spaces
are used to align the wrapped line. As long as it's well aligned in my
editor that is...

> 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 .

It should simply catch the most common problems. It's just work that
I don't want to have to do, reviewing the style in every details for every
patch. I want people to be able to get it mostly right without me and I
want to be able to quickly verify it.

> 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

I have no problem keeping it as is, but I prefer to not have to enforce
it.

And really I would prefer if you could review more patch because you
don't feel obliged to replace tabs with spaces on submitted code. But
then, that's your call. :-)

> > 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).

Right, fixed.

> Hmm, maybe something like (completely untested):

Ok, adopted.

> I've skimmed over the branch. The asprintf might fail so it would need
> to get wrapped too.

Done.

> 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.

Would you prefer something like this to make it more obvious that
the action is really in the test?

while (do_something)
/* do nothing */;

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/


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Archive: 20100609213831.GA1920@rivendell">http://lists.debian.org/20100609213831.GA1920@rivendell
 

Thread Tools




All times are GMT. The time now is 05:34 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org