Fixing Recursive Copy

Discussions related to using the OSE version of VirtualBox.
Post Reply
jchatham
Posts: 19
Joined: 15. Jul 2013, 20:57

Fixing Recursive Copy

Post by jchatham »

There are a couple of bugs with this, and I'm planning on patching the lot of them, but I'll start with this quick fix that makes recursive copy from guest to host no longer cause the guest to crash.

Code: Select all

Index: src/VBox/Main/src-client/GuestDirectoryImpl.cpp
===================================================================
--- src/VBox/Main/src-client/GuestDirectoryImpl.cpp	(original code)
+++ src/VBox/Main/src-client/GuestDirectoryImpl.cpp	(working copy)
@@ -89,7 +89,7 @@
     {
         /* Start the directory process on the guest. */
         GuestProcessStartupInfo procInfo;
-        procInfo.mName      = Utf8StrFmt(tr("Reading directory \"%s\"", openInfo.mPath.c_str()));
+        procInfo.mName      = Utf8StrFmt(tr("Reading directory \"%s\""), openInfo.mPath.c_str());
         procInfo.mCommand   = Utf8Str(VBOXSERVICE_TOOL_LS);
         procInfo.mTimeoutMS = 5 * 60 * 1000; /* 5 minutes timeout. */
         procInfo.mFlags     = ProcessCreateFlag_WaitForStdOut;
Perryg
Site Moderator
Posts: 34369
Joined: 6. Sep 2008, 22:55
Primary OS: Linux other
VBox Version: OSE self-compiled
Guest OSses: *NIX

Re: Fixing Recursive Copy

Post by Perryg »

You need to post this kind of thing to the DEVs mail list.
frank
Oracle Corporation
Posts: 3362
Joined: 7. Jun 2007, 09:11
Primary OS: Debian Sid
VBox Version: PUEL
Guest OSses: Linux, Windows
Location: Dresden, Germany
Contact:

Re: Fixing Recursive Copy

Post by frank »

Thank you for this fix! It was applied to trunk and will be also part of the next 4.3.x maintenance release.
jchatham
Posts: 19
Joined: 15. Jul 2013, 20:57

Re: Fixing Recursive Copy

Post by jchatham »

Frank Mehnert wrote:Thank you for this fix! It was applied to trunk and will be also part of the next 4.3.x maintenance release.
Thanks! Much appreciated.
Perryg wrote:You need to post this kind of thing to the DEVs mail list.
I've now joined the dev mailing list and so can post things there... but given the fast response time here, I'm inclined to not worry too much about the difference. Or is there some other reason you suggested this?

Regardless, here's fix number two; this patch gets recursive copy guest-to-host working fully (at least on the platforms and cases I've tested - ubuntu host & both ubuntu and windows guests). It doesn't fix the --follow flag, though; that appears to be both non-functional and non-trivial to fix.

Edit: removed patch; this fix is included in the larger patch a few posts down.
Last edited by jchatham on 27. Oct 2014, 20:58, edited 1 time in total.
Perryg
Site Moderator
Posts: 34369
Joined: 6. Sep 2008, 22:55
Primary OS: Linux other
VBox Version: OSE self-compiled
Guest OSses: *NIX

Re: Fixing Recursive Copy

Post by Perryg »

Actually the DEVs do come by here on occasion, but it is much faster usually posting to the DEV mail list.
jchatham
Posts: 19
Joined: 15. Jul 2013, 20:57

Re: Fixing Recursive Copy

Post by jchatham »

Alright; I'll drop a comment on the dev mailing list as well, once I'm done here. Which is, oh hey, right now; here's a patch that includes fixes for recursive copy host to guest (as well as the previously posted patch for guest to host).

Code: Select all

--- src/VBox/Frontends/VBoxManage/VBoxManageGuestCtrl.cpp	(original code)
+++ src/VBox/Frontends/VBoxManage/VBoxManageGuestCtrl.cpp	(working copy)
@@ -1776,11 +1776,13 @@
     if (fOnGuest)
     {
         BOOL fDirExists = FALSE;
+        /* This function is bugged - it always returns an error if the directory does not exist. */
         HRESULT rc = pContext->pCmdCtx->pGuestSession->DirectoryExists(Bstr(pszDir).raw(), &fDirExists);
-        if (FAILED(rc))
+        *fExists = fDirExists ? true : false;
+        /* If it set fDirExists and returned an error, then we want to error.  Otherwise, we
+         * need to return success with fExists set to false. */
+        if(fDirExists && FAILED(rc))
             vrc = ctrlPrintError(pContext->pCmdCtx->pGuestSession, COM_IIDOF(IGuestSession));
-        else
-            *fExists = fDirExists ? true : false;
     }
     else
         *fExists = RTDirExists(pszDir);
@@ -1983,9 +1985,15 @@
     if (pContext->pCmdCtx->fVerbose)
         RTPrintf("Processing host directory: %s\n", szCurDir);
 
