[PATCH] Add checking num-threads of makedumpfile

["Zhou, Wenjian/周文剑"]

On 08/25/2015 03:36 PM, Minfei Huang wrote:
> On 08/25/15 at 03:21pm, "Zhou, Wenjian/周文剑" wrote:
>> On 08/25/2015 02:57 PM, Minfei Huang wrote:
>>> On 08/25/15 at 02:17pm, "Zhou, Wenjian/周文剑" wrote:
>>>>>>>> +	if [ $? -eq 0 ];then
>>>>>>>> +		local nr_cpus=1
>>>>>>>> +		local num_threads=0
>>>>>>>> +		local core_collector=`grep -v "^#" $KDUMP_CONFIG_FILE | grep "^core_collector"`
>>>>>>>> +
>>>>>>>> +		num_threads=`echo ${core_collector#*--num-threads} | awk '{print $1}'`
>>>>>>>> +		nr_cpus=`echo ${KDUMP_COMMANDLINE_APPEND#*nr_cpus=} | awk '{print $1}'`
>>>>>>>> +
>>>>>>>> +		test $num_threads -ge $nr_cpus &> /dev/null
>>>>>>>> +		if [ $? -eq 0 ];then
>>>>>>>
>>>>>>> To keep the style, it is better to use "if [ $a -ge $b ]".
>>>>>>>
>>>>>>
>>>>>> Using "test" is to handle the situation that the $num_threads or $nr_cpus is not an integer.
>>>>>>
>>>>>
>>>>> Is it allowed for $num_threads or $nr_cpus, if the value is not the
>>>>> integer?
>>>>>
>>>>
>>>> Of course not integer is invalid. But the value of num_threads won't be checked
>>>> until makedumpfile is executed.
>>>> If user sets "core_collector makedumpfile --num-threads a" in kdump.conf, it will
>>>> lead to a fault here.
>>>>
>>>
>>> I think it is fine without validating the type of num-threads, since
>>> makedumpfile will do this work.
>>>
>>> kdump does not check other usages of makedumpfile as well. It is better
>>> to wrap a new function to check it in kdumpctl, if you want to try to
>>> validate the usage value.
>>>
>>
>> I agree with you that we should not validate the type of num-threads.
>> But using "test" is not to validate the type. It is just for avoiding the validating.
>> If don't avoid it, the fault will occur here. And user may feel strange about the
>> error message.
>
> If we can detect the error as early as we can, the rate we dump the
> vmcore successfully is higher.
>

Do you mean it's also needed to check the usage of makedumpfile in kdumpctl,
even makedumpfile will do this?

I think kdump should not do makedumpfile's work. kdump just provides a way
for users to use different core collectors. There is no need for kdump to
check how the core collector users use.

-- 
Thanks
Zhou