Re: [PATCH v5 04/18] dump: Rework get_start_block

[Janosch Frank <frankja@linux.ibm.com>]

On 8/29/22 22:17, Janis Schoetterl-Glausch wrote:
> On Thu, 2022-08-11 at 12:10 +0000, Janosch Frank wrote:
>> get_start_block() returns the start address of the first memory block
>> or -1.
>>
>> With the GuestPhysBlock iterator conversion we don't need to set the
>> start address and can therefore remove that code and the "start"
>> DumpState struct member. The only functionality left is the validation
>> of the start block so it only makes sense to re-name the function to
>> validate_start_block()
> 
> Nit, since you don't return an address anymore, I find retaining the -
> 1/0 return value instead of true/false weird.

I'm trying to wrap up fixing things for a new version.
My fix for this is calling it is_start_block_valid() and making the 
return bool. That being said, having int for true/false is not uncommon 
in C but explicitly checking -1 makes it look weird.



diff --git i/dump/dump.c w/dump/dump.c
index e204912a89..2239bd324e 100644
--- i/dump/dump.c
+++ w/dump/dump.c
@@ -1500,12 +1500,12 @@ static void create_kdump_vmcore(DumpState *s, 
Error **errp)
      }
  }

-static int validate_start_block(DumpState *s)
+static bool is_start_block_valid(DumpState *s)
  {
      GuestPhysBlock *block;

      if (!s->has_filter) {
-        return 0;
+        return true;
      }

      QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) {
@@ -1514,10 +1514,10 @@ static int validate_start_block(DumpState *s)
              block->target_end <= s->begin) {
              continue;
          }
-        return 0;
+        return true;
     }

-    return -1;
+    return false;
  }

  static void get_max_mapnr(DumpState *s)
@@ -1663,7 +1663,7 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
      }

      /* Is the filter filtering everything? */
-    if (validate_start_block(s) == -1) {
+    if (!is_start_block_valid(s)) {
          error_setg(errp, QERR_INVALID_PARAMETER, "begin");
          goto cleanup;
      }