ECKD DASD - Locate Record Write Track Operation (x'0B') Not Working Properly #601
Comments
|
I should have added the following paragraph on page 38 as well. It makes it clear that a count of 1 must clear the track:
|
|
I was able to fix this with the update below to ckddasd.c for case: 0x05 (Write Data). Best regards, case 0x05:
/*---------------------------------------------------------------*/
/* WRITE DATA */
/*---------------------------------------------------------------*/
..
..
..
/* Write data */
rc = ckd_write_data (dev, iobuf, num, unitstat);
if (rc < 0) break;
/*****Start of added code **************************************/
/* If lrcount=1 & r0 then erase rest of the track */
if (1
&& (dev->ckdloper & CKDOPER_CODE) == CKDOPER_WRTTRK
&& dev->ckdcurrec == 0
&& dev->ckdlcount == 1
)
{
/* Write end of track marker */
rc = ckd_erase (dev, iobuf, count, (int*)&size, unitstat);
if (rc < 0) break;
/* Return normal status */
*unitstat = CSW_CE | CSW_DE;
break;
}
/******End of added code **********************************/
/* If track overflow, keep writing */ |
|
Hi Aaron, So this correction, together with the one for Issue #603, looks like this, right? --- SDL-hyperion_4f007805_/ckddasd.c 2023-11-16 15:16:55.497072079 +0100
+++ SDL-hyperion_issue_601_and_603_fix/ckddasd.c 2023-11-19 14:10:22.622284514 +0100
@@ -3322,6 +3322,7 @@
{
DefineExtent( dev, code, flags, chained, count, prevcode,
ccwseq, iobuf, more, unitstat, residual );
+ prevcode = 0x63;
}
else
{
@@ -3339,10 +3340,12 @@
ccwseq, iobuf, more, unitstat, residual );
if (*unitstat != (CSW_CE | CSW_DE))
break; // (error!)
+ prevcode = 0x63;
}
LocateRecordExtended( dev, code, flags, chained, count, prevcode,
ccwseq, iobuf, more, unitstat, residual );
+ prevcode = 0x4B;
break; // Done!
case PFX_F_DE_PSF:
@@ -4045,6 +4048,24 @@
rc = ckd_write_data (dev, iobuf, num, unitstat);
if (rc < 0) break;
+
+ /* If lrcount=1 & r0 then erase rest of the track */
+
+ if (1
+ && (dev->ckdloper & CKDOPER_CODE) == CKDOPER_WRTTRK
+ && dev->ckdcurrec == 0
+ && dev->ckdlcount == 1
+ )
+ {
+ /* Write end of track marker */
+ rc = ckd_erase (dev, iobuf, count, (int*)&size, unitstat);
+ if (rc < 0) break;
+
+ /* Return normal status */
+ *unitstat = CSW_CE | CSW_DE;
+ break;
+ }
+
/* If track overflow, keep writing */
offset = 0;
while (dev->ckdtrkof)
I've successfully tested this, and, whilst it does not fix my issue #608 (as expected), it does no harm either. (I'll work on the I/O traces for issue #608 as soon as I can.) Cheers, Peter |
|
One comment on this... We're not going to check in code with: I have no idea what 0x63 means here. That'll want to be a constant equate someplace, and be commented as to what's actually going on where it's used. Bill |
|
Hi Bill, case 0x63:
/*---------------------------------------------------------------*/
/* DEFINE EXTENT */
/*---------------------------------------------------------------*/
...
...
if (0
|| chained == 0
|| (1
&& prevcode != 0x47 // LOCATE RECORD
&& prevcode != 0x4B // LOCATE RECORD EXTENDED
&& prevcode != 0xDE // READ TRACK
&& prevcode != 0xE7 // PREFIX
) |
|
Hi Peter, |
|
Aaron, my apologies for jumping off the handle. We do need to add the comments though, because very few people will recognize the opcodes. As far as "the development team" goes, there isn't a lot of activity just now. Fish is on a vacation. It's up to the rest of us to try to do the right thing, and I want to be as cautious as possible. Some easy-to-reproduce test cases would be helpful. And practical evidence it doesn't break something else. I myself never wander far from OS/360/MVS, so I'm no help here. Bill |
|
Hi Bill, |
|
Is everything working for you now? |
|
Yes. This issue is now resolved. Thank you. |
Aaron, I'm not seeing the fix anywhere in the code. That is to say, Peter's proposed fix does not appear to ever having been committed, so from the SDL Hercules development team's point of view, the problem has not been fixed yet. May I presume that Peter's proposed fix is what needs to be committed? |
Fish-Git
added
BUG
|
Hi Fish, |
|
Bill wrote:
Aaron wrote:
I've thought about doing that myself, but I never got around to doing so yet. I think I actually like seeing the specific hex codes themselves used in the code instead. To me, seeing And besides, it's a PITA to have to refer back to the header file that contains all of the #defines each time one wants to know what actual hex code is being compared for in the statement you're looking at. It sort of breaks your train of thought when you're trying to follow the logic of the code. I realize that goes against normal good programming practice, but in this particular case I think as long as we're careful to always have a comment on each such statement that documents what CCW code is being compared (like we're already doing today), we'll be okay. If we ever start adding code that doesn't include such informative comments though, then yeah, that would of course be a problem. It's a judgement call, really. If you want to go ahead and do that, Bill (@wrljet) (or anyone else), feel free to do so. I certainly won't complain nor change it back. I'm just sort of in lazy mode right now, and as I said, I think our using comments is OK for now. |
|
Aaron wrote:
Great! I'll go ahead and commit the fix then. (And I'll be sure to add the appropriate identifying comment to each of the |
|
Aaron? (@arfineman) Quick question before I commit: Should the check for I apologize, but I've been away for a while and trying to follow (get caught up with) each of the (Which is why I'd prefer to have access to your repository/fork so that doesn't happen!) |
|
Hi Fish, |
|
Reading through all of the comments in this GitHub Issue and looking at Peter's fix more closely (which you said was correct), I now believe Peter's fix is actually wrong/bad. Near the beginning of this issue (3rd comment), you wrote that you fixed the problem by inserting the necessary code into the 0x05 (Write Data) CCW logic, just after the call to HOWEVER... Peter's proposed fix (patch) is actually erroneously inserting your new code into the 0x85 (Write Update Data) CCW logic, not the expected 0x05 (Write Data) CCW logic! (Oops!) This can be confirmed by looking at his actual proposed patch. As you will see, your new code is being inserted at line 4045(!), which is the wrong place! Line 4045 is in the 0x85 (Write Update Data) CCW logic, not in the 0x05 (Write Data) CCW logic. The actual 0x05 (Write Data) CCW logic where your fix should actually go should actually be at line 3955! Peter's patch is inserting your new proposed code into the wrong place! (Oops!) I will try to conform this by trying to throw together the simple stand-alone test case that you proposed in your issue's opening comments. It might take me a day or three, but I want to be damn sure we're committing the right fix in the right place! Thanks. p.s. Peter? (@Peter-J-Jansen) I would appreciate it if you would confirm my findings please. Sometimes when you post patches in text form like we usually do, looks can be deceiving. I suspect Aaron saw just the code that was being inserted (and the two 'prevcode' statements) and thought to himself "That looks good to me!", but with him likely not being familiar with the format of patch (diff) files, didn't notice the actual line number of where the code was actually being inserted was wrong! For Aaron's sake, the lines that start with Ref: https://www.google.com/search?q=format+of+diff+or+patch+files%3F |
|
Hi Fish, |
Yes please! That would certainly help a lot, for now.
Understood! But if you're interested, we can certainly help you to do that. Trust me, it's not that difficult or complicated. It would really help both us and yourself a lot in the future too. It really does make managing things much easier, and wouldn't impact your use of Hercules Helper at all. It's truly a win-win. |
|
Hi Fish, |
Thanks! Fix committed! (337ad61). Closing issue. |
Fish-Git
removed
the
QUESTION...
. New test #8: Write Data, erase reminder of track . New test #9: Verify test #8 erasure (try reading record on erased track) Note: This update required recreating the "3390.cckd64" test dasd to actually have data on track 0, and adding shadow file support to the test script to ensure the actual base dasd image is never actually modified since it is a part of the repository. (We don't want our verification tests to actually modify any repository files!)
According to SC26-7298-01 page 39:
Under Hercules exception 2 is not honored and track is not erased after R0 data is updated.
Here is a sample channel program that fails (Completes with no errors, but does not erase the track as it should):
A simple test is to create a 1 cylinder disk with
dasdinit -lfs T:/VOL123 3390 VOL123 1and execute the above CCW chain.The track 0 is not being erased.
I see in
ckddasd.croutineckd_write_ckdafter every Write CKD an end-of-track marker is added.I think
ckd_write_dataroutine should be updated to check for Write Data of R0 and the operation of Write Track('0B') and a count of 1. If so, an end-of-track marker should be added.Best regards,
The text was updated successfully, but these errors were encountered: