On Fri, 3 Nov 2023 at 07:02, <marcandre.lur...@redhat.com> wrote:
>
> From: Stephen Brennan <stephen.s.bren...@oracle.com>
>
> The QMP dump API represents the dump format as an enumeration. Add three
> new enumerators, one for each supported kdump compression, each named
> "kdump-raw-*".
>
> For the HMP command line, rather than adding a new flag corresponding to
> each format, it seems more human-friendly to add a single flag "-R" to
> switch the kdump formats to "raw" mode. The choice of "-R" also
> correlates nicely to the "makedumpfile -R" option, which would serve to
> reassemble a flattened vmcore.
>
> Signed-off-by: Stephen Brennan <stephen.s.bren...@oracle.com>
> Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> [ Marc-André: replace loff_t with off_t, indent fixes ]
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> Message-Id: <20230918233233.1431858-4-stephen.s.bren...@oracle.com>
Hi; Coverity points out some issues in this commit:
> diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
> index b038785fee..b428ec33df 100644
> --- a/dump/dump-hmp-cmds.c
> +++ b/dump/dump-hmp-cmds.c
> @@ -19,6 +19,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> bool paging = qdict_get_try_bool(qdict, "paging", false);
> bool zlib = qdict_get_try_bool(qdict, "zlib", false);
> bool lzo = qdict_get_try_bool(qdict, "lzo", false);
> + bool raw = qdict_get_try_bool(qdict, "raw", false);
> bool snappy = qdict_get_try_bool(qdict, "snappy", false);
> const char *file = qdict_get_str(qdict, "filename");
> bool has_begin = qdict_haskey(qdict, "begin");
> @@ -40,16 +41,28 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict
> *qdict)
> dump_format = DUMP_GUEST_MEMORY_FORMAT_WIN_DMP;
> }
>
> - if (zlib) {
> - dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
> + if (zlib && raw) {
> + if (raw) {
> + dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB;
> + } else {
> + dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
This is dead code, beacuse the outer conditional "(zlib && raw)"
ensures that raw can't be false here. What was the intention here?
(CID 1523841)
> + }
> }
>
> if (lzo) {
> - dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
> + if (raw) {
> + dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO;
> + } else {
> + dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
> + }
> }
>
> if (snappy) {
> - dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
> + if (raw) {
> + dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY;
> + } else {
> + dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
> + }
> }
>
> if (has_begin) {
> @@ -2166,6 +2190,10 @@ void qmp_dump_guest_memory(bool paging, const char
> *file,
> error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
> return;
> }
> + if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (off_t) -1) {
> + error_setg(errp, "kdump-raw formats require a seekable file");
> + return;
> + }
This error-exit return path forgets to close(fd), so we leak
the file descriptor. (CID 1523842)
>
> if (!dump_migration_blocker) {
> error_setg(&dump_migration_blocker,
thanks
-- PMM
