On 09/18/17 at 12:17pm, Bhupesh Sharma wrote:
Hi Bao,
Thanks for the review.
On Mon, Sep 18, 2017 at 6:25 AM, Baoquan He <bhe(a)redhat.com> 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?
This was discussed in the original BZ report:
https://bugzilla.redhat.com/show_bug.cgi?id=1484945
i.e.,
- We can either say that there should be no space before the path by
failing the service, or
- We will have to change the code and remove the spaces in /etc/kdump.conf file.
The 2nd option looks better to me if we can fix it. Of course we can
also give a warning if agreement has been made by you and Dave. But using
awk to check that is a little overkilling.
Personal opinion.
>
> However, as discussed with Dave on irc earlier, we need to add proper
> error handling in kdumpctl, as any user can add a space before a
> variable in kdump.conf and this should be handled properly.
>
> This makes us go with the option:
> There should be no space before the path by failing the service.
>
> Kindly let me know if I have missed anything.
>
> @Dave: Kindly share your inputs as well.
>
> Regards,
> Bhupesh