Re: [PATCH] [MTD] UBI: Fix counting of ec value

Timo Lindhorst
2007-02-12T10:50:07Z

Hi Artem,

>>+ ec_hdr = ubi_zalloc_ec_hdr(ubi);
>>+ if (unlikely(!ec_hdr))
>>+ return -ENOMEM;
>
>
> So why have you moved this memory allocation here?
Actually I have not moved up the allocation, but moved down the
'if (unlikely(ec > UBI_MAX_ERASECOUNTER)) { ... ' block, since we need
to know the new ec value before we can do this test.

I thought you want the allocation to be done before erasure, to prevent
the erasure if the ec_hdr cannot be written due to a failure of memory
allocation.
Otherwise, the allocation can be moved down again, but than the new ec
is possibly not written to the EB if no memory can be allocated for ec_hdr.

Do I miss something? What do you think?

>>+
>>+ ret = err = ubi_io_sync_erase(ubi, e->pnum, torture);
>>+ if (unlikely(err < 0))
>>+ goto out_free;
>>+
>>+ ec += ret;
>
> What's the point in the new 'ret' variable? Why ec += err does not work?
I just thought adding an error code to a counter seems odd. Do you
prefer this way?

>
>> if (unlikely(ec > UBI_MAX_ERASECOUNTER)) {
>> /*
>> * Erase counter overflow. Upgrade UBI and use 64-bit
>> * erase counters internally.
>> */
>> ubi_err("erase counter overflow at PEB %d, EC %d",
>>- e->pnum, e->ec);
>>+ e->pnum, ec);
>> return -EINVAL;
>
> And now you do not free memory.
Yes, sorry! I have changed it to:

err = -EINVAL;
goto out_free;
}

I will resend the patch when I have got your comments to the upper points.

Kind regards,
Timo