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

Re: [libvirt] VMware ESX driver announcement



On Thu, May 21, 2009 at 02:54:46AM +0200, Matthias Bolte wrote:
> 2009/5/19 Daniel Veillard <veillard redhat com>:
> >> diff --git a/src/esx_driver.c b/src/esx_driver.c
> >> new file mode 100644
> >> index 0000000..28500b8
> >> --- /dev/null
> >> +++ b/src/esx_driver.c
> >> @@ -0,0 +1,1385 @@
> > [...]
> >> +#define ESX_ERROR(conn, code, fmt...)                                \
> >> +    /* FIXME: remove this VIR_DEBUG0 line when libvirt provides */   \
> >> +    /*        file and line information in error messages       */   \
> >> +    VIR_DEBUG0 ("this is a dummy message to provide a source "       \
> >> +                "position for the following error...");              \
> >> +    virReportErrorHelper (conn, VIR_FROM_ESX, code, __FILE__,        \
> >> +                          __FUNCTION__, __LINE__, fmt)
> >
> >  interesting, what's the problem ?
> 
> virLogMessage() takes filename, funcname and linenr parameters and
> outputs them along with the message. This works fine for the VIR_DEBUG
> macro. virReportErrorHelper() also takes filename, funcname and linenr
> parameters but doesn't use them, because virRaiseError() doesn't
> handle this debugging information, because the virError struct has no
> place to store them. virRaiseError() also contains a comment "TODO:
> pass function name and lineno".
> 
> The VIR_DEBUG0 is part of the ESX_ERROR macro just to provide the
> filename, funcname and linenr information for debugging, because this
> information currently gets lost in virReportErrorHelper() before the
> error is reported/logged. Also virLogMessage() only outputs this
> information for VIR_LOG_DEBUG level.

I've got a patch which fixes this problem with virReportErrorHelper().
Its currently tangled up with some other code though. I'll separate
it out and post it asap to resolve this for you.


> 
> >> +static virDrvOpenStatus
> >> +esxOpen(virConnectPtr conn, virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> >> +{
> >
> >  Any idea if there is a notion of read-only connection in ESX, or is
> >  everything role based ?
> 
> It seems everything is account/group/role/privilege based. So there is
> no simple common way for a read-only connection. The user may login
> via a very restricted account that has been manually preconfigured for
> the ESX host, but there seems to be no special, default read-only
> access that the driver could use if the read-only flag is set for the
> open call.

At least the shared 'do_open' method will store the 'flags'
in the virConnectPtr object. So even if you don't use the
read only flag yourself, the public driver API entry points
in src/libvirt.c will validate it before your driver is called

> 
> >> +static int
> >> +esxDomainMigratePrepare(virConnectPtr dconn,
> >> +                        char **cookie ATTRIBUTE_UNUSED,
> >> +                        int *cookielen ATTRIBUTE_UNUSED,
> >> +                        const char *uri_in,
> >> +                        char **uri_out,
> >> +                        unsigned long flags ATTRIBUTE_UNUSED,
> >> +                        const char *dname ATTRIBUTE_UNUSED,
> >> +                        unsigned long resource ATTRIBUTE_UNUSED)
> >> +{
> >> +    if (uri_in == NULL) {
> >> +        if (virAsprintf(uri_out, "%s://%s%s",
> >> +                        dconn->uri->scheme + 4, dconn->uri->server,
> >> +                        dconn->uri->path) < 0) {
> >> +            virReportOOMError(dconn);
> >> +            return -1;
> >> +        }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >
> >  Is there really no way to check for compatibility for example ?
> 
> As in checking if a proposed migration can succeed before performing
> the actual migration? Yes there is, it's just not implemented yet and
> a note about it is missing in the code :)

The prepare method isn't really intended to verify whether the 
migration can succeed.  It is really just intended to prepare the
destination for incoming migration, if the driver needs any prep.
Xen doesn't, QEMU does. Looks like VMWare doesn't. So what you've
got here is sufficient.

> >  Hum, this is a bit over the top, xmlChar * is just an UTF-8 C string,
> > and unless someone registered specific libxml2 allocators you don't need
> > to use xmlFree, free() is sufficient. Since other pieces of libvirt
> > don't xmlFree their libxml2 allocated strings this is superfluous IMHO.
> 
> Well, the memory handling is currently written in a way that it uses
> the matching alloc/free function pairs and doesn't make any
> assumptions about miscibility of these pairs. Because the objects of
> the VI API client use VIR_ALLOC/VIR_FREE, all elements of them must
> also be allocated using VIR_ALLOC. If you say its okay to free a
> pointer allocated by libxml2 with VIR_FREE then
> esxUtil_MigrateStringFromLibXML() and esxUtil_Strdup() can be removed,
> because their sole purpose is to guarantee the use of matching
> alloc/free functions.

It is safe to free  VIR_ALLOC'd memory using free(), and to free
malloc()'d memory using VIR_FREE. Just that libvirt code should
always use the VIR_ macros - so if libraries you are calling use 
free/malloc that's completely compatible. I believe the same holds
true for libxml2, unless you have registered custom allocation
functions with it, whcih we don't do.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.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]