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

[Jiri Denemark]

On Thu, Apr 24, 2014 at 13:43:48 -0600, Eric Blake wrote:
> 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.

I think we are OK with this notation :-)

...
> > @@ -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?

No, NULL is explicitly allowed for both diskTarget and name to make the
function simpler to call. And the test suite already checks that passing
NULL for either of the two parameters works and returns the expected
result.

> > @@ -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.

I fixed the thing you pointed out and also got rid of "index" to avoid
breaking the build again :-) and pushed the series. Thanks.

Jirka