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