[flashrom] Libflashrom draft patch - initial review

[Stefan Tauner]

On Thu, 11 Jun 2015 23:26:19 +0200
Łukasz Dmitrowski <lukasz.dmitrowski at gmail.com> wrote:

> Hello,
> 
> My name is Łukasz Dmitrowski and I am one of coreboot/flashrom GSoC 
> students this year. You can read about my project here: 
> http://blogs.coreboot.org/blog/author/dmitro/.

Hi Łukasz,

regarding the window showing the supported hardware: Some features that
would make it better than the existing interfaces to this database
(namely the -L output of flashrom as well as the tables in the wiki):
- searching
- sorting
- filtering (e.g. showing flash chips with 4 and 8 MB density only)

The extract components icon is not ideal. The arrow you are using
symbolizes "undo" usually... I would suggest that you use something
like
http://fotofile.net/bt/root/usr/share/icons/Humanity/actions/22/extract-archive.svg
(a typical symbol for unwrapping a parcel).

> 
> One of my first steps is updating and extending Nico Hubers libflashrom 
> patch. Work is still in progress, but some changes are already made, so 
> I thought that it will be good idea to send a patch just for review to 
> know if I am going in a good direction. I want to improve my skills and 
> provide best code quality as I can, so I am open for criticism - I am 
> here to learn :)
> 
> Patch summary:
> 
> Makefile: I added cli_common.o and print.o to LIB_OBJS - it solves 
> undefined references in current libflashrom version, but spoils standard 
> flashrom build with 'make', I did not investigated it yet as my focus is 
> to extend library with new functions and update already existing, need 
> to discuss it with my mentor
> 
> cli_classic.c, cli_output.c, flash.h: Only Nico Hubers changes here, 
> didn't touch it
> 
> libflashrom.c, libflashrom.h: I added 8 functions and made a few changes 
> to existing ones, as some of used functions are static, or does not exist.
> 
> New functions:
> 
> int fl_supported_programmers(const char **supported_programmers);
> int fl_supported_flash_chips(fl_flashchip_info_t *fchips);
> int fl_supported_boards(fl_board_info_t *boards);
> int fl_supported_chipsets(fl_chipset_info_t *chipsets);
> int fl_supported_programmers_number();
> int fl_supported_flash_chips_number();
> int fl_supported_boards_number();
> int fl_supported_chipsets_number();
> 
> New types:
> fl_flashchip_info_t - smaller version of struct flashchip
> fl_board_info_t - smaller version of struct board_info
> fl_chipset_info_t - smaller version of struct penable
> fl_test_state - copy of enum test_state
> 
> So we have 4 functions that return a list of supported programmers, 
> chips, boards and chipsets. Next 4 functions return number of supported 
> hardware of certain type. For example, to obtain a list of supported 
> boards, you can create an array of structures fl_board_info of suitable 
> size (the size is known - fl_supported_boards_number()), then pass it to 
> fl_supported_boards(fl_board_info_t *boards) and you will have it filled 
> with data.
> 
> Regarding changes in Nico Hubers functions: I made temporary comments 
> for every change and marked it with LD_CHANGE_NOTE. Every comment 
> contains previous code and reason why it has been commented out.
> 
> Thanks in advance for your insights!

As Urja mentioned already, your patch got mangled by your MUA, which
introduced some line breaks where there shouldnt be any. Also, you
annotated your changes to Nico's patch within the code. It would make
much more sense if you would post those changes as independent patches
with a proper change log, like you did in your github repository (well
maybe not split in that many patches, but the idea is right). BTW
github... you should have cloned my repository there instead of
starting a complete new one without history.

Also, I agree with Urja that the functions returning only the number of
X are not the way to go. I am not sure yet how to proceed with them and
all the external types yet. Any new types should not have a _t suffix.
These names are reserved by POSIX. However, I do not feel certain if we
need new types at all..

Please repost your changes in a more reviewable manner, thanks.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner