Hi Chunyan, On Mon, Mar 10, 2014 at 03:31:36PM +0800, Chunyan Liu wrote: > This patch series is to replace QEMUOptionParameter with QemuOpts, so that > only > one Qemu Option structure is kept in QEMU code. >
Last night I took some time do take a deeper look at you series and the required effort to do the QemuOptionParameter -> QemuOpts migration. I think you've over complicated the things, I understand you tried to keep your serie's bisectability (?), but the final result was something really hard to review and to integrate as well. The overall approach wasn't even resolving the bisectability problem since it breaks the tree until the last commit. Moreover, in the path of getting things ready you created new problems and their respective fixes, what we really don't need to. In this regards you could have kept things as simple as possible and submitted the patches in a "natural way", even if they were breaking the build between each patch, you could get all the required maintainer's Reviewed-by + Tested-by + Signed-off-by and so on for each individual patch and when it was time to integrate get squashed the needed patches. I mean, add N patches introducing new required QemuOpts API's, 1 patch migrating the block upper layer (block.c, block.h, etc), one patch for each block driver (i.e ssh.c, qcow.c, qcow2.c, etc), one patch for qemu-img.c and finally a last patch removing the QEMUOptionParamer itself. When time comes to integrate your series the patches changing the block layer + patches changing the block drivers + patches changing qemu-img.c could be squashed adding all the collected Reviewed-by to this single squashed patch. As I said, last night I took a deeper look at the problem and, understood most of changes weren't required to do the job. We don't need an adaptation layer between QemuOptionParameter and QemuOpts, we don't need to add new opts accessors (like those qemu_opt_*_del() functions), all we need is 1) that qemu_opts_append() function so we can merge the protocol and drivers options in a single QemuOptList and 2) the default value support. All we need is already present in the QemuOpts APIs. With that simpler approach in mind I ended up putting my hands in the source code trying to see how feasible it is, and turns out I came up with a full solution. I'm sending the job's resulting series to the mailing list so I can show you what I mean and have some more room for discussion. It doesn't mean I want to overlap you work, I just needed to have a little more input on that. Regards.... -- Leandro Dorileo
