Re: [PULL 3/8] dump: Add command interface for kdump-raw formats

[Peter Maydell]

On Fri, 3 Nov 2023 at 07:02, <marcandre.lureau@redhat.com> wrote:
>
> From: Stephen Brennan <stephen.s.brennan@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.brennan@oracle.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> [ Marc-André: replace loff_t with off_t, indent fixes ]
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <20230918233233.1431858-4-stephen.s.brennan@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