public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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


  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