Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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.Dailey@dell.com <Jim.Dailey@dell.com>
> Sent: Tuesday, October 30, 2018 7:32 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-
> qualify paths
> 
> >-----Original Message-----
> >From: Ni, Ruiyu [mailto:ruiyu.ni@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.Dailey@dell.com <Jim.Dailey@dell.com>
> >> Sent: Tuesday, October 30, 2018 5:15 AM
> >> To: edk2-devel@lists.01.org
> >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu
> >> <ruiyu.ni@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