Re: [PATCH] tftp: roll-over block counter to prevent data packets timeouts

[Javier Martinez Canillas]

Hello Daniel,

Thanks for the review.

On 9/7/20 9:36 PM, Daniel Kiper wrote:
> On Tue, Sep 01, 2020 at 02:30:35PM +0200, Javier Martinez Canillas wrote:
>> Commit 781b3e5efc3 ("tftp: Do not use priority queue") caused a regression
> 
> Please drop the quotes.
>

Sure, I can do that but I wonder why you don't want the quotes.
That's the convention used in many other projects.

[snip] 

>>
>> Fixes: 781b3e5efc3 ("tftp: Do not use priority queue")
> 
> Please drop this line.
>

Same question here. I think is important information, specially for
downstream since they could allow people to decide whether they need
to backport this patch or not.

[snip] 

>> +       */
>> +      if (grub_be_to_cpu16 (tftph->u.data.block) < ((data->block + 1) & 
>> BLK_MASK))
>>      ack (data, grub_be_to_cpu16 (tftph->u.data.block));
>>        /* Ignore unexpected block. */
>>        else if (grub_be_to_cpu16 (tftph->u.data.block) > data->block + 1)
> 
> I think the fix is incomplete. You should change the line above too.
>

True, thanks for pointing it out.
 
> However, why do not do "((grub_uint16_t) (data->block + 1))" instead of
> "& BLK_MASK" in general?
> 

Indeed. I'll change that and post a v2.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat