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 103B421E49BA2 for ; Tue, 22 Aug 2017 06:14:42 -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 v7MDErxe022543 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 22 Aug 2017 06:14:55 -0700 To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Cc: Laszlo Ersek , "Justen, Jordan L" , Andrew Fish , "Kinney, Michael D" , "Gao, Liming" , "Zeng, Star" , "Dong, Eric" , "Doran, Mark" , "Wu, Hao A" References: <20170820181557.28761-1-pcacjr@zytor.com> <734D49CCEBEEF84792F5B80ED585239D5B9F3CD8@SHSMSX104.ccr.corp.intel.com> From: Paulo Alcantara Message-ID: Date: Tue, 22 Aug 2017 10:14:48 -0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5B9F3CD8@SHSMSX104.ccr.corp.intel.com> Subject: Re: [PATCH v2 0/6] 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: Tue, 22 Aug 2017 13:14:42 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, I do apologize my late replies. At the moment, I'm only able to work on this during my free time. Thank you all for the reviews! FWIW, my comments below. On 8/20/2017 11:29 PM, Ni, Ruiyu wrote: > Paulo, > 1. Could you please run the ECC check tool (BaseTools\Source\Python\Ecc\) > "CRC" might need to be replaced with "Crc". > I also noticed some TAB key in file content. Sure. > > 2. Your current implementation uses HARD_DRIVE_DEVICE_PATH. > But with: > SignatureType = SIGNATURE_TYPE_UDF > MBRType = MBR_TYPE_PCAT > Signature = * > And later UdfDxe driver checks the SignatureType and MBRType. > I am not sure if it would be better to put the definitions in UEFI Spec, > since they are referenced by different modules. > I also noticed you use PARTITION_TYPE_OTHER for PartitionInfo. > When proposing to UEFI Spec, this also needs to be considered, > for example, add PARTITION_TYPE_UDF to spec. Yes - I agree with you. My only concern is that UEFI specification doesn't either support UDF or there is any interest in supporting it, so by proposing an additional type for something that shouldn't be supported, might not work out. (Andrew, any thoughts?) "PARTITION_TYPE_OTHER" is also used when creating EFI_PARTITION_INFO_PROTOCOL for CDROM "El Torito" partitions, so either we could follow the same approach, or propose creating two other types: e.g., PARTITION_TYPE_ELTORITO and PARTITION_TYPE_UDF. > > 3. The driver model part looks good. Cool! Thanks for looking into that. Paulo