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

[Thomas Huth]

On 17/01/2020 14.33, Igor Mammedov wrote:
> On Fri, 17 Jan 2020 12:14:11 +0100
> Thomas Huth <address@hidden> wrote:
> 
>> On 16/01/2020 18.06, Igor Mammedov wrote:
>>> 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).  
>>
>> 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"

Ah, well, that's what I was missing. Since the if-arch-statements follow
close to each other, I somehow read 'else if (!strcmp(arch, "ppc64"))'
... sorry for that.
Maybe it's more obvious if you'd do it the other way round, first the
"-object" lines, then the "-machine virt" stuff?

Anyway, another note: It's a little bit ugly that one if-statment uses
strcmp() != 0 while the other one uses !strcmp() ... since you're using
GLIB code here anyway, what do you think about converting them to
g_str_equal() instead?

 Thomas