----- Original Message -----
> Hi Dave and all,
>
> Sent on 2011-5-7 0:11, Dave Anderson wrote:
>
> >> 2) Dump notes related information including total number of notes, their
> >> buffers' pointer value and finally offset in the dump file.
> >> 3) Move nt_prstatus_percpu and nt_prstatus_percpu to diskdump_data struct.
> >> However, bt command may need sp/ip from the notes later, so I guess
> >> the buffer should not be freed after diskdump_read_header().
> >
> > I wish I had understood the s390x implementation a bit better when I
> > reviewed your v2 patch. Currently read_dump_header() malloc's a
> > single "notes_buf" buffer and copies the complete ELF notes section
> > from the dumpfile into it. I believe (but am not completely sure) that
> > the s390x code references pieces of that buffer, and that's why they
> > originally kept the "notes_buf" buffer permanently allocated. That
> > being the case, the "notes_buf" pointer should be moved to the
> > diskdump header.
> >
> > Anyway, in the case of x86/x86_64, your patch:
> >
> > (1) malloc's dd->nt_prstatus_percpu as an array of pointers,
> > (2) malloc's a bunch of individual per-cpu ELF prstatus buffers, and
> > then copies the ELF data from the "notes_buf" buffer into
> > each per-cpu buffer.
> > (4) puts a pointer to each per-cpu buffer into each dd->nt_prstatus_percpu
> > pointer.
> >
> > But given that the original "notes_buf" buffer is permanent, your patch
> > should only have to store per-cpu pointers into the original note_buf
> > buffer -- rather than redundantly malloc'ing a bunch of new buffers that
> > contain copies of what's already permanently available. So I would suggest
> > setting it up like this:
> >
> > dd->nt_prstatus_percpu[cpu] => points into relevant "notes_buf"
> > location
> >
>
> Oh, sorry for my misunderstood.
>
> Since we will later refer to ip/sp value in the notes_buf, we should keep the
> notes_buf permanently and do not free them at the end of read_diskdump_header.
> s390x don't have such problem because the notes are copied to the s390x_cpu_vec
> when processing notes_buf.
The dd->notes_buf only needs to be kept permanently if the read_dump_header()
function succeeds (returns TRUE). If we do a "goto err" in read_dump_header(),
that means that dumpfile is not being recognized as a diskdump or compressed
kdump, and it returns FALSE. When that is the case, there is no possible way
that we could ever refer to the ip/sp values at a later point in time.
Accordingly, I have added this to your patch:
...
return TRUE;
err:
free(header);
if (sub_header)
free(sub_header);
if (sub_header_kdump)
free(sub_header_kdump);
if (dd->bitmap)
free(dd->bitmap);
if (dd->dumpable_bitmap)
free(dd->dumpable_bitmap);
+ if (dd->notes_buf)
+ free(dd->notes_buf);
+ if (dd->nt_prstatus_percpu)
+ free(dd->nt_prstatus_percpu);
dd->flags &= ~(DISKDUMP_LOCAL|KDUMP_CMPRS_LOCAL);
return FALSE;
}
> So just as what you suggested, notes_buf was moved to diskdump header in the
> attached v4 patch and nt_prstatus_percpu will store the address in notes_buf then.
I also slightly modified the patches to get_netdump_regs_x86() and
get_netdump_regs_x86_64() to avoid the goto's, but with no functional
changes.
Queued for crash version 5.1.5.
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
