libflashrom: Update the API for progress callback

The initial version of API for progress callback would require the
callback function to make a second call to get the needed data about
progress state (current, total etc).

This patch changes the callback API, so that callback function gets
all needed data straight away as parameters, and with this,
callback has all the data to do its job.

Since the initial version was submitted and it was in the tree for a
while, the change needs to add a _v2 suffix for new thing and
deprecated attribute for old thing.

Testing: both unit tests and cli are libflashrom clients.
All unit tests run successfully, for the cli all scenarios from
commit 75dc0655b95dde91f1426a7e5aecfc04d7b8d631 run successfully.

Change-Id: Ia8cc0461c449b7e65888a64cdc594c55b81eae7a
Signed-off-by: Anastasia Klimchuk <aklm@flashrom.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/86031
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Reviewed-by: Peter Marheine <pmarheine@chromium.org>

cli_classic.c

@@ -1282,11 +1282,8 @@ int main(int argc, char *argv[])
 	fill_flash = &flashes[0];
 
 	struct cli_progress cli_progress = {0};
-	struct flashrom_progress progress_state = {
-		 .user_data = &cli_progress
-	};
 	if (options.show_progress)
-		flashrom_set_progress_callback(fill_flash, &flashrom_progress_cb, &progress_state);
+		flashrom_set_progress_callback_v2(fill_flash, &flashrom_progress_cb, &cli_progress);
 
 	print_chip_support_status(fill_flash->chip);
 

cli_output.c

@@ -92,14 +92,13 @@ static void print_progress(const struct cli_progress *cli_progress, enum flashro
 	msg_ginfo("[%s: %2u%%]", flashrom_progress_stage_to_string(stage), cli_progress->stage_pc[stage]);
 }
 
