Jim,
I checked the other parts in your patch. They looks good.
But I don't quite understand how to handle the if-statement.
Do you mean to remove the below code block?
//
// Handle the degenerate case where Path was only a file system reference.
// In that case we return the current working directory of the file system.
//
if (InputPath == NULL) {
InputPath = L"";
}
If yes, I can remove it for you and push the patch.
Thanks/Ray
> -----Original Message-----
> From: jim.dai...@dell.com <jim.dai...@dell.com>
> Sent: Tuesday, October 30, 2018 7:32 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.car...@intel.com>
> Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-
> qualify paths
>
> >-----Original Message-----
> >From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
> >Sent: Tuesday, October 30, 2018 2:33 AM
> >To: Dailey, Jim; edk2-devel@lists.01.org
> >Cc: Carsey, Jaben
> >Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
> >fully-qualify paths
> >
> >
> >> -----Original Message-----
> >> From: jim.dai...@dell.com <jim.dai...@dell.com>
> >> Sent: Tuesday, October 30, 2018 5:15 AM
> >> To: edk2-devel@lists.01.org
> >> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ruiyu
> >> <ruiyu...@intel.com>
> >> Subject: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
> >> fully-qualify paths
> >>
> >> +CHAR16*
> >> +EFIAPI
> >> +FullyQualifyPath(
> >> + IN CONST CHAR16 *Path
> >> + )
> >> +{
> >> + CONST CHAR16 *WorkingPath;
> >> + CONST CHAR16 *InputPath;
> >> + CHAR16 *InputFileSystem;
> >> + UINTN FileSystemCharCount;
> >> + CHAR16 *FullyQualifiedPath;
> >> + UINTN Size;
> >> +
> >> + FullyQualifiedPath = NULL;
> >> +
> >> + ASSERT(Path != NULL);
> >> + //
> >> + // Handle erroneous input when asserts are disabled.
> >> + //
> >> + if (Path == NULL) {
> >> + return NULL;
> >> + }
> >> + //
> >> + // In paths that contain ":", like fs0:dir/file.ext and
> >> + fs2:\fqpath\file.ext, // we have to consider the file system part
> separately from the "path"
> >> part.
> >> + // If there is a file system in the path, we have to get the
> >> + current working // directory for that file system. Then we need to
> >> + use the part of the path // following the ":". If a path does not
> >> contain
> ":", we use it as given.
> >> + //
> >> + InputPath = StrStr(Path, L":");
> >> + if (InputPath != NULL) {
> >> + InputPath++;
> >> + FileSystemCharCount = ((UINTN)InputPath - (UINTN)Path +
> >> sizeof(CHAR16)) / sizeof(CHAR16);
> >> + InputFileSystem = AllocateCopyPool(FileSystemCharCount *
> >> sizeof(CHAR16), Path);
> >> + if (InputFileSystem != NULL) {
> >> + InputFileSystem[FileSystemCharCount - 1] = CHAR_NULL;
> >> + }
> >> + WorkingPath = ShellGetCurrentDir(InputFileSystem);
> >> + SHELL_FREE_NON_NULL(InputFileSystem);
> >> + //
> >> + // Handle the degenerate case where Path was only a file system
> >> reference.
> >> + // In that case we return the current working directory of the file
> system.
> >> + //
> >> + if (InputPath == NULL) {
> >
> >The "InputPath" should not be NULL.
>
> You are correct. It will simply point to an empty string if the input path
> is only
> a file system reference (e.g. "FS0:"). I thoughtlessly confused an empty
> string
> with NULL. :-(
>
> Do you want me to delete that comment and the "if" and resubmit, or,
> assuming the rest of the patch is acceptable, do you want to delete it and
> push the modified patch?
>
> Regards,
> Jim
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
