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

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?