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

[Petr Vorel <pvorel@suse.cz>]

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.

Kind regards,
Petr

> > Kind regards,
> > Petr

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