| Message ID | 20210120143723.26483-3-rpalethorpe@suse.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert CAN tests to new LTP API | expand |
Hello Richard, On 20.01.21 15:37, Richard Palethorpe wrote: > Note that we call modprobe to set echo=1. However this does seem to be > necessary for the current tests on 5.10. It has been kept to avoid > changing the test behavior unnecessarily, but can most likely be > safely removed if it causes problems. Without echo=1 a shortcut in af_can.c is used. I just checked that it works too - but with echo=1 we increase the test coverage to a test of the message flow down to the virtual CAN driver vcan. > > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> > --- > .../network/can/filter-tests/can_common.h | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > create mode 100644 testcases/network/can/filter-tests/can_common.h > > diff --git a/testcases/network/can/filter-tests/can_common.h b/testcases/network/can/filter-tests/can_common.h > new file mode 100644 > index 000000000..f15145f30 > --- /dev/null > +++ b/testcases/network/can/filter-tests/can_common.h > @@ -0,0 +1,75 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2021 SUSE LLC > + */ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <unistd.h> > +#include <string.h> > + > +#include <sys/types.h> > +#include <sys/socket.h> > +#include <sys/ioctl.h> > +#include <sys/time.h> > + > +#include "tst_cmd.h" > +#include "tst_safe_stdio.h" > +#include "tst_safe_file_ops.h" > + > +#include <linux/if.h> > +#include <linux/can.h> > +#include <linux/can/raw.h> > + > +#ifndef IFF_ECHO > +# define IFF_ECHO (1<<18) > +#endif IFF_ECHO was included into Linux 2.6.25 together with the CAN subsystem itself. So when you run the tests on Kernels < 2.6.25 you don't have CAN support and don't need IFF_ECHO too. Regards, Oliver > + > +static char *can_dev_name; > +static int can_created_dev; > + > +static void can_cmd(const char *const argv[]) > +{ > + tst_cmd(argv, NULL, NULL, TST_CMD_TCONF_ON_MISSING); > +} > + > +#define CAN_CMD(...) can_cmd((const char *const[]){ __VA_ARGS__, NULL }) > + > +static void can_setup_vcan(void) > +{ > + unsigned int flags; > + char *path; > + > + if (can_dev_name) > + goto check_echo; > + > + can_dev_name = "vcan0"; > + > + tst_res(TINFO, "Creating vcan0 device; use -D option to avoid this"); > + > + CAN_CMD("modprobe", "-r", "vcan"); > + CAN_CMD("modprobe", "vcan", "echo=1"); > + > + can_created_dev = 1; > + > + CAN_CMD("ip", "link", "add", "dev", "vcan0", "type", "vcan"); > + CAN_CMD("ip", "link", "set", "dev", "vcan0", "up"); > + > +check_echo: > + /* Precondition for the frame flow test? */ > + SAFE_ASPRINTF(&path, "/sys/class/net/%s/flags", can_dev_name); > + if (FILE_SCANF(path, "%x", &flags) || !(flags & IFF_ECHO)) { > + tst_res(TWARN, "Could not determine if ECHO is set on %s", > + can_dev_name); > + } > +} > + > +static void can_cleanup_vcan(void) > +{ > + if (!can_created_dev) > + return; > + > + CAN_CMD("ip", "link", "set", "dev", "vcan0", "down"); > + CAN_CMD("ip", "link", "del", "dev", "vcan0"); > + CAN_CMD("modprobe", "-r", "vcan"); > +} >
Hello Oliver and Petr, Oliver Hartkopp <socketcan@hartkopp.net> writes: > Hello Richard, > > On 20.01.21 15:37, Richard Palethorpe wrote: >> Note that we call modprobe to set echo=1. However this does seem to be >> necessary for the current tests on 5.10. It has been kept to avoid >> changing the test behavior unnecessarily, but can most likely be >> safely removed if it causes problems. > > Without echo=1 a shortcut in af_can.c is used. > > I just checked that it works too - but with echo=1 we increase the > test coverage to a test of the message flow down to the virtual CAN > driver vcan. Ah, thanks, I will amend the comments. > >> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> >> --- >> .../network/can/filter-tests/can_common.h | 75 +++++++++++++++++++ >> 1 file changed, 75 insertions(+) >> create mode 100644 testcases/network/can/filter-tests/can_common.h >> diff --git a/testcases/network/can/filter-tests/can_common.h >> b/testcases/network/can/filter-tests/can_common.h >> new file mode 100644 >> index 000000000..f15145f30 >> --- /dev/null >> +++ b/testcases/network/can/filter-tests/can_common.h >> @@ -0,0 +1,75 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * Copyright (c) 2021 SUSE LLC >> + */ >> + >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <unistd.h> >> +#include <string.h> >> + >> +#include <sys/types.h> >> +#include <sys/socket.h> >> +#include <sys/ioctl.h> >> +#include <sys/time.h> >> + >> +#include "tst_cmd.h" >> +#include "tst_safe_stdio.h" >> +#include "tst_safe_file_ops.h" >> + >> +#include <linux/if.h> >> +#include <linux/can.h> >> +#include <linux/can/raw.h> >> + >> +#ifndef IFF_ECHO >> +# define IFF_ECHO (1<<18) >> +#endif > > IFF_ECHO was included into Linux 2.6.25 together with the CAN > subsystem itself. > > So when you run the tests on Kernels < 2.6.25 you don't have CAN > support and don't need IFF_ECHO too. Petr, what kernel version and/or distro version did compilation fail on? There is a small chance someone might be compiling with old kernel headers relative to their kernel. However it is a challenge to compile LTP with such an old user land. > > Regards, > Oliver > >> + >> +static char *can_dev_name; >> +static int can_created_dev; >> + >> +static void can_cmd(const char *const argv[]) >> +{ >> + tst_cmd(argv, NULL, NULL, TST_CMD_TCONF_ON_MISSING); >> +} >> + >> +#define CAN_CMD(...) can_cmd((const char *const[]){ __VA_ARGS__, NULL }) >> + >> +static void can_setup_vcan(void) >> +{ >> + unsigned int flags; >> + char *path; >> + >> + if (can_dev_name) >> + goto check_echo; >> + >> + can_dev_name = "vcan0"; >> + >> + tst_res(TINFO, "Creating vcan0 device; use -D option to avoid this"); >> + >> + CAN_CMD("modprobe", "-r", "vcan"); >> + CAN_CMD("modprobe", "vcan", "echo=1"); >> + >> + can_created_dev = 1; >> + >> + CAN_CMD("ip", "link", "add", "dev", "vcan0", "type", "vcan"); >> + CAN_CMD("ip", "link", "set", "dev", "vcan0", "up"); >> + >> +check_echo: >> + /* Precondition for the frame flow test? */ >> + SAFE_ASPRINTF(&path, "/sys/class/net/%s/flags", can_dev_name); >> + if (FILE_SCANF(path, "%x", &flags) || !(flags & IFF_ECHO)) { >> + tst_res(TWARN, "Could not determine if ECHO is set on %s", >> + can_dev_name); >> + } >> +} >> + >> +static void can_cleanup_vcan(void) >> +{ >> + if (!can_created_dev) >> + return; >> + >> + CAN_CMD("ip", "link", "set", "dev", "vcan0", "down"); >> + CAN_CMD("ip", "link", "del", "dev", "vcan0"); >> + CAN_CMD("modprobe", "-r", "vcan"); >> +} >>
Hi Oliver, Richie, > >> --- /dev/null > >> +++ b/testcases/network/can/filter-tests/can_common.h > >> @@ -0,0 +1,75 @@ > >> +// SPDX-License-Identifier: GPL-2.0-or-later > >> +/* > >> + * Copyright (c) 2021 SUSE LLC > >> + */ > >> + > >> +#include <stdio.h> > >> +#include <stdlib.h> > >> +#include <unistd.h> > >> +#include <string.h> > >> + > >> +#include <sys/types.h> > >> +#include <sys/socket.h> > >> +#include <sys/ioctl.h> > >> +#include <sys/time.h> > >> + > >> +#include "tst_cmd.h" > >> +#include "tst_safe_stdio.h" > >> +#include "tst_safe_file_ops.h" > >> + > >> +#include <linux/if.h> > >> +#include <linux/can.h> > >> +#include <linux/can/raw.h> > >> + > >> +#ifndef IFF_ECHO > >> +# define IFF_ECHO (1<<18) > >> +#endif Thanks a lot, Richie! > > IFF_ECHO was included into Linux 2.6.25 together with the CAN > > subsystem itself. > > So when you run the tests on Kernels < 2.6.25 you don't have CAN > > support and don't need IFF_ECHO too. > Petr, what kernel version and/or distro version did compilation fail on? > There is a small chance someone might be compiling with old kernel > headers relative to their kernel. However it is a challenge to compile > LTP with such an old user land. No, we don't support 2.6.25 :). I was playing with Buildroot distro in my spare time. These embedded toolchains suffer compatibility problems (usually uclibc-ng and sometimes musl lack the support). This problem was when using sourcery-arm-*. But this is definitely not a blocker for this patchset. That lapi is not a must, I can fix it some time later. I usually fix few of these problems before each LTP release. Kind regards, Petr
Hi Petr, On 26.01.21 22:28, Petr Vorel wrote: > Hi Oliver, Richie, > >>>> --- /dev/null >>>> +++ b/testcases/network/can/filter-tests/can_common.h >>>> @@ -0,0 +1,75 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>> +/* >>>> + * Copyright (c) 2021 SUSE LLC >>>> + */ >>>> + >>>> +#include <stdio.h> >>>> +#include <stdlib.h> >>>> +#include <unistd.h> >>>> +#include <string.h> >>>> + >>>> +#include <sys/types.h> >>>> +#include <sys/socket.h> >>>> +#include <sys/ioctl.h> >>>> +#include <sys/time.h> >>>> + >>>> +#include "tst_cmd.h" >>>> +#include "tst_safe_stdio.h" >>>> +#include "tst_safe_file_ops.h" >>>> + >>>> +#include <linux/if.h> >>>> +#include <linux/can.h> >>>> +#include <linux/can/raw.h> >>>> + >>>> +#ifndef IFF_ECHO >>>> +# define IFF_ECHO (1<<18) >>>> +#endif > Thanks a lot, Richie! > >>> IFF_ECHO was included into Linux 2.6.25 together with the CAN >>> subsystem itself. > >>> So when you run the tests on Kernels < 2.6.25 you don't have CAN >>> support and don't need IFF_ECHO too. > >> Petr, what kernel version and/or distro version did compilation fail on? > >> There is a small chance someone might be compiling with old kernel >> headers relative to their kernel. However it is a challenge to compile >> LTP with such an old user land. > > No, we don't support 2.6.25 :). I was playing with Buildroot distro in my spare time. > These embedded toolchains suffer compatibility problems (usually uclibc-ng and > sometimes musl lack the support). This problem was when using sourcery-arm-*. :-/ > But this is definitely not a blocker for this patchset. That lapi is not a must, > I can fix it some time later. I usually fix few of these problems before each > LTP release. Ok. No problem. I wasn't aware that e.g. musl or other toolchains select such strange starting points for their include files. Thanks, Oliver
Hello Petr & Oliver, Oliver Hartkopp <socketcan@hartkopp.net> writes: > Hi Petr, > > On 26.01.21 22:28, Petr Vorel wrote: >> Hi Oliver, Richie, >> >>>>> --- /dev/null >>>>> +++ b/testcases/network/can/filter-tests/can_common.h >>>>> @@ -0,0 +1,75 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>> +/* >>>>> + * Copyright (c) 2021 SUSE LLC >>>>> + */ >>>>> + >>>>> +#include <stdio.h> >>>>> +#include <stdlib.h> >>>>> +#include <unistd.h> >>>>> +#include <string.h> >>>>> + >>>>> +#include <sys/types.h> >>>>> +#include <sys/socket.h> >>>>> +#include <sys/ioctl.h> >>>>> +#include <sys/time.h> >>>>> + >>>>> +#include "tst_cmd.h" >>>>> +#include "tst_safe_stdio.h" >>>>> +#include "tst_safe_file_ops.h" >>>>> + >>>>> +#include <linux/if.h> >>>>> +#include <linux/can.h> >>>>> +#include <linux/can/raw.h> >>>>> + >>>>> +#ifndef IFF_ECHO >>>>> +# define IFF_ECHO (1<<18) >>>>> +#endif >> Thanks a lot, Richie! >> >>>> IFF_ECHO was included into Linux 2.6.25 together with the CAN >>>> subsystem itself. >> >>>> So when you run the tests on Kernels < 2.6.25 you don't have CAN >>>> support and don't need IFF_ECHO too. >> >>> Petr, what kernel version and/or distro version did compilation fail on? >> >>> There is a small chance someone might be compiling with old kernel >>> headers relative to their kernel. However it is a challenge to compile >>> LTP with such an old user land. >> No, we don't support 2.6.25 :). I was playing with Buildroot distro >> in my spare time. >> These embedded toolchains suffer compatibility problems (usually uclibc-ng and >> sometimes musl lack the support). This problem was when using sourcery-arm-*. > > :-/ > >> But this is definitely not a blocker for this patchset. That lapi is not a must, >> I can fix it some time later. I usually fix few of these problems before each >> LTP release. > > Ok. No problem. I wasn't aware that e.g. musl or other toolchains > select such strange starting points for their include files. I wonder Petr, is it still necessary to define IFF_ECHO now only <linux/if.h> is included? Or do they somehow symlink linux/if.h -> net/if.h? Indeed it seems the current version of uclibc-ng doesn't include IFF_ECHO in <net/if.h>. OTOH musl does define it.
Hi Richie, <snip> > >>>> IFF_ECHO was included into Linux 2.6.25 together with the CAN > >>>> subsystem itself. > >>>> So when you run the tests on Kernels < 2.6.25 you don't have CAN > >>>> support and don't need IFF_ECHO too. > >>> Petr, what kernel version and/or distro version did compilation fail on? > >>> There is a small chance someone might be compiling with old kernel > >>> headers relative to their kernel. However it is a challenge to compile > >>> LTP with such an old user land. > >> No, we don't support 2.6.25 :). I was playing with Buildroot distro > >> in my spare time. > >> These embedded toolchains suffer compatibility problems (usually uclibc-ng and > >> sometimes musl lack the support). This problem was when using sourcery-arm-*. > > :-/ > >> But this is definitely not a blocker for this patchset. That lapi is not a must, > >> I can fix it some time later. I usually fix few of these problems before each > >> LTP release. > > Ok. No problem. I wasn't aware that e.g. musl or other toolchains > > select such strange starting points for their include files. > I wonder Petr, is it still necessary to define IFF_ECHO now only > <linux/if.h> is included? Or do they somehow symlink linux/if.h -> > net/if.h? No. > Indeed it seems the current version of uclibc-ng doesn't include > IFF_ECHO in <net/if.h>. OTOH musl does define it. Yes => please keep that definition in can_common.h (enough here, we don't have to bother with if.h when we don't have it yet). And I'll send tonight patch to uclibc-ng. The more mature musl is the less relevant uclibc-ng is. Kind regards, Petr
Hi Richie, ... > > I wonder Petr, is it still necessary to define IFF_ECHO now only > > <linux/if.h> is included? Or do they somehow symlink linux/if.h -> > > net/if.h? > No. > > Indeed it seems the current version of uclibc-ng doesn't include > > IFF_ECHO in <net/if.h>. OTOH musl does define it. > Yes => please keep that definition in can_common.h (enough here, we don't have > to bother with if.h when we don't have it yet). > And I'll send tonight patch to uclibc-ng. > The more mature musl is the less relevant uclibc-ng is. Just for a record, I was wrong. Using <linux/if.h> is enough, no need to keep definition on can_common.h. Sorry for wasting your time. I mixed two problems: <linux/if.h> and <net/if.h> conflict (there are more headers which conflict) [1] and sometimes missing definition on uclibc-ng. *But* musl defines IFF_ECHO in <net/if.h> as they try to allow people not having to depend on <linux/*.h>, which is IMHO better than blindly relying on <linux/*.h> which glibc and uclibc{,-ng} (which follows glibc) does much more than musl: $ git grep '^#include <linux/.*\.h>' |wc -l 43 # glibc 37 # uclibc-ng 3 # musl Kind regards, Petr [1] https://sourceware.org/glibc/wiki/Synchronizing_Headers#Known_Pairs_of_Headers_that_Conflict
diff --git a/testcases/network/can/filter-tests/can_common.h b/testcases/network/can/filter-tests/can_common.h new file mode 100644 index 000000000..f15145f30 --- /dev/null +++ b/testcases/network/can/filter-tests/can_common.h @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2021 SUSE LLC + */ + +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> + +#include <sys/types.h> +#include <sys/socket.h> +#include <sys/ioctl.h> +#include <sys/time.h> + +#include "tst_cmd.h" +#include "tst_safe_stdio.h" +#include "tst_safe_file_ops.h" + +#include <linux/if.h> +#include <linux/can.h> +#include <linux/can/raw.h> + +#ifndef IFF_ECHO +# define IFF_ECHO (1<<18) +#endif + +static char *can_dev_name; +static int can_created_dev; + +static void can_cmd(const char *const argv[]) +{ + tst_cmd(argv, NULL, NULL, TST_CMD_TCONF_ON_MISSING); +} + +#define CAN_CMD(...) can_cmd((const char *const[]){ __VA_ARGS__, NULL }) + +static void can_setup_vcan(void) +{ + unsigned int flags; + char *path; + + if (can_dev_name) + goto check_echo; + + can_dev_name = "vcan0"; + + tst_res(TINFO, "Creating vcan0 device; use -D option to avoid this"); + + CAN_CMD("modprobe", "-r", "vcan"); + CAN_CMD("modprobe", "vcan", "echo=1"); + + can_created_dev = 1; + + CAN_CMD("ip", "link", "add", "dev", "vcan0", "type", "vcan"); + CAN_CMD("ip", "link", "set", "dev", "vcan0", "up"); + +check_echo: + /* Precondition for the frame flow test? */ + SAFE_ASPRINTF(&path, "/sys/class/net/%s/flags", can_dev_name); + if (FILE_SCANF(path, "%x", &flags) || !(flags & IFF_ECHO)) { + tst_res(TWARN, "Could not determine if ECHO is set on %s", + can_dev_name); + } +} + +static void can_cleanup_vcan(void) +{ + if (!can_created_dev) + return; + + CAN_CMD("ip", "link", "set", "dev", "vcan0", "down"); + CAN_CMD("ip", "link", "del", "dev", "vcan0"); + CAN_CMD("modprobe", "-r", "vcan"); +}
Note that we call modprobe to set echo=1. However this does seem to be necessary for the current tests on 5.10. It has been kept to avoid changing the test behavior unnecessarily, but can most likely be safely removed if it causes problems. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- .../network/can/filter-tests/can_common.h | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 testcases/network/can/filter-tests/can_common.h