[PATCH 0/5] dump: Minor fixes & improvements

[PATCH 0/5] dump: Minor fixes & improvements

[Markus Armbruster]

Markus Armbruster (5):
  dump: Rename qmp_dump_guest_memory() parameter to match QAPI schema
  dump: Fix g_array_unref(NULL) in dump-guest-memory
  dump: Recognize "fd:" protocols on Windows hosts
  dump: Improve some dump-guest-memory error messages
  dump: Drop redundant check for empty dump

 dump/dump.c | 59 +++++++++++++++--------------------------------------
 1 file changed, 16 insertions(+), 43 deletions(-)

-- 
2.41.0

[PATCH 1/5] dump: Rename qmp_dump_guest_memory() parameter to match QAPI schema

[Markus Armbruster]

The name of the second parameter differs between QAPI schema and C
implementation: it's @protocol in the former and @file in the latter.
Potentially confusing.  Change the C implementation to match the QAPI
schema.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index d355ada62e..a1fad17f9c 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2061,11 +2061,12 @@ DumpQueryResult *qmp_query_dump(Error **errp)
     return result;
 }
 
-void qmp_dump_guest_memory(bool paging, const char *file,
+void qmp_dump_guest_memory(bool paging, const char *protocol,
                            bool has_detach, bool detach,
-                           bool has_begin, int64_t begin, bool has_length,
-                           int64_t length, bool has_format,
-                           DumpGuestMemoryFormat format, Error **errp)
+                           bool has_begin, int64_t begin,
+                           bool has_length, int64_t length,
+                           bool has_format, DumpGuestMemoryFormat format,
+                           Error **errp)
 {
     ERRP_GUARD();
     const char *p;
@@ -2128,7 +2129,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     }
 
 #if !defined(WIN32)