-void flashrom_progress_cb(struct flashrom_flashctx *flashctx)
+void flashrom_progress_cb(enum flashrom_progress_stage stage, size_t current, size_t total, void* user_data)
 {
-	struct flashrom_progress *progress_state = flashctx->progress_state;
 	unsigned int pc = 0;
-	struct cli_progress *cli_progress = progress_state->user_data;
+	struct cli_progress *cli_progress = user_data;
 
 	/* The expectation is that initial progress of zero is reported before doing anything. */
-	if (progress_state->current == 0) {
+	if (current == 0) {
 		if (!cli_progress->stage_setup) {
 			cli_progress->stage_setup = true;
 
@@ -113,17 +112,17 @@ void flashrom_progress_cb(struct flashrom_flashctx *flashctx)
 			}
 		}
 
-		cli_progress->stage_pc[progress_state->stage] = 0;
+		cli_progress->stage_pc[stage] = 0;
 	} else {
 		cli_progress->stage_setup = false;
-		cli_progress->visible_stages |= 1 << progress_state->stage;
+		cli_progress->visible_stages |= 1 << stage;
 	}
 
-	if (progress_state->current > 0 && progress_state->total > 0)
+	if (current > 0 && total > 0)
-		pc = ((unsigned long long) progress_state->current * 100llu) /
+		pc = ((unsigned long long) current * 100llu) /
-		     ((unsigned long long) progress_state->total);
+		     ((unsigned long long) total);
-	if (cli_progress->stage_pc[progress_state->stage] != pc) {
+	if (cli_progress->stage_pc[stage] != pc) {
-		cli_progress->stage_pc[progress_state->stage] = pc;
+		cli_progress->stage_pc[stage] = pc;
 
 		if (line_state == PROGRESS) {
 			/* Erase previous output, because it was previous progress step. */

doc/release_notes/devel.rst

@@ -51,3 +51,16 @@ requirements are not satisfied or the option is disabled, the authors lists will
 be replaced with placeholders unless the ``generate_authors_list`` option is set
 to ``enabled`` in which case the build will fail if the requirements are not
 satisfied.
+
+New libflashrom API for progress reporting
+------------------------------------------
+
+The old ``flashrom_set_progress_callback`` function for requesting progress updates
+during library operations is now deprecated. Users should call
+``flashrom_set_progress_callback_v2`` instead, which also changes the signature
+of the callback function. Specifically, new function type ``flashrom_progress_callback_v2``
+should be used from now on.
+
+This new API fixes limitations with the old one where most users would need to
+define their own global state to track progress, and it was impossible to fix that
+issue while maintaining binary compatibility without adding a new API.

flashrom.c

@@ -1221,7 +1221,7 @@ notfound:
 static void setup_progress_from_layout(struct flashctx *flashctx,
 				       enum flashrom_progress_stage stage)
 {
-	if (!flashctx->progress_callback)
+	if (!flashctx->progress_callback && !flashctx->deprecated_progress_callback)
 		return;
 
 	const struct flashrom_layout *const flash_layout = get_layout(flashctx);
@@ -1242,7 +1242,7 @@ static void setup_progress_from_layout_and_diff(struct flashctx *flashctx,
 						const void *want,
 						enum flashrom_progress_stage stage)
 {
-	if (!flashctx->progress_callback)
+	if (!flashctx->progress_callback && !flashctx->deprecated_progress_callback)
 		return;
 
 	const struct flashrom_layout *flash_layout = get_layout(flashctx);

include/cli_output.h

@@ -26,6 +26,6 @@ int open_logfile(const char * const filename);
 int close_logfile(void);
 void start_logging(void);
 int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap);
-void flashrom_progress_cb(struct flashrom_flashctx *flashctx);
+void flashrom_progress_cb(enum flashrom_progress_stage stage, size_t current, size_t total, void* user_data);
 
 #endif /* __CLI_OUTPUT_H__ */

include/flash.h

@@ -621,9 +621,12 @@ struct flashrom_flashctx {
 		void *data;
 	} chip_restore_fn[MAX_CHIP_RESTORE_FUNCTIONS];
 	/* Progress reporting */
-	flashrom_progress_callback *progress_callback;
+	flashrom_progress_callback_v2 *progress_callback;
-	struct flashrom_progress *progress_state;
+	struct flashrom_progress progress_state;
 	struct stage_progress stage_progress[FLASHROM_PROGRESS_NR];
+	/* deprecated, do not use */
+	flashrom_progress_callback *deprecated_progress_callback;
+	struct flashrom_progress *deprecated_progress_state;
 
 	/* Maximum allowed % of redundant erase */
 	int sacrifice_ratio;

include/libflashrom.h

@@ -75,14 +75,31 @@ enum flashrom_progress_stage {
 	FLASHROM_PROGRESS_ERASE,
 	FLASHROM_PROGRESS_NR,
 };
+
 struct flashrom_progress {
 	enum flashrom_progress_stage stage;
 	size_t current;
 	size_t total;
 	void *user_data;
 };
+
 struct flashrom_flashctx;
 typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx);
+
+/**
+ * @deprecated Use flashrom_set_progress_callback_v2 instead
+ */
+void flashrom_set_progress_callback(
+  struct flashrom_flashctx *const flashctx,
+  flashrom_progress_callback *progress_callback,
+  struct flashrom_progress *progress_state)
+__attribute__((deprecated("Use flashrom_set_progress_callback_v2 instead")));
+
+typedef void(flashrom_progress_callback_v2)(enum flashrom_progress_stage stage,
+						size_t current,
+						size_t total,
+						void *user_data);
+
 /**
  * @brief Set the progress callback function.
  *
@@ -90,12 +107,12 @@ typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx);
  * to indicate the progress has changed. This allows frontends to do whatever
  * they see fit with such values, e.g. update a progress bar in a GUI tool.
  *
- * @param progress_callback Pointer to the new progress callback function.
+ * @param flashrom_progress_callback_v2 Pointer to the new progress callback function.
- * @param progress_state Pointer to progress state to include with the progress
+ * @param user_data A pointer to a piece of data which is managed by progress caller
- * callback.
  */
-void flashrom_set_progress_callback(struct flashrom_flashctx *const flashctx,
+void flashrom_set_progress_callback_v2(struct flashrom_flashctx *const flashctx,
-		flashrom_progress_callback *progress_callback, struct flashrom_progress *progress_state);
+					flashrom_progress_callback_v2 *progress_callback,
+					void* user_data);
 
 /** @} */ /* end flashrom-general */
 

libflashrom.c

@@ -64,15 +64,42 @@ int print(const enum flashrom_log_level level, const char *const fmt, ...)
 	return 0;
 }
 
-void flashrom_set_progress_callback(struct flashrom_flashctx *flashctx, flashrom_progress_callback *progress_callback, struct flashrom_progress *progress_state)
+void flashrom_set_progress_callback(struct flashrom_flashctx *flashctx,
+					flashrom_progress_callback *progress_callback,
+					struct flashrom_progress *progress_state)
 {
+	msg_gwarn("%s: this function is deprecated (for developers: call %s_v2 instead)\n",
+		 __func__, __func__);
+
+	if (flashctx->progress_callback) {
+		msg_gwarn("Progress callback already set by %s_v2, "
+			  "ignoring this call since only one progress callback can be registered.\n",
+			  __func__);
+		return;
+	}
+
+	flashctx->deprecated_progress_callback = progress_callback;
+	flashctx->deprecated_progress_state = progress_state;
+	flashctx->progress_state = *progress_state;
+}
+
+void flashrom_set_progress_callback_v2(struct flashrom_flashctx *flashctx,
+					flashrom_progress_callback_v2 *progress_callback,
+					void *user_data)
+{
+	if (flashctx->deprecated_progress_callback) {
+		msg_gwarn("Deprecated progress callback already set by flashrom_set_progress_callback, "
+			  "ignoring this call since only one progress callback can be registered.\n");
+		return;
+	}
+
 	flashctx->progress_callback = progress_callback;
-	flashctx->progress_state = progress_state;
+	flashctx->progress_state.user_data = user_data;
 }
 /** @private */
 void init_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t total)
 {
-	if (flashctx->progress_callback == NULL)
+	if (!flashctx->progress_callback && !flashctx->deprecated_progress_callback)
 		return;
 
 	struct stage_progress *stage_progress = &flashctx->stage_progress[stage];
@@ -85,7 +112,7 @@ void init_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_st
 /** @private */
 void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t increment)
 {
-	if (flashctx->progress_callback == NULL)
+	if (!flashctx->progress_callback && !flashctx->deprecated_progress_callback)
 		return;
 
 	struct stage_progress *stage_progress = &flashctx->stage_progress[stage];
@@ -96,11 +123,20 @@ void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_
 		stage_progress->total = stage_progress->current;
 	}
 
-	flashctx->progress_state->stage = stage;
+	flashctx->progress_state.stage = stage;
-	flashctx->progress_state->current = stage_progress->current;
+	flashctx->progress_state.current = stage_progress->current;
-	flashctx->progress_state->total = stage_progress->total;
+	flashctx->progress_state.total = stage_progress->total;
 
-	flashctx->progress_callback(flashctx);
+	if (flashctx->progress_callback) {
+		flashctx->progress_callback(stage, stage_progress->current, stage_progress->total,
+						flashctx->progress_state.user_data);
+	} else if (flashctx->deprecated_progress_callback) {
+		*(flashctx->deprecated_progress_state) = flashctx->progress_state;
+		flashctx->deprecated_progress_callback(flashctx);
+		memcpy(&flashctx->progress_state.user_data,
+			flashctx->deprecated_progress_state->user_data,
+			sizeof(flashctx->progress_state.user_data));
+	}
 }
 
 const char *flashrom_version_info(void)

libflashrom.map

@@ -25,6 +25,7 @@ LIBFLASHROM_1.0 {
     flashrom_programmer_shutdown;
     flashrom_set_log_callback;
     flashrom_set_progress_callback;
+    flashrom_set_progress_callback_v2;
     flashrom_shutdown;
     flashrom_supported_boards;
     flashrom_supported_chipsets;

tests/chip.c

@@ -81,20 +81,20 @@ static int block_erase_chip(struct flashctx *flash, unsigned int blockaddr, unsi
 	return 0;
 }
 
-static void progress_callback(struct flashctx *flash) {
+static void progress_callback(enum flashrom_progress_stage stage, size_t current, size_t total, void* user_data) {
-	struct progress_user_data *progress_user_data = flash->progress_state->user_data;
+	struct progress_user_data *progress_user_data = user_data;
-	if (flash->progress_state->current == 0) {
+
-		printf("Progress started for stage %d, initial callback call\n", flash->progress_state->stage);
+	if (current == 0) {
+		printf("Progress started for stage %d, initial callback call\n", stage);
 	} else {
 		/* Progress cannot go backwards. */
-		assert_true(flash->progress_state->current >= progress_user_data->last_seen[flash->progress_state->stage]);
+		assert_true(current >= progress_user_data->last_seen[stage]);
 	}
 
-	if (flash->progress_state->current >= flash->progress_state->total - 1)
+	if (current >= total - 1)
-		printf("Progress almost complete for stage %d, current %ld, total %ld\n",
+		printf("Progress almost complete for stage %d, current %ld, total %ld\n", stage, current, total);
-			flash->progress_state->stage, flash->progress_state->current, flash->progress_state->total);
 
-	progress_user_data->last_seen[flash->progress_state->stage] = flash->progress_state->current;
+	progress_user_data->last_seen[stage] = current;
 }
 
 static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout,
@@ -255,10 +255,7 @@ void erase_chip_with_progress(void **state)
 	setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io);
 
 	struct progress_user_data progress_user_data = {0};
-	struct flashrom_progress progress_state = {
+	flashrom_set_progress_callback_v2(&flashctx, progress_callback, &progress_user_data);
-		.user_data = &progress_user_data
-	};
-	flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state);
 
 	printf("Erase chip operation started.\n");
 	assert_int_equal(0, flashrom_flash_erase(&flashctx));
@@ -357,10 +354,7 @@ void read_chip_with_progress(void **state)
 	setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io);
 
 	struct progress_user_data progress_user_data = {0};
-	struct flashrom_progress progress_state = {
+	flashrom_set_progress_callback_v2(&flashctx, progress_callback, &progress_user_data);
-		.user_data = &progress_user_data
-	};
-	flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state);
 
 	const char *const filename = "read_chip.test";
 	unsigned long size = mock_chip.total_size * 1024;
