[libvirt] [PATCH 4/4] Add support for addressing backing stores by index

[Eric Blake]

On 04/22/2014 06:49 AM, Jiri Denemark wrote:
> Each backing store of a given disk is associated with a unique index
> (which is also formated in domain XML) for easier addressing of any

s/formated/formatted/

> particular backing store. With this patch, any backing store can be
> addressed by its disk target and the index. For example, "vdc[4]"
> addresses the backing store with index equal to 4 of the disk identified
> by "vdc" target. Such shorthand can be used in any API in place for a
> backing file path:
> 
>     virsh blockcommit domain vda --base vda[3] --top vda[2]

You have to quote these names in the shell (if I have a file named
'vda3' in the current directory, and fail to quote the --base argument,
then the shell will expand the glob and pass "vda3" instead of "vda[3]"
to virsh).  Maybe we should consider an alternate syntax 'vda.3', on the
grounds that it has no shell metacharacters.

But, if we are okay with the [2] notation instead of trying to come up
with a shell-friendly notation, then this patch makes sense.

> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/libvirt.c             | 13 ++++++--
>  src/libvirt_private.syms  |  1 +
>  src/qemu/qemu_driver.c    | 29 ++++++++++++-----
>  src/util/virstoragefile.c | 83 ++++++++++++++++++++++++++++++++++++++++-------
>  src/util/virstoragefile.h |  7 ++++
>  tests/virstoragetest.c    | 51 ++++++++++++++++++++++++-----
>  6 files changed, 152 insertions(+), 32 deletions(-)


> +++ b/src/util/virstoragefile.c
> @@ -1501,33 +1501,87 @@ int virStorageFileGetSCSIKey(const char *path,
>  }
>  #endif
>  
> -/* Given a CHAIN, look for the backing file NAME within the chain and
> - * return its canonical name.  Pass NULL for NAME to find the base of
> - * the chain.  If META is not NULL, set *META to the point in the
> - * chain that describes NAME (or to NULL if the backing element is not
> - * a file).  If PARENT is not NULL, set *PARENT to the preferred name
> - * of the parent (or to NULL if NAME matches the start of the chain).
> - * Since the results point within CHAIN, they must not be
> - * independently freed.  Reports an error and returns NULL if NAME is
> - * not found.  */

Ah, you are completely rewriting the comment that I pointed out as being
wrong in 3/4.  I guess in the interest of minimizing churn you could
skip changing patch 3, just to change it again here.

> +
> +/* Given a CHAIN, look for the backing file NAME within the chain starting
> + * from @startFrom or @chain if @startFrom is NULL and return its canonical

Inconsistent mix of 'CHAIN' vs. '@chain' when referring to a parameter
name (similar for NAME/@name, INDEX/@index, ...).  I don't care which of
the two styles you choose, but it would be nice to use the same style
throughout the doc-comment.

> + * NULL, set *PARENT to the preferred name of the parent (or to NULL if NAME
> + * matches the start of the chain).  Since the results point within CHAIN,
> + * independently freed.

Corrupted comment text in this sentence.

> +++ b/src/util/virstoragefile.h
> @@ -281,8 +281,15 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const char *path,
>  int virStorageFileChainGetBroken(virStorageSourcePtr chain,
>                                   char **broken_file);
>  
> +int virStorageFileParseChainIndex(const char *diskTarget,
> +                                  const char *name,
> +                                  unsigned int *index)
> +    ATTRIBUTE_NONNULL(3);

Aren't argument 1 and 2 also nonnull?

> @@ -969,6 +987,21 @@ mymain(void)
>      TEST_LOOKUP(25, NULL, chain->backingStore->backingStore->path,
>                  chain->backingStore->backingStore, chain->backingStore->path);
>  
> +    TEST_LOOKUP_TARGET(26, "vda", "bogus[1]", 0, NULL, NULL, NULL);
> +    TEST_LOOKUP_TARGET(27, "vda", "vda[-1]", 0, NULL, NULL, NULL);
> +    TEST_LOOKUP_TARGET(28, "vda", "vda[1][1]", 0, NULL, NULL, NULL);
> +    TEST_LOOKUP_TARGET(29, "vda", "wrap", 0, chain->path, chain, NULL);
> +    TEST_LOOKUP_TARGET(30, "vda", "vda[0]", 0, NULL, NULL, NULL);
> +    TEST_LOOKUP_TARGET(31, "vda", "vda[1]", 1,
> +                       chain->backingStore->path,
> +                       chain->backingStore,
> +                       chain->path);
> +    TEST_LOOKUP_TARGET(32, "vda", "vda[2]", 2,
> +                       chain->backingStore->backingStore->path,
> +                       chain->backingStore->backingStore,
> +                       chain->backingStore->path);
> +    TEST_LOOKUP_TARGET(33, "vda", "vda[3]", 3, NULL, NULL, NULL);

Nice evidence of it working as designed.  Unless anyone else has
opinions on being shell-friendly, I can live with:

ACK with comments addressed.

-- 
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/20140424/0e1fedf6/attachment-0001.sig>