Re: [Crash-utility] [PATCH RFC 00/14] Minor code cleanups, round zero

-----Original Message----- From: Dave Anderson [mailto:anderson@redhat.com] Sent: Friday, October 27, 2017 11:16 PM To: Oleksandr Natalenko <oleksandr(a)redhat.com&gt; Cc: crash-utility(a)redhat.com; Hatayama, Daisuke <d.hatayama(a)jp.fujitsu.com>; d hatayama <d.hatayama(a)gmail.com&gt; Subject: Re: [PATCH RFC 00/14] Minor code cleanups, round zero ----- Original Message ----- > > I'll take a look at these when I get the chance, but I'm really > not particularly excited unless they are actual bugs. Like this one: --- a/memory.c +++ b/memory.c @@ -17003,8 +17003,8 @@ valid_section(ulong addr) if ((mem_section = read_mem_section(addr))) return (ULONG(mem_section + - OFFSET(mem_section_section_mem_map)) && - SECTION_MARKED_PRESENT); + OFFSET(mem_section_section_mem_map)) + & SECTION_MARKED_PRESENT); return 0; } @@ -17016,7 +17016,7 @@ section_has_mem_map(ulong addr) if ((mem_section = read_mem_section(addr))) return (ULONG(mem_section + OFFSET(mem_section_section_mem_map)) - && SECTION_HAS_MEM_MAP); + & SECTION_HAS_MEM_MAP); return 0; } This code has been like this since the original CONFIG_SPARSEMEM support patch was posted back in 2006. Interesting that this has never been a problem. Apparently nobody's ever bumped into mem_section that didn't have those flags. And then there's this one: --- a/x86_64.c +++ b/x86_64.c @@ -2086,7 +2086,7 @@ x86_64_kvtop(struct task_context *tc, ulong kvaddr, physaddr_t *paddr, int verbo } start_vtop_with_pagetable: - if (!(*pml4) & _PAGE_PRESENT) + if ((!(*pml4)) & _PAGE_PRESENT) goto no_kpage; pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK; FILL_PGD(pgd_paddr, PHYSADDR, PAGESIZE()); @@ -2187,7 +2187,7 @@ x86_64_kvtop_xen_wpt(struct task_context *tc, ulong kvaddr, physaddr_t *paddr, i fprintf(fp, "PML4 DIRECTORY: %lx\n", vt->kernel_pgd[0]); fprintf(fp, "PAGE DIRECTORY: %lx [machine]\n", *pml4); } - if (!(*pml4) & _PAGE_PRESENT) + if ((!(*pml4)) & _PAGE_PRESENT) goto no_kpage; pgd_paddr = (*pml4) & PHYSICAL_PAGE_MASK; pgd_paddr = xen_m2p(pgd_paddr); It's pretty much impossible to have a pml without its PAGE_PRESENT bit set, so it's been a benign bug. But the fix is incorrect. They should be: if (!(*pml4 & _PAGE_PRESENT)) And I'm not sure wheter this one should be fixed by just removing the statement: --- a/sadump.c +++ b/sadump.c @@ -157,9 +157,6 @@ read_dump_header(char *file) } restart: - if (block_size < 0) - return FALSE; - if (!read_device(sph, block_size, &offset)) { error(INFO, "sadump: cannot read partition header\n"); goto err; because farther down there is this: if (sh->block_size != block_size) { block_size = sh->block_size; offset = 0; goto restart; } I'll let the sadump maintainer decide how to deal with this one. He's on this list, but I've cc'd him to get his attention.

Thanks, Dave