On 03/28/2017 at 03:10 PM, Pratyush Anand wrote:
On Tuesday 28 March 2017 11:43 AM, 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.
OK..I read last patch 6/6 first and then backward, and its a bit clear now.
>> 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()
This hook seems to update inventory. It would be good to mention that in comment
*atleast*.
>> +{
>> + 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
Humm..not sure if sharing info between functions by exporting global names is a good
idea...
At the beginning, I used a tmp file to store all the results. Then I found in mkdumprd
there has
"export IN_KDUMP=1" and then use it in 99kdump module-setup.sh, that's why I
changed to
use this way. At least It has one advantage that we don't need to worry about the
problem
of informatin gets removed during kdump starting when using tmp file.
Regards,
Xunlei
>> +}
>> +
>> +# 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]"
>>