Re: [Qemu-devel] [PATCH 0/2] tests: fw_cfg: add reboot-timeout test case

Re: [Qemu-devel] [PATCH 0/2] tests: fw_cfg: add reboot-timeout test case

[Philippe Mathieu-Daudé]

On 3/28/19 11:45 AM, 李强 wrote:
> Ping...
> 
> What's your opinion, Philippe?

Eh sorry my email client tagged this series as reviewed since your
previous v1 was reviewed by Laszlo. I'll review your patches, but please
increase the version between series next time so I won't miss it that
easily ;)

> 
> Thanks,
> Li Qiang

Re: [Qemu-devel] [PATCH 0/2] tests: fw_cfg: add reboot-timeout test case

[Philippe Mathieu-Daudé]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 326 bytes --]

On 3/28/19 11:45 AM, ÀîÇ¿ wrote:
> Ping...
> 
> What's your opinion, Philippe?

Eh sorry my email client tagged this series as reviewed since your
previous v1 was reviewed by Laszlo. I'll review your patches, but please
increase the version between series next time so I won't miss it that
easily ;)

> 
> Thanks,
> Li Qiang

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

[Philippe Mathieu-Daudé]

Hi Li,

On 3/19/19 3:30 AM, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/fw_cfg-test.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 1c5103fe1c..551b51e38f 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -99,6 +99,17 @@ 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 = 0;
> +    size_t filesize;
> +
> +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> +                     &reboot_timeout, sizeof(reboot_timeout));
> +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
> +    g_assert_cmpint(reboot_timeout, ==, 15);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      QTestState *s;
> @@ -106,7 +117,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");

This modify all tests. I'd rather add a specific test with this option.
Doing so, we can easily modify the timeout and add the <0 and >0xffff cases.

Can you think of a 'splash-time' test (for commit 6912bb0b3d3b1)?

Regards,

Phil.

>  
>      fw_cfg = pc_fw_cfg_init(s);
>  
> @@ -125,6 +137,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();
>  
> 

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

[Philippe Mathieu-Daudé]

Hi Li,

On 3/19/19 3:30 AM, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/fw_cfg-test.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> index 1c5103fe1c..551b51e38f 100644
> --- a/tests/fw_cfg-test.c
> +++ b/tests/fw_cfg-test.c
> @@ -99,6 +99,17 @@ 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 = 0;
> +    size_t filesize;
> +
> +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> +                     &reboot_timeout, sizeof(reboot_timeout));
> +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
> +    g_assert_cmpint(reboot_timeout, ==, 15);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      QTestState *s;
> @@ -106,7 +117,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");

This modify all tests. I'd rather add a specific test with this option.
Doing so, we can easily modify the timeout and add the <0 and >0xffff cases.

Can you think of a 'splash-time' test (for commit 6912bb0b3d3b1)?

Regards,

Phil.

>  
>      fw_cfg = pc_fw_cfg_init(s);
>  
> @@ -125,6 +137,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();
>  
> 

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月19日周五 上午5:01写道：

> Hi Li,
>
> On 3/19/19 3:30 AM, Li Qiang wrote:
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  tests/fw_cfg-test.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> > index 1c5103fe1c..551b51e38f 100644
> > --- a/tests/fw_cfg-test.c
> > +++ b/tests/fw_cfg-test.c
> > @@ -99,6 +99,17 @@ 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 = 0;
> > +    size_t filesize;
> > +
> > +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> > +                     &reboot_timeout, sizeof(reboot_timeout));
> > +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
> > +    g_assert_cmpint(reboot_timeout, ==, 15);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      QTestState *s;
> > @@ -106,7 +117,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");
>
> This modify all tests. I'd rather add a specific test with this option.
> Doing so, we can easily modify the timeout and add the <0 and >0xffff
> cases.
>
>
Ok, will do in the next revision.


> Can you think of a 'splash-time' test (for commit 6912bb0b3d3b1)?


