----- "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).
In that case, it would be appropriate to use
profile_only=bool(profilers) or None
so that if profilers is e.g. "kvm_stat", profile_only will be True,
and if profilers is "", profile_only will be None.
> +
> + if profilers is not None:
> + for profiler in profilers.split():
> + job.profilers.delete(profiler)
>
> if not current_status:
> failed = True
> diff --git a/client/tests/kvm/tests_base.cfg.sample
> b/client/tests/kvm/tests_base.cfg.sample
> index d162cf8..cc10713 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -41,7 +41,7 @@ nic_script = scripts/qemu-ifup
> address_index = 0
>
> # Misc
> -run_kvm_stat = yes
> +profilers = "kvm_stat "
We don't need the quotes here now. We'll need them later if we add
more profilers. So it's OK to use
profilers = kvm_stat
and then later if we need another profiler:
profilers += " some_other_profiler"
>
>
> # Tests
>
> --
> 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
--
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
