Changeset 78666 in vbox for trunk/src

Timestamp:

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

Author:

vboxsync

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

Files:

• include/GuestFileImpl.h (modified) (1 diff)
• include/GuestSessionImplTasks.h (modified) (5 diffs)
• include/ProgressImpl.h (modified) (1 diff)
• src-all/ProgressImpl.cpp (modified) (1 diff)
• src-client/GuestFileImpl.cpp (modified) (1 diff)
• src-client/GuestSessionImpl.cpp (modified) (2 diffs)
• src-client/GuestSessionImplTasks.cpp (modified) (10 diffs)

trunk/src/VBox/Main/include/GuestFileImpl.h

@@ -61 +61 @@
     int             i_closeFile(int *pGuestRc);
     EventSource    *i_getEventSource(void) { return mEventSource; }
+    static const char *i_guestVrcToString(int guestRc);
     static Utf8Str  i_guestErrorToString(int guestRc);
     int             i_onFileNotify(PVBOXGUESTCTRLHOSTCBCTX pCbCtx, PVBOXGUESTCTRLHOSTCALLBACK pSvcCbData);

trunk/src/VBox/Main/include/GuestSessionImplTasks.h

@@ -47 +47 @@
         : enmType(FsObjType_Unknown)
         , enmPathStyle(PathStyle_Unknown)
-        , fDryRun(false) { }
+        , fDryRun(false) { } /** @todo r=bird: only half initialized. See comments in GuestSession::fileCopyToGuest(). */
 
     Utf8Str     strSource;
@@ -213 +213 @@
     int setProgressSuccess(void);
     HRESULT setProgressErrorMsg(HRESULT hr, const Utf8Str &strMsg);
+    HRESULT setProgressErrorMsg(HRESULT hrc, int vrc, const char *pszFormat, ...);
 
     inline void setTaskDesc(const Utf8Str &strTaskDesc) throw()
@@ -281 +282 @@
 public:
 
-    GuestSessionTaskCopyFrom(GuestSession *pSession, GuestSessionFsSourceSet vecSrc, const Utf8Str &strDest);
+    GuestSessionTaskCopyFrom(GuestSession *pSession, GuestSessionFsSourceSet const &vecSrc, const Utf8Str &strDest);
     virtual ~GuestSessionTaskCopyFrom(void);
 
@@ -295 +296 @@
 public:
 
-    GuestSessionTaskCopyTo(GuestSession *pSession, GuestSessionFsSourceSet vecSrc, const Utf8Str &strDest);
+    GuestSessionTaskCopyTo(GuestSession *pSession, GuestSessionFsSourceSet const &vecSrc, const Utf8Str &strDest);
     virtual ~GuestSessionTaskCopyTo(void);
 
@@ -309 +310 @@
 public:
 
-    GuestSessionTaskUpdateAdditions(GuestSession *pSession,
-                               const Utf8Str &strSource, const ProcessArguments &aArguments,
-                               uint32_t fFlags);
+    GuestSessionTaskUpdateAdditions(GuestSession *pSession, const Utf8Str &strSource,
+                                    const ProcessArguments &aArguments, uint32_t fFlags);
     virtual ~GuestSessionTaskUpdateAdditions(void);
     int Run(void);

trunk/src/VBox/Main/include/ProgressImpl.h

@@ -139 +139 @@
                               const char *aText,
                               va_list va);
+    HRESULT i_notifyCompleteBoth(HRESULT aResultCode,
+                                 int vrc,
+                                 const GUID &aIID,
+                                 const char *pcszComponent,
+                                 const char *aText,
+                                 ...);
+    HRESULT i_notifyCompleteBothV(HRESULT aResultCode,
+                                  int vrc,
+                                  const GUID &aIID,
+                                  const char *pcszComponent,
+                                  const char *aText,
+                                  va_list va);
 
     bool i_setCancelCallback(void (*pfnCallback)(void *), void *pvUser);

trunk/src/VBox/Main/src-all/ProgressImpl.cpp

@@ -412 +412 @@
     AssertComRCReturnRC(rc);
     errorInfo->init(aResultCode, aIID, pcszComponent, text);
+
+    return notifyComplete(aResultCode, errorInfo);
+}
+
+/**
+ * Wrapper around Progress:notifyCompleteBothV.
+ */
+HRESULT Progress::i_notifyCompleteBoth(HRESULT aResultCode,
+                                       int vrc,
+                                       const GUID &aIID,
+                                       const char *pcszComponent,
+                                       const char *aText,
+                                       ...)
+{
+    va_list va;
+    va_start(va, aText);
+    HRESULT hrc = i_notifyCompleteBothV(aResultCode, vrc, aIID, pcszComponent, aText, va);
+    va_end(va);
+    return hrc;
+}
+
+/**
+ * Marks the operation as complete and attaches full error info.
+ *
+ * @param aResultCode   Operation result (error) code, must not be S_OK.
+ * @param vrc           VBox status code to associate with the error.
+ * @param aIID          IID of the interface that defines the error.
+ * @param pszComponent  Name of the component that generates the error.
+ * @param pszFormat     Error message (must not be null), an RTStrPrintf-like
+ *                      format string in UTF-8 encoding.
+ * @param va            List of arguments for the format string.
+ */
+HRESULT Progress::i_notifyCompleteBothV(HRESULT aResultCode,
+                                        int vrc,
+                                        const GUID &aIID,
+                                        const char *pszComponent,
+                                        const char *pszFormat,
+                                        va_list va)
+{
+    /* expected to be used only in case of error */
+    Assert(FAILED(aResultCode));
+
+    Utf8Str text(pszFormat, va);
+    ComObjPtr<VirtualBoxErrorInfo> errorInfo;
+    HRESULT rc = errorInfo.createObject();
+    AssertComRCReturnRC(rc);
+    errorInfo->initEx(aResultCode, vrc, aIID, pszComponent, text);
 
     return notifyComplete(aResultCode, errorInfo);

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);