[PATCH v2] mkdumprd: Use DUMP_TARGET which printing error message during ssh

[Bhupesh Sharma]

When building kdump initramfs for a SSH dump target, mkdumprd would
check whether it has the write permission on the SSH Server's
$DUMP_TARGET.

However $DUMP_TARGET is missing in the actual error message when the
user doesn't not have the write permission. For example:

 # kdumpctl restart
   kexec: unloaded kdump kernel
   Stopping kdump: [OK]
   Could not create temporary directory on :/home/bhsharma/test. Make
   sure user has write permission on destination
   mkdumprd: failed to make kdump initrd
   Starting kdump: [FAILED]

This patch using $1 value passed to mkdumprd, to print the
$DUMP_TARGET inside mkdir_save_path_ssh() function to
fix the issue.

Signed-off-by: Bhupesh Sharma <bhsharma(a)redhat.com&gt;
---
 mkdumprd | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mkdumprd b/mkdumprd
index 6c33fc565579..9599b338e858 100644
--- a/mkdumprd
+++ b/mkdumprd
@@ -130,7 +130,7 @@ get_ssh_size() {
 
 #mkdir if save path does not exist on ssh dump target
 #$1=ssh dump target
-#caller should ensure write permission on $DUMP_TARGET:$SAVE_PATH
+#caller should ensure write permission on $1:$SAVE_PATH
 #called from while loop and shouldn't read from stdin, so we're using "ssh
-n"
 mkdir_save_path_ssh()
 {
@@ -139,14 +139,14 @@ mkdir_save_path_ssh()
     ssh -qn $_opt $1 mkdir -p $SAVE_PATH 2>&1 > /dev/null
     _ret=$?
     if [ $_ret -ne 0 ]; then
-        perror_exit "mkdir failed on $DUMP_TARGET:$SAVE_PATH"
+        perror_exit "mkdir failed on $1:$SAVE_PATH"
     fi
 
-    #check whether user has write permission on $SAVE_PATH/$DUMP_TARGET
+    #check whether user has write permission on $SAVE_PATH/$1
     _dir=$(ssh -qn $_opt $1 mktemp -dqp $SAVE_PATH 2>/dev/null)
     _ret=$?
     if [ $_ret -ne 0 ]; then
-        perror_exit "Could not create temporary directory on
$DUMP_TARGET:$SAVE_PATH. Make sure user has write permission on destination"
+        perror_exit "Could not create temporary directory on $1:$SAVE_PATH. Make
sure user has write permission on destination"
     fi
     ssh -qn $_opt $1 rmdir $_dir
 
-- 
2.7.4

[Kairui Song]

On Mon, Mar 9, 2020 at 8:39 PM Bhupesh Sharma <bhsharma(a)redhat.com&gt; wrote:
>  When building kdump initramfs for a SSH dump target, mkdumprd would
>  check whether it has the write permission on the SSH Server's
>  $DUMP_TARGET.
>
>  However $DUMP_TARGET is missing in the actual error message when the
>  user doesn't not have the write permission. For example:
>
>   # kdumpctl restart
>     kexec: unloaded kdump kernel
>     Stopping kdump: [OK]
>     Could not create temporary directory on :/home/bhsharma/test. Make
>     sure user has write permission on destination
>     mkdumprd: failed to make kdump initrd
>     Starting kdump: [FAILED]
>
>  This patch using $1 value passed to mkdumprd, to print the
>  $DUMP_TARGET inside mkdir_save_path_ssh() function to
>  fix the issue.
>
>  Signed-off-by: Bhupesh Sharma <bhsharma(a)redhat.com&gt;
>  ---
>   mkdumprd | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
>  diff --git a/mkdumprd b/mkdumprd
>  index 6c33fc565579..9599b338e858 100644
>  --- a/mkdumprd
>  +++ b/mkdumprd
>  @@ -130,7 +130,7 @@ get_ssh_size() {
>
>   #mkdir if save path does not exist on ssh dump target
>   #$1=ssh dump target
>  -#caller should ensure write permission on $DUMP_TARGET:$SAVE_PATH
>  +#caller should ensure write permission on $1:$SAVE_PATH
>   #called from while loop and shouldn't read from stdin, so we're using "ssh
> -n"
>   mkdir_save_path_ssh()
>   {
>  @@ -139,14 +139,14 @@ mkdir_save_path_ssh()
>       ssh -qn $_opt $1 mkdir -p $SAVE_PATH 2>&1 > /dev/null
>       _ret=$?
>       if [ $_ret -ne 0 ]; then
>  -        perror_exit "mkdir failed on $DUMP_TARGET:$SAVE_PATH"
>  +        perror_exit "mkdir failed on $1:$SAVE_PATH"
>       fi
>
>  -    #check whether user has write permission on $SAVE_PATH/$DUMP_TARGET
>  +    #check whether user has write permission on $SAVE_PATH/$1 
Maybe the comment here should be:
#check whether user has write permission on $1:$SAVE_PATH

>       _dir=$(ssh -qn $_opt $1 mktemp -dqp $SAVE_PATH 2>/dev/null)
>       _ret=$?
>       if [ $_ret -ne 0 ]; then
>  -        perror_exit "Could not create temporary directory on
> $DUMP_TARGET:$SAVE_PATH. Make sure user has write permission on destination"
>  +        perror_exit "Could not create temporary directory on $1:$SAVE_PATH. Make
> sure user has write permission on destination"
>       fi
>       ssh -qn $_opt $1 rmdir $_dir
>
>  --
>  2.7.4
>  
Thanks for the update, it looks good to me, if you agree with my reply
I can just fix the comment problem when merging it.

-- 
Best Regards,
Kairui Song

[Bhupesh Sharma]

Hi Kairui,

On Fri, Mar 13, 2020 at 2:04 PM Kairui Song <kasong(a)redhat.com&gt; wrote:
>  On Mon, Mar 9, 2020 at 8:39 PM Bhupesh Sharma <bhsharma(a)redhat.com&gt; wrote:
>  >
>  > When building kdump initramfs for a SSH dump target, mkdumprd would
>  > check whether it has the write permission on the SSH Server's
>  > $DUMP_TARGET.
>  >
>  > However $DUMP_TARGET is missing in the actual error message when the
>  > user doesn't not have the write permission. For example:
>  >
>  >  # kdumpctl restart
>  >    kexec: unloaded kdump kernel
>  >    Stopping kdump: [OK]
>  >    Could not create temporary directory on :/home/bhsharma/test. Make
>  >    sure user has write permission on destination
>  >    mkdumprd: failed to make kdump initrd
>  >    Starting kdump: [FAILED]
>  >
>  > This patch using $1 value passed to mkdumprd, to print the
>  > $DUMP_TARGET inside mkdir_save_path_ssh() function to
>  > fix the issue.
>  >
>  > Signed-off-by: Bhupesh Sharma <bhsharma(a)redhat.com&gt;
>  > ---
>  >  mkdumprd | 8 ++++----
>  >  1 file changed, 4 insertions(+), 4 deletions(-)
>  >
>  > diff --git a/mkdumprd b/mkdumprd
>  > index 6c33fc565579..9599b338e858 100644
>  > --- a/mkdumprd
>  > +++ b/mkdumprd
>  > @@ -130,7 +130,7 @@ get_ssh_size() {
>  >
>  >  #mkdir if save path does not exist on ssh dump target
>  >  #$1=ssh dump target
>  > -#caller should ensure write permission on $DUMP_TARGET:$SAVE_PATH
>  > +#caller should ensure write permission on $1:$SAVE_PATH
>  >  #called from while loop and shouldn't read from stdin, so we're using
> "ssh -n"
>  >  mkdir_save_path_ssh()
>  >  {
>  > @@ -139,14 +139,14 @@ mkdir_save_path_ssh()
>  >      ssh -qn $_opt $1 mkdir -p $SAVE_PATH 2>&1 > /dev/null
>  >      _ret=$?
>  >      if [ $_ret -ne 0 ]; then
>  > -        perror_exit "mkdir failed on $DUMP_TARGET:$SAVE_PATH"
>  > +        perror_exit "mkdir failed on $1:$SAVE_PATH"
>  >      fi
>  >
>  > -    #check whether user has write permission on $SAVE_PATH/$DUMP_TARGET
>  > +    #check whether user has write permission on $SAVE_PATH/$1
>
>  Maybe the comment here should be:
>  #check whether user has write permission on $1:$SAVE_PATH 
Ok.

>  >      _dir=$(ssh -qn $_opt $1 mktemp -dqp $SAVE_PATH
> 2>/dev/null)
>  >      _ret=$?
>  >      if [ $_ret -ne 0 ]; then
>  > -        perror_exit "Could not create temporary directory on
> $DUMP_TARGET:$SAVE_PATH. Make sure user has write permission on destination"
>  > +        perror_exit "Could not create temporary directory on $1:$SAVE_PATH.
> Make sure user has write permission on destination"
>  >      fi
>  >      ssh -qn $_opt $1 rmdir $_dir
>  >
>  > --
>  > 2.7.4
>  >
>
>  Thanks for the update, it looks good to me, if you agree with my reply
>  I can just fix the comment problem when merging it. 
Sure, the suggested change looks fine to me.
Please feel free to fix up the comment while merging the change.

Regards,
Bhupesh