> -----Original Message----- > From: Dave Anderson [mailto:[email protected]] > Sent: Friday, October 27, 2017 11:16 PM > To: Oleksandr Natalenko <[email protected]> > Cc: [email protected]; Hatayama, Daisuke > <[email protected]>; d hatayama <[email protected]> > 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 for telling me this, Dave. Oleksandr, this looks good to me. Thanks for your patch. block_size is of type size_t and size_t is always defined as some unsigned integer type in the C standards. So, the condition on block_size is meaningless and can be removed. > > Thanks, > Dave > > > > > -- Crash-utility mailing list [email protected] https://www.redhat.com/mailman/listinfo/crash-utility
