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

[Janosch Frank]

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;
     }