* [PATCH v2] kexec: Use min_t to simplify logic
@ 2013-02-24 14:37 Zhang Yanfei
2013-02-25 0:36 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Zhang Yanfei @ 2013-02-24 14:37 UTC (permalink / raw)
To: Eric W. Biederman, Andrew Morton, Simon Horman; +Cc: kexec, linux-kernel
From: Zhang Yanfei <zhangyanfei@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.
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
---
kernel/kexec.c | 22 ++++++----------------
1 files changed, 6 insertions(+), 16 deletions(-)
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);
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);
}
@@ -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);
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] kexec: Use min_t to simplify logic
2013-02-24 14:37 [PATCH v2] kexec: Use min_t to simplify logic Zhang Yanfei
@ 2013-02-25 0:36 ` Simon Horman
2013-02-25 23:35 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2013-02-25 0:36 UTC (permalink / raw)
To: Zhang Yanfei; +Cc: Eric W. Biederman, Andrew Morton, kexec, linux-kernel
On Sun, Feb 24, 2013 at 10:37:21PM +0800, Zhang Yanfei wrote:
> From: Zhang Yanfei <zhangyanfei@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@verge.net.au>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Simon Horman <horms@verge.net.au>
> Signed-off-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> ---
> kernel/kexec.c | 22 ++++++----------------
> 1 files changed, 6 insertions(+), 16 deletions(-)
>
> 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);
>
> 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);
> }
> @@ -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);
>
> --
> 1.7.1
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] kexec: Use min_t to simplify logic
2013-02-25 0:36 ` Simon Horman
@ 2013-02-25 23:35 ` Andrew Morton
2013-02-25 23:59 ` Simon Horman
2013-02-26 4:44 ` Zhang Yanfei
0 siblings, 2 replies; 5+ messages in thread
From: Andrew Morton @ 2013-02-25 23:35 UTC (permalink / raw)
To: Simon Horman; +Cc: Zhang Yanfei, Eric W. Biederman, kexec, linux-kernel
On Mon, 25 Feb 2013 09:36:51 +0900
Simon Horman <horms@verge.net.au> wrote:
> On Sun, Feb 24, 2013 at 10:37:21PM +0800, Zhang Yanfei wrote:
> > From: Zhang Yanfei <zhangyanfei@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@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.
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.
> > 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] kexec: Use min_t to simplify logic
2013-02-25 23:35 ` Andrew Morton
@ 2013-02-25 23:59 ` Simon Horman
2013-02-26 4:44 ` Zhang Yanfei
1 sibling, 0 replies; 5+ messages in thread
From: Simon Horman @ 2013-02-25 23:59 UTC (permalink / raw)
To: Andrew Morton; +Cc: Zhang Yanfei, Eric W. Biederman, kexec, linux-kernel
On Mon, Feb 25, 2013 at 03:35:54PM -0800, Andrew Morton wrote:
> On Mon, 25 Feb 2013 09:36:51 +0900
> Simon Horman <horms@verge.net.au> wrote:
>
> > On Sun, Feb 24, 2013 at 10:37:21PM +0800, Zhang Yanfei wrote:
> > > From: Zhang Yanfei <zhangyanfei@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@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.
My bad, I meant:
Reviewed-by: Simon Horman <horms@verge.net.au>
> 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.
>
> 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.
>
> > > 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.
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] kexec: Use min_t to simplify logic
2013-02-25 23:35 ` Andrew Morton
2013-02-25 23:59 ` Simon Horman
@ 2013-02-26 4:44 ` Zhang Yanfei
1 sibling, 0 replies; 5+ messages in thread
From: Zhang Yanfei @ 2013-02-26 4:44 UTC (permalink / raw)
To: Andrew Morton
Cc: Simon Horman, linux-kernel, kexec, Eric W. Biederman, Zhang Yanfei
于 2013年02月26日 07:35, Andrew Morton 写道:
> On Mon, 25 Feb 2013 09:36:51 +0900
> Simon Horman <horms@verge.net.au> wrote:
>
>> On Sun, Feb 24, 2013 at 10:37:21PM +0800, Zhang Yanfei wrote:
>>> From: Zhang Yanfei <zhangyanfei@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@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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-02-26 4:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-24 14:37 [PATCH v2] kexec: Use min_t to simplify logic Zhang Yanfei
2013-02-25 0:36 ` Simon Horman
2013-02-25 23:35 ` Andrew Morton
2013-02-25 23:59 ` Simon Horman
2013-02-26 4:44 ` Zhang Yanfei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).