-    /* Flag indicating whether the current directory was created on the
-     * target or not. */
-    bool fDirCreated = false;
+    /* Always create directory, otherwise we'll fail to copy empty directories. */
+    char *pszDestDir;
+    vrc = ctrlCopyTranslatePath(pszSource, szCurDir,
+                               pszDest, &pszDestDir);
+    if (RT_SUCCESS(vrc))
+    {
+        vrc = ctrlCopyDirCreate(pContext, pszDestDir);
+        RTStrFree(pszDestDir);
+    }
 
     /*
      * Open directory without a filter - RTDirOpenFiltered unfortunately
@@ -2072,36 +2080,19 @@
                     if (pContext->pCmdCtx->fVerbose)
                         RTPrintf("File: %s\n", DirEntry.szName);
 
-                    if (!fDirCreated)
+                    char *pszFileSource = RTPathJoinA(szCurDir, DirEntry.szName);
+                    if (pszFileSource)
                     {
-                        char *pszDestDir;
-                        vrc = ctrlCopyTranslatePath(pszSource, szCurDir,
-                                                    pszDest, &pszDestDir);
+                        char *pszFileDest;
+                        vrc = ctrlCopyTranslatePath(pszSource, pszFileSource,
+                                                   pszDest, &pszFileDest);
                         if (RT_SUCCESS(vrc))
                         {
-                            vrc = ctrlCopyDirCreate(pContext, pszDestDir);
-                            RTStrFree(pszDestDir);
-
-                            fDirCreated = true;
-                        }
-                    }
-
-                    if (RT_SUCCESS(vrc))
-                    {
-                        char *pszFileSource = RTPathJoinA(szCurDir, DirEntry.szName);
-                        if (pszFileSource)
-                        {
-                            char *pszFileDest;
-                            vrc = ctrlCopyTranslatePath(pszSource, pszFileSource,
-                                                       pszDest, &pszFileDest);
-                            if (RT_SUCCESS(vrc))
-                            {
-                                vrc = ctrlCopyFileToDest(pContext, pszFileSource,
-                                                        pszFileDest, 0 /* Flags */);
-                                RTStrFree(pszFileDest);
-                            }
-                            RTStrFree(pszFileSource);
+                            vrc = ctrlCopyFileToDest(pContext, pszFileSource,
+                                                    pszFileDest, 0 /* Flags */);
+                            RTStrFree(pszFileDest);
                         }
+                        RTStrFree(pszFileSource);
                     }
                     break;
                 }
@@ -2155,9 +2146,18 @@
     if (pContext->pCmdCtx->fVerbose)
         RTPrintf("Processing guest directory: %s\n", szCurDir);
 
-    /* Flag indicating whether the current directory was created on the
-     * target or not. */
-    bool fDirCreated = false;
+    /* Create directory here, otherwise we will fail to copy empty directories. */
+    char *pszDestDir;
+    vrc = ctrlCopyTranslatePath(pszSource, szCurDir,
+                                pszDest, &pszDestDir);
+    if (RT_SUCCESS(vrc))
+    {
+        vrc = ctrlCopyDirCreate(pContext, pszDestDir);
+        RTStrFree(pszDestDir);
+    }
+    if (RT_FAILURE(vrc))
+        return vrc;
+
     SafeArray<DirectoryOpenFlag_T> dirOpenFlags; /* No flags supported yet. */
     ComPtr<IGuestDirectory> pDirectory;
     HRESULT rc = pContext->pCmdCtx->pGuestSession->DirectoryOpen(Bstr(szCurDir).raw(), Bstr(pszFilter).raw(),
@@ -2242,39 +2242,22 @@
                 if (pContext->pCmdCtx->fVerbose)
                     RTPrintf("File: %s\n", strFile.c_str());
 
-                if (!fDirCreated)
+                char *pszFileSource = RTPathJoinA(szCurDir, strFile.c_str());
+                if (pszFileSource)
                 {
-                    char *pszDestDir;
-                    vrc = ctrlCopyTranslatePath(pszSource, szCurDir,
-                                                pszDest, &pszDestDir);
+                    char *pszFileDest;
+                    vrc = ctrlCopyTranslatePath(pszSource, pszFileSource,
+                                               pszDest, &pszFileDest);
                     if (RT_SUCCESS(vrc))
                     {
-                        vrc = ctrlCopyDirCreate(pContext, pszDestDir);
-                        RTStrFree(pszDestDir);
-
-                        fDirCreated = true;
+                        vrc = ctrlCopyFileToDest(pContext, pszFileSource,
+                                                pszFileDest, 0 /* Flags */);
+                        RTStrFree(pszFileDest);
                     }
+                    RTStrFree(pszFileSource);
                 }
-
-                if (RT_SUCCESS(vrc))
-                {
-                    char *pszFileSource = RTPathJoinA(szCurDir, strFile.c_str());
-                    if (pszFileSource)
-                    {
-                        char *pszFileDest;
-                        vrc = ctrlCopyTranslatePath(pszSource, pszFileSource,
-                                                   pszDest, &pszFileDest);
-                        if (RT_SUCCESS(vrc))
-                        {
-                            vrc = ctrlCopyFileToDest(pContext, pszFileSource,
-                                                    pszFileDest, 0 /* Flags */);
-                            RTStrFree(pszFileDest);
-                        }
-                        RTStrFree(pszFileSource);
-                    }
-                    else
-                        vrc = VERR_NO_MEMORY;
-                }
+                else
+                    vrc = VERR_NO_MEMORY;
                 break;
             }
 
@@ -2293,6 +2276,7 @@
         switch (rc)
         {
             case E_ABORT: /* No more directory entries left to process. */
+            case VBOX_E_OBJECT_NOT_FOUND: /* Also indicates no more entries to process. */
                 break;
 
             case VBOX_E_FILE_ERROR: /* Current entry cannot be accessed to
Edit: For some reason, the forum inserts four spaces in front of every line in the code block above, when you copy it directly. This breaks just applying the diff as is. If you want to get the actual raw text out with proper formatting, I suggest clicking the button to quote this post, and then copying from there.
jchatham
Posts: 19
Joined: 15. Jul 2013, 20:57

Re: Fixing Recursive Copy

Post by jchatham »

FYI, I just noticed that someone actually bug reported (some) of the recursive copy issues the above patch fixes; see https://www.virtualbox.org/ticket/11231
Post Reply