[LTP] recvmmsg(2) system call tests

[LTP] recvmmsg(2) system call tests

[Ramon Pantin]

Per Rafael's earlier email about how to proceed, find attached the path 
with the test cases.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-recvmmsg-2-test-cases.patch
Type: text/x-patch
Size: 70158 bytes
Desc: not available
URL: <http://lists.linux.it/pipermail/ltp/attachments/20181031/cc02a2b4/attachment-0001.bin>

[LTP] recvmmsg(2) system call tests

[Petr Vorel]

Hi Ramon,

thank you for moving from github pull request to mailing list.

> Per Rafael's earlier email about how to proceed, find attached the path with
> the test cases.
You sent your patch as an attachment.
Please, next time, send is at plain text,
as although patchwork can work with that [1], mailman can't handle it [2].
Here are some instructions [3].
Please add your Signed-off-by: tag.

I've added some comments in github pull request [4], you didn't change them, so
I repeat some of it here. But please, read the doc to provide usable patch [5] [6].
2165 lines for one file suggest to split file into several.

Please have look into the to see code style (short code, easy to read, using
SAFE_*() helpers to prevent reinventing a wheal, avoiding macros, remove comments
which has nothing to say, ...).

LTP code varies, some is really old and ugly. But we try to include reasonably
clean code. You can have look at this as a good example of code style:
testcases/kernel/syscalls/inotify/inotify09.c
59e0a81c5 inotify07: Add test for kernel crash during event notification
It has also TST_TEST_TCONF

> From dea04e5946e8a30787943f347cf3861a8dca6008 Mon Sep 17 00:00:00 2001
> From: Ramon Pantin <pantin@google.com>
> Date: Wed, 31 Oct 2018 16:26:50 -0700
> Subject: [PATCH] recvmmsg(2) test cases
This should be removed.

> ---
>  testcases/kernel/syscalls/recvmmsg/Makefile   |   27 +
>  .../kernel/syscalls/recvmmsg/recvmmsg01.c     | 2165 +++++++++++++++++
>  2 files changed, 2192 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/recvmmsg/Makefile
>  create mode 100644 testcases/kernel/syscalls/recvmmsg/recvmmsg01.c

> diff --git a/testcases/kernel/syscalls/recvmmsg/Makefile b/testcases/kernel/syscalls/recvmmsg/Makefile
> new file mode 100644
> index 000000000..61b3a55e6
> --- /dev/null
> +++ b/testcases/kernel/syscalls/recvmmsg/Makefile
> @@ -0,0 +1,27 @@
> +#  Copyright (c) Google LLC, 2018
> +#  Copyright (c) International Business Machines  Corp., 2001
^ Please delete this IBM copyright.

> +#  This program is free software;  you can redistribute it and/or modify
> +#  it under the terms of the GNU General Public License as published by
> +#  the Free Software Foundation; either version 2 of the License, or
> +#  (at your option) any later version.
> +#
> +#  This program is distributed in the hope that it will be useful,
> +#  but WITHOUT ANY WARRANTY;  without even the implied warranty of
> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> +#  the GNU General Public License for more details.
> +#
> +#  You should have received a copy of the GNU General Public License
> +#  along with this program;  if not, write to the Free Software
> +#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
Please use just SPDX identifier instead of address:
# SPDX-License-Identifier: GPL-2.0-or-later

> +#
> +
> +top_srcdir		?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +
> +CPPFLAGS		+= -I$(abs_srcdir)/../utils
Why this include?

> +CFLAGS			= -g -std=c99
Please can you remove CFLAGS and produce code without warnings?
I'm also not sure about -std=c99. While you can grep for some -std=gnu99 in the
code, these are just some outdated comments in the source, not passed to make.

> +LDLIBS			+= -lpthread
> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
> new file mode 100644
> index 000000000..131bcfd66
> --- /dev/null
> +++ b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
> @@ -0,0 +1,2165 @@
> + //  Copyright (c) Google LLC, 2018
> + //  SPDX-License-Identifier: GPL-2.0-or-later

