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 ?
[...]
> +/**
> + * virStoragePoolCreate:
> + * @pool: pointer to storage pool
> + *
> + * Starts an inactive storage pool
> + *
> + * Returns 0 on success, or -1 if it could not be started
> + */
> +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).
[...]
> +/**
> + * virStoragePoolDelete:
> + * @pool: pointer to storage pool
> + * @flags: flags for obliteration process
> + *
> + * Delete the underlying pool resources. This is
> + * a non-recoverable operation.
Same as before more details are needed I guess.
[...]
> diff -r 42e6f49e4e69 src/virterror.c
> --- a/src/virterror.c Thu Feb 07 12:33:13 2008 -0500
> +++ b/src/virterror.c Thu Feb 07 16:52:34 2008 -0500
> @@ -258,6 +258,9 @@ virDefaultErrorFunc(virErrorPtr err)
> break;
> case VIR_FROM_STATS_LINUX:
> dom = "Linux Stats ";
> + break;
> + case VIR_FROM_STORAGE:
> + dom = "Storage ";
> break;
>
> }
> @@ -665,6 +668,24 @@ __virErrorMsg(virErrorNumber error, cons
> else
> errmsg = _("authentication failed: %s");
> break;
> + case VIR_ERR_INVALID_STORAGE_POOL:
> + if (info == NULL)
> + errmsg = _("invalid storage pool pointer in");
> + else
> + errmsg = _("invalid storage pool pointer in %s");
> + break;
> + case VIR_ERR_INVALID_STORAGE_VOL:
> + if (info == NULL)
> + errmsg = _("invalid storage volume pointer in");
> + else
> + errmsg = _("invalid storage volume pointer in %s");
> + break;
> + case VIR_WAR_NO_STORAGE:
> + if (info == NULL)
> + errmsg = _("Failed to find a storage driver");
> + else
> + errmsg = _("Failed to find a storage driver: %s");
> + break;
> }
> return (errmsg);
> }
Okay, looks fine to me, +1,
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list