On 02/03/2014 09:54 AM, Peter Krempa wrote:
> Implement the APIs added by the previous patch in the default storage
> driver used by qemu.
> ---
> src/check-aclrules.pl | 1 +
> src/storage/storage_backend.c | 37 ++++++++++++++++++
> src/storage/storage_backend.h | 43 +++++++++++++++++++++
> src/storage/storage_driver.c | 89 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 170 insertions(+)
>
> +static int
> +storageFileInit(virStorageFilePtr file)
> +{
> + virStorageFileBackendPrivPtr priv = NULL;
> +
> + if (VIR_ALLOC(priv) < 0)
> + return -1;
> +
> + file->priv = priv;
> +
> + if (!(priv->backend = virStorageFileBackendForType(file->type,
> + file->protocol)))
> + goto error;
> +
> + if (priv->backend->backendInit) {
> + if (priv->backend->backendInit(file) < 0)
> + goto error;
> + }
> +
> + return 0;
So if there's no backendInit callback function, we just succeed?
Shouldn't that error out? Or is backendInit just optional, and the
important thing is wheterh priv->backend was successfully looked up.
> +static int
> +storageFileUnlink(virStorageFilePtr file)
> +{
> + virStorageFileBackendPrivPtr priv = file->priv;
> +
> + if (!priv->backend->storageFileUnlink)
> + return ENOSYS;
Returning a positive errno value? Don't you want a negative here? Any
documentation on the fact that these functions are returning errno
values instead of -1, and with errno itself left indeterminate? Or
maybe make this 'errno = ENOSYS; return -1;'?
> +
> + return priv->backend->storageFileUnlink(file);
> +}
> +
> +
> +static int
> +storageFileStat(virStorageFilePtr file,
> + struct stat *st)
> +{
> + virStorageFileBackendPrivPtr priv = file->priv;
> +
> + if (!(priv->backend->storageFileStat))
> + return ENOSYS;
Same issue about failure returns.
> +
> + return priv->backend->storageFileStat(file, st);
> +}
> +
> +
> static virStorageDriver storageDriver = {
> .name = "storage",
> .storageOpen = storageOpen, /* 0.4.0 */
> @@ -2631,6 +2711,15 @@ static virStorageDriver storageDriver = {
>
> .storagePoolIsActive = storagePoolIsActive, /* 0.7.3 */
> .storagePoolIsPersistent = storagePoolIsPersistent, /* 0.7.3 */
> +
> + /* ----- internal APIs ----- */
> + .storageFileInit = storageFileInit,
> + .storageFileDeinit = storageFileDeinit,
> +
> + .storageFileUnlink = storageFileUnlink,
> + .storageFileStat = storageFileStat,
> + .storageFileCreate = storageFileCreate,
> +
> };
>
>
I can see where you're going with this, but am not quite sure it is
ready to merge without addressing my comments above.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list