blackosx Posted May 24, 2020 Share Posted May 24, 2020 (edited) After some testing this morning with legacy boot, I've narrowed the issue down to egSaveFile() where it reports save dir is Write Protected. 13:142 0:000 no dir Write Protected 13:142 0:000 cant make dir Write Protected 13:142 0:000 no write Write Protected Spoiler 10:403 0:110 GUI ready 12:720 2:316 Make screenshot W=1920 H=1080 13:142 0:422 Loop: Index 0 13:142 0:000 Loop: Name EFI\CLOVER\misc\screenshot0.png 13:142 0:000 File not exist 13:142 0:000 no dir Write Protected 13:142 0:000 cant make dir Write Protected 13:142 0:000 no write Write Protected 13:142 0:000 Loop: Index 1 13:142 0:000 Loop: Name EFI\CLOVER\misc\screenshot1.png 13:142 0:000 File not exist 13:142 0:000 no dir Write Protected 13:142 0:000 cant make dir Write Protected 13:142 0:000 no write Write Protected ... ... 13:147 0:000 Loop: Index 59 13:147 0:000 Loop: Name EFI\CLOVER\misc\screenshot59.png 13:147 0:000 File not exist 13:147 0:000 no dir Write Protected 13:147 0:000 cant make dir Write Protected 13:147 0:000 no write Write Protected I need to do further tests as EFI\CLOVER\misc is not write protected as preboot.log and audicodec dump writes files just fine. I have to go out now but will look again later. For, ref. with UEFI boot I see 9:054 0:212 GUI ready 11:739 2:684 Make screenshot W=1920 H=1080 12:040 0:301 Loop: Index 0 12:040 0:000 Loop: Name EFI\CLOVER\misc\screenshot0.png 12:068 0:027 File not exist 12:083 0:014 not written Success 12:083 0:000 Loop: Break Edited May 24, 2020 by blackosx 1 Link to comment Share on other sites More sharing options...
blackosx Posted May 24, 2020 Share Posted May 24, 2020 hmmm... Pressing F6 to save VBIOS also reports Write Protected, but then is run a second time and succeeds 14:606 1:410 * User press F6 14:606 0:000 egSaveFile() FileName=EFI\CLOVER\misc\c0000.bin | DirName=EFI\CLOVER\misc 14:606 0:000 no dir Write Protected 14:606 0:000 cant make dir Write Protected 14:606 0:000 no write Write Protected 14:606 0:000 egSaveFile() FileName=EFI\CLOVER\misc\c0000.bin | DirName=EFI\CLOVER\misc 14:683 0:077 not written Success But F10 screenshot only runs once?? 16:235 1:552 * User press F10 16:240 0:004 Make screenshot W=1920 H=1080 16:661 0:421 Loop: Index 0 16:661 0:000 Loop: Name EFI\CLOVER\misc\screenshot0.png 16:662 0:000 File not exist 16:662 0:000 egSaveFile() FileName=EFI\CLOVER\misc\screenshot0.png | DirName=EFI\CLOVER\misc 16:662 0:000 no dir Write Protected 16:662 0:000 cant make dir Write Protected 16:662 0:000 no write Write Protected 1 Link to comment Share on other sites More sharing options...
blackosx Posted May 24, 2020 Share Posted May 24, 2020 (edited) So the reason F10 screenshot is not working for me looks like egSaveFile() is called from egScreenShot() once only using SelfRootDir as File Handle. In contrast, F2 Preboot.log, F6 VBIOS and F8 AudoCodec dumps all call egSaveFile() twice. The first time using SelfRootDir as File Handle, then again with a NULL File Handle. case SCAN_F2: SavePreBootLog = TRUE; //let it be twice Status = SaveBooterLog(SelfRootDir, PREBOOT_LOG); if (EFI_ERROR(Status)) { Status = SaveBooterLog(NULL, PREBOOT_LOG); } break; case SCAN_F6: Status = egSaveFile(SelfRootDir, VBIOS_BIN, (UINT8*)(UINTN)0xc0000, 0x20000); if (EFI_ERROR(Status)) { Status = egSaveFile(NULL, VBIOS_BIN, (UINT8*)(UINTN)0xc0000, 0x20000); } break; case SCAN_F8: // testSVG(); SaveHdaDumpBin(); which results in... if (PathHdaDump != NULL) { Status = egSaveFile(SelfRootDir, PathHdaDump, (VOID *)MemLogStart, GetMemLogLen() - MemLogStartLen); if (EFI_ERROR(Status)) Status = egSaveFile(NULL, PathHdaDump, (VOID *)MemLogStart, GetMemLogLen() - MemLogStartLen); FreePool(PathHdaDump); } EDIT: Confirmed. This edit in egScreenShot() now works here. //save file with a first unoccupied name // XStringW CommonName(L"EFI\\CLOVER\\misc\\screenshot"_XSW); for (UINTN Index = 0; Index < 60; Index++) { // ScreenshotName = PoolPrint(L"%a%d.png", ScreenShotName, Index); XStringW Name = SWPrintf("EFI\\CLOVER\\misc\\screenshot%lld.png", Index); if (!FileExists(SelfRootDir, Name.wc_str())) { Status = egSaveFile(SelfRootDir, Name.wc_str(), FileData, FileDataLength); if (EFI_ERROR(Status)) Status = egSaveFile(NULL, Name.wc_str(), FileData, FileDataLength); if (!EFI_ERROR(Status)) { break; } } } FreePool(FileData); return Status; } Edited May 24, 2020 by blackosx 2 Link to comment Share on other sites More sharing options...
Slice Posted May 25, 2020 Share Posted May 25, 2020 Thanks for the investigation! Save to NULL was invented many years ago at probably for the same reason. The resume is SelfRootDir to be not working with legacy boot. The issue can be investigated more deeper but we have a result and I committed it. if (!FileExists(SelfRootDir, Name.wc_str())) { This works anyway? 1 Link to comment Share on other sites More sharing options...
Slice Posted May 25, 2020 Share Posted May 25, 2020 Somehow ReinitSelfLib() will open SelfRootDir in ReadOnlyMode Status = SelfRootDir->Open(SelfRootDir, &SelfDir, SelfDirPath, EFI_FILE_MODE_READ, 0); CheckFatalError(Status, L"while reopening our installation directory"); return Status; There is initial codes from rEFIt itself. I am not sure. Link to comment Share on other sites More sharing options...
apianti Posted May 25, 2020 Share Posted May 25, 2020 8 minutes ago, Slice said: Somehow ReinitSelfLib() will open SelfRootDir in ReadOnlyMode Status = SelfRootDir->Open(SelfRootDir, &SelfDir, SelfDirPath, EFI_FILE_MODE_READ, 0); CheckFatalError(Status, L"while reopening our installation directory"); return Status; There is initial codes from rEFIt itself. I am not sure. There is one situation where the folder can only be opened read only, when it is booted legacy from a non-EFI system partition. But otherwise, it should be opened read/write. I believe the reason was something to do with determining which partition it was started from? I think actually should try to open it in read/write then if that fails for access then open it read only. The other work arounds with the NULLs after a failure probably aren't necessary then. 21 minutes ago, Slice said: Thanks for the investigation! Save to NULL was invented many years ago at probably for the same reason. The resume is SelfRootDir to be not working with legacy boot. The issue can be investigated more deeper but we have a result and I committed it. if (!FileExists(SelfRootDir, Name.wc_str())) { This works anyway? Is it not supposed to generate a unique filename so you can do multiple screenshots? Seems like that should be some sort of uniqueness generator while the filename exists. 1 Link to comment Share on other sites More sharing options...
Slice Posted May 25, 2020 Share Posted May 25, 2020 Legacy Clover is working as well from HFS+ partition and others read-only. But I see no where the restriction checked. Link to comment Share on other sites More sharing options...
apianti Posted May 25, 2020 Share Posted May 25, 2020 (edited) The restriction is in the file interface itself, but it only matters in this situation to use the SelfRootDir to write out files. Which is why the problem happened when the image save for the screen shot did not include the attempt with NULL after failure with SelfRootDir, which seems will always fail to create a file so is unnecessary to use at all and should just always use NULL, or it should be attempted to open correctly so you can write. I mean it's not like the debug.log will be created on the legacy non-ESP boot volume. EDIT: Oh... SelfRootDir is being used to save the file, not SelfDir. SelfRootDir should be the root of the file volume and should be read/write unless the file system is read only. So, there must be some other reason why it was failing to save, SelfRootDir and the found ESP root should end up being the same interface... Unless there's some bug. Edited May 25, 2020 by apianti Link to comment Share on other sites More sharing options...
Slice Posted May 25, 2020 Share Posted May 25, 2020 Looking here if (BaseDir == NULL) { Status = egFindESP(&BaseDir); if (EFI_ERROR(Status)) { DBG("no ESP %s\n", strerror(Status)); return Status; } } I have to propose the legacy boot folder is not ESP. @blackosx Is your legacy boot file in the ESP partition? Link to comment Share on other sites More sharing options...
blackosx Posted May 25, 2020 Share Posted May 25, 2020 No. It’s on HFS+ partition Link to comment Share on other sites More sharing options...
Slice Posted May 25, 2020 Share Posted May 25, 2020 3 minutes ago, blackosx said: No. It’s on HFS+ partition SelfRootDir will be read-only. 1 Link to comment Share on other sites More sharing options...
apianti Posted May 25, 2020 Share Posted May 25, 2020 Makes total sense then, wonder if there are any other missing saves like that? Link to comment Share on other sites More sharing options...
Slice Posted May 25, 2020 Share Posted May 25, 2020 2 hours ago, apianti said: Makes total sense then, wonder if there are any other missing saves like that? because of Link to comment Share on other sites More sharing options...
apianti Posted May 25, 2020 Share Posted May 25, 2020 (edited) I mean, are there any more calls to save a file that rely on those read only interfaces, that need to be supplemented with an additional call with NULL? Seems maybe egSaveFile should behave differently instead of calling it twice. EDIT: I messed up the punctuation, like very badly. Edited May 25, 2020 by apianti Link to comment Share on other sites More sharing options...
blackosx Posted May 25, 2020 Share Posted May 25, 2020 My bad for not spotting it was the HFS+ partition that was returning the Write Protected message. I mis-read EFI\CLOVER\misc\ for EFI\EFI\CLOVER\misc\ Anyway, screenshot is now saved when booting legacy Clover from HFS+ but I only concentrated on one file and the current fix does not allow for multiple screenshots as mentioned by apianti, and just overwrites screenshot0.png. 14 hours ago, apianti said: Is it not supposed to generate a unique filename so you can do multiple screenshots? Seems like that should be some sort of uniqueness generator while the filename exists. I can't check if screenshot file exists on NULL, in the egScreenShot() loop to increment the screenshot index, as NULL is not resolved until reaching that check for NULL BaseDir in egSaveFile() that slice pointed to. So could we maybe bring that check from egSaveFile() in to egScreenShot() to resolve NULL to ESP before the loop then check both SelfRootDir and BaseDir in the loop for screenshot existence before deciding to create new file? Link to comment Share on other sites More sharing options...
apianti Posted May 25, 2020 Share Posted May 25, 2020 I think the better thing to do would be to resolve a second root directory like ESPRootDir to the ESP, it will either be the same as SelfRootDir, or resolve to ESP in same place as lib init/reinit/etc. Then use that and don't allow passing NULL to stuff. 2 Link to comment Share on other sites More sharing options...
blackosx Posted May 25, 2020 Share Posted May 25, 2020 That sounds a cleaner solution to me. @Slice is this something you can do as you know the programming better than me? Link to comment Share on other sites More sharing options...
blackosx Posted May 26, 2020 Share Posted May 26, 2020 In light of apianti's suggestion above to introduce a second root directory for the ESP which looks quite complicated for me, I have gone ahead an produced a simpler/dirtier fix which for me works and allows multiple screenshots to be saved when booting both UEFI and legacy from my HFS+ partition. Here's the diff for /rEFIt_UEFI/libeg/libscreen.cpp --- /Users/blackosx/Desktop/Clover_Changes/original/libscreen.cpp 2020-05-26 08:56:32.000000000 +0100 +++ /Users/blackosx/Desktop/Clover_Changes/new/libscreen.cpp 2020-05-26 08:54:46.000000000 +0100 @@ -538,6 +538,7 @@ EFI_STATUS egScreenShot(VOID) { EFI_STATUS Status = EFI_NOT_READY; + EFI_FILE_HANDLE BaseDir; //take screen XImage Screen(egScreenWidth, egScreenHeight); MsgLog("Make screenshot W=%llu H=%llu\n", egScreenWidth, egScreenHeight); @@ -555,6 +556,15 @@ if (!FileData) { return EFI_NOT_READY; } + + // Quick fix to resolve BaseDir if not booted from write protected partition such as HFS+ + // This is copied from egSaveFile() in image.cpp + Status = egFindESP(&BaseDir); + if (EFI_ERROR(Status)) { + MsgLog("no ESP %s\n", strerror(Status)); + return Status; + } + //save file with a first unoccupied name // XStringW CommonName(L"EFI\\CLOVER\\misc\\screenshot"_XSW); for (UINTN Index = 0; Index < 60; Index++) { @@ -562,9 +572,13 @@ XStringW Name = SWPrintf("EFI\\CLOVER\\misc\\screenshot%lld.png", Index); if (!FileExists(SelfRootDir, Name.wc_str())) { Status = egSaveFile(SelfRootDir, Name.wc_str(), FileData, FileDataLength); - if (EFI_ERROR(Status)) - Status = egSaveFile(NULL, Name.wc_str(), FileData, FileDataLength); + if (!EFI_ERROR(Status)) { + break; + } + } + if (!FileExists(BaseDir, Name.wc_str())) { + Status = egSaveFile(BaseDir, Name.wc_str(), FileData, FileDataLength); if (!EFI_ERROR(Status)) { break; } @@ -621,4 +635,4 @@ return Status; } -/* EOF */ \ No newline at end of file +/* EOF */ Link to comment Share on other sites More sharing options...
Slice Posted May 26, 2020 Share Posted May 26, 2020 Anyway this procedure will be executed once per click F10. It will save one screenshot so it may search EFI partition once. May be it is better to search it at Clover start to exclude search again with F2,F4,F6. Link to comment Share on other sites More sharing options...
blackosx Posted May 26, 2020 Share Posted May 26, 2020 Yeah. I've only patched what's already there as currently egSaveFile() already searches for EFI partition each time NULL is passed to it. I agree to searching for EFI before, either as apianti suggested or you have just done, but either way I don't think I'm the right person to do it as you guys are far better than me and wlll get the code right. Besides, I'm trying to concentrate on Vector themes and only raised the issue about screenshots as I was doing some testing for them While discussing vector themes, can I ask for you to add the following to nanosvg.cpp above line #3540 ? } else if (strcmp(dict[i], "LayoutBannerOffset") == 0) { LayoutBannerOffset = getIntegerDict(dict[i + 1]); } else if (strcmp(dict[i], "LayoutButtonOffset") == 0) { LayoutButtonOffset = getIntegerDict(dict[i + 1]); Link to comment Share on other sites More sharing options...
Slice Posted May 26, 2020 Share Posted May 26, 2020 5 hours ago, blackosx said: Yeah. I've only patched what's already there as currently egSaveFile() already searches for EFI partition each time NULL is passed to it. I agree to searching for EFI before, either as apianti suggested or you have just done, but either way I don't think I'm the right person to do it as you guys are far better than me and wlll get the code right. Besides, I'm trying to concentrate on Vector themes and only raised the issue about screenshots as I was doing some testing for them While discussing vector themes, can I ask for you to add the following to nanosvg.cpp above line #3540 ? } else if (strcmp(dict[i], "LayoutBannerOffset") == 0) { LayoutBannerOffset = getIntegerDict(dict[i + 1]); } else if (strcmp(dict[i], "LayoutButtonOffset") == 0) { LayoutButtonOffset = getIntegerDict(dict[i + 1]); OK, committed. 1 Link to comment Share on other sites More sharing options...
georges valch Posted May 30, 2020 Share Posted May 30, 2020 Hello, I have updated successfully from 10.15.4 to 10.15.5, logged in to desktop once, seen that everything works fine, however after restart, Clover r5118 and r5117 either, doesn't show the 10.15.5 partition anymore and I can't choose the option to boot from it. It shows the backup 10.15.4 partition without problems, I log in there and I can see the 10.15.5 partition with no trouble. Thank you. Link to comment Share on other sites More sharing options...
Slice Posted May 31, 2020 Share Posted May 31, 2020 11 hours ago, georges valch said: Hello, I have updated successfully from 10.15.4 to 10.15.5, logged in to desktop once, seen that everything works fine, however after restart, Clover r5118 and r5117 either, doesn't show the 10.15.5 partition anymore and I can't choose the option to boot from it. It shows the backup 10.15.4 partition without problems, I log in there and I can see the 10.15.5 partition with no trouble. Thank you. It will be more informative if you provide Clover preboot.log in the situation. Link to comment Share on other sites More sharing options...
RandomUser Posted May 31, 2020 Share Posted May 31, 2020 (edited) I don't know if this is the right place to point out my issue with the Clover v5118. Any how the issue is that every time I want to boot into Windows to install, it would immediately crash the whole system upon when the Windows Logo appears and before the spinning dots appears. The BIOS is Legacy and isn't capable of UEFI at all. The install booting off of is formatted as MBR. debug.log Edited May 31, 2020 by RandomUser Link to comment Share on other sites More sharing options...
Slice Posted May 31, 2020 Share Posted May 31, 2020 4 hours ago, RandomUser said: I don't know if this is the right place to point out my issue with the Clover v5118. Any how the issue is that every time I want to boot into Windows to install, it would immediately crash the whole system upon when the Windows Logo appears and before the spinning dots appears. The BIOS is Legacy and isn't capable of UEFI at all. The install booting off of is formatted as MBR. debug.log You have a very problematic hardware for hackintosh. If you have a success then respect to you. Anyway the clover installation doesn't contain the driver DataHubDxe.efi. This is a mistake. Link to comment Share on other sites More sharing options...
Recommended Posts