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

[Laszlo Ersek <lersek@redhat.com>]

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