From: "Gao, Zhichao" <zhichao.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"lersek@redhat.com" <lersek@redhat.com>,
"Ni, Ray" <ray.ni@intel.com>, "glin@suse.com" <glin@suse.com>
Cc: "Wu, Hao A" <hao.a.wu@intel.com>
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change
Date: Tue, 20 Oct 2020 01:06:36 +0000 [thread overview]
Message-ID: <MWHPR11MB16473F844F48F353FE1F628DF61F0@MWHPR11MB1647.namprd11.prod.outlook.com> (raw)
In-Reply-To: <b2dde5c0-d58f-a16e-6169-d5d2ed9f0af2@redhat.com>
Thanks, Laszlo. It helps a lot and gives a nice guide for revert.
Thanks,
Zhichao
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Monday, October 19, 2020 9:13 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>;
> devel@edk2.groups.io; glin@suse.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/PartitionDxe: Revert the child
> handler blocksize change
>
> 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@intel.com>
> >> Sent: Monday, October 19, 2020 1:56 PM
> >> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io;
> >> glin@suse.com; Laszlo Ersek <lersek@redhat.com>
> >> Cc: Wu, Hao A <hao.a.wu@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@intel.com>
> >>> Sent: Monday, October 19, 2020 10:46 AM
> >>> To: devel@edk2.groups.io; glin@suse.com; Laszlo Ersek
> >>> <lersek@redhat.com>
> >>> Cc: Ni, Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gary
> >>>> Lin
> >>>> Sent: Friday, October 16, 2020 2:42 PM
> >>>> To: Laszlo Ersek <lersek@redhat.com>
> >>>> Cc: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Ni,
> >>>> Ray <ray.ni@intel.com>; Wu, Hao A <hao.a.wu@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@intel.com>
> >>>>>> Cc: Hao A Wu <hao.a.wu@intel.com>
> >>>>>> Signed-off-by: Zhichao Gao <zhichao.gao@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@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@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 <glin@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
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >
>
>
>
>
>
next prev parent reply other threads:[~2020-10-20 1:06 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-12 7:22 [PATCH] MdeModulePkg/PartitionDxe: Revert the child handler blocksize change Gao, Zhichao
2020-10-13 2:54 ` Ni, Ray
2020-10-14 5:58 ` [edk2-devel] " Zhiguang Liu
2020-10-15 10:17 ` Laszlo Ersek
2020-10-16 6:42 ` Gary Lin
2020-10-19 2:45 ` Gao, Zhichao
2020-10-19 5:56 ` Ni, Ray
2020-10-19 9:35 ` Gao, Zhichao
2020-10-19 13:12 ` Laszlo Ersek
2020-10-20 1:06 ` Gao, Zhichao [this message]
2020-10-20 9:52 ` Laszlo Ersek
2020-10-21 1:33 ` Gao, Zhichao
2020-10-21 12:26 ` Laszlo Ersek
2020-10-22 5:36 ` Gao, Zhichao
2020-10-19 12:46 ` Laszlo Ersek
2020-10-19 2:41 ` Gao, Zhichao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR11MB16473F844F48F353FE1F628DF61F0@MWHPR11MB1647.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox