| Message ID | 20220811121111.9878-5-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
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. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > dump/dump.c | 20 ++++++-------------- > include/sysemu/dump.h | 2 -- > 2 files changed, 6 insertions(+), 16 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 340de5a1e7..e204912a89 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -1500,30 +1500,22 @@ static void create_kdump_vmcore(DumpState *s, Error **errp) > } > } > > -static ram_addr_t get_start_block(DumpState *s) > +static int validate_start_block(DumpState *s) > { > GuestPhysBlock *block; > > if (!s->has_filter) { > - s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head); > return 0; > } > > QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { > + /* This block is out of the range */ > if (block->target_start >= s->begin + s->length || > block->target_end <= s->begin) { > - /* This block is out of the range */ > continue; > } > - > - s->next_block = block; > - if (s->begin > block->target_start) { > - s->start = s->begin - block->target_start; > - } else { > - s->start = 0; > - } > - return s->start; > - } > + return 0; > + } > > return -1; > } > @@ -1670,8 +1662,8 @@ static void dump_init(DumpState *s, int fd, bool has_format, > goto cleanup; > } > > - s->start = get_start_block(s); > - if (s->start == -1) { > + /* Is the filter filtering everything? */ > + if (validate_start_block(s) == -1) { > error_setg(errp, QERR_INVALID_PARAMETER, "begin"); > goto cleanup; > } > diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h > index ffc2ea1072..7fce1d4af6 100644 > --- a/include/sysemu/dump.h > +++ b/include/sysemu/dump.h > @@ -166,8 +166,6 @@ typedef struct DumpState { > hwaddr memory_offset; > int fd; > > - GuestPhysBlock *next_block; > - ram_addr_t start; > bool has_filter; > int64_t begin; > int64_t length;
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; }
diff --git a/dump/dump.c b/dump/dump.c index 340de5a1e7..e204912a89 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -1500,30 +1500,22 @@ static void create_kdump_vmcore(DumpState *s, Error **errp) } } -static ram_addr_t get_start_block(DumpState *s) +static int validate_start_block(DumpState *s) { GuestPhysBlock *block; if (!s->has_filter) { - s->next_block = QTAILQ_FIRST(&s->guest_phys_blocks.head); return 0; } QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { + /* This block is out of the range */ if (block->target_start >= s->begin + s->length || block->target_end <= s->begin) { - /* This block is out of the range */ continue; } - - s->next_block = block; - if (s->begin > block->target_start) { - s->start = s->begin - block->target_start; - } else { - s->start = 0; - } - return s->start; - } + return 0; + } return -1; } @@ -1670,8 +1662,8 @@ static void dump_init(DumpState *s, int fd, bool has_format, goto cleanup; } - s->start = get_start_block(s); - if (s->start == -1) { + /* Is the filter filtering everything? */ + if (validate_start_block(s) == -1) { error_setg(errp, QERR_INVALID_PARAMETER, "begin"); goto cleanup; } diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h index ffc2ea1072..7fce1d4af6 100644 --- a/include/sysemu/dump.h +++ b/include/sysemu/dump.h @@ -166,8 +166,6 @@ typedef struct DumpState { hwaddr memory_offset; int fd; - GuestPhysBlock *next_block; - ram_addr_t start; bool has_filter; int64_t begin; int64_t length;