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

[Petr Vorel <pvorel@suse.cz>]

> On Fri, Dec 12, 2025 at 12:16:18PM +0100, Petr Vorel wrote:
> > > Hi!
> > > > diff --git a/lib/tst_test.c b/lib/tst_test.c
> > > > index da5314c50..67553f65d 100644
> > > > --- a/lib/tst_test.c
> > > > +++ b/lib/tst_test.c
> > > > @@ -1365,6 +1365,19 @@ static const char *default_fs_type(void)
> > > >  	return tst_dev_fs_type();
> > > >  }

> > > > +bool tst_cmd_present(const char *cmd)
> > > > +{
> > > > +	struct tst_cmd *pcmd = tst_test->needs_cmds;
> > > > +
> > > > +	while (pcmd->cmd) {
> > > > +		if (!strcmp(pcmd->cmd, cmd))
> > > > +			return pcmd->present;
> > > > +
> > > > +		pcmd++;
> > > > +	}
> > > > +	return false;

> > > Once again, we should tst_brk(TBROK, "Unexpected command '%s'", cmd) here since we asked for
> > > something that we haven't checked for!

> > Good point. I'm sorry to overlook this in my review.

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

> 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);
> }

And also:

static struct tst_test test = {
	...
	.needs_cmds = (struct tst_cmd[]) {
		{.cmd = "parted", .optional = 1},
		{}

Therefore IMHO you are wrong.

+	while (pcmd->cmd) {
+		if (!strcmp(pcmd->cmd, cmd))
We always match this for programs which we checked via .needs_cmds (regardless
true or false return).
+			return pcmd->present;
+
+		pcmd++;
+	}

If we get here that means that we ask in tst_cmd_present("...") for program we
did not add .needs_cmds. That's why Cyril asked for tst_brk().
+	return false;

Kind regards,
Petr

> > Kind regards,
> > Petr

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