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@xxxxxxxxxx>
> ---
...
> 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@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
