Re: [libvirt] [PATCHv3 07/36] storage: Switch metadata crawler to use storage driver to get unique path

Peter Krempa wrote: > On 06/07/14 20:35, Roman Bogorodskiy wrote: >> Peter Krempa wrote: >> >>> Use the virStorageFileGetUniqueIdentifier() function to get a unique >>> identifier regardless of the target storage type instead of relying on >>> canonicalize_path(). >>> >>> A new function that checks whether we support a given image is >>> introduced to avoid errors for unimplemented backends. >>> --- >>> >>> Notes: >>> Version 3: >>> - fixed typo >>> - ACKed by Eric >>> >>> src/storage/storage_driver.c | 77 +++++++++++++++++++++++++++++++------------- >>> 1 file changed, 54 insertions(+), 23 deletions(-) >>> >>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c >>> index f92a553..5c4188f 100644 >>> --- a/src/storage/storage_driver.c >>> +++ b/src/storage/storage_driver.c > >>> @@ -3112,49 +3135,63 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, >>> int fd; >>> int ret = -1; >>> struct stat st; >>> + const char *uniqueName; >>> virStorageSourcePtr backingStore = NULL; >>> int backingFormat; >>> >>> - VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", >>> - src->path, canonPath, NULLSTR(src->relDir), src->format, >>> + VIR_DEBUG("path=%s dir=%s format=%d uid=%d gid=%d probe=%d", >>> + src->path, NULLSTR(src->relDir), src->format, >>> (int)uid, (int)gid, allow_probe); >>> >>> - if (virHashLookup(cycle, canonPath)) { >>> - virReportError(VIR_ERR_INTERNAL_ERROR, >>> - _("backing store for %s is self-referential"), >>> - src->path); >>> + /* exit if we can't load information about the current image */ >>> + if (!virStorageFileSupportsBackingChainTraversal(src)) >>> + return 0; >> >> After this change I get virstrogetest failing on FreeBSD, which doesn't >> support any of the file storage backends currently: >> >> >> Program received signal SIGSEGV, Segmentation fault. >> [Switching to Thread 806c06400 (LWP 100061/virstoragetest)] >> 0x0000000000410088 in mymain () at virstoragetest.c:937 >> 937 TEST_LOOKUP(3, "qcow2", chain->backingStore->path, >> chain->backingStore, >> Current language: auto; currently minimal >> (gdb) p chain >> $1 = 0x806c7b100 >> (gdb) p chain->backingStore >> $2 = 0x0 >> (gdb) >> >> > > Hmm, so FreeBSD doesn't currently compile the storage driver? We should > probably look into enabling it. All the stuff that was done by the code > was compiling just fine on FreeBSD previously so enabling just the local > filesystem storage backend should work well. I'll have a look what that > would include. virstorage.c test calls testStorageFileGetMetadata() which calls virStorageFileGetMetadata(). Inside of that, the call sequence is: virStorageFileGetMetadata -> virStorageFileGetMetadataRecurse virStorageFileGetMetadataRecurse then checks !virStorageFileSupportsBackingChainTraversal() and returns 0. virStorageFileSupportsBackingChainTraversal tries to initialise backend using virStorageFileBackendForTypeInternal(). virStorageFileBackendForTypeInternal loops through fileBackends which looks this way: static virStorageFileBackendPtr fileBackends[] = { #if WITH_STORAGE_FS &virStorageFileBackendFile, &virStorageFileBackendBlock, #endif #if WITH_STORAGE_GLUSTER &virStorageFileBackendGluster, #endif NULL }; FreeBSD doesn't support neither WITH_STORAGE_GLUSTER nor WITH_STORAGE_FS. So we end up having chain->backingStore == NULL. PS I may be wrong here as it's my first experience with the storage code and I haven't yet spend much time on it.

Roman Bogorodskiy