* [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
@ 2018-10-29 21:14 Jim.Dailey
2018-10-30 7:33 ` Ni, Ruiyu
0 siblings, 1 reply; 6+ messages in thread
From: Jim.Dailey @ 2018-10-29 21:14 UTC (permalink / raw)
To: edk2-devel; +Cc: jaben.carsey, ruiyu.ni
Add a function to return a clean, fully-qualified version of some path.
This function handles a (possibly "dirty") input path that may or may
not include a file system reference.
If it does not include a file system reference, then if the input path
does not begin with a forward or backward slash, then the input path is
relative to the current working directory of the current file system.
Otherwise, it is an absolute path within the current file system.
If it does include a file system reference, it may be a reference to the
current or some other file system. If the file system reference is not
immediately followed by a forward or backward slash, then the input path
is relative to the current working directory of the given file system.
Otherwise, it is an absolute path within the given file system.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey <jim_dailey@dell.com>
---
ShellPkg/Include/Library/ShellLib.h | 35 ++++++
ShellPkg/Library/UefiShellLib/UefiShellLib.c | 124 ++++++++++++++++++-
2 files changed, 158 insertions(+), 1 deletion(-)
diff --git a/ShellPkg/Include/Library/ShellLib.h b/ShellPkg/Include/Library/ShellLib.h
index 92fddc50f5..2ecc5ee006 100644
--- a/ShellPkg/Include/Library/ShellLib.h
+++ b/ShellPkg/Include/Library/ShellLib.h
@@ -2,6 +2,7 @@
Provides interface to shell functionality for shell commands and applications.
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+ Copyright 2018 Dell Technologies.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -35,6 +36,40 @@
extern EFI_SHELL_PARAMETERS_PROTOCOL *gEfiShellParametersProtocol;
extern EFI_SHELL_PROTOCOL *gEfiShellProtocol;
+/**
+ Return a clean, fully-qualified version of an input path. If the return value
+ is non-NULL the caller must free the memory when it is no longer needed.
+
+ If asserts are disabled, and if the input parameter is NULL, NULL is returned.
+
+ If there is not enough memory available to create the fully-qualified path or
+ a copy of the input path, NULL is returned.
+
+ If there is no working directory, a clean copy of Path is returned.
+
+ Otherwise, the current file system or working directory (as appropriate) is
+ prepended to Path and the resulting path is cleaned and returned.
+
+ NOTE: If the input path is an empty string, then the current working directory
+ (if it exists) is returned. In other words, an empty input path is treated
+ exactly the same as ".".
+
+ @param[in] Path A pointer to some file or directory path.
+
+ @retval NULL The input path is NULL or out of memory.
+
+ @retval non-NULL A pointer to a clean, fully-qualified version of Path.
+ If there is no working directory, then a pointer to a
+ clean, but not necessarily fully-qualified version of
+ Path. The caller must free this memory when it is no
+ longer needed.
+**/
+CHAR16*
+EFIAPI
+FullyQualifyPath(
+ IN CONST CHAR16 *Path
+ );
+
/**
This function will retrieve the information about the file for the handle
specified and store it in allocated pool memory.
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index f04adbb63f..8467eb8953 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -2,7 +2,7 @@
Provides interface to shell functionality for shell commands and applications.
(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
- Copyright 2016 Dell Inc.
+ Copyright 2016-2018 Dell Technologies.<BR>
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
@@ -36,6 +36,128 @@ EFI_HANDLE mEfiShellEnvironment2Handle;
FILE_HANDLE_FUNCTION_MAP FileFunctionMap;
EFI_UNICODE_COLLATION_PROTOCOL *mUnicodeCollationProtocol;
+/**
+ Return a clean, fully-qualified version of an input path. If the return value
+ is non-NULL the caller must free the memory when it is no longer needed.
+
+ If asserts are disabled, and if the input parameter is NULL, NULL is returned.
+
+ If there is not enough memory available to create the fully-qualified path or
+ a copy of the input path, NULL is returned.
+
+ If there is no working directory, a clean copy of Path is returned.
+
+ Otherwise, the current file system or working directory (as appropriate) is
+ prepended to Path and the resulting path is cleaned and returned.
+
+ NOTE: If the input path is an empty string, then the current working directory
+ (if it exists) is returned. In other words, an empty input path is treated
+ exactly the same as ".".
+
+ @param[in] Path A pointer to some file or directory path.
+
+ @retval NULL The input path is NULL or out of memory.
+
+ @retval non-NULL A pointer to a clean, fully-qualified version of Path.
+ If there is no working directory, then a pointer to a
+ clean, but not necessarily fully-qualified version of
+ Path. The caller must free this memory when it is no
+ longer needed.
+**/
+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) {
+ InputPath = L"";
+ }
+ } else {
+ InputPath = Path;
+ WorkingPath = ShellGetEnvironmentVariable(L"cwd");
+ }
+
+ if (WorkingPath == NULL) {
+ //
+ // With no working directory, all we can do is copy and clean the input path.
+ //
+ FullyQualifiedPath = AllocateCopyPool(StrSize(Path), Path);
+ } else {
+ //
+ // Allocate space for both strings plus one more character.
+ //
+ Size = StrSize(WorkingPath) + StrSize(InputPath);
+ FullyQualifiedPath = AllocateZeroPool(Size);
+ if (FullyQualifiedPath == NULL) {
+ //
+ // Try to copy and clean just the input. No harm if not enough memory.
+ //
+ FullyQualifiedPath = AllocateCopyPool(StrSize(Path), Path);
+ } else {
+ if (*InputPath == L'\\' || *InputPath == L'/') {
+ //
+ // Absolute path: start with the current working directory, then
+ // truncate the new path after the file system part.
+ //
+ StrCpyS(FullyQualifiedPath, Size/sizeof(CHAR16), WorkingPath);
+ *(StrStr(FullyQualifiedPath, L":") + 1) = CHAR_NULL;
+ } else {
+ //
+ // Relative path: start with the working directory and append "\".
+ //
+ StrCpyS(FullyQualifiedPath, Size/sizeof(CHAR16), WorkingPath);
+ StrCatS(FullyQualifiedPath, Size/sizeof(CHAR16), L"\\");
+ }
+ //
+ // Now append the absolute or relative path.
+ //
+ StrCatS(FullyQualifiedPath, Size/sizeof(CHAR16), InputPath);
+ }
+ }
+
+ PathCleanUpDirectories(FullyQualifiedPath);
+
+ return FullyQualifiedPath;
+}
+
/**
Check if a Unicode character is a hexadecimal character.
--
2.17.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
2018-10-29 21:14 [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths Jim.Dailey
@ 2018-10-30 7:33 ` Ni, Ruiyu
2018-10-30 11:32 ` Jim.Dailey
0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2018-10-30 7:33 UTC (permalink / raw)
To: Jim.Dailey@dell.com, edk2-devel@lists.01.org; +Cc: Carsey, Jaben
> -----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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
2018-10-30 7:33 ` Ni, Ruiyu
@ 2018-10-30 11:32 ` Jim.Dailey
2018-10-31 9:17 ` Ni, Ruiyu
0 siblings, 1 reply; 6+ messages in thread
From: Jim.Dailey @ 2018-10-30 11:32 UTC (permalink / raw)
To: ruiyu.ni, edk2-devel; +Cc: jaben.carsey
>-----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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
2018-10-30 11:32 ` Jim.Dailey
@ 2018-10-31 9:17 ` Ni, Ruiyu
2018-10-31 11:27 ` Jim.Dailey
0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2018-10-31 9:17 UTC (permalink / raw)
To: Jim.Dailey@dell.com, edk2-devel@lists.01.org; +Cc: Carsey, Jaben
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
2018-10-31 9:17 ` Ni, Ruiyu
@ 2018-10-31 11:27 ` Jim.Dailey
2018-11-05 2:59 ` Ni, Ruiyu
0 siblings, 1 reply; 6+ messages in thread
From: Jim.Dailey @ 2018-10-31 11:27 UTC (permalink / raw)
To: ruiyu.ni; +Cc: jaben.carsey, edk2-devel
Yes, Ray, that is the code block that I meant. It serves no purpose since
InputPath cannot possibly be NULL in that part of the code.
I appreciate your help in deleting this block and pushing the rest of the
patch. Also, thanks for your thorough review of this code (and the first
version)!
Regards,
Jim
-----Original Message-----
From: Ni, Ruiyu [mailto:ruiyu.ni@intel.com]
Sent: Wednesday, October 31, 2018 4:18 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
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
2018-10-31 11:27 ` Jim.Dailey
@ 2018-11-05 2:59 ` Ni, Ruiyu
0 siblings, 0 replies; 6+ messages in thread
From: Ni, Ruiyu @ 2018-11-05 2:59 UTC (permalink / raw)
To: Jim.Dailey@dell.com; +Cc: Carsey, Jaben, edk2-devel@lists.01.org
Pushed. Both patches.
Thanks/Ray
> -----Original Message-----
> From: Jim.Dailey@dell.com <Jim.Dailey@dell.com>
> Sent: Wednesday, October 31, 2018 7:28 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-
> qualify paths
>
> Yes, Ray, that is the code block that I meant. It serves no purpose since
> InputPath cannot possibly be NULL in that part of the code.
>
> I appreciate your help in deleting this block and pushing the rest of the patch.
> Also, thanks for your thorough review of this code (and the first version)!
>
> Regards,
> Jim
>
> -----Original Message-----
> From: Ni, Ruiyu [mailto:ruiyu.ni@intel.com]
> Sent: Wednesday, October 31, 2018 4:18 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
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-11-05 2:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-29 21:14 [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths Jim.Dailey
2018-10-30 7:33 ` Ni, Ruiyu
2018-10-30 11:32 ` Jim.Dailey
2018-10-31 9:17 ` Ni, Ruiyu
2018-10-31 11:27 ` Jim.Dailey
2018-11-05 2:59 ` Ni, Ruiyu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox