On Thu, Feb 14, 2008 at 06:24:09AM -0500, Daniel Veillard wrote:
On Tue, Feb 12, 2008 at 04:30:45AM +0000, Daniel P. Berrange wrote:
This defines the internal driver API for the storage APIs. The pattern follows that used for the existing APIs. NB, both the storage pool and storage volume objects are now top level objects. Previous iterations of this code have the volume as a child of the pool. This unneccessarily complicated the reference counting and forced you to always have a pool available first. [...] + unsigned int flags); +typedef int + (*virDrvStoragePoolCreate) (virStoragePoolPtr pool);
as mentioned previously, a flags here is a safety IMHO
+typedef int + (*virDrvStoragePoolDestroy) (virStoragePoolPtr pool); [...] +/** +* _virNetwork: +* +* Internal structure associated to a storage volume +*/ +struct _virStorageVol { + unsigned int magic; /* specific value to check */ + int refs; /* reference count */ + virConnectPtr conn; /* pointer back to the connection */ + char *pool; /* Pool name of owner */ + char *name; /* the storage vol external name */ + /* XXX currently abusing path for this. Ought not to be so evil */ + char key[PATH_MAX]; /* unique key for storage vol */ };
I'm just a bit surprized by the static allocation of the key. Even if we are passing _virStorageVol data around, I guess the string is zero terminated and we can probably avoid the static size. Or is that too much of a burden ?
Yes, I'll switch this to be allocated on demand - I just happened to have it pre-allocated because I have simply renamed the 'uuid' field which was also pre-allocated.
+int +virStoragePoolCreate(virStoragePoolPtr pool) +{ + virConnectPtr conn; + DEBUG("pool=%p", pool); + + if (pool == NULL) { + TODO; + return (-1); + } + if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) { + virLibStoragePoolError(NULL, VIR_ERR_INVALID_STORAGE_POOL, __FUNCTION__); + return (-1); + } + conn = pool->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibStoragePoolError(pool, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + return (-1); + } + + if (conn->storageDriver && conn->storageDriver->poolCreate) + return conn->storageDriver->poolCreate (pool); + + virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; + +}
Would just like to see a flags parameter here too.
Yes, all Create methods need flags.
+/** + * virStoragePoolDestroy: + * @pool: pointer to storage pool + * + * Destroy an active storage pool. The virStoragePoolPtr + * object should not be used after this method returns + * successfully as it has been free'd
maybe indicating the difference between Free/Destroy and Delete would be good here. people might think it's used to free the on-disk resources (I believe it's not the case, but in the context of storage maybe a bit more details are needed it's not like domains for which the runtime state can be recreated from the defined state, for storage it's a bit different I think).
This comment is actually bogus - the Destroy method does not actually free the virStoragePoolPtr object itself - you still need to call the explicit virStoragePoolFree method. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|