Changeset 78666 in vbox for trunk/src/VBox/Main/src-client

Timestamp:

May 22, 2019 3:27:27 PM (6 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

130737

Message:

Main/GuestControl: Better error message from copyToGuest and various related cleanups and todo notes. Tip: Pass objects like GuestSessionFsSourceSet by reference instead of copies all the time. bugref:9320

Location:

trunk/src/VBox/Main/src-client

Files:

• GuestFileImpl.cpp (modified) (1 diff)
• GuestSessionImpl.cpp (modified) (2 diffs)
• GuestSessionImplTasks.cpp (modified) (10 diffs)

trunk/src/VBox/Main/src-client/GuestFileImpl.cpp

@@ -396 +396 @@
 }
 
-/* static */
-Utf8Str GuestFile::i_guestErrorToString(int rcGuest)
-{
-    Utf8Str strError;
-
+/* static */ const char *GuestFile::i_guestVrcToString(int rcGuest)
+{
     /** @todo pData->u32Flags: int vs. uint32 -- IPRT errors are *negative* !!! */
     switch (rcGuest)
     {
-        case VERR_ACCESS_DENIED:
-            strError += Utf8StrFmt(tr("Access denied"));
-            break;
-
-        case VERR_ALREADY_EXISTS:
-            strError += Utf8StrFmt(tr("File already exists"));
-            break;
-
-        case VERR_FILE_NOT_FOUND:
-            strError += Utf8StrFmt(tr("File not found"));
-            break;
-
-        case VERR_NET_HOST_NOT_FOUND:
-            strError += Utf8StrFmt(tr("Host name not found"));
-            break;
-
-        case VERR_SHARING_VIOLATION:
-            strError += Utf8StrFmt(tr("Sharing violation"));
-            break;
-
-        default:
-            strError += Utf8StrFmt("%Rrc", rcGuest);
-            break;
-    }
-
-    return strError;
+        case VERR_ACCESS_DENIED:        return tr("Access denied");
+        case VERR_ALREADY_EXISTS:       return tr("File already exists");
+        case VERR_FILE_NOT_FOUND:       return tr("File not found");
+        case VERR_NET_HOST_NOT_FOUND:   return tr("Host name not found");
+        case VERR_SHARING_VIOLATION:    return tr("Sharing violation");
+        default:                        return RTErrGetDefine(rcGuest);
+    }
+}
+
+/**
+ * @todo r=bird: This is an absolutely cryptic way of reporting errors.  You may convert
+ *               this to a const char * returning function for explaining rcGuest and
+ *               use that as part of a _proper_ error message.  This alone extremely
+ *               user unfriendly. E.g. which file is not found? One of the source files,
+ *               a destination file, what are you referring to?!?
+ *
+ *               I've addressed one of these that annoyed me, you can do the rest of them.
+ */
+/* static */ Utf8Str GuestFile::i_guestErrorToString(int rcGuest)
+{
+    /** @todo pData->u32Flags: int vs. uint32 -- IPRT errors are *negative* !!! */
+    return i_guestVrcToString(rcGuest);
 }
 

trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp

@@ -793 +793 @@
 
     /* Validate stuff. */
-    if (RT_UNLIKELY(SourceSet.size() == 0 || *(SourceSet[0].strSource.c_str()) == '\0')) /* At least one source must be present. */
-        return setError(E_INVALIDARG, tr("No source(s) specified"));
-    if (RT_UNLIKELY((strDestination.c_str()) == NULL || *(strDestination.c_str()) == '\0'))
+/** @todo r=bird: these validations are better left to the caller.  The first one in particular as there is only one
+ * of the four callers which supplies a user specified source set, making an assertion more appropriate and efficient
+ * here. */
+    if (RT_UNLIKELY(SourceSet.size() == 0)) /* At least one source must be present. */
+        return setError(E_INVALIDARG, tr("No sources specified"));
+    if (RT_UNLIKELY(SourceSet[0].strSource.isEmpty()))
+        return setError(E_INVALIDARG, tr("First source entry is empty"));
+    if (RT_UNLIKELY(strDestination.isEmpty()))
         return setError(E_INVALIDARG, tr("No destination specified"));
 
@@ -3027 +3032 @@
     GuestSessionFsSourceSet SourceSet;
 