> + //
> + //{
Why these lines?

> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <pthread.h>
> +#include <assert.h>
> +#include <sys/types.h>
> +#include <sys/time.h>
> +#include <sys/signal.h>
> +#include <sys/uio.h>
> +#include <sys/file.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <netinet/in.h>
Some of the includes will not be needed if you use API functions (SAFE_*, [5], [6]).

> +
> +// #define RECVMMSG_USE_SCTP
> +#ifdef RECVMMSG_USE_SCTP
> +#include <netinet/sctp.h>
> +#endif
Code related to RECVMMSG_USE_SCTP should be checked with autotools. There is our header
include/lapi/sctp.h, you can include it and add (in separate commit) definitions, which are needed.
If you really need netinet/sctp.h header, you can add exit clause with TST_TEST_TCONF

> +
> +#define TST_NO_DEFAULT_MAIN
You're not supposed to use this

> +#include "tst_test.h"
> +// #include "safe_macros.h"
Please remove all comments left

> +
> + //}
> + // 	Declarations.
> + //
> + //{	<-- (curly-brace match to move through major sections of this file)
Please comments like this.
Please use C style comments /* */

> +int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov);
> +int con_receive_iovec_boundary_checks_peeking(con_t *conp, int tn, size_t niov);
> +int con_receive_file_descriptor(con_t *conp, int tn, int cloexec_flag,
> +				bool some_data, ssize_t controllen_delta);
> +int con_message_too_long_to_fit_might_discard_bytes(con_t *conp, int tn,
> +						    int type);
> +int con_receive_waits_for_message(con_t *conp, int tn);
> +int con_receiver_doesnt_wait_for_messages(con_t *conp, int tn);
> +int con_receive_returns_what_is_available(con_t *conp, int tn);
> +int con_empty_datagram_received_as_empty_datagram(con_t *conp, int tn);
Function signatures aren't needed when used just in one file.
Actually, in this case we declare functions as static.

