Re: [PATCH v2] kexec: Use min_t to simplify logic

[Zhang Yanfei]

ä 2013å02æ26æ 07:35, Andrew Morton åé:
> On Mon, 25 Feb 2013 09:36:51 +0900
> Simon Horman <horms@xxxxxxxxxxxx> wrote:
> 
>> On Sun, Feb 24, 2013 at 10:37:21PM +0800, Zhang Yanfei wrote:
>>> From: Zhang Yanfei <zhangyanfei@xxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/kexec
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/