Re: [PATCH v2 83/86] tests:numa-test: make top level args dynamic and g_autofree(cli) cleanups

[Igor Mammedov]

On Thu, 16 Jan 2020 17:35:32 +0100
Thomas Huth <address@hidden> wrote:

> On 15/01/2020 16.07, Igor Mammedov wrote:
> > Use GString to pass argument to make_cli() so that it would be easy
> > to dynamically change test case arguments from main(). The follow up
> > patch will use it to change RAM size options depending on target.
> > 
> > While at it cleanup 'cli' freeing, using g_autofree annotation.  
> 
> Hmm, I'd use g_autofree for new code or do it in a separate cleanup
> patch, but doing this here distracts quite a bit from the real changes
> that you are doing...
I'll split it into separate patch

> 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > PS:
> >   made as a separate patch so it won't clutter followup testcase changes.
> > 
> > CC: address@hidden
> > CC: address@hidden
> > ---
> >  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
> >  1 file changed, 14 insertions(+), 24 deletions(-)
> > 
> > diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> > index 17dd807..a696dfd 100644
> > --- a/tests/qtest/numa-test.c
> > +++ b/tests/qtest/numa-test.c
> > @@ -14,16 +14,16 @@
> >  #include "qapi/qmp/qdict.h"
> >  #include "qapi/qmp/qlist.h"
> >  
> > -static char *make_cli(const char *generic_cli, const char *test_cli)
> > +static char *make_cli(const GString *generic_cli, const char *test_cli)
> >  {
> > -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", 
> > test_cli);
> > +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
> >  }  
> [...]
> > @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
> >  
> >  int main(int argc, char **argv)
> >  {
> > -    const char *args = NULL;
> > +    g_autoptr(GString) args = g_string_new("");  
> 
> I think g_string_new(NULL) would be better?
> 
> >      const char *arch = qtest_get_arch();
> >  
> >      if (strcmp(arch, "aarch64") == 0) {
> > -        args = "-machine virt";
> > +        g_string_append(args, " -machine virt")>      }  
> 
> Is this really required? Looking at your next patch, you could also
> simply do
> 
>           args = " -object memory-backend-ram,id=ram,size=xxxM"
xxx is variable so options are
 1 build this part of CLI dynamically
 2 mostly duplicate testcase function and include per target size there
 3 make up a test data structure and pass that to test cases

Given simplicity of current testcases, I'd prefer continue with
passing CLI as testcase data (option #1).


> 
> there? So using a GString seems overkill to me here.
> 
>  Thomas