On Fri, Jan 03, 2014 at 11:57:42AM -0800, Russ Allbery wrote:
> Here's a patch to teach update-passwd how to use debconf for
> prompting.
Thanks! I have a number of review comments below, but in general I'm
happy with the approach taken here and will be happy to merge it after a
few fixes. I very much appreciate your work here.
> * I'm not particularly proud of the code layout and repetition,
> particularly how changes to user information are handled.
> Unfortunately, this sort of thing is exactly what C is the worst
> at, and the only alternative I came up with would have involved
> passing a ton of parameters to a bunch of specialist functions.
Indeed. It's not ideal, but it's not as though update-passwd.c was a
model of clarity to begin with.
> * I spot-tested this patch and ran through a few obvious scenarios,
> but I didn't do exhaustive testing. It's possible that there are
> some fiddly bugs remaining in some of the specific cases of data
> changes for users.
I'll go through all the cases in detail once this is otherwise ready to
merge. Nothing jumped out at me upon skim-reading the code in question,
at least.
> * This patch converts postinst to use debconf unconditionally. This
> is more an artifact of how I tested it than an intentional decision.
> Given that base-passwd is an essential package, I suspect there are
> special considerations, but I've not done much work with essential
> packages before. I was hoping you'd sort out the right thing. :)
> Let me know if you need for me to make changes.
Yes, this will need a bit more work. Borrowing the approach from
libc6's postinst, I think the form should be:
# near top of postinst
if [ -f /usr/share/debconf/confmodule ]; then
. /usr/share/debconf/confmodule
fi
# later
if [ -f /usr/share/debconf/confmodule ]; then
# debconf style
else
# traditional style; maybe unset DEBIAN_HAS_FRONTEND for safety
fi
You could keep on redirecting update-passwd --dry-run's output to a
tempfile, and then just put the old cat/askyesno code in the else
branch.
(A long time ago, Santiago reported in #102395 that base-passwd may not
need to be Essential after all, but I have been very conservative about
changing anything here just in case ...)
> Some additional changes that are unrelated, and therefore I didn't
> include in this patch, but which I'd recommend making:
>
> * Now that there's an xasprintf function, you may want to use it in
> the few places where you're currently using asprintf. This would
> also get rid of some build warnings.
>
> * Since this is a native package and you're already running
> dh_autoreconf, I would recommend just deleting configure and
> config.h.in from the package.
I agree with both of these; I'd be happy to make these changes after we
land your patch (well, I might do the latter beforehand).
> +if ! update-passwd --dry-run > /dev/null ; then
> + . /usr/share/debconf/confmodule
In addition to my earlier comments, it's specifically inadvisable to
source /usr/share/debconf/confmodule anywhere other than very near the
top of a script, since it may re-exec the sourcing script. (libc6's
postinst is not the best example here, although in that case I think it
is only inefficient and a timebomb for future developers rather than
actually incorrect; in this case it appears to leak a tempfile.)
> + db_stop
db_stop doesn't generally do quite what people want; it's only necessary
at all when the script also starts a daemon, and it can have some weird
side-effects. You can probably just drop this.
> diff --git a/debian/templates b/debian/templates
> new file mode 100644
> index 0000000..1185b05
> --- /dev/null
> +++ b/debian/templates
> @@ -0,0 +1,229 @@
> +Template: base-passwd/user-move
> +Type: boolean
> +Default: false
I'd be inclined to make the Default value for all these templates "true"
rather than "false"; it seems like a more reasonable default to ensure
that things are synced up with the master files, especially since these
questions are asked any time we change the master files regardless of
whether there are local customisations, and you're generally asking
things at medium so most users won't have the opportunity to answer yes
to these questions.
> +_Description: Do you want to move the user ${name}?
> + update-passwd has found a difference between your system accounts and the
> + current Debian defaults. It is advisable to allow update-passwd to
> + change your system; without those changes some packages might not work
> + correctly. For more documentation on the Debian account policies please
> + see /usr/share/doc/base-passwd/README.
Nit: I think I'd prefer a comma before "please" here and in the other
similar templates.
> + Move user "${name}" (${id}) to before the "+" entry
I wonder if it's worth saying something about NIS compat in this and in
base-passwd/group-move; not that this is original to your patch, but
it's worth thinking about since these strings will be translated.
> --- a/man/update-passwd.8
> +++ b/man/update-passwd.8
> @@ -63,6 +63,18 @@ Show a summary of how to use
> .TP
> .BR \-V ,\ \-\-version
> Show the version number
> +.SH ENVIRONMENT
> +.TP
> +DEBCONF_HAS_FRONTEND
This should be spelled DEBIAN_HAS_FRONTEND. It's correct elsewhere in
your patch, but wrong here and in the commit message.
> +/* Abort the program if talking to debconf fails. Only use ret once. */
> +#define DEBCONF_CHECK(ret) \
Maybe "Use ret exactly once", since it's important that it be evaluated.
> +/* asprintf() with out-of-memory checking. Also fail if formatting fails so
> + * that the caller doesn't have to check any error return.
> + */
> +void xasprintf(char** strp, const char* fmt, ...) {
This is a strange prototype for xasprintf. The one I usually see (e.g.
the one found in gnulib) is:
char* xasprintf(const char* fmt, ...)
I'd prefer this latter version.
> +/* Assuming that we've already queued up a debconf question using REGISTER
> and
> + * any necessary SUBST, ask the question and return the answer as a boolean
> + * flag. Aborts the problem on any failure.
> + */
> +int ask_debconf(const char* priority, const char* question) {
> + int ret;
> + char* response;
Maybe make this const. debconfclient_ret's return type ought to be
const too, although I suspect that's a pain to change now.
> + ret=debconf_input(debconf, priority, question);
> + if (ret==0)
> + ret=debconf_go(debconf);
> + else if (ret==30)
> + ret=0;
Some inconsistent-looking indentation here.
> +/* Escape an arbitrary string for use in a debconf question. We take the
> + * conservative approach of replacing all non-alphanumeric characters with
> + * underscores.
You could probably reasonably allow '-' too without much trouble.
> @@ -648,13 +784,35 @@ void process_old_entries(const struct _info* lst,
> struct _node** passwd, struct
>
> if (find_by_named_entry(master, walk)==NULL) {
> struct _node* oldnode=walk;
> + int make_change=1;
> +
> + if (flag_debconf) {
> + char* question;
> + char* template;
> + char* id;
> + const char* domain=user_domain;
> +
> + if (strcmp(descr, "group")==0)
> + domain=group_domain;
> + xasprintf(&question, "base-passwd/%s/%s/%s/remove", domain,
> descr, oldnode->name);
> + xasprintf(&template, "base-passwd/%s-remove", descr);
> + xasprintf(&id, "%u", oldnode->id);
> + DEBCONF_REGISTER(template, question);
> + DEBCONF_SUBST(question, "name", oldnode->name);
> + DEBCONF_SUBST(question, "id", id);
> + make_change=ask_debconf("medium", question);
> + free(question);
> + free(template);
> + free(id);
> + }
I wonder if removals perhaps merit being asked at high rather than
medium? Same question for UID/GID changes, and perhaps home directory
changes too; any of these might potentially disrupt local customisations
in a way that additions don't.
Thanks,
--
Colin Watson [cjwat...@debian.org]
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
