[Libguestfs] [PATCH 1/4] v2v: domainxml: factor out connect and pool loading

[Richard W.M. Jones]

On Tue, Apr 14, 2015 at 05:05:39PM +0200, Pino Toscano wrote:
> Factor out the connection and pool loading out of v2v_pool_dumpxml, so
> it can be used in later implementations requiring a pool.
> 
> Should be just code motion.
> ---
>  v2v/domainxml-c.c | 84 +++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 33 deletions(-)
> 
> diff --git a/v2v/domainxml-c.c b/v2v/domainxml-c.c
> index 4224d72..077c153 100644
> --- a/v2v/domainxml-c.c
> +++ b/v2v/domainxml-c.c
> @@ -106,6 +106,55 @@ libvirt_auth_default_wrapper (virConnectCredentialPtr cred,
>    }
>  }
>  
> +virStoragePoolPtr
> +connect_and_load_pool (const char *conn_uri, const char *pool_name)
> +{
> +  /* We have to assemble the error on the stack because a dynamic
> +   * string couldn't be freed.
> +   */
> +  char errmsg[256];
> +  virErrorPtr err;
> +  virConnectPtr conn;
> +  virStoragePoolPtr pool;
> +
> +  /* We have to call the default authentication handler, not least
> +   * since it handles all the PolicyKit crap.  However it also makes
> +   * coding this simpler.
> +   */
> +  conn = virConnectOpenAuth (conn_uri, virConnectAuthPtrDefault,
> +                             VIR_CONNECT_RO);
> +  if (conn == NULL) {
> +    if (conn_uri)
> +      snprintf (errmsg, sizeof errmsg,
> +                _("cannot open libvirt connection '%s'"), conn_uri);
> +    else
> +      snprintf (errmsg, sizeof errmsg, _("cannot open libvirt connection"));
> +    caml_invalid_argument (errmsg);
> +  }
> +
> +  /* Suppress default behaviour of printing errors to stderr.  Note
> +   * you can't set this to NULL to ignore errors; setting it to NULL
> +   * restores the default error handler ...
> +   */
> +  virConnSetErrorFunc (conn, NULL, ignore_errors);
> +
> +  /* Look up the pool. */
> +  pool = virStoragePoolLookupByUUIDString (conn, pool_name);
> +
> +  if (!pool)
> +    pool = virStoragePoolLookupByName (conn, pool_name);
> +
> +  if (!pool) {
> +    err = virGetLastError ();
> +    snprintf (errmsg, sizeof errmsg,
> +              _("cannot find libvirt pool '%s': %s"), pool_name, err->message);
> +    virConnectClose (conn);
> +    caml_invalid_argument (errmsg);
> +  }
> +
> +  return pool;
> +}
> +
>  value
>  v2v_dumpxml (value passwordv, value connv, value domnamev)
>  {
> @@ -230,42 +279,11 @@ v2v_pool_dumpxml (value connv, value poolnamev)
>    if (connv != Val_int (0))
>      conn_uri = String_val (Field (connv, 0)); /* Some conn */
>  
> -  /* We have to call the default authentication handler, not least
> -   * since it handles all the PolicyKit crap.  However it also makes
> -   * coding this simpler.
> -   */
> -  conn = virConnectOpenAuth (conn_uri, virConnectAuthPtrDefault,
> -                             VIR_CONNECT_RO);
> -  if (conn == NULL) {
> -    if (conn_uri)
> -      snprintf (errmsg, sizeof errmsg,
> -                _("cannot open libvirt connection '%s'"), conn_uri);
> -    else
> -      snprintf (errmsg, sizeof errmsg, _("cannot open libvirt connection"));
> -    caml_invalid_argument (errmsg);
> -  }
> -
> -  /* Suppress default behaviour of printing errors to stderr.  Note
> -   * you can't set this to NULL to ignore errors; setting it to NULL
> -   * restores the default error handler ...
> -   */
> -  virConnSetErrorFunc (conn, NULL, ignore_errors);
> -
>    /* Look up the pool. */
>    poolname = String_val (poolnamev);
>  
> -  pool = virStoragePoolLookupByUUIDString (conn, poolname);
> -
> -  if (!pool)
> -    pool = virStoragePoolLookupByName (conn, poolname);
> -
> -  if (!pool) {
> -    err = virGetLastError ();
> -    snprintf (errmsg, sizeof errmsg,
> -              _("cannot find libvirt pool '%s': %s"), poolname, err->message);
> -    virConnectClose (conn);
> -    caml_invalid_argument (errmsg);
> -  }
> +  pool = connect_and_load_pool (conn_uri, poolname);
> +  conn = virStoragePoolGetConnect (pool);
>  
>    xml = virStoragePoolGetXMLDesc (pool, 0);
>    if (xml == NULL) {

ACK.

Although this code doesn't suffer from it [I don't think], be aware
that the char * returned by String_val() may become suddenly invalid
if any OCaml allocation function is called (eg. caml_copy_string,
caml_alloc*, or any indirect function that could call those).  The
reason is that a call into an allocation function might trigger the
garbage collector, causing especially a string on the minor heap to
move to the major heap (and other possibilities too).

As a result of this I'm always suspicious of code that calls
String_val, stashes the result in a variable, and then uses the result
over long stretches of code.  It's safer to call String_val() multiple
times whenever it is needed (String_val is a no-op because
value == char * for OCaml strings).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v