On 05/06/16 at 01:52pm, Baoquan He wrote:
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.
~~type, should be
'or'
>
> >
> > ~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
> > > >