[libvirt] [PATCHv5 2/7] storage: Implement file storage APIs in the default storage driver

[Eric Blake]

On 02/11/2014 09:26 AM, Peter Krempa wrote:
> Implement the APIs added by the previous patch in the default storage
> driver used by qemu.
> ---
> 
> Notes:
>     Version 5:
>     - adapt to error reporting change
> 
>  src/check-aclrules.pl         |  1 +
>  src/storage/storage_backend.c | 37 +++++++++++++++++
>  src/storage/storage_backend.h | 43 ++++++++++++++++++++
>  src/storage/storage_driver.c  | 95 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 176 insertions(+)

I tend to agree with Dan's comment on patch 1 (that is, exposing these
directly in storage_driver.c instead of making it go through the
driver.h interface), which may have some fallout on this patch.  That
said, I'll review this one.

> +++ b/src/storage/storage_backend.c
> @@ -121,6 +121,12 @@ static virStorageBackendPtr backends[] = {
>      NULL
>  };
> 
> +
> +static virStorageFileBackendPtr fileBackends[] = {
> +    NULL
> +};

At first I wondered why an empty array, then I realized - this is the
framework, then later patches add in new backends.  Okay.


> +
> +struct _virStorageFileBackend {
> +    int type;
> +    int protocol;
> +
> +    virStorageFileBackendInit backendInit;
> +    virStorageFileBackendDeinit backendDeinit;
> +
> +    virStorageFileBackendCreate storageFileCreate;
> +    virStorageFileBackendUnlink storageFileUnlink;
> +    virStorageFileBackendStat   storageFileStat;

No need to copy existing bad examples; please document which of these
callbacks (if any) must be non-NULL, or explicitly state that all
callbacks are optional.  (When I first added the gluster backend, I had
a hard time figuring out which callbacks storage_driver expected to be
able to use).  It would also be nice to document the contract placed on
implementations of each callback (such as when a callback should return
-1 vs -2, or whether errno must be set on error), at the point where you
declare typedefs for each callback signature.

This patch looks okay in isolation for driving the new callbacks, once
later patches implement them.  ACK with comments added.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140212/0c517eae/attachment-0001.sig>