Re: [Autotest] [PATCH 1/2] KVM test: Make the profiler could be configurated

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

 



On Tue, Mar 30, 2010 at 2:02 PM, Michael Goldish <mgoldish@xxxxxxxxxx> wrote:
>
> ----- "Jason Wang" <jasowang@xxxxxxxxxx> wrote:
>
>> The patch let the profilers could be specified through configuration
>> file. kvm_stat was kept as the default profiler.
>
> Looks good.  Some minor style comments:
>
>> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>> ---
>>  client/tests/kvm/kvm_utils.py          |   23
>> ++++++++++-------------
>>  client/tests/kvm/tests_base.cfg.sample |    2 +-
>>  2 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/client/tests/kvm/kvm_utils.py
>> b/client/tests/kvm/kvm_utils.py
>> index 8531c79..a73d5d4 100644
>> --- a/client/tests/kvm/kvm_utils.py
>> +++ b/client/tests/kvm/kvm_utils.py
>> @@ -866,24 +866,21 @@ def run_tests(test_list, job):
>>          if dependencies_satisfied:
>>              test_iterations = int(dict.get("iterations", 1))
>>              test_tag = dict.get("shortname")
>> -            # Setting up kvm_stat profiling during test execution.
>> -            # We don't need kvm_stat profiling on the build tests.
>> -            if dict.get("run_kvm_stat") == "yes":
>> -                profile = True
>> -            else:
>> -                # None because it's the default value on the base_test class
>> -                # and the value None is specifically checked there.
>> -                profile = None
>> +            # Setting up profilers during test execution.
>> +            profilers = dict.get("profilers")
>> +            if profilers is not None:
>
> I think it's nicer and shorter to say "if profilers" instead of
> "if profilers is not None".
> Better yet, use 'profilers = dict.get("profilers", "")' so that if
> profilers isn't defined, or if the user said 'profilers = ""', you can
> still call profilers.split(), i.e.:
>
> profilers = dict.get("profilers", "")
> for profiler in profilers.split():
>    job.profilers.add(profiler)
>
> and then you don't need the 'if'.
> This is also relevant to the job.profilers.delete() code below.
>
>> +                for profiler in profilers.split():
>> +                    job.profilers.add(profiler)
>>
>> -            if profile:
>> -                job.profilers.add('kvm_stat')
>>              # We need only one execution, profiled, hence we're passing
>>              # the profile_only parameter to job.run_test().
>>              current_status = job.run_test("kvm", params=dict, tag=test_tag,
>>                                            iterations=test_iterations,
>> -                                          profile_only=profile)
>> -            if profile:
>> -                job.profilers.delete('kvm_stat')
>> +                                          profile_only= profilers is not None)
>
> AFAIK, profile_only needs to be either True or None (Lucas, please correct
> me if I'm wrong).

You're right, Michael, the test class code checks specifically for
None. I have just reviewed the 2nd version of the patch and it looks
good. Applied:

http://autotest.kernel.org/changeset/4365
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux