Changeset 24725 in vbox for trunk

Timestamp:

Nov 17, 2009 3:27:03 PM (15 years ago)

Author:

vboxsync

Message:

Main/Medium: Move per-operation state (progress object reference etc) to local variables. Clean up things slightly, and review the code for Medium data accesses

without holding the appropriate locks.

File:

• trunk/src/VBox/Main/MediumImpl.cpp (modified) (24 diffs)

trunk/src/VBox/Main/MediumImpl.cpp

@@ -103 +103 @@
           implicit(false),
           numCreateDiffTasks(0),
-          vdProgress(NULL),
           vdDiskIfaces(NULL)
     {}
@@ -150 +149 @@
 
     Utf8Str vdError;        /*< Error remembered by the VD error callback. */
-    Progress *vdProgress;  /*< Progress for the VD progress callback. */
 
     VDINTERFACE vdIfError;
     VDINTERFACEERROR vdIfCallsError;
-
-    VDINTERFACE vdIfProgress;
-    VDINTERFACEPROGRESS vdIfCallsProgress;
 
     VDINTERFACE vdIfConfig;
@@ -798 +793 @@
     m->vdIfCallsError.pfnMessage = NULL;
 
-    /* Initialize the callbacks of the VD progress interface */
-    m->vdIfCallsProgress.cbSize = sizeof(VDINTERFACEPROGRESS);
-    m->vdIfCallsProgress.enmInterface = VDINTERFACETYPE_PROGRESS;
-    m->vdIfCallsProgress.pfnProgress = vdProgressCall;
-
     /* Initialize the callbacks of the VD config interface */
     m->vdIfCallsConfig.cbSize = sizeof(VDINTERFACECONFIG);
@@ -811 +801 @@
 
     /* Initialize the callbacks of the VD TCP interface (we always use the host
- *      * IP stack for now) */
+     * IP stack for now) */
     m->vdIfCallsTcpNet.cbSize = sizeof(VDINTERFACETCPNET);
     m->vdIfCallsTcpNet.enmInterface = VDINTERFACETYPE_TCPNET;
@@ -829 +819 @@
     AssertRCReturn(vrc, E_FAIL);
 
-    vrc = VDInterfaceAdd(&m->vdIfProgress,
-                         "Medium::vdInterfaceProgress",
-                         VDINTERFACETYPE_PROGRESS,
-                         &m->vdIfCallsProgress, this, &m->vdDiskIfaces);
-    AssertRCReturn(vrc, E_FAIL);
     vrc = VDInterfaceAdd(&m->vdIfConfig,
                          "Medium::vdInterfaceConfig",
@@ -4767 +4752 @@
                                          void *pvUser)
 {
-    Medium *that = static_cast<Medium*>(pvUser);
-    AssertReturn(that != NULL, VERR_GENERAL_FAILURE);
-
-    if (that->m->vdProgress != NULL)
+    Progress *that = static_cast<Progress *>(pvUser);
+
+    if (that != NULL)
     {
         /* update the progress object, capping it at 99% as the final percent
          * is used for additional operations like setting the UUIDs and similar. */
-        HRESULT rc = that->m->vdProgress->SetCurrentOperationProgress(uPercent * 99 / 100);
+        HRESULT rc = that->SetCurrentOperationProgress(uPercent * 99 / 100);
         if (FAILED(rc))
         {
@@ -4866 +4850 @@
 
     Medium *that = task->that;
+
+    /* Set up a per-operation progress interface, can be used freely (for
+     * binary operations you can use it either on the source or target). */
+    VDINTERFACEPROGRESS vdIfCallsProgress;
+    vdIfCallsProgress.cbSize = sizeof(VDINTERFACEPROGRESS);
+    vdIfCallsProgress.enmInterface = VDINTERFACETYPE_PROGRESS;
+    vdIfCallsProgress.pfnProgress = Medium::vdProgressCall;
+    VDINTERFACE vdIfProgress;
+    PVDINTERFACE vdOperationIfaces = NULL;
+    int vrc = VDInterfaceAdd(&vdIfProgress,
+                             "Medium::vdInterfaceProgress",
+                             VDINTERFACETYPE_PROGRESS,
+                             &vdIfCallsProgress, task->progress, &vdOperationIfaces);
+    AssertRCReturn(vrc, E_FAIL);
 
     /// @todo ugly hack, fix ComAssert... later
@@ -4923 +4921 @@
                     PDMMEDIAGEOMETRY geo = { 0 }; /* auto-detect */
 
-                    /* needed for vdProgressCallback */
-                    that->m->vdProgress = task->progress;
-
                     vrc = VDCreateBase(hdd, format.c_str(), location.c_str(),
                                        task->d.size * _1M,
@@ -4931 +4926 @@
                                        NULL, &geo, &geo, id.raw(),
                                        VD_OPEN_FLAGS_NORMAL,
-                                       NULL, that->m->vdDiskIfaces);
-
+                                       NULL, vdOperationIfaces);
                     if (RT_FAILURE(vrc))
                     {
@@ -5042 +5036 @@
                     CheckComRCThrowRC(rc);
 
-                    /* needed for vdProgressCallback */
-                    that->m->vdProgress = task->progress;
-
                     /** @todo add VD_IMAGE_FLAGS_DIFF to the image flags, to
                      * be on the safe side. */
@@ -5054 +5045 @@
                                        VD_OPEN_FLAGS_NORMAL,
                                        target->m->vdDiskIfaces,
-                                       that->m->vdDiskIfaces);
-
-                    that->m->vdProgress = NULL;
-
+                                       vdOperationIfaces);
                     if (RT_FAILURE(vrc))
                     {
@@ -5079 +5067 @@
                  * here), but lock VirtualBox first to follow the rule */
                 AutoMultiWriteLock2 alock(that->mVirtualBox->lockHandle(),
-                                           that->treeLock());
+                                          that->treeLock());
 
                 Assert(target->mParent.isNull());
@@ -5089 +5077 @@
                 target->mVirtualBox->removeDependentChild(target);
 
+                /** @todo r=klaus neither target nor that->base() are locked,
+                 * potential race! */
                 /* diffs for immutable hard disks are auto-reset by default */
                 target->m->autoReset =
@@ -5215 +5205 @@
                     }
 
-                    /* needed for vdProgressCallback */
-                    that->m->vdProgress = task->progress;
-
                     unsigned start = chain->isForward() ?
                         0 : (unsigned)chain->size() - 1;
@@ -5225 +5212 @@
                     LogFlow(("*** MERGE from %d to %d\n", start, end));
 #endif
-                    vrc = VDMerge(hdd, start, end, that->m->vdDiskIfaces);
-
-                    that->m->vdProgress = NULL;
-
+                    vrc = VDMerge(hdd, start, end, vdOperationIfaces);
                     if (RT_FAILURE(vrc))
                         throw vrc;
@@ -5507 +5491 @@
                     }
 
-                    /* unlock before the potentially lengthy operation */
-                    thatLock.leave();
-
                     Utf8Str targetFormat(target->m->strFormat);
                     Utf8Str targetLocation(target->m->strLocationFull);
@@ -5518 +5499 @@
                     Assert(parent.isNull() || parent->m->state == MediumState_LockedRead);
 
+                    /* unlock before the potentially lengthy operation */
+                    thatLock.leave();
+
                     /* ensure the target directory exists */
                     rc = VirtualBox::ensureFilePathExists(targetLocation);
                     CheckComRCThrowRC(rc);
-
-                    /* needed for vdProgressCallback */
-                    that->m->vdProgress = task->progress;
 
                     PVBOXHDD targetHdd;
@@ -5535 +5516 @@
                              it != parentChain->end(); ++ it)
                         {
+                            /** @todo r=klaus (*it) is not locked, lots of
+                             * race opportunities below */
                             /* sanity check */
                             Assert(    (*it)->m->state == MediumState_LockedRead
@@ -5553 +5536 @@
                         }
 
+                        /** @todo r=klaus target isn't locked, race getting the state */
                         vrc = VDCopy(hdd, VD_LAST_IMAGE, targetHdd,
                                      targetFormat.c_str(),
@@ -5559 +5543 @@
                                      task->d.variant, targetId.raw(), NULL,
                                      target->m->vdDiskIfaces,
-                                     that->m->vdDiskIfaces);
-
-                        that->m->vdProgress = NULL;
-
+                                     vdOperationIfaces);
                         if (RT_FAILURE(vrc))
                         {
@@ -5770 +5751 @@
                     }
 
-                    /* needed for vdProgressCallback */
-                    that->m->vdProgress = task->progress;
-
                     vrc = VDCreateDiff(hdd, format.c_str(), location.c_str(),
                                        /// @todo use the same image variant as before
@@ -5780 +5758 @@
                                        VD_OPEN_FLAGS_NORMAL,
                                        that->m->vdDiskIfaces,
-                                       that->m->vdDiskIfaces);
-
-                    that->m->vdProgress = NULL;
-
+                                       vdOperationIfaces);
                     if (RT_FAILURE(vrc))
                     {
@@ -5863 +5838 @@
                     }
 
+                    Assert(that->m->state == MediumState_LockedWrite);
+
+                    Utf8Str location(that->m->strLocationFull);
+
                     /* unlock before the potentially lengthy operation */
                     thatLock.leave();
 
-                    Assert(that->m->state == MediumState_LockedWrite);
-
-                    /* needed for vdProgressCallback */
-                    that->m->vdProgress = task->progress;
-
-                    vrc = VDCompact(hdd, VD_LAST_IMAGE, that->m->vdDiskIfaces);
-
-                    that->m->vdProgress = NULL;
-
+                    vrc = VDCompact(hdd, VD_LAST_IMAGE, vdOperationIfaces);
                     if (RT_FAILURE(vrc))
                     {
@@ -5880 +5851 @@
                             throw setError(VBOX_E_NOT_SUPPORTED,
                                            tr("Compacting is not yet supported for hard disk '%s'"),
-                                           that->m->strLocationFull.raw());
+                                           location.raw());
                         else if (vrc == VERR_NOT_IMPLEMENTED)
                             throw setError(E_NOTIMPL,
                                            tr("Compacting is not implemented, hard disk '%s'"),
-                                           that->m->strLocationFull.raw());
+                                           location.raw());
                         else
                             throw setError(E_FAIL,
                                            tr("Could not compact hard disk '%s'%s"),
-                                           that->m->strLocationFull.raw(),
+                                           location.raw(),
                                            that->vdError(vrc).raw());
                     }