Resolves: BZ1484945 https://bugzilla.redhat.com/show_bug.cgi?id=1484945
Currently the kdumpctl script doesn't handle whitespaces (including TABs) which might be there before an option name in the kdump.conf
This patch addresses this issue, by ensuring that the kdumpctl errors out in case it finds any stray space(s) or tab(s) before a option name.
Reported-by: Kenneth D'souza kdsouza@redhat.com Signed-off-by: Bhupesh Sharma bhsharma@redhat.com --- kdumpctl | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/kdumpctl b/kdumpctl index 7d06efe292aa..56975a3ba1dc 100755 --- a/kdumpctl +++ b/kdumpctl @@ -351,6 +351,8 @@ restore_default_initrd() check_config() { local nr + TMP_FILE='/tmp/kdump.conf.tmp' + touch $TMP_FILE
nr=$(awk 'BEGIN{cnt=0} /^raw|^ssh[[:blank:]]|^nfs|^ext[234]|^xfs|^btrfs|^minix|^dracut_args .*--mount/{cnt++} END{print cnt}' $KDUMP_CONFIG_FILE) [ $nr -gt 1 ] && { @@ -364,6 +366,20 @@ check_config() return 1 }
+ # 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 + } + + rm -f $TMP_FILE + while read config_opt config_val; do case "$config_opt" in #* | "")
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_FILEreturn 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:]]"
Thanks Baoquan
- }
- rm -f $TMP_FILE
- while read config_opt config_val; do case "$config_opt" in #* | "")
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
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_FILEreturn 1How 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?
Thanks Baoquan
- }
- rm -f $TMP_FILE
- while read config_opt config_val; do case "$config_opt" in #* | "")
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Hi Bao,
Thanks for the review.
On Mon, Sep 18, 2017 at 6:25 AM, Baoquan He bhe@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_FILEreturn 1How 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.
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
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@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_FILEreturn 1How 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
On Mon, Sep 18, 2017 at 12:27 PM, Baoquan He bhe@redhat.com wrote:
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@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_FILEreturn 1How 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.
Yes, we discussed the above and agreed on the former.
However, I am not too specific about any of the same and would need inputs from Dave and others also to decide how to go about the same.
Regarding grep v/s awk, I think we can chose either one of the same. v2 used grep but did not handle the space before a comment use case and hence I decided to use the awk in v3 as it is more compact for this particular search.
Regards, Bhupesh
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
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_FILEreturn 1How 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
Thanks Baoquan
- }
- rm -f $TMP_FILE
- while read config_opt config_val; do case "$config_opt" in #* | "")
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
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_FILEreturn 1How 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
should be `egrep -q` so that we can get the return value but no output..
echo "No whitespaces are allowed before a kdump option name." return 1fi
Thanks Baoquan
- }
- rm -f $TMP_FILE
- while read config_opt config_val; do case "$config_opt" in #* | "")
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
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_FILEreturn 1How 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.
Thanks Baoquan
- }
- rm -f $TMP_FILE
- while read config_opt config_val; do case "$config_opt" in #* | "")
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
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_FILEreturn 1How 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.
Thanks Baoquan
- }
- rm -f $TMP_FILE
- while read config_opt config_val; do case "$config_opt" in #* | "")
-- 2.7.4 _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
Thanks Dave
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_FILEreturn 1How 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"
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@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
Thanks Dave _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
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@lists.fedoraproject.org > To unsubscribe send an email to kexec-leave@lists.fedoraproject.org _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
Thanks Dave _______________________________________________ kexec mailing list -- kexec@lists.fedoraproject.org To unsubscribe send an email to kexec-leave@lists.fedoraproject.org
Thanks Dave
-
Baoquan He -
Bhupesh Sharma -
Dave Young -
Xunlei Pang