On Fri, Jun 2, 2017 at 12:09 PM, Dmitry Eremin-Solenikov
<dmitry.ereminsoleni...@linaro.org> wrote:
> On 02.06.2017 18:34, Brian Brooks wrote:
>> On 06/02 10:39:18, Dmitry Eremin-Solenikov wrote:
>>> On 01.06.2017 22:05, Brian Brooks wrote:
>>>> Signed-off-by: Brian Brooks <brian.bro...@arm.com>
>>>> Reviewed-by: Ola Liljedahl <ola.liljed...@arm.com>
>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
>>>> ---
>>>> configure.ac | 5 +++++
>>>> platform/linux-generic/m4/configure.m4 | 4 ++++
>>>> platform/linux-generic/pktio/ipc.c | 6 ++++--
>>>> platform/linux-generic/pktio/sysfs.c | 2 +-
>>>> test/common_plat/validation/api/pktio/pktio.c | 4 +++-
>>>> 5 files changed, 17 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/configure.ac b/configure.ac
>>>> index 7569ebe0..5eabe4d4 100644
>>>> --- a/configure.ac
>>>> +++ b/configure.ac
>>>> @@ -300,6 +300,11 @@ ODP_CFLAGS="$ODP_CFLAGS -Wmissing-declarations
>>>> -Wold-style-definition -Wpointer-
>>>> ODP_CFLAGS="$ODP_CFLAGS -Wcast-align -Wnested-externs -Wcast-qual
>>>> -Wformat-nonliteral"
>>>> ODP_CFLAGS="$ODP_CFLAGS -Wformat-security -Wundef -Wwrite-strings"
>>>> ODP_CFLAGS="$ODP_CFLAGS -std=c99"
>>>> +
>>>> +if test "${CC}" == "gcc" -a ${CC_VERSION_MAJOR} -ge 7; then
>>>> + ODP_CFLAGS="$ODP_CFLAGS -Wimplicit-fallthrough=0"
>>>> +fi
>>>> +
>>>
>>> Shouldn't Wimplicit-fallthrough=2 be enough? Where are you hitting the
>>> warning?
>>
>> Not every fallthrough is commented.
>>
>> Please read the manual if you would like to know more:
>> https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Warning-Options.html#Warning-Options
>
> So, it would be better to add necessary comments in my opinion.
> The warning is useful.
>
>>>> # Extra flags for example to suppress certain warning types
>>>> ODP_CFLAGS="$ODP_CFLAGS $ODP_CFLAGS_EXTRA"
>>>>
>>>> diff --git a/platform/linux-generic/m4/configure.m4
>>>> b/platform/linux-generic/m4/configure.m4
>>>> index a2a25408..3e2978b5 100644
>>>> --- a/platform/linux-generic/m4/configure.m4
>>>> +++ b/platform/linux-generic/m4/configure.m4
>>>> @@ -28,6 +28,10 @@ AC_LINK_IFELSE(
>>>> echo "Use newer version. For gcc > 4.7.0"
>>>> exit -1)
>>>>
>>>> +if test "${CC}" == "gcc" -a ${CC_VERSION_MAJOR} -ge 7; then
>>>> + AM_LDFLAGS="$AM_LDFLAGS -latomic"
>>>> +fi
>>>> +
>>>
>>> This should be replaced with proper AC_CHECK_LIB or AC_SEARCH_LIBS
>>
>> I don't think so. The link to libatomic is needed based on the compiler
>> version, not based on whether a program compiles with -latomic or not
>> which is AC_CHECK_LIB behavior. If you disagree, please show me how it
>> can be done.
>>
>> This is also a very simple (3 line) solution.
>
> Simple solution:
>
> AC_SEARCH_LIBS([your_atomic_func], [atomic])
>
> Cleaner solution:
>
> AC_LINK_IFELSE([AC_LANG_CALL([], [your_atomic_func])], [ATOMIC_LIBS=""],
> [OLDLIBS=$LIBS
> LIBS="$LIBS -latomic"
> AC_LINK_IFELSE([AC_LANG_CALL([], [your_atomic_func])],
> [ATOMIC_LIBS="-latomic"],
> [AC_MSG_FAILURE([your_atomic_func is not available])])
> LIBS=$OLDLIBS])
> AC_SUBST([ATOMIC_LIBS])
>
> Then you can use $(ATOMIC_LIBS) when you need to use your_atomic_function().
>
>
>>>> m4_include([platform/linux-generic/m4/odp_pthread.m4])
>>>> m4_include([platform/linux-generic/m4/odp_openssl.m4])
>>>> m4_include([platform/linux-generic/m4/odp_pcap.m4])
>>>> diff --git a/platform/linux-generic/pktio/ipc.c
>>>> b/platform/linux-generic/pktio/ipc.c
>>>> index 06175e5a..29c3a546 100644
>>>> --- a/platform/linux-generic/pktio/ipc.c
>>>> +++ b/platform/linux-generic/pktio/ipc.c
>>>> @@ -694,8 +694,10 @@ static int ipc_close(pktio_entry_t *pktio_entry)
>>>>
>>>> if (sscanf(dev, "ipc:%d:%s", &pid, tail) == 2)
>>>> snprintf(name, sizeof(name), "ipc:%s", tail);
>>>> - else
>>>> - snprintf(name, sizeof(name), "%s", dev);
>>>> + else {
>>>> + strncpy(name, dev, sizeof(name));
>>>> + name[sizeof(name) - 1] = '\0';
>>>> + }
>>>
>>> Why?
>>
>> New -Wformat-truncation=level behavior. Please read the manual if you'd like
>> to know more.
>
> I'd suggest instead to disable -Wformat-truncation.
>
> Related question: why do we have PKTIO_NAME_LEN of 256?
The reason for the length is the syntax supported by the pcap driver
(see pktio/pcap.c):
* PCAP pktio type
*
* This file provides a pktio interface that allows for reading from
* and writing to pcap capture files. It is intended to be used as
* simple way of injecting test packets into an application for the
* purpose of functional testing.
*
* To use this interface the name passed to odp_pktio_open() must begin
* with "pcap:" and be in the format;
*
* pcap:in=test.pcap:out=test_out.pcap:loops=10
With file path names, this can easily grow "long" so 256 was picked as
a round number that should accommodate most "reasonable" pcap paths.
>
>>>>
>>>> /* unlink this pktio info for both master and slave */
>>>> odp_shm_free(pktio_entry->s.ipc.pinfo_shm);
>
> --
> With best wishes
> Dmitry
