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

[Richard W.M. Jones]

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.

> > +    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?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/