Re: [Libguestfs] [PATCH 5/7] New APIs: add-domain and add-libvirt-dom.

On Wed, Nov 10, 2010 at 11:46:31AM +0000, Richard W.M. Jones wrote: > +# This comes from the Sys::Virt bindings. > +INPUT > +O_OBJECT_domain > + if (sv_isobject ($arg) && (SvTYPE (SvRV ($arg)) == SVt_PVMG)) > + $var = ($type)SvIV ((SV*) SvRV ($arg)); > + else { > + warn(\"${Package}::$func_name() -- $var is not a blessed SV reference\"); > + XSRETURN_UNDEF; > + } I haven't been considering this Sys::Virt type mapping to be part of the stable ABI/API, just an internal impl details. I'm wondering how other Perl XS modules allow extension, without exposing their internal typedef implementation detail.

> + /* Connect to libvirt, find the domain. */ > + conn = virConnectOpenReadOnly (libvirturi); > + if (!conn) { > + err = virGetLastError (); > + error (g, _("could not connect to libvirt (code %d, domain %d): %s"), > + err->code, err->domain, err->message); > + goto cleanup; > + } > + > + dom = virDomainLookupByName (conn, domain_name); > + if (!dom) { > + err = virConnGetLastError (conn); NB, virConnGetLastError() is deprecated because it isn't threadsafe. Instead use virGetLastError() in all places.

> + xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@dev", xpathCtx); > + if (xpfilename == NULL || > + xpfilename->nodesetval == NULL || > + xpfilename->nodesetval->nodeNr == 0) { > + xmlXPathFreeObject (xpfilename); > + xpathCtx->node = nodes->nodeTab[i]; > + xpfilename = xmlXPathEvalExpression (BAD_CAST "./source/@file", xpathCtx); > + if (xpfilename == NULL || > + xpfilename->nodesetval == NULL || > + xpfilename->nodesetval->nodeNr == 0) { > + xmlXPathFreeObject (xpfilename); > + continue; /* disk filename not found, skip this */ > + } > + } Rather than checking for @dev, and then checking for @file, it is safer to fetch @type, and then if type=='file' use @file and if type=='block' use @dev. This protects against addition of future types, which may libguestfs may not be able to treat as simple paths on a local disk.