On Fri, 17 Jan 2020 14:52:35 +0100
Thomas Huth <th...@redhat.com> wrote:
> On 17/01/2020 14.33, Igor Mammedov wrote:
> > On Fri, 17 Jan 2020 12:14:11 +0100
> > Thomas Huth <th...@redhat.com> wrote:
> >
> >> On 16/01/2020 18.06, Igor Mammedov wrote:
> >>> On Thu, 16 Jan 2020 17:35:32 +0100
> >>> Thomas Huth <th...@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 <imamm...@redhat.com>
> >>>>> ---
> >>>>> PS:
> >>>>> made as a separate patch so it won't clutter followup testcase
> >>>>> changes.
> >>>>>
> >>>>> CC: th...@redhat.com
> >>>>> CC: lviv...@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"
>
> 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?
will do that
>
> Thomas
