Re: [Libvir] PATCH: 2/16: Internal driver API

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 ?

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

> +/** > + * 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).