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

[Roman Bogorodskiy]

  Peter Krempa wrote:

> On 06/09/14 16:07, Roman Bogorodskiy wrote:
> >   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.
> 
> Yes, the problem is that VIR_STORAGE_FS isn't enabled on freebsd.
> 
> I don't know whether it's worth analyzing the code to find out if it can
> be enabled on freebsd or not. (the needed portions certainly can be
> enabled, but some of the other portions might be problematic.

Providing a fully functional virStorageBackendFileSystem backend will
certainly require some effort, at least getmntent_r() part and mouting
part needs to be re-implemnented for BSD.

I'll probably take a look later.

> 
> At any rate, I'm working on a refactor of the test that will allow to
> skip the test (and remove a few ugly pieces of code)

Thanks!

Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140611/714c5647/attachment-0001.sig>