Jump to content

Clover problems report & features request

ErmaC

By ErmaC,
in Clover

 Share
953 posts in this topic

Recommended Posts

blackosx
blackosx
Posted
Posted (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 by blackosx
  • Like 1
Link to comment
Share on other sites
blackosx
blackosx
Posted
Posted

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

 

  • Like 1
Link to comment
Share on other sites
blackosx
blackosx
Posted
Posted (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 by blackosx
  • Like 2
Link to comment
Share on other sites
Slice
Slice
Posted
Posted

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?

  • Like 1
Link to comment
Share on other sites
Slice
Slice
Posted
Posted

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
apianti
apianti
Posted
Posted
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.

  • Like 1
Link to comment
Share on other sites
apianti
apianti
Posted
Posted (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 by apianti
Link to comment
Share on other sites
Slice
Slice
Posted
Posted

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
apianti
apianti
Posted
Posted (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 by apianti
Link to comment
Share on other sites
blackosx
blackosx
Posted
Posted

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
apianti
apianti
Posted
Posted

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.

  • Like 2
Link to comment
Share on other sites
blackosx
blackosx
Posted
Posted

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
Slice
Slice
Posted
Posted

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
blackosx
blackosx
Posted
Posted

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
Slice
Slice
Posted
Posted
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.

  • Like 1
Link to comment
Share on other sites
georges valch
georges valch
Posted
Posted

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
Slice
Slice
Posted
Posted
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
RandomUser
RandomUser
Posted
Posted (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 by RandomUser
Link to comment
Share on other sites
Slice
Slice
Posted
Posted
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
 Share
Go to topic listing
×
×
  • Create New...