? 2013?02?26? 07:35, Andrew Morton ??:
> On Mon, 25 Feb 2013 09:36:51 +0900
> Simon Horman <horms at verge.net.au> wrote:
>
>> On Sun, Feb 24, 2013 at 10:37:21PM +0800, Zhang Yanfei wrote:
>>> From: Zhang Yanfei <zhangyanfei at cn.fujitsu.com>
>>>
>>> This is just a tweak: using min_t to simplify logic of variable
>>> assignments.
>>>
>>> v2:
>>> - Rewrite patch description as Simon suggested.
>>> - Fix an inappropriate if test introduced by v1. Thanks Simon.
>>
>> Hi Zhang,
>>
>> thanks for the update.
>>
>> Signed-off-by: Simon Horman <horms at verge.net.au>
>
> Signed-off-by: implies that you were involved in the development or
> patch delivery. Were you? If not, an Acked-by or Reviewed-by is more
> appropriate.
>
> Also, the need to use min_t rather than min is a sign that the types
> are screwed up. Let's take a look.
>
>>>
>>> diff --git a/kernel/kexec.c b/kernel/kexec.c
>>> index 2436ffc..effd655 100644
>>> --- a/kernel/kexec.c
>>> +++ b/kernel/kexec.c
>>> @@ -822,13 +822,8 @@ static int kimage_load_normal_segment(struct kimage *image,
>>> /* Start with a clear page */
>>> clear_page(ptr);
>>> ptr += maddr & ~PAGE_MASK;
>>> - mchunk = PAGE_SIZE - (maddr & ~PAGE_MASK);
>>> - if (mchunk > mbytes)
>>> - mchunk = mbytes;
>>> -
>>> - uchunk = mchunk;
>>> - if (uchunk > ubytes)
>>> - uchunk = ubytes;
>>> + mchunk = min_t(size_t, mbytes, PAGE_SIZE - (maddr & ~PAGE_MASK));
>>> + uchunk = min_t(size_t, ubytes, mchunk);
>
> The types of ubytes and mbytes are clearly wrong. They are initialised
> from a size_t and they are manipulated alongside size_t's.
Ah, yes. I didn't realize this before.
>
> The types of PAGE_SIZE and PAGE_MASK are vague - iirc they once had
> different types on different architectures, so some form of casting is
> unavoidable here.
Yes.
>
>>> result = copy_from_user(ptr, buf, uchunk);
>>> kunmap(page);
>>> @@ -874,13 +869,9 @@ static int kimage_load_crash_segment(struct kimage *image,
>>> }
>>> ptr = kmap(page);
>>> ptr += maddr & ~PAGE_MASK;
>>> - mchunk = PAGE_SIZE - (maddr & ~PAGE_MASK);
>>> - if (mchunk > mbytes)
>>> - mchunk = mbytes;
>>> -
>>> - uchunk = mchunk;
>>> - if (uchunk > ubytes) {
>>> - uchunk = ubytes;
>>> + mchunk = min_t(size_t, mbytes, PAGE_SIZE - (maddr & ~PAGE_MASK));
>>> + uchunk = min_t(size_t, ubytes, mchunk);
>>> + if (mchunk > uchunk) {
>>> /* Zero the trailing part of the page */
>>> memset(ptr + uchunk, 0, mchunk - uchunk);
>>> }
>
> Again, mybtes and ubytes have the wrong type.
>
>>> @@ -1461,8 +1452,7 @@ void vmcoreinfo_append_str(const char *fmt, ...)
>>> r = vsnprintf(buf, sizeof(buf), fmt, args);
>>> va_end(args);
>>>
>>> - if (r + vmcoreinfo_size > vmcoreinfo_max_size)
>>> - r = vmcoreinfo_max_size - vmcoreinfo_size;
>>> + r = min_t(size_t, r, vmcoreinfo_max_size - vmcoreinfo_size);
>>>
>>> memcpy(&vmcoreinfo_data[vmcoreinfo_size], buf, r);
>
> vmcoreinfo_max_size is a size_t and vmcoreinfo_size is a size_t and
> memcpy's `size' argument is a size_t. Therefore the type of `r' should
> be... what? int! bzzzzt.
yes.
So I will split the patch into two, first fix the wrong types here. Then
use min_t to simply the logic.
Thanks
Zhang
>
>
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
