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 -=|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list