[M] Change in flashrom[main]: libflashrom: Update the API for Logger Callback

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Dmitry Zhadinets, Peter Marheine.

Anastasia Klimchuk has posted comments on this change by Dmitry Zhadinets. ( https://review.coreboot.org/c/flashrom/+/86875?usp=email )

Change subject: libflashrom: Update the API for Logger Callback
......................................................................

Patch Set 4:

(13 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/86875/comment/8dfa9491_dd7166d1?usp... :
PS2, Line 7: libflashrom: Update the API for Logger Callback
> I agree that the new API should appear fully-formed, not with two different ABIs (that is, there sho […]

Alright, it seems you both agree the patch can do more than one thing, then the title is fine. I will close this comment.

File include/libflashrom.h:

https://review.coreboot.org/c/flashrom/+/86875/comment/b98e62cb_e7f3c5a8?usp... :
PS4, Line 83:  *
This documentation needs to explain about formatting

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/86875/comment/55caf66e_410ecafd?usp... :
PS2, Line 59: 	/* Dynamic messages and double formatting may not be ideal,
            : 	as they are slow. It would be better to limit the message
            : 	size to a fixed value (e.g., char message[1024] = {0};). */
            : 	char *message;
            : 	va_list args_attempt;
            : 	va_copy(args_attempt, args);
            : 	int size = vsnprintf(NULL, 0, format, args_attempt);
            : 	va_end(args_attempt);
            : 	if (size < 0)
            : 		return size;
            : 	message = malloc(size+1);
            : 	if (!message)
            : 		return -ENOMEM;
            : 	vsnprintf(message, size + 1, format, args);
            : 	global_log_callback_v2(level, message, global_log_user_data);
            : 	free(message);
            : 	return 0;
> I think it's better to do the formatting inside libflashrom, and just pass strings. […]

Alright, I can agree on that. Also, v1 (initial version) works as before, no formatting (in case someone wants v1), is this correct?

Then my comment to this code block is to add few new lines to separate logic pieces of code (63-64, 68-69, 71-72, 73-74)

https://review.coreboot.org/c/flashrom/+/86875/comment/02d83038_bd6a775f?usp... :
PS2, Line 79: 
            : 	global_log_callback = flashrom_log_callback_v2_handler;
> I agree that the current approach (having only one callback stored, and expressing the old API in te […]

Alright, I can understand. Also you both agree! I close this comment, but I have few comments to code style

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/86875/comment/f56b065b_7b5a5b82?usp... :
PS4, Line 56: flashrom_log_callback_v2_handler
This is a static function, so it doesn't have to start with "flashrom" , also I would try to give it a name which explains what it is doing ("handler" can be anything)

I would go with something painfully obvious like `format_message_and_invoke_log_callback_v2` but tell me if you have other ideas

https://review.coreboot.org/c/flashrom/+/86875/comment/0afe4687_059c60e3?usp... :
PS4, Line 57: 										
You probably don't need all these tabs, also I think this fits on one line? fn name and all params

https://review.coreboot.org/c/flashrom/+/86875/comment/88960847_2a3e3286?usp... :
PS4, Line 59: 	/* Dynamic messages and double formatting may not be ideal,
            : 	as they are slow. It would be better to limit the message
            : 	size to a fixed value (e.g., char message[1024] = {0};). */
You need to align with comments style (https://flashrom.org/dev_guide/development_guide.html#coding-style) but tldr, add asterisk on each line

File tests/libflashrom.c:

https://review.coreboot.org/c/flashrom/+/86875/comment/f3b5f3bf_126fe309?usp... :
PS4, Line 3: 
           :  * Copyright 2020 Google LLC
I commented on the other patch too, there should be your name here!

https://review.coreboot.org/c/flashrom/+/86875/comment/e5cae834_6e112cb2?usp... :
PS4, Line 30: last_callback_version = 1;
We usually avoid global state (if possible), and in this case I think you can avoid it by returning a value? (something special like 0x666 or anything non-zero should work). Then print should return it back and then you can assert it.
As a good side-effect, this will also test a propagation of return values.

https://review.coreboot.org/c/flashrom/+/86875/comment/f20912f8_2ee79770?usp... :
PS4, Line 48: logapi_test_success
Thank you so much for adding tests!

Usually, tests should be as granular as possible, and if possible, one test tests one thing.

Concretely here, you need to split your test method into two,
`flashrom_set_log_callback_test_success`
and
`flashrom_set_log_callback_v2_test_success`

the names (ideally) represent the function being tested, so the easiest is to use the fn name in the test name.

https://review.coreboot.org/c/flashrom/+/86875/comment/4c1d8f69_07f9cf2c?usp... :
PS4, Line 59: and 
unfinished sentence?

https://review.coreboot.org/c/flashrom/+/86875/comment/a1181c9d_3181204e?usp... :
PS4, Line 60: assert_int_equal(last_callback_version, 2);
You don't need the global state for v2 , you upgraded it, so it doesn't need global state!
You can modify user_data inside the callback function, and then assert that it is expected value afterwards - that's what the real life usage will be

https://review.coreboot.org/c/flashrom/+/86875/comment/6b58ced9_8673bca2?usp... :
PS4, Line 62: 	flashrom_set_log_callback(test_log_callback);
            : 	print(FLASHROM_MSG_INFO, "%s", "\n");
            : 	/* check that v1 calback set back */
            : 	assert_int_equal(last_callback_version, 1);
why do you reset the callback again (3rd time)? I don't think API clients would be setting callbacks multiple times, and especially different versions?
I would remove that last part, and split first two parts into two separate tests functions (in the same file, in this file)