> -----Original Message----- > From: Zeng, Star > Sent: Tuesday, October 16, 2018 3:50 PM > To: Wu, Hao A; [email protected] > Cc: Ni, Ruiyu; Zeng, Star > Subject: Re: [edk2] [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead > codes in FileSystemOperations.c > > On 2018/10/16 14:59, Zeng, Star wrote: > > On 2018/10/15 12:55, Hao Wu wrote: > >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249 > >> > >> We found potential dead codes within File.c during the code coverage > >> test. > >> > >> After manual review, we think the below ones are positive reports: > >> > >> A. For function GetAllocationDescriptor(): > >> Due to the all the calling places for this function, the input parameter > >> 'RecordingFlags' can only with value 'LongAdsSequence' or > >> 'ShortAdsSequence'. > >> > >> So the below code will never be reached: > >> > >> return EFI_DEVICE_ERROR; > >> > >> This commit will add "ASSERT (FALSE);" before the above line to indicate > >> this. > >> > >> B. For function GetAllocationDescriptorLsn(): > >> Due to the all the calling places for this function, the input parameter > >> 'RecordingFlags' can only with value 'LongAdsSequence' or > >> 'ShortAdsSequence'. > >> > >> So the below code will never be reached: > >> > >> return EFI_UNSUPPORTED; > >> > >> This commit will add "ASSERT (FALSE);" before the above line to indicate > >> this. > >> > >> C. For function SetFileInfo(): > >> Due to the all the calling places for this function, the input parameter > >> 'FileName' will never be a NULL pointer. > >> > >> So the below codes will never be reached: > >> > >> } else { > >> FileInfo->FileName[0] = '\0'; > >> } > >> > >> This commit will add "ASSERT (FALSE);" before the above lines to indicate > >> this. > > > > Hao, > > > > Thanks for the patch. > > > > I think we should see what is the expected value for the parameter, but > > not see how caller uses the parameter. > > From this point of view, I think the C case is valid and may be no need > > to change. > > More information about the C case. There are two places in the function > to handle FileName == NULL, but this patch only updates one place. If > the patch wants to forbid the function to accept FileName == NULL, it > should update those two places and update function description at the > same time. Otherwise keep the function no change. >
Got it. I will update the series accordingly. Best Regards, Hao Wu > Thanks, > Star > > > > > > > Thanks, > > Star > > > >> > >> Cc: Paulo Alcantara <[email protected]> > >> Cc: Ruiyu Ni <[email protected]> > >> Cc: Star Zeng <[email protected]> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Hao Wu <[email protected]> > >> --- > >> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 12 > >> ++++++++++++ > >> 1 file changed, 12 insertions(+) > >> > >> diff --git > a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > >> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > >> index 1400846bf1..19acd0554c 100644 > >> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > >> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > >> @@ -738,6 +738,10 @@ GetAllocationDescriptor ( > >> ); > >> } > >> + // > >> + // Code should never reach here. > >> + // > >> + ASSERT (FALSE); > >> return EFI_DEVICE_ERROR; > >> } > >> @@ -784,6 +788,10 @@ GetAllocationDescriptorLsn ( > >> return EFI_SUCCESS; > >> } > >> + // > >> + // Code should never reach here. > >> + // > >> + ASSERT (FALSE); > >> return EFI_UNSUPPORTED; > >> } > >> @@ -2413,6 +2421,10 @@ SetFileInfo ( > >> if (FileName != NULL) { > >> StrCpyS (FileInfo->FileName, StrLen (FileName) + 1, FileName); > >> } else { > >> + // > >> + // Code should never reach here. > >> + // > >> + ASSERT (FALSE); > >> FileInfo->FileName[0] = '\0'; > >> } > >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
