On Thu, Dec 07, 2017 at 10:22:57AM +0100, Michal Privoznik wrote:
Firstly, this API is creating $domName-install for installation and at the same time it defines $domName (but never runs it). This is not very optimal - libvirt can handle two definitions for a single domain (active and inactive ones).
Secondly, this function is leaking domain objects on any error.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
...
src/libvirt-domain.c | 65 +++++++++++++++++++++++++++++++++++-----------------
snprintf(vncl, sizeof(vncl), "%s:%s", virConnectGetHostname(conn->conn), tmp); @@ -195,23 +204,25 @@ PHP_FUNCTION(libvirt_domain_new) set_vnc_location(vncl TSRMLS_CC);
tmp = installation_get_xml(conn->conn, name, memMB, maxmemMB, - NULL /* arch */, NULL, vcpus, NULL, + NULL /* arch */, uuid, vcpus, NULL, vmDisks, numDisks, vmNetworks, numNets, flags TSRMLS_CC); if (tmp == NULL) { DPRINTF("%s: Cannot get installation XML, step 2\n", PHPFUNC); set_error("Cannot get installation XML, step 2" TSRMLS_CC);
Since you dropped the step1/2 notation from installation_get_xml in patch 1, this error message should be rephrased as well, I'd maybe prefer to have that change in patch 1, but I don't really care that much. Reviewed-by: Erik Skultety <eskultet@redhat.com>