+    /** @todo r=bird: The GuestSessionFsSourceSpec constructor does not zero the
+     *        members you aren't setting here and there are no hints about "input"
+     *        vs "task" members, so you have me worrying about using random stack by
+     *        accident somewhere...  For instance Type.File.phFile sure sounds like
+     *        an input field and thus a disaster waiting to happen. */
     GuestSessionFsSourceSpec source;
     source.strSource            = aSource;

trunk/src/VBox/Main/src-client/GuestSessionImplTasks.cpp

@@ -208 +208 @@
                                                   COM_IIDOF(IGuestSession),
                                                   GuestSession::getStaticComponentName(),
+                                                  /** @todo r=bird: i_notifyComplete takes a format string, so this is
+                                                   *        potentially risky business if a user input mentioned by the message
+                                                   *        text contains '%s'!  With code below for how to do this less
+                                                   *        painfully and with fewer string copies. */
                                                   strMsg.c_str());
         if (FAILED(hr2))
@@ -213 +217 @@
     }
     return hr; /* Return original rc. */
+}
+
+HRESULT GuestSessionTask::setProgressErrorMsg(HRESULT hrc, int vrc, const char *pszFormat, ...)
+{
+    LogFlowFunc(("hrc=%Rhrc, vrc=%Rrc, pszFormat=%s\n", hrc, vrc, pszFormat));
+
+    /* The progress object is optional. */
+    if (!mProgress.isNull())
+    {
+        BOOL fCanceled;
+        BOOL fCompleted;
+        if (   SUCCEEDED(mProgress->COMGETTER(Canceled(&fCanceled)))
+            && !fCanceled
+            && SUCCEEDED(mProgress->COMGETTER(Completed(&fCompleted)))
+            && !fCompleted)
+        {
+            va_list va;
+            va_start(va, pszFormat);
+            HRESULT hrc2 = mProgress->i_notifyCompleteBothV(hrc, vrc, COM_IIDOF(IGuestSession),
+                                                            GuestSession::getStaticComponentName(), pszFormat, va);
+            va_end(va);
+            if (FAILED(hrc2))
+                hrc = hrc2;
+        }
+    }
+    return hrc;
 }
 
@@ -763 +793 @@
     int rcGuest = VERR_IPE_UNINITIALIZED_STATUS;
     int rc = mSession->i_fsQueryInfo(strDest, TRUE /* fFollowSymlinks */, dstObjData, &rcGuest);
-    if (RT_FAILURE(rc))
-    {
-        switch (rc)
-        {
-            case VERR_GSTCTL_GUEST_ERROR:
-                if (rcGuest == VERR_FILE_NOT_FOUND) /* File might not exist on the guest yet. */
-                {
-                    rc = VINF_SUCCESS;
-                }
-                else
-                   setProgressErrorMsg(VBOX_E_IPRT_ERROR, GuestFile::i_guestErrorToString(rcGuest));
-                break;
-
-            default:
-                setProgressErrorMsg(VBOX_E_IPRT_ERROR,
-                                    Utf8StrFmt(GuestSession::tr("Destination file lookup for \"%s\" failed: %Rrc"),
-                                               strDest.c_str(), rc));
-                break;
-        }
+    if (rc == VERR_GSTCTL_GUEST_ERROR && rcGuest == VERR_FILE_NOT_FOUND) /* File might not exist on the guest yet. */
+        rc = VINF_SUCCESS;
+    else if (RT_FAILURE(rc))
+    {
+        setProgressErrorMsg(VBOX_E_IPRT_ERROR, rc == VERR_GSTCTL_GUEST_ERROR ? rcGuest : rc,
+                            GuestSession::tr("Destination file lookup for \"%s\" failed: %Rrc"),
+                            strDest.c_str(), rc == VERR_GSTCTL_GUEST_ERROR ? rcGuest : rc);
+        return rc;
     }
     else
@@ -791 +810 @@
                 /* Build the final file name with destination path (on the host). */
                 char szDstPath[RTPATH_MAX];
+                /** @todo r=bird: WTF IS THIS SUPPOSED TO BE?!? Use RTStrCopy, memcpy, or similar!
+                 * Ever thought about modifying strDestFinal (it's 'Dst' not 'Dest' btw.)
+                 * directly?  There are any number of append and insert methods in the
+                 * RTCString/Utf8Str class for doing that! */
                 RTStrPrintf2(szDstPath, sizeof(szDstPath), "%s", strDest.c_str());
 
+                /** @todo r=bird: You're totally ignoring the guest slash-style here! Callers
+                 *        should have this info. */
                 if (   !strDest.endsWith("\\")
                     && !strDest.endsWith("/"))
@@ -848 +873 @@
     if (RT_FAILURE(rc))
     {
-        switch (rc)
-        {
-            case VERR_GSTCTL_GUEST_ERROR:
-                setProgressErrorMsg(VBOX_E_IPRT_ERROR, GuestFile::i_guestErrorToString(rcGuest));
-                break;
-
-            default:
-                setProgressErrorMsg(VBOX_E_IPRT_ERROR,
-                                    Utf8StrFmt(GuestSession::tr("Destination file \"%s\" could not be opened: %Rrc"),
-                                               strDestFinal.c_str(), rc));
-                break;
-        }
-    }
-
-    if (RT_FAILURE(rc))
+        setProgressErrorMsg(VBOX_E_IPRT_ERROR, rc == VERR_GSTCTL_GUEST_ERROR ? rcGuest : rc,
+                            GuestSession::tr("Destination file \"%s\" could not be opened: %Rrc"),
+                            strDestFinal.c_str(), rc == VERR_GSTCTL_GUEST_ERROR ? rcGuest : rc);
         return rc;
+    }
 
     char szSrcReal[RTPATH_MAX];
