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

Re: [libvirt] [PATCHv4 3/7] storage: Implement file storage APIs in the default storage driver



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


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