[PATCH] Add checking num-threads of makedumpfile

["Zhou, Wenjian/周文剑"]

On 08/25/2015 04:18 PM, Minfei Huang wrote:
> On 08/25/15 at 04:07pm, "Zhou, Wenjian/周文剑" wrote:
>> 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.
>
> You are right that kdump does not need to do the work which makdumpfile
> does.
>
> I mean you can leave the condition to use "if [ $a -ge $b ]". It is fine
> if kdump raises the error message due to the invalid value.
>

I accept it.

-- 
Thanks
Zhou