Am 14.05.2014 00:05 schrieb Stefan Tauner:
> On Mon, 12 May 2014 10:32:32 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2...@gmx.net> wrote:
>
>>> From: Stefan Tauner <stefan.tau...@alumni.tuwien.ac.at>
>>> Date: Fri, 9 May 2014 21:18:40 +0200
>>> Subject: [PATCH] Fix selfcheck of various arrays.
>>>
>>> Stefan Reinauer has reported ridiculous NULL checks for arrays in our
>>> self_check function found by Coverity (CID1130005). This patch removes
>>> the useless checks but keeps and fixes the one responsible for the
>>> flashchips array by exporting the array size in a new constant.
>>>
>>> Signed-off-by: Stefan Tauner <stefan.tau...@alumni.tuwien.ac.at>
>>> ---
>>> flash.h | 1 +
>>> flashchips.c | 4 +++-
>>> flashrom.c | 57 ++++++++++++++++++++++++---------------------------------
>>> 3 files changed, 28 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/flash.h b/flash.h
>>> index 59f7cd4..4765213 100644
>>> --- a/flash.h
>>> +++ b/flash.h
>>> @@ -224,6 +224,7 @@ struct flashctx {
>>> #define TIMING_ZERO -2
>>>
>>> extern const struct flashchip flashchips[];
>>> +extern const unsigned int flashchips_size;
>>>
>>> void chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr);
>>> void chip_writew(const struct flashctx *flash, uint16_t val, chipaddr
>>> addr);
>>> diff --git a/flashchips.c b/flashchips.c
>>> index 8059374..92dc947 100644
>>> --- a/flashchips.c
>>> +++ b/flashchips.c
>>> @@ -13362,5 +13362,7 @@ const struct flashchip flashchips[] = {
>>> .write = NULL,
>>> },
>>>
>>> - { NULL }
>>> + {0}
>>> };
>>> +
>>> +const unsigned int flashchips_size = ARRAY_SIZE(flashchips);
>>> diff --git a/flashrom.c b/flashrom.c
>>> index c20461a..9d64da3 100644
>>> --- a/flashrom.c
>>> +++ b/flashrom.c
>>> @@ -1271,10 +1271,7 @@ out_free:
>>> return ret;
>>> }
>>>
>>> -/* This function shares a lot of its structure with
>>> erase_and_write_flash() and
>>> - * walk_eraseregions().
>>> - * Even if an error is found, the function will keep going and check the
>>> rest.
>>> - */
>>> +/* Even if an error is found, the function will keep going and check the
>>> rest. */
>> Not sure if we want to remove parts of the comment... if we ever change
>> the eraseblock structure or semantics, we may want to be able to quickly
>> locate all places where we have to change stuff. And that may happen
>> soon for some weird chips with erase commands which only happen to work
>> on parts of the chip. The idea some months ago was to mark eraseblocks
>> where the erase function doesn't work by design by using a negative
>> eraseblock size.
>> Hm.
> I'd definitely grep for the modified struct member(s) instead of some
> vague comments that might or might not exist :)
> The old comment reads to me as the author suggests that the similarity
> (which is not very surprising actually) should be exploited somehow
> (which I think would be a very bad idea). (erase_and_write_block_helper
> is already a nightmare that will probably never pay off - at least I
> can't see how that could happen).
OK with removing the comment.
>>> static int selfcheck_eraseblocks(const struct flashchip *chip)
>>> {
>>> int i, j, k;
>>> @@ -1697,8 +1694,7 @@ void print_banner(void)
>>>
>>> int selfcheck(void)
>>> {
>>> - const struct flashchip *chip;
>>> - int i;
>>> + unsigned int i;
>>> int ret = 0;
>>>
>>> /* Safety check. Instead of aborting after the first error, check
>>> @@ -1751,37 +1747,32 @@ int selfcheck(void)
>>> ret = 1;
>>> }
>>> }
>>> - /* It would be favorable if we could also check for correct termination
>>> - * of the following arrays, but we don't know their sizes in here...
>>> - * For 'flashchips' we check the first element to be non-null. In the
>>> - * other cases there exist use cases where the first element can be
>>> - * null. */
>>> - if (flashchips == NULL || flashchips[0].vendor == NULL) {
>>> +
>>> + /* It would be favorable if we could check for the correct layout
>>> (especially termination) of various
>>> + * constant arrays: flashchips, chipset_enables, board_matches,
>>> boards_known, laptops_known.
>>> + * They are all defined as externs in this compilation unit so we don't
>>> know their sizes which vary
>>> + * depending on compiler flags, e.g. the target architecture, and can
>>> sometimes be 0.
>>> + * For 'flashchips' we export the size explicitly to work around this
>>> and to be able to implement the
>>> + * checks below. */
>> IMHO the comment above is superfluous.
> Well, apparently quite a lot of people did not understand the problem
> that has been tried to be solved with the code following the original
> comment, neither was the code itself understood (else it would never
> have been committed), nor was the insanity of a completely bogus "fix"
> for this committed to chromiumos (after a review!) clear to the authors:
> https://chromium-review.googlesource.com/#/c/188240/
> To be fair, I did not notice the original problem myself either.
> All of the above indicates to me that there definitely should be an
> explanation, maybe not as verbose... feel free to suggest an
> alternative.
No alternative needed, your explanation is fine.
>>> + if (flashchips_size <= 1 || flashchips[flashchips_size-1].name != NULL)
>>> {
>> size == 1 is arguably stupid because no chip would be supported, but at
>> least programmer init will work and we can get some valuable info from
>> that as well. Not sure we want to forbid it. Then again, what use is
>> flashrom without any flash chip support? I tend to agree with the code
>> you propose.
> yes... *shrug*
>
>> Whitespace: flashchips_size - 1
> fixed.
>
>>> msg_gerr("Flashchips table miscompilation!\n");
>>> ret = 1;
>>> + } else {
>>> + for (i = 0; i < flashchips_size - 1; i++) {
>>> + const struct flashchip *chip = &flashchips[i];
>>> + if (chip->vendor == NULL || chip->name == NULL ||
>>> chip->bustype == BUS_NONE) {
>>> + ret = 1;
>>> + msg_gerr("ERROR: Some field of flash chip #%d
>>> (%s) is misconfigured.\n"
>>> + "Please report a bug at
>>> flashrom@flashrom.org\n", i,
>>> + chip->name == NULL ? "unnamed" :
>>> chip->name);
>>> + }
>>> + if (selfcheck_eraseblocks(chip)) {
>>> + ret = 1;
>>> + }
>>> + }
>>> }
>>> - for (chip = flashchips; chip && chip->name; chip++)
>>> - if (selfcheck_eraseblocks(chip))
>>> - ret = 1;
>>>
>>> -#if CONFIG_INTERNAL == 1
>>> - if (chipset_enables == NULL) {
>>> - msg_gerr("Chipset enables table does not exist!\n");
>>> - ret = 1;
>>> - }
>>> - if (board_matches == NULL) {
>>> - msg_gerr("Board enables table does not exist!\n");
>>> - ret = 1;
>>> - }
>>> - if (boards_known == NULL) {
>>> - msg_gerr("Known boards table does not exist!\n");
>>> - ret = 1;
>>> - }
>>> - if (laptops_known == NULL) {
>>> - msg_gerr("Known laptops table does not exist!\n");
>>> - ret = 1;
>>> - }
>>> -#endif
>> Please leave out that removal or replace it with similar size checks as
>> for flashchips[] except that the other array sizes only fail if <1. The
>> way you do the checks above is definitely the way to go.
> Why should I keep useless and embarrassing code in when the comment
> below replaces it well?
Yes, given that you added the FIXME comment above, a removal here is OK.
> And BTW i don't deem the other arrays worth
> checking. The flashchips array is special because it is complicated and
> the complexity (i.e. eraser layouts) can easily be checked for sanity
> automatically. The others are simply arrays with easily to check
> semantics and syntaxes.
Except the board enable table which is a nightmare on its own, and even
I don't remember the exact semantics for which fields are required in
which combination for which effect unless I explicitly read the code and
comments.
That said, given funnies like arch-specific contents of some arrays
(e.g. processor enables, chipset enables), I'd like to have some checks
for them as well just to make sure compilation on non-x86 doesn't mess
them up. This can be done in a followup patch, though.
> If I'd touch related code at all then I would
> like to remove the NULL delimiter and use the (then exported) constant
> size variables to iterate over them everywhere.
Hm. I wonder if that causes any fun down the road when someone changes a
checked array and recompiles only the affected file. Should be safe.
Anyway, having a NULL-containing member as terminator allows us to
iterate over pointers instead of having to use an explicit counter
variable somewhere.
>>> + /* TODO: implement similar sanity checks for other arrays where deemed
>>> necessary. */
>>> return ret;
>>> }
>>>
> Sorry for being a bit bitchy today :)
Sorry for the long delay of approval.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
