On 03/28/2017 at 02:13 PM, Pratyush Anand wrote:
Hi Xunlei,
On Monday 27 March 2017 08:12 AM, Xunlei Pang wrote:
> We add a generic hook named kdump_target_hook() for the dump
> target, this is useful for us to do some extra work for the
> dump targets.
>
I have not seen the next patches so far, however I tried to read this patch again and
again but I could not understand why we are doing all these, what problem we are trying to
address. Moreover, patch is not just adding a generic hook, it is modifying existing
functionality of check_files_modified(). IMO, it would again be good to divide it in
multiple patches, with clear explanation that why do we do that. Even cover letter also
does not talk about overall what we try to achieve with the series.
Thanks for your review.
I should explain more...
Besides the dump target(the device where the vmcore will be stored), we actually may have
other targets(devices) including
root device if dump target is non-root, also on rhel7 with "--hostonly-cmdline",
dracut will generate more devices for kdump.
If we want to do something according to the information related to kdump targets(not only
the dump target), for example
from the very beginning I wanted to 1) modify /etc/lvm/lvm.conf if all the devices under
kdump are linear-type lvm; 2) avoid
mounting the root filesystem of the 1st kernel if the dump target is non-root(e.g. network
dumping). 3) remove "lvm" modules
completely if there is no lvm device at all, 4) perhaps we will have more such work in the
future...
So that's the background I introduced this patch, firstly there is a list of job we
want to do, for example for 1) mentioned above,
the job will be "are all the targets under kdump lvm linear-type if any?", so it
can be abstracted to be a check item about the targets
hooked by us, and return the true or not result; then in somewhere we can get the result
and modify /etc/lvm/lvm.conf it true.
So the check job will in the checklist, then we add all the handling logic uniformly at
the hook function, after that we can get the result
of each job simply by invoking kdump_targets_tell "job_name".
That's all the background.
Regards,
Xunlei
~Pratyush
> kdump_targets_tell() is exported as a generic helper that can
> be used as some specific purpose.
>
> To implement a new checklist:
> Add your new checklist(job) "xxx" separated by whitespace, and
> implement its logic to update its tag value in the tag inventory.
> Then "kdump_targets_tell xxx" will be available for you to use.
>
> We will use this mechanism in the following patches.
>
> Signed-off-by: Xunlei Pang <xlpang(a)redhat.com>
> ---
> kdump-lib.sh | 28 +++++++++++++++
> kdumpctl | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 144 insertions(+)
>
> diff --git a/kdump-lib.sh b/kdump-lib.sh
> index ae54b7f..d3c0d7b 100755
> --- a/kdump-lib.sh
> +++ b/kdump-lib.sh
> @@ -434,3 +434,31 @@ get_dracut_args_target()
> {
> echo $1 | grep "\-\-mount" | sed "s/.*--mount .\(.*\)/\1/" |
cut -d' ' -f1
> }
> +
> +# $1: item
> +# Return the item tag value in the inventory.
> +kdump_target_get_inventory()
> +{
> + local item=$1 tag
> +
> + tag=$(eval echo '${KDUMP_TARGET_TAG_'${item}'}')
> + echo $tag
> +}
> +
> +# $1: item
> +kdump_targets_tell()
> +{
> + local item=$1 tagvalue
> +
> + tagvalue=$(kdump_target_get_inventory $item)
> +
> + if [ "$tagvalue" = "1" ]; then
> + return 0
> + elif [ "$tagvalue" = "0" ]; then
> + return 1
> + else
> + echo "Warning: wrong tag value for \"kdump_target_is
$item\""
> + fi
> +
> + return 1
> +}
> diff --git a/kdumpctl b/kdumpctl
> index d8f049f..ca97580 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -23,6 +23,12 @@ image_time=0
> . $dracutbasedir/dracut-functions.sh
> . /lib/kdump/kdump-lib.sh
>
> +# Add your new checklist(job) "xxx" separated by whitespace, and
> +# implement its logic to update its tag value in the tag inventory.
> +# Then "kdump_targets_tell xxx" will be available for you to use.
> +# See kdump_target_handle_item() for examples.
> +KDUMP_TARGET_CHECKLIST=""
> +
> standard_kexec_args="-p"
>
> # Some default values in case /etc/sysconfig/kdump doesn't include
> @@ -435,6 +441,114 @@ setup_initrd()
> fi
> }
>
> +kdump_target_inventory_init()
> +{
> + kdump_target_checklist kdump_target_init_item
> +}
> +
> +# Three values(0, 1, 2) are allowed in the tag inventory,
> +# basically each value has the following meaning:
> +# 0 - "surely no", 1 - "surely yes", 2 - "not sure"
> +# Each item maintains its own way to update the value.
> +#
> +# Update the item tag value to 2
> +# $1: item
> +kdump_target_init_item()
> +{
> + kdump_target_update_inventory $1 2
> +}
> +
> +# $1: ssh, nfs, or block dev name
> +kdump_target_hook()
> +{
> + local dev=$1
> +
> + if [ -b "$dev" ]; then
> + dev=$(get_persistent_dev $dev)
> + fi
> +
> + kdump_target_checklist kdump_target_handle_item $dev
> +}
> +
> +# $1: function to be executed
> +# $2,..: arguments of $1
> +kdump_target_checklist()
> +{
> + local item func
> +
> + func=$1
> + shift
> + for item in $KDUMP_TARGET_CHECKLIST; do
> + "$func" $item $@
> + done
> +}
> +
> +# $1: the item to be checked
> +# $2: persistent block dev name, nfs, or ssh
> +#
> +# You can add an item(job) in KDUMP_TARGET_CHECKLIST, implement its
> +# own logic on how to update its tag value in the tag inventory.
> +kdump_target_handle_item()
> +{
> + local item=$1 dev=$2
> + local value
> +
> + value=$(kdump_target_get_inventory $item)
> +
> + # The item handling begins
> + case "$item" in
> + *)
> + echo "unhandled item in kdump target checklist"
> + return
> + esac
> +
> + kdump_target_update_inventory $item $value
> +}
> +
> +# Update the item in the tag inventory.
> +# $1: item
> +# $2: value
> +kdump_target_update_inventory()
> +{
> + local tag
> +
> + tag=KDUMP_TARGET_TAG_$1
> + eval export $tag=$2
> +}
> +
> +# Currently, we add no more than two target hooks
> +# 1) cases that only one root target
> +# 2) cases that non-root target and the root target
> +check_kdump_targets()
> +{
> + local _target _root
> +
> + kdump_target_inventory_init
> +
> + _target=$(get_block_dump_target)
> + if [ -n "$_target" ]; then
> + kdump_target_hook $_target
> + elif is_ssh_dump_target; then
> + kdump_target_hook "ssh"
> + else
> + kdump_target_hook "nfs"
> + fi
> +
> + # Add the root device if needed, currently kdump always mounts the root
> + _root=$(get_root_fs_device)
> + if [[ -b "$_root" && "$_root" !=
"$_target" ]]; then
> + kdump_target_hook $_root
> + fi
> +
> + # NOTE:
> + # dracut parses devices from "/etc/fstab" with the
"x-initrd.mount" option,
> + # which will be added as host_devs, it also includes usually simple devices
> + # (say mounted to /boot, /boot/efi/, etc) plus the root device. Then kdump
> + # must wait for these devices if initramfs is built with
"--hostonly-cmdline".
> + #
> + # Fedora doesn't have "--hostonly-cmdline" specified, so
there's no problem.
> +}
> +
> check_files_modified()
> {
> local modified_files=""
> @@ -1144,6 +1258,8 @@ start()
> fi
> fi
>
> + check_kdump_targets
> +
> check_rebuild
> if [ $? != 0 ]; then
> echo "Starting kdump: [FAILED]"
>