[PATCH v2 00/11] tools/kvm_stat: misc fixes

[PATCH v2 00/11] tools/kvm_stat: misc fixes

[Stefan Raspl]

Fixed a few more glitches, most of which should be pretty straight forward.
I'm not happy with what the man page looks like, in that the "INTERACTIVE
COMMANDS" section's line width is way too small. I noticed this in
particular with 5/11 "tools/kvm_stat: fix child trace events accounting",
but couldn't find a way to get ASCIIDOC to widen those lines a bit.


Stefan Raspl (11):
  tools/kvm_stat: fix command line option '-g'
  tools/kvm_stat: fix drilldown in events-by-guests mode
  tools/kvm_stat: fix missing field update after filter change
  tools/kvm_stat: fix extra handling of 'help' with fields filter
  tools/kvm_stat: fix child trace events accounting
  tools/kvm_stat: add hint on '-f help' to man page
  tools/kvm_stat: handle invalid regular expressions
  tools/kvm_stat: suppress usage information on command line errors
  tools/kvm_stat: stop ignoring unhandled arguments
  tools/kvm_stat: sort '-f help' output
  tools/kvm_stat: add line for totals

 tools/kvm/kvm_stat/kvm_stat     | 76 ++++++++++++++++++++++++-----------------
 tools/kvm/kvm_stat/kvm_stat.txt |  4 ++-
 2 files changed, 48 insertions(+), 32 deletions(-)

-- 
2.13.5

[PATCH v2 01/11] tools/kvm_stat: fix command line option '-g'

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Specifying a guest via '-g foo' always results in an error:
  $ kvm_stat -g foo
  Usage: kvm_stat [options]

  kvm_stat: error: Error while searching for guest "foo", use "-p" to
  specify a pid instead

Reason is that Tui.get_pid_from_gname() is not static, as it is supposed
to be.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 217cf6f95c36..884a74b8ca87 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -950,7 +950,8 @@ class Tui(object):
             curses.nocbreak()
             curses.endwin()
 
-    def get_all_gnames(self):
+    @staticmethod
+    def get_all_gnames():
         """Returns a list of (pid, gname) tuples of all running guests"""
         res = []
         try:
@@ -963,7 +964,7 @@ class Tui(object):
             # perform a sanity check before calling the more expensive
             # function to possibly extract the guest name
             if ' -name ' in line[1]:
-                res.append((line[0], self.get_gname_from_pid(line[0])))
+                res.append((line[0], Tui.get_gname_from_pid(line[0])))
         child.stdout.close()
 
         return res
@@ -984,7 +985,8 @@ class Tui(object):
         except Exception:
             self.screen.addstr(row + 1, 2, 'Not available')
 
-    def get_pid_from_gname(self, gname):
+    @staticmethod
+    def get_pid_from_gname(gname):
         """Fuzzy function to convert guest name to QEMU process pid.
 
         Returns a list of potential pids, can be empty if no match found.
@@ -992,7 +994,7 @@ class Tui(object):
 
         """
         pids = []
-        for line in self.get_all_gnames():
+        for line in Tui.get_all_gnames():
             if gname == line[1]:
                 pids.append(int(line[0]))
 
-- 
2.13.5

[PATCH v2 02/11] tools/kvm_stat: fix drilldown in events-by-guests mode

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

When displaying debugfs events listed by guests, an attempt to switch to
reporting of stats for individual child trace events results in garbled
output. Reason is that when toggling drilldown, the update of the stats
doesn't honor when events are displayed by guests, as indicated by
Tui._display_guests.
To reproduce, run 'kvm_stat -d' and press 'b' followed by 'x'.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 884a74b8ca87..6347ad5d0d35 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1360,7 +1360,7 @@ class Tui(object):
                 if char == 'x':
                     self.update_drilldown()
                     # prevents display of current values on next refresh
-                    self.stats.get()
+                    self.stats.get(self._display_guests)
             except KeyboardInterrupt:
                 break
             except curses.error:
-- 
2.13.5

[PATCH v2 03/11] tools/kvm_stat: fix missing field update after filter change

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

