[libvirt] [PATCHv2 09/33] storage: backend: Add unique id retrieval API

[Eric Blake]

On 05/22/2014 07:47 AM, Peter Krempa wrote:
> Different protocols have different means to uniquely identify a storage
> file. This patch implements a storage driver API to retrieve a unique
> string describing a volume. The current implementation works for local
> storage only and returns the canonical path of the volume.
> 
> To add caching support the local filesystem driver now has a private
> structure holding the cached string, which is created only when it's
> initially accessed.
> 
> This patch provides the implementation for local files only for start.
> ---
>  src/storage/storage_backend.h    |  3 +++
>  src/storage/storage_backend_fs.c | 49 ++++++++++++++++++++++++++++++++++++++++
>  src/storage/storage_driver.c     | 30 ++++++++++++++++++++++++
>  src/storage/storage_driver.h     |  1 +
>  4 files changed, 83 insertions(+)
> 

> 
> +typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv;
> +typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr;
> +
> +struct _virStorageFileBackendFsPriv {
> +    char *uid; /* unique file identifier (canonical path) */

The name 'uid' is misleading (too commonly used to mean user-id, which
it is not).  Can you change it to something like 'fid' (short for
file-id) or 'canon'?

> +};
> +
> +
>  static void
>  virStorageFileBackendFileDeinit(virStorageSourcePtr src)
>  {
> @@ -1346,16 +1354,27 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src)
>                virStorageTypeToString(virStorageSourceGetActualType(src)),
>                src->path);
> 
> +    virStorageFileBackendFsPrivPtr priv = src->drv->priv;
> +
> +    VIR_FREE(priv->uid);

As evidence, if I see this line in isolation, I think "why are you
freeing an integer" - I had to look at the header to learn "oh, it's not
a usual uid, but a string".


> +/*
> + * virStorageFileGetUniqueIdentifier: Get a unique string describing the volume
> + *
> + * @src: file structure pointing to the file
> + *
> + * Returns a string uniquely describing a single volume (canonical path).
> + * The string shall not be freed. Returns NULL on error and sets a libvirt
> + * error code */

Is it also worth documenting that the string is invalidated when the
input parameter 'src' is freed?

The idea makes sense, but I'd like to see a v3.

-- 
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/20140522/f122acc9/attachment-0001.sig>