Hi Xunlei,
On 09/25/17 at 09:12am, Xunlei Pang wrote:
On 09/19/2017 at 02:42 PM, Dave Young wrote:
> On 09/19/17 at 02:30pm, Baoquan He wrote:
>> On 09/19/17 at 02:17pm, Dave Young wrote:
>>> On 09/18/17 at 08:55am, Baoquan He wrote:
>>>> On 09/18/17 at 08:34am, Baoquan He wrote:
>>>>> On 09/17/17 at 09:28pm, Bhupesh Sharma wrote:
>>>>>> + # Check if we have any leading spaces (or tabs) before the
>>>>>> + # variable name in the kdump conf file. While doing so exclude
any
>>>>>> + # comments which appear to have leading spaces (or tabs)
>>>>>> + awk 'BEGIN{cnt=0} {cnt=match($0, /^[[:blank:]]/);
if(cnt!=0) print $0}' $KDUMP_CONFIG_FILE >> $TMP_FILE
>>>>>> +
>>>>>> + nr=$(awk 'BEGIN{cnt_hash=0; cnt_blank=0}
{cnt_hash=match($0, /#.*$/); if(cnt_hash!=0) next; else cnt_blank++} END{print
cnt_blank}' $TMP_FILE)
>>>>>> + [ $nr -gt 0 ] && {
>>>>>> + echo "No whitespaces are allowed before a kdump option
name. Please check $KDUMP_CONFIG_FILE"
>>>>>> + rm -f $TMP_FILE
>>>>>> + return 1
>>>>> How about this:
>>>>>
>>>>> if grep -v "^[[:blank:]]*#" /etc/kdump.conf| grep -v
"^$"| grep "^[[:space:]]"; then
>>>>> fi
>>>>>
>>>>> It can ignore the comment at the beginning with [[:blank:]], and
blank
>>>>> line, only check line with config info.
>>>>>
>>>>> And why don't we just help erase the [[:blank:]] at the
beginning of
>>>>> config line, like this:
>>>>>
>>>>> grep -v "^[[:blank:]]*#" /etc/kdump.conf| grep -v
"^$"| grep -v "^[[:space:]]"
>>>> OK, forget this saying. Here it's just checking. So the command
'read'
>>>> won't filter out the [[:blank:]] for config and value?
>>> It should, I think it is casued by below code in mkdumprd, just guess I
>>> did not verify it:
>>> SAVE_PATH=$(grep ^path $conf_file| cut -d' ' -f2)
>>>
>>> So we have several different places parsing kdump.conf, kdumpctl,
>>> mkdumprd kdump.sh etc. If we support the leading whitespace, we need
>>> make sure it works on every cases, so I worried that it make the code
>>> more complex.
>>>
>>> But in this patch it seems also not worth the awk scripts, something
>>> like below should work:
>>>
>>> if egrep '^[[:blank:]]+[a-z]' $KDUMP_CONFIG_FILE; then
>>> echo "No whitespaces are allowed before a kdump option name."
>>> return 1
>>> fi
>> Hmm, this looks better. I think it's worth to try to see where the bug
>> is caused and if we can fix it. As we know, in kernel we try to so hard
>> to parse cmdline, erase space. If we can't do it well in shell, I worry
>> people will mock on kdump team.
> I was wrong, the mkdumprd code I mentioned is not the culprit,
> Agree that it is good to find where is the code which causes the bug
> though.
>
> But as to support the leasing whitei space, I'm still not sure.
> If the fix is simple enough I think we can, if not then we can
> define that kdump.conf options need start with non-whitespace. As we
> have many places use ^ to find the option name it should be not easy.
> To remove whitespace in user kdump.conf or to use a temp file sounds also
> not a good option.
Hmm, I think at least we should say sorry to users like below:
echo "Sorry! No whitespaces are allowed before a kdump option name in
$KDUMP_CONFIG_FILE"
It sounds not good to add "Sorry" in user visible messages.
There are a lot of places assuming ^$option_name in the code,
I do not object to support leading white space, just want a clean and
elegant patch, if we can not then it is fine to error out until we have
a good solution in the future.
Regards,
Xunlei
>>
>>>>> Thanks
>>>>> Baoquan
>>>>>
>>>>>> + }
>>>>>> +
>>>>>> + rm -f $TMP_FILE
>>>>>> +
>>>>>> while read config_opt config_val; do
>>>>>> case "$config_opt" in
>>>>>> \#* | "")
>>>>>> --
>>>>>> 2.7.4
>>>>>> _______________________________________________
>>>>>> kexec mailing list -- kexec(a)lists.fedoraproject.org
>>>>>> To unsubscribe send an email to
kexec-leave(a)lists.fedoraproject.org
>>>>> _______________________________________________
>>>>> kexec mailing list -- kexec(a)lists.fedoraproject.org
>>>>> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
>>>> _______________________________________________
>>>> kexec mailing list -- kexec(a)lists.fedoraproject.org
>>>> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
>>> Thanks
>>> Dave
> Thanks
> Dave
> _______________________________________________
> kexec mailing list -- kexec(a)lists.fedoraproject.org
> To unsubscribe send an email to kexec-leave(a)lists.fedoraproject.org
Thanks
Dave