| Message ID | 20220726092248.128336-5-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | dump: Add arch section and s390x PV dump | expand |
On Tue, Jul 26, 2022 at 1:23 PM Janosch Frank <frankja@linux.ibm.com> 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. 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() > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > dump/dump.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 35b9833a00..b59faf9941 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -1498,30 +1498,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; > } > @@ -1668,8 +1660,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; > } > -- > 2.34.1 >
On 7/26/22 11:22, 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. 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() > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> See suggenstions below. > --- > dump/dump.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 35b9833a00..b59faf9941 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -1498,30 +1498,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; > } If you change the dump_get_memblock_* functions to take the DumpState, you could do bool has_unfiltered_mem(DumpState *s) { GuestPhysBlock *block; QTAILQ_FOREACH(block, &s->guest_phys_blocks.head, next) { if (dump_get_memblock_size(block, s) > 0) { return true; } } return false; } or you could do the same with the existing dump_get_memblock_size, add if (has_filter && !length) { error_setg(errp, QERR_INVALID_PARAMETER, "length"); goto cleanup; } to dump_init, encode has_filter in length as you're currently doing and get rid of s->has_filter entirely. > @@ -1668,8 +1660,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; > }
On 7/26/22 11:22, 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. 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() > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com> > --- > dump/dump.c | 20 ++++++-------------- > 1 file changed, 6 insertions(+), 14 deletions(-) > > diff --git a/dump/dump.c b/dump/dump.c > index 35b9833a00..b59faf9941 100644 > --- a/dump/dump.c > +++ b/dump/dump.c > @@ -1498,30 +1498,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; > } > @@ -1668,8 +1660,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/dump/dump.c b/dump/dump.c index 35b9833a00..b59faf9941 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -1498,30 +1498,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; } @@ -1668,8 +1660,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; }
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. 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() Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- dump/dump.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)