-    if (strstart(file, "fd:", &p)) {
+    if (strstart(protocol, "fd:", &p)) {
         fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
             return;
@@ -2136,7 +2137,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     }
 #endif
 
-    if  (strstart(file, "file:", &p)) {
+    if  (strstart(protocol, "file:", &p)) {
         fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
         if (fd < 0) {
             error_setg_file_open(errp, errno, p);
-- 
2.41.0

Re: [PATCH 1/5] dump: Rename qmp_dump_guest_memory() parameter to match QAPI schema

[Marc-André Lureau]

On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> The name of the second parameter differs between QAPI schema and C
> implementation: it's @protocol in the former and @file in the latter.
> Potentially confusing.  Change the C implementation to match the QAPI
> schema.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d355ada62e..a1fad17f9c 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -2061,11 +2061,12 @@ DumpQueryResult *qmp_query_dump(Error **errp)
>      return result;
>  }
>
> -void qmp_dump_guest_memory(bool paging, const char *file,
> +void qmp_dump_guest_memory(bool paging, const char *protocol,
>                             bool has_detach, bool detach,
> -                           bool has_begin, int64_t begin, bool has_length,
> -                           int64_t length, bool has_format,
> -                           DumpGuestMemoryFormat format, Error **errp)
> +                           bool has_begin, int64_t begin,
> +                           bool has_length, int64_t length,
> +                           bool has_format, DumpGuestMemoryFormat format,
> +                           Error **errp)
>  {
>      ERRP_GUARD();
>      const char *p;
> @@ -2128,7 +2129,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>      }
>
>  #if !defined(WIN32)
> -    if (strstart(file, "fd:", &p)) {
> +    if (strstart(protocol, "fd:", &p)) {
>          fd = monitor_get_fd(monitor_cur(), p, errp);
>          if (fd == -1) {
>              return;
> @@ -2136,7 +2137,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>      }
>  #endif
>
> -    if  (strstart(file, "file:", &p)) {
> +    if  (strstart(protocol, "file:", &p)) {
>          fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
>          if (fd < 0) {
>              error_setg_file_open(errp, errno, p);
> --
> 2.41.0
>

[PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory

[Markus Armbruster]

When dump_init()'s check for non-zero @length fails, dump_cleanup()
passes null s->string_table_buf to g_array_unref(), which spews "GLib:
g_array_unref: assertion 'array' failed" to stderr.

Guard the g_array_unref().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dump/dump.c b/dump/dump.c
index a1fad17f9c..d8ea364af2 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
     memory_mapping_list_free(&s->list);
     close(s->fd);
     g_free(s->guest_note);
-    g_array_unref(s->string_table_buf);
+    if (s->string_table_buf) {
+        g_array_unref(s->string_table_buf);
+    }
     s->guest_note = NULL;
     if (s->resume) {
         if (s->detached) {
-- 
2.41.0

Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory

[Marc-André Lureau]

Hi

On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> When dump_init()'s check for non-zero @length fails, dump_cleanup()
> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
> g_array_unref: assertion 'array' failed" to stderr.
>
> Guard the g_array_unref().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a1fad17f9c..d8ea364af2 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
>      memory_mapping_list_free(&s->list);
>      close(s->fd);
>      g_free(s->guest_note);
> -    g_array_unref(s->string_table_buf);
> +    if (s->string_table_buf) {
> +        g_array_unref(s->string_table_buf);
> +    }

or:
g_clear_pointer(&s->string_table_buf, g_array_unref)

>      s->guest_note = NULL;
>      if (s->resume) {
>          if (s->detached) {
> --
> 2.41.0
>

Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory

[Markus Armbruster]

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

> Hi
>
> On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> When dump_init()'s check for non-zero @length fails, dump_cleanup()
>> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
>> g_array_unref: assertion 'array' failed" to stderr.
>>
>> Guard the g_array_unref().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> ---
>>  dump/dump.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index a1fad17f9c..d8ea364af2 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
>>      memory_mapping_list_free(&s->list);
>>      close(s->fd);
>>      g_free(s->guest_note);
>> -    g_array_unref(s->string_table_buf);
>> +    if (s->string_table_buf) {
>> +        g_array_unref(s->string_table_buf);
>> +    }
>
> or:
> g_clear_pointer(&s->string_table_buf, g_array_unref)

Since dump_cleanup() doesn't clear any of the other members of @s, I'll
stick to g_array_unref() for consistency and simplicity.

>>      s->guest_note = NULL;
>>      if (s->resume) {
>>          if (s->detached) {
>> --
>> 2.41.0
>>

Thanks!

Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Hi
>>
>> On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> When dump_init()'s check for non-zero @length fails, dump_cleanup()
>>> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
>>> g_array_unref: assertion 'array' failed" to stderr.
>>>
>>> Guard the g_array_unref().
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>>> ---
>>>  dump/dump.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/dump/dump.c b/dump/dump.c
>>> index a1fad17f9c..d8ea364af2 100644
>>> --- a/dump/dump.c
>>> +++ b/dump/dump.c
>>> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
>>>      memory_mapping_list_free(&s->list);
>>>      close(s->fd);
>>>      g_free(s->guest_note);
>>> -    g_array_unref(s->string_table_buf);
>>> +    if (s->string_table_buf) {
>>> +        g_array_unref(s->string_table_buf);
>>> +    }
>>
>> or:
>> g_clear_pointer(&s->string_table_buf, g_array_unref)
>
> Since dump_cleanup() doesn't clear any of the other members of @s, I'll
> stick to g_array_unref() for consistency and simplicity.

Wait!  You suggest *unconditional*

         g_clear_pointer(&s->string_table_buf, g_array_unref)         

don't you?

Got a preference?

>>>      s->guest_note = NULL;
>>>      if (s->resume) {
>>>          if (s->detached) {
>>> --
>>> 2.41.0
>>>
>
> Thanks!

Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory

[Marc-André Lureau]

Hi

On Tue, Oct 31, 2023 at 1:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >
> >> Hi
> >>
> >> On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>> When dump_init()'s check for non-zero @length fails, dump_cleanup()
> >>> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
> >>> g_array_unref: assertion 'array' failed" to stderr.
> >>>
> >>> Guard the g_array_unref().
> >>>
> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>
> >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >>> ---
> >>>  dump/dump.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/dump/dump.c b/dump/dump.c
> >>> index a1fad17f9c..d8ea364af2 100644
> >>> --- a/dump/dump.c
> >>> +++ b/dump/dump.c
> >>> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
> >>>      memory_mapping_list_free(&s->list);
> >>>      close(s->fd);
> >>>      g_free(s->guest_note);
> >>> -    g_array_unref(s->string_table_buf);
> >>> +    if (s->string_table_buf) {
> >>> +        g_array_unref(s->string_table_buf);
> >>> +    }
> >>
> >> or:
> >> g_clear_pointer(&s->string_table_buf, g_array_unref)
> >
> > Since dump_cleanup() doesn't clear any of the other members of @s, I'll
> > stick to g_array_unref() for consistency and simplicity.
>
> Wait!  You suggest *unconditional*
>
>          g_clear_pointer(&s->string_table_buf, g_array_unref)
>
> don't you?
>
> Got a preference?

Yes, I think it's safe and more future proof in general. Up to you if
you respin.

thanks

>
> >>>      s->guest_note = NULL;
> >>>      if (s->resume) {
> >>>          if (s->detached) {
> >>> --
> >>> 2.41.0
> >>>
> >
> > Thanks!
>

[PATCH 3/5] dump: Recognize "fd:" protocols on Windows hosts

[Markus Armbruster]

A few QMP command can work with named file descriptors.

The only way to create a named file descriptor used to be QMP command
getfd, which only works on POSIX hosts.  Thus, named file descriptors
were actually usable only there.

They became usable on Windows hosts when we added QMP command
get-win32-socket (commit 4cda177c601 "qmp: add 'get-win32-socket'").

Except in dump-guest-memory, because qmp_dump_guest_memory() compiles
its named file descriptor code only #if !defined(WIN32).

Compile it unconditionally, like we do for the other commands
supporting them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index d8ea364af2..a5e9a06ef1 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2130,14 +2130,12 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
         return;
     }
 
-#if !defined(WIN32)
     if (strstart(protocol, "fd:", &p)) {
         fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
             return;
         }
     }
-#endif
 
     if  (strstart(protocol, "file:", &p)) {
         fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
-- 
2.41.0

Re: [PATCH 3/5] dump: Recognize "fd:" protocols on Windows hosts

[Marc-André Lureau]

On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> A few QMP command can work with named file descriptors.
>
> The only way to create a named file descriptor used to be QMP command
> getfd, which only works on POSIX hosts.  Thus, named file descriptors
> were actually usable only there.
>
> They became usable on Windows hosts when we added QMP command
> get-win32-socket (commit 4cda177c601 "qmp: add 'get-win32-socket'").
>
> Except in dump-guest-memory, because qmp_dump_guest_memory() compiles
> its named file descriptor code only #if !defined(WIN32).
>
> Compile it unconditionally, like we do for the other commands
> supporting them.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d8ea364af2..a5e9a06ef1 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -2130,14 +2130,12 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
>          return;
>      }
>
> -#if !defined(WIN32)
>      if (strstart(protocol, "fd:", &p)) {
>          fd = monitor_get_fd(monitor_cur(), p, errp);
>          if (fd == -1) {
>              return;
>          }
>      }
> -#endif
>
>      if  (strstart(protocol, "file:", &p)) {
>          fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
> --
> 2.41.0
>

[PATCH 4/5] dump: Improve some dump-guest-memory error messages

[Markus Armbruster]

Zero @length is rejected with "Invalid parameter 'length'".  Improve
to "Parameter 'length' expects a non-zero length".

@protocol values not starting with "fd:" or "file:" are rejected with
"Invalid parameter 'protocol'".  Improve to "parameter 'protocol' must
start with 'file:' or 'fd:'".

While there, make the conditional checking @protocol a little more
obvious.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index a5e9a06ef1..d888e4bd3c 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1812,7 +1812,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
 
     s->fd = fd;
     if (has_filter && !length) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "length");
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "length",
+                   "a non-zero size");
         goto cleanup;
     }
     s->filter_area_begin = begin;
@@ -2072,7 +2073,7 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
 {
     ERRP_GUARD();
     const char *p;
-    int fd = -1;
+    int fd;
     DumpState *s;
     bool detach_p = false;
 
@@ -2135,18 +2136,15 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
         if (fd == -1) {
             return;
         }
-    }
-
-    if  (strstart(protocol, "file:", &p)) {
+    } else if  (strstart(protocol, "file:", &p)) {
         fd = qemu_open_old(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
         if (fd < 0) {
             error_setg_file_open(errp, errno, p);
             return;
         }
-    }
-
-    if (fd == -1) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
+    } else {
+        error_setg(errp,
+                   "parameter 'protocol' must start with 'file:' or 'fd:'");
         return;
     }
 
-- 
2.41.0

Re: [PATCH 4/5] dump: Improve some dump-guest-memory error messages

[Philippe Mathieu-Daudé]

Hi Markus,

On 30/10/23 14:37, Markus Armbruster wrote:
> Zero @length is rejected with "Invalid parameter 'length'".  Improve
> to "Parameter 'length' expects a non-zero length".
> 
> @protocol values not starting with "fd:" or "file:" are rejected with
> "Invalid parameter 'protocol'".  Improve to "parameter 'protocol' must
> start with 'file:' or 'fd:'".
> 
> While there, make the conditional checking @protocol a little more
> obvious.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   dump/dump.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index a5e9a06ef1..d888e4bd3c 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1812,7 +1812,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>   
>       s->fd = fd;
>       if (has_filter && !length) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "length");
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "length",

Per commit 4629ed1e98 ("qerror: Finally unused, clean up", 2015):

  /*
   * These macros will go away, please don't use in new code, ...

Instead we can use:

            error_setg(errp, "Parameter '%s' expects %s", "length",

> +                   "a non-zero size");
>           goto cleanup;
>       }

Re: [PATCH 4/5] dump: Improve some dump-guest-memory error messages

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Markus,
>
> On 30/10/23 14:37, Markus Armbruster wrote:
>> Zero @length is rejected with "Invalid parameter 'length'".  Improve
>> to "Parameter 'length' expects a non-zero length".
>>
>> @protocol values not starting with "fd:" or "file:" are rejected with
>> "Invalid parameter 'protocol'".  Improve to "parameter 'protocol' must
>> start with 'file:' or 'fd:'".
>>
>> While there, make the conditional checking @protocol a little more
>> obvious.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   dump/dump.c | 16 +++++++---------
>>   1 file changed, 7 insertions(+), 9 deletions(-)
>> diff --git a/dump/dump.c b/dump/dump.c
>> index a5e9a06ef1..d888e4bd3c 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -1812,7 +1812,8 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>         s->fd = fd;
>>       if (has_filter && !length) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER, "length");
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "length",
>
> Per commit 4629ed1e98 ("qerror: Finally unused, clean up", 2015):
>
>  /*
>   * These macros will go away, please don't use in new code, ...
>
> Instead we can use:
>
>            error_setg(errp, "Parameter '%s' expects %s", "length",

I left this to the next version of your "qapi: Kill 'qapi/qmp/qerror.h'
for good" out of laziness.  Since you prefer the deed to be done right
away, I will in v2.

>> +                   "a non-zero size");
>>           goto cleanup;
>>       }

[PATCH 5/5] dump: Drop redundant check for empty dump

[Markus Armbruster]

dump_init() first computes the size of the dump, taking the filter
area into account, and fails if its zero.  It then looks for memory in
the filter area, and fails if there is none.

This is redundant: if the size of the dump is zero, there is no
memory, and vice versa.  Delete this check.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index d888e4bd3c..03627a4c17 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1674,26 +1674,6 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
     }
 }
 
-static int validate_start_block(DumpState *s)
-{
-    GuestPhysBlock *block;
-
-    if (!dump_has_filter(s)) {
-        return 0;
-    }
-
-    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
-        /* This block is out of the range */
-        if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
-            block->target_end <= s->filter_area_begin) {
-            continue;
-        }
-        return 0;
-   }
-
-    return -1;
-}
-
 static void get_max_mapnr(DumpState *s)
 {
     GuestPhysBlock *last_block;
@@ -1842,12 +1822,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         goto cleanup;
     }
 
-    /* Is the filter filtering everything? */
-    if (validate_start_block(s) == -1) {
-        error_setg(errp, QERR_INVALID_PARAMETER, "begin");
-        goto cleanup;
-    }
-
     /* get dump info: endian, class and architecture.
      * If the target architecture is not supported, cpu_get_dump_info() will
      * return -1.
-- 
2.41.0

Re: [PATCH 5/5] dump: Drop redundant check for empty dump

[Marc-André Lureau]

On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> dump_init() first computes the size of the dump, taking the filter
> area into account, and fails if its zero.  It then looks for memory in
> the filter area, and fails if there is none.
>
> This is redundant: if the size of the dump is zero, there is no
> memory, and vice versa.  Delete this check.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c | 26 --------------------------
>  1 file changed, 26 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d888e4bd3c..03627a4c17 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1674,26 +1674,6 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
>      }
>  }
>
> -static int validate_start_block(DumpState *s)
> -{
> -    GuestPhysBlock *block;
> -
> -    if (!dump_has_filter(s)) {
> -        return 0;
> -    }
> -
> -    QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
> -        /* This block is out of the range */
> -        if (block->target_start >= s->filter_area_begin + s->filter_area_length ||
> -            block->target_end <= s->filter_area_begin) {
> -            continue;
> -        }
> -        return 0;
> -   }
> -
> -    return -1;
> -}
> -
>  static void get_max_mapnr(DumpState *s)
>  {
>      GuestPhysBlock *last_block;
> @@ -1842,12 +1822,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          goto cleanup;
>      }
>
> -    /* Is the filter filtering everything? */
> -    if (validate_start_block(s) == -1) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, "begin");
> -        goto cleanup;
> -    }
> -
>      /* get dump info: endian, class and architecture.
>       * If the target architecture is not supported, cpu_get_dump_info() will
>       * return -1.
> --
> 2.41.0
>
>


-- 
Marc-André Lureau