Re: [LTP] [PATCH v8 0/4] Improve ioctl02.c

[Petr Vorel <pvorel@suse.cz>]

Hi Marius,

> I implemented again the changes requested for the first and last
> patch.

+1

> That means I removed quite a few comments; I totally agree that
> this code was over-commented. I kept a few comments (mainly in the
> prepare function for the struct) because some struct members are
> over-abbreviated (e.g. `c_cc`) so the comments actually do help
> understanding the code. Some comments were also stating the
> intention of the code which also seemed not completely useless.

+1

> I dropped the intermediate patch to use just termios.

I don't know why, but that was for sure discussed in some previous versions.
Or do you want to get back to it after this effort gets merged?

> In the last patch I decided to use the double-assignment
> suggestion after all because the fields in termios are consistently
> wider than the fields in termio so when just swapping the
> assignment order this should be fine (there can never be a lossy
> conversion). I also decided to make the loop a macro as well
> because at this point we might as well go all-in with the macros.

We usually prefer functions, which are more readable. But I'll comment that more
at the 4th patch.

> Btw, you're sure you don't want to give C++ a try at some point :-)
> (Just mentioning this because I really would have liked using C++
> templates in this case and with C++ you can really pick what you
> like and keep everything else in the usual C-style.)

I don't think either C++ templates for LTP would be a good idea.
The code is very simple to include any templating system.
Also testing (g)libc headers should be IMHO done with plain C.

Kind regards,
Petr

> Marius Kittler (4):
>   Refactor ioctl02.c to use the new test API
>   Make checks for termio flags more strict
>   Remove disabled code in ioctl02.c
>   Extend ioctl02 to test termio and termios

>  testcases/kernel/syscalls/ioctl/ioctl02.c | 551 +++++++---------------
>  1 file changed, 171 insertions(+), 380 deletions(-)

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp