From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8B14D210C2D67 for ; Mon, 30 Jul 2018 07:13:59 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E3DB587A87; Mon, 30 Jul 2018 14:13:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-238.rdu2.redhat.com [10.10.120.238]) by smtp.corp.redhat.com (Postfix) with ESMTP id E9A221121301; Mon, 30 Jul 2018 14:13:55 +0000 (UTC) To: "Ni, Ruiyu" , edk2-devel-01 Cc: Chao Zhang , Eric Dong , Jaben Carsey , Jiaxin Wu , Jiewen Yao , Liming Gao , Michael D Kinney , Roman Bacik , Siyuan Fu , Star Zeng References: <20180718205043.17574-1-lersek@redhat.com> <20180718205043.17574-2-lersek@redhat.com> <425d0b30-a322-23c7-6d56-8f23696b86cd@Intel.com> <9bb5eed1-f8b0-8d22-e801-53ba7a06cdc5@redhat.com> <57ac2b07-41fc-d01d-e20b-9be4a68a0f1b@Intel.com> From: Laszlo Ersek Message-ID: <4a445d20-4216-545d-cc2e-ee21bdb90925@redhat.com> Date: Mon, 30 Jul 2018 16:13:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <57ac2b07-41fc-d01d-e20b-9be4a68a0f1b@Intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Mon, 30 Jul 2018 14:13:59 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Mon, 30 Jul 2018 14:13:59 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Jul 2018 14:14:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 07/30/18 03:54, Ni, Ruiyu wrote: > On 7/27/2018 8:06 PM, Laszlo Ersek wrote: >> On 07/27/18 11:28, Ni, Ruiyu wrote: >>> On 7/19/2018 4:50 AM, Laszlo Ersek wrote: >>> >>>> +  // >>>> +  // Traverse the device path nodes relative to the filesystem. >>>> +  // >>>> +  while (!IsDevicePathEnd (*FilePath)) { >>>> +    // >>>> +    // Keep local variables that relate to the current device path >>>> node tightly >>>> +    // scoped. >>>> +    // >>>> +    FILEPATH_DEVICE_PATH *FilePathNode; >>>> +    CHAR16               *AlignedPathName; >>>> +    CHAR16               *PathName; >>>> +    EFI_FILE_PROTOCOL    *NextFile; >>> 1. Not sure if it follows the coding style. I would prefer to move the >>> definition to the beginning of the function. >> >> OK, will do. > > Thanks! > >> >>> >>>> + >>>> +    if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH || >>>> +        DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) { >>>> +      Status = EFI_INVALID_PARAMETER; >>>> +      goto CloseLastFile; >>>> +    } >>>> +    FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath; >>>> + >>>> +    // >>>> +    // FilePathNode->PathName may be unaligned, and the UEFI >>>> specification >>>> +    // requires pointers that are passed to protocol member functions >>>> to be >>>> +    // aligned. Create an aligned copy of the pathname if necessary. >>>> +    // >>>> +    if ((UINTN)FilePathNode->PathName % sizeof >>>> *FilePathNode->PathName == 0) { >>>> +      AlignedPathName = NULL; >>>> +      PathName = FilePathNode->PathName; >>>> +    } else { >>>> +      AlignedPathName = AllocateCopyPool ( >>>> +                          (DevicePathNodeLength (FilePathNode) - >>>> +                           SIZE_OF_FILEPATH_DEVICE_PATH), >>>> +                          FilePathNode->PathName >>>> +                          ); >>>> +      if (AlignedPathName == NULL) { >>>> +        Status = EFI_OUT_OF_RESOURCES; >>>> +        goto CloseLastFile; >>>> +      } >>>> +      PathName = AlignedPathName; >>>> +    } >>>> + >>>> +    // >>>> +    // Open the next pathname fragment with EFI_FILE_MODE_CREATE >>>> masked out and >>>> +    // with Attributes set to 0. >>>> +    // >>>> +    Status = LastFile->Open ( >>>> +                         LastFile, >>>> +                         &NextFile, >>>> +                         PathName, >>>> +                         OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE, >>>> +                         0 >>>> +                         ); >>> 2. As I said in previous mail, is it really needed? >>> Per spec it's not required. Per FAT driver implementation, it's also not >>> required. >> >> I can do that, but it's out of scope for this series. The behavior that >> you point out is not a functionality bug (it is not observably erroneous >> behavior), just sub-optimal implementation. This series is about >> unifying the implementation and fixing those issues that are actual >> bugs. I suggest that we report a separate BZ about this question, >> discuss it separately, and then I can send a separate patch (which will >> benefit all client code at once). >> >> Does that sound acceptable? > > I agree. > >> >>> >>>> + >>>> +    // >>>> +    // Retry with EFI_FILE_MODE_CREATE and the original Attributes if >>>> the first >>>> +    // attempt failed, and the caller specified EFI_FILE_MODE_CREATE. >>>> +    // >>>> +    if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != >>>> 0) { >>>> +      Status = LastFile->Open ( >>>> +                           LastFile, >>>> +                           &NextFile, >>>> +                           PathName, >>>> +                           OpenMode, >>>> +                           Attributes >>>> +                           ); >>>> +    } >>>> + >>>> +    // >>>> +    // Release any AlignedPathName on both error and success paths; >>>> PathName is >>>> +    // no longer needed. >>>> +    // >>>> +    if (AlignedPathName != NULL) { >>>> +      FreePool (AlignedPathName); >>>> +    } >>>> +    if (EFI_ERROR (Status)) { >>>> +      goto CloseLastFile; >>>> +    } >>>> + >>>> +    // >>>> +    // Advance to the next device path node. >>>> +    // >>>> +    LastFile->Close (LastFile); >>>> +    LastFile = NextFile; >>>> +    *FilePath = NextDevicePathNode (FilePathNode); >>>> +  } >>>> + >>>> +  *File = LastFile; >>>> +  return EFI_SUCCESS; >>>> + >>>> +CloseLastFile: >>>> +  LastFile->Close (LastFile); >>>> + >>>> +  ASSERT (EFI_ERROR (Status)); >>> 3. ASSERT_EFI_ERROR (Status); >> >> No, that's not correct; I *really* meant >> >>    ASSERT (EFI_ERROR (Status)) >> >> Please find the explanation here: >> >> https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html >> >> However, given that both Jaben and you were confused by this, I agree >> that I should add a comment before the assert: >> >>    // >>    // We are on the error path; we must have set an error Status for >>    // returning to the caller. >>    // > > I just found there is no "!" before "EFI_ERROR". > It's really confusing. I agree a comment before that is better. > Thanks! > > With the comment added, Reviewed-by: Ruiyu Ni Thanks, Ray -- looks like I've almost got enough feedback for posting v2; however I haven't received any MdePkg maintainer feedback (from Mike and/or Liming) yet. Am I to understand your review as a substitute for theirs, or as an addition to theirs? Thanks! Laszlo