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

[Li Wang]

Hi  Wei,

Petr Vorel <pvorel@suse.cz> wrote:


>
> > 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:
>

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.
>

Obviously, this is a good suggestion.

Wei, if you take a look at the git-history of LTP feature change,
most patchset organizations follow this principle.

We must keep a clean code and compile pass at the same time,
I have roughly gone through your patches, and they are seems
not so hard to rebase.



>
> 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.
>

Yes, this is also acceptable to me.

-- 
Regards,
Li Wang