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

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

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.