[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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:
> diff --git a/perl/typemap b/perl/typemap
> index d978e60..223aee1 100644
> --- a/perl/typemap
> +++ b/perl/typemap
> @@ -3,6 +3,7 @@ char *		T_PV
>  const char *	T_PV
>  guestfs_h *	O_OBJECT_guestfs_h
>  int64_t	  	T_IV
> +virDomainPtr O_OBJECT_domain
>  
>  INPUT
>  O_OBJECT_guestfs_h
> @@ -18,6 +19,16 @@ O_OBJECT_guestfs_h
>          croak (\"${Package}::$func_name(): $var is not a blessed HV reference\");
>      }
>  
> +# 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.

> diff --git a/src/virt.c b/src/virt.c
> new file mode 100644
> index 0000000..5816f61
> --- /dev/null
> +++ b/src/virt.c
> +
> +int
> +guestfs__add_domain (guestfs_h *g, const char *domain_name,
> +                     const struct guestfs_add_domain_argv *optargs)
> +{
> +  virErrorPtr err;
> +  virConnectPtr conn = NULL;
> +  virDomainPtr dom = NULL;
> +  int r = -1;
> +  const char *libvirturi;
> +  int readonly;
> +  const char *iface;
> +  struct guestfs_add_libvirt_dom_argv optargs2 = { .bitmask = 0 };
> +
> +  libvirturi = optargs->bitmask & GUESTFS_ADD_DOMAIN_LIBVIRTURI_BITMASK
> +               ? optargs->libvirturi : NULL;
> +  readonly = optargs->bitmask & GUESTFS_ADD_DOMAIN_READONLY_BITMASK
> +             ? optargs->readonly : 0;
> +  iface = optargs->bitmask & GUESTFS_ADD_DOMAIN_IFACE_BITMASK
> +          ? optargs->iface : NULL;
> +
> +  /* 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.

> +    error (g, _("no libvirt domain called '%s': %s"),
> +           domain_name, err->message);
> +    goto cleanup;
> +  }
> +
> +  if (readonly) {
> +    optargs2.bitmask |= GUESTFS_ADD_LIBVIRT_DOM_READONLY_BITMASK;
> +    optargs2.readonly = readonly;
> +  }
> +  if (iface) {
> +    optargs2.bitmask |= GUESTFS_ADD_LIBVIRT_DOM_IFACE_BITMASK;
> +    optargs2.iface = iface;
> +  }
> +
> +  r = guestfs__add_libvirt_dom (g, dom, &optargs2);
> +
> + cleanup:
> +  if (dom) virDomainFree (dom);
> +  if (conn) virConnectClose (conn);
> +
> +  return r;
> +}
> +
> +int
> +guestfs__add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
> +                          const struct guestfs_add_libvirt_dom_argv *optargs)
> +{
> +  int r = -1, nr_added = 0, i;
> +  virErrorPtr err;
> +  virConnectPtr conn = virDomainGetConnect (dom);
> +  xmlDocPtr doc = NULL;
> +  xmlXPathContextPtr xpathCtx = NULL;
> +  xmlXPathObjectPtr xpathObj = NULL;
> +  char *xml = NULL;
> +  int readonly;
> +  const char *iface;
> +  int cmdline_pos;
> +
> +  cmdline_pos = guestfs___checkpoint_cmdline (g);
> +
> +  readonly = optargs->bitmask & GUESTFS_ADD_LIBVIRT_DOM_READONLY_BITMASK
> +             ? optargs->readonly : 0;
> +  iface = optargs->bitmask & GUESTFS_ADD_LIBVIRT_DOM_IFACE_BITMASK
> +          ? optargs->iface : NULL;
> +
> +  if (!readonly) {
> +    virDomainInfo info;
> +    if (virDomainGetInfo (dom, &info) == -1) {
> +      err = virConnGetLastError (conn);
> +      error (g, _("error getting domain info: %s"), err->message);
> +      goto cleanup;
> +    }
> +    if (info.state != VIR_DOMAIN_SHUTOFF) {
> +      error (g, _("error: domain is a live virtual machine.\nYou must use readonly access because write access to a running virtual machine\ncan cause disk corruption."));
> +      goto cleanup;
> +    }
> +  }
> +
> +  /* Domain XML. */
> +  xml = virDomainGetXMLDesc (dom, 0);
> +
> +  if (!xml) {
> +    err = virConnGetLastError (conn);
> +    error (g, _("error reading libvirt XML information: %s"),
> +           err->message);
> +    goto cleanup;
> +  }
> +
> +  /* Now the horrible task of parsing out the fields we need from the XML.
> +   * http://www.xmlsoft.org/examples/xpath1.c
> +   */
> +  doc = xmlParseMemory (xml, strlen (xml));
> +  if (doc == NULL) {
> +    error (g, _("unable to parse XML information returned by libvirt"));
> +    goto cleanup;
> +  }
> +
> +  xpathCtx = xmlXPathNewContext (doc);
> +  if (xpathCtx == NULL) {
> +    error (g, _("unable to create new XPath context"));
> +    goto cleanup;
> +  }
> +
> +  /* This gives us a set of all the <disk> nodes. */
> +  xpathObj = xmlXPathEvalExpression (BAD_CAST "//devices/disk", xpathCtx);
> +  if (xpathObj == NULL) {
> +    error (g, _("unable to evaluate XPath expression"));
> +    goto cleanup;
> +  }
> +
> +  xmlNodeSetPtr nodes = xpathObj->nodesetval;
> +  for (i = 0; i < nodes->nodeNr; ++i) {
> +    xmlXPathObjectPtr xpfilename;
> +    xmlXPathObjectPtr xpformat;
> +
> +    /* Change the context to the current <disk> node.
> +     * DV advises to reset this before each search since older versions of
> +     * libxml2 might overwrite it.
> +     */
> +    xpathCtx->node = nodes->nodeTab[i];
> +
> +    /* Filename can be in <source dev=..> or <source file=..> attribute. */
> +    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.

> +
> +    assert (xpfilename->nodesetval->nodeTab[0]);
> +    assert (xpfilename->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
> +    xmlAttrPtr attr = (xmlAttrPtr) xpfilename->nodesetval->nodeTab[0];
> +    char *filename = (char *) xmlNodeListGetString (doc, attr->children, 1);
> +
> +    /* Get the disk format (may not be set). */
> +    xpathCtx->node = nodes->nodeTab[i];
> +    xpformat = xmlXPathEvalExpression (BAD_CAST "./driver/@type", xpathCtx);
> +    char *format = NULL;
> +    if (xpformat != NULL &&
> +        xpformat->nodesetval &&
> +        xpformat->nodesetval->nodeNr > 0) {
> +      assert (xpformat->nodesetval->nodeTab[0]);
> +      assert (xpformat->nodesetval->nodeTab[0]->type == XML_ATTRIBUTE_NODE);
> +      attr = (xmlAttrPtr) xpformat->nodesetval->nodeTab[0];
> +      format = (char *) xmlNodeListGetString (doc, attr->children, 1);
> +    }

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]