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 20359210D7F52 for ; Thu, 2 Aug 2018 07:46:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 52FDACFB7F; Thu, 2 Aug 2018 14:46:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-32.rdu2.redhat.com [10.10.123.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6CBD22166BA2; Thu, 2 Aug 2018 14:45:58 +0000 (UTC) To: "Gao, Liming" , "Ni, Ruiyu" , edk2-devel-01 Cc: "Dong, Eric" , "Kinney, Michael D" , "Wu, Jiaxin" , "Yao, Jiewen" , "Zeng, Star" , "Carsey, Jaben" , "Fu, Siyuan" , "Zhang, Chao B" 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> <4a445d20-4216-545d-cc2e-ee21bdb90925@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E2C466F@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <8b599780-2a7f-c438-557d-385d867cfa61@redhat.com> Date: Thu, 2 Aug 2018 16:45:57 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14E2C466F@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 02 Aug 2018 14:46:00 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 02 Aug 2018 14:46:00 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.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: Thu, 02 Aug 2018 14:46:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 08/02/18 06:06, Gao, Liming wrote: > Laszlo: > I have no other comments. IntelFrameworkPkg has another UefiLib library instance FrameworkUefiLib. Could you help update it also? > > For MdePkg, you can add Reviewed-by: Liming Gao Thanks! I have now enough feedback for posting v2. Cheers Laszlo > > Thanks > Liming >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, July 30, 2018 10:14 PM >> To: Ni, Ruiyu ; edk2-devel-01 >> Cc: Zhang, Chao B ; Dong, Eric >> ; Carsey, Jaben ; Wu, Jiaxin >> ; Yao, Jiewen ; Gao, Liming >> ; Kinney, Michael D ; >> Roman Bacik ; Fu, Siyuan >> ; Zeng, Star >> Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce >> EfiOpenFileByDevicePath() >> >> 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 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >