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

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