Hi, Pratyush
On 04/20/16 at 08:52am, Pratyush Anand wrote:
Hi Dave,
Thanks for your feedback.
On 20/04/2016:10:11:51 AM, Dave Young wrote:
> On 04/18/16 at 03:23pm, Pratyush Anand wrote:
> > There could be some dynamic system modification, which may affect kdump
> > kernel boot process. In such situation initramfs must be rebulit on the
> > basis of changes.
> > Since most of these checking methods will use information from
> > TARGET_INITRD, therefore check for its existence in common code.
> >
> > Signed-off-by: Pratyush Anand <panand(a)redhat.com>
> > ---
> > kdumpctl | 21 +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index ef18a2d4f6ce..64dc92da605b 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -327,10 +327,21 @@ setup_target_initrd()
> > fi
> > }
> >
> > +# returns 0 if system is not modified
> > +# returns 1 if system is modified
> > +# returns 2 if system modification is invalid
> > +is_system_modified()
> > +{
> > + [[ -f $TARGET_INITRD ]] || return 1
>
> Since earlier code will check image_time -eq 0 so it will never return 1
> How about add replace it with a comment about we do not worry about
> initrd existance since we will check image_time -eq 0 in other place?
is_system_modified() is called before checking of "image_time -eq 0". Since,
most of the sub-functions called from is_system_modified() needs $TARGET_INITRD,
so I thought to check it in common path.
Ok, I do not have strong opinion about this.
>
> > +
> > + return 0
> > +}
> > +
> > check_rebuild()
> > {
> > local extra_modules modified_files=""
> > local _force_rebuild force_rebuild="0"
> > + local ret system_modified="0"
> > local initramfs_has_fadump
> >
> > check_boot_dir
> > @@ -388,6 +399,14 @@ check_rebuild()
> > fi
> > done
> >
> > + is_system_modified
> > + ret=$?
> > + if [ "$ret" -eq 2 ]; then
> > + return 1
> > + elif [ "$ret" -eq 1 ];then
>
> Nitpick: not necessary to quote "$ret"
OK, will correct.
>
> > + system_modified="1"
> > + fi
> > +
> > #check if target initrd has fadump support
> > if [ "$DEFAULT_DUMP_MODE" = "fadump" ] && [ -f
"$TARGET_INITRD" ]; then
> > initramfs_has_fadump=`lsinitrd -m $TARGET_INITRD | grep ^kdumpbase$ | wc
-l`
> > @@ -399,6 +418,8 @@ check_rebuild()
> > echo "$TARGET_INITRD has no fadump support"
> > elif [ "$force_rebuild" != "0" ]; then
> > echo -n "Force rebuild $TARGET_INITRD"; echo
> > + elif [ "$system_modified" != "0" ]; then
> > + echo -n "System has been modified"; echo
>
> A concern about the message is if we move modified files to is_system_modified
> in later patch for those cases there will print more messages than before.
> Ideally only below "Dected change(s) in the following file(s):" should be
> better.
>
> Maybe for system_modified we can differenciate the cases by more values?
> ie. -1 means error, 1 means files being modified, 1 means fs uuid being modified
> etc. then print different messages?
I think, we already have relevant debug messages coming from the different
functions called by is_system_modified(), so probably we can skip any message
printing here.
So may in stead of last "else return 0" we can have "elif [
"$system_modified"
= "0" ]; then return 0". What do you say?
It is fine to print in different functions to me. how about keep the
original else return 0, just add a nul command:
elif [ "$system_modified" != "0" ]; then
:
else
return 0
fi
Thanks
Dave