On Mon, Nov 27, 2006 at 03:51:09PM +0900, Magnus Damm wrote:
> kexec-tools: add nr_matched argument to kexec_iomem_for_each_line()
>
> This patch passes the number of matched lines using a pointer argument
> instead of using the return value as suggested by Vivek.
>
> Signed-off-by: Magnus Damm <[EMAIL PROTECTED]>
I have to say that I don't really see the merit of this change. I think
its quite ok for functions that process a number of items, (like
sprintf()), return the number of items processed. Though I am prepared
to put it in anyway. Some more specific coments are below.
> ---
>
> kexec/crashdump-xen.c | 8 +++++---
> kexec/kexec-iomem.c | 33 +++++++++++++++++----------------
> kexec/kexec.h | 3 ++-
> 3 files changed, 24 insertions(+), 20 deletions(-)
>
> --- 0001/kexec/crashdump-xen.c
> +++ work/kexec/crashdump-xen.c 2006-11-27 15:29:09.000000000 +0900
> @@ -48,20 +48,22 @@ int xen_get_nr_phys_cpus(void)
> if (xen_phys_cpus)
> return xen_phys_cpus;
>
> - if ((cpus = kexec_iomem_for_each_line(match, NULL, NULL))) {
> + if (kexec_iomem_for_each_line(match, NULL, NULL, &cpus) == 0) {
> n = sizeof(struct crash_note_info) * cpus;
> + xen_phys_cpus = cpus;
> xen_phys_notes = malloc(n);
> if (xen_phys_notes) {
> memset(xen_phys_notes, 0, n);
> kexec_iomem_for_each_line(match,
> xen_crash_note_callback,
> + NULL,
> NULL);
> }
>
> - xen_phys_cpus = cpus;
> + return xen_phys_cpus;
> }
>
> - return cpus;
> + return -1;
> }
I don't think that the above code takes into account the case where
cpus is set to < 1 by kexec_iomem_for_each_line().
> int xen_get_note(int cpu, uint64_t *addr, uint64_t *len)
> --- 0001/kexec/kexec-iomem.c
> +++ work/kexec/kexec-iomem.c 2006-11-27 15:30:29.000000000 +0900
> @@ -17,7 +17,6 @@
> *
> * Iterate over each line in /proc/iomem. If match is NULL or if the line
> * matches with our match-pattern then call the callback if non-NULL.
> - * Return the number of lines matched.
> */
>
> int kexec_iomem_for_each_line(char *match,
> @@ -26,7 +25,8 @@ int kexec_iomem_for_each_line(char *matc
> char *str,
> unsigned long base,
> unsigned long length),
> - void *data)
> + void *data,
> + int *nr_matched)
> {
> const char iomem[]= "/proc/iomem";
> char line[MAX_LINE];
> @@ -58,7 +58,10 @@ int kexec_iomem_for_each_line(char *matc
>
> fclose(fp);
>
> - return nr;
> + if (nr_matched)
> + *nr_matched = nr;
> +
> + return 0;
> }
I notice that if kexec_iomem_for_each_line() returns, it always returns 0.
I guess that is ok as it leaves room to return -1 later if the need
arises.
> static int kexec_iomem_single_callback(void *data, int nr,
> @@ -79,23 +82,21 @@ static int kexec_iomem_single_callback(v
> int parse_iomem_single(char *str, uint64_t *start, uint64_t *end)
> {
> struct memory_range range;
> - int ret;
> + int nr;
>
> memset(&range, 0, sizeof(range));
>
> - ret = kexec_iomem_for_each_line(str, kexec_iomem_single_callback,
> - &range);
> + if (kexec_iomem_for_each_line(str, kexec_iomem_single_callback,
> + &range, &nr) == 0) {
> + if (nr == 1) {
> + if (start)
> + *start = range.start;
> + if (end)
> + *end = range.end;
>
> - if (ret == 1) {
> - if (start)
> - *start = range.start;
> - if (end)
> - *end = range.end;
> -
> - ret = 0;
> + return 0;
> + }
> }
> - else
> - ret = -1;
>
> - return ret;
> + return -1;
> }
I think that something like the following is a bit clearer than the
fragment above.
@@ -79,23 +82,18 @@
int parse_iomem_single(char *str, uint64_t *start, uint64_t *end)
{
struct memory_range range;
- int ret;
+ int nr;
memset(&range, 0, sizeof(range));
- ret = kexec_iomem_for_each_line(str, kexec_iomem_single_callback,
- &range);
-
- if (ret == 1) {
+ if (kexec_iomem_for_each_line(str, kexec_iomem_single_callback,
+ &range, &nr) < 0 || nr != 1) {
if (start)
*start = range.start;
if (end)
*end = range.end;
-
- ret = 0;
+ return 0;
}
- else
- ret = -1;
- return ret;
+ return -1;
}
> --- 0001/kexec/kexec.h
> +++ work/kexec/kexec.h 2006-11-27 15:24:35.000000000 +0900
> @@ -207,7 +207,8 @@ int kexec_iomem_for_each_line(char *matc
> char *str,
> unsigned long base,
> unsigned long length),
> - void *data);
> + void *data,
> + int *nr_matched);
> int parse_iomem_single(char *str, uint64_t *start, uint64_t *end);
>
>
_______________________________________________
fastboot mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/fastboot
