Am Donnerstag, 30 Juni 2016, 17:43:57 schrieb Dave Young: > On 06/30/16 at 01:42pm, Thiago Jung Bauermann wrote: > > Am Donnerstag, 30 Juni 2016, 12:49:44 schrieb Thiago Jung Bauermann: > > > 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. Also, I think that kexec_buf abstracts something that, from the perspective of the user of the kexec API, lives only for the duration of a single call to either of kexec_add_buffer, kexec_locate_mem_hole, or kexec_add_handover_buffer. Because of this, there's no need from the perspective of the API user to initialize this "object", so this just adds to their cognitive load without any benefit to them. I understand that this is all somewhat subjective, so if you still disagree with my points I can provide a patch set implementing the change above. []'s Thiago Jung Bauermann IBM Linux Technology Center
