Messages in this thread | | | Date | Thu, 07 Feb 2013 14:22:52 +0800 | From | Tang Chen <> | Subject | Re: [PATCH 3/3] acpi, memory-hotplug: Support getting hotplug info from SRAT. |
| |
On 02/07/2013 05:54 AM, Andrew Morton wrote: > On Wed, 06 Feb 2013 10:20:57 +0800 > Tang Chen<tangchen@cn.fujitsu.com> wrote: > >>>> >>>> + if (!strncmp(p, "acpi", max(4, strlen(p)))) >>>> + movablemem_map.acpi = true; >>> >>> Generates a warning: >>> >>> mm/page_alloc.c: In function 'cmdline_parse_movablemem_map': >>> mm/page_alloc.c:5312: warning: comparison of distinct pointer types lacks a cast >>> >>> due to max(int, size_t). >>> >>> This is easily fixed, but the code looks rather pointless. If the >>> incoming string is supposed to be exactly "acpi" then use strcmp(). If >>> the incoming string must start with "acpi" then use strncmp(p, "acpi", 4). >>> >>> IOW, the max is unneeded? >> >> Hi Andrew, >> >> I think I made another mistake here. I meant to use min(4, strlen(p)) in >> case p is >> something like 'aaa' whose length is less then 4. But I mistook it with >> max(). >> >> But after I dig into strcmp() in the kernel, I think it is OK to use >> strcmp(). >> min() or max() is not needed. > > OK, I did that. > > But the code still looks a bit more complex than we need. Could we do > > static int __init cmdline_parse_movablemem_map(char *p) > { > char *oldp; > u64 start_at, mem_size; > > if (!p) > goto err; > > /* > * If user decide to use info from BIOS, all the other user specified > * ranges will be ingored. > */ > if (!strcmp(p, "acpi")) { > movablemem_map.acpi = true; > if (movablemem_map.nr_map) { > memset(movablemem_map.map, 0, > sizeof(struct movablemem_entry) > * movablemem_map.nr_map); > movablemem_map.nr_map = 0; > } > return 0; > } > > No, I don't think so.
If user specified like this:
1) movablemem_map=aaa@bbb ---------- will be added into array 2) movablemem_map=acpi ---------- will empty the array 3) movablemem_map=ccc@ddd ---------- will be added into array again (wrong!)
So, we need to code like this:
+ if (!strncmp(p, "acpi", max(4, strlen(p)))) + movablemem_map.acpi = true;
In this way, 3) movablemem_map=ccc@ddd will not go into this if segment.
+ + /* + * If user decide to use info from BIOS, all the other user specified + * ranges will be ingored. + */ + if (movablemem_map.acpi) { + if (movablemem_map.nr_map) { + memset(movablemem_map.map, 0, + sizeof(struct movablemem_entry) + * movablemem_map.nr_map); + movablemem_map.nr_map = 0; + } + return 0; + }
But it will go into this if segment, and will not add the range into array.
Thanks. :)
| |