Ok.

PS: any comments in the first patch.

Thanks,
Li Qiang


>
> Regards,
>
> Phil.
>
> >
> >      fw_cfg = pc_fw_cfg_init(s);
> >
> > @@ -125,6 +137,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();
> >
> >
>

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月19日周五 上午5:01写道：

> Hi Li,
>
> On 3/19/19 3:30 AM, Li Qiang wrote:
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  tests/fw_cfg-test.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> > index 1c5103fe1c..551b51e38f 100644
> > --- a/tests/fw_cfg-test.c
> > +++ b/tests/fw_cfg-test.c
> > @@ -99,6 +99,17 @@ 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 = 0;
> > +    size_t filesize;
> > +
> > +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> > +                     &reboot_timeout, sizeof(reboot_timeout));
> > +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
> > +    g_assert_cmpint(reboot_timeout, ==, 15);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      QTestState *s;
> > @@ -106,7 +117,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");
>
> This modify all tests. I'd rather add a specific test with this option.
> Doing so, we can easily modify the timeout and add the <0 and >0xffff
> cases.
>
>
Ok, will do in the next revision.


> Can you think of a 'splash-time' test (for commit 6912bb0b3d3b1)?


Ok.

PS: any comments in the first patch.

Thanks,
Li Qiang


>
> Regards,
>
> Phil.
>
> >
> >      fw_cfg = pc_fw_cfg_init(s);
> >
> > @@ -125,6 +137,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();
> >
> >
>

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月19日周五 上午5:01写道：

> Hi Li,
>
> On 3/19/19 3:30 AM, Li Qiang wrote:
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  tests/fw_cfg-test.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> > index 1c5103fe1c..551b51e38f 100644
> > --- a/tests/fw_cfg-test.c
> > +++ b/tests/fw_cfg-test.c
> > @@ -99,6 +99,17 @@ 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 = 0;
> > +    size_t filesize;
> > +
> > +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> > +                     &reboot_timeout, sizeof(reboot_timeout));
> > +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
> > +    g_assert_cmpint(reboot_timeout, ==, 15);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      QTestState *s;
> > @@ -106,7 +117,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");
>
> This modify all tests. I'd rather add a specific test with this option.
> Doing so, we can easily modify the timeout and add the <0 and >0xffff
> cases.
>
> Can you think of a 'splash-time' test (for commit 6912bb0b3d3b1)?
>
>
Hi Philippe,

I have sent out the new revision patchset.
Please notice as the new patchset changed a lot(refactor the fw_cfg_test
and add two test cases)
I don't bump the version.

Thanks,
Li Qiang



> Regards,
>
> Phil.
>
> >
> >      fw_cfg = pc_fw_cfg_init(s);
> >
> > @@ -125,6 +137,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();
> >
> >
>

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

[Li Qiang]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2019年4月19日周五 上午5:01写道：

> Hi Li,
>
> On 3/19/19 3:30 AM, Li Qiang wrote:
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >  tests/fw_cfg-test.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
> > index 1c5103fe1c..551b51e38f 100644
> > --- a/tests/fw_cfg-test.c
> > +++ b/tests/fw_cfg-test.c
> > @@ -99,6 +99,17 @@ 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 = 0;
> > +    size_t filesize;
> > +
> > +    filesize = qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
> > +                     &reboot_timeout, sizeof(reboot_timeout));
> > +    g_assert_cmpint(filesize, ==, sizeof(reboot_timeout));
> > +    g_assert_cmpint(reboot_timeout, ==, 15);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      QTestState *s;
> > @@ -106,7 +117,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");
>
> This modify all tests. I'd rather add a specific test with this option.
> Doing so, we can easily modify the timeout and add the <0 and >0xffff
> cases.
>
> Can you think of a 'splash-time' test (for commit 6912bb0b3d3b1)?
>
>
Hi Philippe,

I have sent out the new revision patchset.
Please notice as the new patchset changed a lot(refactor the fw_cfg_test
and add two test cases)
I don't bump the version.

Thanks,
Li Qiang



> Regards,
>
> Phil.
>
> >
> >      fw_cfg = pc_fw_cfg_init(s);
> >
> > @@ -125,6 +137,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();
> >
> >
>

Re: [Qemu-devel] [PATCH 1/2] tests: fw_cfg: add a function to get the fw_cfg file

[Thomas Huth]

On 19/03/2019 03.30, Li Qiang wrote:
> This is useful to write qtest about fw_cfg file entry.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/fw_cfg.h |  2 ++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
> index d0889d1e22..2df33df859 100644
> --- a/tests/libqos/fw_cfg.c
> +++ b/tests/libqos/fw_cfg.c
> @@ -16,12 +16,57 @@
>  #include "libqos/fw_cfg.h"
>  #include "libqtest.h"
>  #include "qemu/bswap.h"
> +#include "hw/nvram/fw_cfg.h"
>  
>  void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>  {
>      fw_cfg->select(fw_cfg, key);
>  }
>  
> +/*
> + * The caller need check the return value. 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.
> + *
> + * 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.
> + */
> +size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
> +                      void *data, size_t buflen)
> +{
> +    uint32_t count;
> +    uint32_t i;
> +    unsigned char *filesbuf = NULL;
> +    size_t dsize;
> +    FWCfgFile *pdir_entry;
> +    size_t filesize = 0;
> +
> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
> +    count = be32_to_cpu(count);
> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
> +    filesbuf = g_malloc0(dsize);
> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);

If I get the code right,  qfw_cfg_get() fills the whole buffer here...
in that case, g_malloc() should be sufficient, so you don't need
g_malloc0() here.

 Thomas

Re: [Qemu-devel] [PATCH 1/2] tests: fw_cfg: add a function to get the fw_cfg file

[Thomas Huth]

On 19/03/2019 03.30, Li Qiang wrote:
> This is useful to write qtest about fw_cfg file entry.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  tests/libqos/fw_cfg.h |  2 ++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
> index d0889d1e22..2df33df859 100644
> --- a/tests/libqos/fw_cfg.c
> +++ b/tests/libqos/fw_cfg.c
> @@ -16,12 +16,57 @@
>  #include "libqos/fw_cfg.h"
>  #include "libqtest.h"
>  #include "qemu/bswap.h"
> +#include "hw/nvram/fw_cfg.h"
>  
>  void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>  {
>      fw_cfg->select(fw_cfg, key);
>  }
>  
> +/*
> + * The caller need check the return value. 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.
> + *
> + * 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.
> + */
> +size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
> +                      void *data, size_t buflen)
> +{
> +    uint32_t count;
> +    uint32_t i;
> +    unsigned char *filesbuf = NULL;
> +    size_t dsize;
> +    FWCfgFile *pdir_entry;
> +    size_t filesize = 0;
> +
> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
> +    count = be32_to_cpu(count);
> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
> +    filesbuf = g_malloc0(dsize);
> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);

If I get the code right,  qfw_cfg_get() fills the whole buffer here...
in that case, g_malloc() should be sufficient, so you don't need
g_malloc0() here.

 Thomas

Re: [Qemu-devel] [PATCH 1/2] tests: fw_cfg: add a function to get the fw_cfg file

[Philippe Mathieu-Daudé]

Hi Thomas,

