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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 17 Jan 2020 12:14:11 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 16/01/2020 18.06, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 17:35:32 +0100
> > Thomas Huth <thuth@redhat.com> 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 <imammedo@redhat.com>
> >>> ---
> >>> PS:
> >>>   made as a separate patch so it won't clutter followup testcase changes.
> >>>
> >>> CC: thuth@redhat.com
> >>> CC: lvivier@redhat.com
> >>> ---
> >>>  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).  
> 
> Sorry, I think I missed something here... currently I see in the next patch:
> 
> +    if (!strcmp(arch, "ppc64")) {
> +        g_string_append(args, " -object
> memory-backend-ram,id=ram,size=512M");
> +    } else {
> +        g_string_append(args, " -object
> memory-backend-ram,id=ram,size=128M");
> +    }
> 
> ... so these are static strings which could also be handled fine without
> GString? Or do you plan to update this in later patches?
it's 1 or concatenation of 2 strings
  " -object memory-backend-ram,id=ram,size=128M"
  " -object memory-backend-ram,id=ram,size=512M"
  " -machine virt" + " -object memory-backend-ram,id=ram,size=128M"

It's possible to use static strings in expense of a bit of duplication
and long winded "if" chain. So using dynamic string here, looks better to me.

>  Thomas