<div dir="ltr">Hi Christophe,<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 3, 2013 at 5:12 AM, Christophe Fergeau <span dir="ltr"><<a href="mailto:cfergeau@redhat.com" target="_blank">cfergeau@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the patch, a few quick initial comments:<br>
- formatting of the commit log was off, see (for example)<br>
  <a href="https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_GIT_commit_message_structure" target="_blank">https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_GIT_commit_message_structure</a><br>
  for the recommended format of git commit logs (helps when using 'git shortlog' for example)<br>
  This document as a whole is an interesting read imo ;)<br></blockquote><div><br></div><div>Thanks for the tip!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

- I'd use ovirt_vmpool_allocate_vm (rather than _allocatevm)<br></blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- imo the ovirt_invoke_action stuff should be moved to a generic place<br>
  before this patch, but I can look into that<br></blockquote><div><br></div><div>Yes, it's basically duplicated code, but I didn't think it was up to me to restructure :).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

- there were a few minor white space issues (see patch below)<br></blockquote><div><br></div><div>My editor was misconfigured...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

- you need to export the new public functions in govirt/govirt.sym (see<br>
  patch below)<br></blockquote><div><br></div><div>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 :))<br><br><br></div><div>Would you like me to generate a patch with all the suggested changes and a better log entry?<br>
<br>Cheers!<br>iordan<br></div></div><br>-- <br>The conscious mind has only one thread of execution.
</div></div></div>