Re: [PATCH 1/2] serdev: Add serdev_device_write subroutine

[Andy Shevchenko]

On Thu, Mar 16, 2017 at 4:23 PM, Andrey Smirnov
<andrew.smirnov@xxxxxxxxx> wrote:
> On Tue, Mar 14, 2017 at 4:17 PM, Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
>> On Tue, Mar 14, 2017 at 3:48 PM, Andrey Smirnov
>> <andrew.smirnov@xxxxxxxxx> wrote:

>>> +int serdev_device_write(struct serdev_device *serdev,
>>> +                       const unsigned char *buf, size_t count)
>>> +{
>>
>>> +       int ret = count;
>>
>> If count by some reason bigger than INT_MAX...

> True, I was merely copying type signature of serdev_device_write_buf,
> which would have the same problem. I can add an appropriate check to
> the code, but at the same time, I'd love to know why it wasn't a
> concern in the latter function.

Perhaps we may survive without changing it, just document that count
should not be bigger than INT_MAX.
OTOH it's counter intuitive to have size_t type which is used as int.

>>> +       for (;;) {
>>> +               size_t chunk;
>>> +
>>> +               reinit_completion(&serdev->write_wakeup);
>>> +
>>> +               chunk = serdev_device_write_buf(serdev, buf, count);
>>
>>
>>> +               if (chunk < 0) {
>>
>> This will never happen. What kind of test did you try?

> None of my code using that function ever hit that condition, OTOH, I
> am not sure why I should ignore the API's signed return type, which I
> assume is so in order to facilitate negative error returns. My
> thinking is that even if in the current codebase is incapable of ever
> returning negative result today, that doesn't mean it won't ever be,
> and IMHO the chances of that are no different from the chances of
> someone passing 'count' that is bigger than INT_MAX. Given that I'd
> rather keep the check until the type signature of
> serdev_device_write_buf changes.

You missed the point

size_t is *unsigned* type!

>>> +               if (!count)
>>
>> What is supposed to be returned? Initial count? Does it make any sense?

> I'll change it to do that in v2, but I am open to suggestions.

Since function lacks of description (or I missed it?) I am out of
knowledge what you are trying to achieve here.

> I can see how that might make the diff look nice, but OTOH, I'd rather
> not limit myself to only ~4 letters.

Up to you and Rob.

-- 
With Best Regards,
Andy Shevchenko