From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.12813.1603113182383599076 for ; Mon, 19 Oct 2020 06:13:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SZBlcud4; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1603113181; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nWbDYA62DAncg2jUScrrn/E7YGTo+NNwwv6fQE/iyPM=; b=SZBlcud4PksxTa/uXgmIgR/ms3wyxy6g0dm60pC3/FQnDcMTnuLMtQkAwgU2aJ5d2CCMsN vjl+F/XH+0llxNc/RG0vuS9VUAac2I7E8Dr28fad2noYSFn3qOtfjzGGnyWa1YFzreDMSY QXAHfBxxsitsJkowwCwp/OkGRzK4Lpc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-179-_Ku6Gek6NBmSu1B4pITUrQ-1; Mon, 19 Oct 2020 09:12:55 -0400 X-MC-Unique: _Ku6Gek6NBmSu1B4pITUrQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 59543107B0EF; Mon, 19 Oct 2020 13:12:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-119.ams2.redhat.com [10.36.113.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id A691860C13; Mon, 19 Oct 2020 13:12:52 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change To: "Gao, Zhichao" , "Ni, Ray" , "devel@edk2.groups.io" , "glin@suse.com" Cc: "Wu, Hao A" References: <20201012072230.46152-1-zhichao.gao@intel.com> <4f20aa01-793a-8477-53c1-56a5899caa14@redhat.com> <20201016064205.GI19552@GaryWorkstation> From: "Laszlo Ersek" Message-ID: Date: Mon, 19 Oct 2020 15:12:51 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/19/20 11:35, Gao, Zhichao wrote: > Yes. But I need to confirm the comment #3. The directly revert would > make the title of the commit message larger which may be larger than > 72 chars. That is another reason I didn't use revert directly. If the desired action is to undo (roll back / revert) a specific earlier commit, then git-revert is always the right tool to *start with*. Then git-revert either works, or it doesn't: - if git-revert completes at once, code-wise, then the commit message is supposed to be edited manually, in every case. The commit message prepared by git-revert is just a template. What's important is that the subject of the revert patch clearly reflect (a) the subject of the original patch, and (b) the fact that we're undoing an earlier commit. Additionally, the commit message body should clearly identify the commit hash of the patch being rolled back. Beyond that, the author of the revert patch is welcome to extend the commit message with as many details as necessary. - git-revert might find that, due to changes committed in the meantime, it cannot just undo the original commit (i.e., the inverse diff doesn't apply cleanly, and cannot be fixed up automatically). In that case, manual code editing is necessary. This can mean a successful manual conflict resolution; after wich git-revert counts as "successful", so see the previous paragraph. - Failure of git-revert can also necessitate entirely new, manual coding, for rolling back the original patch. Semantically, the situation stays the same, thus the same information as in the previous bullets should be included in the commit message. The formal requirements are a bit looser, because git-revert never completed, and so it didn't give us a commit message template to extend. It's still preferred to say something like "Undo ''" in the subject line, and to include the original commit hash in the commit message body. If git-revert completes, but proposes an overlong subject (according to PatchCheck.py), then personally I prefer truncating the original, quoted subject, with an ellipsis (...). In this particular case, "git revert e0eacd7daa6f" seems to work without manual code editing. The subject line it creates is 3 characters too long. Editing that to the following variant: Revert "MdeModulePkg/PartitionDxe: Fix the incorrect LBA size in child ..." works OK. Summary: (1) always start with git-revert please, (2) try to stick with the subject pattern that git-revert creates; truncate as needed, (3) *always* name the commit being reverted by commit hash, in the new commit message body, (4) append as much information as you see fit, to the commit message body. Thanks Laszlo > > Thanks, > Zhichao > >> -----Original Message----- >> From: Ni, Ray >> Sent: Monday, October 19, 2020 1:56 PM >> To: Gao, Zhichao ; devel@edk2.groups.io; >> glin@suse.com; Laszlo Ersek >> Cc: Wu, Hao A >> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child >> handler blocksize change >> >> Zhichao, >> Can you please update the commit message to address Laszlo's comments? >> >> Thanks, >> Ray >> >>> -----Original Message----- >>> From: Gao, Zhichao >>> Sent: Monday, October 19, 2020 10:46 AM >>> To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek >>> >>> Cc: Ni, Ray ; Wu, Hao A >>> Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert >>> the child handler blocksize change >>> >>> Thanks Gary for your test. I have give my comments base on Laszlo's >>> reply. I don't think the regression would affect Linux ISO image >>> except there is one Linux image with boot catalog media type not >>> NO_EMULATOR. Anyway, thanks for your test and your quickly response. >>> >>> Thanks, >>> Zhichao >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io On Behalf Of Gary >>>> Lin >>>> Sent: Friday, October 16, 2020 2:42 PM >>>> To: Laszlo Ersek >>>> Cc: Gao, Zhichao ; devel@edk2.groups.io; Ni, >>>> Ray ; Wu, Hao A >>>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert >>>> the >>> child >>>> handler blocksize change >>>> >>>> On Thu, Oct 15, 2020 at 12:17:50PM +0200, Laszlo Ersek wrote: >>>>> On 10/12/20 09:22, Gao, Zhichao wrote: >>>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2843 >>>>>> >>>>>> Revert the patch to change the block size in child handler. It >>>>>> would block the CD (Eltorito) Hard disk media type's sub >>>>>> partition being observed. >>>>>> The blocksize patch used to fix the CD image's MBR table issue. >>>>>> The CD MBR table would always be ignored because it would be >>>>>> handled by the Eltorito partition handler first and never go >>>>>> into the MBR handler. So directly revert it. >>>>>> >>>>>> Cc: Ray Ni >>>>>> Cc: Hao A Wu >>>>>> Signed-off-by: Zhichao Gao >>>>>> --- >>>>>> MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c | 12 >>>>>> +++++++++--- >>>>>> 1 file changed, 9 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git >>>>>> a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> index f10ce7c65b..473e091320 100644 >>>>>> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Partition.c >>>>>> @@ -1149,8 +1149,8 @@ PartitionInstallChildHandle ( >>>>>> >>>>>> Private->Signature = PARTITION_PRIVATE_DATA_SIGNATURE; >>>>>> >>>>>> - Private->Start = MultU64x32 (Start, BlockSize); >>>>>> - Private->End = MultU64x32 (End + 1, BlockSize); >>>>>> + Private->Start = MultU64x32 (Start, ParentBlockIo->Media- >>>>> BlockSize); >>>>>> + Private->End = MultU64x32 (End + 1, ParentBlockIo->Media- >>>>> BlockSize); >>>>>> >>>>>> Private->BlockSize = BlockSize; >>>>>> Private->ParentBlockIo = ParentBlockIo; >>>>>> @@ -1187,7 +1187,13 @@ PartitionInstallChildHandle ( >>>>>> >>>>>> Private->Media.IoAlign = 0; >>>>>> Private->Media.LogicalPartition = TRUE; >>>>>> - Private->Media.LastBlock = End - Start; >>>>>> + Private->Media.LastBlock = DivU64x32 ( >>>>>> + MultU64x32 ( >>>>>> + End - Start + 1, >>>>>> + ParentBlockIo->Media->BlockSize >>>>>> + ), >>>>>> + BlockSize >>>>>> + ) - 1; >>>>>> >>>>>> Private->Media.BlockSize = (UINT32) BlockSize; >>>>>> >>>>>> >>>>> >>>> Hi Laszlo, >>>> >>>>> (1) Adding Gary Lin to the CC list. >>>>> >>>> Thanks for noticing me :) >>>> >>>>> >>>>> (2) I can see that the TianoCore bugzilla ticket, namely >>>>> , has been >> reopened. >>>>> >>>>> That's wrong. >>>>> >>>>> TianoCore#2843 was about calculating the starting and ending LBAs >>>>> with incorrect block sizes. It was fixed by commit e0eacd7daa6f. >>>>> Therefore >>>>> TianoCore#2843 should stay in RESOLVED|FIXED status. >>>>> >>>>> Now that we have realized that commit e0eacd7daa6f caused a >>>>> regression, a *new BZ* should be filed, stating the particular >>>>> compatibility issue (regression). It is a different symptom from >>>>> the symptom originally reported under TianoCore#2843, so it >>>>> belongs in a >>>> different ticket. >>>>> >>>>> In particular, the statement in >>>>> that the >>>>> original commit (which now should be reverted) "doesn't fix any >>>>> specific issue", is *completely wrong*. If you look at commit >>>>> e0eacd7daa6f, it contains the tag >>>>> >>>>> Tested-by: Gary Lin >>>>> >>>>> Furthermore, if you look at the mailing list archive, you will >>>>> find the following confirmation from Gary: >>>>> >>>>> After applying this patch series, the firmware recognizes >>>>> openSUSE/SUSE iso images again. >>>>> >>>>> In the v1 thread at >>>>> >>>>> [edk2-devel] [PATCH 0/3] >>>>> MdeModulePkg/PartitionDxe: Make the parition driver match the >>>>> spec >>>>> >>>>> https://edk2.groups.io/g/devel/message/63972 >>>>> http://mid.mail- >>> archive.com/20200811075443.GG21538@GaryWorkstation >>>>> >>>>> And then, in the v2 thread, Gary wrote >>>>> >>>>> I've tested the following ISO images and all booted as expected. >>>>> [...] >>>>> >>>>> again giving a Tested-by: >>>>> >>>>> [edk2-devel] [PATCH V2 0/3] >>>>> MdeModulePkg/PartitionDxe: Make the parition driver match the >>>>> spec >>>>> >>>>> https://edk2.groups.io/g/devel/message/64047 >>>>> http://mid.mail- >>> archive.com/20200812062652.GL21538@GaryWorkstation >>>>> >>>>> >>>>> Now that you are proposing a revert, you have missed all of the >>>>> above feedback from Gary. That's because you never bothered to >>>>> link the v1 and >>>>> v2 mailing list threads into the bugzilla ticket. >>>>> >>>>> So this patch risks reintroducing the issue that Gary reported originally. >>>>> >>>>> (Of course, the original bug report from Gary is *also* not linked >>>>> into >>>>> TianoCore#2843: >>>>> >>>>> https://edk2.groups.io/g/devel/message/62648 >>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2823#c6 >>>>> >>>>> http://mid.mail-archive.com/20200716033255.GL6058@GaryWorkstation >>>>> >>>>> so it's no wonder we have no idea whose use case we could regress >>>>> with a >>>>> revert!) >>>>> >>>>> This patch should *NOT* be merged until Gary confirms it's OK. >>>>> >>>>> (And if it's not OK, then a solution is needed that fixes both >>>>> Gary's use case, and the compatibility regression. It might even >>>>> need a PCD, if there is media out there that needs one kind of >>>>> logic, and other media that needs the other kind of logic.) >>>>> >>>> I just tested the patch with the ISO files with SLE15-SP2, openSUSE >>>> Leap 15.2, Fedora 32, and ubuntu 20.04, and the VM loads them >>>> without any problem, so there is no regression I had before. >>>> >>>> I'd give it my Tested-by. >>>> >>>> Tested-by: Gary Lin >>>> >>>> Gary Lin >>>> >>>>> >>>>> (3) If this patch is a revert of commit e0eacd7daa6f, then the >>>>> revert should be prepared with the "git revert" command. In >>>>> particular, the commit message should very clearly state that this >>>>> patch reverts commit e0eacd7daa6f. >>>>> >>>>> >>>>> Thanks >>>>> Laszlo >>>>> >>>> >>>> >>>> >>>> >>>> >