On 05/06/16 at 10:07am, Pratyush Anand wrote:
Hi Baoquan,
Thanks for the review.
On 06/05/2016:11:06:08 AM, Baoquan He wrote:
> On 05/05/16 at 05:20pm, 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 8ec6b2de914b..6bd2608343c0 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
>
> Well, we might not need to implement functions like C programming
> language. Seems shell has its own conventions. Like 0 means true, 1
> means false. And for is_system_modified its return value should reflect
> its naming, 0 means it is modified, 1 means it is not modified. If
> invalid, we can warning or error and return directly in the detect
> function. It may not be necessary to use return value 2 here.
Actually this is coming from next patch. is_files_modified() needs 3 states. So
if minimum required files does not exist, or may be some required binary does
not exist then we need to abort rebuild process and kdump service should fail.
So, basically when it returns 2, then we should land into "Starting kdump:
[FAILED]".
Yeah, I was not sure where I should comment, so just put it here.
Usually a function named is_xxx return a vaule with bool type, true or
false. If it's tristate, name might not be good. Now I understand why
you need return value 2, since result of check_exist and
check_executable need be passed out. If it has to how about rename it to
be check_files_modified() or something like that. Because is_xx usually
tell people the result is is isn't something.
>
> ~Pratyush
> >
> > > +is_system_modified()
> > > +{
> > > + [[ -f $TARGET_INITRD ]] || return 1
> > > +
> > > + 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
> > > + 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
> > > + :
> > > elif [ -n "$modified_files" ]; then
> > > echo "Detected change(s) in the following file(s):"
> > > echo -n " "; echo "$modified_files" | sed
's/\s/\n /g'
> > > --
> > > 2.5.5
> > >