| Message ID | 1422975201-3364-2-git-send-email-ola.liljedahl@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 3 February 2015 at 16:40, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Ola Liljedahl >> Sent: Tuesday, February 03, 2015 4:53 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> odp_cpumask_to_str() return chars written or error >> >> Add define ODP_CPUMASK_BUFSIZE for minimum output buffer size for >> odp_cpumask_to_str(). >> odp_cpumask_to_str() takes output buffer size as input and returns number >> of chars written (on success), <0 on failure. >> Updated the implementation. >> Updated all usages in example and test programs. >> >> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> >> --- >> (This document/code contribution attached is provided under the terms of >> agreement LES-LTM-21309) >> >> example/generator/odp_generator.c | 4 ++-- >> example/ipsec/odp_ipsec.c | 4 ++-- >> example/l2fwd/odp_l2fwd.c | 4 ++-- >> example/packet/odp_pktio.c | 4 ++-- >> example/timer/odp_timer_test.c | 4 ++-- >> include/odp/api/cpumask.h | 23 ++++++++++++++--- >> -- >> .../linux-generic/include/odp/plat/cpumask_types.h | 5 +++++ >> platform/linux-generic/odp_cpumask.c | 26 +++++++++++++---- >> ----- >> test/api_test/odp_common.c | 4 ++-- >> test/performance/odp_scheduling.c | 4 ++-- >> 10 files changed, 51 insertions(+), 31 deletions(-) >> >> diff --git a/example/generator/odp_generator.c >> b/example/generator/odp_generator.c >> index 03dab50..2433cb6 100644 >> --- a/example/generator/odp_generator.c >> +++ b/example/generator/odp_generator.c >> @@ -545,7 +545,7 @@ int main(int argc, char *argv[]) >> int i; >> odp_shm_t shm; >> odp_cpumask_t cpumask; >> - char cpumaskstr[64]; >> + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; >> odp_pool_param_t params; >> >> /* Init ODP before calling anything else */ >> @@ -596,7 +596,7 @@ int main(int argc, char *argv[]) >> * Start mapping thread from CPU #1 >> */ >> num_workers = odph_linux_cpumask_default(&cpumask, num_workers); >> - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> >> printf("num worker threads: %i\n", num_workers); >> printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); >> diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c >> index 03f2419..869fd3a 100644 >> --- a/example/ipsec/odp_ipsec.c >> +++ b/example/ipsec/odp_ipsec.c >> @@ -1193,7 +1193,7 @@ main(int argc, char *argv[]) >> int stream_count; >> odp_shm_t shm; >> odp_cpumask_t cpumask; >> - char cpumaskstr[64]; >> + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; >> odp_pool_param_t params; >> >> /* Init ODP before calling anything else */ >> @@ -1242,7 +1242,7 @@ main(int argc, char *argv[]) >> * Start mapping thread from CPU #1 >> */ >> num_workers = odph_linux_cpumask_default(&cpumask, num_workers); >> - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> >> printf("num worker threads: %i\n", num_workers); >> printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); >> diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c >> index 7a520fb..e22ec0c 100644 >> --- a/example/l2fwd/odp_l2fwd.c >> +++ b/example/l2fwd/odp_l2fwd.c >> @@ -294,7 +294,7 @@ int main(int argc, char *argv[]) >> int num_workers; >> odp_shm_t shm; >> odp_cpumask_t cpumask; >> - char cpumaskstr[64]; >> + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; >> odp_pool_param_t params; >> >> /* Init ODP before calling anything else */ >> @@ -336,7 +336,7 @@ int main(int argc, char *argv[]) >> * Start mapping thread from CPU #1 >> */ >> num_workers = odph_linux_cpumask_default(&cpumask, num_workers); >> - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> >> printf("num worker threads: %i\n", num_workers); >> printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); >> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >> index 28ed98c..e38029a 100644 >> --- a/example/packet/odp_pktio.c >> +++ b/example/packet/odp_pktio.c >> @@ -287,7 +287,7 @@ int main(int argc, char *argv[]) >> int i; >> int cpu; >> odp_cpumask_t cpumask; >> - char cpumaskstr[64]; >> + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; >> odp_pool_param_t params; >> >> args = calloc(1, sizeof(args_t)); >> @@ -324,7 +324,7 @@ int main(int argc, char *argv[]) >> * Start mapping thread from CPU #1 >> */ >> num_workers = odph_linux_cpumask_default(&cpumask, num_workers); >> - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> >> printf("num worker threads: %i\n", num_workers); >> printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); >> diff --git a/example/timer/odp_timer_test.c >> b/example/timer/odp_timer_test.c >> index 0df041f..299ed89 100644 >> --- a/example/timer/odp_timer_test.c >> +++ b/example/timer/odp_timer_test.c >> @@ -317,7 +317,7 @@ int main(int argc, char *argv[]) >> odp_timer_pool_param_t tparams; >> odp_timer_pool_info_t tpinfo; >> odp_cpumask_t cpumask; >> - char cpumaskstr[64]; >> + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; >> >> printf("\nODP timer example starts\n"); >> >> @@ -358,7 +358,7 @@ int main(int argc, char *argv[]) >> * Start mapping thread from CPU #1 >> */ >> num_workers = odph_linux_cpumask_default(&cpumask, num_workers); >> - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); >> >> printf("num worker threads: %i\n", num_workers); >> printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); >> diff --git a/include/odp/api/cpumask.h b/include/odp/api/cpumask.h >> index 7482899..65969c3 100644 >> --- a/include/odp/api/cpumask.h >> +++ b/include/odp/api/cpumask.h >> @@ -18,12 +18,20 @@ >> extern "C" { >> #endif >> >> +#include <sys/types.h> >> +#include <odp/config.h> >> + >> /** @addtogroup odp_scheduler >> * CPU mask operations. >> * @{ >> */ >> >> - /** >> +/** >> + * @def ODP_CPUMASK_BUFSIZE >> + * Minimum size of output buffer for odp_cpumask_to_str() >> + */ > > ODP_CPUMASK_STRLEN is better. Better to use term string when it's a character string. > > >> + >> +/** >> * Add CPU mask bits from a string >> * >> * @param mask CPU mask to modify >> @@ -33,13 +41,16 @@ extern "C" { >> void odp_cpumask_from_str(odp_cpumask_t *mask, const char *str); >> >> /** >> - * Write CPU mask as a string of hexadecimal digits >> + * Format CPU mask as a string of hexadecimal digits >> + * >> + * @param mask CPU mask to format >> + * @param[out] buf Output buffer (use ODP_CPUMASK_BUFSIZE) >> + * @param bufsz Size of output buffer >> * >> - * @param mask CPU mask >> - * @param str String for output >> - * @param len Size of string length (incl. ending zero) >> + * @return number of characters written (including terminating null char) >> + * @retval <0 on failure (buffer too small) >> */ >> -void odp_cpumask_to_str(const odp_cpumask_t *mask, char *str, int len); >> +ssize_t odp_cpumask_to_str(const odp_cpumask_t *mask, char *buf, ssize_t >> bufsz); > > Don't rename parameters in this or other patches, if there's no strong reason for that from the return value cleanup point of view. Some of these patches (the first four) are not only about return values. This is clearly documented. For example, after applying this patch the odp_cpumask_from_str() above would have a parameter called "str", while here it's renamed to "buf". These are both string functions, and "str" describes the purpose of the parameter better. I don't view an uninitialized buffer as a string(e.g. can you call strlen() on it?). The purpose of this specific parameter is to specify a buffer that will be filled in with a string. But I will change it back. > > One patch series should fix one thing. If parameter renames are important, send another series for that. Please, remove all unnecessary parameter renames from the patch set. OK I will remove gratuitous name changes. Some will be still be kept in order to unify the parameter names that are used by a related set of functions (or where the names are semantically wrong (e.g. an uninitialized buffer has a size, not a length). > > -Petri > >
On 4 February 2015 at 09:38, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Savolainen, Petri (NSN - >> FI/Espoo) >> Sent: Wednesday, February 04, 2015 10:31 AM >> To: ext Ola Liljedahl >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> odp_cpumask_to_str() return chars written or error >> >> > >> diff --git a/include/odp/api/cpumask.h b/include/odp/api/cpumask.h >> > >> index 7482899..65969c3 100644 >> > >> --- a/include/odp/api/cpumask.h >> > >> +++ b/include/odp/api/cpumask.h >> > >> @@ -18,12 +18,20 @@ >> > >> extern "C" { >> > >> #endif >> > >> >> > >> +#include <sys/types.h> >> > >> +#include <odp/config.h> >> > >> + >> > >> /** @addtogroup odp_scheduler >> > >> * CPU mask operations. >> > >> * @{ >> > >> */ >> > >> >> > >> - /** >> > >> +/** >> > >> + * @def ODP_CPUMASK_BUFSIZE >> > >> + * Minimum size of output buffer for odp_cpumask_to_str() >> > >> + */ >> > > >> > > ODP_CPUMASK_STRLEN is better. Better to use term string when it's a >> > character string. But is it not a character string. The user specifies a buffer and a buffer size. The function writes a string into that buffer (if it fits into the buffer). The buffer then *contains* a string (which will be shorter that the buffer size value specified, e.g. because trailing CPU bits may not be set and also the string length does not include the terminating null char). But a string and a buffer are not the same thing. >> >> This one is missing from v5. >> >> Better name it STRLEN when it used for _to_str() and _from_str() function >> calls. I think this is a poor choice of name. The length of a string is the number of characters in the string (excluding the terminating null char). But we need to pass the buffer size. >> >> char mask_str[ODP_CPUMASK_STRLEN]; >> memset(mask_str, 0, sizeof(mask_str)); Why is the memset() here? It is not needed. >> odp_cpumask_to_str(mask, mask_str, sizeof(mask_str)); >> odp_cpumask_from_str(mask, mask_str); Yes this is how it is done, before my patch and after my patch. >> >> >> -Petri > > > Or ODP_CPUMASK_STRLEN_MAX or ODP_CPUMASK_STR_MAX_CHARS ... > > The point is that, it's defining the max number of chars (not bytes) needed for the string output - and it cannot be mixed with the odp_cpumask_t (buffer) size. odp_cpumask_t has a separate well-defined type (no char bufs or void pointers used when you are passing an odp_cpumask_t value), I see no risk for mixing it with ODP_CPUMASK_BUFSIZE. > > > -Petri > > > >
On 4 February 2015 at 10:27, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >> Sent: Wednesday, February 04, 2015 11:20 AM >> To: Savolainen, Petri (NSN - FI/Espoo) >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> odp_cpumask_to_str() return chars written or error >> >> On 4 February 2015 at 09:38, Savolainen, Petri (NSN - FI/Espoo) >> <petri.savolainen@nsn.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> >> bounces@lists.linaro.org] On Behalf Of ext Savolainen, Petri (NSN - >> >> FI/Espoo) >> >> Sent: Wednesday, February 04, 2015 10:31 AM >> >> To: ext Ola Liljedahl >> >> Cc: lng-odp@lists.linaro.org >> >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> >> odp_cpumask_to_str() return chars written or error >> >> >> >> > >> diff --git a/include/odp/api/cpumask.h b/include/odp/api/cpumask.h >> >> > >> index 7482899..65969c3 100644 >> >> > >> --- a/include/odp/api/cpumask.h >> >> > >> +++ b/include/odp/api/cpumask.h >> >> > >> @@ -18,12 +18,20 @@ >> >> > >> extern "C" { >> >> > >> #endif >> >> > >> >> >> > >> +#include <sys/types.h> >> >> > >> +#include <odp/config.h> >> >> > >> + >> >> > >> /** @addtogroup odp_scheduler >> >> > >> * CPU mask operations. >> >> > >> * @{ >> >> > >> */ >> >> > >> >> >> > >> - /** >> >> > >> +/** >> >> > >> + * @def ODP_CPUMASK_BUFSIZE >> >> > >> + * Minimum size of output buffer for odp_cpumask_to_str() >> >> > >> + */ >> >> > > >> >> > > ODP_CPUMASK_STRLEN is better. Better to use term string when it's a >> >> > character string. >> But is it not a character string. >> >> The user specifies a buffer and a buffer size. The function writes a >> string into that buffer (if it fits into the buffer). >> The buffer then *contains* a string (which will be shorter that the >> buffer size value specified, e.g. because trailing CPU bits may not be >> set and also the string length does not include the terminating null >> char). But a string and a buffer are not the same thing. >> >> >> >> >> This one is missing from v5. >> >> >> >> Better name it STRLEN when it used for _to_str() and _from_str() >> function >> >> calls. >> I think this is a poor choice of name. The length of a string is the >> number of characters in the string (excluding the terminating null >> char). But we need to pass the buffer size. >> >> >> >> >> char mask_str[ODP_CPUMASK_STRLEN]; >> >> memset(mask_str, 0, sizeof(mask_str)); >> Why is the memset() here? It is not needed. >> >> >> odp_cpumask_to_str(mask, mask_str, sizeof(mask_str)); >> >> odp_cpumask_from_str(mask, mask_str); >> Yes this is how it is done, before my patch and after my patch. >> >> >> >> >> >> >> -Petri >> > >> > >> > Or ODP_CPUMASK_STRLEN_MAX or ODP_CPUMASK_STR_MAX_CHARS ... >> > >> > The point is that, it's defining the max number of chars (not bytes) >> needed for the string output - and it cannot be mixed with the >> odp_cpumask_t (buffer) size. >> odp_cpumask_t has a separate well-defined type (no char bufs or void >> pointers used when you are passing an odp_cpumask_t value), I see no >> risk for mixing it with ODP_CPUMASK_BUFSIZE. > > > If you reserve ODP_CPUMASK_BUFSIZE for the string API. What if we add odp_cpumask_to_u64(), odp_cpumask_to_u8(),odp_cpumask_to_buf(), etc API calls later on? It's better to reserve BUF for those buffers and use STR for strings (no matter if sting initialized or not before the call, output is still a string). Purely hypothetical. One can invent any number of counter arguments, that does not make them realistic or meaningful. What if we add other odp_cpumask string calls that need some (other) type of buffer to hold their strings? But I do want to make the name as easy as possible to understand and try to see your criticism in a constructive light. Perhaps ODP_CPUMASK_STR_BUFSIZE? Buffer size for cpumask strings. The buffer size is still different from the string length just as a buffer is not a string. > > -Petri > > > >
On 4 February 2015 at 11:13, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >> Sent: Wednesday, February 04, 2015 11:50 AM >> To: Savolainen, Petri (NSN - FI/Espoo) >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> odp_cpumask_to_str() return chars written or error >> >> On 4 February 2015 at 10:27, Savolainen, Petri (NSN - FI/Espoo) >> <petri.savolainen@nsn.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >> >> Sent: Wednesday, February 04, 2015 11:20 AM >> >> To: Savolainen, Petri (NSN - FI/Espoo) >> >> Cc: lng-odp@lists.linaro.org >> >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> >> odp_cpumask_to_str() return chars written or error >> >> >> >> On 4 February 2015 at 09:38, Savolainen, Petri (NSN - FI/Espoo) >> >> <petri.savolainen@nsn.com> wrote: >> >> > >> >> > >> >> >> -----Original Message----- >> >> >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> >> >> bounces@lists.linaro.org] On Behalf Of ext Savolainen, Petri (NSN - >> >> >> FI/Espoo) >> >> >> Sent: Wednesday, February 04, 2015 10:31 AM >> >> >> To: ext Ola Liljedahl >> >> >> Cc: lng-odp@lists.linaro.org >> >> >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> >> >> odp_cpumask_to_str() return chars written or error >> >> >> >> >> >> > >> diff --git a/include/odp/api/cpumask.h >> b/include/odp/api/cpumask.h >> >> >> > >> index 7482899..65969c3 100644 >> >> >> > >> --- a/include/odp/api/cpumask.h >> >> >> > >> +++ b/include/odp/api/cpumask.h >> >> >> > >> @@ -18,12 +18,20 @@ >> >> >> > >> extern "C" { >> >> >> > >> #endif >> >> >> > >> >> >> >> > >> +#include <sys/types.h> >> >> >> > >> +#include <odp/config.h> >> >> >> > >> + >> >> >> > >> /** @addtogroup odp_scheduler >> >> >> > >> * CPU mask operations. >> >> >> > >> * @{ >> >> >> > >> */ >> >> >> > >> >> >> >> > >> - /** >> >> >> > >> +/** >> >> >> > >> + * @def ODP_CPUMASK_BUFSIZE >> >> >> > >> + * Minimum size of output buffer for odp_cpumask_to_str() >> >> >> > >> + */ >> >> >> > > >> >> >> > > ODP_CPUMASK_STRLEN is better. Better to use term string when >> it's a >> >> >> > character string. >> >> But is it not a character string. >> >> >> >> The user specifies a buffer and a buffer size. The function writes a >> >> string into that buffer (if it fits into the buffer). >> >> The buffer then *contains* a string (which will be shorter that the >> >> buffer size value specified, e.g. because trailing CPU bits may not be >> >> set and also the string length does not include the terminating null >> >> char). But a string and a buffer are not the same thing. >> >> >> >> >> >> >> >> This one is missing from v5. >> >> >> >> >> >> Better name it STRLEN when it used for _to_str() and _from_str() >> >> function >> >> >> calls. >> >> I think this is a poor choice of name. The length of a string is the >> >> number of characters in the string (excluding the terminating null >> >> char). But we need to pass the buffer size. >> >> >> >> >> >> >> >> char mask_str[ODP_CPUMASK_STRLEN]; >> >> >> memset(mask_str, 0, sizeof(mask_str)); >> >> Why is the memset() here? It is not needed. >> >> >> >> >> odp_cpumask_to_str(mask, mask_str, sizeof(mask_str)); >> >> >> odp_cpumask_from_str(mask, mask_str); >> >> Yes this is how it is done, before my patch and after my patch. >> >> >> >> >> >> >> >> >> >> >> -Petri >> >> > >> >> > >> >> > Or ODP_CPUMASK_STRLEN_MAX or ODP_CPUMASK_STR_MAX_CHARS ... >> >> > >> >> > The point is that, it's defining the max number of chars (not bytes) >> >> needed for the string output - and it cannot be mixed with the >> >> odp_cpumask_t (buffer) size. >> >> odp_cpumask_t has a separate well-defined type (no char bufs or void >> >> pointers used when you are passing an odp_cpumask_t value), I see no >> >> risk for mixing it with ODP_CPUMASK_BUFSIZE. >> > >> > >> > If you reserve ODP_CPUMASK_BUFSIZE for the string API. What if we add >> odp_cpumask_to_u64(), odp_cpumask_to_u8(),odp_cpumask_to_buf(), etc API >> calls later on? It's better to reserve BUF for those buffers and use STR >> for strings (no matter if sting initialized or not before the call, output >> is still a string). >> Purely hypothetical. One can invent any number of counter arguments, >> that does not make them realistic or meaningful. >> What if we add other odp_cpumask string calls that need some (other) >> type of buffer to hold their strings? >> >> But I do want to make the name as easy as possible to understand and >> try to see your criticism in a constructive light. >> Perhaps ODP_CPUMASK_STR_BUFSIZE? Buffer size for cpumask strings. >> >> The buffer size is still different from the string length just as a >> buffer is not a string. >> > > And "hypothetical" seems to be favorite counter-counter argument... Because you are the expert at finding hypothetical counter arguments as soon as anyone else has a different opinion. This happens like ten times every time we have a call. > We already had _to_u64() but removed it from the API. It may have a comeback (in some form), if we need to start packing these masks into messages and send those between devices (e.g. transmit portable cpumask definitions between control and data plane devices). What's wrong with these cpumask strings for that purpose? Variable length yes, I think that is a strength. You don't want the control plane and the protocols to have dependencies on how many CPU's there are in the dataplane. ASCII encoded instead of binary (so taking ~twice the size), is that a problem? If you start by specifying the problem, more people can collaborate on finding the best solution. > > ODP_CPUMASK_STR_SIZE would be shorter. I can think of even shorter names. It doesn't mean shorter names are better. At least you are using SIZE and not LEN. I think we are converging. > > ODP_CPUMASK_STR_XXX, with or without BUF - is understandable to me. > > > -Petri > >
On 4 February 2015 at 09:38, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com> wrote: > > >> -----Original Message----- >> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- >> bounces@lists.linaro.org] On Behalf Of ext Savolainen, Petri (NSN - >> FI/Espoo) >> Sent: Wednesday, February 04, 2015 10:31 AM >> To: ext Ola Liljedahl >> Cc: lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv4 01/18] api: odp_cpumask.h: >> odp_cpumask_to_str() return chars written or error >> >> > >> diff --git a/include/odp/api/cpumask.h b/include/odp/api/cpumask.h >> > >> index 7482899..65969c3 100644 >> > >> --- a/include/odp/api/cpumask.h >> > >> +++ b/include/odp/api/cpumask.h >> > >> @@ -18,12 +18,20 @@ >> > >> extern "C" { >> > >> #endif >> > >> >> > >> +#include <sys/types.h> >> > >> +#include <odp/config.h> >> > >> + >> > >> /** @addtogroup odp_scheduler >> > >> * CPU mask operations. >> > >> * @{ >> > >> */ >> > >> >> > >> - /** >> > >> +/** >> > >> + * @def ODP_CPUMASK_BUFSIZE >> > >> + * Minimum size of output buffer for odp_cpumask_to_str() >> > >> + */ >> > > >> > > ODP_CPUMASK_STRLEN is better. Better to use term string when it's a >> > character string. >> >> This one is missing from v5. >> >> Better name it STRLEN when it used for _to_str() and _from_str() function >> calls. >> >> char mask_str[ODP_CPUMASK_STRLEN]; >> memset(mask_str, 0, sizeof(mask_str)); >> odp_cpumask_to_str(mask, mask_str, sizeof(mask_str)); >> odp_cpumask_from_str(mask, mask_str); >> >> >> -Petri > > > Or ODP_CPUMASK_STRLEN_MAX or ODP_CPUMASK_STR_MAX_CHARS ... > > The point is that, it's defining the max number of chars (not bytes) needed for the string output In C, "byte" means storage unit. A "char" is defined to take one storage unit (sizeof(char)==1). See e.g. C - A Reference Manual; Harbison, Steele Jr. (chapter 6.1.1 in the 4th edition) or http://c0x.coding-guidelines.com/6.5.3.4.html So for an array of char (the odp_cpumask_to_str() uses "char *" to reference this array), there is no practical nor semantic difference between number of chars or number of bytes. We could have added some more words to the description (e.g. "size of output buffer in chars") but I think this would be redundant and potentially confusing (as the size in chars is the default and the output buffer is specified as an array of char). In a case where the output buffer is of some non-char type, it makes sense to specify what type is used for the size if this is not otherwise obvious from the description. > - and it cannot be mixed with the odp_cpumask_t (buffer) size. > > > -Petri > > > >
diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c index 03dab50..2433cb6 100644 --- a/example/generator/odp_generator.c +++ b/example/generator/odp_generator.c @@ -545,7 +545,7 @@ int main(int argc, char *argv[]) int i; odp_shm_t shm; odp_cpumask_t cpumask; - char cpumaskstr[64]; + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; odp_pool_param_t params; /* Init ODP before calling anything else */ @@ -596,7 +596,7 @@ int main(int argc, char *argv[]) * Start mapping thread from CPU #1 */ num_workers = odph_linux_cpumask_default(&cpumask, num_workers); - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); printf("num worker threads: %i\n", num_workers); printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c index 03f2419..869fd3a 100644 --- a/example/ipsec/odp_ipsec.c +++ b/example/ipsec/odp_ipsec.c @@ -1193,7 +1193,7 @@ main(int argc, char *argv[]) int stream_count; odp_shm_t shm; odp_cpumask_t cpumask; - char cpumaskstr[64]; + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; odp_pool_param_t params; /* Init ODP before calling anything else */ @@ -1242,7 +1242,7 @@ main(int argc, char *argv[]) * Start mapping thread from CPU #1 */ num_workers = odph_linux_cpumask_default(&cpumask, num_workers); - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); printf("num worker threads: %i\n", num_workers); printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); diff --git a/example/l2fwd/odp_l2fwd.c b/example/l2fwd/odp_l2fwd.c index 7a520fb..e22ec0c 100644 --- a/example/l2fwd/odp_l2fwd.c +++ b/example/l2fwd/odp_l2fwd.c @@ -294,7 +294,7 @@ int main(int argc, char *argv[]) int num_workers; odp_shm_t shm; odp_cpumask_t cpumask; - char cpumaskstr[64]; + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; odp_pool_param_t params; /* Init ODP before calling anything else */ @@ -336,7 +336,7 @@ int main(int argc, char *argv[]) * Start mapping thread from CPU #1 */ num_workers = odph_linux_cpumask_default(&cpumask, num_workers); - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); printf("num worker threads: %i\n", num_workers); printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c index 28ed98c..e38029a 100644 --- a/example/packet/odp_pktio.c +++ b/example/packet/odp_pktio.c @@ -287,7 +287,7 @@ int main(int argc, char *argv[]) int i; int cpu; odp_cpumask_t cpumask; - char cpumaskstr[64]; + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; odp_pool_param_t params; args = calloc(1, sizeof(args_t)); @@ -324,7 +324,7 @@ int main(int argc, char *argv[]) * Start mapping thread from CPU #1 */ num_workers = odph_linux_cpumask_default(&cpumask, num_workers); - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); printf("num worker threads: %i\n", num_workers); printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); diff --git a/example/timer/odp_timer_test.c b/example/timer/odp_timer_test.c index 0df041f..299ed89 100644 --- a/example/timer/odp_timer_test.c +++ b/example/timer/odp_timer_test.c @@ -317,7 +317,7 @@ int main(int argc, char *argv[]) odp_timer_pool_param_t tparams; odp_timer_pool_info_t tpinfo; odp_cpumask_t cpumask; - char cpumaskstr[64]; + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; printf("\nODP timer example starts\n"); @@ -358,7 +358,7 @@ int main(int argc, char *argv[]) * Start mapping thread from CPU #1 */ num_workers = odph_linux_cpumask_default(&cpumask, num_workers); - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); printf("num worker threads: %i\n", num_workers); printf("first CPU: %i\n", odp_cpumask_first(&cpumask)); diff --git a/include/odp/api/cpumask.h b/include/odp/api/cpumask.h index 7482899..65969c3 100644 --- a/include/odp/api/cpumask.h +++ b/include/odp/api/cpumask.h @@ -18,12 +18,20 @@ extern "C" { #endif +#include <sys/types.h> +#include <odp/config.h> + /** @addtogroup odp_scheduler * CPU mask operations. * @{ */ - /** +/** + * @def ODP_CPUMASK_BUFSIZE + * Minimum size of output buffer for odp_cpumask_to_str() + */ + +/** * Add CPU mask bits from a string * * @param mask CPU mask to modify @@ -33,13 +41,16 @@ extern "C" { void odp_cpumask_from_str(odp_cpumask_t *mask, const char *str); /** - * Write CPU mask as a string of hexadecimal digits + * Format CPU mask as a string of hexadecimal digits + * + * @param mask CPU mask to format + * @param[out] buf Output buffer (use ODP_CPUMASK_BUFSIZE) + * @param bufsz Size of output buffer * - * @param mask CPU mask - * @param str String for output - * @param len Size of string length (incl. ending zero) + * @return number of characters written (including terminating null char) + * @retval <0 on failure (buffer too small) */ -void odp_cpumask_to_str(const odp_cpumask_t *mask, char *str, int len); +ssize_t odp_cpumask_to_str(const odp_cpumask_t *mask, char *buf, ssize_t bufsz); /** * Clear entire mask diff --git a/platform/linux-generic/include/odp/plat/cpumask_types.h b/platform/linux-generic/include/odp/plat/cpumask_types.h index 6d9e129..20626f0 100644 --- a/platform/linux-generic/include/odp/plat/cpumask_types.h +++ b/platform/linux-generic/include/odp/plat/cpumask_types.h @@ -25,6 +25,11 @@ extern "C" { #include <odp/std_types.h> /** + * Minimum size of output buffer for odp_cpumask_to_str() + */ +#define ODP_CPUMASK_BUFSIZE ((ODP_CONFIG_MAX_THREADS + 3) / 4 + 3) + +/** * CPU mask * * Don't access directly, use access functions. diff --git a/platform/linux-generic/odp_cpumask.c b/platform/linux-generic/odp_cpumask.c index 6088ca2..0bae1ce 100644 --- a/platform/linux-generic/odp_cpumask.c +++ b/platform/linux-generic/odp_cpumask.c @@ -8,6 +8,7 @@ #define _GNU_SOURCE #endif #include <sched.h> +#include <sys/types.h> #include <odp/cpumask.h> #include <odp_debug_internal.h> @@ -60,29 +61,31 @@ void odp_cpumask_from_str(odp_cpumask_t *mask, const char *str_in) memcpy(&mask->set, &cpuset, sizeof(cpuset)); } -void odp_cpumask_to_str(const odp_cpumask_t *mask, char *str, int len) +ssize_t odp_cpumask_to_str(const odp_cpumask_t *mask, char *str, ssize_t len) { char *p = str; int cpu = odp_cpumask_last(mask); - int nibbles; + unsigned int nibbles; int value; - /* Quickly handle bad string length or empty mask */ - if (len <= 0) - return; - *str = 0; + /* Handle bad string length, need at least 4 chars for "0x0" and + * terminating null char */ + if (len < 4) + return -1; /* Failure */ + + /* Handle no CPU found */ if (cpu < 0) { - if (len >= 4) - strcpy(str, "0x0"); - return; + strcpy(str, "0x0"); + return strlen(str) + 1; /* Success */ } + /* CPU was found and cpu >= 0 */ - /* Compute number nibbles in cpumask that have bits set */ + /* Compute number of nibbles in cpumask that have bits set */ nibbles = (cpu / 4) + 1; /* Verify minimum space (account for "0x" and termination) */ if (len < (3 + nibbles)) - return; + return -1; /* Failure */ /* Prefix */ *p++ = '0'; @@ -110,6 +113,7 @@ void odp_cpumask_to_str(const odp_cpumask_t *mask, char *str, int len) /* Terminate the string */ *p++ = 0; + return p - str; /* Success */ } void odp_cpumask_zero(odp_cpumask_t *mask) diff --git a/test/api_test/odp_common.c b/test/api_test/odp_common.c index 5f9504c..b5f2d4d 100644 --- a/test/api_test/odp_common.c +++ b/test/api_test/odp_common.c @@ -30,14 +30,14 @@ __thread test_shared_data_t *test_shared_data; /**< pointer to shared data * void odp_print_system_info(void) { odp_cpumask_t cpumask; - char str[32]; + char str[ODP_CPUMASK_BUFSIZE]; memset(str, 1, sizeof(str)); odp_cpumask_zero(&cpumask); odp_cpumask_from_str(&cpumask, "0x1"); - odp_cpumask_to_str(&cpumask, str, sizeof(str)); + (void)odp_cpumask_to_str(&cpumask, str, sizeof(str)); printf("\n"); printf("ODP system info\n"); diff --git a/test/performance/odp_scheduling.c b/test/performance/odp_scheduling.c index 3d7167f..ba13d8e 100644 --- a/test/performance/odp_scheduling.c +++ b/test/performance/odp_scheduling.c @@ -828,7 +828,7 @@ int main(int argc, char *argv[]) int prios; odp_shm_t shm; test_globals_t *globals; - char cpumaskstr[64]; + char cpumaskstr[ODP_CPUMASK_BUFSIZE]; odp_pool_param_t params; printf("\nODP example starts\n\n"); @@ -879,7 +879,7 @@ int main(int argc, char *argv[]) * Start mapping thread from CPU #1 */ num_workers = odph_linux_cpumask_default(&cpumask, num_workers); - odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); + (void)odp_cpumask_to_str(&cpumask, cpumaskstr, sizeof(cpumaskstr)); printf("num worker threads: %i\n", num_workers); printf("first CPU: %i\n", odp_cpumask_first(&cpumask));
Add define ODP_CPUMASK_BUFSIZE for minimum output buffer size for odp_cpumask_to_str(). odp_cpumask_to_str() takes output buffer size as input and returns number of chars written (on success), <0 on failure. Updated the implementation. Updated all usages in example and test programs. Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org> --- (This document/code contribution attached is provided under the terms of agreement LES-LTM-21309) example/generator/odp_generator.c | 4 ++-- example/ipsec/odp_ipsec.c | 4 ++-- example/l2fwd/odp_l2fwd.c | 4 ++-- example/packet/odp_pktio.c | 4 ++-- example/timer/odp_timer_test.c | 4 ++-- include/odp/api/cpumask.h | 23 ++++++++++++++----- .../linux-generic/include/odp/plat/cpumask_types.h | 5 +++++ platform/linux-generic/odp_cpumask.c | 26 +++++++++++++--------- test/api_test/odp_common.c | 4 ++-- test/performance/odp_scheduling.c | 4 ++-- 10 files changed, 51 insertions(+), 31 deletions(-)