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

[Dmitry Zhadinets (Code Review)]

Attention is currently required from: Anastasia Klimchuk, Peter Marheine.

Dmitry Zhadinets 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 5:

(11 comments)

File include/libflashrom.h:

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

Hmmmm. The user does not affect this, but as you wish.

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/86875/comment/9e3097ef_424d2a04?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;
> Alright, I can agree on that. […]

I hope I understood markers correctly

File libflashrom.c:

https://review.coreboot.org/c/flashrom/+/86875/comment/3048a4ba_74ce1643?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 […]

Done

https://review.coreboot.org/c/flashrom/+/86875/comment/e60ddde1_ee8e6916?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

Done

https://review.coreboot.org/c/flashrom/+/86875/comment/6be5d942_a8bfae88?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. […]

This is a question to a reviewer. I propose removing double allocation and maintaining messages at a constant length. How long can a single message generated by Flashrom be? A length of 1K might be excessive.

File tests/libflashrom.c:

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

Done

https://review.coreboot.org/c/flashrom/+/86875/comment/e9c39f97_5714c31c?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  […]

Done

https://review.coreboot.org/c/flashrom/+/86875/comment/6627a992_91e7cd05?usp... :
PS4, Line 48: logapi_test_success
> Thank you so much for adding tests! […]

Done

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

fixed

https://review.coreboot.org/c/flashrom/+/86875/comment/fab052dd_344858d3?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! […]

I re-implemented tests

https://review.coreboot.org/c/flashrom/+/86875/comment/eb8f6956_063b5372?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 […]

Done