On 4/19/19 9:03 AM, Thomas Huth wrote:
> On 19/03/2019 03.30, Li Qiang wrote:
>> This is useful to write qtest about fw_cfg file entry.
>>
>> Signed-off-by: Li Qiang <liq3ea@163.com>
>> ---
>>  tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>>  tests/libqos/fw_cfg.h |  2 ++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
>> index d0889d1e22..2df33df859 100644
>> --- a/tests/libqos/fw_cfg.c
>> +++ b/tests/libqos/fw_cfg.c
>> @@ -16,12 +16,57 @@
>>  #include "libqos/fw_cfg.h"
>>  #include "libqtest.h"
>>  #include "qemu/bswap.h"
>> +#include "hw/nvram/fw_cfg.h"
>>  
>>  void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>>  {
>>      fw_cfg->select(fw_cfg, key);
>>  }
>>  
>> +/*
>> + * The caller need check the return value. 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.
>> + *
>> + * 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.
>> + */
>> +size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
>> +                      void *data, size_t buflen)
>> +{
>> +    uint32_t count;
>> +    uint32_t i;
>> +    unsigned char *filesbuf = NULL;
>> +    size_t dsize;
>> +    FWCfgFile *pdir_entry;
>> +    size_t filesize = 0;
>> +
>> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
>> +    count = be32_to_cpu(count);
>> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
>> +    filesbuf = g_malloc0(dsize);
>> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
> 
> If I get the code right,  qfw_cfg_get() fills the whole buffer here...
> in that case, g_malloc() should be sufficient, so you don't need
> g_malloc0() here.

Correct.

Apparently Li did address your suggestion in his next iteration of this
patch (same subject, Message-Id: <20190420100056.116305-3-liq3ea@163.com>).

Thanks,

Phil.

Re: [Qemu-devel] [PATCH 1/2] tests: fw_cfg: add a function to get the fw_cfg file

[Philippe Mathieu-Daudé]

Hi Thomas,

On 4/19/19 9:03 AM, Thomas Huth wrote:
> On 19/03/2019 03.30, Li Qiang wrote:
>> This is useful to write qtest about fw_cfg file entry.
>>
>> Signed-off-by: Li Qiang <liq3ea@163.com>
>> ---
>>  tests/libqos/fw_cfg.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>>  tests/libqos/fw_cfg.h |  2 ++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/tests/libqos/fw_cfg.c b/tests/libqos/fw_cfg.c
>> index d0889d1e22..2df33df859 100644
>> --- a/tests/libqos/fw_cfg.c
>> +++ b/tests/libqos/fw_cfg.c
>> @@ -16,12 +16,57 @@
>>  #include "libqos/fw_cfg.h"
>>  #include "libqtest.h"
>>  #include "qemu/bswap.h"
>> +#include "hw/nvram/fw_cfg.h"
>>  
>>  void qfw_cfg_select(QFWCFG *fw_cfg, uint16_t key)
>>  {
>>      fw_cfg->select(fw_cfg, key);
>>  }
>>  
>> +/*
>> + * The caller need check the return value. 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.
>> + *
>> + * 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.
>> + */
>> +size_t qfw_cfg_get_file(QFWCFG *fw_cfg, const char *filename,
>> +                      void *data, size_t buflen)
>> +{
>> +    uint32_t count;
>> +    uint32_t i;
>> +    unsigned char *filesbuf = NULL;
>> +    size_t dsize;
>> +    FWCfgFile *pdir_entry;
>> +    size_t filesize = 0;
>> +
>> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, &count, sizeof(count));
>> +    count = be32_to_cpu(count);
>> +    dsize = sizeof(uint32_t) + count * sizeof(struct fw_cfg_file);
>> +    filesbuf = g_malloc0(dsize);
>> +    qfw_cfg_get(fw_cfg, FW_CFG_FILE_DIR, filesbuf, dsize);
> 
> If I get the code right,  qfw_cfg_get() fills the whole buffer here...
> in that case, g_malloc() should be sufficient, so you don't need
> g_malloc0() here.

Correct.

Apparently Li did address your suggestion in his next iteration of this
patch (same subject, Message-Id: <20190420100056.116305-3-liq3ea@163.com>).

Thanks,

Phil.

[Qemu-devel] [PATCH 0/2] tests: fw_cfg: add reboot-timeout test case

[Li Qiang]

The first patch adds a util function to get 
the fw_cfg file entry.
And second adds a reboot-timeout test case.

Li Qiang (2):
  tests: fw_cfg: add a function to get the fw_cfg file entry
  tests: fw_cfg: add reboot_timeout test case

 tests/fw_cfg-test.c   | 13 ++++++++++++-
 tests/libqos/fw_cfg.c | 33 +++++++++++++++++++++++++++++++++
 tests/libqos/fw_cfg.h |  2 ++
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@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);
+}
+
 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();
 
