Re: [Qemu-devel] [PATCH 4/7] dump: add vmcoreinfo ELF note

[Laszlo Ersek <lersek@redhat.com>]

On 07/05/17 23:52, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jul 5, 2017 at 1:48 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 06/29/17 15:23, Marc-André Lureau wrote:

>>> @@ -258,6 +280,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>>>              return;
>>>          }
>>>      }
>>> +
>>> +    write_vmcoreinfo_note(f, s, errp);
>>>  }
>>>
>>>  static void write_elf32_note(DumpState *s, Error **errp)
>>> @@ -303,6 +327,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
>>>              return;
>>>          }
>>>      }
>>> +
>>> +    write_vmcoreinfo_note(f, s, errp);
>>>  }
>>>
>>
>> Wait, I'm confused again. You explained why it was OK to hook this logic
>> into the kdump handling too, but I don't think I understand your
>> explanation, so let me repeat my confusion below :)
>>
>> In the ELF case, this code works fine, I think. As long as the guest
>> provided us with a well-formed note, a well-formed note will be appended
>> to the ELF dump.
>>
>> But, this code is also invoked in the kdump case, and I don't understand
>> why that's a good thing. If I understand the next patch correctly, the
>> kdump format already provides crash with a (trimmed) copy of the guest
>> kernels vmcoreinfo note. So in the kdump case, why do we have to create
>> yet another copy of the guest kernel's vmcoreinfo note?
>>
>> Thus, my confusion persists, and I can only think (again) that
>> write_vmcoreinfo_note() should be called from dump_begin() only (at the
>> end). (And the s->note_size adjustment should take that into account.)
>>
>> Alternatively, we should keep this logic, and drop patch #5.
> 
> You are right, sorry for my misunderstanding, although crash is seems
> fine as long as size/offsets are ok.
> 
> So, instead of duplicating the info, let's keep the complete ELF note
> (with the header) in the dump, and simply adjust kdump header to point
> to the adjusted offset/size. This has the advantage of not making the
> code unnecessarily more complicated wrt s->note_size handling etc, and
> is consistent with the rest of the elf notes.

I guess that's OK, although I don't really see why even that is
necessary. The vmcoreinfo note should be possible to find in the kdump
output with the rest of the ELF notes, even without pointing some kdump
header fields into that specific note.

Snipping the rest because I'm going to see updates on those things in v2
anyway (which you've just posted).

Thanks
Laszlo