When updating the fields filter, tracepoint events of fields previously
not visible were not enabled, as TracepointProvider.update_fields()
updated the member variable directly instead of using the setter, which
triggers the event enable/disable.
To reproduce, run 'kvm_stat -f kvm_exit', press 'c' to remove the
filter, and notice that no add'l fields that do not match the regex
'kvm_exit' will appear.
This issue was introduced by commit c469117df059 ("tools/kvm_stat:
simplify initializers").

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 6347ad5d0d35..f133755fdde2 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -549,8 +549,8 @@ class TracepointProvider(Provider):
 
     def update_fields(self, fields_filter):
         """Refresh fields, applying fields_filter"""
-        self._fields = [field for field in self.get_available_fields()
-                        if self.is_field_wanted(fields_filter, field)]
+        self.fields = [field for field in self.get_available_fields()
+                       if self.is_field_wanted(fields_filter, field)]
 
     @staticmethod
     def get_online_cpus():
-- 
2.13.5

[PATCH v2 04/11] tools/kvm_stat: fix extra handling of 'help' with fields filter

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Commit 67fbcd62f54d ("tools/kvm_stat: add '-f help' to get the available
event list") added support for '-f help'. However, the extra handling of
'help' will also take effect when 'help' is specified as a regex in
interactive mode via 'f'. This results in display of all events while
only those matching this regex should be shown.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index f133755fdde2..4faf9f85a00e 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -478,7 +478,7 @@ class Provider(object):
     @staticmethod
     def is_field_wanted(fields_filter, field):
         """Indicate whether field is valid according to fields_filter."""
-        if not fields_filter or fields_filter == "help":
+        if not fields_filter:
             return True
         return re.match(fields_filter, field) is not None
 
@@ -1567,6 +1567,7 @@ def main():
     stats = Stats(options)
 
     if options.fields == "help":
+        stats.fields_filter = None
         event_list = "\n"
         s = stats.get()
         for key in s.keys():
-- 
2.13.5

[PATCH v2 05/11] tools/kvm_stat: fix child trace events accounting

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Child trace events were included in calculation of the overall total,
which is used for calculation of the percentages of the '%Total' column.
However, the parent trace envents' stats summarize the child trace
events, hence we'd incorrectly account for them twice, leading to
slightly wrong stats.
With this fix, we use the correct total. Consequently, the sum of the
child trace events' '%Total' column values is identical to the
respective value of the respective parent event. However, this also
means that the sum of the '%Total' column values will aggregate to more
than 100 percent.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat     | 6 +++---
 tools/kvm/kvm_stat/kvm_stat.txt | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 4faf9f85a00e..90f0445d7808 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1092,14 +1092,14 @@ class Tui(object):
             # sort by totals
             return (0, -stats[x][0])
         total = 0.
-        for val in stats.values():
-            total += val[0]
+        for key in stats.keys():
+            if key.find('(') is -1:
+                total += stats[key][0]
         if self._sorting == SORT_DEFAULT:
             sortkey = sortCurAvg
         else:
             sortkey = sortTotal
         for key in sorted(stats.keys(), key=sortkey):
-
             if row >= self.screen.getmaxyx()[0]:
                 break
             values = stats[key]
diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index e5cf836be8a1..75368a3c285f 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -50,6 +50,8 @@ INTERACTIVE COMMANDS
 *s*::   set update interval
 
 *x*::	toggle reporting of stats for child trace events
+ ::     *Note*: The stats for the parents summarize the respective child trace
+                events
 
 Press any other key to refresh statistics immediately.
 
-- 
2.13.5

[PATCH v2 06/11] tools/kvm_stat: add hint on '-f help' to man page

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

The man page update for this new functionality was omitted.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
index 75368a3c285f..b5b3810c9e94 100644
--- a/tools/kvm/kvm_stat/kvm_stat.txt
+++ b/tools/kvm/kvm_stat/kvm_stat.txt
@@ -88,7 +88,7 @@ OPTIONS
 
 -f<fields>::
 --fields=<fields>::
-	fields to display (regex)
+	fields to display (regex), "-f help" for a list of available events
 
 -h::
 --help::
-- 
2.13.5

[PATCH v2 07/11] tools/kvm_stat: handle invalid regular expressions

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Passing an invalid regular expression on the command line results in a
traceback. Note that interactive specification of invalid regular
expressions is not affected
To reproduce, run "kvm_stat -f '*'".

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 90f0445d7808..29c56f3a05dc 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1521,6 +1521,13 @@ Press any other key to refresh statistics immediately.
                          callback=cb_guest_to_pid,
                          )
     (options, _) = optparser.parse_args(sys.argv)
+    try:
+        # verify that we were passed a valid regex up front
+        re.compile(options.fields)
+    except re.error:
+        sys.exit('Error: "' + options.fields + '" is not a valid regular '
+                 'expression')
+
     return options
 
 
-- 
2.13.5

[PATCH v2 08/11] tools/kvm_stat: suppress usage information on command line errors

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Errors while parsing the '-g' command line argument result in display of
usage information prior to the error message. This is a bit confusing,
as the command line is syntactically correct.
To reproduce, run 'kvm_stat -g' and specify a non-existing or inactive
guest.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 29c56f3a05dc..6ca18d15488e 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1453,16 +1453,13 @@ Press any other key to refresh statistics immediately.
         try:
             pids = Tui.get_pid_from_gname(val)
         except:
-            raise optparse.OptionValueError('Error while searching for guest '
-                                            '"{}", use "-p" to specify a pid '
-                                            'instead'.format(val))
+            sys.exit('Error while searching for guest "{}", use "-p" to '
+                     'specify a pid instead'.format(val))
         if len(pids) == 0:
-            raise optparse.OptionValueError('No guest by the name "{}" '
-                                            'found'.format(val))
+            sys.exit('Error: No guest by the name "{}" found'.format(val))
         if len(pids) > 1:
-            raise optparse.OptionValueError('Multiple processes found (pids: '
-                                            '{}) - use "-p" to specify a pid '
-                                            'instead'.format(" ".join(pids)))
+            sys.exit('Error: Multiple processes found (pids: {}) - use "-p" '
+                     'to specify a pid instead'.format(" ".join(pids)))
         parser.values.pid = pids[0]
 
     optparser = optparse.OptionParser(description=description_text,
-- 
2.13.5

[PATCH v2 09/11] tools/kvm_stat: stop ignoring unhandled arguments

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Unhandled arguments, which could easily include typos, are simply
ignored. We should be strict to avoid undetected typos.
To reproduce start kvm_stat with an extra argument, e.g.
'kvm_stat -d bnuh5ol' and note that this will actually work.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 6ca18d15488e..42c34b8818f7 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1517,7 +1517,9 @@ Press any other key to refresh statistics immediately.
                          help='restrict statistics to guest by name',
                          callback=cb_guest_to_pid,
                          )
-    (options, _) = optparser.parse_args(sys.argv)
+    options, ukn = optparser.parse_args(sys.argv)
+    if len(ukn) != 1:
+        sys.exit('Error: Unknown argument(s): ' + ', '.join(ukn[-2:]))
     try:
         # verify that we were passed a valid regex up front
         re.compile(options.fields)
-- 
2.13.5

[PATCH v2 10/11] tools/kvm_stat: sort '-f help' output

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Sort the fields returned by specifying '-f help' on the command line.
While at it, simplify the code a bit, indent the output and eliminate an
extra blank line at the beginning.

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 42c34b8818f7..929c8379d82a 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -33,6 +33,7 @@ import resource
 import struct
 import re
 import subprocess
+from sets import Set
 from collections import defaultdict
 
 VMX_EXIT_REASONS = {
@@ -1572,17 +1573,14 @@ def main():
 
     stats = Stats(options)
 
-    if options.fields == "help":
+    if options.fields == 'help':
         stats.fields_filter = None
-        event_list = "\n"
-        s = stats.get()
-        for key in s.keys():
-            if key.find('(') != -1:
-                key = key[0:key.find('(')]
-            if event_list.find('\n' + key + '\n') == -1:
-                event_list += key + '\n'
-        sys.stdout.write(event_list)
-        return ""
+        event_list = []
+        for key in stats.get().keys():
+            event_list.append(key.split('(', 1)[0])
+        event_list.sort()
+        sys.stdout.write('  ' + '\n  '.join(sorted(Set(event_list))) + '\n')
+        sys.exit(0)
 
     if options.log:
         log(stats)
-- 
2.13.5

[PATCH v2 11/11] tools/kvm_stat: add line for totals

[Stefan Raspl]

From: Stefan Raspl <stefan.raspl@de.ibm.com>

Add a line for the total number of events and current average at the
bottom of the body.
Note that both values exclude child trace events. I.e. if drilldown is
activated via interactive command 'x', only the totals are accounted, or
we'd be counting these twice (see previous commit "tools/kvm_stat: fix
child trace events accounting").

Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
---
 tools/kvm/kvm_stat/kvm_stat | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index 929c8379d82a..1094637b312c 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1100,8 +1100,9 @@ class Tui(object):
             sortkey = sortCurAvg
         else:
             sortkey = sortTotal
+        tavg = 0
         for key in sorted(stats.keys(), key=sortkey):
-            if row >= self.screen.getmaxyx()[0]:
+            if row >= self.screen.getmaxyx()[0] - 1:
                 break
             values = stats[key]
             if not values[0] and not values[1]:
@@ -1113,9 +1114,15 @@ class Tui(object):
                 self.screen.addstr(row, 1, '%-40s %10d%7.1f %8s' %
                                    (key, values[0], values[0] * 100 / total,
                                     cur))
+                if cur is not '' and key.find('(') is -1:
+                    tavg += cur
             row += 1
         if row == 3:
             self.screen.addstr(4, 1, 'No matching events reported yet')
+        else:
+            self.screen.addstr(row, 1, '%-40s %10d        %8s' %
+                               ('Total', total, tavg if tavg else ''),
+                               curses.A_BOLD)
         self.screen.refresh()
 
     def show_msg(self, text):
-- 
2.13.5

Re: [PATCH v2 10/11] tools/kvm_stat: sort '-f help' output

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1968 bytes --]

On 11.12.2017 12:25, Stefan Raspl wrote:
> From: Stefan Raspl <stefan.raspl@de.ibm.com>
> 
> Sort the fields returned by specifying '-f help' on the command line.
> While at it, simplify the code a bit, indent the output and eliminate an
> extra blank line at the beginning.
> 
> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
> ---
>  tools/kvm/kvm_stat/kvm_stat | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
> index 42c34b8818f7..929c8379d82a 100755
> --- a/tools/kvm/kvm_stat/kvm_stat
> +++ b/tools/kvm/kvm_stat/kvm_stat
> @@ -33,6 +33,7 @@ import resource
>  import struct
>  import re
>  import subprocess
> +from sets import Set

What's the reason for this import, shouldn't set be built in from at
least 2.7 on? It even seems to be 2.4. The module should be deprecated
from 2.6 on.

>  from collections import defaultdict
> 
>  VMX_EXIT_REASONS = {
> @@ -1572,17 +1573,14 @@ def main():
> 
>      stats = Stats(options)
> 
> -    if options.fields == "help":
> +    if options.fields == 'help':
>          stats.fields_filter = None
> -        event_list = "\n"
> -        s = stats.get()
> -        for key in s.keys():
> -            if key.find('(') != -1:
> -                key = key[0:key.find('(')]
> -            if event_list.find('\n' + key + '\n') == -1:
> -                event_list += key + '\n'
> -        sys.stdout.write(event_list)
> -        return ""
> +        event_list = []
> +        for key in stats.get().keys():
> +            event_list.append(key.split('(', 1)[0])

Definitely better.

> +        event_list.sort()
> +        sys.stdout.write('  ' + '\n  '.join(sorted(Set(event_list))) + '\n')

You just sorted the list, why do you need a set and why do you sort it
again?

> +        sys.exit(0)
> 
>      if options.log:
>          log(stats)
> 




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 10/11] tools/kvm_stat: sort '-f help' output

[Stefan Raspl]

On 11.12.2017 13:20, Janosch Frank wrote:
> On 11.12.2017 12:25, Stefan Raspl wrote:
>> From: Stefan Raspl <stefan.raspl@de.ibm.com>
>>
>> Sort the fields returned by specifying '-f help' on the command line.
>> While at it, simplify the code a bit, indent the output and eliminate an
>> extra blank line at the beginning.
>>
>> Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>
>> ---
>>  tools/kvm/kvm_stat/kvm_stat | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
>> index 42c34b8818f7..929c8379d82a 100755
>> --- a/tools/kvm/kvm_stat/kvm_stat
>> +++ b/tools/kvm/kvm_stat/kvm_stat
>> @@ -33,6 +33,7 @@ import resource
>>  import struct
>>  import re
>>  import subprocess
>> +from sets import Set
> 
> What's the reason for this import, shouldn't set be built in from at
> least 2.7 on? It even seems to be 2.4. The module should be deprecated
> from 2.6 on.

without that line:
$ python --version
Python 2.7.12
$ ./kvm_stat -f help
Traceback (most recent call last):
  File "./kvm_stat", line 1600, in <module>
    main()
  File "./kvm_stat", line 1588, in main
    sys.stdout.write('  ' + '\n  '.join(sorted(Set(event_list))) + '\n')
NameError: global name 'Set' is not defined
>> +        event_list.sort()
>> +        sys.stdout.write('  ' + '\n  '.join(sorted(Set(event_list))) + '\n')
> 
> You just sorted the list, why do you need a set and why do you sort it
> again?

Oooops - my bad, we can drop the first sort.

Thx!

Re: [PATCH v2 10/11] tools/kvm_stat: sort '-f help' output

[Stefan Raspl]

On 11.12.2017 13:20, Janosch Frank wrote:
> On 11.12.2017 12:25, Stefan Raspl wrote:
>> diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
>> index 42c34b8818f7..929c8379d82a 100755
>> --- a/tools/kvm/kvm_stat/kvm_stat
>> +++ b/tools/kvm/kvm_stat/kvm_stat
>> @@ -33,6 +33,7 @@ import resource
>>  import struct
>>  import re
>>  import subprocess
>> +from sets import Set
> 
> What's the reason for this import, shouldn't set be built in from at
> least 2.7 on? It even seems to be 2.4. The module should be deprecated
> from 2.6 on.
> 
>> +        event_list.sort()
>> +        sys.stdout.write('  ' + '\n  '.join(sorted(Set(event_list))) + '\n')

Ah, now I know what mean: Use the built-in 'set' instead of 'Set'?
Yup, good point, will do.

Ciao,
Stefan

Re: [PATCH v2 08/11] tools/kvm_stat: suppress usage information on command line errors

[Paolo Bonzini]

On 11/12/2017 12:25, Stefan Raspl wrote:
>          except:
> -            raise optparse.OptionValueError('Error while searching for guest '
> -                                            '"{}", use "-p" to specify a pid '
> -                                            'instead'.format(val))
> +            sys.exit('Error while searching for guest "{}", use "-p" to '
> +                     'specify a pid instead'.format(val))

Maybe:

Error while searching for guest "{}". Use "-p" to specify a pid instead?

Thanks,

Paolo

Re: [PATCH v2 09/11] tools/kvm_stat: stop ignoring unhandled arguments

[Paolo Bonzini]

On 11/12/2017 12:25, Stefan Raspl wrote:
>                           )
> -    (options, _) = optparser.parse_args(sys.argv)
> +    options, ukn = optparser.parse_args(sys.argv)
> +    if len(ukn) != 1:
> +        sys.exit('Error: Unknown argument(s): ' + ', '.join(ukn[-2:]))

