[Crash-utility] add support for incomplete elf dump file

["Zhou, Wenjian/周文剑"]

On 10/23/2014 08:10 PM, Dave Anderson wrote:
>
>
> ----- Original Message -----
>> Hello Dave,
>>
>> On 10/22/2014 09:49 PM, Dave Anderson wrote:
>>>
>>> Hello Wenjian,
>>>
>>> First -- please send patches as email attachments.  Even if I cut-and-paste
>>> your
>>> patch from the crash-utility archives (which normally works), there are
>>> still no tabs in your patch.
>>
>> Appreciate your suggestion.
>>
>>> Now, with respect to the patch, it's not clear to me what would happen if you
>>> make no changes to check_dumpfile_size()?
>>>
>>> It seems to me that a read error would occur regardless whether you change
>>> the PT_LOAD-related contents or not.
>>>
>>> If no changes are made:
>>>
>>>     If a readmem() of a truncated page is attempted, read_netdump() would
>>>     calculate an offset based upon the original PT_LOAD contents, but then
>>>     the subsequent read() would fail, and would return a READ_ERROR.
>>>
>>> If your patch is applied:
>>>
>>>     If a readmem() of a truncated page is attempted, read_netdump() would not
>>>     be able to calculate an offset, and would return a READ_ERROR.
>>>
>>> What's the difference?  Why even bother making the changes?
>>>
>> If my patch is applied:
>> 	If a readmem() of a truncated page is attempted,
>>
>> 	(pls->zero_fill&&  (paddr>= pls->phys_end)&&  (paddr<  pls->zero_fill)),
>> 	,his will be right. So read_netdump() will fill bufptr with zero and
>> 	return cnt.
>>
>> In my patch, information of PT_LOAD segment is modified, so that the segment will
>> not go beyond the file size. And use zero to replace the lost part.
>>
>> Previously, we intend to do this work in makedumpfile by modifying the PT_LOAD
>> header of the ELF dump file. But kumagai atsushi thought it's not good to make the
>> irreversible change. So we think do it in crash maybe a better choice.
>>
>>> I also don't understand what the difference between "truncated" and "incomplete"
>>> is?  Why did you separate the messages into two?
>>>
>> At first, I thought the difference is the incomplete flag.
>> Now, I think it's not necessary to distinguish them.
>>
>>
>>> Anyway, I had already created a patch in preparation for the changes to
>>> makedumpfile for ELF and compressed kdump vmcores.  The patch will apply
>>> to the current github master branch.  Please apply it alone, and tell me
>>> what happens.
>>
>> The patch can show the incomplete information, but will be interrupted by the
>> READ_ERROR (we don't change the PT_LOAD headers in makedumpfile any more).
>
> Well, it *should* be interrupted.  What's worse, "interruption" or the receipt
> of completely bogus zero-filled data?
>
> This whole scheme is even crazier than I thought.  I'm going to have
> to make the initialization-time warning message even more ominous than
> I already have it.
>
Actually, I haven't realized it until doing the work in crash.
I think which is better or worse depends on what people want to do.
If incomplete flag exists, it may indicate people want to analyse the file,
and returning zero-filled data is better. Otherwise, "interruption" may be expected.

> But I still don't think it's necessary to modify the original PT_LOAD
> contents.  If read_netdump() finds a legitimate offset, but that offset
> is beyond the end of the incomplete/truncated file, it can just check the
> INCOMPLETE flag and return a zero-filled buffer if it's set.   Wouldn't
> that work?

It does work. And now, the question turns to be which way is better,
first:   do it in check_dumpfile_size() by modifying relevant information
	 (not modifying the information in ELF file)

second:  do it in read_netdump() by return zero-filled data with
          checking offset and flag
, isn't it?

The attachment is the patch of second method.

Thanks
Zhou Wenjian
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: SUPPROT_INCOMPLETE_ELF_DUMPFILE.patch
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20141024/949b1aec/attachment.ksh>