-- 
2.17.1

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

[Laszlo Ersek]

On 01/20/19 08:13, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@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.

So I think we need the function to transfer exactly (sizeof
reboot_timeout) bytes.

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

>  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

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

[Li Qiang]

Laszlo Ersek <lersek@redhat.com> 于2019年1月22日周二 上午5:38写道：

> On 01/20/19 08:13, Li Qiang wrote:
> > Signed-off-by: Li Qiang <liq3ea@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
>

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 <lersek@redhat.com> 于2019年1月22日周二 上午5:38写道：
> 
>> On 01/20/19 08:13, Li Qiang wrote:
>>> Signed-off-by: Li Qiang <liq3ea@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)?

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

[Qemu-devel] [PATCH 0/2] test: fw_cfg: add reboot-timeout test case

[Li Qiang]

This patchset is the test case for the following patch:

https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05254.html

Li Qiang (2):
  tests: fw_cfg: add a function to get the fw_cfg file entry
  tests: fw_cfg: add reboot_timeout test case

 tests/fw_cfg-test.c   | 13 ++++++++++++-
 tests/libqos/fw_cfg.c | 30 ++++++++++++++++++++++++++++++
 tests/libqos/fw_cfg.h |  2 ++
 3 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.17.1

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

[Li Qiang]

Signed-off-by: Li Qiang <liq3ea@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..37765f15f8 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, <=, 65535);
+}
+
 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=4294967295");
 
     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();
 
-- 
2.17.1

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

[Philippe Mathieu-Daudé]

On 28/10/18 13:40, Li Qiang wrote:
> Signed-off-by: Li Qiang <liq3ea@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..37765f15f8 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, <=, 65535);
> +}
> +
>   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=4294967295");

I'd rather change this test to use qtest_add_data_func() ...:

         qtest_add_data_func("fw_cfg/reboot_timeout", "-boot 
reboot-timeout=4294967295 ", test_fw_cfg_reboot_timeout);

... to avoid adding this command line option to all the tests, because 
all tests are now failing:

$ make check-qtest-i386
[...]
ERROR:qemu/tests/fw_cfg-test.c:33:test_fw_cfg_signature: assertion 
failed (buf == "QEMU"): ("\377\377\377\377" == "QEMU")
ERROR:qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id: assertion failed: ((id 
== 1) || (id == 3))
ERROR:qemu/tests/fw_cfg-test.c:52:test_fw_cfg_uuid: assertion failed: 
(memcmp(buf, uuid, sizeof(buf)) == 0)
ERROR:qemu/tests/fw_cfg-test.c:57:test_fw_cfg_ram_size: assertion failed 
(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE) == ram_size): (-1 == 134217728)
ERROR:qemu/tests/fw_cfg-test.c:62:test_fw_cfg_nographic: assertion 
failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC) == 0): (65535 == 0)
ERROR:qemu/tests/fw_cfg-test.c:67:test_fw_cfg_nb_cpus: assertion failed 
(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS) == nb_cpus): (65535 == 1)
ERROR:qemu/tests/fw_cfg-test.c:72:test_fw_cfg_max_cpus: assertion failed 
(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS) == max_cpus): (65535 == 1)
ERROR:qemu/tests/fw_cfg-test.c:80:test_fw_cfg_numa: assertion failed 
(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA) == nb_nodes): (-1 == 0)
ERROR:qemu/tests/fw_cfg-test.c:99:test_fw_cfg_boot_menu: assertion 
failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU) == boot_menu): (65535 
== 0)


