Re: [PATCH v2 1/3] tools:iio:iio_event_monitor: check if event is fully read

[Hartmut Knaack <knaack.h@gmx.de>]

Daniel Baluta schrieb am 13.06.2015 um 10:46:
> On Sat, Jun 13, 2015 at 11:36 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> Daniel Baluta schrieb am 10.06.2015 um 22:16:
>>> On Wed, Jun 10, 2015 at 10:51 PM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>> Check that the read event is of the expected size.
>>>>
>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>>> ---
>>>>  tools/iio/iio_event_monitor.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
>>>> index 016760e..2559fba 100644
>>>> --- a/tools/iio/iio_event_monitor.c
>>>> +++ b/tools/iio/iio_event_monitor.c
>>>> @@ -299,6 +299,12 @@ int main(int argc, char **argv)
>>>>                         }
>>>>                 }
>>>>
>>>> +               if (ret != sizeof(event)) {
>>>> +                       printf("Reading event failed!\n");
>>>> +                       ret = -EIO;
>>>> +                       break;
>>>> +               }
>>>> +
>>>
>>> For a test program this should be good enough, not sure if it does make sense
>>> to read until we get all data.
>>
>> Not sure, if I get your intention here. But as Jonathan suggested, something
>> went seriously wrong, if the sizes don't match here. Reading further, until
>> the desired size is reached could result in receiving the missing data, or
>> getting already partial data from the next event. I would leave it as is for
>> now and see if anyone ever gets this error. Then we can still dig deeper.
> 
> Ok.
> 
>>
>>>
>>> Also, we should print the error at stderr.
>>>
>>
>> I basically agree on this one, but since the majority of the error messages
>> in this file are currently just printed to stdout, I would prefer to take it
>> as is (for consistency) and have a follow-up patch to send all error messages
>> to stderr.
> 
> I've also noticed this. So I agree with you, a follow up patch would be better.
> 
> Cristina can do this, if you haven't already started working on it.
> 

Sounds good to me. I would have waited with such changes until this set got
applied, anyway.

> As a part of Outreachy Program [1] we are also investigating these user space
> tools plus the IIO dummy driver which hopefully at the end it will be moved
> out of staging.
> 

>From my experience so far, I would regard it a good idea to chop up the
workflow to get code out of staging:
  - get all the changes you have in mind applied
  - announce your intention to move it out in a reasonable time (2 weeks?), so
    people who mind will feel pressured to have a look at the code and fix
    issues in staging
  - move out smoothly ;-)

Also, making use of gits copy detection is a good idea to avoid huge patches,
which basically just copy/move a file without changes. Makes the reviewers
life much easier ;-)

Thanks,
Hartmut

> 
> Daniel.
> 
> [1] http://kernelnewbies.org/OutreachyIntro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>