Re: [PATCH 07/13] kexec: Implementation of new syscall kexec_file_load

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Jun 06, 2014 at 04:11:33AM +0200, Borislav Petkov wrote:

[..]
> > > Might wanna define pr_fmt when using the pr_* things fo the first time
> > > in this file.
> > 
> > Hmm....
> > 
> > I see that printk.h already provides a definition is pr_fmt is not
> > defined. So that means I shouldn't have to define pr_fmt() before I
> > use pr_*?
> > 
> > #ifndef pr_fmt
> > #define pr_fmt(fmt) fmt
> > #endif
> 
> Yep, so you could do
> 
> #undef pr_fmt
> #define pr_fmt(fmt) "kexec: "
> 
> or you can do the standard
> 
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> Just look around the tree for examples, there's plenty.

Ok, got it. So this will allow me to prefix subsystem name to the
message. It is a good idea. Will do.

[..]
> > > > +	kbuf->buf_align = max(buf_align, PAGE_SIZE);
> > > > +	kbuf->buf_min = buf_min;
> > > > +	kbuf->buf_max = buf_max;
> > > > +	kbuf->top_down = top_down;
> > > > +
> > > > +	/* Walk the RAM ranges and allocate a suitable range for the buffer */
> > > > +	walk_system_ram_res(0, -1, kbuf, walk_ram_range_callback);
> > > > +
> > > > +	/*
> > > > +	 * If range could be found successfully, it would have incremented
> > > > +	 * the nr_segments value.
> > > > +	 */
> > > > +	new_nr_segments = image->nr_segments;
> > > > +
> > > > +	/* A suitable memory range could not be found for buffer */
> > > > +	if (new_nr_segments == nr_segments)
> > > > +		return -EADDRNOTAVAIL;
> > > 
> > > Right, why don't you check walk_system_ram_res's retval? If it is != 0,
> > > i.e. walk_ram_range_callback gives a 1 on "success", you can drop this
> > > way of checking whether finding a new range succeeded.
> > 
> > In last version when I had ELF header support, I was checking for return
> > code 1 at one place and you had not liked that.
> > 
> > Anyway, I am thinking that problem here is that walk_* variants use
> > return code of called function to decide whether to continue looping
> > or not. I think these are two independent activities.  Pass a boolean
> > to called function which should be set to false if callee wants to
> > stop the loop.
> > 
> > That way, callee can pass both errors and success without having to
> > worry about loop. And callee can return 0 to represent success and
> > negative error code to represent error.
> 
> But why? It should be caller's responsibility to deal with the errors.
> If it encounters one, it either decides to stop looping or not.

There are cases where there is no error still looping needs to stop.

For example, suppose you are looking for a memory range of size x
between addresses A and B. Assume there are 20 SYSTEM RAM entries
between address A and B. Now lets say you found a suitable range
in the first call itself. In that called function is successful
and does not want to be called again.

But upon returning success "0", walk_* functions will continue to
call with rest of the overlapping ranges. Its seems pretty wasteful
and called function will have to keep a state which tells that
ignore further calls.

Now to stop looping we can't return error as that return code
will be passed to the function who called walk_* and that function
will think that error happened. But actually it did not.

So to me it makes sense to decouple two things. Error code and when
to stop looping.

> 
> In any case, you don't need a second bool arg to pass around.

I would say give it some more thought. It makes dealing with errors
easy.

> 
> If you want to make it more explicit, you could do
> 
> #define RES_OK		0
> #define RES_ERR		1
> #define RES_STOP	2

You are saying that called back function should return this to walk_*
functions? But then we lose the actual error code which should be
passed to parent function which actually called walk_* function.

Thanks
Vivek