Fixing Recursive Copy

Discussions related to using the OSE version of VirtualBox.

Fixing Recursive Copy

Postby jchatham » 22. Oct 2014, 21:21

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   Expand viewCollapse view
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;
jchatham
 
Posts: 19
Joined: 15. Jul 2013, 20:57

Re: Fixing Recursive Copy

Postby Perryg » 22. Oct 2014, 22:35

You need to post this kind of thing to the DEVs mail list.
Perryg
Site Moderator
 
Posts: 34373
Joined: 6. Sep 2008, 22:55
Primary OS: Linux other
VBox Version: OSE self-compiled
Guest OSses: *NIX

Re: Fixing Recursive Copy

Postby frank » 23. Oct 2014, 11:31

Thank you for this fix! It was applied to trunk and will be also part of the next 4.3.x maintenance release.
frank
Oracle Corporation
 
Posts: 3362
Joined: 7. Jun 2007, 09:11
Location: Dresden, Germany
Primary OS: Debian Sid
VBox Version: PUEL
Guest OSses: Linux, Windows

Re: Fixing Recursive Copy

Postby jchatham » 24. Oct 2014, 00:34

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.
jchatham
 
Posts: 19
Joined: 15. Jul 2013, 20:57

Re: Fixing Recursive Copy

Postby Perryg » 24. Oct 2014, 01:31

Actually the DEVs do come by here on occasion, but it is much faster usually posting to the DEV mail list.
Perryg
Site Moderator
 
Posts: 34373
Joined: 6. Sep 2008, 22:55
Primary OS: Linux other
VBox Version: OSE self-compiled
Guest OSses: *NIX

Re: Fixing Recursive Copy

Postby jchatham » 24. Oct 2014, 22:05

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   Expand viewCollapse view
--- 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

Postby jchatham » 27. Oct 2014, 21:04

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
jchatham
 
Posts: 19
Joined: 15. Jul 2013, 20:57


Return to VirtualBox OSE

Who is online

Users browsing this forum: No registered users and 2 guests