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.

The rules (which I've tried to explain on this patchset) are
* each commit must compile (even temporarily break build is not acceptable)
* split changes into small logical parts

People talk about atomic changes: "As small as possible, but complete" so that
they don't break CI.
https://dev.to/samuelfaure/how-atomic-git-commits-dramatically-increased-my-productivity-and-will-increase-yours-too-4a84

Kind regards,
Petr

> ioctl_loop01.c code:
> static void setup(void)
> {
>         parted_sup = tst_cmd_present("parted");  <=====

> .....
>         if (parted_sup)
>                 SAFE_CMD(cmd_parted, NULL, NULL);  <===

>         sprintf(partscan_path, "/sys/block/loop%d/loop/partscan", dev_num);
> }

> > Kind regards,
> > Petr

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