On 06/05/2016:01:52:48 PM, 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.
OK.
~Pratyush