Re: [PATCH 0/3] UDF partition driver fix

[Laszlo Ersek <lersek@redhat.com>]

Hi Paulo,

On 09/16/17 23:36, Paulo Alcantara wrote:
> This series fixes an UDF issue with Partition driver as discussed in ML:
> 
> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
> 
> Thanks!
> Paulo
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-partition-fix
> 
> Paulo Alcantara (3):
>   MdePkg: Add UDF volume structure definitions
>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
>  MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
>  6 files changed, 560 insertions(+), 443 deletions(-)
> 

Thank you very much for submitting this patchset quickly. I hope it will
work out, and we won't need the PartitionExperimentalDxe.inf file!

I have some trivial process-level suggestions:

* when submitting a patchset, please collect the Cc: tags from all the
commit messages, and add them to the cover letter manually. This way
everybody you CC on at least some of the patches will get the cover
letter too, presonally.

This matters because otherwise replies to the blurb will also miss those
people, personally. (I'm now adding everyone manually.)

* Because edk2 uses long directory and file names, the diffstats are
frequently truncated like above (see "..."). You can avoid this if you
format the patches like this:

  --stat=1000 --stat-graph-width=20

this will make the pathname column just as wide as necessary, and will
also keep the chart to the right reasonably narrow.

* It's probably best to include a reference to
<https://bugzilla.tianocore.org/show_bug.cgi?id=707> in the commit
messages (in particular patch #2).

* Once you post a patchset for a TianoCore BZ, it's useful to link the
series (from the mailing list archive) in the BZ itself.


Regarding the code itself, I don't think I can help here in any sensible
way. (If UDF support were located under OvmfPkg, I would totally
consider you the owner of those files, verify your patches for them on a
formal level only, and if that part was fine, I'd give an Acked-by.)

Thanks!
Laszlo