Re: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number

["Zeng, Star" <star.zeng@intel.com>]

Basically, people are totally encouraged and welcome to contribute, no one can make sure his/her patch will never break anything.
I want to emphasize I was not challenging the code/people.

Staging branch does not mean it is unmaintained, here I do not want to argue the position of Staging branch, and I am not saying the code must be in Staging branch.

I was confirming the development status of the code, if it is in very early development, should it be in master? If the code has been evaluated and reviewed before like you said, I am totally fine with it kept in master.
About the issues, I was thinking and raising how can we do better to avoid further effort (in future).


I also really want to appreciate all your contribution and dedication all the time.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, September 15, 2017 6:15 PM
To: Zeng, Star <star.zeng@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Bi, Dandan <dandan.bi@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org>
Subject: Re: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number

Hi Star,

On 09/15/17 08:14, Zeng, Star wrote:
> Hi,
>
> Based on recent issues about UDF since the code was pushed for 
> https://lists.01.org/pipermail/edk2-devel/2017-September/014360.html, 
> I want to raise some questions kindly.
>
> https://lists.01.org/pipermail/edk2-devel/2017-September/014409.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014518.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014542.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014489.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014551.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014560.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014649.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
> https://lists.01.org/pipermail/edk2-devel/2017-September/014695.html
>
> Is the code expected to be got upstream originally? (I may be a stupid 
> question since the code has been gotten upstream, I just want to 
> double confirm that as Paulo's reply below "I believed that it would 
> never get upstream".) Is the code ready to be in master? Should it be 
> in Staging branch first?
>
>
> Paulo,
> Could you help do more evaluation to the code as you said "I *do* know 
> that the code really needs refactoring, documentation, etc"? I believe 
> you are most familiar with the code and know its quality. :)
>
>
> BTW: More test seems need to be done before the code check in, for 
> example, build with various tool chains, ECC scan for coding style, 
> static tool scan, etc. That is what we(especially me) need to improve 
> in future when developing and reviewing.
> Anyway, let's help keep improving UDF codes.

I've been thinking about these questions.

I think a staging branch is appropriate when a feature contains intrusive changes that risk breaking core functionality, for several (maybe all) platforms. In my opinion, this is not the case for UdfDxe; the only functional regressions it could have caused (but didn't!) were in PartitionDxe, and in the generic driver binding code. Issues in these parts could have interfered with the normal operation of other parts of the firmware, but these parts were not large, and they were thoroughly reviewed.

The build breakages were annoying (I know first hand). However, that's exactly an area where a staging branch doesn't help. Such build breakages can only be found via actual exposure to a large set of toolchains, and they can only be avoided (during development) with long
edk2 development experience.

For example, contributors that mainly use MSVC tend to *remember* to cast &Interface to (VOID**) when fetching it, even though MSVC doesn't complain if it's not done -- the developers know from memory that GCC will complain (for good reason actually, even the (VOID**) cast is questionable, but I digress). Similarly, even though gcc doesn't complain about "Uint32Variable = Uint64Variable", people that mostly use gcc now *remember* to add an explicit (UINT32) cast, otherwise -- they know -- MSVC will complain.

Even with this experience, and in spite of the code review that we regularly do, such build issues slip through quite frequently. The only difference in this case was that the code addition was large (~5500 lines), so we got many warnings all at once.

Putting the driver on a staging branch first would have actually hampered the discovery of these issues. Similarly, including UdfDxe in OvmfPkg, ArmVirtPkg, and Nt32Pkg only conditionally (e.g. with -DUDF_ENABLE), would have had the same effect -- most people wouldn't have cared about UdfDxe at all, and UdfDxe wouldn't have been exposed to a good number of toolchains.

(Note that UdfDxe wasn't immediately included in MdeModulePkg.dsc, so it's not like the driver broke the building of one of the most important Packages in the tree. Although, based on the wide feedback, now I wonder if OvmfPkg *is* one of the most important packages in the three, haha :)
)

The build breakages could have been avoided in advance only if:

- people with access to various toolchains would have offered to
  test-build Paulo's private branch (or a staging branch) before merging
  the driver,

- or if a build farm / CI environment existed that exercised *all*
  toolchains (no exceptions!).

None of these two options seemed to work, so I'm actually happier with the way things turned out, effectively forcing people to help. (I'm saying this after personally having my urgent TODO list disrupted by the
fallout.)

Regarding the review process, nobody can be expected to thoroughly review a ~5500 code drop. This doesn't mean the driver shouldn't have been included -- it absolutely should have been; it's perfectly fine if we call it "experimental" for now, as long as it doesn't get in people's way. I don't think this justifies a staging branch; I think such material should be available to developers on the master branch, but perhaps not under MdeModulePkg.

We've talked many times about FileSystemPkg for example, but it just doesn't seem to happen -- it would require new Maintainers to be added to Maintainers.txt, and apparently that's an unsurmountable obstacle. In different projects, a FileSystemPkg/Experimental/UdfDxe/ directory would have been created, and Paulo would have been added minimally as Reviewer for that one subdirectory. Thankfully, at least registering in the TianoCore Bugzilla, and assigning bugs to the real owner, aren't difficult things to do.

If you recall, at one point I "threatened" to include UdfDxe somewhere under OvmfPkg, even though it didn't belong there at all. I just couldn't bear it any longer that a useful, albeit somewhat unpolished, contribution couldn't be accepted simply because we couldn't find the right place for it, and we were apparently *also* unwilling to *create* the right place for it.

So ultimately it landed under MdeModulePkg. Not the best location, but it will do.

Tying in with patch review, and more generally with the development process, such large features should be developed in very small steps (many small patches), over a longer time, posting prototypes early for review. While on one hand, we could attribute the overly coarse structuring of the patch series to the fact that UdfDxe had been Paulo's first large edk2 contribution -- i.e., he may have lacked the experience that he should start with the driver model first, without actual functionality, then build up the UDF stuff from small blocks, and finally integrate with PartitionDxe --, it certainly didn't help that the project basically died three years ago due to lack of interest, analysis paralysis, and inability to make a decision about the device path node.

In summary:

- Build breakages can only be prevented if we either introduce a build
  farm with *universal* toolchain coverage, or individual developers
  offer the contributor to fetch his/her branch and test-build it for
  him/her.

- Experimental material should be fine to add to the master branch, as
  long as it is not disruptive to core functionality. It should be
  clearly placed as experimental into the project tree. And, it should
  not be difficult to assign dedicated Reviewers / Maintainers
  ("owners") to just the relevant subdirectories. It's fine to exclude
  experimental drivers from platform builds by default, but that choice
  really depends on the previous point (i.e., people then really have to
  help out with test-building, otherwise toolchain coverage will
  suffer.)

- Large code drops are not uncommon from seasoned edk2 developers
  either. We should all take care to erect brand new code (drivers,
  modules, applications) in small logical steps. This requires a
  different thinking from just writing the code. It requires the
  programmer to ask themselves, "how am I going to walk my peers through
  this new code". It takes extra work, in fact a whole separate work
  phase, to re-structure someone's code -- after it is functionally
  complete -- for public review.

Okay, I ranted enough. Thanks for listening, and thank you everyone for your dedication to fixing the build issues urgently!

Laszlo