[PATCH] kexec-tools: add nr_matched argument to kexec_iomem_for_each_line()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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 <magnus at valinux.co.jp>

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


[Netdev]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Photo]     [Yosemite]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Linux Media]     [Linux Resources]

Powered by Linux