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

[horms at verge.net.au (Horms)]

On Mon, Nov 27, 2006 at 05:34:33PM +0900, Magnus Damm wrote:
> On Mon, 2006-11-27 at 17:02 +0900, Horms wrote:
> > 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.
> 
> I totally agree with you - I don't see the merit either. I think the
> code just becomes messier with this change.
> 
> Vivek, what do you think? Do you have a better implementation?
> 
> > > ---
> > > 
> > >  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().
> 
> The nr variable in kexec_iomem_for_each_line() starts counting from 0.
> So it should never set cpus to less than 0.

I was actually most worried about the 0 case. I am guessing that
malloc(0) and memset(x, y, 0), is ok, but it doesn't make me feel very 
comfortable.

As for cpus < 0, which doesn't occur, perhaps the prototype of
kexec_iomem_for_each_line() should specify an unsigned int rather than
an int.

> 
> > >  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.
> 
> Yeah, I did not want to convert the die-call to return -1. I felt it was
> out of the scope for this particular patch.

Agreed.

> > >  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) {
> 
> Uhm, the code may be clearer but I think you should test for == 0 && nr
> == 1 instead.

Yes, sorry I mucked up my patch a bit.

> >  		if (start)
> >  			*start = range.start;
> >  		if (end)
> >  			*end = range.end;
> > -
> > -		ret = 0;
> > +		return 0;
> >  	}
> > -	else
> > -		ret = -1;
> >  
> > -	return ret;
> > +	return -1;
> >  }