Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young:

quoted

On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote:

quoted

Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann:

quoted

To be honest I think struct kexec_buf is an implementation detail
inside
kexec_locate_mem_hole, made necessary because the callback functions
it
uses need to access its arguments. Callers of kexec_locate_mem_hole,
kexec_add_handover_buffer and kexec_add_buffer shouldn't need to know
it
exists.

Elaborating a bit more: the argument list for these three functions are
equal or similar because kexec_add_handover_buffer uses
kexec_add_buffer,
which uses kexec_locate_mem_hole.

It could be beneficial to have a struct to collect the arguments to
these
functions if someone using one of them would be likely to use another
one
with the same arguments. In that case, you set up kexec_buf once and
then
just pass it whenever you need to call one of those functions.

But that is unlikely to happen. A user of the kexec API will need to use
only one of these functions with a given set of arguments, so they don't
gain anything by setting up a struct.

Syntactically, I also don't think it's clearer to set struct members
instead of simply passing arguments to a function, even if the argument
list is long.

Sorry, I'm not sure I get your points but the long argument list really
looks ugly, since you are introducing more callbacks I still think a
cleanup is necessary.

kexec_buffer struct is pretty fine to be a abstract of all these buffers.

What I understood from what you said is that making the following change
results in code that is easier to understand:

@@ -650,6 +639,7 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 	const Elf_Shdr *sechdrs_c;
 	Elf_Shdr *sechdrs = NULL;
 	void *purgatory_buf = NULL;
+	struct kexec_buf buf;
 
 	/*
 	 * sechdrs_c points to section headers in purgatory and are read

@@ -757,11 +747,19 @@ static int __kexec_load_purgatory(struct kimage *image, unsigned long min,
 		buf_align = bss_align;
 
 	/* Add buffer to segment list */
-	ret = kexec_add_buffer(image, purgatory_buf, buf_sz, memsz,
-				buf_align, min, max, top_down,
-				&pi->purgatory_load_addr);
+	memset(&buf, 0, sizeof(struct kexec_buf));
+	buf.image = image;
+	buf.buffer = purgatory_buf;
+	buf.bufsz = buf_sz,
+	buf.memsz = memsz;
+	buf.buf_align = buf_align;
+	buf.buf_min = min;
+	buf.buf_max = max;
+	buf.top_down = top_down;
+	ret = kexec_add_buffer(&buf);
 	if (ret)
 		goto out;
+	pi->purgatory_load_addr = buf.mem;
 
 	/* Load SHF_ALLOC sections */
 	buf_addr = purgatory_buf;

There are 9 calls to kexec_add_buffer in the kernel (including arch/x86,
arch/powerpc/ and kernel/), plus 1 to kexec_locate_mem_hole
and 1 to kexec_add_handover_buffer, so there would be 11 places in
the code settings up kexec_buf. My opinion is that this change doesn't
improve code readability.