@@ -1383 +1397 @@
 }
 
-GuestSessionTaskCopyFrom::GuestSessionTaskCopyFrom(GuestSession *pSession, GuestSessionFsSourceSet vecSrc, const Utf8Str &strDest)
-                                                   : GuestSessionCopyTask(pSession)
+GuestSessionTaskCopyFrom::GuestSessionTaskCopyFrom(GuestSession *pSession, GuestSessionFsSourceSet const &vecSrc,
+                                                   const Utf8Str &strDest)
+    : GuestSessionCopyTask(pSession)
 {
     m_strTaskName = "gctlCpyFrm";
@@ -1629 +1644 @@
 }
 
-GuestSessionTaskCopyTo::GuestSessionTaskCopyTo(GuestSession *pSession, GuestSessionFsSourceSet vecSrc, const Utf8Str &strDest)
-                                               : GuestSessionCopyTask(pSession)
+GuestSessionTaskCopyTo::GuestSessionTaskCopyTo(GuestSession *pSession, GuestSessionFsSourceSet const &vecSrc,
+                                               const Utf8Str &strDest)
+    : GuestSessionCopyTask(pSession)
 {
     m_strTaskName = "gctlCpyTo";
@@ -1801 +1817 @@
         if (pList->mSourceSpec.enmType == FsObjType_Directory)
         {
-            fCopyIntoExisting = pList->mSourceSpec.Type.Dir.fCopyFlags & DirectoryCopyFlag_CopyIntoExisting;
+            fCopyIntoExisting = RT_BOOL(pList->mSourceSpec.Type.Dir.fCopyFlags & DirectoryCopyFlag_CopyIntoExisting);
             fFollowSymlinks   = pList->mSourceSpec.Type.Dir.fFollowSymlinks;
 
@@ -1814 +1830 @@
         else if (pList->mSourceSpec.enmType == FsObjType_File)
         {
-            fCopyIntoExisting = !RT_BOOL(pList->mSourceSpec.Type.File.fCopyFlags & FileCopyFlag_NoReplace);
+            fCopyIntoExisting = !(pList->mSourceSpec.Type.File.fCopyFlags & FileCopyFlag_NoReplace);
             fFollowSymlinks   = RT_BOOL(pList->mSourceSpec.Type.File.fCopyFlags & FileCopyFlag_FollowLinks);
         }
@@ -1937 +1953 @@
 
 int GuestSessionTaskUpdateAdditions::copyFileToGuest(GuestSession *pSession, RTVFS hVfsIso,
-                                                Utf8Str const &strFileSource, const Utf8Str &strFileDest,
-                                                bool fOptional)
+                                                     Utf8Str const &strFileSource, const Utf8Str &strFileDest,
+                                                     bool fOptional)
 {
     AssertPtrReturn(pSession, VERR_INVALID_POINTER);