From 9ab1dae5534556f4c1f1f6cc29b2c6586483029f Mon Sep 17 00:00:00 2001
From: David Given <dg@cowlark.com>
Date: Sat, 26 Mar 2022 00:19:07 +0100
Subject: [PATCH] Correctly support retrying on hardware.

---
 lib/flux.h          |  3 +++
 lib/readerwriter.cc | 34 +++++++++++++++++++++++++++-------
 lib/sector.cc       |  8 ++++++++
 lib/sector.h        |  5 +++++
 4 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/lib/flux.h b/lib/flux.h
index 5b5f3075..17b247f6 100644
--- a/lib/flux.h
+++ b/lib/flux.h
@@ -1,6 +1,9 @@
 #ifndef FLUX_H
 #define FLUX_H
 
+#include "bytes.h"
+
+class Fluxmap;
 class Sector;
 class Image;
 
diff --git a/lib/readerwriter.cc b/lib/readerwriter.cc
index 74abb25b..36a27ff1 100644
--- a/lib/readerwriter.cc
+++ b/lib/readerwriter.cc
@@ -20,6 +20,13 @@
 #include "proto.h"
 #include <optional>
 
+enum ReadResult
+{
+    GOOD_READ,
+    BAD_AND_CAN_RETRY,
+    BAD_AND_CAN_NOT_RETRY
+};
+
 static std::set<std::shared_ptr<const Sector>> collectSectors(
     std::set<std::shared_ptr<const Sector>>& track_sectors,
     bool collapse_conflicts = true)
@@ -76,7 +83,7 @@ static std::set<std::shared_ptr<const Sector>> collectSectors(
 }
 
 /* Returns true if the result contains bad sectors. */
-bool combineRecordAndSectors(TrackFlux& trackFlux, AbstractDecoder& decoder)
+bool combineRecordAndSectors(TrackFlux& trackFlux, AbstractDecoder& decoder, const Location& location)
 {
     std::set<std::shared_ptr<const Sector>> track_sectors;
 
@@ -86,7 +93,7 @@ bool combineRecordAndSectors(TrackFlux& trackFlux, AbstractDecoder& decoder)
 
 	for (unsigned logical_sector : decoder.requiredSectors(trackFlux.location))
 	{
-        auto sector = std::make_shared<Sector>();
+        auto sector = std::make_shared<Sector>(location);
         sector->logicalSector = logical_sector;
         sector->status = Sector::MISSING;
 		track_sectors.insert(sector);
@@ -103,16 +110,20 @@ bool combineRecordAndSectors(TrackFlux& trackFlux, AbstractDecoder& decoder)
 }
 
 /* Returns true if the result contains bad sectors. */
-bool readGroup(FluxSource& fluxSource,
+ReadResult readGroup(FluxSource& fluxSource,
     const Location& location,
     TrackFlux& trackFlux,
     AbstractDecoder& decoder)
 {
+    ReadResult result = BAD_AND_CAN_NOT_RETRY;
+
     for (unsigned offset = 0; offset < location.groupSize;
          offset += config.drive().head_width())
     {
         auto fluxSourceIterator = fluxSource.readFlux(
             location.physicalTrack + offset, location.head);
+        if (!fluxSourceIterator->hasNext())
+            continue;
 
         Logger() << BeginReadOperationLogMessage{
             location.physicalTrack + offset, location.head};
@@ -126,11 +137,13 @@ bool readGroup(FluxSource& fluxSource,
 
         auto trackdataflux = decoder.decodeToSectors(fluxmap, location);
         trackFlux.trackDatas.push_back(trackdataflux);
-        if (!combineRecordAndSectors(trackFlux, decoder))
-			return false;
+        if (!combineRecordAndSectors(trackFlux, decoder, location))
+            return GOOD_READ;
+        if (fluxSourceIterator->hasNext())
+            result = BAD_AND_CAN_RETRY;
     }
 
-	return true;
+	return result;
 }
 
 void writeTracks(FluxSink& fluxSink,
@@ -302,8 +315,15 @@ std::shared_ptr<const DiskFlux> readDiskCommand(
         int retriesRemaining = config.decoder().retries();
         for (;;)
         {
-            if (!readGroup(fluxSource, location, *trackFlux, decoder))
+            auto result = readGroup(fluxSource, location, *trackFlux, decoder);
+            if (result == GOOD_READ)
                 break;
+            if (result == BAD_AND_CAN_NOT_RETRY)
+            {
+                failures = true;
+                Logger() << fmt::format("no more data; giving up");
+                break;
+            }
 
             if (retriesRemaining == 0)
             {
diff --git a/lib/sector.cc b/lib/sector.cc
index 4d6460a0..4a27a441 100644
--- a/lib/sector.cc
+++ b/lib/sector.cc
@@ -1,7 +1,15 @@
 #include "globals.h"
+#include "flux.h"
 #include "sector.h"
 #include "fmt/format.h"
 
+Sector::Sector(const Location& location):
+    physicalTrack(location.physicalTrack),
+    physicalHead(location.head),
+    logicalTrack(location.logicalTrack),
+    logicalSide(location.head)
+{}
+
 std::string Sector::statusToString(Status status)
 {
     switch (status)
diff --git a/lib/sector.h b/lib/sector.h
index 5bd50f8b..eaaf2946 100644
--- a/lib/sector.h
+++ b/lib/sector.h
@@ -5,6 +5,7 @@
 #include "fluxmap.h"
 
 class Record;
+class Location;
 
 /*
  * Note that sectors here used zero-based numbering throughout (to make the
@@ -43,6 +44,10 @@ public:
     Bytes data;
     std::vector<std::shared_ptr<Record>> records;
 
+    Sector() {}
+
+    Sector(const Location& location);
+
     std::tuple<int, int, int, Status> key() const
     {
         return std::make_tuple(