I'm replacing this with "unkn".  But, why "-2:" instead of "1:"?

Thanks,

Paolo

Re: [PATCH v2 00/11] tools/kvm_stat: misc fixes

[Paolo Bonzini]

On 11/12/2017 12:25, Stefan Raspl wrote:
> Fixed a few more glitches, most of which should be pretty straight forward.
> I'm not happy with what the man page looks like, in that the "INTERACTIVE
> COMMANDS" section's line width is way too small. I noticed this in
> particular with 5/11 "tools/kvm_stat: fix child trace events accounting",
> but couldn't find a way to get ASCIIDOC to widen those lines a bit.
> 
> 
> Stefan Raspl (11):
>   tools/kvm_stat: fix command line option '-g'
>   tools/kvm_stat: fix drilldown in events-by-guests mode
>   tools/kvm_stat: fix missing field update after filter change
>   tools/kvm_stat: fix extra handling of 'help' with fields filter
>   tools/kvm_stat: fix child trace events accounting
>   tools/kvm_stat: add hint on '-f help' to man page
>   tools/kvm_stat: handle invalid regular expressions
>   tools/kvm_stat: suppress usage information on command line errors
>   tools/kvm_stat: stop ignoring unhandled arguments
>   tools/kvm_stat: sort '-f help' output
>   tools/kvm_stat: add line for totals
> 
>  tools/kvm/kvm_stat/kvm_stat     | 76 ++++++++++++++++++++++++-----------------
>  tools/kvm/kvm_stat/kvm_stat.txt |  4 ++-
>  2 files changed, 48 insertions(+), 32 deletions(-)
> 

