Laszlo Ersek <ler...@redhat.com> 于2019年1月22日周二 上午5:38写道:
> On 01/20/19 08:13, Li Qiang wrote: > > Signed-off-by: Li Qiang <liq...@163.com> > > --- > > tests/fw_cfg-test.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c > > index 1c5103fe1c..c28e6c3fb5 100644 > > --- a/tests/fw_cfg-test.c > > +++ b/tests/fw_cfg-test.c > > @@ -99,6 +99,15 @@ static void test_fw_cfg_boot_menu(void) > > g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, > boot_menu); > > } > > > > +static void test_fw_cfg_reboot_timeout(void) > > +{ > > + uint32_t reboot_timeout; > > + > > + qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait", > > + &reboot_timeout, sizeof(reboot_timeout)); > > + g_assert_cmpint(reboot_timeout, ==, 15); > > +} > > + > > You don't check the return status of qfw_cfg_get_file(), before reading > "reboot_timeout". If the qfw_cfg_get_file() fails (returning 0), then > the comparison will refer to an indeterminate value. Also, it's > theoretically possible for qfw_cfg_get_file() to overwrite only part of > the "reboot_timeout" object. > > Right. I will change in the next revision. > So I think we need the function to transfer exactly (sizeof > reboot_timeout) bytes. > > What does this mean? check the return of 'qfw_cfg_get_file' if it is sizeof(reboot_timeout)? > BTW, this reminds me, qfw_cfg_get_file() seems to return the number of > bytes that would be necessary for transferring the entire file. That > looks like a good idea, but it should be documented. Please add some > docs on top of qfw_cfg_get_file(). > > The docs like "return 0 means failed and non-zero means successful but the caller need check the exactly size to avoid partially file size" ? Thanks, Li Qiang > > int main(int argc, char **argv) > > { > > QTestState *s; > > @@ -106,7 +115,8 @@ int main(int argc, char **argv) > > > > g_test_init(&argc, &argv, NULL); > > > > - s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8"); > > + s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8 " > > + "-boot reboot-timeout=15"); > > > > fw_cfg = pc_fw_cfg_init(s); > > > > @@ -125,6 +135,7 @@ int main(int argc, char **argv) > > qtest_add_func("fw_cfg/max_cpus", test_fw_cfg_max_cpus); > > qtest_add_func("fw_cfg/numa", test_fw_cfg_numa); > > qtest_add_func("fw_cfg/boot_menu", test_fw_cfg_boot_menu); > > + qtest_add_func("fw_cfg/reboot_timeout", test_fw_cfg_reboot_timeout); > > > > ret = g_test_run(); > > > > > > Looks OK otherwise. > > Thanks > Laszlo >