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

[<Jim.Dailey@dell.com>]

>-----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