| Message ID | 20170220154211.11882-5-raspl@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/02/2017 16:41, Stefan Raspl wrote: > @@ -339,8 +338,7 @@ def get_filters(): > filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) > return filters > > -libc = ctypes.CDLL('libc.so.6', use_errno=True) > -syscall = libc.syscall > +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall > > class perf_event_attr(ctypes.Structure): > """Struct that holds the necessary data to set up a trace event. > @@ -950,11 +948,10 @@ class Tui(object): > while True: > self.refresh(sleeptime) > curses.halfdelay(int(sleeptime * 10)) > - sleeptime = 3 > + sleeptime = 3. > try: > char = self.screen.getkey() > if char == 'x': > - self.drilldown = not self.drilldown > self.update_drilldown() > if char == 'q': > break I'm not sure I understand the point of these; the rest is fine. Paolo
On 09.03.2017 17:51, Paolo Bonzini wrote: > > > On 20/02/2017 16:41, Stefan Raspl wrote: >> @@ -339,8 +338,7 @@ def get_filters(): >> filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) >> return filters >> >> -libc = ctypes.CDLL('libc.so.6', use_errno=True) >> -syscall = libc.syscall >> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall >> >> class perf_event_attr(ctypes.Structure): >> """Struct that holds the necessary data to set up a trace event. >> @@ -950,11 +948,10 @@ class Tui(object): >> while True: >> self.refresh(sleeptime) >> curses.halfdelay(int(sleeptime * 10)) >> - sleeptime = 3 >> + sleeptime = 3. >> try: >> char = self.screen.getkey() >> if char == 'x': >> - self.drilldown = not self.drilldown >> self.update_drilldown() >> if char == 'q': >> break > > I'm not sure I understand the point of these; the rest is fine. 'sleeptime' starts out as a float (sleeptime = 0.25), but is here re-defined to an int - so we make it float all the way. The variable 'drilldown' is never used, so we remove its initialization in __init__() and the sole place where it is ever used, which is the line above. Ciao, Stefan
----- Original Message ----- > From: "Stefan Raspl" <raspl@linux.vnet.ibm.com> > To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org > Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com > Sent: Friday, March 10, 2017 7:04:46 AM > Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches > > On 09.03.2017 17:51, Paolo Bonzini wrote: > > > > > > On 20/02/2017 16:41, Stefan Raspl wrote: > >> @@ -339,8 +338,7 @@ def get_filters(): > >> filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) > >> return filters > >> > >> -libc = ctypes.CDLL('libc.so.6', use_errno=True) > >> -syscall = libc.syscall > >> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall > >> > >> class perf_event_attr(ctypes.Structure): > >> """Struct that holds the necessary data to set up a trace event. > >> @@ -950,11 +948,10 @@ class Tui(object): > >> while True: > >> self.refresh(sleeptime) > >> curses.halfdelay(int(sleeptime * 10)) > >> - sleeptime = 3 > >> + sleeptime = 3. > >> try: > >> char = self.screen.getkey() > >> if char == 'x': > >> - self.drilldown = not self.drilldown > >> self.update_drilldown() > >> if char == 'q': > >> break > > > > I'm not sure I understand the point of these; the rest is fine. > > 'sleeptime' starts out as a float (sleeptime = 0.25), but is here > re-defined to an int - so we make it float all the way. > The variable 'drilldown' is never used, so we remove its > initialization in __init__() and the sole place where it is ever > used, which is the line above. Yes, I was referring to libc and sleeptime. I don't like the "3.", especially since Python 3 has "3/2" return a float. Is this a PEP8 complaint? Does it still complain if you do "from __future__ import division"? Paolo
On 10.03.2017 09:14, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com> >> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org >> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com >> Sent: Friday, March 10, 2017 7:04:46 AM >> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches >> >> On 09.03.2017 17:51, Paolo Bonzini wrote: >>> >>> >>> On 20/02/2017 16:41, Stefan Raspl wrote: >>>> @@ -339,8 +338,7 @@ def get_filters(): >>>> filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) >>>> return filters >>>> >>>> -libc = ctypes.CDLL('libc.so.6', use_errno=True) >>>> -syscall = libc.syscall >>>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall >>>> >>>> class perf_event_attr(ctypes.Structure): >>>> """Struct that holds the necessary data to set up a trace event. >>>> @@ -950,11 +948,10 @@ class Tui(object): >>>> while True: >>>> self.refresh(sleeptime) >>>> curses.halfdelay(int(sleeptime * 10)) >>>> - sleeptime = 3 >>>> + sleeptime = 3. >>>> try: >>>> char = self.screen.getkey() >>>> if char == 'x': >>>> - self.drilldown = not self.drilldown >>>> self.update_drilldown() >>>> if char == 'q': >>>> break >>> >>> I'm not sure I understand the point of these; the rest is fine. >> >> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here >> re-defined to an int - so we make it float all the way. >> The variable 'drilldown' is never used, so we remove its >> initialization in __init__() and the sole place where it is ever >> used, which is the line above. > > Yes, I was referring to libc and sleeptime. I don't like the > "3.", especially since Python 3 has "3/2" return a float. Is > this a PEP8 complaint? Does it still complain if you do > "from __future__ import division"? Ah, sorry, missed the first chunk: That was to eliminate unused variable 'libc'. As for the "3.": I noticed through pylint, PEP8 is fine. Seemed valid to me, but in the grand scheme of things it certainly doesn't matter (and wasn't aware it becomes moot in Python 3), so I'd take it out if you want. Sidenote: I only ever hear the complaint about the '3.' from kernel folks, seemed quite common to me elsewhere - maybe 'cause there's usually no floats in the kernel?! But as I said, I'd rather take it out altogether in here instead of adding more imports, and fix 07/17 to redefine to 3 (instead of 3.), OK? Ciao, Stefan
On 10/03/2017 09:33, Stefan Raspl wrote: > On 10.03.2017 09:14, Paolo Bonzini wrote: >> >> >> ----- Original Message ----- >>> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com> >>> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org >>> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com >>> Sent: Friday, March 10, 2017 7:04:46 AM >>> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches >>> >>> On 09.03.2017 17:51, Paolo Bonzini wrote: >>>> >>>> >>>> On 20/02/2017 16:41, Stefan Raspl wrote: >>>>> @@ -339,8 +338,7 @@ def get_filters(): >>>>> filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) >>>>> return filters >>>>> >>>>> -libc = ctypes.CDLL('libc.so.6', use_errno=True) >>>>> -syscall = libc.syscall >>>>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall >>>>> >>>>> class perf_event_attr(ctypes.Structure): >>>>> """Struct that holds the necessary data to set up a trace event. >>>>> @@ -950,11 +948,10 @@ class Tui(object): >>>>> while True: >>>>> self.refresh(sleeptime) >>>>> curses.halfdelay(int(sleeptime * 10)) >>>>> - sleeptime = 3 >>>>> + sleeptime = 3. >>>>> try: >>>>> char = self.screen.getkey() >>>>> if char == 'x': >>>>> - self.drilldown = not self.drilldown >>>>> self.update_drilldown() >>>>> if char == 'q': >>>>> break >>>> >>>> I'm not sure I understand the point of these; the rest is fine. >>> >>> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here >>> re-defined to an int - so we make it float all the way. >>> The variable 'drilldown' is never used, so we remove its >>> initialization in __init__() and the sole place where it is ever >>> used, which is the line above. >> >> Yes, I was referring to libc and sleeptime. I don't like the >> "3.", especially since Python 3 has "3/2" return a float. Is >> this a PEP8 complaint? Does it still complain if you do >> "from __future__ import division"? > > > Ah, sorry, missed the first chunk: That was to eliminate unused > variable 'libc'. It's not unused, it's used on the following line. :) > As for the "3.": I noticed through pylint, PEP8 is fine. Seemed > valid to me, but in the grand scheme of things it certainly doesn't > matter (and wasn't aware it becomes moot in Python 3), so I'd take > it out if you want. I think it's a valid complaint with Python 2 division. If pylint complaints even with "from __future__ import division", it would be a pylint bug in my opinion. Maybe it's just me... I wouldn't have complained about "3.0" for example. :) Paolo > Sidenote: I only ever hear the complaint about the '3.' from kernel > folks, seemed quite common to me elsewhere - maybe 'cause there's > usually no floats in the kernel?! But as I said, I'd rather take it > out altogether in here instead of adding more imports, and fix 07/17 > to redefine to 3 (instead of 3.), OK?
On 10.03.2017 09:38, Paolo Bonzini wrote: > > > On 10/03/2017 09:33, Stefan Raspl wrote: >> On 10.03.2017 09:14, Paolo Bonzini wrote: >>> >>> >>> ----- Original Message ----- >>>> From: "Stefan Raspl" <raspl@linux.vnet.ibm.com> >>>> To: "Paolo Bonzini" <pbonzini@redhat.com>, kvm@vger.kernel.org >>>> Cc: rkrcmar@redhat.com, frankja@linux.vnet.ibm.com >>>> Sent: Friday, March 10, 2017 7:04:46 AM >>>> Subject: Re: [PATCH 04/17] tools/kvm_stat: fix misc glitches >>>> >>>> On 09.03.2017 17:51, Paolo Bonzini wrote: >>>>> >>>>> >>>>> On 20/02/2017 16:41, Stefan Raspl wrote: >>>>>> @@ -339,8 +338,7 @@ def get_filters(): >>>>>> filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) >>>>>> return filters >>>>>> >>>>>> -libc = ctypes.CDLL('libc.so.6', use_errno=True) >>>>>> -syscall = libc.syscall >>>>>> +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall >>>>>> >>>>>> class perf_event_attr(ctypes.Structure): >>>>>> """Struct that holds the necessary data to set up a trace event. >>>>>> @@ -950,11 +948,10 @@ class Tui(object): >>>>>> while True: >>>>>> self.refresh(sleeptime) >>>>>> curses.halfdelay(int(sleeptime * 10)) >>>>>> - sleeptime = 3 >>>>>> + sleeptime = 3. >>>>>> try: >>>>>> char = self.screen.getkey() >>>>>> if char == 'x': >>>>>> - self.drilldown = not self.drilldown >>>>>> self.update_drilldown() >>>>>> if char == 'q': >>>>>> break >>>>> >>>>> I'm not sure I understand the point of these; the rest is fine. >>>> >>>> 'sleeptime' starts out as a float (sleeptime = 0.25), but is here >>>> re-defined to an int - so we make it float all the way. >>>> The variable 'drilldown' is never used, so we remove its >>>> initialization in __init__() and the sole place where it is ever >>>> used, which is the line above. >>> >>> Yes, I was referring to libc and sleeptime. I don't like the >>> "3.", especially since Python 3 has "3/2" return a float. Is >>> this a PEP8 complaint? Does it still complain if you do >>> "from __future__ import division"? >> >> >> Ah, sorry, missed the first chunk: That was to eliminate unused >> variable 'libc'. > > It's not unused, it's used on the following line. :) Sorry, don't want to get annoying: Yeah, right, so I was eliminating that variable - do you want me to remove the chunk from the patch? >> As for the "3.": I noticed through pylint, PEP8 is fine. Seemed >> valid to me, but in the grand scheme of things it certainly doesn't >> matter (and wasn't aware it becomes moot in Python 3), so I'd take >> it out if you want. > > I think it's a valid complaint with Python 2 division. If pylint > complaints even with "from __future__ import division", it would be a > pylint bug in my opinion. > > Maybe it's just me... I wouldn't have complained about "3.0" for example. :) Tried the import, pylint still complains :O So: Just switch to 3.0 here and in 07/17, and repost? Ciao, Stefan
On 10/03/2017 10:42, Stefan Raspl wrote: >>> Ah, sorry, missed the first chunk: That was to eliminate unused >>> variable 'libc'. >> It's not unused, it's used on the following line. :) > Sorry, don't want to get annoying: Yeah, right, so I was eliminating > that variable - do you want me to remove the chunk from the patch? Yes, please. >>> As for the "3.": I noticed through pylint, PEP8 is fine. Seemed >>> valid to me, but in the grand scheme of things it certainly doesn't >>> matter (and wasn't aware it becomes moot in Python 3), so I'd take >>> it out if you want. >> I think it's a valid complaint with Python 2 division. If pylint >> complaints even with "from __future__ import division", it would be a >> pylint bug in my opinion. >> >> Maybe it's just me... I wouldn't have complained about "3.0" for example. :) > Tried the import, pylint still complains :O > So: Just switch to 3.0 here and in 07/17, and repost? And this too. Thanks, Paolo
diff --git a/tools/kvm/kvm_stat/kvm_stat b/tools/kvm/kvm_stat/kvm_stat index 14536c0..a09179e 100755 --- a/tools/kvm/kvm_stat/kvm_stat +++ b/tools/kvm/kvm_stat/kvm_stat @@ -31,7 +31,6 @@ import resource import struct import re from collections import defaultdict -from time import sleep VMX_EXIT_REASONS = { 'EXCEPTION_NMI': 0, @@ -339,8 +338,7 @@ def get_filters(): filters['kvm_exit'] = ('exit_reason', ARCH.exit_reasons) return filters -libc = ctypes.CDLL('libc.so.6', use_errno=True) -syscall = libc.syscall +syscall = ctypes.CDLL('libc.so.6', use_errno=True).syscall class perf_event_attr(ctypes.Structure): """Struct that holds the necessary data to set up a trace event. @@ -657,6 +655,7 @@ class DebugfsProvider(object): self._fields = self.get_available_fields() self._pid = 0 self.do_read = True + self.paths = [] def get_available_fields(self): """"Returns a list of available fields. @@ -794,7 +793,6 @@ class Tui(object): def __init__(self, stats): self.stats = stats self.screen = None - self.drilldown = False self.update_drilldown() def __enter__(self): @@ -950,11 +948,10 @@ class Tui(object): while True: self.refresh(sleeptime) curses.halfdelay(int(sleeptime * 10)) - sleeptime = 3 + sleeptime = 3. try: char = self.screen.getkey() if char == 'x': - self.drilldown = not self.drilldown self.update_drilldown() if char == 'q': break @@ -1064,12 +1061,12 @@ Requirements: help='fields to display (regex)', ) optparser.add_option('-p', '--pid', - action='store', - default=0, - type=int, - dest='pid', - help='restrict statistics to pid', - ) + action='store', + default=0, + type='int', + dest='pid', + help='restrict statistics to pid', + ) (options, _) = optparser.parse_args(sys.argv) return options @@ -1099,8 +1096,8 @@ def check_access(options): "Also ensure, that the kvm modules are loaded.\n") sys.exit(1) - if not os.path.exists(PATH_DEBUGFS_TRACING) and (options.tracepoints - or not options.debugfs): + if not os.path.exists(PATH_DEBUGFS_TRACING) and (options.tracepoints or + not options.debugfs): sys.stderr.write("Please enable CONFIG_TRACING in your kernel " "when using the option -t (default).\n" "If it is enabled, make {0} readable by the " @@ -1111,7 +1108,7 @@ def check_access(options): sys.stderr.write("Falling back to debugfs statistics!\n") options.debugfs = True - sleep(5) + time.sleep(5) return options