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

[Daniel P. Berrange]

On Wed, Nov 10, 2010 at 01:23:50PM +0000, Richard W.M. Jones wrote:
> On Wed, Nov 10, 2010 at 01:17:54PM +0000, Daniel P. Berrange wrote:
> > 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.
> 
> Yup, this is a general problem with providing integration for these
> pointers through any non-C language bindings.  It could be alleviated
> by having Sys::Virt provide a little bit of C to perform the SV ->
> pointer conversion -- I think if it was in Sys/Virt/Virt.so then we
> would be able to link to it.
> 
> > > +  /* 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.
> 
> Can I call this if I don't have a connection pointer?  See first case
> above.

/**
 * virGetLastError:
 *
 * Provide a pointer to the last error caught at the library level
 *
 * The error object is kept in thread local storage, so separate
 * threads can safely access this concurrently.
 *
 */

/**
 * virConnGetLastError:
 * @conn: pointer to the hypervisor connection
 *
 * Provide a pointer to the last error caught on that connection
 *
 * This method is not protected against access from multiple
 * threads. In a multi-threaded application, always use the
 * global virGetLastError() API which is backed by thread
 * local storage.
 *
 * If the connection object was discovered to be invalid by
 * an API call, then the error will be reported against the
 * global error object.
 *
 * Since 0.6.0, all errors reported in the per-connection object
 * are also duplicated in the global error object. As such an
 * application can always use virGetLastError(). This method
 * remains for backwards compatability.
 */


> > > +    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.
> 
> OK I will change this.  Was there a reason to split dev and file in
> the first place?

I don't really know

Regards.
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|