Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx> writes:
> On 02/25/2015 01:29 PM, Jes Sorensen wrote:
>> Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx> writes:
>>> On 02/24/2015 10:00 PM, Jes.Sorensen@xxxxxxxxxx wrote:
>>>> From: Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
>>>>
>>>> This avoids adding the same orom entry to the oroms list multiple
>>>> times, as the comparison of pointers is never going to succeed, in
>>>> particular when '*orom' points to a local stack variable in the
>>>> calling function.
>>>>
>>>> Signed-off-by: Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
>>>> ---
>>>> platform-intel.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/platform-intel.c b/platform-intel.c
>>>> index 37274da..a4ffa9f 100644
>>>> --- a/platform-intel.c
>>>> +++ b/platform-intel.c
>>>> @@ -255,8 +255,8 @@ static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>>>> int i;
>>>>
>>>> for (i = 0; i < SYS_DEV_MAX; i++) {
>>>> - if (&oroms[i].orom == orom)
>>>> - return orom;
>>>> + if (!memcmp(&oroms[i].orom, orom, sizeof(struct imsm_orom)))
>>>> + return &oroms[i].orom;
>>>> if (oroms[i].orom.signature[0] == 0) {
>>>> oroms[i].orom = *orom;
>>>> return &oroms[i].orom;
>>>>
>>>
>>> Hi Jes,
>>>
>>> You are right that this can add the same entry multiple times, but this
>>> is how it is supposed to work. The oroms list should contain all the
>>> platform's oroms and they can be the same, this is why memcmp() should
>>> not be used here. We don't want to compare the contents of the
>>> structure, just its address. Sorry if it's not clear.
>>
>> Artur,
>>
>> Then the code is fundamentally broken, since you end up comparing a
>> stack variable against the oroms array when you call it from
>> find_imsm_efi(). Worse you can end up returning the local stack variable
>> declared in find_imsm_efi() to the calling function - there is no way
>> that can be correct.
>>
>> Look at this:
>>
>> static const struct imsm_orom *add_orom(const struct imsm_orom *orom)
>> {
>> int i;
>>
>> for (i = 0; i < SYS_DEV_MAX; i++) {
>> if (&oroms[i].orom == orom)
>> return orom;
>> if (oroms[i].orom.signature[0] == 0) {
>> oroms[i].orom = *orom;
>> return &oroms[i].orom;
>> }
>> }
>> return NULL;
>> }
>>
>> const struct imsm_orom *find_imsm_efi(struct sys_dev *hba)
>> {
>> struct imsm_orom orom;
>> const struct imsm_orom *ret;
>> int err;
>>
>> ....
>>
>> ret = add_orom(&orom);
>> add_orom_device_id(ret, hba->dev_id);
>>
>> return ret;
>> }
>
> I can't see how this can lead to returning a stack variable. The oroms
> array is global and add_orom() will always return a pointer to a struct
> in this array. This comparison will always fail when we pass a pointer
> to a stack variable to add_orom():
>
> if (&oroms[i].orom == orom)
> return orom;
>
> This was meant to prevent adding an orom again like this:
>
> ret = add_orom(&orom);
> add_orom(ret);
>
> Maybe it would be more appropriate to return NULL to indicate that
> nothing was added instead of returning back the same pointer. I can do a
> patch for this. What do you think?
It will fail because we know we're comparing a stack pointer, but it
raises red flags with tools like coverity and it is really bad coding
practice to rely on hacks like this.
I also don't understand why you want to keep a table of identical
entries in the orom structure if multiple identical entries are found.
Each entry ought to match onto a specific physical controller, unless I
get something wrong?
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
