Re: [PATCH v3 qemu 2/3] dump: Allow directly outputting raw kdump format

[Stephen Brennan <stephen.s.brennan@oracle.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi Stephen
>
> On Tue, Sep 19, 2023 at 3:32 AM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> The flattened format (currently output by QEMU) is used by makedumpfile
>> only when it is outputting a vmcore to a file which is not seekable. The
>> flattened format functions essentially as a set of instructions of the
>> form "seek to the given offset, then write the given bytes out".
>>
>> The flattened format can be reconstructed using makedumpfile -R, or
>> makedumpfile-R.pl, but it is a slow process because it requires copying
>> the entire vmcore. The flattened format can also be directly read by
>> crash, but still, it requires a lengthy reassembly phase.
>>
>> To sum up, the flattened format is not an ideal one: it should only be
>> used on files which are actually not seekable. This is the exact
>> strategy which makedumpfile uses, as seen in the implementation of
>> "write_buffer()" in makedumpfile [1]. However, QEMU has always used the
>> flattened format. For compatibility it is best not to change the default
>> output format without warning. So, add a flag to DumpState which changes
>> the output to use the normal (i.e. raw) format. This flag will be added
>> to the QMP and HMP commands in the next change.
>>
>> [1]: https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>  dump/dump.c           | 32 +++++++++++++++++++++++++-------
>>  include/sysemu/dump.h |  1 +
>>  2 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 74071a1565..10aa2c79e0 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -814,6 +814,10 @@ static int write_start_flat_header(DumpState *s)
>>      MakedumpfileHeader *mh;
>>      int ret = 0;
>>
>> +    if (s->kdump_raw) {
>> +        return 0;
>> +    }
>> +
>>      QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
>>      mh = g_malloc0(MAX_SIZE_MDF_HEADER);
>>
>> @@ -837,6 +841,10 @@ static int write_end_flat_header(DumpState *s)
>>  {
>>      MakedumpfileDataHeader mdh;
>>
>> +    if (s->kdump_raw) {
>> +        return 0;
>> +    }
>> +
>>      mdh.offset = END_FLAG_FLAT_HEADER;
>>      mdh.buf_size = END_FLAG_FLAT_HEADER;
>>
>> @@ -853,13 +861,21 @@ static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size
>>  {
>>      size_t written_size;
>>      MakedumpfileDataHeader mdh;
>> +    loff_t seek_loc;
>
> Any reason to use loff_t over off_t here? It fails to compile on win32
> for ex. I can touch on PR commit otherwise.

I think that was an oversight on my part: lseek() uses off_t.

I see that qemu is compiled with _FILE_OFFSET_BITS=64 so off_t should be
64 bits even on 32-bit architectures. So this should be off_t. I believe
the compile error would also happen in qmp_dump_guest_memory() where I
have:

    if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (loff_t) -1) {

If it's easiest for you to tweak it in the PR, please do. Otherwise I
can respin the series replacing loff_t with off_t.

Thank you,
Stephen

>>
>> -    mdh.offset = cpu_to_be64(offset);
>> -    mdh.buf_size = cpu_to_be64(size);
>> +    if (s->kdump_raw) {
>> +        seek_loc = lseek(s->fd, offset, SEEK_SET);
>> +        if (seek_loc == (off_t) -1) {
>> +            return -1;
>> +        }
>> +    } else {
>> +        mdh.offset = cpu_to_be64(offset);
>> +        mdh.buf_size = cpu_to_be64(size);
>>
>> -    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
>> -    if (written_size != sizeof(mdh)) {
>> -        return -1;
>> +        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
>> +        if (written_size != sizeof(mdh)) {
>> +            return -1;
>> +        }
>>      }
>>
>>      written_size = qemu_write_full(s->fd, buf, size);
>> @@ -1775,7 +1791,8 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
>>
>>  static void dump_init(DumpState *s, int fd, bool has_format,
>>                        DumpGuestMemoryFormat format, bool paging, bool has_filter,
>> -                      int64_t begin, int64_t length, Error **errp)
>> +                      int64_t begin, int64_t length, bool kdump_raw,
>> +                      Error **errp)
>>  {
>>      ERRP_GUARD();
>>      VMCoreInfoState *vmci = vmcoreinfo_find();
>> @@ -1786,6 +1803,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>      s->has_format = has_format;
>>      s->format = format;
>>      s->written_size = 0;
>> +    s->kdump_raw = kdump_raw;
>>
>>      /* kdump-compressed is conflict with paging and filter */
>>      if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> @@ -2168,7 +2186,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>>      dump_state_prepare(s);
>>
>>      dump_init(s, fd, has_format, format, paging, has_begin,
>> -              begin, length, errp);
>> +              begin, length, false, errp);
>>      if (*errp) {
>>          qatomic_set(&s->status, DUMP_STATUS_FAILED);
>>          return;
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index e27af8fb34..d702854853 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -157,6 +157,7 @@ typedef struct DumpState {
>>      MemoryMappingList list;
>>      bool resume;
>>      bool detached;
>> +    bool kdump_raw;
>>      hwaddr memory_offset;
>>      int fd;
>>
>> --
>> 2.39.3
>>
>
>
> -- 
> Marc-André Lureau