Re: [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

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

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.

Thanks,
Star

> 
> 
> Thanks,
> Star
> 
>>
>> Cc: Paulo Alcantara <paulo@paulo.ac>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
>> ---
>>   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';
>>     }
>>