On 03/14/2012 02:47 AM, Chris Evich wrote: > On 03/13/2012 04:12 AM, guyanhua wrote: >> >> This adds configuration for "virsh capabilities" test. >> >> Signed-off-by: Gu Yanhua <guyanhua-f...@cn.fujitsu.com> >> --- >> client/virt/subtests.cfg.sample | 16 ++++++++++++++++ >> 1 files changed, 16 insertions(+), 0 deletions(-) >> >> diff --git a/client/virt/subtests.cfg.sample >> b/client/virt/subtests.cfg.sample >> index 5061f42..8552a58 100644 >> --- a/client/virt/subtests.cfg.sample >> +++ b/client/virt/subtests.cfg.sample >> @@ -218,6 +218,22 @@ variants: >> status_error = "yes" >> libvirtd = "off" >> >> + - virsh_capabilities: >> + type = virsh_capabilities >> + vms = '' >> + variants: >> + - no_option: >> + options = "" >> + status_error = "no" >> + libvirtd = "on" >> + - unexpect_option: >> + options = "xyz" >> + status_error = "yes" >> + libvirtd = "on" >> + - with_libvirtd_stop: >> + options = "" >> + status_error = "yes" >> + libvirtd = "off" >> >> - module_probe: >> type = module_probe > > This looks great! One small nit-pick of mine, and maybe it's my fault > (lost mail or whatever), but I thought I commented on the config. key > names, no? > > Assuming it was lost, my suggestion was: It's good to prefix any > possibly ambiguous names, with something more obvious as to > purpose/source, to ease debugging. Does that make sense? > > Remember if you're looking at test results, all you get is 'key = > value' and not much other context. So, for example, if you see > something like "options = whatever", it's hard to know what 'options' > and what it's used for w/o digging through code (which could be very > old or hard to access or whatever reason). Also, because (IIRC) the > keys are sorted alphabetically in the output, if you put a common > prefix, then relevant options all get sorted together. > > One suggestion would be for prefixing the keys with 'virsh_cap_' or > "capabilities_" or something Good point, virsh_caps should be okay as a short/simple name. > like that. Though since it's a simple thing, maybe this can be fixed > on commit. It's just a small nit-pick of mine, otherwise this > contribution looks great! Thanks! This series are fine for me.
Chris, thank you for cc to me, otherwise, I maybe miss this thread :-) Yanhua, it's just a improvement if you want to do it, thanks for your contribution. Regards, Alex > > _______________________________________________ Autotest mailing list Autotest@test.kernel.org http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
