On 02/10/16 20:37, Ryan Harkin wrote:
> On 10 February 2016 at 14:16, Ryan Harkin <ryan.har...@linaro.org> 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, <jim_dai...@dell.com> 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 <ryan.har...@linaro.org>
>>
>
> 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 :(
You should be getting worried about the workflow. The forking and the
merges were unintended. It's growing pains after the move to git.
I'll do my best to remedy the damage.
Thanks
Laszlo
>
> 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
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
