MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 --- 1 file changed, 3 deletions(-)
This patch gets rid of a negative comparison of an UINT64 type (Offset)
as it'll never evaluate to true.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Reported-by: Star Zeng <star.zeng@intel.com>
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7286265373..2039f80289 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1082,9 +1082,6 @@ ReadFile (
if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
Offset = ReadFileInfo->FilePosition - FilePosition;
- if (Offset < 0) {
- Offset = -(Offset);
- }
} else {
Offset = 0;
}
--
2.11.0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
I do not understand the context of the code.
The change is good to fix the build failure, but I want to ask a question before I gave Rb. :)
Is it possible ReadFileInfo->FilePosition less than FilePosition?
Thanks,
Star
-----Original Message-----
From: Paulo Alcantara [mailto:pcacjr@zytor.com]
Sent: Wednesday, September 13, 2017 12:45 PM
To: edk2-devel@lists.01.org
Cc: Paulo Alcantara <pcacjr@zytor.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number
This patch gets rid of a negative comparison of an UINT64 type (Offset) as it'll never evaluate to true.
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Reported-by: Star Zeng <star.zeng@intel.com>
Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
---
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7286265373..2039f80289 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1082,9 +1082,6 @@ ReadFile (
if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
Offset = ReadFileInfo->FilePosition - FilePosition;
- if (Offset < 0) {
- Offset = -(Offset);
- }
} else {
Offset = 0;
}
--
2.11.0
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star" <star.zeng@intel.com> wrote:
>I do not understand the context of the code.
>The change is good to fix the build failure, but I want to ask a
>question before I gave Rb. :)
>
>Is it possible ReadFileInfo->FilePosition less than FilePosition?
Nope. When doing my tests, I briefly looked at code how it's used and also added an ASSERT() to make sure it is never lesser than FilePosition.
BTW, I *do* know that the code really needs refactoring, documentation, etc. -- I didnt do that before because I believed that it would never get upstream -- since its now -- I will look forward to that in my free time.
Its 2:46am here so I should get some sleep :-) Thanks!
Paulo
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Wednesday, September 13, 2017 12:45 PM
>To: edk2-devel@lists.01.org
>Cc: Paulo Alcantara <pcacjr@zytor.com>; Zeng, Star
><star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu
><ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>unsigned number
>
>This patch gets rid of a negative comparison of an UINT64 type (Offset)
>as it'll never evaluate to true.
>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Eric Dong <eric.dong@intel.com>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Dandan Bi <dandan.bi@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Reported-by: Star Zeng <star.zeng@intel.com>
>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>---
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>index 7286265373..2039f80289 100644
>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>@@ -1082,9 +1082,6 @@ ReadFile (
>
> if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
> Offset = ReadFileInfo->FilePosition - FilePosition;
>- if (Offset < 0) {
>- Offset = -(Offset);
>- }
> } else {
> Offset = 0;
> }
>--
>2.11.0
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ok, got it.
Really thanks for the contribution. :)
Reviewed-by: Star Zeng <star.zeng@intel.com> and pushed the patch at 35aec96c221b643b26e78305f6acda8ab0647cf3.
Thanks,
Star
-----Original Message-----
From: Paulo Alcantara [mailto:pcacjr@zytor.com]
Sent: Wednesday, September 13, 2017 1:47 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: RE: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number
On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star" <star.zeng@intel.com> wrote:
>I do not understand the context of the code.
>The change is good to fix the build failure, but I want to ask a
>question before I gave Rb. :)
>
>Is it possible ReadFileInfo->FilePosition less than FilePosition?
Nope. When doing my tests, I briefly looked at code how it's used and also added an ASSERT() to make sure it is never lesser than FilePosition.
BTW, I *do* know that the code really needs refactoring, documentation, etc. -- I didnt do that before because I believed that it would never get upstream -- since its now -- I will look forward to that in my free time.
Its 2:46am here so I should get some sleep :-) Thanks!
Paulo
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Wednesday, September 13, 2017 12:45 PM
>To: edk2-devel@lists.01.org
>Cc: Paulo Alcantara <pcacjr@zytor.com>; Zeng, Star
><star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu
><ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>unsigned number
>
>This patch gets rid of a negative comparison of an UINT64 type (Offset)
>as it'll never evaluate to true.
>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Eric Dong <eric.dong@intel.com>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Dandan Bi <dandan.bi@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Reported-by: Star Zeng <star.zeng@intel.com>
>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>---
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>index 7286265373..2039f80289 100644
>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>@@ -1082,9 +1082,6 @@ ReadFile (
>
> if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
> Offset = ReadFileInfo->FilePosition - FilePosition;
>- if (Offset < 0) {
>- Offset = -(Offset);
>- }
> } else {
> Offset = 0;
> }
>--
>2.11.0
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
Thanks,
Star
-----Original Message-----
From: Paulo Alcantara [mailto:pcacjr@zytor.com]
Sent: Wednesday, September 13, 2017 1:47 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
Subject: RE: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number
On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star" <star.zeng@intel.com> wrote:
>I do not understand the context of the code.
>The change is good to fix the build failure, but I want to ask a
>question before I gave Rb. :)
>
>Is it possible ReadFileInfo->FilePosition less than FilePosition?
Nope. When doing my tests, I briefly looked at code how it's used and also added an ASSERT() to make sure it is never lesser than FilePosition.
BTW, I *do* know that the code really needs refactoring, documentation, etc. -- I didnt do that before because I believed that it would never get upstream -- since its now -- I will look forward to that in my free time.
Its 2:46am here so I should get some sleep :-) Thanks!
Paulo
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Wednesday, September 13, 2017 12:45 PM
>To: edk2-devel@lists.01.org
>Cc: Paulo Alcantara <pcacjr@zytor.com>; Zeng, Star
><star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu
><ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>unsigned number
>
>This patch gets rid of a negative comparison of an UINT64 type (Offset)
>as it'll never evaluate to true.
>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Eric Dong <eric.dong@intel.com>
>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Dandan Bi <dandan.bi@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Reported-by: Star Zeng <star.zeng@intel.com>
>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>---
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>index 7286265373..2039f80289 100644
>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>@@ -1082,9 +1082,6 @@ ReadFile (
>
> if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
> Offset = ReadFileInfo->FilePosition - FilePosition;
>- if (Offset < 0) {
>- Offset = -(Offset);
>- }
> } else {
> Offset = 0;
> }
>--
>2.11.0
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Star,
On September 15, 2017 3:14:38 AM GMT-03:00, "Zeng, Star" <star.zeng@intel.com> 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?
Sorry if I wasn't clear enough, but when I sent UDF support to ML in ~3 years ago, nobody seemed to care and had no feedback since then, so I thought it would never get accepted and then I gave up on trying to improve, cleanup, etc. -- I even started working write support for it but also gave up. It would not make sense to work on it anymore. Right?
I would not know to tell you in which branch it should go, but as the driver gives you the ability to access an UDF fs as it's supposed to, then, IMHO, it'd be ready to go either way.
>
>
>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.
Yep. We should refactor the code, fix coding style issues, improve documetation. Since I was the only one who tested it, it definitely needs a little more testing.
With you help, we definitely can resolve all those TODO items.
Thanks!
Paulo
>
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Wednesday, September 13, 2017 1:47 PM
>To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
>Bi, Dandan <dandan.bi@intel.com>
>Subject: RE: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>unsigned number
>
>
>
>On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star"
><star.zeng@intel.com> wrote:
>>I do not understand the context of the code.
>>The change is good to fix the build failure, but I want to ask a
>>question before I gave Rb. :)
>>
>>Is it possible ReadFileInfo->FilePosition less than FilePosition?
>
>Nope. When doing my tests, I briefly looked at code how it's used and
>also added an ASSERT() to make sure it is never lesser than
>FilePosition.
>
>BTW, I *do* know that the code really needs refactoring, documentation,
>etc. -- I didnt do that before because I believed that it would never
>get upstream -- since its now -- I will look forward to that in my free
>time.
>
>Its 2:46am here so I should get some sleep :-) Thanks!
>
>Paulo
>
>>
>>
>>Thanks,
>>Star
>>-----Original Message-----
>>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>>Sent: Wednesday, September 13, 2017 12:45 PM
>>To: edk2-devel@lists.01.org
>>Cc: Paulo Alcantara <pcacjr@zytor.com>; Zeng, Star
>><star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu
>><ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>>unsigned number
>>
>>This patch gets rid of a negative comparison of an UINT64 type
>(Offset)
>>as it'll never evaluate to true.
>>
>>Cc: Star Zeng <star.zeng@intel.com>
>>Cc: Eric Dong <eric.dong@intel.com>
>>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>Cc: Dandan Bi <dandan.bi@intel.com>
>>Contributed-under: TianoCore Contribution Agreement 1.1
>>Reported-by: Star Zeng <star.zeng@intel.com>
>>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>---
>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>index 7286265373..2039f80289 100644
>>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>@@ -1082,9 +1082,6 @@ ReadFile (
>>
>> if (FilePosition + ExtentLength > ReadFileInfo->FilePosition)
>{
>> Offset = ReadFileInfo->FilePosition - FilePosition;
>>- if (Offset < 0) {
>>- Offset = -(Offset);
>>- }
>> } else {
>> Offset = 0;
>> }
>>--
>>2.11.0
>
>--
>Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Really thanks for the contribution (by using your free time). :)
Please help take care the issue raised at https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html and https://bugzilla.tianocore.org/show_bug.cgi?id=707 with high priority,
and also the review request at https://lists.01.org/pipermail/edk2-devel/2017-September/014695.html.
Thanks,
Star
-----Original Message-----
From: Paulo Alcantara [mailto:pcacjr@zytor.com]
Sent: Friday, September 15, 2017 2:49 PM
To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek (lersek@redhat.com) <lersek@redhat.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>
Subject: RE: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number
Star,
On September 15, 2017 3:14:38 AM GMT-03:00, "Zeng, Star" <star.zeng@intel.com> 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?
Sorry if I wasn't clear enough, but when I sent UDF support to ML in ~3 years ago, nobody seemed to care and had no feedback since then, so I thought it would never get accepted and then I gave up on trying to improve, cleanup, etc. -- I even started working write support for it but also gave up. It would not make sense to work on it anymore. Right?
I would not know to tell you in which branch it should go, but as the driver gives you the ability to access an UDF fs as it's supposed to, then, IMHO, it'd be ready to go either way.
>
>
>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.
Yep. We should refactor the code, fix coding style issues, improve documetation. Since I was the only one who tested it, it definitely needs a little more testing.
With you help, we definitely can resolve all those TODO items.
Thanks!
Paulo
>
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>Sent: Wednesday, September 13, 2017 1:47 PM
>To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
>Bi, Dandan <dandan.bi@intel.com>
>Subject: RE: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>unsigned number
>
>
>
>On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star"
><star.zeng@intel.com> wrote:
>>I do not understand the context of the code.
>>The change is good to fix the build failure, but I want to ask a
>>question before I gave Rb. :)
>>
>>Is it possible ReadFileInfo->FilePosition less than FilePosition?
>
>Nope. When doing my tests, I briefly looked at code how it's used and
>also added an ASSERT() to make sure it is never lesser than
>FilePosition.
>
>BTW, I *do* know that the code really needs refactoring, documentation,
>etc. -- I didnt do that before because I believed that it would never
>get upstream -- since its now -- I will look forward to that in my free
>time.
>
>Its 2:46am here so I should get some sleep :-) Thanks!
>
>Paulo
>
>>
>>
>>Thanks,
>>Star
>>-----Original Message-----
>>From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>>Sent: Wednesday, September 13, 2017 12:45 PM
>>To: edk2-devel@lists.01.org
>>Cc: Paulo Alcantara <pcacjr@zytor.com>; Zeng, Star
>><star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu
>><ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
>>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>>unsigned number
>>
>>This patch gets rid of a negative comparison of an UINT64 type
>(Offset)
>>as it'll never evaluate to true.
>>
>>Cc: Star Zeng <star.zeng@intel.com>
>>Cc: Eric Dong <eric.dong@intel.com>
>>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>Cc: Dandan Bi <dandan.bi@intel.com>
>>Contributed-under: TianoCore Contribution Agreement 1.1
>>Reported-by: Star Zeng <star.zeng@intel.com>
>>Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>---
>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>index 7286265373..2039f80289 100644
>>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>@@ -1082,9 +1082,6 @@ ReadFile (
>>
>> if (FilePosition + ExtentLength > ReadFileInfo->FilePosition)
>{
>> Offset = ReadFileInfo->FilePosition - FilePosition;
>>- if (Offset < 0) {
>>- Offset = -(Offset);
>>- }
>> } else {
>> Offset = 0;
>> }
>>--
>>2.11.0
>
>--
>Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Agree with most of your words.
Build farm is very needed for this case.
But as I said in another mail, there is a bug (#707) in current implementation
and I expect it to be fixed ASAP. Our QA is complaining!
-----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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 09/15/17 15:02, Ni, Ruiyu wrote: > Agree with most of your words. > Build farm is very needed for this case. > > But as I said in another mail, there is a bug (#707) in current implementation > and I expect it to be fixed ASAP. Our QA is complaining! I'm looking into it. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.