@@ -488,10 +482,7 @@ void write_chip_with_progress(void **state)
 	setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io);
 
 	struct progress_user_data progress_user_data = {0};
-	struct flashrom_progress progress_state = {
+	flashrom_set_progress_callback_v2(&flashctx, progress_callback, &progress_user_data);
-		.user_data = &progress_user_data
-	};
-	flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state);
 
 	const char *const filename = "-";
 	unsigned long size = mock_chip.total_size * 1024;
@@ -621,10 +612,7 @@ void write_chip_feature_no_erase_with_progress(void **state)
 	assert_non_null(newcontents);
 
 	struct progress_user_data progress_user_data = {0};
-	struct flashrom_progress progress_state = {
+	flashrom_set_progress_callback_v2(&flashctx, progress_callback, &progress_user_data);
-		.user_data = &progress_user_data
-	};
-	flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state);
 
 	printf("Write chip operation started.\n");
 	assert_int_equal(0, read_buf_from_file(newcontents, size, filename));

tests/spi25.c

@@ -69,26 +69,25 @@ int __wrap_spi_send_command(const struct flashctx *flash,
 	return 0;
 }
 
-static void spi_read_progress_cb(struct flashrom_flashctx *flashctx)
+static void spi_read_progress_cb(enum flashrom_progress_stage stage, size_t current, size_t total, void* user_data)
 {
-	struct flashrom_progress *progress_state = flashctx->progress_state;
+	uint32_t *cnt = (uint32_t *) user_data;
-	uint32_t *cnt = (uint32_t *) progress_state->user_data;
+	assert_int_equal(0x400, total);
-	assert_int_equal(0x400, progress_state->total);
 	switch (*cnt) {
 		case 0:
-			assert_int_equal(0x0, progress_state->current);
+			assert_int_equal(0x0, current);
 			break;
 		case 1:
-			assert_int_equal(0x100, progress_state->current);
+			assert_int_equal(0x100, current);
 			break;
 		case 2:
-			assert_int_equal(0x200, progress_state->current);
+			assert_int_equal(0x200, current);
 			break;
 		case 3:
-			assert_int_equal(0x300, progress_state->current);
+			assert_int_equal(0x300, current);
 			break;
 		case 4:
-			assert_int_equal(0x400, progress_state->current);
+			assert_int_equal(0x400, current);
 			break;
 		default:
 			fail();
@@ -113,10 +112,7 @@ void default_spi_read_test_success(void **state)
 		.chip = &mock_chip,
 		.mst = &mst
 	};
-	struct flashrom_progress progress_state = {
+	flashrom_set_progress_callback_v2(&flashctx, spi_read_progress_cb, (void *) &cnt);
-		.user_data = (void *) &cnt,
-	};
-	flashrom_set_progress_callback(&flashctx, spi_read_progress_cb, &progress_state);
 	init_progress(&flashctx, FLASHROM_PROGRESS_READ, 0x400);
 	for (int i = 0; i < 4; i++) {
 		will_return(__wrap_spi_send_command, JEDEC_WRDI);
@@ -126,7 +122,7 @@ void default_spi_read_test_success(void **state)
 	assert_int_equal(0, default_spi_read(&flashctx, buf, offset, sizeof(buf)));
 	assert_int_equal(5, cnt);
 
-	flashrom_set_progress_callback(&flashctx, NULL, NULL);
+	flashrom_set_progress_callback_v2(&flashctx, NULL, NULL);
 }
 
 void spi_write_enable_test_success(void **state)