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

[Dave Anderson]

----- 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