Applying all except patch 10, please double check -2: vs 1: in patch 9.

Paolo

Re: [PATCH v2 09/11] tools/kvm_stat: stop ignoring unhandled arguments

[Stefan Raspl]

On 12.12.2017 16:56, Paolo Bonzini wrote:
> On 11/12/2017 12:25, Stefan Raspl wrote:
>>                           )
>> -    (options, _) = optparser.parse_args(sys.argv)
>> +    options, ukn = optparser.parse_args(sys.argv)
>> +    if len(ukn) != 1:
>> +        sys.exit('Error: Unknown argument(s): ' + ', '.join(ukn[-2:]))
> 
> I'm replacing this with "unkn".  But, why "-2:" instead of "1:"?

Yes, that needs to be a 1: indeed - not sure what I was thinking :O

Ciao,
Stefan

Re: [PATCH v2 09/11] tools/kvm_stat: stop ignoring unhandled arguments

[Paolo Bonzini]

On 12/12/2017 19:04, Stefan Raspl wrote:
> On 12.12.2017 16:56, Paolo Bonzini wrote:
>> On 11/12/2017 12:25, Stefan Raspl wrote:
>>>                           )
>>> -    (options, _) = optparser.parse_args(sys.argv)
>>> +    options, ukn = optparser.parse_args(sys.argv)
>>> +    if len(ukn) != 1:
>>> +        sys.exit('Error: Unknown argument(s): ' + ', '.join(ukn[-2:]))
>>
>> I'm replacing this with "unkn".  But, why "-2:" instead of "1:"?
> 
> Yes, that needs to be a 1: indeed - not sure what I was thinking :O

Fine then. :)

Paolo

Re: [PATCH v2 08/11] tools/kvm_stat: suppress usage information on command line errors

[Stefan Raspl]

On 12.12.2017 16:53, Paolo Bonzini wrote:
> On 11/12/2017 12:25, Stefan Raspl wrote:
>>          except:
>> -            raise optparse.OptionValueError('Error while searching for guest '
>> -                                            '"{}", use "-p" to specify a pid '
>> -                                            'instead'.format(val))
>> +            sys.exit('Error while searching for guest "{}", use "-p" to '
>> +                     'specify a pid instead'.format(val))
> 
> Maybe:
> 
> Error while searching for guest "{}". Use "-p" to specify a pid instead?

Yeah - 2 short sentences instead of a single longer one...why not?

Ciao,
Stefan

Re: [PATCH v2 10/11] tools/kvm_stat: sort '-f help' output

[Stefan Raspl]

Here's a new version of this patch, addressing the feedback.

Ciao,
Stefan


--------------------------------------------

From: Stefan Raspl <stefan.raspl@de.ibm.com>

    tools/kvm_stat: sort '-f help' output

    Sort the fields returned by specifying '-f help' on the command line.
    While at it, simplify the code a bit, indent the output and eliminate an
    extra blank line at the beginning.

    Signed-off-by: Stefan Raspl <raspl@linux.vnet.ibm.com>

diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat
index e37ea70..99c81e0 100755
--- a/tools/kvm/kvm_stat/kvm_stat
+++ b/tools/kvm/kvm_stat/kvm_stat
@@ -1579,17 +1579,13 @@ def main():

     stats = Stats(options)

-    if options.fields == "help":
+    if options.fields == 'help':
         stats.fields_filter = None
-        event_list = "\n"
-        s = stats.get()
-        for key in s.keys():
-            if key.find('(') != -1:
-                key = key[0:key.find('(')]
-            if event_list.find('\n' + key + '\n') == -1:
-                event_list += key + '\n'
-        sys.stdout.write(event_list)
-        return ""
+        event_list = []
+        for key in stats.get().keys():
+            event_list.append(key.split('(', 1)[0])
+        sys.stdout.write('  ' + '\n  '.join(sorted(set(event_list))) + '\n')
+        sys.exit(0)

     if options.log:
         log(stats)