Rereading disks through the GUI now works.

davidgiven · davidgiven · commit 3d063e932acd · 2025-10-15T00:41:49.000+02:00
diff --git a/lib/algorithms/readerwriter.cc b/lib/algorithms/readerwriter.cc
@@ -349,7 +349,7 @@ static void adjustTrackOnError(FluxSource& fluxSource, int baseTrack)
 struct ReadGroupResult
 {
     ReadResult result;
-    std::vector<std::shared_ptr<const Sector>> sectors;
+    std::vector<std::shared_ptr<const Sector>> combinedSectors;
 };
 
 static ReadGroupResult readGroup(const DiskLayout& diskLayout,
@@ -360,6 +360,21 @@ static ReadGroupResult readGroup(const DiskLayout& diskLayout,
 {
     ReadGroupResult rgr = {BAD_AND_CAN_NOT_RETRY};
 
+    /* Before doing the read, look to see if we already have the necessary
+     * sectors. */
+
+    {
+        auto [result, sectors] = combineRecordAndSectors(tracks, decoder, ltl);
+        rgr.combinedSectors = sectors;
+        if (result == HAS_NO_BAD_SECTORS)
+        {
+            /* We have all necessary sectors, so can stop here. */
+            rgr.result = GOOD_READ;
+            if (globalConfig()->decoder().skip_unnecessary_tracks())
+                return rgr;
+        }
+    }
+
     for (unsigned offset = 0; offset < ltl->groupSize;
         offset += diskLayout.headWidth)
     {
@@ -389,7 +404,7 @@ static ReadGroupResult readGroup(const DiskLayout& diskLayout,
         /* Decode what we've got so far. */
 
         auto [result, sectors] = combineRecordAndSectors(tracks, decoder, ltl);
-        rgr.sectors = sectors;
+        rgr.combinedSectors = sectors;
         if (result == HAS_NO_BAD_SECTORS)
         {
             /* We have all necessary sectors, so can stop here. */
@@ -399,8 +414,8 @@ static ReadGroupResult readGroup(const DiskLayout& diskLayout,
         }
         else if (fluxSourceIterator.hasNext())
         {
-            /* The flux source claims it can do more reads, so mark this group
-             * as being retryable. */
+            /* The flux source claims it can do more reads, so mark this
+             * group as being retryable. */
             rgr.result = BAD_AND_CAN_RETRY;
         }
     }
@@ -634,13 +649,13 @@ void writeRawDiskCommand(const DiskLayout& diskLayout,
         diskLayout.logicalLocations);
 }
 
-TracksAndSectors readAndDecodeTrack(const DiskLayout& diskLayout,
+void readAndDecodeTrack(const DiskLayout& diskLayout,
     FluxSource& fluxSource,
     Decoder& decoder,
-    const std::shared_ptr<const LogicalTrackLayout>& ltl)
+    const std::shared_ptr<const LogicalTrackLayout>& ltl,
+    std::vector<std::shared_ptr<const Track>>& tracks,
+    std::vector<std::shared_ptr<const Sector>>& combinedSectors)
 {
-    TracksAndSectors fas;
-
     if (fluxSource.isHardware())
         measureDiskRotation();
 
@@ -649,9 +664,8 @@ TracksAndSectors readAndDecodeTrack(const DiskLayout& diskLayout,
     for (;;)
     {
         auto [result, sectors] = readGroup(
-            diskLayout, fluxSourceIteratorHolder, ltl, fas.tracks, decoder);
-        std::copy(
-            sectors.begin(), sectors.end(), std::back_inserter(fas.sectors));
+            diskLayout, fluxSourceIteratorHolder, ltl, tracks, decoder);
+        combinedSectors = sectors;
         if (result == GOOD_READ)
             break;
         if (result == BAD_AND_CAN_NOT_RETRY)
@@ -673,8 +687,6 @@ TracksAndSectors readAndDecodeTrack(const DiskLayout& diskLayout,
             retriesRemaining--;
         }
     }
-
-    return fas;
 }
 
 void readDiskCommand(const DiskLayout& diskLayout,
@@ -687,6 +699,13 @@ void readDiskCommand(const DiskLayout& diskLayout,
         outputFluxSinkFactory =
             FluxSinkFactory::create(globalConfig()->decoder().copy_flux_to());
 
+    std::map<CylinderHead, std::vector<std::shared_ptr<const Track>>>
+        tracksByLogicalLocation;
+    for (auto& [ch, track] : disk.tracksByPhysicalLocation)
+        tracksByLogicalLocation[CylinderHead(track->ltl->logicalCylinder,
+                                    track->ltl->logicalHead)]
+            .push_back(track);
+
     log(BeginOperationLogMessage{"Reading and decoding disk"});
     {
         std::unique_ptr<FluxSink> outputFluxSink;
@@ -702,13 +721,31 @@ void readDiskCommand(const DiskLayout& diskLayout,
 
             testForEmergencyStop();
 
-            auto [trackFluxes, trackSectors] =
-                readAndDecodeTrack(diskLayout, fluxSource, decoder, ltl);
+            auto& trackFluxes = tracksByLogicalLocation[logicalLocation];
+            std::vector<std::shared_ptr<const Sector>> trackSectors;
+            readAndDecodeTrack(diskLayout,
+                fluxSource,
+                decoder,
+                ltl,
+                trackFluxes,
+                trackSectors);
+
+            /* Replace all tracks on the disk by the new combined set. */
+
+            for (const auto& flux : trackFluxes)
+                disk.tracksByPhysicalLocation.erase(CylinderHead{
+                    flux->ptl->physicalCylinder, flux->ptl->physicalHead});
             for (const auto& flux : trackFluxes)
                 disk.tracksByPhysicalLocation.emplace(
                     CylinderHead{
                         flux->ptl->physicalCylinder, flux->ptl->physicalHead},
                     flux);
+
+            /* Likewise for sectors. */
+
+            for (const auto& sector : trackSectors)
+                disk.sectorsByPhysicalLocation.erase(
+                    sector->physicalLocation.value());
             for (const auto& sector : trackSectors)
                 disk.sectorsByPhysicalLocation.emplace(
                     sector->physicalLocation.value(), sector);
diff --git a/lib/algorithms/readerwriter.h b/lib/algorithms/readerwriter.h
@@ -107,19 +107,19 @@ extern void writeDiskCommand(const DiskLayout& diskLayout,
     Decoder* decoder = nullptr,
     FluxSource* fluxSource = nullptr);
 
-extern void writeRawDiskCommand(
-    const DiskLayout& diskLayout, FluxSource& fluxSource, FluxSinkFactory& fluxSinkFactory);
+extern void writeRawDiskCommand(const DiskLayout& diskLayout,
+    FluxSource& fluxSource,
+    FluxSinkFactory& fluxSinkFactory);
 
-struct TracksAndSectors
-{
-    std::vector<std::shared_ptr<const Track>> tracks;
-    std::vector<std::shared_ptr<const Sector>> sectors;
-};
+/* Reads a single group of tracks. tracks and combinedSectors are populated.
+ * tracks may contain preexisting data which will be taken into account. */
 
-extern TracksAndSectors readAndDecodeTrack(const DiskLayout& diskLayout,
+extern void readAndDecodeTrack(const DiskLayout& diskLayout,
     FluxSource& fluxSource,
     Decoder& decoder,
-    const std::shared_ptr<const LogicalTrackLayout>& ltl);
+    const std::shared_ptr<const LogicalTrackLayout>& ltl,
+    std::vector<std::shared_ptr<const Track>>& tracks,
+    std::vector<std::shared_ptr<const Sector>>& combinedSectors);
 
 extern void readDiskCommand(const DiskLayout& diskLayout,
     FluxSource& fluxSource,
diff --git a/lib/core/utils.h b/lib/core/utils.h
@@ -37,6 +37,15 @@ std::map<V, K> reverseMap(const std::map<K, V>& map)
     return reverse;
 }
 
+template <typename K, typename V>
+std::map<K, std::vector<V>> multimapToMapOfVectors(const std::multimap<K, V>& multimap)
+{
+    std::map<K, std::vector<V>> results;
+    for (auto& [k, v] : multimap)
+        results[k].push_back(v);
+    return results;
+}
+
 /* If set, any running job will terminate as soon as possible (with an error).
  */
 
diff --git a/lib/vfs/fluxsectorinterface.cc b/lib/vfs/fluxsectorinterface.cc
@@ -132,10 +132,16 @@ class FluxSectorInterface : public SectorInterface
     {
         CylinderHead logicalLocation = {logicalCylinder, logicalSide};
         auto& ltl = _diskLayout->layoutByLogicalLocation.at(logicalLocation);
-        auto trackdata =
-            readAndDecodeTrack(*_diskLayout, *_fluxSource, *_decoder, ltl);
-
-        for (const auto& sector : trackdata.sectors)
+        std::vector<std::shared_ptr<const Track>> trackFluxes;
+        std::vector<std::shared_ptr<const Sector>> trackSectors;
+        readAndDecodeTrack(*_diskLayout,
+            *_fluxSource,
+            *_decoder,
+            ltl,
+            trackFluxes,
+            trackSectors);
+
+        for (const auto& sector : trackSectors)
             *_loadedSectors.put(logicalLocation, sector->logicalSector) =
                 *sector;
         _loadedTracks.insert(logicalLocation);
diff --git a/src/gui2/controlpanelview.cc b/src/gui2/controlpanelview.cc
@@ -85,9 +85,13 @@ void ControlPanelView::drawContent()
         };
 
         ImGui::TableNextRow();
-        button(ICON_TA_DEVICE_FLOPPY,
+        button(
+            ICON_TA_DEVICE_FLOPPY,
             "fluxengine.view.controlpanel.readDevice"_lang,
-            Datastore::beginRead,
+            []
+            {
+                Datastore::beginRead(false);
+            },
             busy);
         ImGui::TableNextColumn();
         button(ICON_VS_SAVE_AS,
@@ -96,9 +100,13 @@ void ControlPanelView::drawContent()
             busy || !hasImage);
 
         ImGui::TableNextRow();
-        button(ICON_TA_REPEAT,
+        button(
+            ICON_TA_REPEAT,
             "fluxengine.view.controlpanel.rereadBad"_lang,
-            nullptr,
+            []
+            {
+                Datastore::beginRead(true);
+            },
             busy || !disk);
         ImGui::TableNextColumn();
         button(ICON_TA_DOWNLOAD,
diff --git a/src/gui2/datastore.cc b/src/gui2/datastore.cc
@@ -526,11 +526,11 @@ void Datastore::onLogMessage(const AnyLogMessage& message)
         message);
 }
 
-void Datastore::beginRead()
+void Datastore::beginRead(bool rereadBadSectors)
 {
     Events::OperationStart::post("fluxengine.view.status.readDevice"_lang);
     Datastore::runOnWorkerThread(
-        []
+        [=]
         {
             busy = true;
             failed = false;
@@ -539,12 +539,19 @@ void Datastore::beginRead()
             try
             {
                 wtRebuildConfiguration();
-                wtClearDiskData();
-                wtWaitForUiThreadToCatchUp();
+                if (!rereadBadSectors)
+                    wtClearDiskData();
+
+                std::shared_ptr<Disk> disk;
+                wtRunSynchronouslyOnUiThread((std::function<void()>)[&] {
+                    if (::disk)
+                        disk = std::make_shared<Disk>(*::disk);
+                    else
+                        disk = std::make_shared<Disk>();
+                });
                 auto fluxSource = FluxSource::create(globalConfig());
                 auto decoder = Arch::createDecoder(globalConfig());
 
-                auto disk = std::make_shared<Disk>();
                 readDiskCommand(*diskLayout, *fluxSource, *decoder, *disk);
             }
             catch (...)
diff --git a/src/gui2/datastore.h b/src/gui2/datastore.h
@@ -35,7 +35,7 @@ class Datastore
 
     /* Begins a transation. Rebuilds the configuration. */
     static void reset();
-    static void beginRead();
+    static void beginRead(bool rereadBadSectors);
     static void beginWrite();
     static void readImage(const std::fs::path& path);
     static void writeImage(const std::fs::path& path);
diff --git a/src/gui2/diskprovider.cc b/src/gui2/diskprovider.cc
@@ -80,7 +80,9 @@ void DiskProvider::readRaw(u64 offset, void* buffer, size_t size)
             unsigned blockOffset = realOffset - offset;
             unsigned bytesRemaining =
                 std::min((unsigned)size, ltl->sectorSize - blockOffset);
-            auto bytes = sector->data.slice(blockOffset, bytesRemaining);
+            auto bytes = sector
+                             ? sector->data.slice(blockOffset, bytesRemaining)
+                             : Bytes(bytesRemaining);
             memcpy(buffer, bytes.cbegin(), bytes.size());
 
             offset += bytesRemaining;
diff --git a/tests/utils.cc b/tests/utils.cc
@@ -4,6 +4,18 @@
 
 using namespace snowhouse;
 
+template <typename F, typename S>
+struct snowhouse::Stringizer<std::pair<F, S>>
+{
+    static std::string ToString(const std::pair<F, S>& a)
+    {
+        std::stringstream stream;
+        stream << "pair(" << Stringizer<F>::ToString(a.first) << ", "
+               << Stringizer<S>::ToString(a.second) << ')';
+        return stream.str();
+    }
+};
+
 static void testJoin()
 {
     AssertThat(join({}, "/"), Equals(""));
@@ -56,6 +68,26 @@ static void testUnbcd()
     AssertThat(unbcd(0x87654321), Equals(87654321));
 }
 
+static void testMultimapToMap()
+{
+    std::multimap<int, std::string> input = {
+        {0, "zero" },
+        {0, "nil"  },
+        {1, "one"  },
+        {3, "two"  },
+        {3, "drei" },
+        {3, "trois"}
+    };
+    std::map<int, std::vector<std::string>> wanted = {
+        {0, {"zero", "nil"}         },
+        {1, {"one"}                 },
+        {3, {"two", "drei", "trois"}}
+    };
+
+    auto output = multimapToMapOfVectors(input);
+    AssertThat(output, Equals(wanted));
+}
+
 int main(void)
 {
     testJoin();
@@ -65,5 +97,6 @@ int main(void)
     testLeafname();
     testUnhex();
     testUnbcd();
+    testMultimapToMap();
     return 0;
 }
