[PATCH] Add checking num-threads of makedumpfile

[Minfei Huang]

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