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