Re: Functionality issues in UDF support

[Laszlo Ersek <lersek@redhat.com>]

Hi Jiewen,

thank you for looking into this. I'm replying below quite at length;
please bear with me. At the end I'm going to propose a new alternative
to the PCD:

On 09/16/17 01:58, Yao, Jiewen wrote:
> Hi Laszlo/Ruiyu/Star
> I went through the whole conversation and have some thought:
> 
> For staging, I quote the description from https://github.com/tianocore/edk2-staging
> 
> I think UDF matches all below criteria. "edk2 required quality criteria" is the key.
> 
> We have lots of features there - such as StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem, StructurePcd.
> 
> All those features need more validation.
> We are following the rule and I think UDF feature can follow the same rule.
> 
> If there is quality concern and the quality concern cannot be resolved in a short period of time, the staging tree is a better choice.
> (NOTE: "Fix it immediately" is not an alternative; we can't do that.)

Moving the UDF feature, including the PartitionDxe changes, to
edk2-staging would require the following:


(1) A maintainer that owns and maintains the UDF branch.

- I definitely cannot be this person, first because I have no UDF
knowledge, and second because I have no time for any more maintainer
responsibilities than I currently have. Again, my interest in UDF is
*only* as a user.

- I assume Ray will also have no time for maintaining a UDF feature
branch in edk2-staging.

- If Paulo is willing to become a maintainer for the UDF branch in
edk2-staging, that's good. Assuming he is *allowed* to become such a
maintainer.

(According to the edk2-staging description, periodic rebases to
edk2/master are required, hence Paulo would be required to dedicate time
to maintaining "edk2-staging/udf" on an on-going basis.)


(2) The UDF-related commits would have to be reverted from edk2, and
applied to edk2-staging/udf.

The first part (the reverts) would be bad. The commits that have gone
into UDF support thus far -- namely the build error fixes from everyone,
and there's also a pending style improvement series from Hao Wu -- have
already improved UDF a lot, and would have never happened in the staging
repository.

Furthermore, let me explain how most end-users (upstream users) consume
OVMF:
- edk2 maintainers push patches to edk2/master,
- various people (Ard and others from Linaro, my colleague Gerd Hoffmann
on his own time, probably others)  operate build bots / CI environments
that fetch edk2 master every day, and build RPM, DEB etc. packages for OVMF
- end-users set up YUM or APT repositories on their machines, pointing
to these packages, and periodically update their OVMF packages.

This means we have a significant user base. If we break something in
edk2/master, the build bots report build failures, and the users report
functional failures. This is good for edk2/master -- not just for
OvmfPkg, but for many other modules built into the OVMF binary.

If we move UDF to edk2-staging/udf, then UDF support will never reach
these end-users. The people that operate the CI environments will not
repeatedly merge edk2-staging/udf into edk2/master on their end, just so
they can build UDF support into OVMF. Such merges can even fail, if
rebasing the edk2-staging/udf branch to the current edk2/master branch
requires *manual* conflict resolution, and the edk2-staging/udf
maintainer is busy.

--*--

I understand that you guys absolutely don't want a new FeaturePCD. How
about the following alternative. When the edk2-staging repository was
initially discussed, I mentioned to Mike that Staging or Experimental
directories would also be a possibility, inside edk2/master. So the
below is a variant of that idea:

(a) In the "MdeModulePkg/Universal/Disk/PartitionDxe" directory, create
another directory called "Experimental".

(b) Move the "Udf.c" source file to:

  MdeModulePkg/Universal/Disk/PartitionDxe/Experimental/Udf.c

(c) Create a file called "Stubs.c":

  MdeModulePkg/Universal/Disk/PartitionDxe/Stubs.c

This file would contain an implementation of
PartitionInstallUdfChildHandles() that returned EFI_NOT_FOUND
*unconditionally*, and did nothing else.

(d) In the "PartitionDxe.inf" file, replace "Udf.c" with "Stubs.c",
under [Sources].

(e) Create "PartitionExperimentalDxe.inf" in the same directory where
"PartitionDxe.inf" is:

  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionExperimentalDxe.inf

The second INF file would be identical to "PartitionDxe.inf", except it
would include "Experimental/Udf.c", rather than "Stubs.c".


The end result is that the PartitionDxe driver is built with just a UDF
stub function that does nothing, and the PartitionExperimentalDxe driver
is built with UDF support. The current regression would be fixed for all
platforms immediately that use

  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf

No new PCD for MdeModulePkg, just a new INF file, and a stub file.

In turn, OvmfPkg, ArmVirtPkg, and Nt32Pkg could accept -D UDF_ENABLE,
and select the proper INF file, based on UDF_ENABLE.

The -D UDF_ENABLE flag would also be very easy to add, for the people
that operate the CI environments / build bots.


If this were acceptable, I'd be glad to send patches for it.

If it's not acceptable, then I think we should simply revert the UDF
patches from edk2, and *not* bother about creating an edk2-staging/udf
branch. I don't believe that CI operators and end-users would care about
edk2-staging/udf, even if Paulo spent the time and rebased that branch
to edk2/master very frequently. (Above, you mentioned the staging
branches StandaloneSmm, CustomizedSecureBoot, RiscV, ResetSystem,
StructurePcd -- and those are branches that *I* don't know about.)

So, can we do "PartitionExperimentalDxe.inf"?

Thanks!
Laszlo



> ========================================================
> This repository is used by EDK II as a staging location for new
> features that are not yet ready for inclusion in EDK II.
> 
> Introduction
> =================
> Need place on tianocore.org where new features that are not ready for product
> integration can be checked in for evaluation by the EDK II community prior to
> adding to the edk2 trunk.  This serves several purposes:
> 
> * Encourage source code to be shared earlier in the development process
> * Allow source code to be shared that does not yet meet all edk2 required quality criteria
> * Allow source code to be shared so the EDK II community can help finish and validate new features
> * Provide a location to hold new features until they are deemed ready for product integration
> * Provide a location to hold new features until there is a natural point in edk2 release cycle to fully validate the new feature.
> 
> 
> 
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Saturday, September 16, 2017 7:39 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] Functionality issues in UDF support
> 
> Hi Laszlo and Ruiyu
> I can think 1 possible alternative, for your consideration only.
> 
> 1)       Move the feature to OvmfPkg.
> 
> As such, it won't block us at this moment.
> 
> Once the UDF solution has good quality, we can move it back to MdeModulePkg.
> 
> Thank you
> Yao Jiewen
> 
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, September 16, 2017 12:51 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Paulo Alcantara <pcacjr@zytor.com<mailto:pcacjr@zytor.com>>
> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
> Subject: Re: [edk2] Functionality issues in UDF support
> 
> On 09/15/17 18:40, Ni, Ruiyu wrote:
>> Laszlo,
>> Please do not add a PCD for this. Too many PCDs are no good to the project.
> 
> I understand that new MdeModulePkg PCDs are not liked, but what do you
> propose instead? If we simply revert the PartitionDxe changes, then
> people that want to experiment with general UDF support under OVMF won't
> be able to do that at all.
> 
> I'm in the process of adding -D UDF_ENABLE to OvmfPkg, ArmVirtPkg, and
> Nt32Pkg, which would control both the FeaturePCD and the inclusion of
> UdfDxe in the build. If you disagree with the FeaturePCD, I can stop
> working on this, but I don't know what the alternative is. "Fix it
> immediately" is not an alternative; we can't do that. If you want to
> revert the change, it's your prerogative, but that will prevent
> everybody from testing gradual UDF improvements. (No 3rd parties build
> OVMF from any staging branches, so if the feature is only available on a
> staging branch, it might as well not exist, for the outside world.)
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>