From 6e1992166f9a5ef282354bed1109f4ddb3f8267a Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 30 Aug 2024 21:25:37 -0400 Subject: [PATCH 1/5] progress-bar: Put a space after the prefix We always want a space there. So let's just put one in the drawing routine, and adjust the call cites to avoid adding a second one. --- src/import/importctl.c | 2 +- src/partition/repart.c | 2 +- src/shared/pretty-print.c | 10 ++++++---- src/test/test-progress-bar.c | 2 +- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/import/importctl.c b/src/import/importctl.c index 27c26a70e88..85903bb8c69 100644 --- a/src/import/importctl.c +++ b/src/import/importctl.c @@ -45,7 +45,7 @@ static const char* arg_format = NULL; static sd_json_format_flags_t arg_json_format_flags = SD_JSON_FORMAT_OFF; static ImageClass arg_image_class = _IMAGE_CLASS_INVALID; -#define PROGRESS_PREFIX "Total: " +#define PROGRESS_PREFIX "Total:" static int settle_image_class(void) { diff --git a/src/partition/repart.c b/src/partition/repart.c index e14c9a4750b..48b3b430137 100644 --- a/src/partition/repart.c +++ b/src/partition/repart.c @@ -4553,7 +4553,7 @@ static int progress_bytes(uint64_t n_bytes, void *userdata) { p->copy_blocks_done += n_bytes; - if (asprintf(&s, "%s %s %s %s/%s ", + if (asprintf(&s, "%s %s %s %s/%s", strna(p->copy_blocks_path), special_glyph(SPECIAL_GLYPH_ARROW_RIGHT), strna(p->definition_path), diff --git a/src/shared/pretty-print.c b/src/shared/pretty-print.c index f361c4760a0..b2a1393fcea 100644 --- a/src/shared/pretty-print.c +++ b/src/shared/pretty-print.c @@ -469,12 +469,14 @@ void draw_progress_bar(const char *prefix, double percentage) { setvbuf(stderr, buffer, _IOFBF, sizeof(buffer)); fputc('\r', stderr); - if (prefix) + if (prefix) { fputs(prefix, stderr); + fputc(' ', stderr); + } if (!terminal_is_dumb()) { size_t cols = columns(); - size_t prefix_width = utf8_console_width(prefix); + size_t prefix_width = utf8_console_width(prefix) + 1 /* space */; size_t length = cols > prefix_width + 6 ? cols - prefix_width - 6 : 0; if (length > 5 && percentage >= 0.0 && percentage <= 100.0) { @@ -533,8 +535,8 @@ void clear_progress_bar(const char *prefix) { if (terminal_is_dumb()) fputs(strrepa(" ", - prefix ? utf8_console_width(prefix) + 4 : - LESS_BY(columns(), 1U)), /* 4: %3.0f%% */ + prefix ? utf8_console_width(prefix) + 5 : /* %3.0f%% (4 chars) + space */ + LESS_BY(columns(), 1U)), stderr); else fputs(ANSI_ERASE_TO_END_OF_LINE, stderr); diff --git a/src/test/test-progress-bar.c b/src/test/test-progress-bar.c index b47adf0c289..abc1d292dcb 100644 --- a/src/test/test-progress-bar.c +++ b/src/test/test-progress-bar.c @@ -4,7 +4,7 @@ #include "random-util.h" #include "tests.h" -#define PROGRESS_PREFIX "test: " +#define PROGRESS_PREFIX "test:" TEST(progress_bar) { From aa2e664e1861e8874c8ead61419d1ea057b92ceb Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 30 Aug 2024 21:43:44 -0400 Subject: [PATCH 2/5] sysupdated: Register known error types This fixes a bug introduced during review of sysupdated. Originally, we just returned EALREADY verbatim to signify that the target is already up-to-date. Then we switched this to a proper error (org.freedesktop.sysupdate1.NoCandidate) during review. But that now maps to EIO, not EALREADY. Thus, whenever there's nothing to update, updatectl would report I/O errors to the user, even though nothing actually went wrong. --- src/libsystemd/sd-bus/bus-common-errors.c | 2 ++ src/libsystemd/sd-bus/bus-common-errors.h | 2 ++ src/sysupdate/sysupdated.c | 5 +++-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-common-errors.c b/src/libsystemd/sd-bus/bus-common-errors.c index 5b18241f00e..db1285e7f0f 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.c +++ b/src/libsystemd/sd-bus/bus-common-errors.c @@ -153,5 +153,7 @@ BUS_ERROR_MAP_ELF_REGISTER const sd_bus_error_map bus_common_errors[] = { SD_BUS_ERROR_MAP(BUS_ERROR_REBALANCE_NOT_NEEDED, EALREADY), SD_BUS_ERROR_MAP(BUS_ERROR_HOME_NOT_REFERENCED, EBADR), + SD_BUS_ERROR_MAP(BUS_ERROR_NO_UPDATE_CANDIDATE, EALREADY), + SD_BUS_ERROR_MAP_END }; diff --git a/src/libsystemd/sd-bus/bus-common-errors.h b/src/libsystemd/sd-bus/bus-common-errors.h index fb1d4211681..622cf4933db 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.h +++ b/src/libsystemd/sd-bus/bus-common-errors.h @@ -158,4 +158,6 @@ #define BUS_ERROR_REBALANCE_NOT_NEEDED "org.freedesktop.home1.RebalanceNotNeeded" #define BUS_ERROR_HOME_NOT_REFERENCED "org.freedesktop.home1.HomeNotReferenced" +#define BUS_ERROR_NO_UPDATE_CANDIDATE "org.freedesktop.sysupdate1.NoCandidate" + BUS_ERROR_MAP_ELF_USE(bus_common_errors); diff --git a/src/sysupdate/sysupdated.c b/src/sysupdate/sysupdated.c index 4edfea937e1..9a078aa84a2 100644 --- a/src/sysupdate/sysupdated.c +++ b/src/sysupdate/sysupdated.c @@ -4,6 +4,7 @@ #include "sd-json.h" #include "build-path.h" +#include "bus-common-errors.h" #include "bus-error.h" #include "bus-get-properties.h" #include "bus-label.h" @@ -1044,8 +1045,8 @@ static int target_method_update_finished_early( /* Called when job finishes w/ a successful exit code, but before any work begins. * This happens when there is no candidate (i.e. we're already up-to-date), or * specified update is already installed. */ - return sd_bus_error_setf(error, "org.freedesktop.sysupdate1.NoCandidate", - "Job exited successfully with no work to do, assume already updated"); + return sd_bus_error_setf(error, BUS_ERROR_NO_UPDATE_CANDIDATE, + "Job exited successfully with no work to do, assume already updated"); } static int target_method_update_detach(sd_bus_message *msg, const Job *j) { From 2aff6efe677b1c1ff42677d9c8a7679373fb9712 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 30 Aug 2024 21:53:14 -0400 Subject: [PATCH 3/5] updatectl: Ensure we clear the progress bar Otherwise we end up half-overwriting the progress bar, which looks buggy --- src/sysupdate/updatectl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sysupdate/updatectl.c b/src/sysupdate/updatectl.c index d5f74bff3ef..8f1e4e72db8 100644 --- a/src/sysupdate/updatectl.c +++ b/src/sysupdate/updatectl.c @@ -808,12 +808,15 @@ static int update_render_progress(sd_event_source *source, void *userdata) { int progress = PTR_TO_INT(p); if (progress == UPDATE_PROGRESS_FAILED) { + clear_progress_bar(target); fprintf(stderr, "%s %s\n", RED_CROSS_MARK(), target); total += 100; } else if (progress == -EALREADY) { + clear_progress_bar(target); fprintf(stderr, "%s %s (Already up-to-date)\n", GREEN_CHECK_MARK(), target); n--; /* Don't consider this target in the total */ } else if (progress < 0) { + clear_progress_bar(target); fprintf(stderr, "%s %s (%s)\n", RED_CROSS_MARK(), target, STRERROR(progress)); total += 100; } else { From ca7490c5ad180614926d1d126333ec6632b31180 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 30 Aug 2024 21:57:07 -0400 Subject: [PATCH 4/5] updatectl: Improve behavior of progress logging This applies a couple of aesthetic changes to the way updatectl renders progress information 1. We invert from "ICON TARGET MESSAGE" to "TARGET: ICON MESSAGE" to better fit in with the systemd progress bars, which look like "TARGET [==========---------] XX%". The original version of the sysupdated PR implemented its own progress bars that were oriented differently: "[==========---------] TARGET XX%". When we swapped the progress bar we didn't swap the status messages 2. When a target finishes updating, instead of leaving a 100% progress bar on screen for potentially extended periods of time (which implies to the user that the update isn't actually done...), we show a status message saying the target is done updating. 3. Fixed a minor bug where an extra newline would be printed after the total progress bar. At the top of the rendering function, we scroll the terminal's scroll-back just enough to fit a line for each target, and one for the total. This means that we should not print an additional line after the total, or else it'll scroll the terminal's buffer by an additional character. This bug was introduced at some point during review 4. Clears the Total progress bar before quitting. By the time we're quitting, that progress bar will be showing no useful status for the user. Also, the fix in point 3 will cause the shell's prompt to appear on the same line as the Total progress bar, partially overwriting it and leaving the shell in a glitchy state. --- src/sysupdate/updatectl.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/sysupdate/updatectl.c b/src/sysupdate/updatectl.c index 8f1e4e72db8..400e521c1ba 100644 --- a/src/sysupdate/updatectl.c +++ b/src/sysupdate/updatectl.c @@ -775,6 +775,7 @@ static int verb_check(int argc, char **argv, void *userdata) { } #define UPDATE_PROGRESS_FAILED INT_MIN +#define UPDATE_PROGRESS_DONE INT_MAX /* Make sure it doesn't overlap w/ errno values */ assert_cc(UPDATE_PROGRESS_FAILED < -ERRNO_MAX); @@ -809,15 +810,19 @@ static int update_render_progress(sd_event_source *source, void *userdata) { if (progress == UPDATE_PROGRESS_FAILED) { clear_progress_bar(target); - fprintf(stderr, "%s %s\n", RED_CROSS_MARK(), target); + fprintf(stderr, "%s: %s Unknown failure\n", target, RED_CROSS_MARK()); total += 100; } else if (progress == -EALREADY) { clear_progress_bar(target); - fprintf(stderr, "%s %s (Already up-to-date)\n", GREEN_CHECK_MARK(), target); + fprintf(stderr, "%s: %s Already up-to-date\n", target, GREEN_CHECK_MARK()); n--; /* Don't consider this target in the total */ } else if (progress < 0) { clear_progress_bar(target); - fprintf(stderr, "%s %s (%s)\n", RED_CROSS_MARK(), target, STRERROR(progress)); + fprintf(stderr, "%s: %s %s\n", target, RED_CROSS_MARK(), STRERROR(progress)); + total += 100; + } else if (progress == UPDATE_PROGRESS_DONE) { + clear_progress_bar(target); + fprintf(stderr, "%s: %s Done\n", target, GREEN_CHECK_MARK()); total += 100; } else { draw_progress_bar(target, progress); @@ -827,8 +832,13 @@ static int update_render_progress(sd_event_source *source, void *userdata) { } if (n > 1) { - draw_progress_bar("TOTAL", (double) total / n); - fputs("\n", stderr); + if (exiting) + clear_progress_bar(target); + else { + draw_progress_bar("Total", (double) total / n); + if (terminal_is_dumb()) + fputs("\n", stderr); + } } if (!terminal_is_dumb()) { @@ -898,7 +908,7 @@ static int update_finished(sd_bus_message *m, void *userdata, sd_bus_error *erro } if (status == 0) /* success */ - status = 100; + status = UPDATE_PROGRESS_DONE; else if (status > 0) /* exit status without errno */ status = UPDATE_PROGRESS_FAILED; /* i.e. EXIT_FAILURE */ /* else errno */ From 5f9dd9c64d20e7cdf8b509421e28cfebf31b7c32 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 30 Aug 2024 22:39:17 -0400 Subject: [PATCH 5/5] progress-bar: Add unbuffered variant The progress_bar functions do their own buffering: they reconfigure stderr, then print, then flush and disable buffering on their own. In situations where multiple progress bars are being drawn at a time (for example, in updatectl), it's even more efficient to hoist the buffering and flushing to the call site, and avoid drawing each progress bar individually. To that end, new _unbuffered variants of the progress_bar functions. And we use them in updatectl. --- src/shared/pretty-print.c | 41 ++++++++++++++++++++++++--------------- src/shared/pretty-print.h | 2 ++ src/sysupdate/updatectl.c | 21 +++++++++++++------- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/shared/pretty-print.c b/src/shared/pretty-print.c index b2a1393fcea..cc226e72ea7 100644 --- a/src/shared/pretty-print.c +++ b/src/shared/pretty-print.c @@ -460,14 +460,7 @@ bool shall_tint_background(void) { return cache != 0; } -void draw_progress_bar(const char *prefix, double percentage) { - - /* We are going output a bunch of small strings that shall appear as a single line to STDERR which is - * unbuffered by default. Let's temporarily turn on full buffering, so that this is passed to the tty - * as a single buffer, to make things more efficient. */ - char buffer[LONG_LINE_MAX]; - setvbuf(stderr, buffer, _IOFBF, sizeof(buffer)); - +void draw_progress_bar_unbuffered(const char *prefix, double percentage) { fputc('\r', stderr); if (prefix) { fputs(prefix, stderr); @@ -520,17 +513,10 @@ void draw_progress_bar(const char *prefix, double percentage) { fputs(ANSI_ERASE_TO_END_OF_LINE, stderr); fputc('\r', stderr); - fflush(stderr); - /* Disable buffering again */ - setvbuf(stderr, NULL, _IONBF, 0); } -void clear_progress_bar(const char *prefix) { - - char buffer[LONG_LINE_MAX]; - setvbuf(stderr, buffer, _IOFBF, sizeof(buffer)); - +void clear_progress_bar_unbuffered(const char *prefix) { fputc('\r', stderr); if (terminal_is_dumb()) @@ -542,8 +528,31 @@ void clear_progress_bar(const char *prefix) { fputs(ANSI_ERASE_TO_END_OF_LINE, stderr); fputc('\r', stderr); +} + +void draw_progress_bar(const char *prefix, double percentage) { + + /* We are going output a bunch of small strings that shall appear as a single line to STDERR which is + * unbuffered by default. Let's temporarily turn on full buffering, so that this is passed to the tty + * as a single buffer, to make things more efficient. */ + char buffer[LONG_LINE_MAX]; + setvbuf(stderr, buffer, _IOFBF, sizeof(buffer)); + + draw_progress_bar_unbuffered(prefix, percentage); + fflush(stderr); /* Disable buffering again */ setvbuf(stderr, NULL, _IONBF, 0); } + +void clear_progress_bar(const char *prefix) { + char buffer[LONG_LINE_MAX]; + setvbuf(stderr, buffer, _IOFBF, sizeof(buffer)); + + clear_progress_bar_unbuffered(prefix); + + fflush(stderr); + + setvbuf(stderr, NULL, _IONBF, 0); +} diff --git a/src/shared/pretty-print.h b/src/shared/pretty-print.h index 2c97c354ef0..d3af149a2ef 100644 --- a/src/shared/pretty-print.h +++ b/src/shared/pretty-print.h @@ -55,3 +55,5 @@ bool shall_tint_background(void); void draw_progress_bar(const char *prefix, double percentage); void clear_progress_bar(const char *prefix); +void draw_progress_bar_unbuffered(const char *prefix, double percentage); +void clear_progress_bar_unbuffered(const char *prefix); diff --git a/src/sysupdate/updatectl.c b/src/sysupdate/updatectl.c index 400e521c1ba..c298b244641 100644 --- a/src/sysupdate/updatectl.c +++ b/src/sysupdate/updatectl.c @@ -13,6 +13,7 @@ #include "bus-map-properties.h" #include "bus-util.h" #include "errno-list.h" +#include "fileio.h" #include "format-table.h" #include "json-util.h" #include "main-func.h" @@ -795,6 +796,11 @@ static int update_render_progress(sd_event_source *source, void *userdata) { if (n == 0) return 0; + /* We're outputting lots of small strings to STDERR, which is unbuffered by default. So let's turn + * on full buffering, so we pass this all to the TTY in one go, to make things more efficient */ + char buffer[LONG_LINE_MAX]; + setvbuf(stderr, buffer, _IOFBF, sizeof(buffer)); + if (!terminal_is_dumb()) { for (size_t i = 0; i <= n; i++) fputs("\n", stderr); /* Possibly scroll the terminal to make room (including total)*/ @@ -809,23 +815,23 @@ static int update_render_progress(sd_event_source *source, void *userdata) { int progress = PTR_TO_INT(p); if (progress == UPDATE_PROGRESS_FAILED) { - clear_progress_bar(target); + clear_progress_bar_unbuffered(target); fprintf(stderr, "%s: %s Unknown failure\n", target, RED_CROSS_MARK()); total += 100; } else if (progress == -EALREADY) { - clear_progress_bar(target); + clear_progress_bar_unbuffered(target); fprintf(stderr, "%s: %s Already up-to-date\n", target, GREEN_CHECK_MARK()); n--; /* Don't consider this target in the total */ } else if (progress < 0) { - clear_progress_bar(target); + clear_progress_bar_unbuffered(target); fprintf(stderr, "%s: %s %s\n", target, RED_CROSS_MARK(), STRERROR(progress)); total += 100; } else if (progress == UPDATE_PROGRESS_DONE) { - clear_progress_bar(target); + clear_progress_bar_unbuffered(target); fprintf(stderr, "%s: %s Done\n", target, GREEN_CHECK_MARK()); total += 100; } else { - draw_progress_bar(target, progress); + draw_progress_bar_unbuffered(target, progress); fputs("\n", stderr); total += progress; } @@ -833,9 +839,9 @@ static int update_render_progress(sd_event_source *source, void *userdata) { if (n > 1) { if (exiting) - clear_progress_bar(target); + clear_progress_bar_unbuffered(target); else { - draw_progress_bar("Total", (double) total / n); + draw_progress_bar_unbuffered("Total", (double) total / n); if (terminal_is_dumb()) fputs("\n", stderr); } @@ -850,6 +856,7 @@ static int update_render_progress(sd_event_source *source, void *userdata) { fputs("------\n", stderr); fflush(stderr); + setvbuf(stderr, NULL, _IONBF, 0); /* Disable buffering again */ return 0; }