[Spice-devel] VM pools and libgovirt

[i iordanov]

Hi Christophe,

On Tue, Dec 3, 2013 at 5:12 AM, Christophe Fergeau <cfergeau at redhat.com>wrote:

> Thanks for the patch, a few quick initial comments:
> - formatting of the commit log was off, see (for example)
>
> https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_GIT_commit_message_structure
>   for the recommended format of git commit logs (helps when using 'git
> shortlog' for example)
>   This document as a whole is an interesting read imo ;)
>

Thanks for the tip!


> - I'd use ovirt_vmpool_allocate_vm (rather than _allocatevm)
>

I used allocatevm because the action is actually "allocatevm" not
"allocate_vm", but if you think the function name doesn't need to be in
line with the action, then I agree.


> - imo the ovirt_invoke_action stuff should be moved to a generic place
>   before this patch, but I can look into that
>

Yes, it's basically duplicated code, but I didn't think it was up to me to
restructure :).


> - there were a few minor white space issues (see patch below)
>

My editor was misconfigured...


> - you need to export the new public functions in govirt/govirt.sym (see
>   patch below)
>

Yes, I meant to ask you about that, but I wasn't sure what version to put,
so I left it up to you (which was a good idea I think :))


Would you like me to generate a patch with all the suggested changes and a
better log entry?

Cheers!
iordan

-- 
The conscious mind has only one thread of execution.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20131203/ad48edd8/attachment.html>