From: <Jim.Dailey@dell.com>
To: <ruiyu.ni@intel.com>
Cc: <jaben.carsey@intel.com>, <edk2-devel@lists.01.org>
Subject: Re: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
Date: Wed, 31 Oct 2018 11:27:31 +0000 [thread overview]
Message-ID: <063b9052d04648e2890f0f95232929df@ausx13mps335.AMER.DELL.COM> (raw)
In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BEE8E87@SHSMSX104.ccr.corp.intel.com>
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
next prev parent reply other threads:[~2018-10-31 11:27 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2018-11-05 2:59 ` Ni, Ruiyu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=063b9052d04648e2890f0f95232929df@ausx13mps335.AMER.DELL.COM \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox