[Patch] Rawhide - Allow inline comments in config file.

[Wade Mealing]

The RHEL 5 release of mkdumprd allowed for comments in the kdump config file 
as shown below:

net 192.168.1.1 # this is the comment part

The Fedora 18/19/Rawhide release would fail when inline comments were used
at runtime and when building the initrd.

This patch introduces a function which is repeated a few times, but
could be broken out at a later date into its an existing or new shell
library which could be source.

This patch does not introduce any new functionality.
This patch includes the suggestions by David Young to fix my whitespace
errors.

I have not tested this patch as I do not yet have build permissions on koji.

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Wade Mealing]

From: Wade Mealing

The RHEL 5 release of mkdumprd allowed for comments in the kdump config
file as shown below:

net 192.168.1.1 # this is the comment part

This patch strips them out during processing, but leaves the configuration
file in original condition.
---
 dracut-kdump.sh        |    8 ++++++++
 dracut-module-setup.sh |    6 ++++++
 kdumpctl               |    9 +++++++++
 mkdumprd               |    6 ++++++
 4 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/dracut-kdump.sh b/dracut-kdump.sh
index 7baa673..fab176a 100755
--- a/dracut-kdump.sh
+++ b/dracut-kdump.sh
@@ -230,6 +230,10 @@ get_host_ip()
     return 0
 }

+strip_comments() {
+    echo $1 | sed -e 's/(.*)#.*/\1/'
+}
+
 read_kdump_conf()
 {
     if [ ! -f "$conf_file" ]; then
@@ -240,6 +244,8 @@ read_kdump_conf()
     # first get the necessary variables
     while read config_opt config_val;
     do
+        # remove inline comments after the end of a directive.
+        config_val=$(strip_comments $config_val)
         case "$config_opt" in
         path)
         KDUMP_PATH="$config_val"
@@ -283,6 +289,8 @@ read_kdump_conf()
     # rescan for add code for dump target
     while read config_opt config_val;
     do
+        # remove inline comments after the end of a directive.
+        config_val=$(strip_comments $config_val)
         case "$config_opt" in
         ext[234]|xfs|btrfs|minix|nfs)
             add_dump_code "dump_fs $config_val"
diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
index 099c03a..cbf2912 100755
--- a/dracut-module-setup.sh
+++ b/dracut-module-setup.sh
@@ -62,6 +62,10 @@ kdump_is_vlan() {
      [ -f /proc/net/vlan/"$1" ]
 }

+strip_comments() {
+    echo $1 | sed -e 's/(.*)#.*/\1/'
+}        
+
 # $1: netdev name
 kdump_setup_dns() {
     _dnsfile=${initdir}/etc/cmdline.d/42dns.conf
@@ -252,6 +256,8 @@ kdump_install_conf() {

while read config_opt config_val;
     do
+        # remove inline comments after the end of a directive.
+        config_val=$(strip_comments $config_val)
         case "$config_opt" in
         ext[234]|xfs|btrfs|minix|raw)
             sed -i -e "s#$config_val#$(kdump_to_udev_name $config_val)#" /tmp/$$-kdump.conf
diff --git a/kdumpctl b/kdumpctl
index aba1e3c..e2c4960 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -78,6 +78,10 @@ function check_exist()
    done
 }

+strip_comments() {
+    echo $1 | sed -e 's/(.*)#.*/\1/'
+}
+
 #$1: the files to be checked with IFS=' '
 function check_executable()
 {
@@ -100,6 +104,8 @@ function check_config()
    }

while read config_opt config_val; do
+		# remove inline comments after the end of a directive.
+		config_val=$(strip_comments $config_val)
    	case "$config_opt" in
    	#* | "")
    		;;
@@ -257,6 +263,9 @@ function load_kdump()
 function check_ssh_config()
 {
    while read config_opt config_val; do
+		# remove inline comments after the end of a directive.
+		config_val=$(strip_comments $config_val)	
+
    	case "$config_opt" in
    	sshkey)
    		if [ -f "$config_val" ]; then
diff --git a/mkdumprd b/mkdumprd
index 26fe5af..57217d1 100644
--- a/mkdumprd
+++ b/mkdumprd
@@ -518,8 +518,14 @@ if [ "$(uname -m)" = "s390x" ]; then
     add_dracut_module "znet"
 fi

+strip_comments() {
+    echo $1 | sed -e 's/(.*)#.*/\1/'
+}
+
 while read config_opt config_val;
 do
+    # remove inline comments after the end of a directive.
+    config_val=$(strip_comments $config_val)
     case "$config_opt" in
     extra_modules)
         extra_modules="$extra_modules $config_val"

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Dave Young]

On 08/20/2013 09:33 AM, [e-mail] wrote:
> From: Wade Mealing
>
> The RHEL 5 release of mkdumprd allowed for comments in the kdump config
> file as shown below:
>
> net 192.168.1.1 # this is the comment part
>
> This patch strips them out during processing, but leaves the configuration
> file in original condition.

Missing sob line (Signed-off-by) though this is not strictly necessary
in kdump. Also there's still 2 trailing white space. It's more of style
issue.

For checking patch coding style you can use the kernel scripts of below:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/scripts...

Download the script and run it as below:
#checkpatch.pl --notree xxx.patch

Otherwise the patch itself is ok to me, assume you have tested it.

Care to resend with simple fix for the style issue?
> dracut-kdump.sh        |    8 ++++++++
>  dracut-module-setup.sh |    6 ++++++
>  kdumpctl               |    9 +++++++++
>  mkdumprd               |    6 ++++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/dracut-kdump.sh b/dracut-kdump.sh
> index 7baa673..fab176a 100755
> --- a/dracut-kdump.sh
> +++ b/dracut-kdump.sh
> @@ -230,6 +230,10 @@ get_host_ip()
>      return 0
>  }
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> read_kdump_conf()
>  {
>      if [ ! -f "$conf_file" ]; then
> @@ -240,6 +244,8 @@ read_kdump_conf()
>      # first get the necessary variables
>      while read config_opt config_val;
>      do
>    # remove inline comments after the end of a directive.
>    config_val=$(strip_comments $config_val)
>    case "$config_opt" in
>    path)
>    KDUMP_PATH="$config_val"
>
> @@ -283,6 +289,8 @@ read_kdump_conf()
>      # rescan for add code for dump target
>      while read config_opt config_val;
>      do
>    # remove inline comments after the end of a directive.
>    config_val=$(strip_comments $config_val)
>    case "$config_opt" in
>    ext[234]|xfs|btrfs|minix|nfs)
>        add_dump_code "dump_fs $config_val"
>
> diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> index 099c03a..cbf2912 100755
> --- a/dracut-module-setup.sh
> +++ b/dracut-module-setup.sh
> @@ -62,6 +62,10 @@ kdump_is_vlan() {
>       [ -f /proc/net/vlan/"$1" ]
>  }
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> # $1: netdev name
>  kdump_setup_dns() {
>      _dnsfile=${initdir}/etc/cmdline.d/42dns.conf
> @@ -252,6 +256,8 @@ kdump_install_conf() {
>  while read config_opt config_val;
>  do
>    # remove inline comments after the end of a directive.
>    config_val=$(strip_comments $config_val)
>    case "$config_opt" in
>    ext[234]|xfs|btrfs|minix|raw)
>        sed -i -e "s#$config_val#$(kdump_to_udev_name $config_val)#" /tmp/$$-kdump.conf
>
> diff --git a/kdumpctl b/kdumpctl
> index aba1e3c..e2c4960 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -78,6 +78,10 @@ function check_exist()
>    done
>  }
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> #$1: the files to be checked with IFS=' '
>  function check_executable()
>  {
> @@ -100,6 +104,8 @@ function check_config()
>    }
>
> while read config_opt config_val; do
> # remove inline comments after the end of a directive.
> config_val=$(strip_comments $config_val)
> case "$config_opt" in
> #* | "")
> 	;;
>
> @@ -257,6 +263,9 @@ function load_kdump()
>  function check_ssh_config()
>  {
>    while read config_opt config_val; do
> # remove inline comments after the end of a directive.
> config_val=$(strip_comments $config_val)	
> case "$config_opt" in
>   sshkey)
>   	if [ -f "$config_val" ]; then
>
> diff --git a/mkdumprd b/mkdumprd
> index 26fe5af..57217d1 100644
> --- a/mkdumprd
> +++ b/mkdumprd
> @@ -518,8 +518,14 @@ if [ "$(uname -m)" = "s390x" ]; then
>      add_dracut_module "znet"
>  fi
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> while read config_opt config_val;
>  do
> # remove inline comments after the end of a directive.
> config_val=$(strip_comments $config_val)
> case "$config_opt" in
> extra_modules)
>     extra_modules="$extra_modules $config_val"

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Wade Mealing]

G'day,

I'm going to have to say that this has fallen so far off my priority list, that I
don't have time for it anymore.  The patch can be taken as is or someone else
can work on it.

I think that there should be a white space removal tool when applying patches to a repository
to prevent this kind of re-work.

Wade Mealing.

----- Original Message -----
From: "Dave Young" 
To: [e-mail]
Cc: [e-mail]
Sent: Wednesday, August 21, 2013 3:42:41 PM
Subject: Re: [PATCH] Strip inline comments from the kdump config file before use as values in shell.

On 08/20/2013 09:33 AM, [e-mail] wrote:
> From: Wade Mealing
>
> The RHEL 5 release of mkdumprd allowed for comments in the kdump config
> file as shown below:
>
> net 192.168.1.1 # this is the comment part
>
> This patch strips them out during processing, but leaves the configuration
> file in original condition.

Missing sob line (Signed-off-by) though this is not strictly necessary
in kdump. Also there's still 2 trailing white space. It's more of style
issue.

For checking patch coding style you can use the kernel scripts of below:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/scripts...

Download the script and run it as below:
#checkpatch.pl --notree xxx.patch

Otherwise the patch itself is ok to me, assume you have tested it.

Care to resend with simple fix for the style issue?
> dracut-kdump.sh        |    8 ++++++++
>  dracut-module-setup.sh |    6 ++++++
>  kdumpctl               |    9 +++++++++
>  mkdumprd               |    6 ++++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/dracut-kdump.sh b/dracut-kdump.sh
> index 7baa673..fab176a 100755
> --- a/dracut-kdump.sh
> +++ b/dracut-kdump.sh
> @@ -230,6 +230,10 @@ get_host_ip()
>      return 0
>  }
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> read_kdump_conf()
>  {
>      if [ ! -f "$conf_file" ]; then
> @@ -240,6 +244,8 @@ read_kdump_conf()
>      # first get the necessary variables
>      while read config_opt config_val;
>      do
>    # remove inline comments after the end of a directive.
>    config_val=$(strip_comments $config_val)
>    case "$config_opt" in
>    path)
>    KDUMP_PATH="$config_val"
>
> @@ -283,6 +289,8 @@ read_kdump_conf()
>      # rescan for add code for dump target
>      while read config_opt config_val;
>      do
>    # remove inline comments after the end of a directive.
>    config_val=$(strip_comments $config_val)
>    case "$config_opt" in
>    ext[234]|xfs|btrfs|minix|nfs)
>        add_dump_code "dump_fs $config_val"
>
> diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> index 099c03a..cbf2912 100755
> --- a/dracut-module-setup.sh
> +++ b/dracut-module-setup.sh
> @@ -62,6 +62,10 @@ kdump_is_vlan() {
>       [ -f /proc/net/vlan/"$1" ]
>  }
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> # $1: netdev name
>  kdump_setup_dns() {
>      _dnsfile=${initdir}/etc/cmdline.d/42dns.conf
> @@ -252,6 +256,8 @@ kdump_install_conf() {
>  while read config_opt config_val;
>  do
>    # remove inline comments after the end of a directive.
>    config_val=$(strip_comments $config_val)
>    case "$config_opt" in
>    ext[234]|xfs|btrfs|minix|raw)
>        sed -i -e "s#$config_val#$(kdump_to_udev_name $config_val)#" /tmp/$$-kdump.conf
>
> diff --git a/kdumpctl b/kdumpctl
> index aba1e3c..e2c4960 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -78,6 +78,10 @@ function check_exist()
>    done
>  }
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> #$1: the files to be checked with IFS=' '
>  function check_executable()
>  {
> @@ -100,6 +104,8 @@ function check_config()
>    }
>
> while read config_opt config_val; do
> # remove inline comments after the end of a directive.
> config_val=$(strip_comments $config_val)
> case "$config_opt" in
> #* | "")
> 	;;
>
> @@ -257,6 +263,9 @@ function load_kdump()
>  function check_ssh_config()
>  {
>    while read config_opt config_val; do
> # remove inline comments after the end of a directive.
> config_val=$(strip_comments $config_val)	
> case "$config_opt" in
>   sshkey)
>   	if [ -f "$config_val" ]; then
>
> diff --git a/mkdumprd b/mkdumprd
> index 26fe5af..57217d1 100644
> --- a/mkdumprd
> +++ b/mkdumprd
> @@ -518,8 +518,14 @@ if [ "$(uname -m)" = "s390x" ]; then
>      add_dracut_module "znet"
>  fi
>
> +strip_comments() {
> echo $1 | sed -e 's/(.*)#.*/\1/'
>
> +}
>
> while read config_opt config_val;
>  do
> # remove inline comments after the end of a directive.
> config_val=$(strip_comments $config_val)
> case "$config_opt" in
> extra_modules)
>     extra_modules="$extra_modules $config_val"

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Baoquan He]

Hi Wade,

Thanks for your patch. I know high priority tasks need be handled
firstly.

Don't be discouraged. I will merge it with tiny adjusting.

Baoquan
Thanks a lot

On 08/21/13 at 07:39pm, Wade Mealing wrote:
> G'day,
>
> I'm going to have to say that this has fallen so far off my priority list, that I
> don't have time for it anymore.  The patch can be taken as is or someone else
> can work on it.
>
> I think that there should be a white space removal tool when applying patches to a repository
> to prevent this kind of re-work.
>
> Wade Mealing.

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Vivek Goyal]

On Thu, Aug 22, 2013 at 09:06:32AM +0800, Baoquan He wrote:
> Hi Wade,
>
> Thanks for your patch. I know high priority tasks need be handled
> firstly.
>
> Don't be discouraged. I will merge it with tiny adjusting.

Bao,

This path define strip_xyz() 2 times in two separate files. I think it
is high time we start thinking of intorducing a shared functions file
for kdump which various kdump scripts can share.

Adding duplicate code is bad.

Chao, you were planning to look into this. Did you make any progress?

Thanks
Vivek

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[WANG Chao]

On 08/30/13 at 01:20pm, Vivek Goyal wrote:
> On Thu, Aug 22, 2013 at 09:06:32AM +0800, Baoquan He wrote:
> > Hi Wade,
> >
> > Thanks for your patch. I know high priority tasks need be handled
> > firstly.
> >
> > Don't be discouraged. I will merge it with tiny adjusting.
>
> Bao,
>
> This path define strip_xyz() 2 times in two separate files. I think it
> is high time we start thinking of intorducing a shared functions file
> for kdump which various kdump scripts can share.
>
> Adding duplicate code is bad.
>
> Chao, you were planning to look into this. Did you make any progress?

Last time I checked, I could only extract about 6 or 7 common functions
without doing major change crossing the kdump source code.

It looks like we do have common functions used by multiple file. But
that's quite a small group. I don't know if it's worth it to have
minority in a shared lib file and leave the majority still. What do you
think?

Thanks
WANG Chao

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Vivek Goyal]

On Mon, Sep 02, 2013 at 03:44:15PM +0800, WANG Chao wrote:
> On 08/30/13 at 01:20pm, Vivek Goyal wrote:
> > On Thu, Aug 22, 2013 at 09:06:32AM +0800, Baoquan He wrote:
> > > Hi Wade,
> > >
> > > Thanks for your patch. I know high priority tasks need be handled
> > > firstly.
> > >
> > > Don't be discouraged. I will merge it with tiny adjusting.
> >
> > Bao,
> >
> > This path define strip_xyz() 2 times in two separate files. I think it
> > is high time we start thinking of intorducing a shared functions file
> > for kdump which various kdump scripts can share.
> >
> > Adding duplicate code is bad.
> >
> > Chao, you were planning to look into this. Did you make any progress?
>
> Last time I checked, I could only extract about 6 or 7 common functions
> without doing major change crossing the kdump source code.

I think that's a good start to create a file for shared functions.
> It looks like we do have common functions used by multiple file.

Where is that. We don't seem to have any file with common functions
which is included by others?
> But
> that's quite a small group. I don't know if it's worth it to have
> minority in a shared lib file and leave the majority still. What do you
> think?

I still think that we need to introduce a common function file. We might
start small but once we have a place holder, future common functions will
go there.

Only thing we need to make sure is that same functions should be usable
in both dracut context as well as mkdumprd context.

Thanks
Vivek

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[WANG Chao]

On 09/23/13 at 02:59pm, Vivek Goyal wrote:
> On Mon, Sep 02, 2013 at 03:44:15PM +0800, WANG Chao wrote:
> > On 08/30/13 at 01:20pm, Vivek Goyal wrote:
> > > On Thu, Aug 22, 2013 at 09:06:32AM +0800, Baoquan He wrote:
> > > > Hi Wade,
> > > >
> > > > Thanks for your patch. I know high priority tasks need be handled
> > > > firstly.
> > > >
> > > > Don't be discouraged. I will merge it with tiny adjusting.
> > >
> > > Bao,
> > >
> > > This path define strip_xyz() 2 times in two separate files. I think it
> > > is high time we start thinking of intorducing a shared functions file
> > > for kdump which various kdump scripts can share.
> > >
> > > Adding duplicate code is bad.
> > >
> > > Chao, you were planning to look into this. Did you make any progress?
> >
> > Last time I checked, I could only extract about 6 or 7 common functions
> > without doing major change crossing the kdump source code.
>
> I think that's a good start to create a file for shared functions.

Yeah, sure.
> > It looks like we do have common functions used by multiple file.
>
> Where is that. We don't seem to have any file with common functions
> which is included by others?

We don't have any file to share common functions.
> > But
> > that's quite a small group. I don't know if it's worth it to have
> > minority in a shared lib file and leave the majority still. What do you
> > think?
>
> I still think that we need to introduce a common function file. We might
> start small but once we have a place holder, future common functions will
> go there.
>
> Only thing we need to make sure is that same functions should be usable
> in both dracut context as well as mkdumprd context.

We might also need it in 2nd kernel.

I'll start working on this.

Thanks
WANG Chao

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Baoquan He]

On 08/30/13 at 01:20pm, Vivek Goyal wrote:
> On Thu, Aug 22, 2013 at 09:06:32AM +0800, Baoquan He wrote:
> > Hi Wade,
> >
> > Thanks for your patch. I know high priority tasks need be handled
> > firstly.
> >
> > Don't be discouraged. I will merge it with tiny adjusting.
>
> Bao,
>
> This path define strip_xyz() 2 times in two separate files. I think it
> is high time we start thinking of intorducing a shared functions file
> for kdump which various kdump scripts can share.
>
> Adding duplicate code is bad.
>
> Chao, you were planning to look into this. Did you make any progress?

Hi,

Since Chao has taken the task to introducing a shared functions file for
kdump file, I would like to merge this patch directly. Or if you think
it's not good, I can wait until Chao's patch for shared functions file
is OK.

Baoquan
Thanks
> kexec mailing list
> [e-mail]
> https://lists.fedoraproject.org/mailman/listinfo/kexec

[PATCH] Strip inline comments from the kdump config file before use as values in shell.

[Vivek Goyal]

On Tue, Sep 10, 2013 at 11:13:00AM +0800, Baoquan He wrote:
> On 08/30/13 at 01:20pm, Vivek Goyal wrote:
> > On Thu, Aug 22, 2013 at 09:06:32AM +0800, Baoquan He wrote:
> > > Hi Wade,
> > >
> > > Thanks for your patch. I know high priority tasks need be handled
> > > firstly.
> > >
> > > Don't be discouraged. I will merge it with tiny adjusting.
> >
> > Bao,
> >
> > This path define strip_xyz() 2 times in two separate files. I think it
> > is high time we start thinking of intorducing a shared functions file
> > for kdump which various kdump scripts can share.
> >
> > Adding duplicate code is bad.
> >
> > Chao, you were planning to look into this. Did you make any progress?
>
> Hi,
>
> Since Chao has taken the task to introducing a shared functions file for
> kdump file, I would like to merge this patch directly. Or if you think
> it's not good, I can wait until Chao's patch for shared functions file
> is OK.

We have let it go multiple times. I don't like the idea of duplicating
functions any more.

So somebody please create a common functions file and put strip_xyz()
in that. Let us not create common instance of that function twice.

Thanks
Vivek

[Dave Young]

For update version of patch, usually we will add the version number
in subject like:

[patch v2] Allow inline comments in config file

If there's just one patch then you need not send a cover letter. I do
not use git-send-email, but I believe there should be an option to send
patch only without cover letter.

On 08/20/2013 09:33 AM, [e-mail] wrote:
> The RHEL 5 release of mkdumprd allowed for comments in the kdump config file 
> as shown below:
>
> net 192.168.1.1 # this is the comment part
>
> The Fedora 18/19/Rawhide release would fail when inline comments were used
> at runtime and when building the initrd.
>
> This patch introduces a function which is repeated a few times, but
> could be broken out at a later date into its an existing or new shell
> library which could be source.
>
> This patch does not introduce any new functionality.
> This patch includes the suggestions by David Young to fix my whitespace
> errors.
>
> I have not tested this patch as I do not yet have build permissions on koji.
> _______________________________________________
> kexec mailing list
> [e-mail]
> https://lists.fedoraproject.org/mailman/listinfo/kexec