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

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



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  -=| 


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