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.
Thanks
Minfei