[PATCH] Add checking num-threads of makedumpfile

["Zhou, Wenjian/周文剑"]

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.

-- 
Thanks
Zhou

>>>> BTW, should we add some descriptions to this feature of makedumpfile in some where?
>>>
>>> IMO, it is appropriate to document it in the makedumpfile manpage, like
>>> the feature --split. For kdump, we only document the default usages for
>>> makedumpfile in kexec-kdump-howto.txt.
>>>