Re: [LTP] [PATCH v5 1/3] lib: Add support option for .needs_cmds

[Petr Vorel <pvorel@suse.cz>]

> > Hi Wei,

> > ...
> > > > Also, strictly speaking tst_cmd_present() is only defined, not used. The commit
> > > > is about changing .needs_cmds from char array (string) to struct tst_cmd.
> > > > Therefore it could be in a separate commit. We don't have to be too strict, but
> > > > given how many tests needed to be adapted adding tst_cmd_present() is somehow
> > > > buried with other changes.

> > > Thanks all for detail feedback. Let's me give some explaination why skip
> > > tst_brk in above function:
> > > tst_cmd_present will be used in latest ioctl_loop01.c and it should not
> > > use tst_brk otherwise we will brk out of test in setup() too early.
> > > The new support needs_cmds.optional in current patch is used for support 
> > > tst_cmd_present scenario in ioctl_loop01.c.
> > > Correct me if any mistake.

> > Sure, I noticed tst_cmd_present() usage in a later commit :).

> > My point was (while this patchset is also about ideal feature change split into
> > commits) that if you touch many files with struct tst_cmd change, adding
> > unrelated change (tst_cmd_present()) will hide this change. I would personally
> > move adding tst_cmd_present() to a separate commit. It's not that important,
> > just to make review easier.
> Thanks, i get your point now.
> @Petr @Cyril
> If we have are agree with following changes then i can sent next patch:
> 1) Just move the declaration and definition of tst_cmd_present to a separate commit without any functional content changes.
> 2) Modify function comments such as add ref:`...`

+1, thank you!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp