On 10 February 2016 at 14:16, Ryan Harkin <[email protected]> wrote: > Hi Jim, > > Thanks for the quick fix! It works for me on ARM Juno R0, R1 and R2 > and Versatile Express TC2. > > On 10 February 2016 at 13:45, <[email protected]> wrote: >> ShellPkg: ShellFileHandleReadLine must return UCS2 lines. >> >> An earlier change had this function returning the type of lines that were in >> the file being read (ASCII or UCS2). The way it is used, UCS2 output is >> expected, even when the file being read is ASCII. This change restores that >> behavior and documents it. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jim Dailey > ^^ your email address is missing from your sign-off, I think it's > mandatory, but don't hold me to that. > > I haven't had chance to read and understand the code, but I can at > least provide: > > Tested-by: Ryan Harkin <[email protected]> >
I've just updated to the tip of tianocore and I see the same problem as before. I have made sure I'm running the binary I built from this point in the tree, after the fix has been merged: 62989e0 2016-02-10 Merge branch 'master' of https://github.com/tianocore/edk2 [Jaben Carsey] And it doesn't work. So I went back to this commit: d2a0d2e 2016-02-08 ShellPkg: Fix ASCII and UNICODE file pipes. [jaben carsey] And applied the patch myself from the original email in this thread and it works for me. So I've looked again. If I checkout the tree right where the fix went in: 2dda8a1 2016-02-10 ShellPkg: ShellFileHandleReadLine must return UCS2 lines. [Jim Dailey] It also works. If I then checkout the tree at the next commit, a strange merge commit: 3a01358 2016-02-10 Merge branch 'master' of https://github.com/tianocore/edk2 [Jaben Carsey] Then part of the fix vanishes and the code no longer works. If I do a "git show -1 -m 3a01358", then I can see that the merge commit does indeed appear to add some of the code back. A further merge commit in the tree: 62989e0 2016-02-10 Merge branch 'master' of https://github.com/tianocore/edk2 [Jaben Carsey] Has loads of other funky stuff in it and it adds part of the change back, but only the comment part, the code code hunk that fixes my problem. Whilst I was surprised to see these merge commits in the tree in the first place, I didn't think too much about it. But now I'm seeing the diffs they are introducing, I'm either going mad or I'm getting worried about the workflow in the tree :( Can someone have a look into the tree and please confirm either that I've lost the plot or that something strange has happened? > Thanks, > Ryan. > >> --- >> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c >> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c >> index 4b53c70..abff0d3 100644 >> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c >> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c >> @@ -4084,9 +4084,20 @@ ShellFileHandleReturnLine( >> If the position upon start is 0, then the Ascii Boolean will be set. >> This should be >> maintained and not changed for all operations with the same file. >> >> + NOTE: LINES THAT ARE RETURNED BY THIS FUNCTION ARE UCS2, EVEN IF THE FILE >> BEING READ >> + IS IN ASCII FORMAT. >> + >> @param[in] Handle SHELL_FILE_HANDLE to read from. >> - @param[in, out] Buffer The pointer to buffer to read into. >> - @param[in, out] Size The pointer to number of bytes in Buffer. >> + @param[in, out] Buffer The pointer to buffer to read into. If >> this function >> + returns EFI_SUCCESS, then on output Buffer >> will >> + contain a UCS2 string, even if the file >> being >> + read is ASCII. >> + @param[in, out] Size On input, pointer to number of bytes in >> Buffer. >> + On output, unchanged unless Buffer is too >> small >> + to contain the next line of the file. In >> that >> + case Size is set to the number of bytes >> needed >> + to hold the next line of the file (as a >> UCS2 >> + string, even if it is an ASCII file). >> @param[in] Truncate If the buffer is large enough, this has no >> effect. >> If the buffer is is too small and Truncate >> is TRUE, >> the line will be truncated. >> @@ -4165,43 +4176,27 @@ ShellFileHandleReadLine( >> // >> // if we have space save it... >> // >> - if ((CountSoFar + 1) * CharSize < *Size){ >> + if ((CountSoFar+1)*sizeof(CHAR16) < *Size){ >> ASSERT(Buffer != NULL); >> - if (*Ascii) { >> - ((CHAR8*)Buffer)[CountSoFar] = (CHAR8) CharBuffer; >> - ((CHAR8*)Buffer)[CountSoFar+1] = '\0'; >> - } >> - else { >> - ((CHAR16*)Buffer)[CountSoFar] = CharBuffer; >> - ((CHAR16*)Buffer)[CountSoFar+1] = CHAR_NULL; >> - } >> + ((CHAR16*)Buffer)[CountSoFar] = CharBuffer; >> + ((CHAR16*)Buffer)[CountSoFar+1] = CHAR_NULL; >> } >> } >> >> // >> // if we ran out of space tell when... >> // >> - if (Status != EFI_END_OF_FILE){ >> - if ((CountSoFar + 1) * CharSize > *Size){ >> - *Size = (CountSoFar + 1) * CharSize; >> - if (!Truncate) { >> - gEfiShellProtocol->SetFilePosition(Handle, OriginalFilePosition); >> - } else { >> - DEBUG((DEBUG_WARN, "The line was truncated in >> ShellFileHandleReadLine")); >> - } >> - return (EFI_BUFFER_TOO_SMALL); >> - } >> - >> - if (*Ascii) { >> - if (CountSoFar && ((CHAR8*)Buffer)[CountSoFar - 1] == '\r') { >> - ((CHAR8*)Buffer)[CountSoFar - 1] = '\0'; >> - } >> - } >> - else { >> - if (CountSoFar && Buffer[CountSoFar - 1] == L'\r') { >> - Buffer[CountSoFar - 1] = CHAR_NULL; >> - } >> + if ((CountSoFar+1)*sizeof(CHAR16) > *Size){ >> + *Size = (CountSoFar+1)*sizeof(CHAR16); >> + if (!Truncate) { >> + gEfiShellProtocol->SetFilePosition(Handle, OriginalFilePosition); >> + } else { >> + DEBUG((DEBUG_WARN, "The line was truncated in >> ShellFileHandleReadLine")); >> } >> + return (EFI_BUFFER_TOO_SMALL); >> + } >> + while(Buffer[StrLen(Buffer)-1] == L'\r') { >> + Buffer[StrLen(Buffer)-1] = CHAR_NULL; >> } >> >> return (Status); >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
