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