<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Please see below. Before I email another patch please advise about my response below to each one of your individual items of feedback.<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Nov 1, 2018, at 4:38 AM, Petr Vorel <<a href="mailto:pvorel@suse.cz" class="">pvorel@suse.cz</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hi Ramon,<br class=""><br class="">thank you for moving from github pull request to mailing list.<br class=""><br class=""><blockquote type="cite" class="">Per Rafael's earlier email about how to proceed, find attached the path with<br class="">the test cases.<br class=""></blockquote>You sent your patch as an attachment.<br class="">Please, next time, send is at plain text,<br class="">as although patchwork can work with that [1], mailman can't handle it [2].<br class="">Here are some instructions [3].<br class="">Please add your Signed-off-by: tag.<br class=""></div></div></blockquote><div><br class=""></div><div>I am still confused where it is that you want that to be added and what is it that it is supposed to contain?</div><div><br class=""></div><div>Should I have as the first line of the patch a line that says?</div><div><span class="Apple-tab-span" style="white-space:pre">   </span>Signed-off-by: <a href="mailto:pantin@google.com" class="">pantin@google.com</a></div><div>If not what should it be?</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">I've added some comments in github pull request [4], you didn't change them, so<br class="">I repeat some of it here. But please, read the doc to provide usable patch [5] [6].<br class="">2165 lines for one file suggest to split file into several.<br class=""></div></div></blockquote><div><br class=""></div><div>That is unnecessary, it would then require a header file. Its needless complexity.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Please have look into the to see code style (short code, easy to read, using<br class="">SAFE_*() helpers to prevent reinventing a wheal, avoiding macros, remove comments<br class="">which has nothing to say, ...).<br class=""><br class="">LTP code varies, some is really old and ugly. But we try to include reasonably<br class="">clean code. You can have look at this as a good example of code style:<br class="">testcases/kernel/syscalls/inotify/inotify09.c<br class="">59e0a81c5 inotify07: Add test for kernel crash during event notification<br class="">It has also TST_TEST_TCONF<br class=""><br class=""><blockquote type="cite" class="">From dea04e5946e8a30787943f347cf3861a8dca6008 Mon Sep 17 00:00:00 2001<br class="">From: Ramon Pantin <<a href="mailto:pantin@google.com" class="">pantin@google.com</a>><br class="">Date: Wed, 31 Oct 2018 16:26:50 -0700<br class="">Subject: [PATCH] recvmmsg(2) test cases<br class=""></blockquote>This should be removed.<br class=""></div></div></blockquote><div><br class=""></div><div>In an earlier email you asked me to use this tool:</div><div><span class="Apple-tab-span" style="white-space:pre">  </span><code class="">git format-patch</code> </div><div>Which I used, which put that in the patch file. Confused. What harm does that cause?</div><div><br class=""></div><div>In any case I will delete those lines. You mean delete the 4 lines right?</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">---<br class=""> testcases/kernel/syscalls/recvmmsg/Makefile   |   27 +<br class=""> .../kernel/syscalls/recvmmsg/recvmmsg01.c     | 2165 +++++++++++++++++<br class=""> 2 files changed, 2192 insertions(+)<br class=""> create mode 100644 testcases/kernel/syscalls/recvmmsg/Makefile<br class=""> create mode 100644 testcases/kernel/syscalls/recvmmsg/recvmmsg01.c<br class=""></blockquote><br class=""><blockquote type="cite" class="">diff --git a/testcases/kernel/syscalls/recvmmsg/Makefile b/testcases/kernel/syscalls/recvmmsg/Makefile<br class="">new file mode 100644<br class="">index 000000000..61b3a55e6<br class="">--- /dev/null<br class="">+++ b/testcases/kernel/syscalls/recvmmsg/Makefile<br class="">@@ -0,0 +1,27 @@<br class="">+#  Copyright (c) Google LLC, 2018<br class="">+#  Copyright (c) International Business Machines  Corp., 2001<br class=""></blockquote>^ Please delete this IBM copyright.<br class=""></div></div></blockquote><div><br class=""></div><div>As I said I said in an earlier email I copied the Makefile from an IBM Makefile that had that copyright.</div><div>I will find another one to copy from that uses the SPDX license identifier instead and copy from that.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+#  This program is free software;  you can redistribute it and/or modify<br class="">+#  it under the terms of the GNU General Public License as published by<br class="">+#  the Free Software Foundation; either version 2 of the License, or<br class="">+#  (at your option) any later version.<br class="">+#<br class="">+#  This program is distributed in the hope that it will be useful,<br class="">+#  but WITHOUT ANY WARRANTY;  without even the implied warranty of<br class="">+#  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See<br class="">+#  the GNU General Public License for more details.<br class="">+#<br class="">+#  You should have received a copy of the GNU General Public License<br class="">+#  along with this program;  if not, write to the Free Software<br class="">+#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA<br class=""></blockquote>Please use just SPDX identifier instead of address:<br class=""># SPDX-License-Identifier: GPL-2.0-or-later<br class=""><br class=""><blockquote type="cite" class="">+#<br class="">+<br class="">+top_srcdir<span class="Apple-tab-span" style="white-space:pre">     </span><span class="Apple-tab-span" style="white-space:pre">    </span>?= ../../../..<br class="">+<br class="">+include $(top_srcdir)/include/mk/testcases.mk<br class="">+<br class="">+CPPFLAGS<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre">    </span>+= -I$(abs_srcdir)/../utils<br class=""></blockquote>Why this include?<br class=""></div></div></blockquote><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+CFLAGS<span class="Apple-tab-span" style="white-space:pre">      </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>= -g -std=c99<br class=""></blockquote>Please can you remove CFLAGS and produce code without warnings?<br class=""></div></div></blockquote><div><br class=""></div><div>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”</div><div><br class=""></div><div>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.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="">I'm also not sure about -std=c99. While you can grep for some -std=gnu99 in the<br class="">code, these are just some outdated comments in the source, not passed to make.<br class=""><br class=""><blockquote type="cite" class="">+LDLIBS<span class="Apple-tab-span" style="white-space:pre">   </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>+= -lpthread<br class="">+<br class="">+include $(top_srcdir)/include/mk/generic_leaf_target.mk<br class="">diff --git a/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c<br class="">new file mode 100644<br class="">index 000000000..131bcfd66<br class="">--- /dev/null<br class="">+++ b/testcases/kernel/syscalls/recvmmsg/recvmmsg01.c<br class="">@@ -0,0 +1,2165 @@<br class="">+ //  Copyright (c) Google LLC, 2018<br class="">+ //  SPDX-License-Identifier: GPL-2.0-or-later<br class=""></blockquote><br class=""><blockquote type="cite" class="">+ //<br class="">+ //{<br class=""></blockquote>Why these lines?<br class=""></div></div></blockquote><div><br class=""></div>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.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+#define _GNU_SOURCE<br class="">+#include <stdio.h><br class="">+#include <stdlib.h><br class="">+#include <stdbool.h><br class="">+#include <unistd.h><br class="">+#include <string.h><br class="">+#include <errno.h><br class="">+#include <fcntl.h><br class="">+#include <pthread.h><br class="">+#include <assert.h><br class="">+#include <sys/types.h><br class="">+#include <sys/time.h><br class="">+#include <sys/signal.h><br class="">+#include <sys/uio.h><br class="">+#include <sys/file.h><br class="">+#include <sys/socket.h><br class="">+#include <sys/un.h><br class="">+#include <netinet/in.h><br class=""></blockquote>Some of the includes will not be needed if you use API functions (SAFE_*, [5], [6]).<br class=""></div></div></blockquote><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+<br class="">+// #define RECVMMSG_USE_SCTP<br class="">+#ifdef RECVMMSG_USE_SCTP<br class="">+#include <netinet/sctp.h><br class="">+#endif<br class=""></blockquote>Code related to RECVMMSG_USE_SCTP should be checked with autotools. There is our header<br class="">include/lapi/sctp.h, you can include it and add (in separate commit) definitions, which are needed.<br class="">If you really need netinet/sctp.h header, you can add exit clause with TST_TEST_TCONF<br class=""></div></div></blockquote><div><br class=""></div><div>I’ll address this later with a later patch. For now it causes no harm. Right?</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+<br class="">+#define TST_NO_DEFAULT_MAIN<br class=""></blockquote>You're not supposed to use this<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div>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?</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class="">+#include "tst_test.h"<br class="">+// #include "safe_macros.h"<br class=""></blockquote>Please remove all comments left<br class=""></div></div></blockquote><div><br class=""></div><div>I will remove that.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class="">+<br class="">+ //}<br class="">+ // <span class="Apple-tab-span" style="white-space:pre">   </span>Declarations.<br class="">+ //<br class="">+ //{<span class="Apple-tab-span" style="white-space:pre">    </span><-- (curly-brace match to move through major sections of this file)<br class=""></blockquote>Please comments like this.<br class="">Please use C style comments /* */<br class=""></div></div></blockquote><div><br class=""></div><div>C99 allows for // as comments. I don’t want to make the code bigger needlessly.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov);<br class="">+int con_receive_iovec_boundary_checks_peeking(con_t *conp, int tn, size_t niov);<br class="">+int con_receive_file_descriptor(con_t *conp, int tn, int cloexec_flag,<br class="">+<span class="Apple-tab-span" style="white-space:pre">     </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>bool some_data, ssize_t controllen_delta);<br class="">+int con_message_too_long_to_fit_might_discard_bytes(con_t *conp, int tn,<br class="">+<span class="Apple-tab-span" style="white-space:pre">      </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>    int type);<br class="">+int con_receive_waits_for_message(con_t *conp, int tn);<br class="">+int con_receiver_doesnt_wait_for_messages(con_t *conp, int tn);<br class="">+int con_receive_returns_what_is_available(con_t *conp, int tn);<br class="">+int con_empty_datagram_received_as_empty_datagram(con_t *conp, int tn);<br class=""></blockquote>Function signatures aren't needed when used just in one file.<br class="">Actually, in this case we declare functions as static.<br class=""></div></div></blockquote><div><br class=""></div><div>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.</div><div><br class=""></div><div>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.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+ //  ensure() tests something that shouldn't fail, doesn't abort like assert()<br class="">+<br class="">+#define ensure(test_number, expr)<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span>((expr) ? (test_verbose < 2 ||<span class="Apple-tab-span" style="white-space:pre">   </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>    <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre">    </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span><span class="Apple-tab-span" style="white-space:pre">    </span>   (print_prefix(test_number),<span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span><span class="Apple-tab-span" style="white-space:pre">    </span>    printf("%s(): true: " #expr " \n", __FUNCTION__)))<span class="Apple-tab-span" style="white-space:pre">   </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span><span class="Apple-tab-span" style="white-space:pre">    </span>: (++ensure_failures,<span class="Apple-tab-span" style="white-space:pre">       </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span><span class="Apple-tab-span" style="white-space:pre">    </span>   printf("test_number = %d: %s(): false: "<span class="Apple-tab-span" style="white-space:pre">     </span><span class="Apple-tab-span" style="white-space:pre">    </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>  #expr " : failed\n",<span class="Apple-tab-span" style="white-space:pre">       </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>    \<br class="">+<span class="Apple-tab-span" style="white-space:pre">  </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>  test_number, __FUNCTION__)))<br class=""></blockquote>We use tst_res(TINFO, ...) instead of printf.<br class="">We try to avoid macros.<br class=""></div></div></blockquote><div><br class=""></div><div>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.</div><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+int con_receive_iovec_boundary_checks(con_t *conp, int tn, size_t niov)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>size_t iov_max = iovec_max();<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>int salt = tn + subtest_number();<span class="Apple-tab-span" style="white-space:pre">   </span>// salt test data<br class="">+<br class="">+<span class="Apple-tab-span" style="white-space:pre">       </span>int client_data[niov];<br class="">+<span class="Apple-tab-span" style="white-space:pre">        </span>iovec_t client_iov[niov];<br class="">+<span class="Apple-tab-span" style="white-space:pre">     </span>for (int i = 0; i < niov; ++i) {<br class=""></blockquote>int i should be defined before.<br class=""></div></div></blockquote><div><br class=""></div><div>No it is not needed outside the loop.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class=""><blockquote type="cite" class="">+<span class="Apple-tab-span" style="white-space:pre">     </span><span class="Apple-tab-span" style="white-space:pre">    </span>client_data[i] = i + salt;<br class="">+<span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>iovec_init(&client_iov[i], &client_data[i],<br class="">+<span class="Apple-tab-span" style="white-space:pre">   </span><span class="Apple-tab-span" style="white-space:pre">    </span><span class="Apple-tab-span" style="white-space:pre">    </span>   sizeof(client_data[0]));<br class=""></blockquote><br class="">…<br class=""><br class=""></div></div></blockquote><div><br class=""></div>regards,</div><div>Ramon<br class=""><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Kind regards,<br class="">Petr<br class=""><br class="">[1] <a href="https://patchwork.ozlabs.org/patch/991864/" class="">https://patchwork.ozlabs.org/patch/991864/</a><br class="">[2] <a href="http://lists.linux.it/pipermail/ltp/2018-November/009781.html" class="">http://lists.linux.it/pipermail/ltp/2018-November/009781.html</a><br class="">[3] <a href="https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text" class="">https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text</a><br class="">[4] <a href="https://github.com/linux-test-project/ltp/pull/414" class="">https://github.com/linux-test-project/ltp/pull/414</a><br class="">[5] <a href="https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2-writing-a-testcase" class="">https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2-writing-a-testcase</a>)<br class="">[6] <a href="https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial" class="">https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial</a><br class=""></div></div></blockquote></div><br class=""></body></html>