| Message ID | 1406672792-7151-3-git-send-email-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | 5967886d58e4ac7d46e0c6b7cc34fd9ba94fd6d1 |
| Headers | show |
On 07/30/14 00:51, Carsey, Jaben wrote: > If you write to the file and the original size was zero, don't you need to update the file size before moving to that location? The FileSize variable is not updated in the WriteFileTag() branch because FileSize is not used at any later point. In addition, if FileSize is zero originally, we don't manually move to that location (the SetFilePosition() branch of the ternary operator is not reached); we just rely on WriteFileTag() to increment the file position, which happens internally to WriteFileTag() --> WriteFile(). See EFI_SHELL_WRITE_FILE [ShellPkg/Include/Protocol/EfiShell.h]: "The current file position is advanced the actual number of bytes written". ... At least I hope I understood your question correctly. Thanks, Laszlo > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, July 29, 2014 3:27 PM > To: edk2-devel@lists.sourceforge.net; Lowell_Dennis@Dell.com > Subject: [edk2] [PATCH 2/2] ShellPkg: UpdateStdInStdOutStdErr(): append BOM to new unicode file > > The >> operator redirects stdout to a file, using append mode and unicode encoding. Write the BOM when redirection happens to a new file (which starts out empty). > > This makes the >> operator behave similarly to the > operator, when the redirection target doesn't exist originally: > > OutUnicode && OutAppend && FileSize == 0 // >> to new unicode file vs. > OutUnicode && !OutAppend // > to any unicode file > > (Note that (FileSize == 0) is equivalent to "new file" in this context, due to the earlier "Check that filetypes (Unicode/Ascii) do not change during an append".) > > Reported-by: Lowell Dennis <Lowell_Dennis@Dell.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > ShellPkg/Application/Shell/ShellParametersProtocol.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c > index 88453a2..9d76954 100644 > --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c > +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c > @@ -1111,12 +1111,18 @@ UpdateStdInStdOutStdErr( > } else if (!OutAppend && OutUnicode && !EFI_ERROR(Status)) { > Status = WriteFileTag (TempHandle); > } else if (OutAppend) { > - // > - // Move to end of file > - // > Status = ShellInfoObject.NewEfiShellProtocol->GetFileSize(TempHandle, &FileSize); > if (!EFI_ERROR(Status)) { > - Status = ShellInfoObject.NewEfiShellProtocol->SetFilePosition(TempHandle, FileSize); > + // > + // When appending to a new unicode file, write the file tag. > + // Otherwise (ie. when appending to a new ASCII file, or an > + // existent file with any encoding), just seek to the end. > + // > + Status = (FileSize == 0 && OutUnicode) ? > + WriteFileTag (TempHandle) : > + ShellInfoObject.NewEfiShellProtocol->SetFilePosition ( > + TempHandle, > + > + FileSize); > } > } > if (!OutUnicode && !EFI_ERROR(Status)) { > -- > 1.8.3.1 > > > ------------------------------------------------------------------------------ > Infragistics Professional > Build stunning WinForms apps today! > Reboot your WinForms applications with our WinForms controls. > Build a bridge from your legacy apps to the future. > http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------ Infragistics Professional Build stunning WinForms apps today! Reboot your WinForms applications with our WinForms controls. Build a bridge from your legacy apps to the future. http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
Good point. I missed the ?: operation in the patch. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Tuesday, July 29, 2014 4:13 PM To: Carsey, Jaben; edk2-devel@lists.sourceforge.net; Lowell_Dennis@Dell.com Subject: Re: [edk2] [PATCH 2/2] ShellPkg: UpdateStdInStdOutStdErr(): append BOM to new unicode file Importance: High On 07/30/14 00:51, Carsey, Jaben wrote: > If you write to the file and the original size was zero, don't you need to update the file size before moving to that location? The FileSize variable is not updated in the WriteFileTag() branch because FileSize is not used at any later point. In addition, if FileSize is zero originally, we don't manually move to that location (the SetFilePosition() branch of the ternary operator is not reached); we just rely on WriteFileTag() to increment the file position, which happens internally to WriteFileTag() --> WriteFile(). See EFI_SHELL_WRITE_FILE [ShellPkg/Include/Protocol/EfiShell.h]: "The current file position is advanced the actual number of bytes written". ... At least I hope I understood your question correctly. Thanks, Laszlo > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, July 29, 2014 3:27 PM > To: edk2-devel@lists.sourceforge.net; Lowell_Dennis@Dell.com > Subject: [edk2] [PATCH 2/2] ShellPkg: UpdateStdInStdOutStdErr(): append BOM to new unicode file > > The >> operator redirects stdout to a file, using append mode and unicode encoding. Write the BOM when redirection happens to a new file (which starts out empty). > > This makes the >> operator behave similarly to the > operator, when the redirection target doesn't exist originally: > > OutUnicode && OutAppend && FileSize == 0 // >> to new unicode file vs. > OutUnicode && !OutAppend // > to any unicode file > > (Note that (FileSize == 0) is equivalent to "new file" in this context, due to the earlier "Check that filetypes (Unicode/Ascii) do not change during an append".) > > Reported-by: Lowell Dennis <Lowell_Dennis@Dell.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > ShellPkg/Application/Shell/ShellParametersProtocol.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c > index 88453a2..9d76954 100644 > --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c > +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c > @@ -1111,12 +1111,18 @@ UpdateStdInStdOutStdErr( > } else if (!OutAppend && OutUnicode && !EFI_ERROR(Status)) { > Status = WriteFileTag (TempHandle); > } else if (OutAppend) { > - // > - // Move to end of file > - // > Status = ShellInfoObject.NewEfiShellProtocol->GetFileSize(TempHandle, &FileSize); > if (!EFI_ERROR(Status)) { > - Status = ShellInfoObject.NewEfiShellProtocol->SetFilePosition(TempHandle, FileSize); > + // > + // When appending to a new unicode file, write the file tag. > + // Otherwise (ie. when appending to a new ASCII file, or an > + // existent file with any encoding), just seek to the end. > + // > + Status = (FileSize == 0 && OutUnicode) ? > + WriteFileTag (TempHandle) : > + ShellInfoObject.NewEfiShellProtocol->SetFilePosition ( > + TempHandle, > + > + FileSize); > } > } > if (!OutUnicode && !EFI_ERROR(Status)) { > -- > 1.8.3.1 > > > ------------------------------------------------------------------------------ > Infragistics Professional > Build stunning WinForms apps today! > Reboot your WinForms applications with our WinForms controls. > Build a bridge from your legacy apps to the future. > http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------ Infragistics Professional Build stunning WinForms apps today! Reboot your WinForms applications with our WinForms controls. Build a bridge from your legacy apps to the future. http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
On 07/30/14 01:16, Carsey, Jaben wrote: > Good point. I missed the ?: operation in the patch. > Thanks. Is that an R-b? :) Laszlo ------------------------------------------------------------------------------ Infragistics Professional Build stunning WinForms apps today! Reboot your WinForms applications with our WinForms controls. Build a bridge from your legacy apps to the future. http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
Reviewed-by: Jaben Carsey <Jaben.carsey@intel.com> -----Original Message----- From: Carsey, Jaben Sent: Tuesday, July 29, 2014 4:16 PM To: Laszlo Ersek; edk2-devel@lists.sourceforge.net; Lowell_Dennis@Dell.com Cc: Carsey, Jaben Subject: RE: [edk2] [PATCH 2/2] ShellPkg: UpdateStdInStdOutStdErr(): append BOM to new unicode file Good point. I missed the ?: operation in the patch. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Tuesday, July 29, 2014 4:13 PM To: Carsey, Jaben; edk2-devel@lists.sourceforge.net; Lowell_Dennis@Dell.com Subject: Re: [edk2] [PATCH 2/2] ShellPkg: UpdateStdInStdOutStdErr(): append BOM to new unicode file Importance: High On 07/30/14 00:51, Carsey, Jaben wrote: > If you write to the file and the original size was zero, don't you need to update the file size before moving to that location? The FileSize variable is not updated in the WriteFileTag() branch because FileSize is not used at any later point. In addition, if FileSize is zero originally, we don't manually move to that location (the SetFilePosition() branch of the ternary operator is not reached); we just rely on WriteFileTag() to increment the file position, which happens internally to WriteFileTag() --> WriteFile(). See EFI_SHELL_WRITE_FILE [ShellPkg/Include/Protocol/EfiShell.h]: "The current file position is advanced the actual number of bytes written". ... At least I hope I understood your question correctly. Thanks, Laszlo > > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, July 29, 2014 3:27 PM > To: edk2-devel@lists.sourceforge.net; Lowell_Dennis@Dell.com > Subject: [edk2] [PATCH 2/2] ShellPkg: UpdateStdInStdOutStdErr(): > append BOM to new unicode file > > The >> operator redirects stdout to a file, using append mode and unicode encoding. Write the BOM when redirection happens to a new file (which starts out empty). > > This makes the >> operator behave similarly to the > operator, when the redirection target doesn't exist originally: > > OutUnicode && OutAppend && FileSize == 0 // >> to new unicode file vs. > OutUnicode && !OutAppend // > to any unicode file > > (Note that (FileSize == 0) is equivalent to "new file" in this > context, due to the earlier "Check that filetypes (Unicode/Ascii) do > not change during an append".) > > Reported-by: Lowell Dennis <Lowell_Dennis@Dell.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > ShellPkg/Application/Shell/ShellParametersProtocol.c | 14 > ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c > b/ShellPkg/Application/Shell/ShellParametersProtocol.c > index 88453a2..9d76954 100644 > --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c > +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c > @@ -1111,12 +1111,18 @@ UpdateStdInStdOutStdErr( > } else if (!OutAppend && OutUnicode && !EFI_ERROR(Status)) { > Status = WriteFileTag (TempHandle); > } else if (OutAppend) { > - // > - // Move to end of file > - // > Status = ShellInfoObject.NewEfiShellProtocol->GetFileSize(TempHandle, &FileSize); > if (!EFI_ERROR(Status)) { > - Status = ShellInfoObject.NewEfiShellProtocol->SetFilePosition(TempHandle, FileSize); > + // > + // When appending to a new unicode file, write the file tag. > + // Otherwise (ie. when appending to a new ASCII file, or an > + // existent file with any encoding), just seek to the end. > + // > + Status = (FileSize == 0 && OutUnicode) ? > + WriteFileTag (TempHandle) : > + ShellInfoObject.NewEfiShellProtocol->SetFilePosition ( > + > + TempHandle, > + > + FileSize); > } > } > if (!OutUnicode && !EFI_ERROR(Status)) { > -- > 1.8.3.1 > > > ---------------------------------------------------------------------- > -------- > Infragistics Professional > Build stunning WinForms apps today! > Reboot your WinForms applications with our WinForms controls. > Build a bridge from your legacy apps to the future. > http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg. > clktrk _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > ------------------------------------------------------------------------------ Infragistics Professional Build stunning WinForms apps today! Reboot your WinForms applications with our WinForms controls. Build a bridge from your legacy apps to the future. http://pubads.g.doubleclick.net/gampad/clk?id=153845071&iu=/4140/ostg.clktrk
diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c index 88453a2..9d76954 100644 --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c @@ -1111,12 +1111,18 @@ UpdateStdInStdOutStdErr( } else if (!OutAppend && OutUnicode && !EFI_ERROR(Status)) { Status = WriteFileTag (TempHandle); } else if (OutAppend) { - // - // Move to end of file - // Status = ShellInfoObject.NewEfiShellProtocol->GetFileSize(TempHandle, &FileSize); if (!EFI_ERROR(Status)) { - Status = ShellInfoObject.NewEfiShellProtocol->SetFilePosition(TempHandle, FileSize); + // + // When appending to a new unicode file, write the file tag. + // Otherwise (ie. when appending to a new ASCII file, or an + // existent file with any encoding), just seek to the end. + // + Status = (FileSize == 0 && OutUnicode) ? + WriteFileTag (TempHandle) : + ShellInfoObject.NewEfiShellProtocol->SetFilePosition ( + TempHandle, + FileSize); } } if (!OutUnicode && !EFI_ERROR(Status)) {
The >> operator redirects stdout to a file, using append mode and unicode encoding. Write the BOM when redirection happens to a new file (which starts out empty). This makes the >> operator behave similarly to the > operator, when the redirection target doesn't exist originally: OutUnicode && OutAppend && FileSize == 0 // >> to new unicode file vs. OutUnicode && !OutAppend // > to any unicode file (Note that (FileSize == 0) is equivalent to "new file" in this context, due to the earlier "Check that filetypes (Unicode/Ascii) do not change during an append".) Reported-by: Lowell Dennis <Lowell_Dennis@Dell.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- ShellPkg/Application/Shell/ShellParametersProtocol.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)