Change in libosmocore[master]: add osmo_hexdump_b(), osmo_hexdump_nospc_b(), osmo_hexdump_buf()

2019-01-21 Thread Harald Welte
Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/12658 )

Change subject: add osmo_hexdump_b(), osmo_hexdump_nospc_b(), osmo_hexdump_buf()
..


Patch Set 3: Code-Review+1

I'm a bit skeptical about those "second static buffer" functions.  If we, in 
general, added functions for a user-supplied bufer and one for a static buffer, 
it should be enough.  Then a user who needs two buffers can put one buffer on 
his stack... and a user who needs three can do the same, without adding a _c() 
variant...


--
To view, visit https://gerrit.osmocom.org/12658
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I590595567b218b24e53c9eb1fd8736c0324d371d
Gerrit-Change-Number: 12658
Gerrit-PatchSet: 3
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Comment-Date: Mon, 21 Jan 2019 18:50:04 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes


Change in libosmocore[master]: add osmo_hexdump_b(), osmo_hexdump_nospc_b(), osmo_hexdump_buf()

2019-01-20 Thread Neels Hofmeyr
Neels Hofmeyr has uploaded this change for review. ( 
https://gerrit.osmocom.org/12658


Change subject: add osmo_hexdump_b(), osmo_hexdump_nospc_b(), osmo_hexdump_buf()
..

add osmo_hexdump_b(), osmo_hexdump_nospc_b(), osmo_hexdump_buf()

Add osmo_hexdump_b() and osmo_hexdump_nospc_b() to use a second static buffer,
allowing more than one hexdump per printf()-like call.

Add osmo_hexdump_buf() as an all-purpose hexdump function, which all other
osmo_hexdump_*() implementations now call. It absorbs the static
_osmo_hexdump(). Add tests for osmo_hexdump_buf().

Rationale: recently during patch review, a situation came up where two hexdumps
in a single printf would have been useful. Now I've faced a similar situation
again, in ongoing development. So I decided it is time to provide this API.

Naming: before, I named functions that use a secondary string buffer with a '2'
suffix, like osmo_plmn_name() and osmo_plmn_name2(). This time, I decided to
use a '_b' suffix, osmo_hexdump_b(), instead. The reason is, by now I think
that '2' is a bad choice for secondary-buffer functions: the '2' suffix is
already used for introducing a newer API version of function signatures. If we,
for example, introduce an osmo_hexdump() that has no final delimiter after the
last byte, that would qualify for osmo_hexdump2(); and that would confuse with
the implementation simply using a secondary buffer. Sometimes during code
review, I assume that the existence of a foo2() function means the patch
submitter should use foo2() instead of foo(), and am annoyed by my previous
choice of overloading the '2' suffix with a secondary meaning besides "this is
newer API".

Change-Id: I590595567b218b24e53c9eb1fd8736c0324d371d
---
M include/osmocom/core/utils.h
M src/utils.c
M tests/utils/utils_test.c
M tests/utils/utils_test.ok
4 files changed, 186 insertions(+), 11 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/58/12658/1

diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h
index 987080e..ffe6cdd 100644
--- a/include/osmocom/core/utils.h
+++ b/include/osmocom/core/utils.h
@@ -55,7 +55,12 @@

 char *osmo_ubit_dump(const uint8_t *bits, unsigned int len);
 char *osmo_hexdump(const unsigned char *buf, int len);
+char *osmo_hexdump_b(const unsigned char *buf, int len);
 char *osmo_hexdump_nospc(const unsigned char *buf, int len);
+char *osmo_hexdump_nospc_b(const unsigned char *buf, int len);
+char *osmo_hexdump_buf(char *out_buf, size_t out_buf_size, const unsigned char 
*buf, int len, const char *delim,
+  bool delim_after_last);
+
 char *osmo_osmo_hexdump_nospc(const unsigned char *buf, int len) 
__attribute__((__deprecated__));

 #define osmo_static_assert(exp, name) typedef int dummy##name [(exp) ? 1 : -1] 
__attribute__((__unused__));
diff --git a/src/utils.c b/src/utils.c
index d1da4fa..06c2f8a 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -217,32 +217,58 @@
 }

 static char hexd_buff[4096];
+static char hexd_buff2[4096];
 static const char hex_chars[] = "0123456789abcdef";

-static char *_osmo_hexdump(const unsigned char *buf, int len, char *delim)
+/*! Convert binary sequence to hexadecimal ASCII string.
+ *  \param[out] out_buf  Output buffer to write the resulting string to.
+ *  \param[in] out_buf_size  sizeof(out_buf).
+ *  \param[in] buf  Input buffer, pointer to sequence of bytes.
+ *  \param[in] len  Length of input buf in number of bytes.
+ *  \param[in] delim  String to separate each byte; NULL or "" for no delim.
+ *  \param[in] delim_after_last  If true, end the string in delim (true: 
"1a:ef:d9:", false: "1a:ef:d9");
+ *   if out_buf has insufficient space, the string 
will always end in a delim.
+ *  \returns out_buf, containing a zero-terminated string, or NULL if out_buf 
== NULL or out_buf_size < 1.
+ *
+ * This function will print a sequence of bytes as hexadecimal numbers, adding 
one delim between each byte (e.g. for
+ * delim passed as ":", return a string like "1a:ef:d9").
+ *
+ * The delim_after_last argument exists to be able to exactly show the 
original osmo_hexdump() behavior, which always
+ * ends the string with a delimiter.
+ */
+char *osmo_hexdump_buf(char *out_buf, size_t out_buf_size, const unsigned char 
*buf, int len, const char *delim,
+  bool delim_after_last)
 {
int i;
-   char *cur = hexd_buff;
+   char *cur = out_buf;
+   size_t delim_len;
 
-   hexd_buff[0] = 0;
+   if (!out_buf || !out_buf_size)
+   return NULL;
+
+   delim = delim ? : "";
+   delim_len = strlen(delim);
+
for (i = 0; i < len; i++) {
const char *delimp = delim;
-   int len_remain = sizeof(hexd_buff) - (cur - hexd_buff);
-   if (len_remain < 3)
+   int len_remain = out_buf_size - (cur - out_buf) - 1;
+   if (len_remain < (2 +