It is very strange, I already use the pep8, the version of my pep8 is 1.7.0, does this version not worked? About the comments, I think I misunderstood the Jan's remarks. He ask me put the updates information, I thought only put the updated messge, Ok, I will put all of the information plus the updates.
2016-07-02 1:08 GMT+08:00 Ralf Ramsauer <[email protected]>: > > On 07/01/16 05:57, Xuguo Wang wrote: >> Some comments of code are redundant, since the codes are >> self-explanatory, so omit them. > This message explains the changes were made from V4 to V5, but not the > commit message that should actually appear in the repository. >> >> Signed-off-by: Xuguo Wang <[email protected]> >> --- >> tools/jailhouse-config-create | 180 >> +++++++++++++++++++++++++++++++++--------- >> 1 file changed, 143 insertions(+), 37 deletions(-) >> >> diff --git a/tools/jailhouse-config-create b/tools/jailhouse-config-create >> index 4e61abc..9fe7f96 100755 >> --- a/tools/jailhouse-config-create >> +++ b/tools/jailhouse-config-create >> @@ -368,22 +368,30 @@ class PCIPCIBridge(PCIDevice): >> return (secondbus, subordinate) >> >> >> -class MemRegion: >> - def __init__(self, start, stop, typestr, comments=None): >> +class IORegion: >> + def __init__(self, start, stop, typestr): >> self.start = start >> self.stop = stop >> self.typestr = typestr >> + >> + >> +# Each IOMemRegion represents one of the /proc/iomem entries. >> +# It extends IORegion by the typestr property. > Does it? IORegion already has the 'typestr' member. It extends IORegion > by the 'comments' variable. My fault, I missed this message. This time I think carefully, Maybe this message is redundant, since the function already self-explanatory. >> +class IOMemRegion(IORegion): >> + def __init__(self, start, stop, typestr, comments=None): >> + >> if comments is None: >> self.comments = [] >> else: >> self.comments = comments > These lines already changed in next, your patch should apply against > next. Sorry that I didn't mention that earlier, otherwise Jan only gets > conflicts when merging. >> + IORegion.__init__(self, start, stop, typestr) >> >> def __str__(self): >> - return 'MemRegion: %08x-%08x : %s' % \ >> + return 'IOMemRegion: %08x-%08x : %s' % \ >> (self.start, self.stop, self.typestr) >> >> def size(self): >> - # round up to full PAGE_SIZE >> + # round up to PAGE_SIZE >> return int((self.stop - self.start + 0xfff) / 0x1000) * 0x1000 >> >> def flagstr(self, p=''): >> @@ -400,6 +408,19 @@ class MemRegion: >> return 'JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE' >> >> >> +# Each IOPortRegion represents one of the /proc/ioports entries. >> +# It extends IORegion typestr priority. > Really? IOPortRegion extends IORegion by adding the 'value' member variable. My fault, I missed this message. This time I think carefully, Maybe this message is redundant, since the function already self-explanatory. >> +# the value == 0 means permit, 1 means intercept. >> +class IOPortRegion(IORegion): >> + def __init__(self, start, stop, typestr, value=0): >> + self.value = value >> + IORegion.__init__(self, start, stop, typestr) >> + >> + def size(self): >> + # the range is from start to stop including self.start >> + return self.stop - self.start + 1 >> + >> + >> class IOAPIC: >> def __init__(self, id, address, gsi_base, iommu=0, bdf=0): >> self.id = id >> @@ -416,7 +437,11 @@ class IOAPIC: >> return (self.iommu << 16) | self.bdf >> >> >> -class IOMemRegionTree: >> +class IORegionTree: >> + # This regex matches entries in /proc/ioports, like > ... in /proc/ioports or /proc/iomem ... > We use this class and its parse_line static method for both, ioports and > iomem. Mention /proc/iomem to avoid misunderstandings. ok, got it. >> + # ' 0020-0021 : pic1' >> + IOEntry_Regex = re.compile('( *)([0-9a-f]*)-([0-9a-f]*) : (\w?.*)') >> + >> def __init__(self, region, level): >> self.region = region >> self.level = level >> @@ -434,6 +459,27 @@ class IOMemRegionTree: >> s += str(c) >> return s >> >> + @staticmethod >> + def parse_line(regionclass, line): >> + match = IORegionTree.IOEntry_Regex.match(line) >> + if not match: >> + raise ValueError('invalid entry') > Let the error message be more verbose: > 'Invalid entry: %s' % line > > But take care, I just realised, that 'line' has a trailing newline > character at the moment, I have a further comment on how to avoid > trailing '\n's later. got it. >> + >> + # blank level >> + level = int(match.group(1).count(' ') / 2) + 1 >> + # start address >> + start = match.group(2) >> + # end address >> + stop = match.group(3) >> + # comments >> + typestr = match.group(4) >> + return level, regionclass(int(start, 16), int(stop, 16), typestr) >> + >> + >> +class IOMemRegionTree(IORegionTree): >> + def __init__(self, region, level): >> + IORegionTree.__init__(self, region, level) >> + >> def regions_split_by_kernel(self): >> kernel = [x for x in self.children if >> x.region.typestr.startswith('Kernel ')] >> @@ -457,38 +503,30 @@ class IOMemRegionTree: >> >> # before Kernel if any >> if (r.start < kernel_start): >> - before_kernel = MemRegion(r.start, kernel_start - 1, s) >> + before_kernel = IOMemRegion(r.start, kernel_start - 1, s) >> >> - kernel_region = MemRegion(kernel_start, kernel_stop, "Kernel") >> + kernel_region = IOMemRegion(kernel_start, kernel_stop, "Kernel") >> >> # after Kernel if any >> if (r.stop > kernel_stop): >> - after_kernel = MemRegion(kernel_stop + 1, r.stop, s) >> + after_kernel = IOMemRegion(kernel_stop + 1, r.stop, s) >> >> return [before_kernel, kernel_region, after_kernel] >> >> @staticmethod >> - def parse_iomem_line(line): >> - a = line.split(':', 1) >> - level = int(a[0].count(' ') / 2) + 1 >> - region = a[0].split('-', 1) >> - a[1] = a[1].strip() >> - return level, MemRegion(int(region[0], 16), int(region[1], 16), >> a[1]) >> - >> - @staticmethod >> def parse_iomem_file(): >> root = IOMemRegionTree(None, 0) >> f = input_open('/proc/iomem') >> lastlevel = 0 >> lastnode = root >> for line in f: > for line in f.read().splitlines() > will avoid trailing newline characters. got it. >> - (level, r) = IOMemRegionTree.parse_iomem_line(line) >> + level, r = IOMemRegionTree.parse_line(IOMemRegion, line) >> t = IOMemRegionTree(r, level) >> - if (t.level > lastlevel): >> + if t.level > lastlevel: >> t.parent = lastnode >> - if (t.level == lastlevel): >> + if t.level == lastlevel: >> t.parent = lastnode.parent >> - if (t.level < lastlevel): >> + if t.level < lastlevel: > Why do you start to fix PEP8 issues of old code fragments? That's not > part of your actual commit. I think we should move non-functional > cosmetic changes of existing code to separate commits. > > Nevertheless - I'm using the pycharm IDE, and it complains that on > master(!!) there are > 30 warnings > 95 weak warnings > 242 typos > :-D > > Most of those warnings are redundant parentheses and in deed, the > absolute number of PEP8 warning decreases after applying your three > patches (though they introduce new ones as well). Anyway, we probably > should move PEP8, cosmetic changes and syntactic sugar to separate > commits and then fix it all at once and not just pick out some parts. That is so strange, like I said, I already use the pep8 to check, and nothing warning left, and then i submit the patch, jan already told me this before, so every time every patch, I did the pep8 check.... Strange.. Let me check my pep8 version. And first step I will check my codes use the pep8, and after these patch, I want to refactor all others pep8 warning. >> p = lastnode.parent >> while(t.level < p.level): > Like this one... >> p = p.parent >> @@ -510,11 +548,11 @@ class IOMemRegionTree: >> r = tree.region >> s = r.typestr >> >> - if (s.find('HPET') >= 0): >> + if s.find('HPET') >= 0: >> regions.append(r) >> >> # if the tree continues recurse further down ... >> - if (len(tree.children) > 0): >> + if len(tree.children) > 0: >> regions.extend(IOMemRegionTree.find_hpet_regions(tree)) >> >> return regions >> @@ -543,12 +581,12 @@ class IOMemRegionTree: >> continue >> >> # generally blacklisted, unless we find an HPET behind it >> - if (s == 'reserved'): >> + if s == 'reserved': >> regions.extend(IOMemRegionTree.find_hpet_regions(tree)) >> continue >> >> # if the tree continues recurse further down ... >> - if (len(tree.children) > 0): >> + if len(tree.children) > 0: >> regions.extend(IOMemRegionTree.parse_iomem_tree(tree)) >> continue >> >> @@ -558,6 +596,72 @@ class IOMemRegionTree: >> return regions >> >> >> +class IOPortRegionTree(IORegionTree): >> + def __init__(self, region, level): >> + IORegionTree.__init__(self, region, level) >> + >> + @staticmethod >> + def parse_ioport_file(): >> + root = IOPortRegionTree(None, 0) >> + f = input_open('/proc/ioports') >> + lastlevel = 0 >> + lastnode = root >> + for line in f: > Same here, use f.read().splitlines() to get rid of trailing \n's. got it. >> + level, r = IOPortRegionTree.parse_line(IOPortRegion, line) >> + t = IOPortRegionTree(r, level) >> + if t.level > lastlevel: >> + t.parent = lastnode >> + if t.level == lastlevel: >> + t.parent = lastnode.parent >> + if t.level < lastlevel: >> + p = lastnode.parent >> + while(t.level < p.level): > Here you even introduce new PEP8 warnings But my pep8 does not report the warning.. :(. >> + p = p.parent >> + t.parent = p.parent >> + >> + t.parent.children.append(t) >> + lastnode = t >> + lastlevel = t.level >> + f.close() >> + >> + return root >> + >> + # recurse down the tree >> + @staticmethod > I don't understand why this method has to be static. There are more > (syntactic) advantages if it is not static: > > You're calling this static function from two different locations: from > parse_ioports() and recursively from parse_ioports_tree. In any case, > you already have an IOPortRegionTree object. Why not using it? It is the old codes, the next patch I think I could refactor this. >> + def parse_ioport_tree(tree): > s/tree/self/ >> + regions = [] >> + >> + for tree in tree.children: >> + r = tree.region >> + c = r.typestr >> + >> + # intercept keyboard and PCI conf1 >> + if (c is not None and (c == 'keyboard' or c == 'PCI conf1')): > Here again, new pep8 warnings... > Even if there are already tons of pep8 issues in current code, try to > not introduce new ones :-) Here same strange... >> + r.value = IOPortRegionTree.get_first_ioport_byte( >> + r.start, r.stop) >> + >> + # if the tree continues recurse further down ... >> + if len(tree.children) > 0: >> + regions.extend(IOPortRegionTree.parse_ioport_tree(tree)) > And here you get the reward of un-staticfying the function: > regions.extend(tree.parse_ioport_tree()) > > There's a follow-up comment on this in 2/3 got it. >> + continue >> + >> + # add all remaining leaves >> + regions.append(r) >> + >> + return regions >> + >> + # get a byte of ioport region mask from start_permitted, end >> + # at the last_permitted bit. >> + @staticmethod >> + def get_first_ioport_byte(start_permitted, last_permitted): >> + byte_head = start_permitted % 8 >> + byte_tail = last_permitted % 8 >> + byte = (1 << byte_head) - 1 >> + byte |= (~((1 << (byte_tail + 1)) - 1) & 0xff) >> + byte &= 0xff >> + return int(byte) >> + >> + >> class IOMMUConfig(object): >> def __init__(self, props): >> self.base_addr = props['base_addr'] >> @@ -577,7 +681,7 @@ def parse_iomem(pcidevices): >> regions = IOMemRegionTree.parse_iomem_tree( >> IOMemRegionTree.parse_iomem_file()) >> >> - rom_region = MemRegion(0xc0000, 0xdffff, 'ROMs') >> + rom_region = IOMemRegion(0xc0000, 0xdffff, 'ROMs') >> add_rom_region = False >> >> ret = [] >> @@ -588,12 +692,12 @@ def parse_iomem(pcidevices): >> for d in pcidevices: >> if d.msix_address >= r.start and d.msix_address <= r.stop: >> if d.msix_address > r.start: >> - head_r = MemRegion(r.start, d.msix_address - 1, >> - r.typestr, r.comments) >> + head_r = IOMemRegion(r.start, d.msix_address - 1, >> + r.typestr, r.comments) >> ret.append(head_r) >> if d.msix_address + d.msix_region_size < r.stop: >> - tail_r = MemRegion(d.msix_address + d.msix_region_size, >> - r.stop, r.typestr, r.comments) >> + tail_r = IOMemRegion(d.msix_address + >> d.msix_region_size, >> + r.stop, r.typestr, r.comments) >> ret.append(tail_r) >> append_r = False >> break >> @@ -667,11 +771,12 @@ def alloc_mem(regions, size): >> r.stop + 1 >= mem[0] + mem[1] >> ): >> if r.start < mem[0]: >> - head_r = MemRegion(r.start, mem[0] - 1, r.typestr, >> r.comments) >> + head_r = IOMemRegion(r.start, mem[0] - 1, >> + r.typestr, r.comments) >> regions.insert(regions.index(r), head_r) >> if r.stop + 1 > mem[0] + mem[1]: >> - tail_r = MemRegion(mem[0] + mem[1], r.stop, r.typestr, >> - r.comments) >> + tail_r = IOMemRegion(mem[0] + mem[1], r.stop, >> + r.typestr, r.comments) >> regions.insert(regions.index(r), tail_r) >> regions.remove(r) >> return mem >> @@ -826,7 +931,7 @@ def parse_dmar(pcidevices, ioapics, dmar_regions): >> comments.append('DMAR parser could not decode device >> path') >> offset += scope_len >> >> - reg = MemRegion(base, limit, 'ACPI DMAR RMRR', comments) >> + reg = IOMemRegion(base, limit, 'ACPI DMAR RMRR', comments) >> regions.append(reg) >> >> f.seek(struct_len - offset, os.SEEK_CUR) >> @@ -1003,7 +1108,8 @@ def parse_ivrs(pcidevices, ioapics): >> 'regions. The memory at 0x%x will be mapped >> accessible ' >> 'to all devices.' % mem_addr) >> >> - regions.append(MemRegion(mem_addr, mem_len, 'ACPI IVRS', >> comment)) >> + regions.append(IOMemRegion(mem_addr, mem_len, >> + 'ACPI IVRS', comment)) >> elif type == 0x40: >> raise RuntimeError( >> 'You board uses IVRS Rev. 2 feature Jailhouse doesn\'t ' >> @@ -1135,9 +1241,9 @@ elif (total > ourmem[1]): >> >> hvmem[0] = ourmem[0] >> >> -inmatereg = MemRegion(ourmem[0] + hvmem[1], >> - ourmem[0] + hvmem[1] + inmatemem - 1, >> - 'JAILHOUSE Inmate Memory') >> +inmatereg = IOMemRegion(ourmem[0] + hvmem[1], >> + ourmem[0] + hvmem[1] + inmatemem - 1, >> + 'JAILHOUSE Inmate Memory') >> regions.append(inmatereg) >> >> cpucount = count_cpus() >> > > -- > Ralf Ramsauer > PGP: 0x8F10049B -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
