Re: [Qemu-devel] [PATCH 2/2] tests: fw_cfg: add reboot_timeout test case

[Laszlo Ersek]

On 01/22/19 02:28, Li Qiang wrote:
> Laszlo Ersek <address@hidden> 于2019年1月22日周二 上午5:38写道：
> 
>> On 01/20/19 08:13, Li Qiang wrote:
>>> Signed-off-by: Li Qiang <address@hidden>
>>> ---
>>>  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)?

Yes, that's what I meant.

>> 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" ?

Yes. A bit more precisely, when the return value is nonzero, it means
that some bytes have been transferred. If the fw_cfg file in question is
smaller than the allocated & passed-in buffer, then the buffer has been
populated only in part.

Vice versa, if the fw_cfg file in question is larger than the passed-in
buffer, then the return value explains how much room would have been
necessary in total. And, while the caller's buffer has been fully
populated, it has received only a starting slice of the fw_cfg file.

In the comparison that follows qfw_cfg_get_file(), we want to be sure
that the "reboot_timeout" integer object has been fully populated,
*plus* that we aren't ignoring any trailing bytes from the fw_cfg file.
Hence the strict equality on the size.

Thanks
Laszlo