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 :) [..] > > > 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, propably something like --assert-status-deb822 (unless I add a hard dependency) and I'm not sure if the added test is a useful addition for the repo (it was/is for me during development). > 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. > > > (As an aside, I see the code is not keying on "status:" so if there's a > > > new record type introduced, apt might misbehave.) > > > > Correct, thanks for pointing this out. The code is currently getting > > reworked and this was one of the issues fixed. > > I might not be looking at the right place or maybe the code has not > been pushed, but I don't see a fix for this? [..] I was only in my "mvo" branch, not merged when you looked at it. But its there now. > 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. Cheers, Michael
>From e2e0b2bbe79ed92866f3198ecb32864706340a52 Mon Sep 17 00:00:00 2001 From: Michael Vogt <m...@debian.org> Date: Wed, 23 Oct 2013 10:24:26 +0200 Subject: [PATCH] initial version (rought) --- lib/dpkg/dbmodify.c | 4 ++ lib/dpkg/dpkg.h | 3 ++ lib/dpkg/libdpkg.map | 2 + lib/dpkg/log.c | 58 ++++++++++++++++++---- src/configure.c | 8 +++ src/errors.c | 3 ++ src/help.c | 4 ++ src/main.c | 14 ++++++ test/integration/debs/minimal_1.0_all.deb | Bin 0 -> 1222 bytes test/integration/test-dpkg-status-fd-deb822 | 73 ++++++++++++++++++++++++++++ 10 files changed, 159 insertions(+), 10 deletions(-) create mode 100644 test/integration/debs/minimal_1.0_all.deb create mode 100755 test/integration/test-dpkg-status-fd-deb822 diff --git a/lib/dpkg/dbmodify.c b/lib/dpkg/dbmodify.c index 61cfcb3..e015423 100644 --- a/lib/dpkg/dbmodify.c +++ b/lib/dpkg/dbmodify.c @@ -412,6 +412,10 @@ 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); + statusfd_send_deb822("Status: status\n" + "Package: %s\n" + "Action: %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..47a3fda 100644 --- a/lib/dpkg/dpkg.h +++ b/lib/dpkg/dpkg.h @@ -127,7 +127,10 @@ extern const char *log_file; void log_message(const char *fmt, ...) DPKG_ATTR_PRINTF(1); void statusfd_add(int fd); +void statusfd_add_deb822(int fd); void statusfd_send(const char *fmt, ...) DPKG_ATTR_PRINTF(1); +void statusfd_send_deb822(const char *fmt, ...) DPKG_ATTR_PRINTF(1); + /*** cleanup.c ***/ diff --git a/lib/dpkg/libdpkg.map b/lib/dpkg/libdpkg.map index 318e997..5bbc23a 100644 --- a/lib/dpkg/libdpkg.map +++ b/lib/dpkg/libdpkg.map @@ -173,7 +173,9 @@ LIBDPKG_PRIVATE { # Action logging statusfd_add; + statusfd_add_deb822; statusfd_send; + statusfd_send_deb822; # Progress report support progress_init; diff --git a/lib/dpkg/log.c b/lib/dpkg/log.c index ce9d1ea..796ea43 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 *status_pipes_deb822 = NULL; -void -statusfd_add(int fd) +static void +statusfd_add_internal(int fd, struct pipef **to_pipes) { struct pipef *pipe_new; @@ -86,15 +87,37 @@ 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 = *to_pipes; + *to_pipes = pipe_new; +} + +void +statusfd_add(int fd) +{ + statusfd_add_internal(fd, &status_pipes); +} + +void +statusfd_add_deb822(int fd) +{ + statusfd_add_internal(fd, &status_pipes_deb822); +} + +static void +statusfd_send_to_pipes(struct pipef *to_pipes, struct varbuf vb) +{ + struct pipef *pipef; + for (pipef = to_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); + } } void statusfd_send(const char *fmt, ...) { static struct varbuf vb; - struct pipef *pipef; va_list args; if (!status_pipes) @@ -109,9 +132,24 @@ 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); - } + statusfd_send_to_pipes(status_pipes, vb); +} + +void +statusfd_send_deb822(const char *fmt, ...) +{ + static struct varbuf vb; + va_list args; + + if (!status_pipes_deb822) + return; + + va_start(args, fmt); + varbuf_reset(&vb); + varbuf_vprintf(&vb, fmt, args); + // append final \n + varbuf_add_char(&vb, '\n'); + va_end(args); + + statusfd_send_to_pipes(status_pipes_deb822, vb); } diff --git a/src/configure.c b/src/configure.c index fd54582..a676606 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); + statusfd_send_deb822("Status: %s\n" + "ConfigFile: configfile-prompt\n" + "ConfigOld: %s\n" + "ConfigNew: %s\n" + "UserEdited: %i\n" + "DistEdited: %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 9dc6724..1d0d142 100644 --- a/src/errors.c +++ b/src/errors.c @@ -60,6 +60,9 @@ void print_error_perpackage(const char *emsg, const char *arg) { notice(_("error processing %s (--%s):\n %s"), arg, cipaction->olong, emsg); statusfd_send("status: %s : %s : %s", arg, "error", emsg); + statusfd_send_deb822("Status: error\n" + "Package: %s\n" + "Message: %s\n", arg, emsg); nr= malloc(sizeof(struct error_report)); if (!nr) { diff --git a/src/help.c b/src/help.c index 2af873c..2e41e14 100644 --- a/src/help.c +++ b/src/help.c @@ -403,4 +403,8 @@ 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)); + statusfd_send_deb822("Status: processing\n" + "Action: %s\n" + "Package: %s\n", action, + pkgbin_name(pkg, pkgbin, pnaw_always)); } diff --git a/src/main.c b/src/main.c index 9a1c443..59f4025 100644 --- a/src/main.c +++ b/src/main.c @@ -148,6 +148,7 @@ usage(const struct cmdinfo *ci, const char *value) " Just say what we would do - don't do it.\n" " -D|--debug=<octal> Enable debugging (see -Dhelp or --debug=help).\n" " --status-fd <n> Send status change updates to file descriptor <n>.\n" +" --status-fd-deb822 <n> Send status change updates in deb822 format 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" " --ignore-depends=<package>,...\n" @@ -403,6 +404,18 @@ static void setpipe(const struct cmdinfo *cip, const char *value) { statusfd_add(v); } +static void setpipe_deb822(const struct cmdinfo *cip, const char *value) { + long v; + char *ep; + + errno = 0; + v = strtol(value, &ep, 0); + if (value == ep || *ep || v < 0 || v > INT_MAX || errno != 0) + badusage(_("invalid integer for --%s: `%.250s'"),cip->olong,value); + + statusfd_add_deb822(v); +} + static bool is_invoke_action(enum action action) { @@ -686,6 +699,7 @@ 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 }, + { "status-fd-deb822", 0, 1, NULL, NULL, setpipe_deb822, 0 }, { "log", 0, 1, NULL, &log_file, NULL, 0 }, { "pending", 'a', 0, &f_pending, NULL, NULL, 1 }, { "recursive", 'R', 0, &f_recursive, NULL, NULL, 1 }, diff --git a/test/integration/debs/minimal_1.0_all.deb b/test/integration/debs/minimal_1.0_all.deb new file mode 100644 index 0000000000000000000000000000000000000000..9098969cc1274dc56e3ee0143249d3a5ddd1bb33 GIT binary patch literal 1222 zcmY$iNi0gvu;WTeP0CEn(@o0EODw8XP*5;5wlFd^G&MIgQ&2Df@?oT*fq|KciGl(U zK|unSk)8opa(-S(QGSkINn(*+dKF>)re>CK{qo%$3?RV7{PxP(yekF*3=eFdYqnY6 z^_6+Pp2<bED@0{Um+qap6_X@{tT)MSeR$;ZhXfl=zuQwaf*xM^wD{+e&@G+D#`iwH zDcfFIP#BPMv&y$i=ikSw4W1I-la<m6&m8`%^t^`o<}<-F=eW5y96a|Te(mqOpX;xb zU6**fR#I-q|7_>?uj|7;?EdR7B-Y1$<jQ~ca=lspiw-ACObuHsv4}rouF)@vNx3p} zZ!8O!leWLT!G*{9OPI#n!|ZPj_ZV_imzuT;e0a0?;PeXBW0?omZoJ$5!lymHOd>$; z?bYWW%vLX|?I@AG{ljia=ee~;VLw&x%(%v`_*c86H}vw^P1f-(Y=+gB6Hn;ccun|h zq1V1^etS{*=UJQN44O8Ji5WYu-oj#TZp$$vuZ-(efVjk7y-g3qo)^|gH5x1bihIeY z7g+tGck3!Su^n67%KaZKI?md1ba$zXLA%Q4-Fy;a*SRZ7_6hREYE+%gkG}9M?Xp3n zv1I%GeKo!R`hLo%K6`fY@4=t`-Cv&n^#9cK?7!g@PI1rMX1>~nVwq=BZ-=Cw3_6y2 zCW!B0%I&$Y4{yBhdHGG=@!apfzC3yKclYJm7SpaCZoS}n;jWR_W2SLV)?IzuOw+qC zzG0d8uEPgQ0%vblbBo-xMq`qI-sC-Jil!}(?AFnLx>xYbmWZog?GA0>d8y*feK7FK zjHIJodpsjmE2f5QpS@zL-Ja?#|EA`0D7FYVp%4@PA1zP+&8^6w!N9<kl30=mPjbNY zL@?EwLDI5<LIM|3s+-~Hf7?Lhc=5aC6$cu7FCHs<BxPup>$ITv^b*EfH+|(j+oZ$G zn7-V8?5E%;=rb#9d(QjvjgNjziU0ThrreT~ml7FOpVmC_+SLBomQ!5maTn*YPX_DT zD(5x$C|&p&X5GNzeS6QAcjfQnHJi75){8a%Kj&$p<<I_S_rq@`?y~#0b4u5r<KOJJ zx$ctt=c=~i-})o}m*l<bG<%u#H{FZt(|)i2$KB_jKeu0g?mylCpLy?Y{`dJ|Q~k-x zX$;E$z0=v%u5Ev?`_ZKZ-yghnt(L80n_*@Bu1o9#SFFC`zrcB?>jPgdb4fnU@sppw z`=>mY>(Bm!bMq?>v%k?T@%<sC-28rFv8vh8H_aQb@0B~iSCji{8y~l<)D^kIDf{`= z#U5_iH}lmaA-=k(KP~1}My3`U*8So5z<hkSg3%ALR~;O&vKF_~tAF1VxOdh!I5P6_ zpW83bTur@t?8X0^lmBSD{k7+6Ke+i|x0u^z_sn_6r+)gm|4Ds_x%VFCKh3}86<_?* z-}Ha?sr%QD=^Xrj`nmnqiht*eJipqrMN990@_SXgYii=}I{O!2&m4XxZgMOyCofoC Ufujk6D&F2<+Pe4z11KQ?0RD?GVE_OC literal 0 HcmV?d00001 diff --git a/test/integration/test-dpkg-status-fd-deb822 b/test/integration/test-dpkg-status-fd-deb822 new file mode 100755 index 0000000..2b99355 --- /dev/null +++ b/test/integration/test-dpkg-status-fd-deb822 @@ -0,0 +1,73 @@ +#!/bin/sh + +set -e + +BASEDIR=$(dirname $(readlink -f $0)) +TMP_ROOT=$(mktemp -d) +trap "rm -rf $TMP_ROOT" HUP INT QUIT ILL ABRT FPE SEGV PIPE TERM + +if [ -x "${BASEDIR}/../../src/dpkg" ]; then + DPKG="${BASEDIR}/../../src/dpkg" +elif [ -x "${BASEDIR}/../../build/src/dpkg" ]; then + DPKG="${BASEDIR}/../../build/src/dpkg" +else + echo "Can't find dpkg build" + exit 1 +fi + +# help dpkg to find its stuff +mkdir -p $TMP_ROOT/var/lib/dpkg/updates \ + $TMP_ROOT/var/lib/dpkg/triggers \ + $TMP_ROOT/var/log \ + $TMP_ROOT/var/lib/dpkg/info +touch $TMP_ROOT/var/lib/dpkg/status +touch $TMP_ROOT/var/lib/dpkg/available +touch $TMP_ROOT/var/log/dpkg.log + +DPKG="fakeroot $DPKG --root=$TMP_ROOT --force-not-root --force-bad-path --log=${TMP_ROOT}/var/log/dpkg.log" + +DPKG_PROGRESS_LOG="$TMP_ROOT/dpkg-progress.log" +DPKG_PROGRESS_LOG_E="$TMP_ROOT/dpkg-progress-expected.log" + +exec 3> $DPKG_PROGRESS_LOG +$DPKG --status-fd-deb822=3 -i $BASEDIR/debs/minimal_1.0_all.deb >/dev/null +cat > $DPKG_PROGRESS_LOG_E <<EOF +Status: processing +Action: install +Package: minimal:all + +Status: status +Package: minimal:all +Action: half-installed + +Status: status +Package: minimal:all +Action: unpacked + +Status: status +Package: minimal:all +Action: unpacked + +Status: processing +Action: configure +Package: minimal:all + +Status: status +Package: minimal:all +Action: unpacked + +Status: status +Package: minimal:all +Action: half-configured + +Status: status +Package: minimal:all +Action: installed + +EOF +if ! diff -u $DPKG_PROGRESS_LOG $DPKG_PROGRESS_LOG_E; then + echo "Got unexpected progress" + exit 1 +fi + +echo "PASS" -- 1.8.3.2