Attention is currently required from: Dmitry Zhadinets.

View Change

4 comments:

• Patchset:

  • Patch Set #6:

    I have two small readability comments, otherwise the only question to decide is message size right?

• File include/libflashrom.h:

  • Patch Set #4, Line 83: *

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

    I was trying to find it, but I couldn't, but then I found the addition at `flashrom_set_progress_callback_v2` :) I was thinking to add it at `flashrom_set_log_callback_v2` (the new fn that you are adding)

• File libflashrom.c:

  • Patch Set #4, 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};). */
    

    I would go shorter to perhaps 128 or 256 bytes, but have a fallback to dynamic allocation in case a […]

    I found one very long message in cli_classic.c, in `cli_classic_usage` (it prints help, as of `flashrom -h`), but I don't think it trigger a callback, since it's using printf directly?

    This was the only very long message that I found. The rest probably fits into 256, but maybe let's do it 512 just in case?

    Also 100% agree let's not drop long messages silently.
    Actually if we don't drop than 256 is fine

• File libflashrom.c:

  • Patch Set #6, Line 84:

    	if (!log_callback && global_log_callback == format_message_and_invoke_log_callback_v2)
    		global_log_callback = NULL;
    

    I spent some time looking at this, then I figured this is doing a reset (when log_callback is passed NULL).
    It is possible to understand, but I have a suggestion to change this a bit, to make it more readable (more obvious).

    What if you check if log_callback is NULL, and then assign all 3 values to NULL? and with 1-line comment like "Reset all callbacks since NULL is passed"

    I know this code works already, my comment is only about readability.

    ```
    if (!log_callback) {
       /* Reset all callbacks because NULL is passed */
       global_log_callback = NULL;
       global_log_callback_v2 = NULL;
       global_log_user_data = NULL;
    } else {
    .... and the rest as normal flow
    }
    ```

To view, visit change 86875. To unsubscribe, or for help writing mail filters, visit settings.