[dpdk-dev,v4,01/23] lib/librte_eal: import libbsd strlcpy
Checks
Commit Message
Signed-off-by: Andy Green <andy@warmcat.com>
---
lib/librte_eal/common/eal_common_string_fns.c | 34 ++++++++++++++++++++++++
lib/librte_eal/common/include/rte_string_fns.h | 7 +----
2 files changed, 36 insertions(+), 5 deletions(-)
Comments
On Mon, May 14, 2018 at 01:09:32PM +0800, Andy Green wrote:
> Signed-off-by: Andy Green <andy@warmcat.com>
> ---
> lib/librte_eal/common/eal_common_string_fns.c | 34 ++++++++++++++++++++++++
> lib/librte_eal/common/include/rte_string_fns.h | 7 +----
> 2 files changed, 36 insertions(+), 5 deletions(-)
>
While I'm aware this was suggested by other reviewers, I really don't feel
that it is necessary to actually import the code. If libbsd is present on
the system, we will use it directly. If libbsd is not present, the snprintf
provides an acceptable fallback for strlcpy IMHO. Having the full function
without good justification seems excessive.
/Bruce
On 05/17/2018 06:36 PM, Bruce Richardson wrote:
> On Mon, May 14, 2018 at 01:09:32PM +0800, Andy Green wrote:
>> Signed-off-by: Andy Green <andy@warmcat.com>
>> ---
>> lib/librte_eal/common/eal_common_string_fns.c | 34 ++++++++++++++++++++++++
>> lib/librte_eal/common/include/rte_string_fns.h | 7 +----
>> 2 files changed, 36 insertions(+), 5 deletions(-)
>>
>
> While I'm aware this was suggested by other reviewers, I really don't feel
> that it is necessary to actually import the code. If libbsd is present on
> the system, we will use it directly. If libbsd is not present, the snprintf
> provides an acceptable fallback for strlcpy IMHO. Having the full function
> without good justification seems excessive.
Well, as you can probably guess, I don't really mind either way.
This also implies another patch to export rte_strlcpy since it's no
longer an inline in the headers this way.
I removed these patches and rebuilt dpdk and then lagopus without it
with the idea of pasting the compile error. But I can't reproduce the
original problem... since then I rebased on current master dpdk, got
updated to gcc 8.1 and added more patches on lagopus.
So just drop this patch if you don't want the bsd lstrcpy.
-Andy
> /Bruce
>
On Thu, May 17, 2018 at 08:35:21PM +0800, Andy Green wrote:
>
>
> On 05/17/2018 06:36 PM, Bruce Richardson wrote:
> > On Mon, May 14, 2018 at 01:09:32PM +0800, Andy Green wrote:
> > > Signed-off-by: Andy Green <andy@warmcat.com>
> > > ---
> > > lib/librte_eal/common/eal_common_string_fns.c | 34 ++++++++++++++++++++++++
> > > lib/librte_eal/common/include/rte_string_fns.h | 7 +----
> > > 2 files changed, 36 insertions(+), 5 deletions(-)
> > >
> >
> > While I'm aware this was suggested by other reviewers, I really don't feel
> > that it is necessary to actually import the code. If libbsd is present on
> > the system, we will use it directly. If libbsd is not present, the snprintf
> > provides an acceptable fallback for strlcpy IMHO. Having the full function
> > without good justification seems excessive.
>
> Well, as you can probably guess, I don't really mind either way.
>
> This also implies another patch to export rte_strlcpy since it's no longer
> an inline in the headers this way.
>
> I removed these patches and rebuilt dpdk and then lagopus without it with
> the idea of pasting the compile error. But I can't reproduce the original
> problem... since then I rebased on current master dpdk, got updated to gcc
> 8.1 and added more patches on lagopus.
>
> So just drop this patch if you don't want the bsd lstrcpy.
>
Yes, let's drop it from the set for now anyway, if it's not solving any
specific error. We can relook at it in 18.08 anyway.
/Bruce
On 05/17/2018 08:56 PM, Bruce Richardson wrote:
> On Thu, May 17, 2018 at 08:35:21PM +0800, Andy Green wrote:
>>
>>
>> On 05/17/2018 06:36 PM, Bruce Richardson wrote:
>>> On Mon, May 14, 2018 at 01:09:32PM +0800, Andy Green wrote:
>>>> Signed-off-by: Andy Green <andy@warmcat.com>
>>>> ---
>>>> lib/librte_eal/common/eal_common_string_fns.c | 34 ++++++++++++++++++++++++
>>>> lib/librte_eal/common/include/rte_string_fns.h | 7 +----
>>>> 2 files changed, 36 insertions(+), 5 deletions(-)
>>>>
>>>
>>> While I'm aware this was suggested by other reviewers, I really don't feel
>>> that it is necessary to actually import the code. If libbsd is present on
>>> the system, we will use it directly. If libbsd is not present, the snprintf
>>> provides an acceptable fallback for strlcpy IMHO. Having the full function
>>> without good justification seems excessive.
>>
>> Well, as you can probably guess, I don't really mind either way.
>>
>> This also implies another patch to export rte_strlcpy since it's no longer
>> an inline in the headers this way.
>>
>> I removed these patches and rebuilt dpdk and then lagopus without it with
>> the idea of pasting the compile error. But I can't reproduce the original
>> problem... since then I rebased on current master dpdk, got updated to gcc
>> 8.1 and added more patches on lagopus.
>>
>> So just drop this patch if you don't want the bsd lstrcpy.
>>
> Yes, let's drop it from the set for now anyway, if it's not solving any
> specific error. We can relook at it in 18.08 anyway.
Sorry to immediately contradict myself but I forgot a step in the rather
complicated flow to force the lagopus dpdk submodule HEAD... by default
making lagopus forces the submodule commit to something else. It does
need a much smaller one-liner to avoid
In file included from ./dpdk/dpdk.c:88:
/projects/lagopus/src/dpdk/build/include/rte_string_fns.h: In
function 'rte_strlcpy':
/projects/lagopus/src/dpdk/build/include/rte_string_fns.h:58:9:
warning: conversion to 'size_t' {aka 'long unsigned int'} from
'int' may change the sign of the result [-Wsign-conversion]
return snprintf(dst, size, "%s", src);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It just needs a cast to make the return type of the snprintf compatible
with the expected return type of strlcpy().
I'll include this in the next send in an hour or two.
-Andy
> /Bruce
>
On Thu, May 17, 2018 at 09:00:25PM +0800, Andy Green wrote:
>
>
> On 05/17/2018 08:56 PM, Bruce Richardson wrote:
> > On Thu, May 17, 2018 at 08:35:21PM +0800, Andy Green wrote:
> > >
> > >
> > > On 05/17/2018 06:36 PM, Bruce Richardson wrote:
> > > > On Mon, May 14, 2018 at 01:09:32PM +0800, Andy Green wrote:
> > > > > Signed-off-by: Andy Green <andy@warmcat.com>
> > > > > ---
> > > > > lib/librte_eal/common/eal_common_string_fns.c | 34 ++++++++++++++++++++++++
> > > > > lib/librte_eal/common/include/rte_string_fns.h | 7 +----
> > > > > 2 files changed, 36 insertions(+), 5 deletions(-)
> > > > >
> > > >
> > > > While I'm aware this was suggested by other reviewers, I really don't feel
> > > > that it is necessary to actually import the code. If libbsd is present on
> > > > the system, we will use it directly. If libbsd is not present, the snprintf
> > > > provides an acceptable fallback for strlcpy IMHO. Having the full function
> > > > without good justification seems excessive.
> > >
> > > Well, as you can probably guess, I don't really mind either way.
> > >
> > > This also implies another patch to export rte_strlcpy since it's no longer
> > > an inline in the headers this way.
> > >
> > > I removed these patches and rebuilt dpdk and then lagopus without it with
> > > the idea of pasting the compile error. But I can't reproduce the original
> > > problem... since then I rebased on current master dpdk, got updated to gcc
> > > 8.1 and added more patches on lagopus.
> > >
> > > So just drop this patch if you don't want the bsd lstrcpy.
> > >
> > Yes, let's drop it from the set for now anyway, if it's not solving any
> > specific error. We can relook at it in 18.08 anyway.
>
> Sorry to immediately contradict myself but I forgot a step in the rather
> complicated flow to force the lagopus dpdk submodule HEAD... by default
> making lagopus forces the submodule commit to something else. It does need
> a much smaller one-liner to avoid
>
> In file included from ./dpdk/dpdk.c:88:
> /projects/lagopus/src/dpdk/build/include/rte_string_fns.h: In
> function 'rte_strlcpy':
> /projects/lagopus/src/dpdk/build/include/rte_string_fns.h:58:9:
> warning: conversion to 'size_t' {aka 'long unsigned int'} from
> 'int' may change the sign of the result [-Wsign-conversion]
> return snprintf(dst, size, "%s", src);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> It just needs a cast to make the return type of the snprintf compatible with
> the expected return type of strlcpy().
>
> I'll include this in the next send in an hour or two.
>
Great, thanks for the all the patches!
@@ -38,3 +38,37 @@ rte_strsplit(char *string, int stringlen,
errno = EINVAL;
return -1;
}
+
+/*
+ * Copyright (c) 1998 Todd C. Miller <Todd.Miller@courtesan.com>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ */
+
+size_t
+rte_strlcpy(char *dst, const char *src, size_t siz)
+{
+ char *d = dst;
+ const char *s = src;
+ size_t n = siz;
+
+ /* Copy as many bytes as will fit */
+ if (n != 0) {
+ while (--n != 0) {
+ if ((*d++ = *s++) == '\0')
+ break;
+ }
+ }
+
+ /* Not enough room in dst, add NUL and traverse rest of src */
+ if (n == 0) {
+ if (siz != 0)
+ *d = '\0'; /* NUL-terminate dst */
+ while (*s++)
+ ;
+ }
+
+ return(s - src - 1); /* count does not include NUL */
+}
@@ -52,11 +52,8 @@ rte_strsplit(char *string, int stringlen,
* DPDK-specific version of strlcpy for systems without
* libc or libbsd copies of the function
*/
-static inline size_t
-rte_strlcpy(char *dst, const char *src, size_t size)
-{
- return snprintf(dst, size, "%s", src);
-}
+size_t
+rte_strlcpy(char *dst, const char *src, size_t size);
/* pull in a strlcpy function */
#ifdef RTE_EXEC_ENV_BSDAPP