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

[Hatayama, Daisuke]

> -----Original Message-----
> From: Dave Anderson [mailto:anderson at redhat.com]
> Sent: Friday, October 27, 2017 11:16 PM
> To: Oleksandr Natalenko <oleksandr at redhat.com>
> Cc: crash-utility at redhat.com; Hatayama, Daisuke 
> <d.hatayama at jp.fujitsu.com>; d hatayama <d.hatayama at gmail.com>
> 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
> 
> 
> 
> 
>