On 5/18/20 2:08 PM, Guoqing Jiang wrote:
> On 5/18/20 7:50 PM, Jes Sorensen wrote:
>> On 5/18/20 1:32 PM, Guoqing Jiang wrote:
>>> On 5/18/20 7:22 PM, Jes Sorensen wrote:
>>>> On 5/15/20 9:40 AM, Guoqing Jiang wrote:
>>>>> Got below error when run "make everything".
>>>>>
>>>>> restripe.c: In function ‘test_stripes’:
>>>>> restripe.c:870:4: error: ignoring return value of ‘read’, declared
>>>>> with attribute warn_unused_result [-Werror=unused-result]
>>>>> read(source[i], stripes[i], chunk_size);
>>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> Fix it by set the return value of ‘read’ to diskP, which should be
>>>>> harmless since diskP will be set again before it is used.
>>>>>
>>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>> restripe.c | 6 +++++-
>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/restripe.c b/restripe.c
>>>>> index 31b07e8..21c90f5 100644
>>>>> --- a/restripe.c
>>>>> +++ b/restripe.c
>>>>> @@ -867,7 +867,11 @@ int test_stripes(int *source, unsigned long long
>>>>> *offsets,
>>>>> for (i = 0 ; i < raid_disks ; i++) {
>>>>> lseek64(source[i], offsets[i]+start, 0);
>>>>> - read(source[i], stripes[i], chunk_size);
>>>>> + /*
>>>>> + * To resolve "ignoring return value of ‘read’", it
>>>>> + * should be harmless since diskP will be again later.
>>>>> + */
>>>>> + diskP = read(source[i], stripes[i], chunk_size);
>>>> It doesn't complain on Fedora 32, however checking the return value of
>>>> lseek64 and read is a good thing.
>>>>
>>>> However what you have done is to just masking the return value and
>>>> throwing it away, is not OK. Please do it properly.
>>> Yes, it is used to suppress the warning. And set the return value to a
>>> new variable
>>> could cause unused-but-set-variable, not sure if there is better way
>>> now.
>> The correct way is to check the return values and take appropriate
>> action if an error is returned.
>
> Thanks, just find other places in restripe.c has check like this.
>
> "read(source[dnum], buf+disk * chunk_size, chunk_size) != chunk_size)
>
> Will send below changes if it is ok.
>
> diff --git a/restripe.c b/restripe.c
> index 31b07e8..48c6506 100644
> --- a/restripe.c
> +++ b/restripe.c
> @@ -867,7 +867,15 @@ int test_stripes(int *source, unsigned long long
> *offsets,
>
> for (i = 0 ; i < raid_disks ; i++) {
> lseek64(source[i], offsets[i]+start, 0);
> - read(source[i], stripes[i], chunk_size);
> + if (read(source[i], stripes[i], chunk_size) !=
> + chunk_size) {
> + free(q);
> + free(p);
> + free(blocks);
> + free(stripes);
> + free(stripe_buf);
> + return -1;
> + }
That should work, however you really should check the return value of
lseek as well, while looking at this.
Cheers,
Jes
