On 8/24/20 2:18 PM, Vito Caputo wrote:
> On Mon, Aug 24, 2020 at 02:10:27PM -0700, trix@xxxxxxxxxx wrote:
>> From: Tom Rix <trix@xxxxxxxxxx>
>>
>> clang static analysis reports this representative problem
>>
>> transport.c:495:15: warning: Assigned value is garbage or
>> undefined
>> length_left -= partial;
>> ^ ~~~~~~~
>> partial is set only when usb_stor_bulk_transfer_sglist()
>> is successful.
>>
>> So set partial on entry to 0.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
>> ---
>> drivers/usb/storage/transport.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
>> index 238a8088e17f..044429717dcc 100644
>> --- a/drivers/usb/storage/transport.c
>> +++ b/drivers/usb/storage/transport.c
>> @@ -414,6 +414,9 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
>> {
>> int result;
>>
>> + if (act_len)
>> + *act_len = 0;
>> +
>> /* don't submit s-g requests during abort processing */
>> if (test_bit(US_FLIDX_ABORTING, &us->dflags))
>> return USB_STOR_XFER_ERROR;
> At a glance this seems odd to me. If the caller insists on ignoring
> the return value, shouldn't it just initialize partial to zero?
>
> In my experience it's generally frowned upon for functions to store
> results in error paths.
Then maybe v1 is more appropriate.
Else i can spin a v3.
My preference is v1 as it doesn't add any runtime if-checks.
Tom
> Regards,
> Vito Caputo
>
