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

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/