Re: [libvirt] [PATCH 0/4] qemu_monitor_json: Refactor even more

On 05/04/2016 08:33 AM, Michal Privoznik wrote: > All these patches will be squashed into one, but I've split them > into multiple because of easier review. > > Michal Privoznik (4): > qemu_monitor_json: Follow refactor > qemu_monitor_json: Follow refactor > qemu_monitor_json: Follow refactor > qemu_monitor_json: Follow refactor > > src/qemu/qemu_monitor_json.c | 673 ++++++++++++++++++++++--------------------- > 1 file changed, 349 insertions(+), 324 deletions(-) > Not a deal breaker, but most code has: if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; ret = 0; cleanup: while a few instances [1] have: ret = qemuMonitorJSONCheckError(cmd, reply); cleanup: IDC either way since the result is the same, but I suppose they "could" be all done the same way. I'd probably lean towards the latter, but since you already went with the former in commit '7884d089' it's probably best to keep using that. [1] qemuMonitorJSONSetMigrationCompression qemuMonitorJSONBlockCommit qemuMonitorJSONInjectNMI [2] qemuMonitorJSONSendKey [2] qemuMonitorJSONSetMigrationCapability [2] both instances use the same HMP call attempt to return ret - they could follow the qemuMonitorJSONSetCPU model...

Finally, I see the comments move 4 spaces to the right in qemuMonitorJSONSetObjectProperty? (patch 4)... Was that a remnant or expected?

ACK series (your call if you want to adjust [2] now or in a followup patch. it's trivial enough as long as you remember to also remove the {} around what would become "one line" if then else statements).