<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">Hi Xiao,</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Mar 29, 2020 at 1:36 PM Xiao Yang <<a href="mailto:yangx.jy@cn.fujitsu.com">yangx.jy@cn.fujitsu.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2020/3/28 11:42, Petr Vorel wrote:<br>
> Hi Xiao,<br>
><br>
>> On 3/28/20 5:39 AM, Petr Vorel wrote:<br>
>>> + There is a double check (first in<br>
>>> .needs_cmds, then in SAFE_RUN_CMD()), maybe that's not needed.<br>
><br>
>> Hi Petr,<br>
><br>
>> Why do you need the duplicate .needs_cmds flag?(it introduces the double<br>
>> check as you said)<br>
><br>
>> Usually, all tests run commands by tst_run_cmd()/SAFE_RUN_CMD() and they can<br>
>> report TCONF<br>
><br>
>> by passing TST_RUN_CMD_CHECK_CMD so it is fair to be a part of<br>
>> tst_run_cmd()/SAFE_RUN_CMD().<br>
><br>
> Thanks for your review.<br>
> I guess Cyril will prefer .needs_cmds, as it can be parsed - metadata project:<br>
> <a href="https://people.kernel.org/metan/towards-parallel-kernel-test-runs" rel="noreferrer" target="_blank">https://people.kernel.org/metan/towards-parallel-kernel-test-runs</a><br>
> <a href="https://github.com/metan-ucw/ltp/tree/master/docparse" rel="noreferrer" target="_blank">https://github.com/metan-ucw/ltp/tree/master/docparse</a><br>
Hi Petr，<br>
<br>
Thank you for sharing these info.<br>
Does Cyril want to get metadata from struct tst_test directly?<br>
<br>
How about the rough design?<br>
1) .needs_cmds only saves the required commands.(doesn't do any check)<br>
2) pass the corresponding member of .needs_cmds to <br>
tst_run_cmd()/SAFE_RUN_CMD()(do check in these functions).<br>
For example:<br>
----------------------------------------------<br>
# grep tst_needs_cmds include/tst_cmd.h<br>
extern const char *const *tst_needs_cmds;<br>
<br>
# grep -B1 tst_needs_cmds lib/tst_test.c<br>
<br>
const char *const *tst_needs_cmds;<br>
-- <br>
     if (tst_test->needs_cmds)<br>
         tst_needs_cmds = tst_test->needs_cmds;<br>
<br>
# grep -A2 'needs_cmds' testcases/kernel/syscalls/add_key/add_key05.c<br>
     const char *const cmd_useradd[] = {tst_needs_cmds[0], username, NULL};<br>
     int rc;<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I don't see any advantage of involving this struct in a test case, and it also makes things more complicated.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">IMO, the '.needs_cmds' should do check and guarantee all the cmds exist. That's a hard requirement for the test. If a situation that the commands are only part of the requirement(soft), we could avoid using '.needs_cmds' in the test and just calling tst_run_cmd() without passing TST_RUN_CMD_CHECK_CMD flag. This satisfies most situations we have, it is safe enough and choosable for people.</div><div class="gmail_default" style="font-size:small"><br></div></div><div class="gmail_default" style="font-size:small">Or maybe I'm wrong here too:).</div><div class="gmail_default" style="font-size:small"><br></div></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>