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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list