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

[Wei Gao via ltp <ltp@lists.linux.it>]

On Mon, Oct 20, 2025 at 03:21:40PM +0200, Petr Vorel wrote:
> Hi Wei,
> 
> > > > -	const char *const *needs_cmds;
> > > > +	struct tst_cmd *needs_cmds;
> 
> > > As I wrote in all previous versions, changing struct used in struct tst_test
> > > alone without changing will break this particular commit.
> 
> > > See when I apply just this commit.:
> > > https://github.com/pevik/ltp/actions/runs/18595891586
> > > https://github.com/pevik/ltp/commits/refs/heads/wei/needs_cmds.v4.first-commit-broken/
> 
> > > CC lib/newlib_tests/tst_expiration_timer
> > > tst_needs_cmds01.c:15:23: error: initialization of ‘struct tst_cmd *’ from incompatible pointer type ‘const char **’ [-Wincompatible-pointer-types]
> > >    15 |         .needs_cmds = (const char *[]) {
> > >       |                       ^
> > > tst_needs_cmds01.c:15:23: note: (near initialization for ‘test.needs_cmds’)
> 
> > > ...
> 
> > > quotactl01.c:226:23: error: initialization of ‘struct tst_cmd *’ from incompatible pointer type ‘const char * const*’ [-Wincompatible-pointer-types]
> > >   226 |         .needs_cmds = (const char *const []) {
> > >       |                       ^
> 
> > > Please you need to have this commit together with "Update test cases use new
> > > needs_cmds" commit into single commit (squash these two into a single commit).
> 
> > > Or am I missing something?
> > Thanks for your time do verify test can review.
> yw :).
> 
> > The reason i split this patch to small commits is easy for review, if you
> 
> Yes, it's desired that changes are split into smaller logical parts if *possible*.
> Yes, this really helps readability. But if possible means at lest 1) not
> breaking compilation (the worst case) 2) not breaking test functionality.
> 
> Therefore it's much easier to split into more commits some new library
> functionality (i.e. add some library functionality and enable it in separate
> commits, enable tests in a separate commit)
> 
> But modifying the functionality like this (when it breaks compilation) is worse
> than a bit more complex commit. This is the rule in many open source projects.
> 
> > need every commits can pass compile phase then i have to combine all
> > commits into a single big one, is that your request?
> 
> No, that's other extreme :). There is something in between, right?
> You did not get me correct, therefore in v4 you not only kept broken
> functionality, but you also joined the part which could be separated.  At least
> "ioctl_loop01.c: Update to new .needs_cmds struct" from v3 could have been added
> as a separate commit (after the main change, not before). Or am I missing
> something?
> 
> Unfortunately "lib: Add support option for .needs_cmds" and "Update test cases use
> new needs_cmds" and "tst_run_shell.c: Add new function handle new
> needs_cmds" needs to be in a single commit, but maybe you could add functions
> which implement it in a separate commits (e.g. preparation for a new change) and
> do the change (when it's actually used) in the last commit).  I'm not sure if
> it's worth of a separation, maybe not (hopefully we get a feedback from others).
> If yes:
> 
> 1) commit (lib preparation) would have: struct tst_cmd, bool
> tst_cmd_present(const char *cmd)
> 
> 2) commit (shell loader preparation) would have: enum cmd_ids, static
> ujson_obj_attr cmd_attrs[], static ujson_obj cmd_obj, static struct tst_cmd
> *parse_cmds(ujson_reader *reader, ujson_val *val).
> 
> 3) commit (use new functionality) would have: from "lib: Add support option for
> .needs_cmds":
> 
> -	const char *const *needs_cmds;
> +	struct tst_cmd *needs_cmds;
> 
> and use of tst_check_cmd()
> 
> from "tst_run_shell.c: Add new function handle new needs_cmds"
> -			test.needs_cmds = parse_strarr(&reader, &val);
> +			test.needs_cmds = parse_cmds(&reader, &val);
> 
> all code changes in "Update test cases use new needs_cmds"
> 
> 4) "ioctl_loop01.c: Update to new .needs_cmds struct" from v3 can be separate,
> it just have to be after library function changed.
> 
> 5) commit: modify some test to actually use some of new functionality.
> 
> If we are ok to do too many separate commits, then:
> 
> 1) commit: everything from this v4 in a single commit, but separate at least
> "ioctl_loop01.c: Update to new .needs_cmds struct" from v3.
> 
> 2) "ioctl_loop01.c: Update to new .needs_cmds struct" from v3.
> 
> 3) commit: modify some test to actually use some of new functionality.

Thanks for such detail explaination :)
Let's wait one more day for feedback from others, i prefer second
solution squash core changes into one commit without break from function
level.

BTW: Absent timely feedback, I will automatically begin creating the next patch with
the second solution.
> 
> Kind regards,
> Petr
> 
> > > Kind regards,
> > > Petr

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