From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754675AbcGEHKN (ORCPT ); Tue, 5 Jul 2016 03:10:13 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:11086 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751301AbcGEHKK (ORCPT ); Tue, 5 Jul 2016 03:10:10 -0400 X-IBM-Helo: d23dlp01.au.ibm.com X-IBM-MailFrom: hbathini@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Subject: Re: [v2,1/2] refactor code parsing size based on memory range To: Michael Ellerman , lkml , linuxppc-dev References: <3rbRd54y9Vz9t0p@ozlabs.org> <52b513d4-16a2-ca16-975e-a755b7a5ea8d@linux.vnet.ibm.com> <87twg4br3q.fsf@@concordia.ellerman.id.au> Cc: Rusty Russell , kexec@lists.infradead.org, Eric Biederman , Vivek Goyal From: Hari Bathini Organization: IBM LTC INDIA Date: Tue, 5 Jul 2016 12:40:00 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <87twg4br3q.fsf@@concordia.ellerman.id.au> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16070507-0052-0000-0000-000001AAB236 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16070507-0053-0000-0000-0000065100D4 Message-Id: <626eae51-9e4a-573e-9ae5-6417315522f1@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-07-05_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1607050066 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/05/2016 10:48 AM, Michael Ellerman wrote: >> On 06/24/2016 10:56 AM, Michael Ellerman wrote: >>> On Wed, 2016-22-06 at 19:25:26 UTC, Hari Bathini wrote: > ... >> While the code is moved to kernel/params.c file, there is no change in logic >> for crashkernel parameter parsing as the moved code is invoked with function >> calls at appropriate places. Hi Michael, > Are you sure that's true? Yes. I tested it. > > The old code would return -EINVAL from parse_crashkernel_mem() for any > error, regardless of whether it had already parsed some of the string. > > eg: > >>>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c >>>> index 56b3ed0..d43f5cc 100644 >>>> --- a/kernel/kexec_core.c >>>> +++ b/kernel/kexec_core.c >>>> @@ -1083,59 +1083,9 @@ static int __init parse_crashkernel_mem(char *cmdline, >>>> char *cur = cmdline, *tmp; >>>> >>>> /* for each entry of the comma-separated list */ >>>> - do { >>>> - unsigned long long start, end = ULLONG_MAX, size; >>>> - >>>> - /* get the start of the range */ >>>> - start = memparse(cur, &tmp); >>>> - if (cur == tmp) { >>>> - pr_warn("crashkernel: Memory value expected\n"); >>>> - return -EINVAL; >>>> - } >>>> - cur = tmp; >>>> - if (*cur != '-') { >>>> - pr_warn("crashkernel: '-' expected\n"); >>>> - return -EINVAL; >>>> - } >>>> - cur++; >>>> - >>>> - /* if no ':' is here, than we read the end */ >>>> - if (*cur != ':') { >>>> - end = memparse(cur, &tmp); >>>> - if (cur == tmp) { >>>> - pr_warn("crashkernel: Memory value expected\n"); >>>> - return -EINVAL; >>>> - } > So eg, if I give it "128M-foo" it will modify cur, and then error out here ^ It does modify cur (local variable) but that would have no bearing on parsing logic as we are returning immediately.. > You've changed that to: > >>>> + *crash_size = parse_mem_range_size("crashkernel", &cur, system_ram); >>>> + if (cur == cmdline) >>>> + return -EINVAL; > Which only returns EINVAL if cur is not modified at all. I think the confusion is with the same local variable cur in parse_crashkernel_mem() & parse_mem_range_size() functions. We modified cur (local variable) in parse_mem_range_size() but the output parameter (char **str) remains unchanged unless we find a match. Thanks Hari > And looking below: > >>>> diff --git a/kernel/params.c b/kernel/params.c >>>> index a6d6149..84e40ae 100644 >>>> --- a/kernel/params.c >>>> +++ b/kernel/params.c > ... >>>> +unsigned long long __init parse_mem_range_size(const char *param, >>>> + char **str, >>>> + unsigned long long system_ram) >>>> +{ >>>> + char *cur = *str, *tmp; >>>> + unsigned long long mem_size = 0; >>>> + >>>> + /* for each entry of the comma-separated list */ >>>> + do { >>>> + unsigned long long start, end = ULLONG_MAX, size; >>>> + >>>> + /* get the start of the range */ >>>> + start = memparse(cur, &tmp); >>>> + if (cur == tmp) { >>>> + printk(KERN_INFO "%s: Memory value expected\n", param); >>>> + return mem_size; >>>> + } >>>> + cur = tmp; >>>> + if (*cur != '-') { >>>> + printk(KERN_INFO "%s: '-' expected\n", param); >>>> + return mem_size; >>>> + } >>>> + cur++; >>>> + >>>> + /* if no ':' is here, than we read the end */ >>>> + if (*cur != ':') { >>>> + end = memparse(cur, &tmp); >>>> + if (cur == tmp) { >>>> + printk(KERN_INFO "%s: Memory value expected\n", >>>> + param); >>>> + return mem_size; > If we error out here for example, we have modified cur, so the code above > *won't* return EINVAL. > > Which looks like a behaviour change to me? > > cheers > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev