On Tue, Sep 19, 2006 at 10:46:33AM +0900, Horms wrote:
>
> Hi Vivek,
>
> that seems generally fine to me. I'm not sure that I understand the
> finer points of the elf parsing that you are doing, but again, it
> seems fine. I've put some comments, mainly about formatting and the
> like, inline.
Hi Horms,
Thanks for such a detailed review and reply. I have taken care of most of
the comments. Pleas find the response inline. I am reposting the patches
in a separate mail.
> > +#if 0
> > + printf("%016Lx-%016Lx : %s",
> > + start, end, str);
> > +#endif
>
> I really don't like the use of #if 0. I know its elsewhere in the code,
> but I think it would be a lot nicer if we moved to something
> like #if DEBUG, which could be toggled by ./configure. Not really
> relevant to this patch, but I thought I would mention it anyway.
>
Agreed. I have moved the code under #ifdef DEBUG.
> > +
> > + fd = open(kcore, O_RDONLY);
> > + if (fd < 0) {
> > + fprintf(stderr, "Cannot open %s: %s\n", kcore, strerror(errno));
> > + goto backward_compatible;
> > + }
> > +#define KCORE_ELF_HEADERS_SIZE 4096
>
> Personally I'm much more comofortable with #defines going
> either towards the top a .c file, or in a .h file. Could you
> move this and the definition of KERN_VADDR_ALIGN below to
> one of these two locations, the .c one seems most appropriate
> here.
>
Ok. I have moved KCORE_ELF_HEADERS_SIZE in crashdump.h. KERN_VADDRA_ALIGN
I have moved to the top in .c file.
> > + size = KCORE_ELF_HEADERS_SIZE;
> > + buf = xmalloc(size);
>
> Is there is a reason buf needs to be on the heap rather than the stack?
> As it is a static size it seems easier just to declare it as
> a fixed-size array. For starters it means there is no chance
> of it leaking.
>
Now this code has moved inside function slurp_file_len() and it has
to allocate buf from heap so that calling function can make use of
that buffer.
> > + memset(buf, 0, size);
> > + progress = 0;
> > + while(progress < size) {
> > + result = read(fd, buf + progress, size - progress);
> > + if (result < 0) {
> > + if ((errno == EINTR) || (errno == EAGAIN))
> > + continue;
> > + die("read on %s of %ld bytes failed: %s\n",
> > + kcore, (size - progress)+ 0UL, strerror(errno));
> > + }
> > + if (result == 0)
> > + /* EOF */
> > + break;
> > + progress += result;
> > + }
>
> Is this kind of read X bytes untill you have X, EOF or error loop
> a common thing in kexec-tools? It might be worth breaking it out into
> some common code somewhere.
>
Point taken. Wrote a function slurp_file_len() on the lines of slurp_file().
This function just reads len bytes as specified by the caller and not the
whole of the file.
> > + result = close(fd);
> > + if (result < 0)
> > + die("Close of %s failed: %s\n", kcore, strerror(errno));
>
> The result of close() is checked here, but the result of fclose()
> is not checked in some code above. Is there a reason for this
> inconsistency?
>
Not intentional. Just I had copied the code from slurp_file() and
it was doing the check on close().
> > +
> > + /* Don't perform checks to make sure stated phdrs and shdrs are
> > + * actually present in the core file. It is not practical
> > + * to read the GB size file into a user space buffer, Given the
> > + * fact that we don't use any info from that.
> > + */
>
> That is true, but it is might be practical to memmap it.
> Have you given some thought to whether that is worthwile or not?
I tried it but /proc/kcore does not support mmapping it. Otherwise
its a good idea.
>
> > + ignore_elf_len_check = 1;
>
> Should ignore_elf_len_check be turned off again at some point,
> or alternatively changed into a parameter rather than a global?
Now I am turning it off.
Its a global for keeping the code changes less. If I make it a parameter
then I have to change whole lot of functions and code across arches due
to interdependencies. So for the time being I have kept it a separate
global. But if you think that it is absolute no-no programming practice,
then I will write a separate patch to make it a function parameter.
>
> > + result = build_elf_core_info(buf, size, &ehdr);
> > + if (result < 0) {
> > + fprintf(stderr, "ELF core (kcore) parse failed\n");
> > + goto backward_compatible;
>
> Could the backward_compatible handling be moved into a
> separate function, rather than a goto? It seems that
> only info would need to be passed.
>
Done. Added a new function hardcode_kernel_vaddr_size() to handle it.
> > + }
> > + /* Traverse through the Elf headers and find the region where
> > + * kernel is mapped. */
> > +#define KERN_VADDR_ALIGN 0x200000 /* 2MB */
> > + end_phdr = &ehdr.e_phdr[ehdr.e_phnum];
> > + for(phdr = ehdr.e_phdr; phdr != end_phdr; phdr++) {
> > + if (phdr->p_type == PT_LOAD) {
> > + unsigned long saddr = phdr->p_vaddr;
> > + unsigned long eaddr = phdr->p_vaddr + phdr->p_memsz;
> > + unsigned long size;
> > + /* Look for kernel text mapping header. */
>
> It seems like there is one tab too few for the indentation
> of the line above.
>
Done.
> > + if ((saddr >= __START_KERNEL_map) &&
> > + (eaddr <= __START_KERNEL_map + KERNEL_TEXT_SIZE)) {
>
> I think that the above lines should be indented as follows
>
> if ((saddr >= __START_KERNEL_map) &&
> (eaddr <= __START_KERNEL_map + KERNEL_TEXT_SIZE)) {
> ^ 2nd line is indented to start inside the
> relevant ( of the 1st line
>
Done.
> > + saddr = (saddr) & (~(KERN_VADDR_ALIGN - 1));
> > + info->kern_vaddr_start = saddr;
> > + size = eaddr - saddr;
> > + /* Align size to page size boundary. */
> > + size = (size + align - 1) & (~(align - 1));
> > + info->kern_size = size;
> > +#if 0
> > + printf("kernel vaddr = 0x%lx size = 0x%lx\n",
> > + saddr, size);
> > +#endif
> > + return 0;
> > + }
>
> I'm not sure that I understand the alingment that the above
> if statment is adding, but I guess its fine :-)
Actually /proc/kcore reports the vaddr of symbol _stext
and not actually the starting kernel virtual address in the program
header created for kernel text. Generally there is some code before symbol
_stext. Anyway I load the kernel at 2MB aligned boundary so I forced this
alignment to make sure a neat separate program header is created. Otherwise
in /proc/vmcore we will end up creating an header starting at and odd
address, something like 0xfffff....1f1.
> >
> > +/* This parameter can be set to force ELF phdr and shdr building routines
> > + * to skip the check on file length which make sure that data pointed by
> > + * phdrs and shdrs is actually present in the file. This has been
> > introduced
> > + * to handle the corner case of reading /proc/kcore. /proc/kcore can be
> > + * of huge size (GB, TB), depending on RAM size. It does not make sense
> > + * to read that whole core file just to extract some info from kcore ELF
> > + * headers.
> > + * This is hackish. Please suggest a better way to handle it.
> > + */
> > +int ignore_elf_len_check = 0;
> > +
>
> Is there a reason this is global, rather than being passed
> as an argument to the relevant functions?
See above.
>
> > uint16_t elf16_to_cpu(const struct mem_ehdr *ehdr, uint16_t value)
> > {
> > if (ehdr->ei_data == ELFDATA2LSB) {
> > @@ -420,12 +431,14 @@ static int build_mem_phdrs(const char *b
> > * they are safe to use.
> > */
> > phdr = &ehdr->e_phdr[i];
> > - if ((phdr->p_offset + phdr->p_filesz) > len) {
> > - /* The segment does not fit in the buffer */
> > - if (probe_debug) {
> > - fprintf(stderr, "ELF segment not in file\n");
> > + if (!ignore_elf_len_check) {
> > + if ((phdr->p_offset + phdr->p_filesz) > len) {
> > + /* The segment does not fit in the buffer */
> > + if (probe_debug) {
> > + fprintf(stderr, "ELF segment not in
> > file\n");
> > + }
> > + return -1;
> > }
> > - return -1;
> > }
> > if ((phdr->p_paddr + phdr->p_memsz) < phdr->p_paddr) {
> > /* The memory address wraps */
>
> I think the above (mostly whitespace) change could be simplified to
> the following, which has the advantage of not being so heavily
> nested.
>
Done.
> > /* The memory address wraps */
>
> Again, I think the following is a bit simpler.
>
Done.
Thanks
Vivek
_______________________________________________
fastboot mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/fastboot
