Re: [patch] always send the full name on the --status-fd
[ CCing the cupt package as affected dpkg frontend, but not using the
alioth address as I usually cannot directly send there; leaving
previous context for the benefit of the new readers. ]
Hi!
Eugene, this is about adding a new option to replace --status-fd,
currently proposed as emitting deb822 format messages, I'd like your
input on this.
On Wed, 2013-10-23 at 12:40:24 +0200, Michael Vogt wrote:
> On Mon, Oct 21, 2013 at 07:54:22AM +0200, Guillem Jover wrote:
> > On Tue, 2013-10-15 at 19:39:32 +0200, Michael Vogt wrote:
> > > On Tue, Oct 15, 2013 at 05:39:39PM +0200, Guillem Jover wrote:
> > > > Help the progress reporting code as in making it simpler or something
> > > > else?
> > >
> > > It would help making the current code simpler and more consistent. But
> > > apt has enough information to work without it.
>
> I'm actually not so sure anymore if it is not required for correctness
> too, out-of-order messages like disappearing packages may need to be
> fully qualified, but I haven't investigated further as I hope we can
> get a new status-fd format :)
Sure.
> [..]
> > > > I guess I still have the same issue with this as when devising the
> > > > current rationale for this: backward-compatibility with other dpkg
> > > > users not switching to multiarch.
> > >
> > > This could be controlled via e.g. a switch, but given that apt would
> > > have to support the old behavior for some time I guess the net win is
> > > very small (for apt).
> >
> > If we are going to add a new format, we might as well add any
> > additional information that frontends might find useful and that makes
> > sense sending. We can also cleanup the format and make it more sane
> > and consistent. Perhaps also in deb822-style, inspired from the
> > APT::Status-deb822-Fd comment in apt's changelog. :) But perhaps
> > that complicates the parsing too much?
>
> Great, I think that is a good idea, apt has a parser for deb822, so it
> should not complicate stuff on our side. I attached a patch that
> outlines what it could look like. Feedback very welcome, it probably
> needs some more more work,
Thanks, although I was more interested in the needed information for
the messages, working code is good too :), but see my comments below.
> propably something like
> --assert-status-deb822 (unless I add a hard dependency)
The assert commands are there to be able to probe for internal features.
Adding a new assert for each new option sets a precedent I'm not sure
I'd want to follow. I guess the problem is that dpkg does not return a
meaningful error code on bad options, hmmm, I could fix that, but it
would be too late for this.
> and I'm not
> sure if the added test is a useful addition for the repo (it was/is
> for me during development).
I turned it into a test case for the dpkg/pkg-tests.git repo, see the
attached patch.
> > I'm not sure what's your policy regarding backward compat, and when
> > you consider a hard dependency on a newer dpkg version is enough to be
> > able to remove old code, but it should eventually make things better.
> > dpkg will not have that luxiry though. :)
> [..]
>
> Indeed.
Actually, dpkg could just do a normal deprecation process as usual,
introduce the new interface, mark old as deprecated, issue warnings
after a release cycle, and target for removal after another release
cycle.
> > In any case as mentioned above, I'm absolutely fine with adding an
> > explicit option to select a new status format. Either --status-fd-format
> > or similar. We'd need to come up first with that'd frontends need from
> > it.
>
> Great, hope the attached patch gives us a starting point, with deb822
> it should easy enough to expand what we send.
Ok, I've ended up with two new options which better describe their
usage --events-fd and --events-logger, and they have nicer names than
--status-fd-deb822 which we could end up with as the only option, if
--status-fd gets deprecated and eventually removed.
I renamed some of the fields, and reworked the per package errors so
that they emit an event for package errors and another one for archive
errors (pkgname vs filename). I'll push these preceding patches later
today, after I've done some final review/polish.
I'm not too sold on the action event, maybe it should be named
processing as the statusfd stuff, or something else. The Foo-Edited
values should probably be strings instead of integers. And I'm wondering
if adding more information would be useful, stuff like package Version
for example. The nice thing is that we can always add new fields, safely.
Thanks,
Guillem
From 07b424bf9fb5eccaad0660246520e5989c9d5b20 Mon Sep 17 00:00:00 2001
From: Guillem Jover <guillem@debian.org>
Date: Sat, 2 Nov 2013 21:56:37 +0100
Subject: [PATCH] dpkg: Add new --events-fd and --events-logger options
Based-on-patch-by: Michael Vogt <mvo@debian.org>
---
lib/dpkg/dbmodify.c | 5 +++++
lib/dpkg/dpkg.h | 3 +++
lib/dpkg/libdpkg.map | 2 ++
lib/dpkg/log.c | 59 +++++++++++++++++++++++++++++++++++++++++++---------
man/dpkg.1 | 37 ++++++++++++++++++++++++++++++++
src/configure.c | 8 +++++++
src/errors.c | 8 +++++++
src/help.c | 7 +++++++
src/main.c | 29 ++++++++++++++++++++++++++
9 files changed, 148 insertions(+), 10 deletions(-)
diff --git a/lib/dpkg/dbmodify.c b/lib/dpkg/dbmodify.c
index 6fd2151..1dd8066 100644
--- a/lib/dpkg/dbmodify.c
+++ b/lib/dpkg/dbmodify.c
@@ -413,6 +413,11 @@ void modstatdb_note(struct pkginfo *pkg) {
versiondescribe(&pkg->installed.version, vdew_nonambig));
statusfd_send("status: %s: %s", pkg_name(pkg, pnaw_nonambig),
statusinfos[pkg->status].name);
+ dpkg_event_emit("Event: state-change\n"
+ "Package: %s\n"
+ "Status: %s\n",
+ pkg_name(pkg, pnaw_always),
+ statusinfos[pkg->status].name);
if (cstatus >= msdbrw_write)
modstatdb_note_core(pkg);
diff --git a/lib/dpkg/dpkg.h b/lib/dpkg/dpkg.h
index 55b373b..c5b6fe6 100644
--- a/lib/dpkg/dpkg.h
+++ b/lib/dpkg/dpkg.h
@@ -129,6 +129,9 @@ void log_message(const char *fmt, ...) DPKG_ATTR_PRINTF(1);
void statusfd_add(int fd);
void statusfd_send(const char *fmt, ...) DPKG_ATTR_PRINTF(1);
+void dpkg_event_add_listener(int fd);
+void dpkg_event_emit(const char *fmt, ...) DPKG_ATTR_PRINTF(1);
+
/*** cleanup.c ***/
void cu_closestream(int argc, void **argv);
diff --git a/lib/dpkg/libdpkg.map b/lib/dpkg/libdpkg.map
index 52c6368..b13b1ba 100644
--- a/lib/dpkg/libdpkg.map
+++ b/lib/dpkg/libdpkg.map
@@ -173,6 +173,8 @@ LIBDPKG_PRIVATE {
log_message;
# Action logging
+ dpkg_event_add_listener;
+ dpkg_event_emit;
statusfd_add;
statusfd_send;
diff --git a/lib/dpkg/log.c b/lib/dpkg/log.c
index ce9d1ea..c2a4a40 100644
--- a/lib/dpkg/log.c
+++ b/lib/dpkg/log.c
@@ -76,9 +76,10 @@ struct pipef {
};
static struct pipef *status_pipes = NULL;
+static struct pipef *events_pipes = NULL;
-void
-statusfd_add(int fd)
+static void
+pipes_fd_enqueue(struct pipef **pipes_head, int fd)
{
struct pipef *pipe_new;
@@ -86,15 +87,32 @@ statusfd_add(int fd)
pipe_new = nfmalloc(sizeof(struct pipef));
pipe_new->fd = fd;
- pipe_new->next = status_pipes;
- status_pipes = pipe_new;
+ pipe_new->next = *pipes_head;
+ *pipes_head = pipe_new;
+}
+
+static void
+pipes_fd_broadcast(struct pipef *pipes_head, struct varbuf vb)
+{
+ struct pipef *pipef;
+
+ for (pipef = pipes_head; pipef; pipef = pipef->next) {
+ if (fd_write(pipef->fd, vb.buf, vb.used) < 0)
+ ohshite(_("unable to write to status fd %d"),
+ pipef->fd);
+ }
+}
+
+void
+statusfd_add(int fd)
+{
+ pipes_fd_enqueue(&status_pipes, fd );
}
void
statusfd_send(const char *fmt, ...)
{
static struct varbuf vb;
- struct pipef *pipef;
va_list args;
if (!status_pipes)
@@ -109,9 +127,30 @@ statusfd_send(const char *fmt, ...)
varbuf_add_char(&vb, '\n');
va_end(args);
- for (pipef = status_pipes; pipef; pipef = pipef->next) {
- if (fd_write(pipef->fd, vb.buf, vb.used) < 0)
- ohshite(_("unable to write to status fd %d"),
- pipef->fd);
- }
+ pipes_fd_broadcast(status_pipes, vb);
+}
+
+void
+dpkg_event_add_listener(int fd)
+{
+ pipes_fd_enqueue(&events_pipes, fd);
+}
+
+void
+dpkg_event_emit(const char *fmt, ...)
+{
+ static struct varbuf vb;
+ va_list args;
+
+ if (!events_pipes)
+ return;
+
+ va_start(args, fmt);
+ varbuf_reset(&vb);
+ varbuf_vprintf(&vb, fmt, args);
+ /* Append final \n */
+ varbuf_add_char(&vb, '\n');
+ va_end(args);
+
+ pipes_fd_broadcast(events_pipes, vb);
}
diff --git a/man/dpkg.1 b/man/dpkg.1
index 19692bf..e661bd8 100644
--- a/man/dpkg.1
+++ b/man/dpkg.1
@@ -638,6 +638,43 @@ with a '\fB5\fP' on the third character. The line is followed by a space
and an attribute character (currently '\fBc\fP' for conffiles), another
space and the pathname.
.TP
+\fB\-\-events\-fd \fR\fIn\fR
+Send machine-readable package status and progress information to file
+descriptor \fIn\fP. This option can be specified multiple times. The
+information is in deb822 format, one paragraph per event, the following
+events are currently emitted:
+
+.nf
+Event: action
+Action: \fIaction-name\fP
+Package: \fIpackage-name\fP
+Version: \fIpackage-version\fP
+
+Event: state-change
+Package: \fIpackage-name\fP
+Status: \fIpackage-state\fP
+
+Event: error
+Package: \fIpackage-name\fP
+Message: \fIerror-message\fP
+
+Event: error
+Archive: \fIarchive-name\fP
+Message: \fIerror-message\fP
+
+Event: conffile-prompt
+Conffile-Path: \fIconffile-path\fP
+Conffile-Old: \fIold-conffile\fP
+Conffile-New: \fInew-conffile\fP
+User-Edited: \fIvalue\fP
+Dist-Edited: \fIvalue\fP
+.fi
+.TP
+\fB\-\-events\-logger\fR=\fIcommand\fR
+Send machine-readable package status and progress information to the
+shell \fIcommand\fR's standard input. This option can be specified
+multiple times. The output format used is the same as in \fB\-\-events\-fd.
+.TP
\fB\-\-status\-fd \fR\fIn\fR
Send machine-readable package status and progress information to file
descriptor \fIn\fP. This option can be specified multiple times. The
diff --git a/src/configure.c b/src/configure.c
index fd54582..ad4f4cc 100644
--- a/src/configure.c
+++ b/src/configure.c
@@ -699,6 +699,14 @@ promptconfaction(struct pkginfo *pkg, const char *cfgfile,
statusfd_send("status: %s : %s : '%s' '%s' %i %i ",
cfgfile, "conffile-prompt",
realold, realnew, useredited, distedited);
+ dpkg_event_emit("Event: conffile-prompt\n"
+ "Conffile-Path: %s\n"
+ "Conffile-Old: %s\n"
+ "Conffile-New: %s\n"
+ "User-Edited: %i\n"
+ "Dist-Edited: %i\n",
+ cfgfile, realold, realnew,
+ useredited, distedited);
do {
/* Flush the terminal's input in case the user involuntarily
diff --git a/src/errors.c b/src/errors.c
index 4fcfacc..a00ab67 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -85,6 +85,10 @@ print_error_perpackage(const char *emsg, const void *data)
what, cipaction->olong, emsg);
statusfd_send("status: %s : %s : %s", what, "error", emsg);
+ dpkg_event_emit("Event: error\n"
+ "Package: %s\n"
+ "Message: %s\n",
+ what, emsg);
enqueue_error_report(what);
}
@@ -98,6 +102,10 @@ print_error_perarchive(const char *emsg, const void *data)
what, cipaction->olong, emsg);
statusfd_send("status: %s : %s : %s", what, "error", emsg);
+ dpkg_event_emit("Event: error\n"
+ "Archive: %s\n"
+ "Message: %s\n",
+ what, emsg);
enqueue_error_report(what);
}
diff --git a/src/help.c b/src/help.c
index 2af873c..37d3991 100644
--- a/src/help.c
+++ b/src/help.c
@@ -403,4 +403,11 @@ log_action(const char *action, struct pkginfo *pkg, struct pkgbin *pkgbin)
versiondescribe(&pkg->available.version, vdew_nonambig));
statusfd_send("processing: %s: %s", action,
pkgbin_name(pkg, pkgbin, pnaw_nonambig));
+ dpkg_event_emit("Event: action\n"
+ "Action: %s\n"
+ "Package: %s\n"
+ "Version: %s\n",
+ action,
+ pkgbin_name(pkg, pkgbin, pnaw_always),
+ versiondescribe(&pkgbin->version, vdew_nonambig));
}
diff --git a/src/main.c b/src/main.c
index 1f6a271..4aece71 100644
--- a/src/main.c
+++ b/src/main.c
@@ -147,6 +147,8 @@ usage(const struct cmdinfo *ci, const char *value)
" --no-act|--dry-run|--simulate\n"
" Just say what we would do - don't do it.\n"
" -D|--debug=<octal> Enable debugging (see -Dhelp or --debug=help).\n"
+" --events-fd <n> Send events to file descriptor <n>.\n"
+" --events-logger=<command> Send events to <command>'s stdin.\n"
" --status-fd <n> Send status change updates to file descriptor <n>.\n"
" --status-logger=<command> Send status change updates to <command>'s stdin.\n"
" --log=<filename> Log status changes and actions to <filename>.\n"
@@ -392,6 +394,15 @@ static void setpipe(const struct cmdinfo *cip, const char *value) {
statusfd_add(v);
}
+static void
+set_events_pipe(const struct cmdinfo *cip, const char *value)
+{
+ int fd;
+
+ fd = dpkg_options_parse_integer(cip, value);
+ dpkg_event_add_listener(fd);
+}
+
static bool
is_invoke_action(enum action action)
{
@@ -414,6 +425,8 @@ struct invoke_hook *post_invoke_hooks = NULL;
struct invoke_hook **post_invoke_hooks_tail = &post_invoke_hooks;
struct invoke_hook *status_loggers = NULL;
struct invoke_hook **status_loggers_tail = &status_loggers;
+struct invoke_hook *events_loggers = NULL;
+struct invoke_hook **events_loggers_tail = &events_loggers;
static void
set_invoke_hook(const struct cmdinfo *cip, const char *value)
@@ -488,6 +501,19 @@ run_status_loggers(struct invoke_hook *hook_head)
}
}
+static void
+run_events_loggers(struct invoke_hook *hook_head)
+{
+ struct invoke_hook *hook;
+
+ for (hook = hook_head; hook; hook = hook->next) {
+ int fd;
+
+ fd = run_logger(hook, _("events logger"));
+ dpkg_event_add_listener(fd);
+ }
+}
+
static int
arch_add(const char *const *argv)
{
@@ -684,6 +710,8 @@ static const struct cmdinfo cmdinfos[]= {
{ "verify-format", 0, 1, NULL, NULL, set_verify_format },
{ "status-logger", 0, 1, NULL, NULL, set_invoke_hook, 0, &status_loggers_tail },
{ "status-fd", 0, 1, NULL, NULL, setpipe, 0 },
+ { "events-fd", 0, 1, NULL, NULL, set_events_pipe, 0 },
+ { "events-logger", 0, 1, NULL, NULL, set_invoke_hook, 0, &events_loggers_tail },
{ "log", 0, 1, NULL, &log_file, NULL, 0 },
{ "pending", 'a', 0, &f_pending, NULL, NULL, 1 },
{ "recursive", 'R', 0, &f_recursive, NULL, NULL, 1 },
@@ -865,6 +893,7 @@ int main(int argc, const char *const *argv) {
if (is_invoke_action(cipaction->arg_int)) {
run_invoke_hooks(cipaction->olong, pre_invoke_hooks);
run_status_loggers(status_loggers);
+ run_events_loggers(events_loggers);
}
filesdbinit();
--
1.8.5.rc0
From 741c5de2020b4de0d5064319bb4b58bf6ac84d2e Mon Sep 17 00:00:00 2001
From: Guillem Jover <guillem@debian.org>
Date: Sat, 2 Nov 2013 22:14:01 +0100
Subject: [PATCH] t-events-fd: New test case
Based-on-patch-by: Michael Vogt <mvo@debian.org>
---
t-events-fd/.gitignore | 1 +
t-events-fd/Makefile | 14 ++++++++++++++
t-events-fd/events-ref.log | 34 ++++++++++++++++++++++++++++++++++
t-events-fd/pkg-events/DEBIAN/control | 7 +++++++
t-events-fd/pkg-events/test/file-own | 0
5 files changed, 56 insertions(+)
create mode 100644 t-events-fd/.gitignore
create mode 100644 t-events-fd/Makefile
create mode 100644 t-events-fd/events-ref.log
create mode 100644 t-events-fd/pkg-events/DEBIAN/control
create mode 100644 t-events-fd/pkg-events/test/file-own
diff --git a/t-events-fd/.gitignore b/t-events-fd/.gitignore
new file mode 100644
index 0000000..fa3d752
--- /dev/null
+++ b/t-events-fd/.gitignore
@@ -0,0 +1 @@
+events-got.log
diff --git a/t-events-fd/Makefile b/t-events-fd/Makefile
new file mode 100644
index 0000000..c833ac5
--- /dev/null
+++ b/t-events-fd/Makefile
@@ -0,0 +1,14 @@
+TESTS_DEB := pkg-events
+
+include ../Test.mk
+
+test-case:
+ # Note: we have to reuse stdin for the events file descriptor as
+ # sudo will not allow file descriptors higher than 3 to pass through.
+ $(DPKG_INSTALL) --events-fd=0 pkg-events.deb 0>events-got.log
+
+ diff -u events-got.log events-ref.log
+
+test-clean:
+ $(DPKG_PURGE) pkg-events
+ rm -f events-got.log
diff --git a/t-events-fd/events-ref.log b/t-events-fd/events-ref.log
new file mode 100644
index 0000000..e953088
--- /dev/null
+++ b/t-events-fd/events-ref.log
@@ -0,0 +1,34 @@
+Event: action
+Action: install
+Package: pkg-events:all
+Version: 0
+
+Event: state-change
+Package: pkg-events:all
+Status: half-installed
+
+Event: state-change
+Package: pkg-events:all
+Status: unpacked
+
+Event: state-change
+Package: pkg-events:all
+Status: unpacked
+
+Event: action
+Action: configure
+Package: pkg-events:all
+Version: 0
+
+Event: state-change
+Package: pkg-events:all
+Status: unpacked
+
+Event: state-change
+Package: pkg-events:all
+Status: half-configured
+
+Event: state-change
+Package: pkg-events:all
+Status: installed
+
diff --git a/t-events-fd/pkg-events/DEBIAN/control b/t-events-fd/pkg-events/DEBIAN/control
new file mode 100644
index 0000000..9050641
--- /dev/null
+++ b/t-events-fd/pkg-events/DEBIAN/control
@@ -0,0 +1,7 @@
+Package: pkg-events
+Version: 0
+Section: test
+Priority: extra
+Maintainer: Dpkg Developers <debian-dpkg@lists.debian.org>
+Architecture: all
+Description: test package - generate events
diff --git a/t-events-fd/pkg-events/test/file-own b/t-events-fd/pkg-events/test/file-own
new file mode 100644
index 0000000..e69de29
--
1.8.5.rc0
Reply to: