On 03/20/14 at 10:14am, Vivek Goyal wrote:
On Thu, Mar 20, 2014 at 01:57:12PM +0800, Baoquan He wrote:
> kdump need create the dir specified in "path" formerly if it does not
> exist. Now change the behavior to be that ueser takes charge of the
> "path", make sure "path" has been created, especially when
separate disk
> is mounted on this "path".
>
> Also introduce 2 helper functions to help check the existence of path.
>
> Signed-off-by: Baoquan He <bhe(a)redhat.com>
> ---
> kdump-lib.sh | 18 ++++++++++++++++++
> mkdumprd | 33 +--------------------------------
> 2 files changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index fdcde83..6803cb6 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -82,3 +82,21 @@ get_mntpoint_from_target()
> echo $(findmnt -k -f -n -r -o TARGET $1)
> }
Thanks for the patches bao. They are almost ready. Some minor nits.
In general, let us put some comments before helper functions. Which
tells user what does a function do, what arguments does it expect and
what's the expected outcome.
You don't have to do it for really simple functions but if something
is little more twisted, we need to do it so that code reuse later is
easier.
>
> +make_save_path_fs()
For example, for this function we need to explain what is "target". Looks
like it could be UUID, LABEL, block device or even nfs server export of
the form of "my.server.com:/tmp/export"?
Given the fact that we are creating an absolute path from dump target
and path variable, may be a better name could be.
get_absolute_save_path().
Yes, will change.
And possibly this could be used for both default case as well as when
dump taret is specified.
When dump target is not specified, then $target would be null.
You mean in this function check target is specified explicitly or is a
default target. If you mean this, it doesn't have to pass target as
argument. Otherwise the target always need be passed in. Because it's
done outside of this fucntion to check whether target is a specified
target or a default target.
Rest of the patch looks good to me.
Thanks
Vivek
> +{
> + local _target=$1
> + local _mnt
> +
> + _mnt=$(get_mntpoint_from_target $1)
> + echo "${_mnt}/$SAVE_PATH"
> +}
> +
> +check_save_path_fs()
> +{
> + local _path=$1
> +
> + if [ ! -d $_path ]; then
> + perror_exit "Dump path $_path does not exist."
> + fi
> +}
> +
> diff --git a/mkdumprd b/mkdumprd
> index 0295009..2bd25ee 100644
> --- a/mkdumprd
> +++ b/mkdumprd
> @@ -173,37 +173,6 @@ mkdir_save_path_ssh()
> return 0
> }
>
> -
> -#mkdir if save path does not exist on dump target filesystem
> -#$1=dump target
> -#caller should ensure $1 is mounted
> -mkdir_save_path_fs() {
> - local _mnt=$(to_mount_point $1)
> - local _remount="no"
> - local _ret
> -
> - [ ! -d ${_mnt}/$SAVE_PATH ] && {
> - if is_readonly_mount $1; then
> - echo "Mounting $1 as read-write for creating dump
directory.."
> - mount -o remount,rw $1 || {
> - perror_exit "Mounting $1 as read-write failed."
> - }
> - _remount="yes"
> - fi
> - mkdir -p ${_mnt}/$SAVE_PATH
> - _ret=$?
> - [ "$_remount" = "yes" ] && {
> - echo "Remounting $1 as read-only."
> - mount -o remount,ro $1 || {
> - perror_exit "Remounting $1 as read-only failed."
> - }
> - }
> - [ $_ret -ne 0 ] && {
> - perror_exit "Creating ${_mnt}/$SAVE_PATH failed."
> - }
> - }
> -}
> -
> #Function: get_fs_size
> #$1=dump target
> get_fs_size() {
> @@ -539,7 +508,7 @@ do
> add_dracut_module "nfs"
> fi
> add_mount "$config_val"
> - mkdir_save_path_fs $config_val
> + check_save_path_fs $(make_save_path_fs $config_val)
> check_size fs $config_val
> ;;
> raw)
> --
> 1.8.5.3
_______________________________________________
kexec mailing list
kexec(a)lists.fedoraproject.org
https://lists.fedoraproject.org/mailman/listinfo/kexec