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 8DCB721BADAB2 for ; Fri, 27 Jul 2018 05:06:16 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D38E84219DA0; Fri, 27 Jul 2018 12:06:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-66.rdu2.redhat.com [10.10.121.66]) by smtp.corp.redhat.com (Postfix) with ESMTP id D62557C27; Fri, 27 Jul 2018 12:06:13 +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> From: Laszlo Ersek Message-ID: <9bb5eed1-f8b0-8d22-e801-53ba7a06cdc5@redhat.com> Date: Fri, 27 Jul 2018 14:06:08 +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: <425d0b30-a322-23c7-6d56-8f23696b86cd@Intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 27 Jul 2018 12:06:15 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 27 Jul 2018 12:06:15 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.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: Fri, 27 Jul 2018 12:06:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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. > >> + >> +    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? > >> + >> +    // >> +    // 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. // Thanks! Laszlo > >> +  return Status; >> +} >> > >