libdrgn: accept flattened KDUMP format #366
Conversation
libdrgn/program.c
Outdated
| && memcmp(signature, FLATTENED_SIGNATURE, FLATTENED_SIG_LEN) == 0) { | ||
| return drgn_error_create(DRGN_ERROR_INVALID_ARGUMENT, | ||
| "the given file is in the makedumpfile flattened " | ||
| "format, which drgn does not support; use " | ||
| "'makedumpfile -R newfile <oldfile' to reassemble " | ||
| "the file into a format drgn can read"); | ||
| } | ||
| *ret = (r >= KDUMP_SIG_LEN | ||
| *ret = (r >= FLATTENED_SIG_LEN | ||
| && memcmp(signature, FLATTENED_SIGNATURE, FLATTENED_SIG_LEN) == 0) || | ||
| (r >= KDUMP_SIG_LEN | ||
| && memcmp(signature, KDUMP_SIGNATURE, KDUMP_SIG_LEN) == 0); | ||
| return NULL; |
There was a problem hiding this comment.
This seems great, thanks for the work adding the support into libkdumpfile! This will definitely be a usability improvement, though I think for the larger vmcores, it will still be helpful to use makedumpfile to reassemble the flattened format.
I see that you added support in libkdumpfile 0.5.3. Does libkdumpfile have any runtime feature-detection or version detection? Obviously we can check at compile time, and that would work well enough for the PyPI wheels, since they're distributed with a bundled libkdumpfile.
But for distribution packages, it would be nice to be able to check what version we're actually using, and if we're using earlier versions, fall back to the error message (and maybe add the minimum libkdumpfile version into that message text).
The other comment I have, and I'm not sure that this will be universally agreed upon, is that I'd like Drgn to emit some sort of warning when opening a flattened vmcore. My experience with crash's flattened vmcore support has been good, but it does spend a while indexing larger vmcores at startup, and it doesn't bother to give any indication to the user that they could cut back on the startup time by using makedumpfile -R just once. I would like to avoid that with drgn, so users know they have the option to reassemble the vmcore.
There was a problem hiding this comment.
…
But for distribution packages, it would be nice to be able to check what version we're actually using, and if we're using earlier versions, fall back to the error message (and maybe add the minimum libkdumpfile version into that message text).
I am not a big fan of version checks, because a newer feature may be backported to an older version. I prefer the AFF approach: “Ask for forgiveness, not for permission.” I would just try to open the file and if that fails with KDUMP_ERR_ NOTIMPL, the flattened format is not supported.
The other comment I have, and I'm not sure that this will be universally agreed upon, is that I'd like Drgn to emit some sort of warning when opening a flattened vmcore. My experience with crash's flattened vmcore support has been good, but it does spend a while indexing larger vmcores at startup, and it doesn't bother to give any indication to the user that they could cut back on the startup time by using
makedumpfile -Rjust once. I would like to avoid that with drgn, so users know they have the option to reassemble the vmcore.
Agreed. The flattened file format is such that the whole file must be scanned first to know where to find data. For a flattened dump, the file.description attribute starts with Flattened . However, this may not be available early enough. IIUC you would like to emit a warning before libkdumpfile spends a lot of time opening the file, so users can press CTRL+C, rearrange the file and run again. There are no progress callbacks in libkdumpfile, but since drgn checks the signature anyway, I can keep the message there.
There was a problem hiding this comment.
I am not a big fan of version checks, because a newer feature may be backported to an older version. I prefer the AFF approach: “Ask for forgiveness, not for permission.” I would just try to open the file and if that fails with KDUMP_ERR_ NOTIMPL, the flattened format is not supported.
Agreed in principle! Half of the problems in my life are caused by backported commits :P
In practice many times you can get away with a version check, but I understand wanting to avoid it.
The way your v2 is implemented works for me. Essentially, when we open a flattened vmcore, log the message, and if it fails, the user has a good idea of why.
To allow logging in the function, let it take prog as a parameter. OTOH the file descriptor then need not be passed separately. Signed-off-by: Petr Tesarik <[email protected]>
Reduce the error to a warning. Support for the flattened KDUMP format was added in libkdumpfile-0.5.3, so a sufficiently recent libkdumpfile can open flattened files directly. However, the flattened file format requires scanning the whole file first to build a map of flattened file segments, so opening a large file may be too slow. Issue a warning, so users know they have the option to reassemble the vmcore. Signed-off-by: Petr Tesarik <[email protected]>
|
Um. OK, let me add a commit. |
Return DRGN_ERROR_INVALID_ARGUMENT with a reasonable error message if
libkdumpfile cannot open a file.
The error output currently looks something like:
Traceback (most recent call last):
File "/research/bin/drgn", line 33, in <module>
sys.exit(load_entry_point('drgn', 'console_scripts', 'drgn')())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/research/src/drgn/drgn/cli.py", line 264, in _main
prog.set_core_dump(args.core)
Exception: kdump_set_number_attr(KDUMP_ATTR_FILE_FD): File #0: Unknown file format
Replace it with this:
error: file #0: Unknown file format
This is similar to the error message when trying to open a file that is
neither KDUMP nor ELF:
error: not an ELF core file
Signed-off-by: Petr Tesarik <[email protected]>
|
Thanks for adding support to libkdumpfile and for this change! |
Support for the flattened KDUMP format was added in libkdumpfile-0.5.3.