[edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change
Laszlo Ersek
lersek at redhat.com
Mon Oct 19 13:12:51 UTC 2020
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 '<original subject>'" 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 <ray.ni at intel.com>
>> Sent: Monday, October 19, 2020 1:56 PM
>> To: Gao, Zhichao <zhichao.gao at intel.com>; devel at edk2.groups.io;
>> glin at suse.com; Laszlo Ersek <lersek at redhat.com>
>> Cc: Wu, Hao A <hao.a.wu at intel.com>
>> 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 <zhichao.gao at intel.com>
>>> Sent: Monday, October 19, 2020 10:46 AM
>>> To: devel at edk2.groups.io; glin at suse.com; Laszlo Ersek
>>> <lersek at redhat.com>
>>> Cc: Ni, Ray <ray.ni at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
>>> 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Gary
>>>> Lin
>>>> Sent: Friday, October 16, 2020 2:42 PM
>>>> To: Laszlo Ersek <lersek at redhat.com>
>>>> Cc: Gao, Zhichao <zhichao.gao at intel.com>; devel at edk2.groups.io; Ni,
>>>> Ray <ray.ni at intel.com>; Wu, Hao A <hao.a.wu at intel.com>
>>>> 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 <ray.ni at intel.com>
>>>>>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>>>>> Signed-off-by: Zhichao Gao <zhichao.gao at intel.com>
>>>>>> ---
>>>>>> 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
>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843>, 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
>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2843#c8> 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 <glin at suse.com>
>>>>>
>>>>> 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 at 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 at 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 <glin at suse.com>
>>>>
>>>> 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
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66388): https://edk2.groups.io/g/devel/message/66388
Mute This Topic: https://groups.io/mt/77455892/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list