qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()


From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()
Date: Tue, 12 Sep 2017 08:32:43 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

On 09/12/2017 05:14 AM, Thomas Huth wrote:
> On 11.09.2017 19:20, Eric Blake wrote:
>> We have several callers that were formatting the argument strings
>> themselves; consolidate this effort by adding new convenience
>> functions directly in libqtest, and update all call-sites that
>> can benefit from it.
> [...]
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index e8c2e11817..b535d7768f 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args)
>>      return global_qtest = s;
>>  }
>>
>> +QTestState *qtest_vstartf(const char *fmt, va_list ap)
>> +{
>> +    char *args = g_strdup_vprintf(fmt, ap);
>> +    QTestState *s;
>> +
>> +    s = qtest_start(args);
>> +    global_qtest = NULL;
> 
> Don't you need a g_free(args) here?

D'oh.  Yes, I do.

>> +    qtest_quit(qtest_startf("-device %s", model));
> 
> Just my personal taste, but I think I'd be nicer to keep this on
> separate lines:
> 
>     QTestState *s;
> 
>     s = qtest_startf("-device %s", model);
>     qtest_quit(s);

Sure.  I debated about it.  If we ever do more than just create/destroy,
then having the separate lines makes it easier to stick the actual test
in between the two lines, so I'll avoid my abbreviation and go with the
longer form on respin.

>>  static void test_mon_partial(const void *data)
>>  {
>>      char *s;
>> -    char *cli;
>> +    const char *args = data;
>>
>> -    cli = make_cli(data, "-smp 8 "
>> -                   "-numa node,nodeid=0,cpus=0-1 "
>> -                   "-numa node,nodeid=1,cpus=4-5 ");
>> -    qtest_start(cli);
>> +    global_qtest = qtest_startf("%s -smp 8 "
>> +                                "-numa node,nodeid=0,cpus=0-1 "
>> +                                "-numa node,nodeid=1,cpus=4-5 ", args);
> 
> Does GCC emit a warning if you'd used data here directly? Otherwise I
> think you could simply do this without the local args variable...

Passing void* through varargs, with the intent of the receiver parsing
it as char*, is technically undefined in C.  I don't know if gcc warns,
but I'm also worried that clang might warn.  I prefer to err on the side
of defined behavior in this case, even though it annoyingly requires a
temporary variable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]