[XrdCl] Make sure recursive cp / ls does not segv if server does not …

…support bulk stat.
xrootd · Dec 6, 2017 · bf93dc2 · bf93dc2
1 parent a8d526b
commit bf93dc2
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 52 deletions.
diff --git a/src/XrdCl/XrdClCopy.cc b/src/XrdCl/XrdClCopy.cc
@@ -444,35 +444,30 @@ XrdCpFile *IndexRemote( XrdCl::FileSystem *fs,
 {
   using namespace XrdCl;
 
-  XrdCpFile   start;
-  XrdCpFile  *end   = &start;
-  XrdCpFile  *current;
-  URL         source( basePath );
-  int         badUrl;
-
-  std::vector<std::string> *files       = new std::vector<std::string>();
-  std::vector<std::string> *directories = new std::vector<std::string>();
-
   Log *log = DefaultEnv::GetLog();
   log->Debug( AppMsg, "Indexing %s", basePath.c_str() );
 
-  XRootDStatus status = GetDirList( fs, source, files, directories );
-  if( !status.IsOK() )
+  DirectoryList *dirList = 0;
+  XRootDStatus st = fs->DirList( basePath, DirListFlags::Recursive, dirList );
+  if( !st.IsOK() )
   {
     log->Info( AppMsg, "Failed to get directory listing for %s: %s",
-                       source.GetURL().c_str(),
-                       status.GetErrorMessage().c_str() );
+                       basePath.c_str(),
+                       st.GetErrorMessage().c_str() );
+    return 0;
   }
 
-  std::vector<std::string>::iterator it;
-  for( it = files->begin(); it != files->end(); ++it )
+  XrdCpFile start, *current = 0;
+  XrdCpFile *end   = &start;
+  int       badUrl = 0;
+  for( auto itr = dirList->Begin(); itr != dirList->End(); ++itr )
   {
-    std::string file = basePath + "/" + (*it);
-    log->Dump( AppMsg, "Found file %s", file.c_str() );
-
-    current = new XrdCpFile( file.c_str(), badUrl );
+    DirectoryList::ListEntry *e = *itr;
+    std::string path = basePath + '/' + e->GetName();
+    current = new XrdCpFile( path.c_str(), badUrl );
     if( badUrl )
     {
+      // TODO release the memory !!!
       log->Error( AppMsg, "Bad URL: %s", current->Path );
       return 0;
     }
@@ -482,17 +477,6 @@ XrdCpFile *IndexRemote( XrdCl::FileSystem *fs,
     end           = current;
   }
 
-  for( it = directories->begin(); it != directories->end(); ++it )
-  {
-    std::string directory = basePath + "/" + (*it);
-    log->Dump( AppMsg, "Found directory %s", directory.c_str() );
-
-    end->Next = IndexRemote( fs, directory, dirOffset );
-    while( end->Next ) end = end->Next;
-  }
-
-  delete files;
-  delete directories;
   return start.Next;
 }
 

diff --git a/src/XrdCl/XrdClFileSystem.cc b/src/XrdCl/XrdClFileSystem.cc
@@ -407,9 +407,23 @@ namespace
           {
             DirectoryList::ListEntry *entry = *itr;
             StatInfo *info = entry->GetStatInfo();
+            if( !info )
+              throw RecDirLsErr( XRootDStatus( stError, errNotSupported ) );
             std::string path = dirList->GetParentName() + entry->GetName();
 
-            if( info->TestFlags( StatInfo::IsDir ) ) // it is a directory
+            // check the prefix
+            if( path.find( parent ) != 0 )
+              throw RecDirLsErr( XRootDStatus( stError, errInternal ) );
+
+            // add new entry to the result
+            path = path.substr( parent.size() );
+            entry->SetStatInfo( 0 ); // StatInfo is no longer owned by dirList
+            DirectoryList::ListEntry *e =
+                new DirectoryList::ListEntry( entry->GetHostAddress(), path, info );
+            pCtx->dirList->Add( e );
+
+            // if it's a directory do a recursive call
+            if( info->TestFlags( StatInfo::IsDir ) )
             {
               // bump the pending counter
               ++pCtx->pending;
@@ -429,22 +443,10 @@ namespace
                   throw RecDirLsErr( XRootDStatus( stError, errOperationExpired ) );
               }
               // send the request
-              XRootDStatus st = pCtx->fs->DirList( path, flags, handler, timeout );
+              XRootDStatus st = pCtx->fs->DirList( parent + path, flags, handler, timeout );
               if( !st.IsOK() )
                 throw RecDirLsErr( st );
             }
-            else // it's a file
-            {
-              // check the prefix
-              if( path.find( parent ) != 0 )
-                throw RecDirLsErr( XRootDStatus( stError, errDataError ) );
-
-              path = path.substr( parent.size() );
-              entry->SetStatInfo( 0 ); // StatInfo is no longer owned by dirList
-              DirectoryList::ListEntry *e =
-                  new DirectoryList::ListEntry( entry->GetHostAddress(), path, info );
-              pCtx->dirList->Add( e );
-            }
           }
 
           if( pCtx->pending == 0 )
@@ -464,17 +466,14 @@ namespace
           }
         }
 
+        // clean up the context if necessary
+        bool delctx = ( pCtx->pending == 0 );
+        scoped.UnLock();
+        if( delctx )
+          delete pCtx;
         // clean up the arguments
         delete status;
         delete response;
-        // clean up the context if necessary
-        if( pCtx->pending == 0 )
-        {
-          scoped.UnLock();
-          delete pCtx;
-        }
-        else
-          scoped.UnLock();
         // and finally commit suicide
         delete this;
       }