[libvirt] [libvirt-php][PATCH 2/7] Rework libvirt_domain_new a bit

[Erik Skultety]

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 at 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 at redhat.com>