From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [65.50.211.136]) (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 2906221DFE907 for ; Wed, 9 Aug 2017 06:26:48 -0700 (PDT) Received: from [10.26.0.110] (corporativo.static.gvt.net.br [177.135.97.54] (may be forged)) (authenticated bits=0) by mail.zytor.com (8.15.2/8.15.2) with ESMTPSA id v79DQjUY015610 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 9 Aug 2017 06:26:48 -0700 To: "Ni, Ruiyu" , "Zeng, Star" , "edk2-devel@lists.01.org" Cc: Laszlo Ersek , "Justen, Jordan L" , Andrew Fish , "Kinney, Michael D" , "Gao, Liming" , "Dong, Eric" , "Doran, Mark" , "Wu, Hao A" References: <20170808193143.18128-1-pcacjr@zytor.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9132AF@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5B9DED45@SHSMSX104.ccr.corp.intel.com> From: Paulo Alcantara Message-ID: <50a39f82-576f-f0bb-c133-4b448c332ca5@zytor.com> Date: Wed, 9 Aug 2017 10:26:40 -0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5B9DED45@SHSMSX104.ccr.corp.intel.com> Subject: Re: [PATCH 0/4] read-only UDF file system support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Aug 2017 13:26:48 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Hi Ray, Thanks for the review. My comments below. On 8/9/2017 3:05 AM, Ni, Ruiyu wrote: > Paulo, > Thanks for enabling the UDF support into EDKII. > Below are several comments: > 1. Could you please separate the patch to modify MdePkg and MdeModulePkg? Sure. > 2. UDF_CDROM_VOLUME_IDENTIFIER is also defined in Eltorito.h as CDVOL_ID. > Maybe we could redefine CDVOL_ID to UDF_CDROM_VOLUME_IDENTIFIER? The Volume Identifier structure is defined in ECMA-167 specification (not UDF specific), so perhaps it would be better if we create a separate header file (e.g., Ecma-167.h) and define ECMA167_VOLUME_IDENTIFIER structure. That would be used in both ElTorito and UDF codes. What do you think? > > 2. Why do you need a PCD to control the UDF support? I prefer no. More choices > is not always good😊 The original idea of including this PCD was to avoid adding unnecessary overhead in PartitionDxe driver to something (UDF) that wasn't actually defined in UEFI specification -- leaving it as an optional feature. But yes, I agree with you that just complicates things and would be better to remove it. > > 3. Is gUdfVolumeSignatureGuid only used in Partition driver to produce the device > path? Can we just use HARDDRIVE_DEVICE_PATH? Or at least gUdfVolumeSignatureGuid > can be removed, the GUID macro is enough? > > 4. Have you verified on a CDROM which contains both ElTorito and CDFS? In the past I tested it with some Windows 8 and 10 ISO images I grabbed somewhere (both had ISO9660 & ElTorito's boot catalogs) but I haven't verified if it *still* works with them, yet. I'll do it when I get a chance. Thanks, Paulo