On 06.05.2016 20:03, John Ferlan wrote:
>
>
> 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...
>
Oh. Thanks for catching that. I'll fix those too.
>
> Finally, I see the comments move 4 spaces to the right in
> qemuMonitorJSONSetObjectProperty? (patch 4)... Was that a remnant or
> expected?
While working on this I didn't bother with adjusting the code that much
initially. After that I've fired up my code alignment key shortcut over
each function I've touched. I will leave the comments as they were.
>
>
> 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).
>
Thanks, pushed now.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list