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.

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

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.

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.