[Libguestfs] [PATCH 1/4] v2v: domainxml: factor out connect and pool loading
Richard W.M. Jones
rjones at redhat.com
Tue Apr 14 16:14:50 UTC 2015
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
More information about the Libguestfs
mailing list