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

Re: [PATCH] Re: [Libvir] Segfault with invalid virConnectPtr



On Wed, Sep 12, 2007 at 12:04:45PM +0100, Richard W.M. Jones wrote:
> Richard W.M. Jones wrote:
> >Program terminated with signal 11, Segmentation fault.
> >#0  0x0000003d8b472a1b in free () from /lib64/libc.so.6
> >(gdb) bt
> >#0  0x0000003d8b472a1b in free () from /lib64/libc.so.6
> >#1  0x00002aaaaaae8dd7 in virResetError (err=0x33535c8) at virterror.c:111
> >#2  0x00002aaaaaae8fce in __virRaiseError (conn=0x33535a0, dom=0x0, 
> >net=0x0,
> >    domain=0, code=6, level=VIR_ERR_ERROR,
> >    str1=0x2aaaaab0c678 "invalid connection pointer in %s",
> >    str2=0x2aaaaab08560 "virConnectNumOfDomains", str3=0x0, int1=0, int2=0,
> >    msg=0x2aaaaab0c678 "invalid connection pointer in %s") at 
> >virterror.c:358
> >#3  0x00002aaaaaacfa8e in virLibConnError (conn=0x33535a0,
> >    error=VIR_ERR_INVALID_CONN, info=0x2aaaaab08560 
> >"virConnectNumOfDomains")
> >    at libvirt.c:127
> >#4  0x00002aaaaaad1052 in virConnectNumOfDomains (conn=0x736e6961)
> >    at libvirt.c:758
> >#5  0x000000000043fa4e in ?? ()
> >
> >
> >A preliminary look at the code seems to indicate a fault in this logic:
> >
> >int
> >virConnectNumOfDomains(virConnectPtr conn)
> >{
> >    DEBUG("conn=%p", conn);
> >
> >    if (!VIR_IS_CONNECT(conn)) {
> >        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> >
> >The VIR_IS_CONNECT macro is defined as:
> >
> >#define VIR_CONNECT_MAGIC   0x4F23DEAD
> >#define VIR_IS_CONNECT(obj) ((obj) && (obj)->magic==VIR_CONNECT_MAGIC)
> >
> >Obviously if VIR_IS_CONNECT fails then "conn" should not be used 
> >further, so calling virLibConnError (conn, ...) is wrong.  Personally I 
> >think when we detect memory corruption in a C program we should just 
> >call abort().
> >
> >I'll see if I can come up with a patch to fix this later ... at the 
> >moment I'm more interested in why my program is passing an invalid 
> >connection pointer in the first place :-(
> 
> So a couple of things to say about this.
> 
> Firstly attached is a patch which fixes the libvirt problem, by passing 
> NULL when we find that a connection is invalid, rather than passing the 
> known invalid connection to virLibConnError.  (It also fixes the same 
> problem with virDomainPtr and virNetworkPtr objects).
> 
> Secondly I worked out why my program was passing an invalid connection 
> pointer, and it may act as a salutary lesson for anyone writing libvirt 
> bindings for a language which has real garbage collection (hello, Java).
> 
> In the normal case I wrap objects like virConnectPtr, virDomainPtr in an 
> OCaml object which has an attached finaliser.  Thus the OCaml user 
> doesn't need to worry about manually freeing these objects - the garbage 
> collector will collect them when they become unreachable and the 
> finaliser will call virConnectClose, virDomainFree, etc. as appropriate.
> 
> The problem I was hitting was when a libvirt function raised an error 
> (ie. virterror), which contains a virConnectPtr, virDomainPtr and 
> virNetworkPtr.  I was using my normal wrapping functions to attach 
> finalisers, but that was the wrong thing to do in this case because 
> virterror does not increment the reference counts of these objects. 
> When my OCaml virterror exception was garbage collected, that would 
> cause virConnectClose in this case to be called, but that closed the 
> connection that I was still using, resulting in the main program 
> continuing to use a closed virConnectPtr.
> 
> Note that the same double-free or invalid pointer use could also happen 
> with the domain pointer or the network pointer in the virterror.  It was 
> just my good luck that it happened to be the connection pointer that got 
> closed first which made the error rather more obvious and easier to find.
> 
> The partial solution on the OCaml side is to use a special wrapper 
> function for the virterror objects which doesn't attach a finaliser.
> 
> This is not a complete solution because an OCaml program might save the 
> exception and use it later, after the virConnectPtr, virDomainPtr or 
> virNetworkPtr that it contains have been freed elsewhere.  So the OCaml 
> program becomes unsafe, and we have to tell users not to save these 
> exceptions.  Unfortunately there is no good solution on the C side 
> either: if we change virterror so it increments the reference counts 
> then any C program which uses virterror will have to be changed.
> 
> (The above is fixed in ocaml-libvirt bindings >= 0.3.2.6).
> 
> Rich.
> 
> -- 
> Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
> Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
> Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
> England and Wales under Company Registration No. 03798903

> Index: src/libvirt.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/libvirt.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 libvirt.c
> --- src/libvirt.c	10 Sep 2007 09:37:10 -0000	1.98
> +++ src/libvirt.c	12 Sep 2007 11:04:19 -0000
> @@ -573,7 +573,7 @@ virConnectGetType(virConnectPtr conn)
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>  
> @@ -603,7 +603,7 @@ virConnectGetVersion(virConnectPtr conn,
>      DEBUG("conn=%p, hvVer=%p", conn, hvVer);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -637,7 +637,7 @@ virConnectGetHostname (virConnectPtr con
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return NULL;
>      }
>  
> @@ -669,7 +669,7 @@ virConnectGetURI (virConnectPtr conn)
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return NULL;
>      }
>  
> @@ -698,7 +698,7 @@ virConnectGetMaxVcpus(virConnectPtr conn
>      DEBUG("conn=%p, type=%s", conn, type);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -725,7 +725,7 @@ virConnectListDomains(virConnectPtr conn
>      DEBUG("conn=%p, ids=%p, maxids=%d", conn, ids, maxids);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -755,7 +755,7 @@ virConnectNumOfDomains(virConnectPtr con
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }

  yes those changes make sense.

> @@ -786,7 +786,7 @@ virDomainGetConnect (virDomainPtr dom)
>      DEBUG("dom=%p", dom);
>  
>      if (!VIR_IS_DOMAIN (dom)) {
> -        virLibDomainError (dom, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError (NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return NULL;
>      }
>      return dom->conn;

  unclear, if there is no way the conn can be extracted, then yes,
maybe that's true.

> @@ -811,7 +811,7 @@ virDomainCreateLinux(virConnectPtr conn,
>      DEBUG("conn=%p, xmlDesc=%s, flags=%d", conn, xmlDesc, flags);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (xmlDesc == NULL) {
> @@ -847,7 +847,7 @@ virDomainLookupByID(virConnectPtr conn, 
>      DEBUG("conn=%p, id=%d", conn, id);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (id < 0) {
> @@ -878,7 +878,7 @@ virDomainLookupByUUID(virConnectPtr conn
>      DEBUG("conn=%p, uuid=%s", conn, uuid);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (uuid == NULL) {
> @@ -913,7 +913,7 @@ virDomainLookupByUUIDString(virConnectPt
>      DEBUG("conn=%p, uuidstr=%s", conn, uuidstr);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (uuidstr == NULL) {
> @@ -961,7 +961,7 @@ virDomainLookupByName(virConnectPtr conn
>      DEBUG("conn=%p, name=%s", conn, name);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (name == NULL) {
> @@ -996,7 +996,7 @@ virDomainDestroy(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -1028,7 +1028,7 @@ virDomainFree(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (virFreeDomain(domain->conn, domain) < 0)
> @@ -1055,7 +1055,7 @@ virDomainSuspend(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1089,7 +1089,7 @@ virDomainResume(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1126,7 +1126,7 @@ virDomainSave(virDomainPtr domain, const
>      DEBUG("domain=%p, to=%s", domain, to);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1182,7 +1182,7 @@ virDomainRestore(virConnectPtr conn, con
>      DEBUG("conn=%p, from=%s", conn, from);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>      if (conn->flags & VIR_CONNECT_RO) {
> @@ -1240,7 +1240,7 @@ virDomainCoreDump(virDomainPtr domain, c
>      DEBUG("domain=%p, to=%s, flags=%d", domain, to, flags);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1300,7 +1300,7 @@ virDomainShutdown(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1335,7 +1335,7 @@ virDomainReboot(virDomainPtr domain, uns
>      DEBUG("domain=%p, flags=%u", domain, flags);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1367,7 +1367,7 @@ virDomainGetName(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (NULL);
>      }
>      return (domain->name);
> @@ -1388,7 +1388,7 @@ virDomainGetUUID(virDomainPtr domain, un
>      DEBUG("domain=%p, uuid=%p", domain, uuid);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (uuid == NULL) {
> @@ -1421,7 +1421,7 @@ virDomainGetUUIDString(virDomainPtr doma
>      DEBUG("domain=%p, buf=%p", domain, buf);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (buf == NULL) {
> @@ -1450,7 +1450,7 @@ virDomainGetID(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return ((unsigned int) -1);
>      }
>      return (domain->id);
> @@ -1472,7 +1472,7 @@ virDomainGetOSType(virDomainPtr domain)
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (NULL);
>      }
>  
> @@ -1502,7 +1502,7 @@ virDomainGetMaxMemory(virDomainPtr domai
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (0);
>      }
>  
> @@ -1538,7 +1538,7 @@ virDomainSetMaxMemory(virDomainPtr domai
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1581,7 +1581,7 @@ virDomainSetMemory(virDomainPtr domain, 
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -1620,7 +1620,7 @@ virDomainGetInfo(virDomainPtr domain, vi
>      DEBUG("domain=%p, info=%p", domain, info);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (info == NULL) {
> @@ -1657,7 +1657,7 @@ virDomainGetXMLDesc(virDomainPtr domain,
>      DEBUG("domain=%p, flags=%d", domain, flags);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (NULL);
>      }
>      if (flags != 0) {
> @@ -1739,7 +1739,7 @@ virDomainMigrate (virDomainPtr domain,
>            domain, dconn, flags, dname, uri, bandwidth);
>  
>      if (!VIR_IS_DOMAIN (domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return NULL;
>      }
>      conn = domain->conn;        /* Source connection. */
> @@ -1821,7 +1821,7 @@ __virDomainMigratePrepare (virConnectPtr
>      DEBUG("dconn=%p, cookie=%p, cookielen=%p, uri_in=%s, uri_out=%p, flags=%lu, dname=%s, bandwidth=%lu", dconn, cookie, cookielen, uri_in, uri_out, flags, dname, bandwidth);
>  
>      if (!VIR_IS_CONNECT (dconn)) {
> -        virLibConnError (dconn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError (NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return -1;
>      }
>  
> @@ -1850,7 +1850,7 @@ __virDomainMigratePerform (virDomainPtr 
>      DEBUG("domain=%p, cookie=%p, cookielen=%d, uri=%s, flags=%lu, dname=%s, bandwidth=%lu", domain, cookie, cookielen, uri, flags, dname, bandwidth);
>  
>      if (!VIR_IS_DOMAIN (domain)) {
> -        virLibDomainError (domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError (NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return -1;
>      }
>      conn = domain->conn;
> @@ -1878,7 +1878,7 @@ __virDomainMigrateFinish (virConnectPtr 
>      DEBUG("dconn=%p, dname=%s, cookie=%p, cookielen=%d, uri=%s, flags=%lu", dconn, dname, cookie, cookielen, uri, flags);
>  
>      if (!VIR_IS_CONNECT (dconn)) {
> -        virLibConnError (dconn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError (NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return NULL;
>      }
>  
> @@ -1907,7 +1907,7 @@ virNodeGetInfo(virConnectPtr conn, virNo
>      DEBUG("conn=%p, info=%p", conn, info);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>      if (info == NULL) {
> @@ -1938,7 +1938,7 @@ virConnectGetCapabilities (virConnectPtr
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT (conn)) {
> -        virLibConnError (conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError (NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return NULL;
>      }
>  
> @@ -1966,16 +1966,11 @@ virDomainGetSchedulerType(virDomainPtr d
>      DEBUG("domain=%p, nparams=%p", domain, nparams);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return NULL;
>      }
>      conn = domain->conn;
>  
> -    if (!VIR_IS_CONNECT (conn)) {
> -        virLibConnError (conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> -        return NULL;
> -    }
> -
>      if (conn->driver->domainGetSchedulerType){
>          schedtype = conn->driver->domainGetSchedulerType (domain, nparams);
>          return schedtype;
> @@ -2008,16 +2003,11 @@ virDomainGetSchedulerParameters(virDomai
>      DEBUG("domain=%p, params=%p, nparams=%p", domain, params, nparams);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return -1;
>      }
>      conn = domain->conn;
>  
> -    if (!VIR_IS_CONNECT (conn)) {
> -        virLibConnError (conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> -        return -1;
> -    }
> -
>      if (conn->driver->domainGetSchedulerParameters)
>          return conn->driver->domainGetSchedulerParameters (domain, params, nparams);
>  
> @@ -2045,16 +2035,11 @@ virDomainSetSchedulerParameters(virDomai
>      DEBUG("domain=%p, params=%p, nparams=%d", domain, params, nparams);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return -1;
>      }
>      conn = domain->conn;
>  
> -    if (!VIR_IS_CONNECT (conn)) {
> -        virLibConnError (conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> -        return -1;
> -    }
> -
>      if (conn->driver->domainSetSchedulerParameters)
>          return conn->driver->domainSetSchedulerParameters (domain, params, nparams);
>  
> @@ -2099,7 +2084,7 @@ virDomainBlockStats (virDomainPtr dom, c
>          return -1;
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN (dom)) {
> -        virLibDomainError (dom, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError (NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return -1;
>      }
>      conn = dom->conn;
> @@ -2151,7 +2136,7 @@ virDomainInterfaceStats (virDomainPtr do
>          return -1;
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN (dom)) {
> -        virLibDomainError (dom, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError (NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return -1;
>      }
>      conn = dom->conn;
> @@ -2191,7 +2176,7 @@ virDomainDefineXML(virConnectPtr conn, c
>      DEBUG("conn=%p, xml=%s", conn, xml);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (conn->flags & VIR_CONNECT_RO) {
> @@ -2224,7 +2209,7 @@ virDomainUndefine(virDomainPtr domain) {
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      conn = domain->conn;
> @@ -2254,7 +2239,7 @@ virConnectNumOfDefinedDomains(virConnect
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2281,7 +2266,7 @@ virConnectListDefinedDomains(virConnectP
>      DEBUG("conn=%p, names=%p, maxnames=%d", conn, names, maxnames);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2316,7 +2301,7 @@ virDomainCreate(virDomainPtr domain) {
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      conn = domain->conn;
> @@ -2351,7 +2336,7 @@ virDomainGetAutostart(virDomainPtr domai
>      DEBUG("domain=%p, autostart=%p", domain, autostart);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (!autostart) {
> @@ -2386,7 +2371,7 @@ virDomainSetAutostart(virDomainPtr domai
>      DEBUG("domain=%p, autostart=%d", domain, autostart);
>  
>      if (!VIR_IS_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2423,7 +2408,7 @@ virDomainSetVcpus(virDomainPtr domain, u
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -2474,7 +2459,7 @@ virDomainPinVcpu(virDomainPtr domain, un
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -2529,7 +2514,7 @@ virDomainGetVcpus(virDomainPtr domain, v
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if ((info == NULL) || (maxinfo < 1)) {
> @@ -2570,7 +2555,7 @@ virDomainGetMaxVcpus(virDomainPtr domain
>      DEBUG("domain=%p", domain);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2600,7 +2585,7 @@ virDomainAttachDevice(virDomainPtr domai
>      DEBUG("domain=%p, xml=%s", domain, xml);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -2632,7 +2617,7 @@ virDomainDetachDevice(virDomainPtr domai
>      DEBUG("domain=%p, xml=%s", domain, xml);
>  
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> -        virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          return (-1);
>      }
>      if (domain->conn->flags & VIR_CONNECT_RO) {
> @@ -2668,7 +2653,7 @@ virNetworkGetConnect (virNetworkPtr net)
>      DEBUG("net=%p", net);
>  
>      if (!VIR_IS_NETWORK (net)) {
> -        virLibNetworkError (net, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError (NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return NULL;
>      }
>      return net->conn;
> @@ -2688,7 +2673,7 @@ virConnectNumOfNetworks(virConnectPtr co
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2715,7 +2700,7 @@ virConnectListNetworks(virConnectPtr con
>      DEBUG("conn=%p, names=%p, maxnames=%d", conn, names, maxnames);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2745,7 +2730,7 @@ virConnectNumOfDefinedNetworks(virConnec
>      DEBUG("conn=%p", conn);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2773,7 +2758,7 @@ virConnectListDefinedNetworks(virConnect
>      DEBUG("conn=%p, names=%p, maxnames=%d", conn, names, maxnames);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -2806,7 +2791,7 @@ virNetworkLookupByName(virConnectPtr con
>      DEBUG("conn=%p, name=%s", conn, name);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (name == NULL) {
> @@ -2837,7 +2822,7 @@ virNetworkLookupByUUID(virConnectPtr con
>      DEBUG("conn=%p, uuid=%s", conn, uuid);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (uuid == NULL) {
> @@ -2871,7 +2856,7 @@ virNetworkLookupByUUIDString(virConnectP
>      DEBUG("conn=%p, uuidstr=%s", conn, uuidstr);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (uuidstr == NULL) {
> @@ -2919,7 +2904,7 @@ virNetworkCreateXML(virConnectPtr conn, 
>      DEBUG("conn=%p, xmlDesc=%s", conn, xmlDesc);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (xmlDesc == NULL) {
> @@ -2953,7 +2938,7 @@ virNetworkDefineXML(virConnectPtr conn, 
>      DEBUG("conn=%p, xml=%s", conn, xml);
>  
>      if (!VIR_IS_CONNECT(conn)) {
> -        virLibConnError(conn, VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>          return (NULL);
>      }
>      if (conn->flags & VIR_CONNECT_RO) {
> @@ -2986,7 +2971,7 @@ virNetworkUndefine(virNetworkPtr network
>      DEBUG("network=%p", network);
>  
>      if (!VIR_IS_CONNECTED_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>      conn = network->conn;
> @@ -3022,7 +3007,7 @@ virNetworkCreate(virNetworkPtr network)
>  	return (-1);
>      }
>      if (!VIR_IS_CONNECTED_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>      conn = network->conn;
> @@ -3057,7 +3042,7 @@ virNetworkDestroy(virNetworkPtr network)
>      DEBUG("network=%p", network);
>  
>      if (!VIR_IS_CONNECTED_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>  
> @@ -3089,7 +3074,7 @@ virNetworkFree(virNetworkPtr network)
>      DEBUG("network=%p", network);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>      if (virFreeNetwork(network->conn, network) < 0)
> @@ -3112,7 +3097,7 @@ virNetworkGetName(virNetworkPtr network)
>      DEBUG("network=%p", network);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (NULL);
>      }
>      return (network->name);
> @@ -3133,7 +3118,7 @@ virNetworkGetUUID(virNetworkPtr network,
>      DEBUG("network=%p, uuid=%p", network, uuid);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>      if (uuid == NULL) {
> @@ -3163,7 +3148,7 @@ virNetworkGetUUIDString(virNetworkPtr ne
>      DEBUG("network=%p, buf=%p", network, buf);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>      if (buf == NULL) {
> @@ -3196,7 +3181,7 @@ virNetworkGetXMLDesc(virNetworkPtr netwo
>      DEBUG("network=%p, flags=%d", network, flags);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (NULL);
>      }
>      if (flags != 0) {
> @@ -3230,7 +3215,7 @@ virNetworkGetBridgeName(virNetworkPtr ne
>      DEBUG("network=%p", network);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (NULL);
>      }
>  
> @@ -3262,7 +3247,7 @@ virNetworkGetAutostart(virNetworkPtr net
>      DEBUG("network=%p, autostart=%p", network, autostart);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>      if (!autostart) {
> @@ -3297,7 +3282,7 @@ virNetworkSetAutostart(virNetworkPtr net
>      DEBUG("network=%p, autostart=%d", network, autostart);
>  
>      if (!VIR_IS_NETWORK(network)) {
> -        virLibNetworkError(network, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
> +        virLibNetworkError(NULL, VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          return (-1);
>      }
>  


   In general +1, yes, just need to make sure that if the domain checking
macro fails then we really can't get the connection, I guess it's true.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard redhat com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/


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