(did you test your patch?)

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

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

[li qiang]

在 2018/10/30 8:08, Philippe Mathieu-Daudé 写道:
> On 28/10/18 13:40, Li Qiang wrote:
>> Signed-off-by: Li Qiang <liq3ea@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..37765f15f8 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, <=, 65535);
>> +}
>> +
>>   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=4294967295");
>
> I'd rather change this test to use qtest_add_data_func() ...:
>
>         qtest_add_data_func("fw_cfg/reboot_timeout", "-boot 
> reboot-timeout=4294967295 ", test_fw_cfg_reboot_timeout);
>
> ... to avoid adding this command line option to all the tests, because 
> all tests are now failing:
>
> $ make check-qtest-i386
> [...]
> ERROR:qemu/tests/fw_cfg-test.c:33:test_fw_cfg_signature: assertion 
> failed (buf == "QEMU"): ("\377\377\377\377" == "QEMU")
> ERROR:qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id: assertion failed: 
> ((id == 1) || (id == 3))
> ERROR:qemu/tests/fw_cfg-test.c:52:test_fw_cfg_uuid: assertion failed: 
> (memcmp(buf, uuid, sizeof(buf)) == 0)
> ERROR:qemu/tests/fw_cfg-test.c:57:test_fw_cfg_ram_size: assertion 
> failed (qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE) == ram_size): (-1 == 
> 134217728)
> ERROR:qemu/tests/fw_cfg-test.c:62:test_fw_cfg_nographic: assertion 
> failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC) == 0): (65535 == 0)
> ERROR:qemu/tests/fw_cfg-test.c:67:test_fw_cfg_nb_cpus: assertion 
> failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS) == nb_cpus): (65535 == 1)
> ERROR:qemu/tests/fw_cfg-test.c:72:test_fw_cfg_max_cpus: assertion 
> failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS) == max_cpus): (65535 
> == 1)
> ERROR:qemu/tests/fw_cfg-test.c:80:test_fw_cfg_numa: assertion failed 
> (qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA) == nb_nodes): (-1 == 0)
> ERROR:qemu/tests/fw_cfg-test.c:99:test_fw_cfg_boot_menu: assertion 
> failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU) == boot_menu): 
> (65535 == 0)
>
>
> (did you test your patch?)
>
Of course I test it, but only in x86_64.


Here is the result without the fix patch.

xxxqemu# QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 
tests/fw_cfg-test
/x86_64/fw_cfg/signature: OK
/x86_64/fw_cfg/id: OK
/x86_64/fw_cfg/uuid: OK
/x86_64/fw_cfg/ram_size: OK
/x86_64/fw_cfg/nographic: OK
/x86_64/fw_cfg/nb_cpus: OK
/x86_64/fw_cfg/max_cpus: OK
/x86_64/fw_cfg/numa: OK
/x86_64/fw_cfg/boot_menu: OK
/x86_64/fw_cfg/reboot_timeout: **
ERROR:tests/fw_cfg-test.c:108:test_fw_cfg_reboot_timeout: assertion 
failed (reboot_timeout <= 65535): (4294967295 <= 65535)
Aborted
xxxqemu# make check-qtest-x86_64
   GTESTER check-qtest-x86_64
**
ERROR:tests/fw_cfg-test.c:108:test_fw_cfg_reboot_timeout: assertion 
failed (reboot_timeout <= 65535): (4294967295 <= 65535)
GTester: last random seed: R02Sfed7191ede4ed60d4f3069f9ec808e73
xxxqemu/tests/Makefile.include:805: recipe for target 
'check-qtest-x86_64' failed
make: *** [check-qtest-x86_64] Error 1

I did it in i386 right now. Following is the results:

xxxqemu# QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386 tests/fw_cfg-test
/i386/fw_cfg/signature: OK
/i386/fw_cfg/id: OK
/i386/fw_cfg/uuid: OK
/i386/fw_cfg/ram_size: OK
/i386/fw_cfg/nographic: OK
/i386/fw_cfg/nb_cpus: OK
/i386/fw_cfg/max_cpus: OK
/i386/fw_cfg/numa: OK
/i386/fw_cfg/boot_menu: OK
/i386/fw_cfg/reboot_timeout: **
ERROR:tests/fw_cfg-test.c:108:test_fw_cfg_reboot_timeout: assertion 
failed (reboot_timeout <= 65535): (4294967295 <= 65535)
Aborted
xxxqemu# make chek-qtest-i386
make: *** No rule to make target 'chek-qtest-i386'.  Stop.
xxxqemu# make check-qtest-i386
   GTESTER check-qtest-i386
**
ERROR:tests/fw_cfg-test.c:108:test_fw_cfg_reboot_timeout: assertion 
failed (reboot_timeout <= 65535): (4294967295 <= 65535)
GTester: last random seed: R02S7659b379e665961e0060afeb449948b2
xxxqemu/tests/Makefile.include:805: recipe for target 'check-qtest-i386' 
failed
make: *** [check-qtest-i386] Error 1


Thanks,

Li Qiang

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

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

[李强]

At 2018-10-30 08:08:55, "Philippe Mathieu-Daudé" <philmd@redhat.com> wrote:
>On 28/10/18 13:40, Li Qiang wrote:
>> Signed-off-by: Li Qiang <liq3ea@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..37765f15f8 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, <=, 65535);
>> +}
>> +
>>   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=4294967295");
>
>I'd rather change this test to use qtest_add_data_func() ...:
>
>         qtest_add_data_func("fw_cfg/reboot_timeout", "-boot 
>reboot-timeout=4294967295 ", test_fw_cfg_reboot_timeout);

>


Hi here I think use qtest_add_func is ok because there is no need
to make such an exception as all of them uses qtest_add_func.


Second, the uuid is also uses by all the cases though some of them are not needed.
Third, the -boot option will not affect the other cases.


Thanks,
Li Qiang


>... to avoid adding this command line option to all the tests, because 
>all tests are now failing:
>
>$ make check-qtest-i386
>[...]
>ERROR:qemu/tests/fw_cfg-test.c:33:test_fw_cfg_signature: assertion 
>failed (buf == "QEMU"): ("\377\377\377\377" == "QEMU")
>ERROR:qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id: assertion failed: ((id 
>== 1) || (id == 3))
>ERROR:qemu/tests/fw_cfg-test.c:52:test_fw_cfg_uuid: assertion failed: 
>(memcmp(buf, uuid, sizeof(buf)) == 0)
>ERROR:qemu/tests/fw_cfg-test.c:57:test_fw_cfg_ram_size: assertion failed 
>(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE) == ram_size): (-1 == 134217728)
>ERROR:qemu/tests/fw_cfg-test.c:62:test_fw_cfg_nographic: assertion 
>failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC) == 0): (65535 == 0)
>ERROR:qemu/tests/fw_cfg-test.c:67:test_fw_cfg_nb_cpus: assertion failed 
>(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS) == nb_cpus): (65535 == 1)
>ERROR:qemu/tests/fw_cfg-test.c:72:test_fw_cfg_max_cpus: assertion failed 
>(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS) == max_cpus): (65535 == 1)
>ERROR:qemu/tests/fw_cfg-test.c:80:test_fw_cfg_numa: assertion failed 
>(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA) == nb_nodes): (-1 == 0)
>ERROR:qemu/tests/fw_cfg-test.c:99:test_fw_cfg_boot_menu: assertion 
>failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU) == boot_menu): (65535 
>== 0)
>
>
>(did you test your patch?)
>
>>   
>>       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();
>>   
>>