> + //  ensure() tests something that shouldn't fail, doesn't abort like assert()
> +
> +#define ensure(test_number, expr)					    \
> +	((expr) ? (test_verbose < 2 ||			    		    \
> +		   (print_prefix(test_number),				    \
> +		    printf("%s(): true: " #expr " \n", __FUNCTION__)))	    \
> +		: (++ensure_failures,					    \
> +		   printf("test_number = %d: %s(): false: "		    \
> +			  #expr " : failed\n",				    \
> +			  test_number, __FUNCTION__)))
We use tst_res(TINFO, ...) instead of printf.
We try to avoid macros.

> +int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov)
> +{
> +	size_t iov_max = iovec_max();
> +	int salt = tn + subtest_number();	// salt test data
> +
> +	int client_data[niov];
> +	iovec_t client_iov[niov];
> +	for (int i = 0; i < niov; ++i) {
int i should be defined before.

> +		client_data[i] = i + salt;
> +		iovec_init(&client_iov[i], &client_data[i],
> +			   sizeof(client_data[0]));

...


Kind regards,
Petr

[1] https://patchwork.ozlabs.org/patch/991864/
[2] http://lists.linux.it/pipermail/ltp/2018-November/009781.html
[3] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text
[4] https://github.com/linux-test-project/ltp/pull/414
[5] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2-writing-a-testcase)
[6] https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial

[LTP] recvmmsg(2) system call tests

[Ramon Pantin]

Please see below. Before I email another patch please advise about my response below to each one of your individual items of feedback.

> On Nov 1, 2018, at 4:38 AM, Petr Vorel <pvorel@suse.cz> wrote:
> 
> Hi Ramon,
> 
> thank you for moving from github pull request to mailing list.
> 
>> Per Rafael's earlier email about how to proceed, find attached the path with
>> the test cases.
> You sent your patch as an attachment.
> Please, next time, send is at plain text,
> as although patchwork can work with that [1], mailman can't handle it [2].
> Here are some instructions [3].
> Please add your Signed-off-by: tag.

I am still confused where it is that you want that to be added and what is it that it is supposed to contain?

Should I have as the first line of the patch a line that says?
	Signed-off-by: pantin@google.com
If not what should it be?

> 
> I've added some comments in github pull request [4], you didn't change them, so
> I repeat some of it here. But please, read the doc to provide usable patch [5] [6].
> 2165 lines for one file suggest to split file into several.

That is unnecessary, it would then require a header file. Its needless complexity.

> 
> Please have look into the to see code style (short code, easy to read, using
> SAFE_*() helpers to prevent reinventing a wheal, avoiding macros, remove comments
> which has nothing to say, ...).
> 
> LTP code varies, some is really old and ugly. But we try to include reasonably
> clean code. You can have look at this as a good example of code style:
> testcases/kernel/syscalls/inotify/inotify09.c
> 59e0a81c5 inotify07: Add test for kernel crash during event notification
> It has also TST_TEST_TCONF
> 
>> From dea04e5946e8a30787943f347cf3861a8dca6008 Mon Sep 17 00:00:00 2001
>> From: Ramon Pantin <pantin@google.com>
>> Date: Wed, 31 Oct 2018 16:26:50 -0700
>> Subject: [PATCH] recvmmsg(2) test cases
> This should be removed.

In an earlier email you asked me to use this tool:
	git format-patch 
Which I used, which put that in the patch file. Confused. What harm does that cause?

In any case I will delete those lines. You mean delete the 4 lines right?

> 
>> ---
>> testcases/kernel/syscalls/recvmmsg/Makefile   |   27 +
>> .../kernel/syscalls/recvmmsg/recvmmsg01.c     | 2165 +++++++++++++++++
>> 2 files changed, 2192 insertions(+)
>> create mode 100644 testcases/kernel/syscalls/recvmmsg/Makefile
>> create mode 100644 testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
> 
>> diff --git a/testcases/kernel/syscalls/recvmmsg/Makefile b/testcases/kernel/syscalls/recvmmsg/Makefile
>> new file mode 100644
>> index 000000000..61b3a55e6
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/recvmmsg/Makefile
>> @@ -0,0 +1,27 @@
>> +#  Copyright (c) Google LLC, 2018
>> +#  Copyright (c) International Business Machines  Corp., 2001
> ^ Please delete this IBM copyright.

As I said I said in an earlier email I copied the Makefile from an IBM Makefile that had that copyright.
I will find another one to copy from that uses the SPDX license identifier instead and copy from that.

> 
>> +#  This program is free software;  you can redistribute it and/or modify
>> +#  it under the terms of the GNU General Public License as published by
>> +#  the Free Software Foundation; either version 2 of the License, or
>> +#  (at your option) any later version.
>> +#
>> +#  This program is distributed in the hope that it will be useful,
>> +#  but WITHOUT ANY WARRANTY;  without even the implied warranty of
>> +#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>> +#  the GNU General Public License for more details.
>> +#
>> +#  You should have received a copy of the GNU General Public License
>> +#  along with this program;  if not, write to the Free Software
>> +#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> Please use just SPDX identifier instead of address:
> # SPDX-License-Identifier: GPL-2.0-or-later
> 
>> +#
>> +
>> +top_srcdir		?= ../../../..
>> +
>> +include $(top_srcdir)/include/mk/testcases.mk
>> +
>> +CPPFLAGS		+= -I$(abs_srcdir)/../utils
> Why this include?

That was there in the file from the test case that I copied. I guess that was needed when using the original “test.h” (which in your earlier email you said was obsolete) so that came from that. I’ll see if it builds removing that.

> 
>> +CFLAGS			= -g -std=c99
> Please can you remove CFLAGS and produce code without warnings?

To be able to use ANSI C99 features on the builds from the continuous integration I have to have at least C99 enabled. The ANSI C99 standard is 19 years old, its silly to ask that the code be C89. I won’t change that. I’ll remove the “-g”

The test code uses variable declarations intermixed with statements, a feature of C99, reorganizing the code to be compatible with a 29 year old C standard would be silly.


> I'm also not sure about -std=c99. While you can grep for some -std=gnu99 in the
> code, these are just some outdated comments in the source, not passed to make.
> 
>> +LDLIBS			+= -lpthread
>> +
>> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
>> diff --git a/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
>> new file mode 100644
>> index 000000000..131bcfd66
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
>> @@ -0,0 +1,2165 @@
>> + //  Copyright (c) Google LLC, 2018
>> + //  SPDX-License-Identifier: GPL-2.0-or-later
> 
>> + //
>> + //{
> Why these lines?

To navigate better within the source code within various areas in it. You go to the line and press ‘%’ in vi or whatever you use to go to the matching curly-brace in your source code editor.

> 
>> +#define _GNU_SOURCE
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <stdbool.h>
>> +#include <unistd.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <pthread.h>
>> +#include <assert.h>
>> +#include <sys/types.h>
>> +#include <sys/time.h>
>> +#include <sys/signal.h>
>> +#include <sys/uio.h>
>> +#include <sys/file.h>
>> +#include <sys/socket.h>
>> +#include <sys/un.h>
>> +#include <netinet/in.h>
> Some of the includes will not be needed if you use API functions (SAFE_*, [5], [6]).

I’d rather base the test cases on standard UNIX system calls (Linux is mostly UNIX compliant), there is no value in using non standard APIs which I don’t want to have to bother learning about.

> 
>> +
>> +// #define RECVMMSG_USE_SCTP
>> +#ifdef RECVMMSG_USE_SCTP
>> +#include <netinet/sctp.h>
>> +#endif
> Code related to RECVMMSG_USE_SCTP should be checked with autotools. There is our header
> include/lapi/sctp.h, you can include it and add (in separate commit) definitions, which are needed.
> If you really need netinet/sctp.h header, you can add exit clause with TST_TEST_TCONF

I’ll address this later with a later patch. For now it causes no harm. Right?

> 
>> +
>> +#define TST_NO_DEFAULT_MAIN
> You're not supposed to use this
> 

Its there for a reason. I have my own main() program, main() is standard and part of C, don’t want to learn about some other stuff unrelated to what I am testing. In UNIX programs start in main() right?

>> +#include "tst_test.h"
>> +// #include "safe_macros.h"
> Please remove all comments left

I will remove that.

>> +
>> + //}
>> + // 	Declarations.
>> + //
>> + //{	<-- (curly-brace match to move through major sections of this file)
> Please comments like this.
> Please use C style comments /* */

C99 allows for // as comments. I don’t want to make the code bigger needlessly.

> 
>> +int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov);
>> +int con_receive_iovec_boundary_checks_peeking(con_t *conp, int tn, size_t niov);
>> +int con_receive_file_descriptor(con_t *conp, int tn, int cloexec_flag,
>> +				bool some_data, ssize_t controllen_delta);
>> +int con_message_too_long_to_fit_might_discard_bytes(con_t *conp, int tn,
>> +						    int type);
>> +int con_receive_waits_for_message(con_t *conp, int tn);
>> +int con_receiver_doesnt_wait_for_messages(con_t *conp, int tn);
>> +int con_receive_returns_what_is_available(con_t *conp, int tn);
>> +int con_empty_datagram_received_as_empty_datagram(con_t *conp, int tn);
> Function signatures aren't needed when used just in one file.
> Actually, in this case we declare functions as static.

The ARE required unless you reorganize you code to be all in the order of called functions preceding called functions, which is silly. Furthermore if there were mutually recursive functions you couldn’t do that either.

Having “static” for internal functions only matters if the test program is more than one source file. This is just one file. Its easier to debug when the symbols are visible to the debugger, with some compilers “static” functions can not be debugged because the symbols end up not being in the symbol table.

> 
>> + //  ensure() tests something that shouldn't fail, doesn't abort like assert()
>> +
>> +#define ensure(test_number, expr)					    \
>> +	((expr) ? (test_verbose < 2 ||			    		    \
>> +		   (print_prefix(test_number),				    \
>> +		    printf("%s(): true: " #expr " \n", __FUNCTION__)))	    \
>> +		: (++ensure_failures,					    \
>> +		   printf("test_number = %d: %s(): false: "		    \
>> +			  #expr " : failed\n",				    \
>> +			  test_number, __FUNCTION__)))
> We use tst_res(TINFO, ...) instead of printf.
> We try to avoid macros.

Because printf() is documented. You use macros when there is no other choice. For example when you need to stringify something for example with the #expr above, when you want to use __FUNCTION__, __FILE__, and __LINE__, etc.
> 
>> +int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov)
>> +{
>> +	size_t iov_max = iovec_max();
>> +	int salt = tn + subtest_number();	// salt test data
>> +
>> +	int client_data[niov];
>> +	iovec_t client_iov[niov];
>> +	for (int i = 0; i < niov; ++i) {
> int i should be defined before.

No it is not needed outside the loop.

> 
>> +		client_data[i] = i + salt;
>> +		iovec_init(&client_iov[i], &client_data[i],
>> +			   sizeof(client_data[0]));
> 
> …
> 

regards,
Ramon

> 
> Kind regards,
> Petr
> 
> [1] https://patchwork.ozlabs.org/patch/991864/
> [2] http://lists.linux.it/pipermail/ltp/2018-November/009781.html
> [3] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text
> [4] https://github.com/linux-test-project/ltp/pull/414
> [5] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2-writing-a-testcase)
> [6] https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20181101/98f6870a/attachment-0001.html>

[LTP] recvmmsg(2) system call tests

[Cyril Hrubis]

Hi!
> > You sent your patch as an attachment.
> > Please, next time, send is at plain text,
> > as although patchwork can work with that [1], mailman can't handle it [2].
> > Here are some instructions [3].
> > Please add your Signed-off-by: tag.
> 
> I am still confused where it is that you want that to be added and what is it that it is supposed to contain?
> 
> Should I have as the first line of the patch a line that says?
> 	Signed-off-by: pantin@google.com
> If not what should it be?

See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

The process for LTP is more or less the same as for the linux kernel so
most of it applies here as well.

> >> +CFLAGS			= -g -std=c99
> > Please can you remove CFLAGS and produce code without warnings?
> 
> To be able to use ANSI C99 features on the builds from the continuous integration I have to have at least C99 enabled. The ANSI C99 standard is 19 years old, its silly to ask that the code be C89. I won???t change that. I???ll remove the ???-g???
> 
> The test code uses variable declarations intermixed with statements, a feature of C99, reorganizing the code to be compatible with a 29 year old C standard would be silly.

Actually the default for gcc is gnu90 which is close to c99 anyways, so
we do not have to do anything about it.

> > I'm also not sure about -std=c99. While you can grep for some -std=gnu99 in the
> > code, these are just some outdated comments in the source, not passed to make.
> > 
> >> +LDLIBS			+= -lpthread
> >> +
> >> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> >> diff --git a/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
> >> new file mode 100644
> >> index 000000000..131bcfd66
> >> --- /dev/null
> >> +++ b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c
> >> @@ -0,0 +1,2165 @@
> >> + //  Copyright (c) Google LLC, 2018
> >> + //  SPDX-License-Identifier: GPL-2.0-or-later
> > 
> >> + //
> >> + //{
> > Why these lines?
> 
> To navigate better within the source code within various areas in it.
> You go to the line and press ???%??? in vi or whatever you use to go
> to the matching curly-brace in your source code editor.

Please don't do that.

> >> +#define _GNU_SOURCE
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <stdbool.h>
> >> +#include <unistd.h>
> >> +#include <string.h>
> >> +#include <errno.h>
> >> +#include <fcntl.h>
> >> +#include <pthread.h>
> >> +#include <assert.h>
> >> +#include <sys/types.h>
> >> +#include <sys/time.h>
> >> +#include <sys/signal.h>
> >> +#include <sys/uio.h>
> >> +#include <sys/file.h>
> >> +#include <sys/socket.h>
> >> +#include <sys/un.h>
> >> +#include <netinet/in.h>
> > Some of the includes will not be needed if you use API functions (SAFE_*, [5], [6]).
> 
> I???d rather base the test cases on standard UNIX system calls (Linux is mostly UNIX compliant), there is no value in using non standard APIs which I don???t want to have to bother learning about.

These are not non-standard APIs, these are just tiny wrappers around
standard UNIX apis that handle failures automatically, which greatly
simplifies the test code.

> >> +// #define RECVMMSG_USE_SCTP
> >> +#ifdef RECVMMSG_USE_SCTP
> >> +#include <netinet/sctp.h>
> >> +#endif
> > Code related to RECVMMSG_USE_SCTP should be checked with autotools. There is our header
> > include/lapi/sctp.h, you can include it and add (in separate commit) definitions, which are needed.
> > If you really need netinet/sctp.h header, you can add exit clause with TST_TEST_TCONF
> 
> I???ll address this later with a later patch. For now it causes no harm. Right?

Sure.

> >> +
> >> +#define TST_NO_DEFAULT_MAIN
> > You're not supposed to use this
> > 
> 
> Its there for a reason. I have my own main() program, main() is
> standard and part of C, don???t want to learn about some other stuff
> unrelated to what I am testing. In UNIX programs start in main()
> right?

If you are writing LTP test you are not supposed to define main().

Seriously you just need to write the function that does the test and
that's it. Plese do not fight the infrastructure that is there to help
you and to make your life easier.

> >> +int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov);
> >> +int con_receive_iovec_boundary_checks_peeking(con_t *conp, int tn, size_t niov);
> >> +int con_receive_file_descriptor(con_t *conp, int tn, int cloexec_flag,
> >> +				bool some_data, ssize_t controllen_delta);
> >> +int con_message_too_long_to_fit_might_discard_bytes(con_t *conp, int tn,
> >> +						    int type);
> >> +int con_receive_waits_for_message(con_t *conp, int tn);
> >> +int con_receiver_doesnt_wait_for_messages(con_t *conp, int tn);
> >> +int con_receive_returns_what_is_available(con_t *conp, int tn);
> >> +int con_empty_datagram_received_as_empty_datagram(con_t *conp, int tn);
> > Function signatures aren't needed when used just in one file.
> > Actually, in this case we declare functions as static.
> 
> The ARE required unless you reorganize you code to be all in the order of called functions preceding called functions, which is silly. Furthermore if there were mutually recursive functions you couldn???t do that either.
> 
> Having ???static??? for internal functions only matters if the test program is more than one source file. This is just one file. Its easier to debug when the symbols are visible to the debugger, with some compilers ???static??? functions can not be debugged because the symbols end up not being in the symbol table.

Actually neither of that is true.

Having static defined tells the compiler that the function is used only
in the local file, which can for example tell you that some of the
functions are unused.

> >> + //  ensure() tests something that shouldn't fail, doesn't abort like assert()
> >> +
> >> +#define ensure(test_number, expr)					    \
> >> +	((expr) ? (test_verbose < 2 ||			    		    \
> >> +		   (print_prefix(test_number),				    \
> >> +		    printf("%s(): true: " #expr " \n", __FUNCTION__)))	    \
> >> +		: (++ensure_failures,					    \
> >> +		   printf("test_number = %d: %s(): false: "		    \
> >> +			  #expr " : failed\n",				    \
> >> +			  test_number, __FUNCTION__)))
> > We use tst_res(TINFO, ...) instead of printf.
> > We try to avoid macros.
> 
> Because printf() is documented. You use macros when there is no other choice. For example when you need to stringify something for example with the #expr above, when you want to use __FUNCTION__, __FILE__, and __LINE__, etc.

Plase do not use macros like that also the __FILE__ and __LINE__ is
printed by the tst_res() itself. Please do not reinvent the wheel and
use the standard LTP reporting API.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] recvmmsg(2) system call tests

[Ramon Pantin]

Cyril,

Can you help me understand why you don't want to allow this test case to 
make use of the C99 programming language?

It is only a 19 year old standard.

If there is a problem with C99 use, can you indicate that the problem is?

thank you,
Ramon

On 11/02/18 03:52, Cyril Hrubis wrote:
> Actually the default for gcc is gnu90 which is close to c99 anyways, so
> we do not have to do anything about it.

[LTP] recvmmsg(2) system call tests

[Ramon Pantin]

Pardon the typo, can you indicate *what* the problem is with using C99.

On 11/03/18 15:16, Ramon Pantin wrote:
> Cyril,
>
> Can you help me understand why you don't want to allow this test case 
> to make use of the C99 programming language?
>
> It is only a 19 year old standard.
>
> If there is a problem with C99 use, can you indicate that the problem is?
>
> thank you,
> Ramon
>
> On 11/02/18 03:52, Cyril Hrubis wrote:
>> Actually the default for gcc is gnu90 which is close to c99 anyways, so
>> we do not have to do anything about it.
>

[LTP] recvmmsg(2) system call tests

[Ramon Pantin]

Hi Cyril,

About this statemement that you made:

    "Function signatures aren't needed when used just in one file." 

To whch I replied:

    "ARE required unless you reorganize you code to be all in the order
    of called functions preceding called functions"

To which you replied:

    "Actually neither of that is true"

If you don't use prototypes you do have to reorganize the functions in 
the order they are called.

Are you really questioning that? I don't want to have to explain that 
unless you don't understand why prototypes are required in some cases.

If you are not questioning that. Are you ok with me using prototypes?

I don't mind adding static, but do realize that that will indeed mean 
that in some compilation systems the sysmbols won't be in the symbol 
table and will make debugging harder. I don't care if you question this 
statment, don't have time to explain that.

regards,
Ramon Pantin

On 11/02/18 03:52, Cyril Hrubis wrote:
>>>> +int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov);
>>>> +int con_receive_iovec_boundary_checks_peeking(con_t *conp, int tn, size_t niov);
>>>> +int con_receive_file_descriptor(con_t *conp, int tn, int cloexec_flag,
>>>> +				bool some_data, ssize_t controllen_delta);
>>>> +int con_message_too_long_to_fit_might_discard_bytes(con_t *conp, int tn,
>>>> +						    int type);
>>>> +int con_receive_waits_for_message(con_t *conp, int tn);
>>>> +int con_receiver_doesnt_wait_for_messages(con_t *conp, int tn);
>>>> +int con_receive_returns_what_is_available(con_t *conp, int tn);
>>>> +int con_empty_datagram_received_as_empty_datagram(con_t *conp, int tn);
>>> Function signatures aren't needed when used just in one file.
>>> Actually, in this case we declare functions as static.
>> The ARE required unless you reorganize you code to be all in the order of called functions preceding called functions, which is silly. Furthermore if there were mutually recursive functions you couldn???t do that either.
>>
>> Having ???static??? for internal functions only matters if the test program is more than one source file. This is just one file. Its easier to debug when the symbols are visible to the debugger, with some compilers ???static??? functions can not be debugged because the symbols end up not being in the symbol table.
> Actually neither of that is true.
>
> Having static defined tells the compiler that the function is used only
> in the local file, which can for example tell you that some of the
> functions are unused.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20181103/52884ba9/attachment.html>

[LTP] recvmmsg(2) system call tests

[Cyril Hrubis]

Hi!
> Can you help me understand why you don't want to allow this test case to 
> make use of the C99 programming language?
> 
> It is only a 19 year old standard.
> 
> If there is a problem with C99 use, can you indicate that the problem is?

What I'm trying to say is:

* Reasonable recent compilers support c99 features without -c99
  explicitelly passed on command line

* It's a very bad idea to hardcode switches in random leaf Makefiles
  as from maintenance point of view this is real disaster

If we ever want to stick to some C standard it has to be a top level
decision, but honestly I do prefer the compiler defaults.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] recvmmsg(2) system call tests

[Cyril Hrubis]

Hi!
> About this statemement that you made:
> 
>     "Function signatures aren't needed when used just in one file." 
> 
> To whch I replied:
> 
>     "ARE required unless you reorganize you code to be all in the order
>     of called functions preceding called functions"
> 
> To which you replied:
> 
>     "Actually neither of that is true"

I actually answered only the part about 'static' being useless. Sorry
for not being clear on that.

> If you don't use prototypes you do have to reorganize the functions in 
> the order they are called.

That is indeed true.

> Are you really questioning that? I don't want to have to explain that 
> unless you don't understand why prototypes are required in some cases.

Not at all.

> If you are not questioning that. Are you ok with me using prototypes?

I do prefer to organize tests so that we don't have to use them, but I
can live with code that does not follow that as well.

> I don't mind adding static, but do realize that that will indeed mean 
> that in some compilation systems the sysmbols won't be in the symbol 
> table and will make debugging harder. I don't care if you question this 
> statment, don't have time to explain that.

For modern enough compilers and debuggers this is not true. Ancient
tooling may loose some debugging information when functions are inlined
though.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] recvmmsg(2) system call tests

[Ramon Pantin]

Ok, so my test cases are using arrays on the stack dimensioned at 
run-time based on other values computed at run time. C99 is needed for 
that. I don't see any value in changing them to use alloca(), its a 
waste of time.

Is it really going to be an issue if I use a feature of a 19 year old 
language?


On 11/05/2018 01:40 AM, Cyril Hrubis wrote:
> Hi!
>> Can you help me understand why you don't want to allow this test case to
>> make use of the C99 programming language?
>>
>> It is only a 19 year old standard.
>>
>> If there is a problem with C99 use, can you indicate that the problem is?
> What I'm trying to say is:
>
> * Reasonable recent compilers support c99 features without -c99
>    explicitelly passed on command line
>
> * It's a very bad idea to hardcode switches in random leaf Makefiles
>    as from maintenance point of view this is real disaster
>
> If we ever want to stick to some C standard it has to be a top level
> decision, but honestly I do prefer the compiler defaults.
>

[LTP] recvmmsg(2) system call tests

[Cyril Hrubis]

Hi!
> Ok, so my test cases are using arrays on the stack dimensioned at 
> run-time based on other values computed at run time. C99 is needed for 
> that. I don't see any value in changing them to use alloca(), its a 
> waste of time.

That is not true, at least for gcc variable arrays have been supported
as an extension in the default gnu89 mode for years now. I think that it
even predates the c99 standard, which is what I'm trying to explain for
quite some time now.

The only difference I'm aware of is how inline keyword behaves between
c99 and gnu89 mode, there may be some other minor differencies though.

The C accepted by default by a modern (10 years old+) compilers is much
closer than c99 than to the original c89.

Can we please stop this useless arguemnts and focus on the actual code
now?

-- 
Cyril Hrubis
chrubis@suse.cz